Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak
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. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak
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 signature.asc Description: Digital signature
Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak
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. -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak
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! -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak
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 signature.asc Description: Digital signature
Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak
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
> > > > > > ---
> > > > > > 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".
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak
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 signature.asc Description: Digital signature
Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak
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 signature.asc Description: Digital signature
Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak
Hi,
On Fri, May 03, 2013 at 12:13:53AM +0100, Russell King - ARM Linux wrote:
> > > > > Signed-off-by: Alexander Shiyan
> > > > > ---
> > > > > 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
signature.asc
Description: Digital signature
Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak
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
> > > > ---
> > > > 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? :)
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak
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
> > ---
> > 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!
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak
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
> > > ---
> > > 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
signature.asc
Description: Digital signature
Re[2]: [PATCH] ARM: OMAP-USB: Fix possible memory leak
> On 20:03-20130502, Alexander Shiyan wrote:
> >
> > Signed-off-by: Alexander Shiyan
> > ---
> > 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.
---
Re: [PATCH] ARM: OMAP-USB: Fix possible memory leak
On 20:03-20130502, Alexander Shiyan wrote:
>
> Signed-off-by: Alexander Shiyan
> ---
> 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-ker...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
--
Regards,
Nishanth Menon
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] ARM: OMAP-USB: Fix possible memory leak
Signed-off-by: Alexander Shiyan
---
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
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
