[PATCH] ARM: OMAP-USB: Fix possible memory leak

[PATCH] ARM: OMAP-USB: Fix possible memory leak

[Alexander Shiyan]

Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
---
 arch/arm/mach-omap2/usb-host.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/arch/arm/mach-omap2/usb-host.c b/arch/arm/mach-omap2/usb-host.c
index aa27d7f..8d17a0d 100644
--- a/arch/arm/mach-omap2/usb-host.c
+++ b/arch/arm/mach-omap2/usb-host.c
@@ -570,8 +570,10 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
 	supplies->dev_name = dev_id;
 
 	reg_data = kzalloc(sizeof(*reg_data), GFP_KERNEL);
-	if (!reg_data)
+	if (!reg_data) {
+		kfree(supplies);
 		return -ENOMEM;
+	}
 
 	reg_data->constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
 	reg_data->consumer_supplies = supplies;
@@ -579,8 +581,11 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
 
 	config = kmemdup(&hsusb_reg_config, sizeof(hsusb_reg_config),
 			GFP_KERNEL);
-	if (!config)
+	if (!config) {
+		kfree(supplies);
+		kfree(reg_data);
 		return -ENOMEM;
+	}
 
 	config->supply_name = name;
 	config->gpio = gpio;
@@ -589,17 +594,25 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
 
 	/* create a regulator device */
 	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
-	if (!pdev)
+	if (!pdev) {
+		kfree(supplies);
+		kfree(reg_data);
+		kfree(config);
 		return -ENOMEM;
+	}
 
 	pdev->id = PLATFORM_DEVID_AUTO;
 	pdev->name = reg_name;
 	pdev->dev.platform_data = config;
 
 	ret = platform_device_register(pdev);
-	if (ret)
+	if (ret) {
 		pr_err("%s: Failed registering regulator %s for %s\n",
 				__func__, name, dev_id);
+		kfree(supplies);
+		kfree(reg_data);
+		kfree(config);
+	}
 
 	return ret;
 }
-- 
1.8.1.5

[PATCH] ARM: OMAP-USB: Fix possible memory leak

[Nishanth Menon]

On 20:03-20130502, Alexander Shiyan wrote:
> 
> Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> ---
>  arch/arm/mach-omap2/usb-host.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm/mach-omap2/usb-host.c b/arch/arm/mach-omap2/usb-host.c
> index aa27d7f..8d17a0d 100644
> --- a/arch/arm/mach-omap2/usb-host.c
> +++ b/arch/arm/mach-omap2/usb-host.c
> @@ -570,8 +570,10 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
>  	supplies->dev_name = dev_id;
>  
>  	reg_data = kzalloc(sizeof(*reg_data), GFP_KERNEL);
> -	if (!reg_data)
> +	if (!reg_data) {
> +		kfree(supplies);
>  		return -ENOMEM;
> +	}
>  
>  	reg_data->constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
>  	reg_data->consumer_supplies = supplies;
> @@ -579,8 +581,11 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
>  
>  	config = kmemdup(&hsusb_reg_config, sizeof(hsusb_reg_config),
>  			GFP_KERNEL);
> -	if (!config)
> +	if (!config) {
> +		kfree(supplies);
> +		kfree(reg_data);
>  		return -ENOMEM;
> +	}
>  
>  	config->supply_name = name;
>  	config->gpio = gpio;
> @@ -589,17 +594,25 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
>  
>  	/* create a regulator device */
>  	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> -	if (!pdev)
> +	if (!pdev) {
> +		kfree(supplies);
> +		kfree(reg_data);
> +		kfree(config);
>  		return -ENOMEM;
> +	}
>  
>  	pdev->id = PLATFORM_DEVID_AUTO;
>  	pdev->name = reg_name;
>  	pdev->dev.platform_data = config;
>  
>  	ret = platform_device_register(pdev);
> -	if (ret)
> +	if (ret) {
>  		pr_err("%s: Failed registering regulator %s for %s\n",
>  				__func__, name, dev_id);
> +		kfree(supplies);
> +		kfree(reg_data);
> +		kfree(config);
> +	}

Might be better to switch to devm_XXX managed functions?
>  
>  	return ret;
>  }
> -- 
> 1.8.1.5
> 
> 
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

