[PATCH v2] ARM: imx: fix error handling

[PATCH v2] ARM: imx: fix error handling

[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;
 	}
 
 	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(
-- 
1.7.10.4

[PATCH v2] ARM: imx: fix error handling

[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;
 	}
 
 	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(
-- 
1.7.10.4

[PATCH v2] ARM: imx: fix error handling

[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;
 	}
 
 	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(
-- 
1.7.10.4

Re: [PATCH v2] ARM: imx: fix error handling

[walter harms]

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(

Re: [PATCH v2] ARM: imx: fix error handling

[walter harms]

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(

[PATCH v2] ARM: imx: fix error handling

[walter harms]

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(

Re: [PATCH v2] ARM: imx: fix error handling

[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;

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(

Re: [PATCH v2] ARM: imx: fix error handling

[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;

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(

[PATCH v2] ARM: imx: fix error handling

[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;

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(

Re: [PATCH v2] ARM: imx: fix error handling

[walter harms]

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

Re: [PATCH v2] ARM: imx: fix error handling

[walter harms]

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

[PATCH v2] ARM: imx: fix error handling

[walter harms]

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

Re: [PATCH v2] ARM: imx: fix error handling

[Uwe Kleine-König]

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

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

Re: [PATCH v2] ARM: imx: fix error handling

[Uwe Kleine-König]

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

-- 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] ARM: imx: fix error handling

[Uwe Kleine-König]

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

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

Re: [PATCH v2] ARM: imx: fix error handling

[Uwe Kleine-König]

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

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

Re: [PATCH v2] ARM: imx: fix error handling

[Uwe Kleine-König]

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

-- 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] ARM: imx: fix error handling

[Uwe Kleine-König]

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

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

Re: [PATCH v2] ARM: imx: fix error handling

[Dan Carpenter]

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

Re: [PATCH v2] ARM: imx: fix error handling

[Dan Carpenter]

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

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

[PATCH v2] ARM: imx: fix error handling

[Dan Carpenter]

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

Re: [PATCH v2] ARM: imx: fix error handling

[Emil Goode]

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

Re: [PATCH v2] ARM: imx: fix error handling

[Emil Goode]

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

[PATCH v2] ARM: imx: fix error handling

[Emil Goode]

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

Re: [PATCH v2] ARM: imx: fix error handling

[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/  |

Re: [PATCH v2] ARM: imx: fix error handling

[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/  |

[PATCH v2] ARM: imx: fix error handling

[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/  |

Re: [PATCH v2] ARM: imx: fix error handling

[Emil Goode]

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

Re: [PATCH v2] ARM: imx: fix error handling

[Emil Goode]

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



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

[PATCH v2] ARM: imx: fix error handling

[Emil Goode]

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

Re: [PATCH v2] ARM: imx: fix error handling

[Uwe Kleine-König]

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

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

Re: [PATCH v2] ARM: imx: fix error handling

[Uwe Kleine-König]

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

-- 
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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2] ARM: imx: fix error handling

[Uwe Kleine-König]

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

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

Re: [PATCH v2] ARM: imx: fix error handling

[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).

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

Re: [PATCH v2] ARM: imx: fix error handling

[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).

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

[PATCH v2] ARM: imx: fix error handling

[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).

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

Re: [PATCH v2] ARM: imx: fix error handling

[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

Re: [PATCH v2] ARM: imx: fix error handling

[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

[PATCH v2] ARM: imx: fix error handling

[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

Re: [PATCH v2] ARM: imx: fix error handling

[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

Re: [PATCH v2] ARM: imx: fix error handling

[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

[PATCH v2] ARM: imx: fix error handling

[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