| Message ID | 20220519051415.1198248-1-vladimir.zapolskiy@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Headers | show |
| Series | [v2] media: camss: Allocate camss struct as a managed device resource | expand |
Robert, Vladimir, On 5/19/22 07:14, Vladimir Zapolskiy wrote: > The change simplifies driver's probe and remove functions, no functional > change is intended. > > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> > --- > Changes from v1 to v2: > * rebased on top of media/master > > drivers/media/platform/qcom/camss/camss.c | 33 +++++++---------------- > 1 file changed, 10 insertions(+), 23 deletions(-) > > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c > index 79ad82e233cb..2055233af101 100644 > --- a/drivers/media/platform/qcom/camss/camss.c > +++ b/drivers/media/platform/qcom/camss/camss.c > @@ -1529,7 +1529,7 @@ static int camss_probe(struct platform_device *pdev) > struct camss *camss; > int num_subdevs, ret; > > - camss = kzalloc(sizeof(*camss), GFP_KERNEL); > + camss = devm_kzalloc(dev, sizeof(*camss), GFP_KERNEL); In general it is not a good idea to use devm_*alloc. The problem is that if a device instance is forcibly unbound, then all the memory allocated with devm_ is immediately freed. But if an application still has a file handle to a device open, then it can still use that memory. Now in this case the driver is already using devm_kcalloc, so it doesn't matter, but it is something to keep in mind. In general, hotpluggable devices cannot use devm_*alloc. In general, my recommendation is to not use devm_*alloc for this, but since it is already in use in this driver, it's better to use devm_*alloc consistently. Or, alternatively, not use it all. That's up to Robert. This is just as background information. Regards, Hans > if (!camss) > return -ENOMEM; > > @@ -1567,39 +1567,30 @@ static int camss_probe(struct platform_device *pdev) > camss->csid_num = 4; > camss->vfe_num = 4; > } else { > - ret = -EINVAL; > - goto err_free; > + return -EINVAL; > } > > camss->csiphy = devm_kcalloc(dev, camss->csiphy_num, > sizeof(*camss->csiphy), GFP_KERNEL); > - if (!camss->csiphy) { > - ret = -ENOMEM; > - goto err_free; > - } > + if (!camss->csiphy) > + return -ENOMEM; > > camss->csid = devm_kcalloc(dev, camss->csid_num, sizeof(*camss->csid), > GFP_KERNEL); > - if (!camss->csid) { > - ret = -ENOMEM; > - goto err_free; > - } > + if (!camss->csid) > + return -ENOMEM; > > if (camss->version == CAMSS_8x16 || > camss->version == CAMSS_8x96) { > camss->ispif = devm_kcalloc(dev, 1, sizeof(*camss->ispif), GFP_KERNEL); > - if (!camss->ispif) { > - ret = -ENOMEM; > - goto err_free; > - } > + if (!camss->ispif) > + return -ENOMEM; > } > > camss->vfe = devm_kcalloc(dev, camss->vfe_num, sizeof(*camss->vfe), > GFP_KERNEL); > - if (!camss->vfe) { > - ret = -ENOMEM; > - goto err_free; > - } > + if (!camss->vfe) > + return -ENOMEM; > > v4l2_async_nf_init(&camss->notifier); > > @@ -1681,8 +1672,6 @@ static int camss_probe(struct platform_device *pdev) > v4l2_device_unregister(&camss->v4l2_dev); > err_cleanup: > v4l2_async_nf_cleanup(&camss->notifier); > -err_free: > - kfree(camss); > > return ret; > } > @@ -1709,8 +1698,6 @@ void camss_delete(struct camss *camss) > device_link_del(camss->genpd_link[i]); > dev_pm_domain_detach(camss->genpd[i], true); > } > - > - kfree(camss); > } > > /*
On Thu, 19 May 2022 at 09:16, Hans Verkuil <hverkuil@xs4all.nl> wrote: > > Robert, Vladimir, > > On 5/19/22 07:14, Vladimir Zapolskiy wrote: > > The change simplifies driver's probe and remove functions, no functional > > change is intended. > > > > Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> > > --- > > Changes from v1 to v2: > > * rebased on top of media/master > > > > drivers/media/platform/qcom/camss/camss.c | 33 +++++++---------------- > > 1 file changed, 10 insertions(+), 23 deletions(-) > > > > diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c > > index 79ad82e233cb..2055233af101 100644 > > --- a/drivers/media/platform/qcom/camss/camss.c > > +++ b/drivers/media/platform/qcom/camss/camss.c > > @@ -1529,7 +1529,7 @@ static int camss_probe(struct platform_device *pdev) > > struct camss *camss; > > int num_subdevs, ret; > > > > - camss = kzalloc(sizeof(*camss), GFP_KERNEL); > > + camss = devm_kzalloc(dev, sizeof(*camss), GFP_KERNEL); > > In general it is not a good idea to use devm_*alloc. The problem is that if a > device instance is forcibly unbound, then all the memory allocated with devm_ > is immediately freed. But if an application still has a file handle to a device > open, then it can still use that memory. > > Now in this case the driver is already using devm_kcalloc, so it doesn't matter, > but it is something to keep in mind. In general, hotpluggable devices cannot use > devm_*alloc. > > In general, my recommendation is to not use devm_*alloc for this, but since it > is already in use in this driver, it's better to use devm_*alloc consistently. > Or, alternatively, not use it all. That's up to Robert. > > This is just as background information. Thanks for explaining the nuances Hans, that's very helpful. I think this patch is ok, since it doesn't make the situation any worse, and that CSI camera sensors are very likely to be hotplugged. > > Regards, > > Hans > > > if (!camss) > > return -ENOMEM; > > > > @@ -1567,39 +1567,30 @@ static int camss_probe(struct platform_device *pdev) > > camss->csid_num = 4; > > camss->vfe_num = 4; > > } else { > > - ret = -EINVAL; > > - goto err_free; > > + return -EINVAL; > > } > > > > camss->csiphy = devm_kcalloc(dev, camss->csiphy_num, > > sizeof(*camss->csiphy), GFP_KERNEL); > > - if (!camss->csiphy) { > > - ret = -ENOMEM; > > - goto err_free; > > - } > > + if (!camss->csiphy) > > + return -ENOMEM; > > > > camss->csid = devm_kcalloc(dev, camss->csid_num, sizeof(*camss->csid), > > GFP_KERNEL); > > - if (!camss->csid) { > > - ret = -ENOMEM; > > - goto err_free; > > - } > > + if (!camss->csid) > > + return -ENOMEM; > > > > if (camss->version == CAMSS_8x16 || > > camss->version == CAMSS_8x96) { > > camss->ispif = devm_kcalloc(dev, 1, sizeof(*camss->ispif), GFP_KERNEL); > > - if (!camss->ispif) { > > - ret = -ENOMEM; > > - goto err_free; > > - } > > + if (!camss->ispif) > > + return -ENOMEM; > > } > > > > camss->vfe = devm_kcalloc(dev, camss->vfe_num, sizeof(*camss->vfe), > > GFP_KERNEL); > > - if (!camss->vfe) { > > - ret = -ENOMEM; > > - goto err_free; > > - } > > + if (!camss->vfe) > > + return -ENOMEM; > > > > v4l2_async_nf_init(&camss->notifier); > > > > @@ -1681,8 +1672,6 @@ static int camss_probe(struct platform_device *pdev) > > v4l2_device_unregister(&camss->v4l2_dev); > > err_cleanup: > > v4l2_async_nf_cleanup(&camss->notifier); > > -err_free: > > - kfree(camss); > > > > return ret; > > } > > @@ -1709,8 +1698,6 @@ void camss_delete(struct camss *camss) > > device_link_del(camss->genpd_link[i]); > > dev_pm_domain_detach(camss->genpd[i], true); > > } > > - > > - kfree(camss); > > } > > > > /* Reviewed-by: Robert Foss <robert.foss@linaro.org>
diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c index 79ad82e233cb..2055233af101 100644 --- a/drivers/media/platform/qcom/camss/camss.c +++ b/drivers/media/platform/qcom/camss/camss.c @@ -1529,7 +1529,7 @@ static int camss_probe(struct platform_device *pdev) struct camss *camss; int num_subdevs, ret; - camss = kzalloc(sizeof(*camss), GFP_KERNEL); + camss = devm_kzalloc(dev, sizeof(*camss), GFP_KERNEL); if (!camss) return -ENOMEM; @@ -1567,39 +1567,30 @@ static int camss_probe(struct platform_device *pdev) camss->csid_num = 4; camss->vfe_num = 4; } else { - ret = -EINVAL; - goto err_free; + return -EINVAL; } camss->csiphy = devm_kcalloc(dev, camss->csiphy_num, sizeof(*camss->csiphy), GFP_KERNEL); - if (!camss->csiphy) { - ret = -ENOMEM; - goto err_free; - } + if (!camss->csiphy) + return -ENOMEM; camss->csid = devm_kcalloc(dev, camss->csid_num, sizeof(*camss->csid), GFP_KERNEL); - if (!camss->csid) { - ret = -ENOMEM; - goto err_free; - } + if (!camss->csid) + return -ENOMEM; if (camss->version == CAMSS_8x16 || camss->version == CAMSS_8x96) { camss->ispif = devm_kcalloc(dev, 1, sizeof(*camss->ispif), GFP_KERNEL); - if (!camss->ispif) { - ret = -ENOMEM; - goto err_free; - } + if (!camss->ispif) + return -ENOMEM; } camss->vfe = devm_kcalloc(dev, camss->vfe_num, sizeof(*camss->vfe), GFP_KERNEL); - if (!camss->vfe) { - ret = -ENOMEM; - goto err_free; - } + if (!camss->vfe) + return -ENOMEM; v4l2_async_nf_init(&camss->notifier); @@ -1681,8 +1672,6 @@ static int camss_probe(struct platform_device *pdev) v4l2_device_unregister(&camss->v4l2_dev); err_cleanup: v4l2_async_nf_cleanup(&camss->notifier); -err_free: - kfree(camss); return ret; } @@ -1709,8 +1698,6 @@ void camss_delete(struct camss *camss) device_link_del(camss->genpd_link[i]); dev_pm_domain_detach(camss->genpd[i], true); } - - kfree(camss); } /*
The change simplifies driver's probe and remove functions, no functional change is intended. Signed-off-by: Vladimir Zapolskiy <vladimir.zapolskiy@linaro.org> --- Changes from v1 to v2: * rebased on top of media/master drivers/media/platform/qcom/camss/camss.c | 33 +++++++---------------- 1 file changed, 10 insertions(+), 23 deletions(-)