-- 
Regards,
Nishanth Menon

Re[2]: [PATCH] ARM: OMAP-USB: Fix possible memory leak

[Alexander Shiyan]

> On 20:03-20130502, Alexander Shiyan wrote:
> > 
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > ---
> >  arch/arm/mach-omap2/usb-host.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/usb-host.c b/arch/arm/mach-omap2/usb-host.c
> > index aa27d7f..8d17a0d 100644
> > --- a/arch/arm/mach-omap2/usb-host.c
> > +++ b/arch/arm/mach-omap2/usb-host.c
> > @@ -570,8 +570,10 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> >  	supplies->dev_name = dev_id;
> >  
> >  	reg_data = kzalloc(sizeof(*reg_data), GFP_KERNEL);
> > -	if (!reg_data)
> > +	if (!reg_data) {
> > +		kfree(supplies);
> >  		return -ENOMEM;
> > +	}
> >  
> >  	reg_data->constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
> >  	reg_data->consumer_supplies = supplies;
> > @@ -579,8 +581,11 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> >  
> >  	config = kmemdup(&hsusb_reg_config, sizeof(hsusb_reg_config),
> >  			GFP_KERNEL);
> > -	if (!config)
> > +	if (!config) {
> > +		kfree(supplies);
> > +		kfree(reg_data);
> >  		return -ENOMEM;
> > +	}
> >  
> >  	config->supply_name = name;
> >  	config->gpio = gpio;
> > @@ -589,17 +594,25 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> >  
> >  	/* create a regulator device */
> >  	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> > -	if (!pdev)
> > +	if (!pdev) {
> > +		kfree(supplies);
> > +		kfree(reg_data);
> > +		kfree(config);
> >  		return -ENOMEM;
> > +	}
> >  
> >  	pdev->id = PLATFORM_DEVID_AUTO;
> >  	pdev->name = reg_name;
> >  	pdev->dev.platform_data = config;
> >  
> >  	ret = platform_device_register(pdev);
> > -	if (ret)
> > +	if (ret) {
> >  		pr_err("%s: Failed registering regulator %s for %s\n",
> >  				__func__, name, dev_id);
> > +		kfree(supplies);
> > +		kfree(reg_data);
> > +		kfree(config);
> > +	}
> 
> Might be better to switch to devm_XXX managed functions?

If anyone can rewrite driver to use devm_xx, it would have been better.
I'm not going to redo the patch yet, let it be so, I just showed a point
for OMAP-developers.
Thanks.

---

[PATCH] ARM: OMAP-USB: Fix possible memory leak

[Felipe Balbi]

Hi,

On Thu, May 02, 2013 at 08:28:44PM +0400, Alexander Shiyan wrote:
> > On 20:03-20130502, Alexander Shiyan wrote:
> > > 
> > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > > ---
> > >  arch/arm/mach-omap2/usb-host.c | 21 +++++++++++++++++----
> > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/arch/arm/mach-omap2/usb-host.c b/arch/arm/mach-omap2/usb-host.c
> > > index aa27d7f..8d17a0d 100644
> > > --- a/arch/arm/mach-omap2/usb-host.c
> > > +++ b/arch/arm/mach-omap2/usb-host.c
> > > @@ -570,8 +570,10 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> > >  	supplies->dev_name = dev_id;
> > >  
> > >  	reg_data = kzalloc(sizeof(*reg_data), GFP_KERNEL);
> > > -	if (!reg_data)
> > > +	if (!reg_data) {
> > > +		kfree(supplies);
> > >  		return -ENOMEM;
> > > +	}
> > >  
> > >  	reg_data->constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
> > >  	reg_data->consumer_supplies = supplies;
> > > @@ -579,8 +581,11 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> > >  
> > >  	config = kmemdup(&hsusb_reg_config, sizeof(hsusb_reg_config),
> > >  			GFP_KERNEL);
> > > -	if (!config)
> > > +	if (!config) {
> > > +		kfree(supplies);
> > > +		kfree(reg_data);
> > >  		return -ENOMEM;
> > > +	}
> > >  
> > >  	config->supply_name = name;
> > >  	config->gpio = gpio;
> > > @@ -589,17 +594,25 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> > >  
> > >  	/* create a regulator device */
> > >  	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> > > -	if (!pdev)
> > > +	if (!pdev) {
> > > +		kfree(supplies);
> > > +		kfree(reg_data);
> > > +		kfree(config);
> > >  		return -ENOMEM;
> > > +	}
> > >  
> > >  	pdev->id = PLATFORM_DEVID_AUTO;
> > >  	pdev->name = reg_name;
> > >  	pdev->dev.platform_data = config;
> > >  
> > >  	ret = platform_device_register(pdev);
> > > -	if (ret)
> > > +	if (ret) {
> > >  		pr_err("%s: Failed registering regulator %s for %s\n",
> > >  				__func__, name, dev_id);
> > > +		kfree(supplies);
> > > +		kfree(reg_data);
> > > +		kfree(config);
> > > +	}
> > 
> > Might be better to switch to devm_XXX managed functions?

