* [PATCH] ARM: OMAP-USB: Fix possible memory leak @ 2013-05-02 16:03 Alexander Shiyan 2013-05-02 16:24 ` Nishanth Menon 0 siblings, 1 reply; 15+ messages in thread From: Alexander Shiyan @ 2013-05-02 16:03 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply related [flat|nested] 15+ messages in thread
* [PATCH] ARM: OMAP-USB: Fix possible memory leak 2013-05-02 16:03 [PATCH] ARM: OMAP-USB: Fix possible memory leak Alexander Shiyan @ 2013-05-02 16:24 ` Nishanth Menon 2013-05-02 16:28 ` Re[2]: " Alexander Shiyan 2013-05-02 19:52 ` Russell King - ARM Linux 0 siblings, 2 replies; 15+ messages in thread From: Nishanth Menon @ 2013-05-02 16:24 UTC (permalink / raw) To: linux-arm-kernel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re[2]: [PATCH] ARM: OMAP-USB: Fix possible memory leak 2013-05-02 16:24 ` Nishanth Menon @ 2013-05-02 16:28 ` Alexander Shiyan 2013-05-02 16:40 ` Felipe Balbi 2013-05-02 19:52 ` Russell King - ARM Linux 1 sibling, 1 reply; 15+ messages in thread From: Alexander Shiyan @ 2013-05-02 16:28 UTC (permalink / raw) To: linux-arm-kernel > 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. --- ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] ARM: OMAP-USB: Fix possible memory leak 2013-05-02 16:28 ` Re[2]: " Alexander Shiyan @ 2013-05-02 16:40 ` Felipe Balbi 2013-05-02 23:13 ` Russell King - ARM Linux 0 siblings, 1 reply; 15+ messages in thread From: Felipe Balbi @ 2013-05-02 16:40 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] ARM: OMAP-USB: Fix possible memory leak 2013-05-02 16:40 ` Felipe Balbi @ 2013-05-02 23:13 ` Russell King - ARM Linux 2013-05-03 6:21 ` Felipe Balbi 0 siblings, 1 reply; 15+ messages in thread From: Russell King - ARM Linux @ 2013-05-02 23:13 UTC (permalink / raw) To: linux-arm-kernel 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? :) ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] ARM: OMAP-USB: Fix possible memory leak 2013-05-02 23:13 ` Russell King - ARM Linux @ 2013-05-03 6:21 ` Felipe Balbi 2013-05-03 6:25 ` Felipe Balbi ` (2 more replies) 0 siblings, 3 replies; 15+ messages in thread From: Felipe Balbi @ 2013-05-03 6:21 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] ARM: OMAP-USB: Fix possible memory leak 2013-05-03 6:21 ` Felipe Balbi @ 2013-05-03 6:25 ` Felipe Balbi 2013-05-03 6:25 ` Felipe Balbi 2013-05-03 8:15 ` Russell King - ARM Linux 2013-05-03 8:32 ` Russell King - ARM Linux 2 siblings, 1 reply; 15+ messages in thread From: Felipe Balbi @ 2013-05-03 6:25 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] ARM: OMAP-USB: Fix possible memory leak 2013-05-03 6:25 ` Felipe Balbi @ 2013-05-03 6:25 ` Felipe Balbi 0 siblings, 0 replies; 15+ messages in thread From: Felipe Balbi @ 2013-05-03 6:25 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] ARM: OMAP-USB: Fix possible memory leak 2013-05-03 6:21 ` Felipe Balbi 2013-05-03 6:25 ` Felipe Balbi @ 2013-05-03 8:15 ` Russell King - ARM Linux 2013-05-03 8:30 ` Felipe Balbi 2013-05-03 8:32 ` Russell King - ARM Linux 2 siblings, 1 reply; 15+ messages in thread From: Russell King - ARM Linux @ 2013-05-03 8:15 UTC (permalink / raw) To: linux-arm-kernel 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". ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] ARM: OMAP-USB: Fix possible memory leak 2013-05-03 8:15 ` Russell King - ARM Linux @ 2013-05-03 8:30 ` Felipe Balbi 2013-05-03 8:33 ` Russell King - ARM Linux 0 siblings, 1 reply; 15+ messages in thread From: Felipe Balbi @ 2013-05-03 8:30 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] ARM: OMAP-USB: Fix possible memory leak 2013-05-03 8:30 ` Felipe Balbi @ 2013-05-03 8:33 ` Russell King - ARM Linux 0 siblings, 0 replies; 15+ messages in thread From: Russell King - ARM Linux @ 2013-05-03 8:33 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] ARM: OMAP-USB: Fix possible memory leak 2013-05-03 6:21 ` Felipe Balbi 2013-05-03 6:25 ` Felipe Balbi 2013-05-03 8:15 ` Russell King - ARM Linux @ 2013-05-03 8:32 ` Russell King - ARM Linux 2013-05-03 8:39 ` Felipe Balbi 2 siblings, 1 reply; 15+ messages in thread From: Russell King - ARM Linux @ 2013-05-03 8:32 UTC (permalink / raw) To: linux-arm-kernel 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! ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] ARM: OMAP-USB: Fix possible memory leak 2013-05-03 8:32 ` Russell King - ARM Linux @ 2013-05-03 8:39 ` Felipe Balbi 2013-05-03 8:43 ` Russell King - ARM Linux 0 siblings, 1 reply; 15+ messages in thread From: Felipe Balbi @ 2013-05-03 8:39 UTC (permalink / raw) To: linux-arm-kernel 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> ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] ARM: OMAP-USB: Fix possible memory leak 2013-05-03 8:39 ` Felipe Balbi @ 2013-05-03 8:43 ` Russell King - ARM Linux 0 siblings, 0 replies; 15+ messages in thread From: Russell King - ARM Linux @ 2013-05-03 8:43 UTC (permalink / raw) To: linux-arm-kernel 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. ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] ARM: OMAP-USB: Fix possible memory leak 2013-05-02 16:24 ` Nishanth Menon 2013-05-02 16:28 ` Re[2]: " Alexander Shiyan @ 2013-05-02 19:52 ` Russell King - ARM Linux 1 sibling, 0 replies; 15+ messages in thread From: Russell King - ARM Linux @ 2013-05-02 19:52 UTC (permalink / raw) To: linux-arm-kernel 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! ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2013-05-03 8:43 UTC | newest] Thread overview: 15+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-05-02 16:03 [PATCH] ARM: OMAP-USB: Fix possible memory leak Alexander Shiyan 2013-05-02 16:24 ` Nishanth Menon 2013-05-02 16:28 ` Re[2]: " Alexander Shiyan 2013-05-02 16:40 ` Felipe Balbi 2013-05-02 23:13 ` Russell King - ARM Linux 2013-05-03 6:21 ` Felipe Balbi 2013-05-03 6:25 ` Felipe Balbi 2013-05-03 6:25 ` Felipe Balbi 2013-05-03 8:15 ` Russell King - ARM Linux 2013-05-03 8:30 ` Felipe Balbi 2013-05-03 8:33 ` Russell King - ARM Linux 2013-05-03 8:32 ` Russell King - ARM Linux 2013-05-03 8:39 ` Felipe Balbi 2013-05-03 8:43 ` Russell King - ARM Linux 2013-05-02 19:52 ` Russell King - ARM Linux
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox