| Message ID | 20230531-rpm-rproc-v1-9-e0a3b6de1f14@gerhold.net (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add dedicated device tree node for RPM processor/subsystem | expand |
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 >
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
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 --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);
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(-)