I don't think it makes sense since the platform_device hasn't been
registered yet.

Still, patch can be improved with proper goto labels instead of
sprinkling different kfree() calls in every single error branch.

> If anyone can rewrite driver to use devm_xx, it would have been better.
> I'm not going to redo the patch yet, let it be so, I just showed a point
> for OMAP-developers.

fair enough.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130502/4c5991d0/attachment.sig>

[PATCH] ARM: OMAP-USB: Fix possible memory leak

[Russell King - ARM Linux]

On Thu, May 02, 2013 at 11:24:33AM -0500, Nishanth Menon wrote:
> On 20:03-20130502, Alexander Shiyan wrote:
> > 
> > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > ---
> >  arch/arm/mach-omap2/usb-host.c | 21 +++++++++++++++++----
> >  1 file changed, 17 insertions(+), 4 deletions(-)
> > 
> > diff --git a/arch/arm/mach-omap2/usb-host.c b/arch/arm/mach-omap2/usb-host.c
> > index aa27d7f..8d17a0d 100644
> > --- a/arch/arm/mach-omap2/usb-host.c
> > +++ b/arch/arm/mach-omap2/usb-host.c
> > @@ -570,8 +570,10 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> >  	supplies->dev_name = dev_id;
> >  
> >  	reg_data = kzalloc(sizeof(*reg_data), GFP_KERNEL);
> > -	if (!reg_data)
> > +	if (!reg_data) {
> > +		kfree(supplies);
> >  		return -ENOMEM;
> > +	}
> >  
> >  	reg_data->constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
> >  	reg_data->consumer_supplies = supplies;
> > @@ -579,8 +581,11 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> >  
> >  	config = kmemdup(&hsusb_reg_config, sizeof(hsusb_reg_config),
> >  			GFP_KERNEL);
> > -	if (!config)
> > +	if (!config) {
> > +		kfree(supplies);
> > +		kfree(reg_data);
> >  		return -ENOMEM;
> > +	}
> >  
> >  	config->supply_name = name;
> >  	config->gpio = gpio;
> > @@ -589,17 +594,25 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> >  
> >  	/* create a regulator device */
> >  	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> > -	if (!pdev)
> > +	if (!pdev) {
> > +		kfree(supplies);
> > +		kfree(reg_data);
> > +		kfree(config);
> >  		return -ENOMEM;
> > +	}
> >  
> >  	pdev->id = PLATFORM_DEVID_AUTO;
> >  	pdev->name = reg_name;
> >  	pdev->dev.platform_data = config;
> >  
> >  	ret = platform_device_register(pdev);
> > -	if (ret)
> > +	if (ret) {
> >  		pr_err("%s: Failed registering regulator %s for %s\n",
> >  				__func__, name, dev_id);
> > +		kfree(supplies);
> > +		kfree(reg_data);
> > +		kfree(config);
> > +	}
> 
> Might be better to switch to devm_XXX managed functions?

Oh god no.  Stop this madness right now.  Never kmalloc memory for
platform devices.  EVER.

There's an API provided to dynamically create platform devices.  Use it!

[PATCH] ARM: OMAP-USB: Fix possible memory leak

[Russell King - ARM Linux]

