diff mbox series

[09/14] rpmsg: qcom_smd: Use qcom_smem_is_available()

Message ID 20230531-rpm-rproc-v1-9-e0a3b6de1f14@gerhold.net
State New
Headers
Series Add dedicated device tree node for RPM processor/subsystem |

Commit Message

Stephan Gerhold June 5, 2023, 7:08 a.m. UTC 
  Rather than looking up a dummy item from SMEM, use the new
qcom_smem_is_available() function to make the code more clear
(and reduce the overhead slightly).

Add the same check to qcom_smd_register_edge() as well to ensure that
it only succeeds if SMEM is already available - if a driver calls the
function and SMEM is not available yet then the initial state will be
read incorrectly and the RPMSG devices might never become available.

Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
---
 drivers/rpmsg/qcom_smd.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Konrad Dybcio June 5, 2023, 6:56 p.m. UTC | #1 
On 5.06.2023 09:08, Stephan Gerhold wrote:
> Rather than looking up a dummy item from SMEM, use the new
> qcom_smem_is_available() function to make the code more clear
> (and reduce the overhead slightly).
> 
> Add the same check to qcom_smd_register_edge() as well to ensure that
> it only succeeds if SMEM is already available - if a driver calls the
> function and SMEM is not available yet then the initial state will be
> read incorrectly and the RPMSG devices might never become available.
> 
> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> ---
>  drivers/rpmsg/qcom_smd.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
> index 7b9c298aa491..43f601c84b4f 100644
> --- a/drivers/rpmsg/qcom_smd.c
> +++ b/drivers/rpmsg/qcom_smd.c
> @@ -1479,6 +1479,9 @@ struct qcom_smd_edge *qcom_smd_register_edge(struct device *parent,
>  	struct qcom_smd_edge *edge;
>  	int ret;
>  
> +	if (!qcom_smem_is_available())
> +		return ERR_PTR(-EPROBE_DEFER);
> +
>  	edge = kzalloc(sizeof(*edge), GFP_KERNEL);
>  	if (!edge)
>  		return ERR_PTR(-ENOMEM);
> @@ -1553,12 +1556,9 @@ EXPORT_SYMBOL(qcom_smd_unregister_edge);
>  static int qcom_smd_probe(struct platform_device *pdev)
>  {
>  	struct device_node *node;
> -	void *p;
>  
> -	/* Wait for smem */
> -	p = qcom_smem_get(QCOM_SMEM_HOST_ANY, smem_items[0].alloc_tbl_id, NULL);
> -	if (PTR_ERR(p) == -EPROBE_DEFER)
> -		return PTR_ERR(p);
> +	if (!qcom_smem_is_available())
> +		return -EPROBE_DEFER;
>  
>  	for_each_available_child_of_node(pdev->dev.of_node, node)
>  		qcom_smd_register_edge(&pdev->dev, node);
Hm.. we're not checking the return value here, at all.. Perhaps that
could be improved and we could only check for smem presence inside
qcom_smd_register_edge()?

Konrad
>
Stephan Gerhold June 5, 2023, 7:18 p.m. UTC | #2 
On Mon, Jun 05, 2023 at 08:56:44PM +0200, Konrad Dybcio wrote:
> 
> 
> On 5.06.2023 09:08, Stephan Gerhold wrote:
> > Rather than looking up a dummy item from SMEM, use the new
> > qcom_smem_is_available() function to make the code more clear
> > (and reduce the overhead slightly).
> > 
> > Add the same check to qcom_smd_register_edge() as well to ensure that
> > it only succeeds if SMEM is already available - if a driver calls the
> > function and SMEM is not available yet then the initial state will be
> > read incorrectly and the RPMSG devices might never become available.
> > 
> > Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
> > ---
> >  drivers/rpmsg/qcom_smd.c | 10 +++++-----
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> > 
> > diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
> > index 7b9c298aa491..43f601c84b4f 100644
> > --- a/drivers/rpmsg/qcom_smd.c
> > +++ b/drivers/rpmsg/qcom_smd.c
> > @@ -1479,6 +1479,9 @@ struct qcom_smd_edge *qcom_smd_register_edge(struct device *parent,
> >  	struct qcom_smd_edge *edge;
> >  	int ret;
> >  
> > +	if (!qcom_smem_is_available())
> > +		return ERR_PTR(-EPROBE_DEFER);
> > +
> >  	edge = kzalloc(sizeof(*edge), GFP_KERNEL);
> >  	if (!edge)
> >  		return ERR_PTR(-ENOMEM);
> > @@ -1553,12 +1556,9 @@ EXPORT_SYMBOL(qcom_smd_unregister_edge);
> >  static int qcom_smd_probe(struct platform_device *pdev)
> >  {
> >  	struct device_node *node;
> > -	void *p;
> >  
> > -	/* Wait for smem */
> > -	p = qcom_smem_get(QCOM_SMEM_HOST_ANY, smem_items[0].alloc_tbl_id, NULL);
> > -	if (PTR_ERR(p) == -EPROBE_DEFER)
> > -		return PTR_ERR(p);
> > +	if (!qcom_smem_is_available())
> > +		return -EPROBE_DEFER;
> >  
> >  	for_each_available_child_of_node(pdev->dev.of_node, node)
> >  		qcom_smd_register_edge(&pdev->dev, node);
> Hm.. we're not checking the return value here, at all.. Perhaps that
> could be improved and we could only check for smem presence inside
> qcom_smd_register_edge()?
> 

I think the goal here it to register as many of the edges as possible,
so we wouldn't necessarily want to abort if one of them fails. That's
why it's enough to check for only for a possible -EPROBE_DEFER first.

But more importantly after this series this is legacy code that exists
only for backwards compatibility with older DTBs. The probe function
won't be called for DTBs in mainline anymore. So I think it's not worth
to improve it much anymore. ;)

Thanks,
Stephan
Konrad Dybcio June 5, 2023, 7:45 p.m. UTC | #3 
On 5.06.2023 21:18, Stephan Gerhold wrote:
> On Mon, Jun 05, 2023 at 08:56:44PM +0200, Konrad Dybcio wrote:
>>
>>
>> On 5.06.2023 09:08, Stephan Gerhold wrote:
>>> Rather than looking up a dummy item from SMEM, use the new
>>> qcom_smem_is_available() function to make the code more clear
>>> (and reduce the overhead slightly).
>>>
>>> Add the same check to qcom_smd_register_edge() as well to ensure that
>>> it only succeeds if SMEM is already available - if a driver calls the
>>> function and SMEM is not available yet then the initial state will be
>>> read incorrectly and the RPMSG devices might never become available.
>>>
>>> Signed-off-by: Stephan Gerhold <stephan@gerhold.net>
>>> ---
>>>  drivers/rpmsg/qcom_smd.c | 10 +++++-----
>>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
>>> index 7b9c298aa491..43f601c84b4f 100644
>>> --- a/drivers/rpmsg/qcom_smd.c
>>> +++ b/drivers/rpmsg/qcom_smd.c
>>> @@ -1479,6 +1479,9 @@ struct qcom_smd_edge *qcom_smd_register_edge(struct device *parent,
>>>  	struct qcom_smd_edge *edge;
>>>  	int ret;
>>>  
>>> +	if (!qcom_smem_is_available())
>>> +		return ERR_PTR(-EPROBE_DEFER);
>>> +
>>>  	edge = kzalloc(sizeof(*edge), GFP_KERNEL);
>>>  	if (!edge)
>>>  		return ERR_PTR(-ENOMEM);
>>> @@ -1553,12 +1556,9 @@ EXPORT_SYMBOL(qcom_smd_unregister_edge);
>>>  static int qcom_smd_probe(struct platform_device *pdev)
>>>  {
>>>  	struct device_node *node;
>>> -	void *p;
>>>  
>>> -	/* Wait for smem */
>>> -	p = qcom_smem_get(QCOM_SMEM_HOST_ANY, smem_items[0].alloc_tbl_id, NULL);
>>> -	if (PTR_ERR(p) == -EPROBE_DEFER)
>>> -		return PTR_ERR(p);
>>> +	if (!qcom_smem_is_available())
>>> +		return -EPROBE_DEFER;
>>>  
>>>  	for_each_available_child_of_node(pdev->dev.of_node, node)
>>>  		qcom_smd_register_edge(&pdev->dev, node);
>> Hm.. we're not checking the return value here, at all.. Perhaps that
>> could be improved and we could only check for smem presence inside
>> qcom_smd_register_edge()?
>>
> 
> I think the goal here it to register as many of the edges as possible,
> so we wouldn't necessarily want to abort if one of them fails. That's
> why it's enough to check for only for a possible -EPROBE_DEFER first.
Hm I guess that's the better option, killing the entire platform (no
rpm = no anything) because one edge failed to register would be not
very user friendly..

> 
> But more importantly after this series this is legacy code that exists
> only for backwards compatibility with older DTBs. The probe function
> won't be called for DTBs in mainline anymore. So I think it's not worth
> to improve it much anymore. ;)
Right!

Konrad
> 
> Thanks,
> Stephan
diff mbox series

Patch

diff --git a/drivers/rpmsg/qcom_smd.c b/drivers/rpmsg/qcom_smd.c
index 7b9c298aa491..43f601c84b4f 100644
--- a/drivers/rpmsg/qcom_smd.c
+++ b/drivers/rpmsg/qcom_smd.c
@@ -1479,6 +1479,9 @@  struct qcom_smd_edge *qcom_smd_register_edge(struct device *parent,
 	struct qcom_smd_edge *edge;
 	int ret;
 
+	if (!qcom_smem_is_available())
+		return ERR_PTR(-EPROBE_DEFER);
+
 	edge = kzalloc(sizeof(*edge), GFP_KERNEL);
 	if (!edge)
 		return ERR_PTR(-ENOMEM);
@@ -1553,12 +1556,9 @@  EXPORT_SYMBOL(qcom_smd_unregister_edge);
 static int qcom_smd_probe(struct platform_device *pdev)
 {
 	struct device_node *node;
-	void *p;
 
-	/* Wait for smem */
-	p = qcom_smem_get(QCOM_SMEM_HOST_ANY, smem_items[0].alloc_tbl_id, NULL);
-	if (PTR_ERR(p) == -EPROBE_DEFER)
-		return PTR_ERR(p);
+	if (!qcom_smem_is_available())
+		return -EPROBE_DEFER;
 
 	for_each_available_child_of_node(pdev->dev.of_node, node)
 		qcom_smd_register_edge(&pdev->dev, node);