| Message ID | 1400234045-6022-1-git-send-email-emilgoode@gmail.com |
|---|---|
| State | New |
| Headers | show |
Am 16.05.2014 11:54, schrieb Emil Goode: > If we fail to allocate struct platform_device pdev we > dereference it after the goto label err. > > I have rearranged the error handling a bit to fix the issue > and also make it more clear. > > Signed-off-by: Emil Goode <emilgoode@gmail.com> > --- > v2: Changed to return -ENOMEM instead of ret where possible and > updated the subject line. > > arch/arm/mach-imx/devices/platform-ipu-core.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c > index fc4dd7c..68f2a4a 100644 > --- a/arch/arm/mach-imx/devices/platform-ipu-core.c > +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c > @@ -77,34 +77,38 @@ struct platform_device *__init imx_alloc_mx3_camera( > > pdev = platform_device_alloc("mx3-camera", 0); > if (!pdev) > - goto err; > + return ERR_PTR(-ENOMEM); > > pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); > if (!pdev->dev.dma_mask) > - goto err; > + goto put_pdev; > > *pdev->dev.dma_mask = DMA_BIT_MASK(32); > pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > > ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res)); > if (ret) > - goto err; > + goto free_dma_mask; > > if (pdata) { > struct mx3_camera_pdata *copied_pdata; > > ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); > - if (ret) { > -err: > - kfree(pdev->dev.dma_mask); > - platform_device_put(pdev); > - return ERR_PTR(-ENODEV); > - } > + if (ret) > + goto free_dma_mask; > + > copied_pdata = dev_get_platdata(&pdev->dev); > copied_pdata->dma_dev = &imx_ipu_coredev->dev; the patch is fine, but what use is this copied_pdata ? It scope ends next line ? re, wh > } > > return pdev; > + > +free_dma_mask: > + kfree(pdev->dev.dma_mask); > +put_pdev: > + platform_device_put(pdev); > + > + return ERR_PTR(ret); > } > > struct platform_device *__init imx_add_mx3_sdc_fb(
Hello Walter, On Fri, May 16, 2014 at 12:40:19PM +0200, walter harms wrote: > > > Am 16.05.2014 11:54, schrieb Emil Goode: > > If we fail to allocate struct platform_device pdev we > > dereference it after the goto label err. > > > > I have rearranged the error handling a bit to fix the issue > > and also make it more clear. > > > > Signed-off-by: Emil Goode <emilgoode@gmail.com> > > --- > > v2: Changed to return -ENOMEM instead of ret where possible and > > updated the subject line. > > > > arch/arm/mach-imx/devices/platform-ipu-core.c | 22 +++++++++++++--------- > > 1 file changed, 13 insertions(+), 9 deletions(-) > > > > diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c > > index fc4dd7c..68f2a4a 100644 > > --- a/arch/arm/mach-imx/devices/platform-ipu-core.c > > +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c > > @@ -77,34 +77,38 @@ struct platform_device *__init imx_alloc_mx3_camera( > > > > pdev = platform_device_alloc("mx3-camera", 0); > > if (!pdev) > > - goto err; > > + return ERR_PTR(-ENOMEM); > > > > pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); > > if (!pdev->dev.dma_mask) > > - goto err; > > + goto put_pdev; > > > > *pdev->dev.dma_mask = DMA_BIT_MASK(32); > > pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > > > > ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res)); > > if (ret) > > - goto err; > > + goto free_dma_mask; > > > > if (pdata) { > > struct mx3_camera_pdata *copied_pdata; > > > > ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); > > - if (ret) { > > -err: > > - kfree(pdev->dev.dma_mask); > > - platform_device_put(pdev); > > - return ERR_PTR(-ENODEV); > > - } > > + if (ret) > > + goto free_dma_mask; > > + > > copied_pdata = dev_get_platdata(&pdev->dev); > > copied_pdata->dma_dev = &imx_ipu_coredev->dev; > > > the patch is fine, but what use is this copied_pdata ? > It scope ends next line ? > > re, > wh I also thought that looked a bit odd, but copied_pdata is a temporary pointer to platform_data of the dev struct. dev_get_platdata looks like this: static inline void *dev_get_platdata(const struct device *dev) { return dev->platform_data; } So I believe it's a more compact way of writing: pdev->dev->platform_data->dma_dev = &imx_ipu_coredev->dev; Best regards, Emil Goode > > > } > > > > return pdev; > > + > > +free_dma_mask: > > + kfree(pdev->dev.dma_mask); > > +put_pdev: > > + platform_device_put(pdev); > > + > > + return ERR_PTR(ret); > > } > > > > struct platform_device *__init imx_add_mx3_sdc_fb(
Am 16.05.2014 13:16, schrieb Emil Goode: > Hello Walter, > > On Fri, May 16, 2014 at 12:40:19PM +0200, walter harms wrote: >> >> >> Am 16.05.2014 11:54, schrieb Emil Goode: >>> If we fail to allocate struct platform_device pdev we >>> dereference it after the goto label err. >>> >>> I have rearranged the error handling a bit to fix the issue >>> and also make it more clear. >>> >>> Signed-off-by: Emil Goode <emilgoode@gmail.com> >>> --- >>> v2: Changed to return -ENOMEM instead of ret where possible and >>> updated the subject line. >>> >>> arch/arm/mach-imx/devices/platform-ipu-core.c | 22 +++++++++++++--------- >>> 1 file changed, 13 insertions(+), 9 deletions(-) >>> >>> diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c >>> index fc4dd7c..68f2a4a 100644 >>> --- a/arch/arm/mach-imx/devices/platform-ipu-core.c >>> +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c >>> @@ -77,34 +77,38 @@ struct platform_device *__init imx_alloc_mx3_camera( >>> >>> pdev = platform_device_alloc("mx3-camera", 0); >>> if (!pdev) >>> - goto err; >>> + return ERR_PTR(-ENOMEM); >>> >>> pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); >>> if (!pdev->dev.dma_mask) >>> - goto err; >>> + goto put_pdev; >>> >>> *pdev->dev.dma_mask = DMA_BIT_MASK(32); >>> pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); >>> >>> ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res)); >>> if (ret) >>> - goto err; >>> + goto free_dma_mask; >>> >>> if (pdata) { >>> struct mx3_camera_pdata *copied_pdata; >>> >>> ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); >>> - if (ret) { >>> -err: >>> - kfree(pdev->dev.dma_mask); >>> - platform_device_put(pdev); >>> - return ERR_PTR(-ENODEV); >>> - } >>> + if (ret) >>> + goto free_dma_mask; >>> + >>> copied_pdata = dev_get_platdata(&pdev->dev); >>> copied_pdata->dma_dev = &imx_ipu_coredev->dev; >> >> >> the patch is fine, but what use is this copied_pdata ? >> It scope ends next line ? >> >> re, >> wh > > I also thought that looked a bit odd, but copied_pdata is a temporary > pointer to platform_data of the dev struct. > > dev_get_platdata looks like this: > > static inline void *dev_get_platdata(const struct device *dev) > { > return dev->platform_data; > } > > So I believe it's a more compact way of writing: > > pdev->dev->platform_data->dma_dev = &imx_ipu_coredev->dev; ok i see ... Both ways are horrible to read, but the second is at least understandable. fortunately i am not the maintainer. thx, re, wh > Best regards, > > Emil Goode > >> >>> } >>> >>> return pdev; >>> + >>> +free_dma_mask: >>> + kfree(pdev->dev.dma_mask); >>> +put_pdev: >>> + platform_device_put(pdev); >>> + >>> + return ERR_PTR(ret); >>> } >>> >>> struct platform_device *__init imx_add_mx3_sdc_fb( >
Hello Emil, IMHO the subject is too general. Maybe better use: ARM: imx: fix error handling in ipu device registration On Fri, May 16, 2014 at 11:54:05AM +0200, Emil Goode wrote: > If we fail to allocate struct platform_device pdev we > dereference it after the goto label err. > > I have rearranged the error handling a bit to fix the issue > and also make it more clear. > > Signed-off-by: Emil Goode <emilgoode@gmail.com> > --- > v2: Changed to return -ENOMEM instead of ret where possible and > updated the subject line. > > arch/arm/mach-imx/devices/platform-ipu-core.c | 22 +++++++++++++--------- > 1 file changed, 13 insertions(+), 9 deletions(-) > > diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c > index fc4dd7c..68f2a4a 100644 > --- a/arch/arm/mach-imx/devices/platform-ipu-core.c > +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c > @@ -77,34 +77,38 @@ struct platform_device *__init imx_alloc_mx3_camera( > > pdev = platform_device_alloc("mx3-camera", 0); > if (!pdev) > - goto err; > + return ERR_PTR(-ENOMEM); > > pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); > if (!pdev->dev.dma_mask) > - goto err; > + goto put_pdev; > > *pdev->dev.dma_mask = DMA_BIT_MASK(32); > pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > > ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res)); > if (ret) > - goto err; > + goto free_dma_mask; > > if (pdata) { > struct mx3_camera_pdata *copied_pdata; > > ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); > - if (ret) { > -err: > - kfree(pdev->dev.dma_mask); > - platform_device_put(pdev); > - return ERR_PTR(-ENODEV); > - } > + if (ret) > + goto free_dma_mask; > + > copied_pdata = dev_get_platdata(&pdev->dev); > copied_pdata->dma_dev = &imx_ipu_coredev->dev; > } > > return pdev; > + > +free_dma_mask: > + kfree(pdev->dev.dma_mask); > +put_pdev: > + platform_device_put(pdev); > + > + return ERR_PTR(ret); > } I didn't check if it is easily possible, but converting this file to use platform_device_register_full might simplify it considerably. I'm not sure this fix is critical, because the problem happens if an allocation during boot fails. But still, if you want to get this fix into a stable release, you should simplify it, i.e. don't do the code reorganisations. (Also the "more clear" part seems to be subjective, I like the error handling better as it is now. But that might only be me.) Are you using this code? I thought arch/arm/mach-imx/devices to be dead as it is unused on dt platforms. Best regards Uwe
Hello Walter, On Fri, May 16, 2014 at 01:49:10PM +0200, walter harms wrote: > Am 16.05.2014 13:16, schrieb Emil Goode: > > Hello Walter, > > > > On Fri, May 16, 2014 at 12:40:19PM +0200, walter harms wrote: > >> > >> > >> Am 16.05.2014 11:54, schrieb Emil Goode: > >>> If we fail to allocate struct platform_device pdev we > >>> dereference it after the goto label err. > >>> > >>> I have rearranged the error handling a bit to fix the issue > >>> and also make it more clear. > >>> > >>> Signed-off-by: Emil Goode <emilgoode@gmail.com> > >>> --- > >>> v2: Changed to return -ENOMEM instead of ret where possible and > >>> updated the subject line. > >>> > >>> arch/arm/mach-imx/devices/platform-ipu-core.c | 22 +++++++++++++--------- > >>> 1 file changed, 13 insertions(+), 9 deletions(-) > >>> > >>> diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c > >>> index fc4dd7c..68f2a4a 100644 > >>> --- a/arch/arm/mach-imx/devices/platform-ipu-core.c > >>> +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c > >>> @@ -77,34 +77,38 @@ struct platform_device *__init imx_alloc_mx3_camera( > >>> > >>> pdev = platform_device_alloc("mx3-camera", 0); > >>> if (!pdev) > >>> - goto err; > >>> + return ERR_PTR(-ENOMEM); > >>> > >>> pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); > >>> if (!pdev->dev.dma_mask) > >>> - goto err; > >>> + goto put_pdev; > >>> > >>> *pdev->dev.dma_mask = DMA_BIT_MASK(32); > >>> pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > >>> > >>> ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res)); > >>> if (ret) > >>> - goto err; > >>> + goto free_dma_mask; > >>> > >>> if (pdata) { > >>> struct mx3_camera_pdata *copied_pdata; > >>> > >>> ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); > >>> - if (ret) { > >>> -err: > >>> - kfree(pdev->dev.dma_mask); > >>> - platform_device_put(pdev); > >>> - return ERR_PTR(-ENODEV); > >>> - } > >>> + if (ret) > >>> + goto free_dma_mask; > >>> + > >>> copied_pdata = dev_get_platdata(&pdev->dev); > >>> copied_pdata->dma_dev = &imx_ipu_coredev->dev; > >> > >> > >> the patch is fine, but what use is this copied_pdata ? > >> It scope ends next line ? > >> > >> re, > >> wh > > > > I also thought that looked a bit odd, but copied_pdata is a temporary > > pointer to platform_data of the dev struct. > > > > dev_get_platdata looks like this: > > > > static inline void *dev_get_platdata(const struct device *dev) > > { > > return dev->platform_data; > > } > > > > So I believe it's a more compact way of writing: > > > > pdev->dev->platform_data->dma_dev = &imx_ipu_coredev->dev; It's not about compactness. The dev_get_platdata accessor exists to be used instead of directly accessing dev->platform_data. I admit a comment would be nice ... Anyhow this is all ugly, actually you'd want to have the dma_dev member already fixed when calling platform_device_add_data. But you cannot simply do pdata->dma_dev = &imx_ipu_coredev->dev; ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); because *pdata is const. Best regards Uwe
On Fri, May 16, 2014 at 09:24:51PM +0200, Uwe Kleine-König wrote: > I didn't check if it is easily possible, but converting this file to use > platform_device_register_full might simplify it considerably. In a separate patch, though, please. > > I'm not sure this fix is critical, because the problem happens if an > allocation during boot fails. But still, if you want to get this fix > into a stable release, you should simplify it, i.e. don't do the code > reorganisations. (Also the "more clear" part seems to be subjective, I > like the error handling better as it is now. But that might only be me.) Emil's error handling is done exactly in the correct way... The error path and success path are separate. Unwinding in the reverse order. The label names describe the label locations. Most days I spend hours looking at linux kernel error handling and I can assure you that sensible labels like this are a rare and wonderful gift. It's hard for me to imagine how anyone could defend the original error handling. The label was "err". The error handling was randomly plopped in the middle of the success handling. Whenever I see new "creative" error handling like this it drives me nuts because obviously it's going to be buggy like a swamp picnic. regards, dan carpenter
On Sat, May 17, 2014 at 01:21:08AM +0300, Dan Carpenter wrote: > On Fri, May 16, 2014 at 09:24:51PM +0200, Uwe Kleine-König wrote: > > I didn't check if it is easily possible, but converting this file to use > > platform_device_register_full might simplify it considerably. > > In a separate patch, though, please. Will consider another patch. > > > > > I'm not sure this fix is critical, because the problem happens if an > > allocation during boot fails. But still, if you want to get this fix > > into a stable release, you should simplify it, i.e. don't do the code > > reorganisations. (Also the "more clear" part seems to be subjective, I > > like the error handling better as it is now. But that might only be me.) > > Emil's error handling is done exactly in the correct way... The error > path and success path are separate. Unwinding in the reverse order. > The label names describe the label locations. Most days I spend hours > looking at linux kernel error handling and I can assure you that > sensible labels like this are a rare and wonderful gift. > > It's hard for me to imagine how anyone could defend the original error > handling. The label was "err". The error handling was randomly plopped > in the middle of the success handling. Whenever I see new "creative" > error handling like this it drives me nuts because obviously it's going > to be buggy like a swamp picnic. Thank you for the colorful reply, I guess there is no need for me to further defend my choice of labels :) Best regards, Emil Goode
Hello Uwe, Thank you for the review. On Fri, May 16, 2014 at 09:24:51PM +0200, Uwe Kleine-König wrote: > Hello Emil, > > IMHO the subject is too general. Maybe better use: > > ARM: imx: fix error handling in ipu device registration Agreed, will change this and resend. > > On Fri, May 16, 2014 at 11:54:05AM +0200, Emil Goode wrote: > > If we fail to allocate struct platform_device pdev we > > dereference it after the goto label err. > > > > I have rearranged the error handling a bit to fix the issue > > and also make it more clear. > > > > Signed-off-by: Emil Goode <emilgoode@gmail.com> > > --- > > v2: Changed to return -ENOMEM instead of ret where possible and > > updated the subject line. > > > > arch/arm/mach-imx/devices/platform-ipu-core.c | 22 +++++++++++++--------- > > 1 file changed, 13 insertions(+), 9 deletions(-) > > > > diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c > > index fc4dd7c..68f2a4a 100644 > > --- a/arch/arm/mach-imx/devices/platform-ipu-core.c > > +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c > > @@ -77,34 +77,38 @@ struct platform_device *__init imx_alloc_mx3_camera( > > > > pdev = platform_device_alloc("mx3-camera", 0); > > if (!pdev) > > - goto err; > > + return ERR_PTR(-ENOMEM); > > > > pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); > > if (!pdev->dev.dma_mask) > > - goto err; > > + goto put_pdev; > > > > *pdev->dev.dma_mask = DMA_BIT_MASK(32); > > pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > > > > ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res)); > > if (ret) > > - goto err; > > + goto free_dma_mask; > > > > if (pdata) { > > struct mx3_camera_pdata *copied_pdata; > > > > ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); > > - if (ret) { > > -err: > > - kfree(pdev->dev.dma_mask); > > - platform_device_put(pdev); > > - return ERR_PTR(-ENODEV); > > - } > > + if (ret) > > + goto free_dma_mask; > > + > > copied_pdata = dev_get_platdata(&pdev->dev); > > copied_pdata->dma_dev = &imx_ipu_coredev->dev; > > } > > > > return pdev; > > + > > +free_dma_mask: > > + kfree(pdev->dev.dma_mask); > > +put_pdev: > > + platform_device_put(pdev); > > + > > + return ERR_PTR(ret); > > } > I didn't check if it is easily possible, but converting this file to use > platform_device_register_full might simplify it considerably. Will look into this and consider sending another patch. > > I'm not sure this fix is critical, because the problem happens if an > allocation during boot fails. But still, if you want to get this fix > into a stable release, you should simplify it, i.e. don't do the code > reorganisations. (Also the "more clear" part seems to be subjective, I > like the error handling better as it is now. But that might only be me.) To my knowlege there is no impact on end users and I don't think the bug is critical since, as you say, the allocation happens during boot. Probably it's not necessary to have this fixed in stable kernels. > > Are you using this code? I thought arch/arm/mach-imx/devices to be dead > as it is unused on dt platforms. The bug was found using coccinelle. I'm not using the code and I don't know what the plans are for this file in the transition to device tree. But as long as the file is in the kernel I think it's worth fixing the bug. Best regards, Emil Goode > > Best regards > Uwe > > -- > Pengutronix e.K. | Uwe Kleine-König | > Industrial Linux Solutions | http://www.pengutronix.de/ |
Hello Uwe, On Fri, May 16, 2014 at 09:31:39PM +0200, Uwe Kleine-König wrote: > Hello Walter, > > On Fri, May 16, 2014 at 01:49:10PM +0200, walter harms wrote: > > Am 16.05.2014 13:16, schrieb Emil Goode: > > > Hello Walter, > > > > > > On Fri, May 16, 2014 at 12:40:19PM +0200, walter harms wrote: > > >> > > >> > > >> Am 16.05.2014 11:54, schrieb Emil Goode: > > >>> If we fail to allocate struct platform_device pdev we > > >>> dereference it after the goto label err. > > >>> > > >>> I have rearranged the error handling a bit to fix the issue > > >>> and also make it more clear. > > >>> > > >>> Signed-off-by: Emil Goode <emilgoode@gmail.com> > > >>> --- > > >>> v2: Changed to return -ENOMEM instead of ret where possible and > > >>> updated the subject line. > > >>> > > >>> arch/arm/mach-imx/devices/platform-ipu-core.c | 22 +++++++++++++--------- > > >>> 1 file changed, 13 insertions(+), 9 deletions(-) > > >>> > > >>> diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c > > >>> index fc4dd7c..68f2a4a 100644 > > >>> --- a/arch/arm/mach-imx/devices/platform-ipu-core.c > > >>> +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c > > >>> @@ -77,34 +77,38 @@ struct platform_device *__init imx_alloc_mx3_camera( > > >>> > > >>> pdev = platform_device_alloc("mx3-camera", 0); > > >>> if (!pdev) > > >>> - goto err; > > >>> + return ERR_PTR(-ENOMEM); > > >>> > > >>> pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); > > >>> if (!pdev->dev.dma_mask) > > >>> - goto err; > > >>> + goto put_pdev; > > >>> > > >>> *pdev->dev.dma_mask = DMA_BIT_MASK(32); > > >>> pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > > >>> > > >>> ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res)); > > >>> if (ret) > > >>> - goto err; > > >>> + goto free_dma_mask; > > >>> > > >>> if (pdata) { > > >>> struct mx3_camera_pdata *copied_pdata; > > >>> > > >>> ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); > > >>> - if (ret) { > > >>> -err: > > >>> - kfree(pdev->dev.dma_mask); > > >>> - platform_device_put(pdev); > > >>> - return ERR_PTR(-ENODEV); > > >>> - } > > >>> + if (ret) > > >>> + goto free_dma_mask; > > >>> + > > >>> copied_pdata = dev_get_platdata(&pdev->dev); > > >>> copied_pdata->dma_dev = &imx_ipu_coredev->dev; > > >> > > >> > > >> the patch is fine, but what use is this copied_pdata ? > > >> It scope ends next line ? > > >> > > >> re, > > >> wh > > > > > > I also thought that looked a bit odd, but copied_pdata is a temporary > > > pointer to platform_data of the dev struct. > > > > > > dev_get_platdata looks like this: > > > > > > static inline void *dev_get_platdata(const struct device *dev) > > > { > > > return dev->platform_data; > > > } > > > > > > So I believe it's a more compact way of writing: > > > > > > pdev->dev->platform_data->dma_dev = &imx_ipu_coredev->dev; > It's not about compactness. The dev_get_platdata accessor exists to be > used instead of directly accessing dev->platform_data. I admit a comment > would be nice ... > > Anyhow this is all ugly, actually you'd want to have the dma_dev member > already fixed when calling platform_device_add_data. But you cannot > simply do > > pdata->dma_dev = &imx_ipu_coredev->dev; > ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); > > because *pdata is const. Thank you for the explanation. Regarding the possibility of using platform_device_register_full() to simplify this function. It seem to be possible, the following inline function is available to help with this. imx_add_platform_device_dmamask() Available here: arch/arm/mach-imx/devices/devices-common.h But as you mentioned above we need to allocate a new platform_device struct before we can assign &imx_ipu_coredev->dev to dma_dev, since pdata is const. I guess this assignment could be done after calling imx_add_platform_device_dmamask() but I don't think that makes the code easier to read. I think it's best to resend the current patch. (with updated subject line) Best regards, Emil Goode
Hello Emil, On Sat, May 17, 2014 at 05:35:40PM +0200, Emil Goode wrote: > On Fri, May 16, 2014 at 09:31:39PM +0200, Uwe Kleine-König wrote: > > On Fri, May 16, 2014 at 01:49:10PM +0200, walter harms wrote: > > > Am 16.05.2014 13:16, schrieb Emil Goode: > > > > Hello Walter, > > > > > > > > On Fri, May 16, 2014 at 12:40:19PM +0200, walter harms wrote: > > > >> > > > >> > > > >> Am 16.05.2014 11:54, schrieb Emil Goode: > > > >>> If we fail to allocate struct platform_device pdev we > > > >>> dereference it after the goto label err. > > > >>> > > > >>> I have rearranged the error handling a bit to fix the issue > > > >>> and also make it more clear. > > > >>> > > > >>> Signed-off-by: Emil Goode <emilgoode@gmail.com> > > > >>> --- > > > >>> v2: Changed to return -ENOMEM instead of ret where possible and > > > >>> updated the subject line. > > > >>> > > > >>> arch/arm/mach-imx/devices/platform-ipu-core.c | 22 +++++++++++++--------- > > > >>> 1 file changed, 13 insertions(+), 9 deletions(-) > > > >>> > > > >>> diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c > > > >>> index fc4dd7c..68f2a4a 100644 > > > >>> --- a/arch/arm/mach-imx/devices/platform-ipu-core.c > > > >>> +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c > > > >>> @@ -77,34 +77,38 @@ struct platform_device *__init imx_alloc_mx3_camera( > > > >>> > > > >>> pdev = platform_device_alloc("mx3-camera", 0); > > > >>> if (!pdev) > > > >>> - goto err; > > > >>> + return ERR_PTR(-ENOMEM); > > > >>> > > > >>> pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); > > > >>> if (!pdev->dev.dma_mask) > > > >>> - goto err; > > > >>> + goto put_pdev; > > > >>> > > > >>> *pdev->dev.dma_mask = DMA_BIT_MASK(32); > > > >>> pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > > > >>> > > > >>> ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res)); > > > >>> if (ret) > > > >>> - goto err; > > > >>> + goto free_dma_mask; > > > >>> > > > >>> if (pdata) { > > > >>> struct mx3_camera_pdata *copied_pdata; > > > >>> > > > >>> ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); > > > >>> - if (ret) { > > > >>> -err: > > > >>> - kfree(pdev->dev.dma_mask); > > > >>> - platform_device_put(pdev); > > > >>> - return ERR_PTR(-ENODEV); > > > >>> - } > > > >>> + if (ret) > > > >>> + goto free_dma_mask; > > > >>> + > > > >>> copied_pdata = dev_get_platdata(&pdev->dev); > > > >>> copied_pdata->dma_dev = &imx_ipu_coredev->dev; > > > >> > > > >> > > > >> the patch is fine, but what use is this copied_pdata ? > > > >> It scope ends next line ? > > > >> > > > >> re, > > > >> wh > > > > > > > > I also thought that looked a bit odd, but copied_pdata is a temporary > > > > pointer to platform_data of the dev struct. > > > > > > > > dev_get_platdata looks like this: > > > > > > > > static inline void *dev_get_platdata(const struct device *dev) > > > > { > > > > return dev->platform_data; > > > > } > > > > > > > > So I believe it's a more compact way of writing: > > > > > > > > pdev->dev->platform_data->dma_dev = &imx_ipu_coredev->dev; > > It's not about compactness. The dev_get_platdata accessor exists to be > > used instead of directly accessing dev->platform_data. I admit a comment > > would be nice ... > > > > Anyhow this is all ugly, actually you'd want to have the dma_dev member > > already fixed when calling platform_device_add_data. But you cannot > > simply do > > > > pdata->dma_dev = &imx_ipu_coredev->dev; > > ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); > > > > because *pdata is const. > > Thank you for the explanation. Regarding the possibility of using > platform_device_register_full() to simplify this function. It seem to > be possible, the following inline function is available to help with this. > > imx_add_platform_device_dmamask() I'd prefer to use platform_device_register_full directly (and let the wrapper die). > But as you mentioned above we need to allocate a new platform_device > struct before we can assign &imx_ipu_coredev->dev to dma_dev, since > pdata is const. I guess this assignment could be done after calling > imx_add_platform_device_dmamask() but I don't think that makes the No, that won't work, because after platform_device_register_full returns you must assume that the device is already bound by a driver. And then you must not change platform_data anymore. The only thing that would work is: struct mx3_camera_pdata tmppdata; if (pdata) { tmppdata = *pdata; tmppdata.dma_dev = &imx_ipu_coredev->dev; pdata = &tmppdata; } platform_device_register_full(... pdata ...) Best regards Uwe
Hello Uwe, On Sat, May 17, 2014 at 09:05:46PM +0200, Uwe Kleine-König wrote: > Hello Emil, > > On Sat, May 17, 2014 at 05:35:40PM +0200, Emil Goode wrote: > > On Fri, May 16, 2014 at 09:31:39PM +0200, Uwe Kleine-König wrote: > > > On Fri, May 16, 2014 at 01:49:10PM +0200, walter harms wrote: > > > > Am 16.05.2014 13:16, schrieb Emil Goode: > > > > > Hello Walter, > > > > > > > > > > On Fri, May 16, 2014 at 12:40:19PM +0200, walter harms wrote: > > > > >> > > > > >> > > > > >> Am 16.05.2014 11:54, schrieb Emil Goode: > > > > >>> If we fail to allocate struct platform_device pdev we > > > > >>> dereference it after the goto label err. > > > > >>> > > > > >>> I have rearranged the error handling a bit to fix the issue > > > > >>> and also make it more clear. > > > > >>> > > > > >>> Signed-off-by: Emil Goode <emilgoode@gmail.com> > > > > >>> --- > > > > >>> v2: Changed to return -ENOMEM instead of ret where possible and > > > > >>> updated the subject line. > > > > >>> > > > > >>> arch/arm/mach-imx/devices/platform-ipu-core.c | 22 +++++++++++++--------- > > > > >>> 1 file changed, 13 insertions(+), 9 deletions(-) > > > > >>> > > > > >>> diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c > > > > >>> index fc4dd7c..68f2a4a 100644 > > > > >>> --- a/arch/arm/mach-imx/devices/platform-ipu-core.c > > > > >>> +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c > > > > >>> @@ -77,34 +77,38 @@ struct platform_device *__init imx_alloc_mx3_camera( > > > > >>> > > > > >>> pdev = platform_device_alloc("mx3-camera", 0); > > > > >>> if (!pdev) > > > > >>> - goto err; > > > > >>> + return ERR_PTR(-ENOMEM); > > > > >>> > > > > >>> pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); > > > > >>> if (!pdev->dev.dma_mask) > > > > >>> - goto err; > > > > >>> + goto put_pdev; > > > > >>> > > > > >>> *pdev->dev.dma_mask = DMA_BIT_MASK(32); > > > > >>> pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > > > > >>> > > > > >>> ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res)); > > > > >>> if (ret) > > > > >>> - goto err; > > > > >>> + goto free_dma_mask; > > > > >>> > > > > >>> if (pdata) { > > > > >>> struct mx3_camera_pdata *copied_pdata; > > > > >>> > > > > >>> ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); > > > > >>> - if (ret) { > > > > >>> -err: > > > > >>> - kfree(pdev->dev.dma_mask); > > > > >>> - platform_device_put(pdev); > > > > >>> - return ERR_PTR(-ENODEV); > > > > >>> - } > > > > >>> + if (ret) > > > > >>> + goto free_dma_mask; > > > > >>> + > > > > >>> copied_pdata = dev_get_platdata(&pdev->dev); > > > > >>> copied_pdata->dma_dev = &imx_ipu_coredev->dev; > > > > >> > > > > >> > > > > >> the patch is fine, but what use is this copied_pdata ? > > > > >> It scope ends next line ? > > > > >> > > > > >> re, > > > > >> wh > > > > > > > > > > I also thought that looked a bit odd, but copied_pdata is a temporary > > > > > pointer to platform_data of the dev struct. > > > > > > > > > > dev_get_platdata looks like this: > > > > > > > > > > static inline void *dev_get_platdata(const struct device *dev) > > > > > { > > > > > return dev->platform_data; > > > > > } > > > > > > > > > > So I believe it's a more compact way of writing: > > > > > > > > > > pdev->dev->platform_data->dma_dev = &imx_ipu_coredev->dev; > > > It's not about compactness. The dev_get_platdata accessor exists to be > > > used instead of directly accessing dev->platform_data. I admit a comment > > > would be nice ... > > > > > > Anyhow this is all ugly, actually you'd want to have the dma_dev member > > > already fixed when calling platform_device_add_data. But you cannot > > > simply do > > > > > > pdata->dma_dev = &imx_ipu_coredev->dev; > > > ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); > > > > > > because *pdata is const. > > > > Thank you for the explanation. Regarding the possibility of using > > platform_device_register_full() to simplify this function. It seem to > > be possible, the following inline function is available to help with this. > > > > imx_add_platform_device_dmamask() > I'd prefer to use platform_device_register_full directly (and let the > wrapper die). Ok, will skip the wrapper. > > > But as you mentioned above we need to allocate a new platform_device > > struct before we can assign &imx_ipu_coredev->dev to dma_dev, since > > pdata is const. I guess this assignment could be done after calling > > imx_add_platform_device_dmamask() but I don't think that makes the > No, that won't work, because after platform_device_register_full returns > you must assume that the device is already bound by a driver. And then > you must not change platform_data anymore. I see, thanks for explaining. > > The only thing that would work is: > > struct mx3_camera_pdata tmppdata; > > if (pdata) { > tmppdata = *pdata; > tmppdata.dma_dev = &imx_ipu_coredev->dev; > > pdata = &tmppdata; > } > > platform_device_register_full(... pdata ...) You are right, that would work. Will look at this again tomorrow. Thank you! Best regards, Emil Goode
Hello Uwe, On Sat, May 17, 2014 at 09:05:46PM +0200, Uwe Kleine-König wrote: > Hello Emil, > > On Sat, May 17, 2014 at 05:35:40PM +0200, Emil Goode wrote: > > On Fri, May 16, 2014 at 09:31:39PM +0200, Uwe Kleine-König wrote: > > > On Fri, May 16, 2014 at 01:49:10PM +0200, walter harms wrote: > > > > Am 16.05.2014 13:16, schrieb Emil Goode: > > > > > Hello Walter, > > > > > > > > > > On Fri, May 16, 2014 at 12:40:19PM +0200, walter harms wrote: > > > > >> > > > > >> > > > > >> Am 16.05.2014 11:54, schrieb Emil Goode: > > > > >>> If we fail to allocate struct platform_device pdev we > > > > >>> dereference it after the goto label err. > > > > >>> > > > > >>> I have rearranged the error handling a bit to fix the issue > > > > >>> and also make it more clear. > > > > >>> > > > > >>> Signed-off-by: Emil Goode <emilgoode@gmail.com> > > > > >>> --- > > > > >>> v2: Changed to return -ENOMEM instead of ret where possible and > > > > >>> updated the subject line. > > > > >>> > > > > >>> arch/arm/mach-imx/devices/platform-ipu-core.c | 22 +++++++++++++--------- > > > > >>> 1 file changed, 13 insertions(+), 9 deletions(-) > > > > >>> > > > > >>> diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c > > > > >>> index fc4dd7c..68f2a4a 100644 > > > > >>> --- a/arch/arm/mach-imx/devices/platform-ipu-core.c > > > > >>> +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c > > > > >>> @@ -77,34 +77,38 @@ struct platform_device *__init imx_alloc_mx3_camera( > > > > >>> > > > > >>> pdev = platform_device_alloc("mx3-camera", 0); > > > > >>> if (!pdev) > > > > >>> - goto err; > > > > >>> + return ERR_PTR(-ENOMEM); > > > > >>> > > > > >>> pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); > > > > >>> if (!pdev->dev.dma_mask) > > > > >>> - goto err; > > > > >>> + goto put_pdev; > > > > >>> > > > > >>> *pdev->dev.dma_mask = DMA_BIT_MASK(32); > > > > >>> pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); > > > > >>> > > > > >>> ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res)); > > > > >>> if (ret) > > > > >>> - goto err; > > > > >>> + goto free_dma_mask; > > > > >>> > > > > >>> if (pdata) { > > > > >>> struct mx3_camera_pdata *copied_pdata; > > > > >>> > > > > >>> ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); > > > > >>> - if (ret) { > > > > >>> -err: > > > > >>> - kfree(pdev->dev.dma_mask); > > > > >>> - platform_device_put(pdev); > > > > >>> - return ERR_PTR(-ENODEV); > > > > >>> - } > > > > >>> + if (ret) > > > > >>> + goto free_dma_mask; > > > > >>> + > > > > >>> copied_pdata = dev_get_platdata(&pdev->dev); > > > > >>> copied_pdata->dma_dev = &imx_ipu_coredev->dev; > > > > >> > > > > >> > > > > >> the patch is fine, but what use is this copied_pdata ? > > > > >> It scope ends next line ? > > > > >> > > > > >> re, > > > > >> wh > > > > > > > > > > I also thought that looked a bit odd, but copied_pdata is a temporary > > > > > pointer to platform_data of the dev struct. > > > > > > > > > > dev_get_platdata looks like this: > > > > > > > > > > static inline void *dev_get_platdata(const struct device *dev) > > > > > { > > > > > return dev->platform_data; > > > > > } > > > > > > > > > > So I believe it's a more compact way of writing: > > > > > > > > > > pdev->dev->platform_data->dma_dev = &imx_ipu_coredev->dev; > > > It's not about compactness. The dev_get_platdata accessor exists to be > > > used instead of directly accessing dev->platform_data. I admit a comment > > > would be nice ... > > > > > > Anyhow this is all ugly, actually you'd want to have the dma_dev member > > > already fixed when calling platform_device_add_data. But you cannot > > > simply do > > > > > > pdata->dma_dev = &imx_ipu_coredev->dev; > > > ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); > > > > > > because *pdata is const. > > > > Thank you for the explanation. Regarding the possibility of using > > platform_device_register_full() to simplify this function. It seem to > > be possible, the following inline function is available to help with this. > > > > imx_add_platform_device_dmamask() > I'd prefer to use platform_device_register_full directly (and let the > wrapper die). > > > But as you mentioned above we need to allocate a new platform_device > > struct before we can assign &imx_ipu_coredev->dev to dma_dev, since > > pdata is const. I guess this assignment could be done after calling > > imx_add_platform_device_dmamask() but I don't think that makes the > No, that won't work, because after platform_device_register_full returns > you must assume that the device is already bound by a driver. And then > you must not change platform_data anymore. > > The only thing that would work is: > > struct mx3_camera_pdata tmppdata; > > if (pdata) { > tmppdata = *pdata; > tmppdata.dma_dev = &imx_ipu_coredev->dev; > > pdata = &tmppdata; > } > > platform_device_register_full(... pdata ...) Looking at converting to platform_device_register_full() again it is a little bit more complicated than I first thought. The call to platform_device_add() is acctually done in a separate function. The involed functions are these: mx31_3ds_init_camera() imx31_alloc_mx3_camera() dma_declare_coherent_memory() platform_device_add() imx35_3ds_init_camera() imx31_alloc_mx3_camera() dma_declare_coherent_memory() platform_device_add() When looking at similar code like the following function: arch/arm/mach-imx/mach-imx27_visstrim_m10.c +244 visstrim_analog_camera_init() imx27_add_mx2_camera() imx_add_platform_device_dmamask() platform_device_register_full() dma_declare_coherent_memory() It seems to be ok to call dma_declare_coherent_memory() after calling platform_device_register_full(). So I'm considering rearranging the calls in the following way: mx31_3ds_init_camera() imx31_alloc_mx3_camera() platform_device_register_full() dma_declare_coherent_memory() Please let me know if you think this would not be ok. Best regards, Emil Goode
I appologise for providing incomplete information in my previous message. The involved call sites are the following: arch/arm/mach-imx/mach-mx35_3ds.c +265 imx35_3ds_init_camera() imx35_alloc_mx3_camera() imx_alloc_mx3_camera() dma_declare_coherent_memory() platform_device_add() arch/arm/mach-imx/mach-mx31moboard.c +477 mx31moboard_init_cam() imx31_alloc_mx3_camera() imx_alloc_mx3_camera() dma_declare_coherent_memory() platform_device_add() arch/arm/mach-imx/mach-mx31_3ds.c +182 mx31_3ds_init_camera() imx31_alloc_mx3_camera() imx_alloc_mx3_camera() dma_declare_coherent_memory() platform_device_add() arch/arm/mach-imx/mach-pcm037.c +413 pcm037_init_camera() imx31_alloc_mx3_camera() imx_alloc_mx3_camera() dma_declare_coherent_memory() platform_device_add() Sorry about the mistake! Best regards, Emil Goode
diff --git a/arch/arm/mach-imx/devices/platform-ipu-core.c b/arch/arm/mach-imx/devices/platform-ipu-core.c index fc4dd7c..68f2a4a 100644 --- a/arch/arm/mach-imx/devices/platform-ipu-core.c +++ b/arch/arm/mach-imx/devices/platform-ipu-core.c @@ -77,34 +77,38 @@ struct platform_device *__init imx_alloc_mx3_camera( pdev = platform_device_alloc("mx3-camera", 0); if (!pdev) - goto err; + return ERR_PTR(-ENOMEM); pdev->dev.dma_mask = kmalloc(sizeof(*pdev->dev.dma_mask), GFP_KERNEL); if (!pdev->dev.dma_mask) - goto err; + goto put_pdev; *pdev->dev.dma_mask = DMA_BIT_MASK(32); pdev->dev.coherent_dma_mask = DMA_BIT_MASK(32); ret = platform_device_add_resources(pdev, res, ARRAY_SIZE(res)); if (ret) - goto err; + goto free_dma_mask; if (pdata) { struct mx3_camera_pdata *copied_pdata; ret = platform_device_add_data(pdev, pdata, sizeof(*pdata)); - if (ret) { -err: - kfree(pdev->dev.dma_mask); - platform_device_put(pdev); - return ERR_PTR(-ENODEV); - } + if (ret) + goto free_dma_mask; + copied_pdata = dev_get_platdata(&pdev->dev); copied_pdata->dma_dev = &imx_ipu_coredev->dev; } return pdev; + +free_dma_mask: + kfree(pdev->dev.dma_mask); +put_pdev: + platform_device_put(pdev); + + return ERR_PTR(ret); } struct platform_device *__init imx_add_mx3_sdc_fb(
If we fail to allocate struct platform_device pdev we dereference it after the goto label err. I have rearranged the error handling a bit to fix the issue and also make it more clear. Signed-off-by: Emil Goode <emilgoode@gmail.com> --- v2: Changed to return -ENOMEM instead of ret where possible and updated the subject line. arch/arm/mach-imx/devices/platform-ipu-core.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)