On Thu, May 02, 2013 at 07:40:58PM +0300, Felipe Balbi wrote:
> Hi,
> 
> On Thu, May 02, 2013 at 08:28:44PM +0400, Alexander Shiyan wrote:
> > > On 20:03-20130502, Alexander Shiyan wrote:
> > > > 
> > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > > > ---
> > > >  arch/arm/mach-omap2/usb-host.c | 21 +++++++++++++++++----
> > > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/arch/arm/mach-omap2/usb-host.c b/arch/arm/mach-omap2/usb-host.c
> > > > index aa27d7f..8d17a0d 100644
> > > > --- a/arch/arm/mach-omap2/usb-host.c
> > > > +++ b/arch/arm/mach-omap2/usb-host.c
> > > > @@ -570,8 +570,10 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> > > >  	supplies->dev_name = dev_id;
> > > >  
> > > >  	reg_data = kzalloc(sizeof(*reg_data), GFP_KERNEL);
> > > > -	if (!reg_data)
> > > > +	if (!reg_data) {
> > > > +		kfree(supplies);
> > > >  		return -ENOMEM;
> > > > +	}
> > > >  
> > > >  	reg_data->constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
> > > >  	reg_data->consumer_supplies = supplies;
> > > > @@ -579,8 +581,11 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> > > >  
> > > >  	config = kmemdup(&hsusb_reg_config, sizeof(hsusb_reg_config),
> > > >  			GFP_KERNEL);
> > > > -	if (!config)
> > > > +	if (!config) {
> > > > +		kfree(supplies);
> > > > +		kfree(reg_data);
> > > >  		return -ENOMEM;
> > > > +	}
> > > >  
> > > >  	config->supply_name = name;
> > > >  	config->gpio = gpio;
> > > > @@ -589,17 +594,25 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> > > >  
> > > >  	/* create a regulator device */
> > > >  	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> > > > -	if (!pdev)
> > > > +	if (!pdev) {
> > > > +		kfree(supplies);
> > > > +		kfree(reg_data);
> > > > +		kfree(config);
> > > >  		return -ENOMEM;
> > > > +	}
> > > >  
> > > >  	pdev->id = PLATFORM_DEVID_AUTO;
> > > >  	pdev->name = reg_name;
> > > >  	pdev->dev.platform_data = config;
> > > >  
> > > >  	ret = platform_device_register(pdev);
> > > > -	if (ret)
> > > > +	if (ret) {
> > > >  		pr_err("%s: Failed registering regulator %s for %s\n",
> > > >  				__func__, name, dev_id);
> > > > +		kfree(supplies);
> > > > +		kfree(reg_data);
> > > > +		kfree(config);
> > > > +	}
> > > 
> > > Might be better to switch to devm_XXX managed functions?
> 
> I don't think it makes sense since the platform_device hasn't been
> registered yet.
> 
> Still, patch can be improved with proper goto labels instead of
> sprinkling different kfree() calls in every single error branch.
> 
> > If anyone can rewrite driver to use devm_xx, it would have been better.
> > I'm not going to redo the patch yet, let it be so, I just showed a point
> > for OMAP-developers.
> 
> fair enough.

Well, as long as this crap violates the driver model by using kfree() on
a device...  Devices are refcounted and must only be freed when the
refcount drops to zero.

This is exactly why we have platform_device_alloc(),
platform_device_register_full() and friends - so that people don't have to
fsck around with kzalloc themselves and get it wrong like the above does.

Would you like me to pass your details to gregkh for another one of his
public humilation exercises over basic kernel programming stuff? :)

[PATCH] ARM: OMAP-USB: Fix possible memory leak

[Felipe Balbi]

Hi,

