| Message ID | 1367510596-27780-1-git-send-email-shc_work@mail.ru (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/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. ---
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.
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!
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? :)
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.
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()).
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.
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".
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 ?
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!
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.
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
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.
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; }
Signed-off-by: Alexander Shiyan <shc_work@mail.ru> --- arch/arm/mach-omap2/usb-host.c | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-)