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

[Stephan Gerhold]

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@xxxxxxxxxxx>
> > ---
> >  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