On Fri, May 03, 2013 at 12:13:53AM +0100, Russell King - ARM Linux wrote:
> > > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > > > > ---
> > > > >  arch/arm/mach-omap2/usb-host.c | 21 +++++++++++++++++----
> > > > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/arch/arm/mach-omap2/usb-host.c b/arch/arm/mach-omap2/usb-host.c
> > > > > index aa27d7f..8d17a0d 100644
> > > > > --- a/arch/arm/mach-omap2/usb-host.c
> > > > > +++ b/arch/arm/mach-omap2/usb-host.c
> > > > > @@ -570,8 +570,10 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> > > > >  	supplies->dev_name = dev_id;
> > > > >  
> > > > >  	reg_data = kzalloc(sizeof(*reg_data), GFP_KERNEL);
> > > > > -	if (!reg_data)
> > > > > +	if (!reg_data) {
> > > > > +		kfree(supplies);
> > > > >  		return -ENOMEM;
> > > > > +	}
> > > > >  
> > > > >  	reg_data->constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
> > > > >  	reg_data->consumer_supplies = supplies;
> > > > > @@ -579,8 +581,11 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> > > > >  
> > > > >  	config = kmemdup(&hsusb_reg_config, sizeof(hsusb_reg_config),
> > > > >  			GFP_KERNEL);
> > > > > -	if (!config)
> > > > > +	if (!config) {
> > > > > +		kfree(supplies);
> > > > > +		kfree(reg_data);
> > > > >  		return -ENOMEM;
> > > > > +	}
> > > > >  
> > > > >  	config->supply_name = name;
> > > > >  	config->gpio = gpio;
> > > > > @@ -589,17 +594,25 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> > > > >  
> > > > >  	/* create a regulator device */
> > > > >  	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> > > > > -	if (!pdev)
> > > > > +	if (!pdev) {
> > > > > +		kfree(supplies);
> > > > > +		kfree(reg_data);
> > > > > +		kfree(config);
> > > > >  		return -ENOMEM;
> > > > > +	}
> > > > >  
> > > > >  	pdev->id = PLATFORM_DEVID_AUTO;
> > > > >  	pdev->name = reg_name;
> > > > >  	pdev->dev.platform_data = config;
> > > > >  
> > > > >  	ret = platform_device_register(pdev);
> > > > > -	if (ret)
> > > > > +	if (ret) {
> > > > >  		pr_err("%s: Failed registering regulator %s for %s\n",
> > > > >  				__func__, name, dev_id);
> > > > > +		kfree(supplies);
> > > > > +		kfree(reg_data);
> > > > > +		kfree(config);
> > > > > +	}
> > > > 
> > > > Might be better to switch to devm_XXX managed functions?
> > 
> > I don't think it makes sense since the platform_device hasn't been
> > registered yet.
> > 
> > Still, patch can be improved with proper goto labels instead of
> > sprinkling different kfree() calls in every single error branch.
> > 
> > > If anyone can rewrite driver to use devm_xx, it would have been better.
> > > I'm not going to redo the patch yet, let it be so, I just showed a point
> > > for OMAP-developers.
> > 
> > fair enough.
> 
> Well, as long as this crap violates the driver model by using kfree() on
> a device...  Devices are refcounted and must only be freed when the
> refcount drops to zero.

read the patch again, there's no kfree() on any device. There is a kfree
of supplies, reg_data and config.

On top of that, the code being changed here doesn't even exist, so I
wonder which tree is this code based off. usb-host.c has always being
using omap_device_build() which internally calls
platform_device_alloc().

> This is exactly why we have platform_device_alloc(),
> platform_device_register_full() and friends - so that people don't have to
> fsck around with kzalloc themselves and get it wrong like the above does.
> 
> Would you like me to pass your details to gregkh for another one of his
> public humilation exercises over basic kernel programming stuff? :)

How about we pass yours for not reading the patch before flaming ? Note
that $SUBJECT is *not* touching at all that line which kzallocs a
platform_device. Wrong as it is, it's not part of $SUBJECT.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130503/b244bb23/attachment.sig>

[PATCH] ARM: OMAP-USB: Fix possible memory leak

[Felipe Balbi]

Hi,

On Fri, May 03, 2013 at 09:21:51AM +0300, Felipe Balbi wrote:
> > This is exactly why we have platform_device_alloc(),
> > platform_device_register_full() and friends - so that people don't have to
> > fsck around with kzalloc themselves and get it wrong like the above does.
> > 
> > Would you like me to pass your details to gregkh for another one of his
> > public humilation exercises over basic kernel programming stuff? :)
> 
> How about we pass yours for not reading the patch before flaming ? Note
> that $SUBJECT is *not* touching at all that line which kzallocs a
> platform_device. Wrong as it is, it's not part of $SUBJECT.

