ARM: imx: fix error handling

PATCH In review Emil Goode 4 participants 14 messages May 16, 2014 – May 18, 2014

Subsystems (4)

Arm (32-bit) Arm sub-architectures Arm i.MX and Layerscape Kernel janitors

lore

Share & tools

lore

Emil Goode

author

2014-05-16 09:54 UTC #0 lore raw

PATCH v2 1/1 ARM: imx: fix error handling

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 <[email protected]>

---
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(-)

arch/arm/mach-imx/devices/platform-ipu-core.c +13 -9

index fc4dd7c..68f2a4a 100644
@@ -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(

Signature · 1 line

1.7.10.4

walter harms

2014-05-16 10:40 UTC #1 lore raw

Am 16.05.2014 11:54, schrieb Emil Goode:

Quoted 43 lines

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 <[email protected]>

---
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(-)

arch/arm/mach-imx/devices/platform-ipu-core.c +3 -4

index fc4dd7c..68f2a4a 100644
@@ -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) {

arch/arm/mach-imx/devices/platform-ipu-core.c struct platform_device *__init imx_alloc_mx3_camera( · line 101

Quoted 10 lines

arch/arm/mach-imx/devices/platform-ipu-core.c +3 -5

-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

Quoted 13 lines

arch/arm/mach-imx/devices/platform-ipu-core.c +7 -0

 	}
 
 	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(

Emil Goode

author

2014-05-16 11:16 UTC #2 lore raw

Hello Walter,

Quoted 61 lines · walter harms

Am 16.05.2014 11:54, schrieb Emil Goode:
Quoted 43 lines
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 <[email protected]>
---
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(-)
arch/arm/mach-imx/devices/platform-ipu-core.c +3 -4
index fc4dd7c..68f2a4a 100644
@@ -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) {
arch/arm/mach-imx/devices/platform-ipu-core.c struct platform_device *__init imx_alloc_mx3_camera( · line 101
Quoted 10 lines
arch/arm/mach-imx/devices/platform-ipu-core.c +3 -5
-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 ?

Quoted 2 lines

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

Quoted 14 lines

arch/arm/mach-imx/devices/platform-ipu-core.c +7 -0

 	}
 
 	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(

walter harms

2014-05-16 11:49 UTC #3 lore raw

Am 16.05.2014 13:16, schrieb Emil Goode:

Quoted 72 lines

Hello Walter,
Quoted 61 lines · walter harms
Am 16.05.2014 11:54, schrieb Emil Goode:
Quoted 43 lines
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 <[email protected]>
---
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(-)
arch/arm/mach-imx/devices/platform-ipu-core.c +3 -4
index fc4dd7c..68f2a4a 100644
@@ -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) {
arch/arm/mach-imx/devices/platform-ipu-core.c struct platform_device *__init imx_alloc_mx3_camera( · line 101
Quoted 10 lines
arch/arm/mach-imx/devices/platform-ipu-core.c +3 -5
-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 ?
Quoted 2 lines
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:

Quoted 8 lines

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

Quoted 19 lines

Best regards,

Emil Goode

Quoted 14 lines

arch/arm/mach-imx/devices/platform-ipu-core.c +7 -0

 	}
 
 	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(

Uwe Kleine-König

2014-05-16 19:31 UTC #4 lore raw

Hello Walter,

Quoted 81 lines · walter harms

Am 16.05.2014 13:16, schrieb Emil Goode:
Quoted 80 lines
Hello Walter,
Quoted 61 lines · walter harms
Am 16.05.2014 11:54, schrieb Emil Goode:
Quoted 43 lines
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 <[email protected]>
---
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(-)
arch/arm/mach-imx/devices/platform-ipu-core.c +3 -4
index fc4dd7c..68f2a4a 100644
@@ -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) {
arch/arm/mach-imx/devices/platform-ipu-core.c struct platform_device *__init imx_alloc_mx3_camera( · line 101
Quoted 10 lines
arch/arm/mach-imx/devices/platform-ipu-core.c +3 -5
-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 ?
Quoted 2 lines
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

Signature · 6 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Emil Goode

author

2014-05-17 15:35 UTC #5 lore raw

Hello Uwe,

Quoted 88 lines · Uwe Kleine-KÃ¶nig

Hello Walter,
Quoted 81 lines · walter harms
Am 16.05.2014 13:16, schrieb Emil Goode:
Quoted 80 lines
Hello Walter,
Quoted 61 lines · walter harms
Am 16.05.2014 11:54, schrieb Emil Goode:
Quoted 43 lines
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 <[email protected]>
---
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(-)
arch/arm/mach-imx/devices/platform-ipu-core.c +3 -4
index fc4dd7c..68f2a4a 100644
@@ -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) {
arch/arm/mach-imx/devices/platform-ipu-core.c struct platform_device *__init imx_alloc_mx3_camera( · line 101
Quoted 10 lines
arch/arm/mach-imx/devices/platform-ipu-core.c +3 -5
-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 ?
Quoted 2 lines
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 ...

Quoted 8 lines

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

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Uwe Kleine-König

2014-05-17 19:05 UTC #6 lore raw

Hello Emil,

Quoted 94 lines · Emil Goode

Quoted 93 lines · Uwe Kleine-König
Quoted 81 lines · walter harms
Am 16.05.2014 13:16, schrieb Emil Goode:
Quoted 80 lines
Hello Walter,
Quoted 61 lines · walter harms
Am 16.05.2014 11:54, schrieb Emil Goode:
Quoted 43 lines
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 <[email protected]>
---
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(-)
arch/arm/mach-imx/devices/platform-ipu-core.c +3 -4
index fc4dd7c..68f2a4a 100644
@@ -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) {
arch/arm/mach-imx/devices/platform-ipu-core.c struct platform_device *__init imx_alloc_mx3_camera( · line 101
Quoted 10 lines
arch/arm/mach-imx/devices/platform-ipu-core.c +3 -5
-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 ?
Quoted 2 lines
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));

Quoted 7 lines

Quoted 1 line
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).

Quoted 4 lines

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

Signature · 6 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Emil Goode

author

2014-05-17 22:14 UTC #7 lore raw

Hello Uwe,

Quoted 99 lines · Uwe Kleine-König

Hello Emil,
Quoted 96 lines · Emil Goode
Quoted 94 lines · Uwe Kleine-König
Quoted 81 lines · walter harms
Am 16.05.2014 13:16, schrieb Emil Goode:
Quoted 80 lines
Hello Walter,
Quoted 61 lines · walter harms
Am 16.05.2014 11:54, schrieb Emil Goode:
Quoted 43 lines
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 <[email protected]>
---
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(-)
arch/arm/mach-imx/devices/platform-ipu-core.c +3 -4
index fc4dd7c..68f2a4a 100644
@@ -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) {
arch/arm/mach-imx/devices/platform-ipu-core.c struct platform_device *__init imx_alloc_mx3_camera( · line 101
Quoted 10 lines
arch/arm/mach-imx/devices/platform-ipu-core.c +3 -5
-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 ?
Quoted 2 lines
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.

Quoted 7 lines

Quoted 5 lines
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.

Quoted 8 lines

Quoted 5 lines
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.

Quoted 13 lines

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

author

2014-05-18 14:37 UTC #8 lore raw

In reply to Uwe Kleine-König

Hello Uwe,

Quoted 117 lines · Uwe Kleine-König

Hello Emil,
Quoted 94 lines · Emil Goode
Quoted 93 lines · Uwe Kleine-König
Quoted 81 lines · walter harms
Am 16.05.2014 13:16, schrieb Emil Goode:
Quoted 80 lines
Hello Walter,
Quoted 61 lines · walter harms
Am 16.05.2014 11:54, schrieb Emil Goode:
Quoted 43 lines
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 <[email protected]>
---
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(-)
arch/arm/mach-imx/devices/platform-ipu-core.c +3 -4
index fc4dd7c..68f2a4a 100644
@@ -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) {
arch/arm/mach-imx/devices/platform-ipu-core.c struct platform_device *__init imx_alloc_mx3_camera( · line 101
Quoted 10 lines
arch/arm/mach-imx/devices/platform-ipu-core.c +3 -5
-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 ?
Quoted 2 lines
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));
Quoted 7 lines
Quoted 1 line
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).
Quoted 4 lines
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:

Quoted 10 lines

	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

author

2014-05-18 15:38 UTC #9 lore raw

In reply to Uwe Kleine-König

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

Uwe Kleine-König

2014-05-16 19:24 UTC #10 lore raw

In reply to Emil Goode

Hello Emil,

IMHO the subject is too general. Maybe better use:

ARM: imx: fix error handling in ipu device registration

Quoted 54 lines · 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 <[email protected]>

---
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(-)

arch/arm/mach-imx/devices/platform-ipu-core.c +6 -9

index fc4dd7c..68f2a4a 100644
@@ -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;
 	}

arch/arm/mach-imx/devices/platform-ipu-core.c struct platform_device *__init imx_alloc_mx3_camera( · line 112

Quoted 10 lines

arch/arm/mach-imx/devices/platform-ipu-core.c +7 -0

 
 	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

Signature · 6 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Dan Carpenter

2014-05-16 22:21 UTC #11 lore raw

Quoted 2 lines · Uwe Kleine-König

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.

Quoted 6 lines

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

--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Emil Goode

author

2014-05-16 22:47 UTC #12 lore raw

Quoted 5 lines · Dan Carpenter

Quoted 2 lines · Uwe Kleine-KÃ¶nig
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.

Quoted 14 lines

Quoted 7 lines
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.

Quoted 5 lines

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
--
To unsubscribe from this list: send the line "unsubscribe kernel-janitors" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Emil Goode

author

2014-05-16 23:18 UTC #13 lore raw

In reply to Uwe Kleine-König

Hello Uwe,

Thank you for the review.

Quoted 5 lines · Uwe Kleine-König

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.

Quoted 57 lines · Emil Goode

Quoted 55 lines

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 <[email protected]>

---
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(-)

arch/arm/mach-imx/devices/platform-ipu-core.c +6 -9

index fc4dd7c..68f2a4a 100644
@@ -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;
 	}

Quoted 11 lines

arch/arm/mach-imx/devices/platform-ipu-core.c struct platform_device *__init imx_alloc_mx3_camera( · line 112
Quoted 9 lines
arch/arm/mach-imx/devices/platform-ipu-core.c +7 -0
 	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.

Quoted 6 lines

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.

Quoted 3 lines

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

Quoted 7 lines

Best regards
Uwe

Signature · 2 lines

Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |