| Message ID | 20170315114357.14446-1-j.neuschaefer@gmx.net (mailing list archive) |
|---|---|
| State | Rejected, archived |
| Delegated to: | Andy Gross |
| Headers | show |
On Wed, Mar 15, 2017 at 12:43:56PM +0100, Jonathan Neuschäfer wrote: [...] > - TODO: Reading qcom_smsm_probe, I noticed memory leaks in error paths: > smsm, smsm->entries, etc. are allocated (with devm_kzalloc), but not > freed when the function returns early. This should be addressed at > some point (in a separate patch). This is not actually true (see Documentation/driver-model/devres.txt). Sorry for the noise. Jonathan Neuschäfer
On Wed 15 Mar 04:43 PDT 2017, Jonathan Neusch?fer wrote: > If qcom_smem_get or qcom_smem_alloc return -EPROBE_DEFER, let the caller > the caller handle it, instead of treating it as an error. > > Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> > > --- > v1: > - TODO: Reading qcom_smsm_probe, I noticed memory leaks in error paths: > smsm, smsm->entries, etc. are allocated (with devm_kzalloc), but not > freed when the function returns early. This should be addressed at > some point (in a separate patch). > --- > drivers/soc/qcom/smsm.c | 8 ++++++-- > 1 file changed, 6 insertions(+), 2 deletions(-) > > diff --git a/drivers/soc/qcom/smsm.c b/drivers/soc/qcom/smsm.c > index d0337b2a71c8..3918645e5708 100644 > --- a/drivers/soc/qcom/smsm.c > +++ b/drivers/soc/qcom/smsm.c > @@ -439,7 +439,9 @@ static int smsm_get_size_info(struct qcom_smsm *smsm) > } *info; > > info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_SMSM_SIZE_INFO, &size); > - if (PTR_ERR(info) == -ENOENT || size != sizeof(*info)) { > + if (PTR_ERR(info) == -EPROBE_DEFER) { > + return PTR_ERR(info); > + } else if (PTR_ERR(info) == -ENOENT || size != sizeof(*info)) { The following elseif was supposed to take care of this case, but I clearly screwed this up. Rather than adding a special case for EPROBE_DEFER before the two checks and then fix up the original expression to make errors fall back to the original else, I think you should rearrange the conditionals. Probably better to write it like this instead: if (IS_ERR(info) && PTR_ERR(info) != -ENOENT) { if (PTR_ERR(info) != -EPROBE_DEFER) dev_err(smsm->dev, "unable to retrieve smsm size info\n"); return PTR_ERR(info); } else if (IS_ERR(info) || size != sizeof(*info)) { dev_warn(smsm->dev, "no smsm size info, using defaults\n"); smsm->num_entries = SMSM_DEFAULT_NUM_ENTRIES; smsm->num_hosts = SMSM_DEFAULT_NUM_HOSTS; return 0; } > dev_warn(smsm->dev, "no smsm size info, using defaults\n"); > smsm->num_entries = SMSM_DEFAULT_NUM_ENTRIES; > smsm->num_hosts = SMSM_DEFAULT_NUM_HOSTS; > @@ -515,7 +517,9 @@ static int qcom_smsm_probe(struct platform_device *pdev) > /* Acquire the main SMSM state vector */ > ret = qcom_smem_alloc(QCOM_SMEM_HOST_ANY, SMEM_SMSM_SHARED_STATE, > smsm->num_entries * sizeof(u32)); > - if (ret < 0 && ret != -EEXIST) { > + if (ret == -EPROBE_DEFER) { > + return ret; > + } else if (ret < 0 && ret != -EEXIST) { The idiomatic way to write this is: if (ret < 0 && ret != -EEXIST) { if (ret != -EPROBE_DEFER) dev_err(); return ret; } However, for us to reach this point in smsm_probe() the above qcom_smem_get() must have returned successfully, i.e. we have SMEM in place so there's no need to handle this case specifically. > dev_err(&pdev->dev, "unable to allocate shared state entry\n"); > return ret; > } Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 27, 2017 at 11:18:29PM -0700, Bjorn Andersson wrote: > On Wed 15 Mar 04:43 PDT 2017, Jonathan Neusch?fer wrote: [...] > > info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_SMSM_SIZE_INFO, &size); > > - if (PTR_ERR(info) == -ENOENT || size != sizeof(*info)) { > > + if (PTR_ERR(info) == -EPROBE_DEFER) { > > + return PTR_ERR(info); > > + } else if (PTR_ERR(info) == -ENOENT || size != sizeof(*info)) { > > The following elseif was supposed to take care of this case, but I > clearly screwed this up. > > Rather than adding a special case for EPROBE_DEFER before the two checks > and then fix up the original expression to make errors fall back to the > original else, I think you should rearrange the conditionals. > > Probably better to write it like this instead: > > if (IS_ERR(info) && PTR_ERR(info) != -ENOENT) { > if (PTR_ERR(info) != -EPROBE_DEFER) > dev_err(smsm->dev, "unable to retrieve smsm size info\n"); > return PTR_ERR(info); > } else if (IS_ERR(info) || size != sizeof(*info)) { > dev_warn(smsm->dev, "no smsm size info, using defaults\n"); > smsm->num_entries = SMSM_DEFAULT_NUM_ENTRIES; > smsm->num_hosts = SMSM_DEFAULT_NUM_HOSTS; > return 0; > } Indeed, this looks better. And it also obsoletes my patch 2/2, which is nice. > > dev_warn(smsm->dev, "no smsm size info, using defaults\n"); > > smsm->num_entries = SMSM_DEFAULT_NUM_ENTRIES; > > smsm->num_hosts = SMSM_DEFAULT_NUM_HOSTS; > > @@ -515,7 +517,9 @@ static int qcom_smsm_probe(struct platform_device *pdev) > > /* Acquire the main SMSM state vector */ > > ret = qcom_smem_alloc(QCOM_SMEM_HOST_ANY, SMEM_SMSM_SHARED_STATE, > > smsm->num_entries * sizeof(u32)); > > - if (ret < 0 && ret != -EEXIST) { > > + if (ret == -EPROBE_DEFER) { > > + return ret; > > + } else if (ret < 0 && ret != -EEXIST) { > > The idiomatic way to write this is: > > if (ret < 0 && ret != -EEXIST) { > if (ret != -EPROBE_DEFER) > dev_err(); > return ret; > } > > However, for us to reach this point in smsm_probe() the above > qcom_smem_get() must have returned successfully, i.e. we have SMEM in > place so there's no need to handle this case specifically. I came to the same conclusion but wasn't sure. I'll drop this part from my patch. I'll send a v2 of this series, although after applying your suggestions, I can't claim much originality anymore. Thanks for the review, Jonathan Neuschäfer
On Mon 03 Apr 19:38 PDT 2017, Jonathan Neusch?fer wrote: > On Mon, Mar 27, 2017 at 11:18:29PM -0700, Bjorn Andersson wrote: [..] > > However, for us to reach this point in smsm_probe() the above > > qcom_smem_get() must have returned successfully, i.e. we have SMEM in > > place so there's no need to handle this case specifically. > > I came to the same conclusion but wasn't sure. I'll drop this part from > my patch. > > I'll send a v2 of this series, although after applying your suggestions, > I can't claim much originality anymore. > Don't worry about it. Looking forward to version 2. Regards, Bjorn -- To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/soc/qcom/smsm.c b/drivers/soc/qcom/smsm.c index d0337b2a71c8..3918645e5708 100644 --- a/drivers/soc/qcom/smsm.c +++ b/drivers/soc/qcom/smsm.c @@ -439,7 +439,9 @@ static int smsm_get_size_info(struct qcom_smsm *smsm) } *info; info = qcom_smem_get(QCOM_SMEM_HOST_ANY, SMEM_SMSM_SIZE_INFO, &size); - if (PTR_ERR(info) == -ENOENT || size != sizeof(*info)) { + if (PTR_ERR(info) == -EPROBE_DEFER) { + return PTR_ERR(info); + } else if (PTR_ERR(info) == -ENOENT || size != sizeof(*info)) { dev_warn(smsm->dev, "no smsm size info, using defaults\n"); smsm->num_entries = SMSM_DEFAULT_NUM_ENTRIES; smsm->num_hosts = SMSM_DEFAULT_NUM_HOSTS; @@ -515,7 +517,9 @@ static int qcom_smsm_probe(struct platform_device *pdev) /* Acquire the main SMSM state vector */ ret = qcom_smem_alloc(QCOM_SMEM_HOST_ANY, SMEM_SMSM_SHARED_STATE, smsm->num_entries * sizeof(u32)); - if (ret < 0 && ret != -EEXIST) { + if (ret == -EPROBE_DEFER) { + return ret; + } else if (ret < 0 && ret != -EEXIST) { dev_err(&pdev->dev, "unable to allocate shared state entry\n"); return ret; }
If qcom_smem_get or qcom_smem_alloc return -EPROBE_DEFER, let the caller the caller handle it, instead of treating it as an error. Signed-off-by: Jonathan Neuschäfer <j.neuschaefer@gmx.net> --- v1: - TODO: Reading qcom_smsm_probe, I noticed memory leaks in error paths: smsm, smsm->entries, etc. are allocated (with devm_kzalloc), but not freed when the function returns early. This should be addressed at some point (in a separate patch). --- drivers/soc/qcom/smsm.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)