that line was added by Roger on commit
5ecd52e563f1e14f9cfe06130fbf9fdb73f227e8 (ARM: OMAP2+: omap-usb-host: Add
usbhs_init_phys()).

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130503/4a947d27/attachment.sig>

[PATCH] ARM: OMAP-USB: Fix possible memory leak

[Felipe Balbi]

On Fri, May 03, 2013 at 09:25:06AM +0300, Felipe Balbi wrote:
> > > This is exactly why we have platform_device_alloc(),
> > > platform_device_register_full() and friends - so that people don't have to
> > > fsck around with kzalloc themselves and get it wrong like the above does.
> > > 
> > > Would you like me to pass your details to gregkh for another one of his
> > > public humilation exercises over basic kernel programming stuff? :)
> > 
> > How about we pass yours for not reading the patch before flaming ? Note
> > that $SUBJECT is *not* touching at all that line which kzallocs a
> > platform_device. Wrong as it is, it's not part of $SUBJECT.
> 
> that line was added by Roger on commit
> 5ecd52e563f1e14f9cfe06130fbf9fdb73f227e8 (ARM: OMAP2+: omap-usb-host: Add
> usbhs_init_phys()).

forgot to add him to Cc, now done.

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130503/45651a32/attachment-0001.sig>

[PATCH] ARM: OMAP-USB: Fix possible memory leak

[Russell King - ARM Linux]

On Fri, May 03, 2013 at 09:21:51AM +0300, Felipe Balbi wrote:
> Hi,
> 
> On Fri, May 03, 2013 at 12:13:53AM +0100, Russell King - ARM Linux wrote:
> > > > > > Signed-off-by: Alexander Shiyan <shc_work@mail.ru>
> > > > > > ---
> > > > > >  arch/arm/mach-omap2/usb-host.c | 21 +++++++++++++++++----
> > > > > >  1 file changed, 17 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/arch/arm/mach-omap2/usb-host.c b/arch/arm/mach-omap2/usb-host.c
> > > > > > index aa27d7f..8d17a0d 100644
> > > > > > --- a/arch/arm/mach-omap2/usb-host.c
> > > > > > +++ b/arch/arm/mach-omap2/usb-host.c
> > > > > > @@ -570,8 +570,10 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> > > > > >  	supplies->dev_name = dev_id;
> > > > > >  
> > > > > >  	reg_data = kzalloc(sizeof(*reg_data), GFP_KERNEL);
> > > > > > -	if (!reg_data)
> > > > > > +	if (!reg_data) {
> > > > > > +		kfree(supplies);
> > > > > >  		return -ENOMEM;
> > > > > > +	}
> > > > > >  
> > > > > >  	reg_data->constraints.valid_ops_mask = REGULATOR_CHANGE_STATUS;
> > > > > >  	reg_data->consumer_supplies = supplies;
> > > > > > @@ -579,8 +581,11 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> > > > > >  
> > > > > >  	config = kmemdup(&hsusb_reg_config, sizeof(hsusb_reg_config),
> > > > > >  			GFP_KERNEL);
> > > > > > -	if (!config)
> > > > > > +	if (!config) {
> > > > > > +		kfree(supplies);
> > > > > > +		kfree(reg_data);
> > > > > >  		return -ENOMEM;
> > > > > > +	}
> > > > > >  
> > > > > >  	config->supply_name = name;
> > > > > >  	config->gpio = gpio;
> > > > > > @@ -589,17 +594,25 @@ static int usbhs_add_regulator(char *name, char *dev_id, char *dev_supply,
> > > > > >  
> > > > > >  	/* create a regulator device */
> > > > > >  	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> > > > > > -	if (!pdev)
> > > > > > +	if (!pdev) {
> > > > > > +		kfree(supplies);
> > > > > > +		kfree(reg_data);
> > > > > > +		kfree(config);
> > > > > >  		return -ENOMEM;
> > > > > > +	}
> > > > > >  
> > > > > >  	pdev->id = PLATFORM_DEVID_AUTO;
> > > > > >  	pdev->name = reg_name;
> > > > > >  	pdev->dev.platform_data = config;
> > > > > >  
> > > > > >  	ret = platform_device_register(pdev);
> > > > > > -	if (ret)
> > > > > > +	if (ret) {
> > > > > >  		pr_err("%s: Failed registering regulator %s for %s\n",
> > > > > >  				__func__, name, dev_id);
> > > > > > +		kfree(supplies);
> > > > > > +		kfree(reg_data);
> > > > > > +		kfree(config);
> > > > > > +	}
> > > > > 
> > > > > Might be better to switch to devm_XXX managed functions?
> > > 
> > > I don't think it makes sense since the platform_device hasn't been
> > > registered yet.
> > > 
> > > Still, patch can be improved with proper goto labels instead of
> > > sprinkling different kfree() calls in every single error branch.
> > > 
> > > > If anyone can rewrite driver to use devm_xx, it would have been better.
> > > > I'm not going to redo the patch yet, let it be so, I just showed a point
> > > > for OMAP-developers.
> > > 
> > > fair enough.
> > 
> > Well, as long as this crap violates the driver model by using kfree() on
> > a device...  Devices are refcounted and must only be freed when the
> > refcount drops to zero.
> 
> read the patch again, there's no kfree() on any device. There is a kfree
> of supplies, reg_data and config.
> 
> On top of that, the code being changed here doesn't even exist, so I
> wonder which tree is this code based off. usb-host.c has always being
> using omap_device_build() which internally calls
> platform_device_alloc().
> 
> > This is exactly why we have platform_device_alloc(),
> > platform_device_register_full() and friends - so that people don't have to
> > fsck around with kzalloc themselves and get it wrong like the above does.
> > 
> > Would you like me to pass your details to gregkh for another one of his
> > public humilation exercises over basic kernel programming stuff? :)
> 
> How about we pass yours for not reading the patch before flaming ? Note
> that $SUBJECT is *not* touching at all that line which kzallocs a
> platform_device. Wrong as it is, it's not part of $SUBJECT.

It's really simple.  You do not use k*alloc with platform devices.  And
you reject any patch which contains that, and point it out to the patch
author.

It really doesn't matter if there's a kfree or not.  The fact is you do
not allow it in any situation, because such bad practises get copied
and then you end up with kfree's.

How about you gain an understanding of this stuff and why this stuff is
soo "hot".

[PATCH] ARM: OMAP-USB: Fix possible memory leak

[Felipe Balbi]

Hi,

On Fri, May 03, 2013 at 09:15:10AM +0100, Russell King - ARM Linux wrote:
> > > This is exactly why we have platform_device_alloc(),
> > > platform_device_register_full() and friends - so that people don't have to
> > > fsck around with kzalloc themselves and get it wrong like the above does.
> > > 
> > > Would you like me to pass your details to gregkh for another one of his
> > > public humilation exercises over basic kernel programming stuff? :)
> > 
> > How about we pass yours for not reading the patch before flaming ? Note
> > that $SUBJECT is *not* touching at all that line which kzallocs a
> > platform_device. Wrong as it is, it's not part of $SUBJECT.
> 
> It's really simple.  You do not use k*alloc with platform devices.  And

agree, no discussions here

> you reject any patch which contains that, and point it out to the patch
> author.
> 
> It really doesn't matter if there's a kfree or not.  The fact is you do
> not allow it in any situation, because such bad practises get copied
> and then you end up with kfree's.
> 
> How about you gain an understanding of this stuff and why this stuff is
> soo "hot".

how about you look at the git log to figure out I had nothing to do with
that original patch which added k*alloc to the pdev ?

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130503/624e77bc/attachment.sig>

[PATCH] ARM: OMAP-USB: Fix possible memory leak

[Russell King - ARM Linux]

On Fri, May 03, 2013 at 09:21:51AM +0300, Felipe Balbi wrote:
> How about we pass yours for not reading the patch before flaming ?

And another thing.  If you want to be flamed, continue with that tone.
The problem here is that *you* have not been flamed in the previous
message.  The previous message was all about pointing out an error in
the code in a way that hopefully you will take notice of, because you
showed no sign of taking any notice of my previous mail.

If you want to be a total dork, continue behaving as you currently are.
There, *now* I *have* flamed you!

[PATCH] ARM: OMAP-USB: Fix possible memory leak

[Russell King - ARM Linux]

On Fri, May 03, 2013 at 11:30:09AM +0300, Felipe Balbi wrote:
> Hi,
> 
> On Fri, May 03, 2013 at 09:15:10AM +0100, Russell King - ARM Linux wrote:
> > > > This is exactly why we have platform_device_alloc(),
> > > > platform_device_register_full() and friends - so that people don't have to
> > > > fsck around with kzalloc themselves and get it wrong like the above does.
> > > > 
> > > > Would you like me to pass your details to gregkh for another one of his
> > > > public humilation exercises over basic kernel programming stuff? :)
> > > 
> > > How about we pass yours for not reading the patch before flaming ? Note
> > > that $SUBJECT is *not* touching at all that line which kzallocs a
> > > platform_device. Wrong as it is, it's not part of $SUBJECT.
> > 
> > It's really simple.  You do not use k*alloc with platform devices.  And
> 
> agree, no discussions here
> 
> > you reject any patch which contains that, and point it out to the patch
> > author.
> > 
> > It really doesn't matter if there's a kfree or not.  The fact is you do
> > not allow it in any situation, because such bad practises get copied
> > and then you end up with kfree's.
> > 
> > How about you gain an understanding of this stuff and why this stuff is
> > soo "hot".
> 
> how about you look at the git log to figure out I had nothing to do with
> that original patch which added k*alloc to the pdev ?

How about you realise I haven't got time to fuck around like that at the
moment, so in the interests of getting people to fix the glaring error
I commented on it instead in a relevant thread in the hope that some
prat like you would take notice and ask for it to be fixed by someone
working on the driver.

[PATCH] ARM: OMAP-USB: Fix possible memory leak

[Felipe Balbi]

Hi,

On Fri, May 03, 2013 at 09:32:06AM +0100, Russell King - ARM Linux wrote:
> On Fri, May 03, 2013 at 09:21:51AM +0300, Felipe Balbi wrote:
> > How about we pass yours for not reading the patch before flaming ?
> 
> And another thing.  If you want to be flamed, continue with that tone.
> The problem here is that *you* have not been flamed in the previous
> message.  The previous message was all about pointing out an error in
> the code in a way that hopefully you will take notice of, because you
> showed no sign of taking any notice of my previous mail.
> 
> If you want to be a total dork, continue behaving as you currently are.
> There, *now* I *have* flamed you!

what a load of crap. Just because it wasn't pointed out in my previous
email doesn't automatically mean I haven't noticed. Anyway, I won't be
doing this all day.

cheers

-- 
balbi
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130503/660b6d29/attachment-0001.sig>

[PATCH] ARM: OMAP-USB: Fix possible memory leak

[Russell King - ARM Linux]

On Fri, May 03, 2013 at 11:39:00AM +0300, Felipe Balbi wrote:
> Hi,
> 
> On Fri, May 03, 2013 at 09:32:06AM +0100, Russell King - ARM Linux wrote:
> > On Fri, May 03, 2013 at 09:21:51AM +0300, Felipe Balbi wrote:
> > > How about we pass yours for not reading the patch before flaming ?
> > 
> > And another thing.  If you want to be flamed, continue with that tone.
> > The problem here is that *you* have not been flamed in the previous
> > message.  The previous message was all about pointing out an error in
> > the code in a way that hopefully you will take notice of, because you
> > showed no sign of taking any notice of my previous mail.
> > 
> > If you want to be a total dork, continue behaving as you currently are.
> > There, *now* I *have* flamed you!
> 
> what a load of crap. Just because it wasn't pointed out in my previous
> email doesn't automatically mean I haven't noticed. Anyway, I won't be
> doing this all day.

Yes you are a load of crap *you* spout about people flaming you.  Well, if
you can't be bothered to acknowledge a problem then you can expect to get
resends because it's not clear that the message has been received.  And
you do *not* call resends "flames" back to those who were kind enough put
the effort in to tell you about them more than once.

That's just down right insulting and flame provoking.  Which is exactly
how we ended up here.