| Message ID | 1357371910-3164-3-git-send-email-ldewangan@nvidia.com |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Laxman, On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > Use devm_* for memory, clock, input device allocation. This reduces > code for freeing these resources. > > Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> > --- > Changes from V1: > None > > drivers/input/keyboard/tegra-kbc.c | 93 +++++++++++------------------------- > 1 files changed, 28 insertions(+), 65 deletions(-) > > diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c > index f1d3ba0..c036425 100644 > --- a/drivers/input/keyboard/tegra-kbc.c > +++ b/drivers/input/keyboard/tegra-kbc.c > @@ -618,7 +618,7 @@ static struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata( > if (!np) > return NULL; > > - pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); > + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); > if (!pdata) > return NULL; > > @@ -700,33 +700,36 @@ static int tegra_kbc_probe(struct platform_device *pdev) > if (!pdata) > pdata = tegra_kbc_dt_parse_pdata(pdev); > > - if (!pdata) > + if (!pdata) { > + dev_err(&pdev->dev, "Platform data missing\n"); > return -EINVAL; > - > - if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows)) { > - err = -EINVAL; > - goto err_free_pdata; > } > > + if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows)) > + return -EINVAL; > + > res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > if (!res) { > dev_err(&pdev->dev, "failed to get I/O memory\n"); > - err = -ENXIO; > - goto err_free_pdata; > + return -ENXIO; > } > > irq = platform_get_irq(pdev, 0); > if (irq < 0) { > dev_err(&pdev->dev, "failed to get keyboard IRQ\n"); > - err = -ENXIO; > - goto err_free_pdata; > + return -ENXIO; > + } > + > + kbc = devm_kzalloc(&pdev->dev, sizeof(*kbc), GFP_KERNEL); > + if (!kbc) { > + dev_err(&pdev->dev, "failed to alloc memory for kbc\n"); > + return -ENOMEM; > } > > - kbc = kzalloc(sizeof(*kbc), GFP_KERNEL); > - input_dev = input_allocate_device(); > - if (!kbc || !input_dev) { > - err = -ENOMEM; > - goto err_free_mem; > + input_dev = devm_input_allocate_device(&pdev->dev); > + if (!input_dev) { > + dev_err(&pdev->dev, "failed to allocate input device\n"); > + return -ENOMEM; > } > > kbc->pdata = pdata; > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > spin_lock_init(&kbc->lock); > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > - if (!res) { > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > - err = -EBUSY; > - goto err_free_mem; > - } > - > - kbc->mmio = ioremap(res->start, resource_size(res)); > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > if (!kbc->mmio) { > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > - err = -ENXIO; > - goto err_free_mem_region; > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > + return -EADDRNOTAVAIL; Erm, no, -EBUSY please. > } > > - kbc->clk = clk_get(&pdev->dev, NULL); > + kbc->clk = devm_clk_get(&pdev->dev, NULL); > if (IS_ERR(kbc->clk)) { > dev_err(&pdev->dev, "failed to get keyboard clock\n"); > - err = PTR_ERR(kbc->clk); > - goto err_iounmap; > + return PTR_ERR(kbc->clk); > } > > /* > @@ -778,9 +772,9 @@ static int tegra_kbc_probe(struct platform_device *pdev) > input_dev->close = tegra_kbc_close; > > err = tegra_kbd_setup_keymap(kbc); > - if (err) { > + if (err < 0) { Why is this change? As far as I can see tegra_kbd_setup_keymap() never returns positive values. > dev_err(&pdev->dev, "failed to setup keymap\n"); > - goto err_put_clk; > + return err; > } > > __set_bit(EV_REP, input_dev->evbit); > @@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > err = request_irq(kbc->irq, tegra_kbc_isr, > IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc); > - if (err) { > + if (err < 0) { Neither request_irq(). BTW, why not devm_request_irq? > dev_err(&pdev->dev, "failed to request keyboard IRQ\n"); > - goto err_put_clk; > + return err; > } > > disable_irq(kbc->irq); > > err = input_register_device(kbc->idev); > - if (err) { > + if (err < 0) { Again, input_register_device() returns 0 on success. > dev_err(&pdev->dev, "failed to register input device\n"); > goto err_free_irq; > } > @@ -810,46 +804,15 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > err_free_irq: > free_irq(kbc->irq, pdev); > -err_put_clk: > - clk_put(kbc->clk); > -err_iounmap: > - iounmap(kbc->mmio); > -err_free_mem_region: > - release_mem_region(res->start, resource_size(res)); > -err_free_mem: > - input_free_device(input_dev); > - kfree(kbc); > -err_free_pdata: > - if (!pdev->dev.platform_data) > - kfree(pdata); > - > return err; > } > > static int tegra_kbc_remove(struct platform_device *pdev) > { > struct tegra_kbc *kbc = platform_get_drvdata(pdev); > - struct resource *res; > - > - platform_set_drvdata(pdev, NULL); > > free_irq(kbc->irq, pdev); > - clk_put(kbc->clk); > - > input_unregister_device(kbc->idev); You do not need to call input_unregister_device for managed input devices, and if you switch request_irq to managed variant you can remove tegra_kbc_remove() altogether. > - iounmap(kbc->mmio); > - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); > - release_mem_region(res->start, resource_size(res)); > - > - /* > - * If we do not have platform data attached to the device we > - * allocated it ourselves and thus need to free it. > - */ > - if (!pdev->dev.platform_data) > - kfree(kbc->pdata); > - > - kfree(kbc); > - > return 0; > } > > -- > 1.7.1.1 > Thanks.
HI Dmitry, Thanks for quick review. I will take care of your comment in next version. Some have my answer. On Saturday 05 January 2013 01:36 PM, Dmitry Torokhov wrote: > Hi Laxman, > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: >> Use devm_* for memory, clock, input device allocation. This reduces >> code for freeing these resources. >> err = tegra_kbd_setup_keymap(kbc); >> - if (err) { >> + if (err < 0) { > Why is this change? As far as I can see tegra_kbd_setup_keymap() never > returns positive values. Ok, mostly errors are in negative and hence this change, I will revert it and will keep original. > >> dev_err(&pdev->dev, "failed to setup keymap\n"); >> - goto err_put_clk; >> + return err; >> } >> >> __set_bit(EV_REP, input_dev->evbit); >> @@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev) >> >> err = request_irq(kbc->irq, tegra_kbc_isr, >> IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc); >> - if (err) { >> + if (err < 0) { > Neither request_irq(). BTW, why not devm_request_irq? I understand from Mark B on different patches that using devm_request_irq() can create race condition when removing device. Interrupt can occur when device resource release is in process and so it can cause isr call which can use the freed pointer. devm_request_irq() should be avoided. Thanks, Laxman -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jan 05, 2013 at 04:50:58PM +0530, Laxman Dewangan wrote: > HI Dmitry, > Thanks for quick review. > > I will take care of your comment in next version. Some have my answer. > > > On Saturday 05 January 2013 01:36 PM, Dmitry Torokhov wrote: > >Hi Laxman, > > > >On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > >>Use devm_* for memory, clock, input device allocation. This reduces > >>code for freeing these resources. > > >> err = tegra_kbd_setup_keymap(kbc); > >>- if (err) { > >>+ if (err < 0) { > >Why is this change? As far as I can see tegra_kbd_setup_keymap() never > >returns positive values. > > Ok, mostly errors are in negative and hence this change, I will > revert it and will keep original. > > > > >> dev_err(&pdev->dev, "failed to setup keymap\n"); > >>- goto err_put_clk; > >>+ return err; > >> } > >> __set_bit(EV_REP, input_dev->evbit); > >>@@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev) > >> err = request_irq(kbc->irq, tegra_kbc_isr, > >> IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc); > >>- if (err) { > >>+ if (err < 0) { > >Neither request_irq(). BTW, why not devm_request_irq? > > I understand from Mark B on different patches that using > devm_request_irq() can create race condition when removing device. > Interrupt can occur when device resource release is in process and > so it can cause isr call which can use the freed pointer. > devm_request_irq() should be avoided. devm_request_irq() has a potential of creating a race condition, but it depents on the driver. In this particular case tegra driver ensures that interrupts are inhibited when input device is unregistered by providing tegra_kbc_close() method, so in this particular case it is safe to use devm_request_irq(). Also, when using managed input devices, the unregistering and final freeing is a 2-step process, so even in absence of close() method, if initialization sequence was: devm_input_allocate_device() ... devm_request_irq() ... input_unregister_device() then order of freeing resources (behind the scenes) will be devm_input_device_unregister(); /* input device is still present in memory and can * handle input_event() calls. */ free_irq(); devm_input_device_release(); So using managed request_irq() _together_ with managed input devices is OK. Thanks.
On Sunday 06 January 2013 04:48 AM, Dmitry Torokhov wrote: > On Sat, Jan 05, 2013 at 04:50:58PM +0530, Laxman Dewangan wrote: >> HI Dmitry, >> Thanks for quick review. >> >> I will take care of your comment in next version. Some have my answer. >> >> >> On Saturday 05 January 2013 01:36 PM, Dmitry Torokhov wrote: >>> Hi Laxman, >>> >>> On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: >>>> Use devm_* for memory, clock, input device allocation. This reduces >>>> code for freeing these resources. >>>> err = tegra_kbd_setup_keymap(kbc); >>>> - if (err) { >>>> + if (err < 0) { >>> Why is this change? As far as I can see tegra_kbd_setup_keymap() never >>> returns positive values. >> Ok, mostly errors are in negative and hence this change, I will >> revert it and will keep original. >> >>>> dev_err(&pdev->dev, "failed to setup keymap\n"); >>>> - goto err_put_clk; >>>> + return err; >>>> } >>>> __set_bit(EV_REP, input_dev->evbit); >>>> @@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev) >>>> err = request_irq(kbc->irq, tegra_kbc_isr, >>>> IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc); >>>> - if (err) { >>>> + if (err < 0) { >>> Neither request_irq(). BTW, why not devm_request_irq? >> I understand from Mark B on different patches that using >> devm_request_irq() can create race condition when removing device. >> Interrupt can occur when device resource release is in process and >> so it can cause isr call which can use the freed pointer. >> devm_request_irq() should be avoided. > devm_request_irq() has a potential of creating a race condition, but it > depents on the driver. In this particular case tegra driver ensures that > interrupts are inhibited when input device is unregistered by providing > tegra_kbc_close() method, so in this particular case it is safe to > use devm_request_irq(). > > Also, when using managed input devices, the unregistering and final > freeing is a 2-step process, so even in absence of close() method, if > initialization sequence was: > > devm_input_allocate_device() > ... > devm_request_irq() > ... > input_unregister_device() > > then order of freeing resources (behind the scenes) will be > > devm_input_device_unregister(); > /* input device is still present in memory and can > * handle input_event() calls. > */ > free_irq(); > devm_input_device_release(); > > So using managed request_irq() _together_ with managed input devices is > OK. Thanks for detail explanation. I will modify and sent the new version of patch. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: [...] > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > spin_lock_init(&kbc->lock); > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > - if (!res) { > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > - err = -EBUSY; > > - goto err_free_mem; > > - } > > - > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > if (!kbc->mmio) { > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > - err = -ENXIO; > > - goto err_free_mem_region; > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > + return -EADDRNOTAVAIL; > > Erm, no, -EBUSY please. EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() failure. The kerneldoc comment in lib/devres.c even gives a short example that uses this error code. Thierry
On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > [...] > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > spin_lock_init(&kbc->lock); > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > - if (!res) { > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > - err = -EBUSY; > > > - goto err_free_mem; > > > - } > > > - > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > if (!kbc->mmio) { > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > - err = -ENXIO; > > > - goto err_free_mem_region; > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > + return -EADDRNOTAVAIL; > > > > Erm, no, -EBUSY please. > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > failure. The kerneldoc comment in lib/devres.c even gives a short > example that uses this error code. I am sorry, but I do not consider a function that was added a little over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it is used predominantly in networking code to indicate that attempted _network_ address is not available. Thanks.
On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote: > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > > [...] > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > > spin_lock_init(&kbc->lock); > > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > > - if (!res) { > > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > > - err = -EBUSY; > > > > - goto err_free_mem; > > > > - } > > > > - > > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > > if (!kbc->mmio) { > > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > - err = -ENXIO; > > > > - goto err_free_mem_region; > > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > > + return -EADDRNOTAVAIL; > > > > > > Erm, no, -EBUSY please. > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > > failure. The kerneldoc comment in lib/devres.c even gives a short > > example that uses this error code. > > I am sorry, but I do not consider a function that was added a little > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > is used predominantly in networking code to indicate that attempted > _network_ address is not available. EBUSY might be misleading, though. devm_request_and_ioremap() can fail in both the request_mem_region() and ioremap() calls. Furthermore it'd be good to settle on a consistent error-code instead of doing it differently depending on subsystem and/or driver. Currently the various error codes used are: EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, EIO, EFAULT, EADDRINUSE Also if we can settle on one error code we should follow up with a patch to make it consistent across the tree and also update that kerneldoc comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing Wolfram (the original author), maybe he has some thoughts on this. Thierry
On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote: > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote: > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > > > [...] > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > > > spin_lock_init(&kbc->lock); > > > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > > > - if (!res) { > > > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > > > - err = -EBUSY; > > > > > - goto err_free_mem; > > > > > - } > > > > > - > > > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > > > if (!kbc->mmio) { > > > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > > - err = -ENXIO; > > > > > - goto err_free_mem_region; > > > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > > > + return -EADDRNOTAVAIL; > > > > > > > > Erm, no, -EBUSY please. > > > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > > > failure. The kerneldoc comment in lib/devres.c even gives a short > > > example that uses this error code. > > > > I am sorry, but I do not consider a function that was added a little > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > is used predominantly in networking code to indicate that attempted > > _network_ address is not available. > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > in both the request_mem_region() and ioremap() calls. Furthermore it'd > be good to settle on a consistent error-code instead of doing it > differently depending on subsystem and/or driver. Currently the various > error codes used are: > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > EIO, EFAULT, EADDRINUSE > > Also if we can settle on one error code we should follow up with a patch > to make it consistent across the tree and also update that kerneldoc > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > Wolfram (the original author), maybe he has some thoughts on this. > If you going to change all drivers make devm_request_and_ioremap() return ERR_PTR()-encoded errors and then we can differentiate what part of it failed. Thanks.
On Wed, Jan 09, 2013 at 01:19:39AM -0800, Dmitry Torokhov wrote: > On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote: > > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote: > > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > > > > [...] > > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > > > > spin_lock_init(&kbc->lock); > > > > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > > > > - if (!res) { > > > > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > > > > - err = -EBUSY; > > > > > > - goto err_free_mem; > > > > > > - } > > > > > > - > > > > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > > > > if (!kbc->mmio) { > > > > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > > > - err = -ENXIO; > > > > > > - goto err_free_mem_region; > > > > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > > > > + return -EADDRNOTAVAIL; > > > > > > > > > > Erm, no, -EBUSY please. > > > > > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > > > > failure. The kerneldoc comment in lib/devres.c even gives a short > > > > example that uses this error code. > > > > > > I am sorry, but I do not consider a function that was added a little > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > > is used predominantly in networking code to indicate that attempted > > > _network_ address is not available. > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > > in both the request_mem_region() and ioremap() calls. Furthermore it'd > > be good to settle on a consistent error-code instead of doing it > > differently depending on subsystem and/or driver. Currently the various > > error codes used are: > > > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > > EIO, EFAULT, EADDRINUSE > > > > Also if we can settle on one error code we should follow up with a patch > > to make it consistent across the tree and also update that kerneldoc > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > > Wolfram (the original author), maybe he has some thoughts on this. > > > > If you going to change all drivers make devm_request_and_ioremap() > return ERR_PTR()-encoded errors and then we can differentiate what > part of it failed. Yeah, that thought also crossed my mind. I'll give other people some time to comment before hurling myself into preparing patches. Thierry
On Wed, Jan 09, 2013 at 10:23:52AM +0100, Thierry Reding wrote: > On Wed, Jan 09, 2013 at 01:19:39AM -0800, Dmitry Torokhov wrote: > > On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote: > > > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote: > > > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > > > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > > > > > [...] > > > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > > > > > spin_lock_init(&kbc->lock); > > > > > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > > > > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > > > > > - if (!res) { > > > > > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > > > > > - err = -EBUSY; > > > > > > > - goto err_free_mem; > > > > > > > - } > > > > > > > - > > > > > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > > > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > > > > > if (!kbc->mmio) { > > > > > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > > > > - err = -ENXIO; > > > > > > > - goto err_free_mem_region; > > > > > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > > > > > + return -EADDRNOTAVAIL; > > > > > > > > > > > > Erm, no, -EBUSY please. > > > > > > > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > > > > > failure. The kerneldoc comment in lib/devres.c even gives a short > > > > > example that uses this error code. > > > > > > > > I am sorry, but I do not consider a function that was added a little > > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > > > is used predominantly in networking code to indicate that attempted > > > > _network_ address is not available. > > > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > > > in both the request_mem_region() and ioremap() calls. Furthermore it'd > > > be good to settle on a consistent error-code instead of doing it > > > differently depending on subsystem and/or driver. Currently the various > > > error codes used are: > > > > > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > > > EIO, EFAULT, EADDRINUSE > > > > > > Also if we can settle on one error code we should follow up with a patch > > > to make it consistent across the tree and also update that kerneldoc > > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > > > Wolfram (the original author), maybe he has some thoughts on this. > > > > > > > If you going to change all drivers make devm_request_and_ioremap() > > return ERR_PTR()-encoded errors and then we can differentiate what > > part of it failed. > > Yeah, that thought also crossed my mind. I'll give other people some > time to comment before hurling myself into preparing patches. I've prepared a patch that changes devm_request_and_ioremap() to return ERR_PTR()-encoded errors and adjusts all callers. As you can imagine it is a bit on the huge side. scripts/get_maintainer.pl lists 156 people and mailing lists. I've gone through the list, and as far as I can tell everyone on that list is actually affected by the patch, so there's not much potential for tuning it down. There is also the issue of whose tree this should go into. Unfortunately the patch can't be broken down into smaller chunks because it changes how the devm_request_and_ioremap() function's return value is handled in an incompatible way, so it won't be possible to gradually phase this in. Furthermore I can imagine that until the end of the release cycle new code may be added on which the same transformations need to be done. I have a semantic patch to do the bulk of the work, but quite a bit of coordination will be required. I'm adding Arnd and Greg on Cc, maybe they can advise on how best to handle this kind of patch. Thierry
On Mon, Jan 14, 2013 at 04:49:59PM +0100, Thierry Reding wrote: > On Wed, Jan 09, 2013 at 10:23:52AM +0100, Thierry Reding wrote: > > On Wed, Jan 09, 2013 at 01:19:39AM -0800, Dmitry Torokhov wrote: > > > On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote: > > > > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote: > > > > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > > > > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > > > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > > > > > > [...] > > > > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > > > > > > spin_lock_init(&kbc->lock); > > > > > > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > > > > > > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > > > > > > - if (!res) { > > > > > > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > > > > > > - err = -EBUSY; > > > > > > > > - goto err_free_mem; > > > > > > > > - } > > > > > > > > - > > > > > > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > > > > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > > > > > > if (!kbc->mmio) { > > > > > > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > > > > > - err = -ENXIO; > > > > > > > > - goto err_free_mem_region; > > > > > > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > > > > > > + return -EADDRNOTAVAIL; > > > > > > > > > > > > > > Erm, no, -EBUSY please. > > > > > > > > > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > > > > > > failure. The kerneldoc comment in lib/devres.c even gives a short > > > > > > example that uses this error code. > > > > > > > > > > I am sorry, but I do not consider a function that was added a little > > > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > > > > is used predominantly in networking code to indicate that attempted > > > > > _network_ address is not available. > > > > > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > > > > in both the request_mem_region() and ioremap() calls. Furthermore it'd > > > > be good to settle on a consistent error-code instead of doing it > > > > differently depending on subsystem and/or driver. Currently the various > > > > error codes used are: > > > > > > > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > > > > EIO, EFAULT, EADDRINUSE > > > > > > > > Also if we can settle on one error code we should follow up with a patch > > > > to make it consistent across the tree and also update that kerneldoc > > > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > > > > Wolfram (the original author), maybe he has some thoughts on this. > > > > > > > > > > If you going to change all drivers make devm_request_and_ioremap() > > > return ERR_PTR()-encoded errors and then we can differentiate what > > > part of it failed. > > > > Yeah, that thought also crossed my mind. I'll give other people some > > time to comment before hurling myself into preparing patches. > > I've prepared a patch that changes devm_request_and_ioremap() to return > ERR_PTR()-encoded errors and adjusts all callers. As you can imagine it > is a bit on the huge side. scripts/get_maintainer.pl lists 156 people > and mailing lists. I've gone through the list, and as far as I can tell > everyone on that list is actually affected by the patch, so there's not > much potential for tuning it down. > > There is also the issue of whose tree this should go into. Unfortunately > the patch can't be broken down into smaller chunks because it changes > how the devm_request_and_ioremap() function's return value is handled in > an incompatible way, so it won't be possible to gradually phase this in. > Furthermore I can imagine that until the end of the release cycle new > code may be added on which the same transformations need to be done. I > have a semantic patch to do the bulk of the work, but quite a bit of > coordination will be required. > > I'm adding Arnd and Greg on Cc, maybe they can advise on how best to > handle this kind of patch. You should provide a "wrapper" function that does the correct return value, convert drivers over to it, and then, when everyone is changed, drop the old call. To change 156 different drivers all at once, in a way that is not detectable by the compiler breaking the build, is not a good thing to do at all. By doing it in this manner, it will take longer, but you can push the patches through the different maintainer's trees. If they are slow, I'll be glad to take the remaining patches in my driver-core tree to do the final bits. Hope this helps, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 14, 2013 at 08:16:44AM -0800, Greg Kroah-Hartman wrote: > On Mon, Jan 14, 2013 at 04:49:59PM +0100, Thierry Reding wrote: > > On Wed, Jan 09, 2013 at 10:23:52AM +0100, Thierry Reding wrote: > > > On Wed, Jan 09, 2013 at 01:19:39AM -0800, Dmitry Torokhov wrote: > > > > On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote: > > > > > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote: > > > > > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > > > > > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > > > > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > > > > > > > [...] > > > > > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > > > > > > > spin_lock_init(&kbc->lock); > > > > > > > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > > > > > > > > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > > > > > > > - if (!res) { > > > > > > > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > > > > > > > - err = -EBUSY; > > > > > > > > > - goto err_free_mem; > > > > > > > > > - } > > > > > > > > > - > > > > > > > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > > > > > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > > > > > > > if (!kbc->mmio) { > > > > > > > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > > > > > > - err = -ENXIO; > > > > > > > > > - goto err_free_mem_region; > > > > > > > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > > > > > > > + return -EADDRNOTAVAIL; > > > > > > > > > > > > > > > > Erm, no, -EBUSY please. > > > > > > > > > > > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > > > > > > > failure. The kerneldoc comment in lib/devres.c even gives a short > > > > > > > example that uses this error code. > > > > > > > > > > > > I am sorry, but I do not consider a function that was added a little > > > > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > > > > > is used predominantly in networking code to indicate that attempted > > > > > > _network_ address is not available. > > > > > > > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > > > > > in both the request_mem_region() and ioremap() calls. Furthermore it'd > > > > > be good to settle on a consistent error-code instead of doing it > > > > > differently depending on subsystem and/or driver. Currently the various > > > > > error codes used are: > > > > > > > > > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > > > > > EIO, EFAULT, EADDRINUSE > > > > > > > > > > Also if we can settle on one error code we should follow up with a patch > > > > > to make it consistent across the tree and also update that kerneldoc > > > > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > > > > > Wolfram (the original author), maybe he has some thoughts on this. > > > > > > > > > > > > > If you going to change all drivers make devm_request_and_ioremap() > > > > return ERR_PTR()-encoded errors and then we can differentiate what > > > > part of it failed. > > > > > > Yeah, that thought also crossed my mind. I'll give other people some > > > time to comment before hurling myself into preparing patches. > > > > I've prepared a patch that changes devm_request_and_ioremap() to return > > ERR_PTR()-encoded errors and adjusts all callers. As you can imagine it > > is a bit on the huge side. scripts/get_maintainer.pl lists 156 people > > and mailing lists. I've gone through the list, and as far as I can tell > > everyone on that list is actually affected by the patch, so there's not > > much potential for tuning it down. > > > > There is also the issue of whose tree this should go into. Unfortunately > > the patch can't be broken down into smaller chunks because it changes > > how the devm_request_and_ioremap() function's return value is handled in > > an incompatible way, so it won't be possible to gradually phase this in. > > Furthermore I can imagine that until the end of the release cycle new > > code may be added on which the same transformations need to be done. I > > have a semantic patch to do the bulk of the work, but quite a bit of > > coordination will be required. > > > > I'm adding Arnd and Greg on Cc, maybe they can advise on how best to > > handle this kind of patch. > > You should provide a "wrapper" function that does the correct return > value, convert drivers over to it, and then, when everyone is changed, > drop the old call. To change 156 different drivers all at once, in a > way that is not detectable by the compiler breaking the build, is not a > good thing to do at all. > > By doing it in this manner, it will take longer, but you can push the > patches through the different maintainer's trees. If they are slow, > I'll be glad to take the remaining patches in my driver-core tree to do > the final bits. It certainly sounds like a less complicated way to do it. But it also involves adding a function with a made up name and drop a function with a perfectly good name instead. I wouldn't even know what name to choose for the new API. Thierry
On Monday 14 January 2013, Thierry Reding wrote: > It certainly sounds like a less complicated way to do it. But it also > involves adding a function with a made up name and drop a function with > a perfectly good name instead. I wouldn't even know what name to choose > for the new API. > How about devm_ioremap_resource()? Sounds equally fitting, and is shorter. Arnd -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jan 14, 2013 at 10:24:11PM +0000, Arnd Bergmann wrote: > On Monday 14 January 2013, Thierry Reding wrote: > > It certainly sounds like a less complicated way to do it. But it also > > involves adding a function with a made up name and drop a function with > > a perfectly good name instead. I wouldn't even know what name to choose > > for the new API. > > > > How about devm_ioremap_resource()? Sounds equally fitting, and is shorter. Yes, that sounds good. Thanks for the suggestion. Thierry
On Mon, Jan 14, 2013 at 10:24:11PM +0000, Arnd Bergmann wrote: > On Monday 14 January 2013, Thierry Reding wrote: > > It certainly sounds like a less complicated way to do it. But it also > > involves adding a function with a made up name and drop a function with > > a perfectly good name instead. I wouldn't even know what name to choose > > for the new API. > > > > How about devm_ioremap_resource()? Sounds equally fitting, and is shorter. 2 cents, I wrote back then: * the new function is not named 'devm_ioremap_resource' because it does more than just ioremap, namely request_mem_region. I didn't want to create implicit knowledge here.
Hi, > > > > > I am sorry, but I do not consider a function that was added a little > > > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > > > > is used predominantly in networking code to indicate that attempted > > > > > _network_ address is not available. > > > > > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > > > > in both the request_mem_region() and ioremap() calls. Furthermore it'd > > > > be good to settle on a consistent error-code instead of doing it > > > > differently depending on subsystem and/or driver. Currently the various > > > > error codes used are: > > > > > > > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > > > > EIO, EFAULT, EADDRINUSE > > > > > > > > Also if we can settle on one error code we should follow up with a patch > > > > to make it consistent across the tree and also update that kerneldoc > > > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > > > > Wolfram (the original author), maybe he has some thoughts on this. Handling the error case was the biggest discussion back then. I initially did not want to use ERR_PTR, because I see already enough patches adding a forgotten ERR_PTR to drivers. My initial idea was to return a simple errno and have the pointer a function argument. I was convinced [1], however, that the dev_err printout is enough to make visible what actually went wrong and return a NULL pointer instead. So much for why the function does NOT return a PTR_ERR, and I still prefer that. Then, I added the example code in the documentation using EADDRNOTAVAIL. Yes, I was brave with this one. Yet, EINVAL, EBUSY, ENOENT, did not really cut it and are so heavily used in drivers that they turned into a generic "something is wrong" error. I tried here to use a not overloaded error code in order to be specific again. Since the patches were accepted, I assumed it wasn't seen as a namespace violation. (Then again, it probably would have been if that error code would go out to userspace) Naturally, I didn't have the resources to check all patches for a consistent error code. > > > If you going to change all drivers make devm_request_and_ioremap() > > > return ERR_PTR()-encoded errors and then we can differentiate what > > > part of it failed. > > > > Yeah, that thought also crossed my mind. I'll give other people some > > time to comment before hurling myself into preparing patches. As said above, that was argued away when committing the patches. But there is more to that: When working with this function, there was also the idea to abstract getting the resource away. Which then gave Sascha Hauer and me the question, if drivers really have to do this or if this couldn't be done by the kernel somehow, i.e. giving the drivers already the resources they need, completely prepared. Of course, then we would need a similar function for interrupt resources. Which has much bigger problem with return codes, since we then step into the area of the "0 is no interrupt" topic (while platform_get_irq returns an error code). As a result, I got the impression that the whole topic needs ONE concentrated, major rehaul or at least a master plan. Adding an idea here and there doesn't seem to cut it, at least not in the way devm_request_and_ioremap() was done. I would be interested in doing that but my resources don't allow me to even think about it at the moment, sadly. Regards, Wolfram [1] http://lkml.org/lkml/2011/10/24/278
On Tue, Jan 15, 2013 at 02:06:23PM +0100, Wolfram Sang wrote: > Hi, > > > > > > > I am sorry, but I do not consider a function that was added a little > > > > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > > > > > is used predominantly in networking code to indicate that attempted > > > > > > _network_ address is not available. > > > > > > > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > > > > > in both the request_mem_region() and ioremap() calls. Furthermore it'd > > > > > be good to settle on a consistent error-code instead of doing it > > > > > differently depending on subsystem and/or driver. Currently the various > > > > > error codes used are: > > > > > > > > > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > > > > > EIO, EFAULT, EADDRINUSE > > > > > > > > > > Also if we can settle on one error code we should follow up with a patch > > > > > to make it consistent across the tree and also update that kerneldoc > > > > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > > > > > Wolfram (the original author), maybe he has some thoughts on this. > > Handling the error case was the biggest discussion back then. I > initially did not want to use ERR_PTR, because I see already enough > patches adding a forgotten ERR_PTR to drivers. My initial idea was to > return a simple errno and have the pointer a function argument. I was > convinced [1], however, that the dev_err printout is enough to make > visible what actually went wrong and return a NULL pointer instead. So > much for why the function does NOT return a PTR_ERR, and I still prefer > that. > > Then, I added the example code in the documentation using EADDRNOTAVAIL. > Yes, I was brave with this one. Yet, EINVAL, EBUSY, ENOENT, did not > really cut it and are so heavily used in drivers that they turned into a > generic "something is wrong" error. I tried here to use a not overloaded > error code in order to be specific again. Since the patches were > accepted, I assumed it wasn't seen as a namespace violation. (Then > again, it probably would have been if that error code would go out to > userspace) Naturally, I didn't have the resources to check all patches > for a consistent error code. The problem with the current approach is that people (me included) keep telling people to use this or that error code in an attempt to achieve some kind of consistency. Also using an error message to distinguish between reasons for failure makes it impossible to handle the error other than by visual inspection. Granted, there are currently no code paths that require this. One problem with the original patch was also that it didn't actually convert any existing uses, so there was little chance of anyone noticing potential problems. More than a year later this function is used by many subsystems and a lot of drivers. It just so happened that I observed how many people just didn't know what error codes to choose and often just grabbing one randomly. By adding devm_ioremap_resource() and having it return ERR_PTR()-encoded error codes we get rid of all these problems and put the responsibility for choosing the error code where, in my opinion, it belongs: the failing function. > > > > If you going to change all drivers make devm_request_and_ioremap() > > > > return ERR_PTR()-encoded errors and then we can differentiate what > > > > part of it failed. > > > > > > Yeah, that thought also crossed my mind. I'll give other people some > > > time to comment before hurling myself into preparing patches. > > As said above, that was argued away when committing the patches. > > But there is more to that: > > When working with this function, there was also the idea to abstract > getting the resource away. Which then gave Sascha Hauer and me the > question, if drivers really have to do this or if this couldn't be done > by the kernel somehow, i.e. giving the drivers already the resources > they need, completely prepared. I'm not sure I like that very much. That could possibly lead to a new problem where drivers that need to do something special have to jump through hoops to achieve something that may otherwise be simple. Anyway, if people don't think this is a sensible conversion I should waste no more time on it. On the other hand I have the patch series ready so I might as well post it for broader review. Thierry
Hi, > > Then, I added the example code in the documentation using EADDRNOTAVAIL. > > Yes, I was brave with this one. Yet, EINVAL, EBUSY, ENOENT, did not > > really cut it and are so heavily used in drivers that they turned into a > > generic "something is wrong" error. I tried here to use a not overloaded > > error code in order to be specific again. Since the patches were > > accepted, I assumed it wasn't seen as a namespace violation. (Then > > again, it probably would have been if that error code would go out to > > userspace) Naturally, I didn't have the resources to check all patches > > for a consistent error code. > > The problem with the current approach is that people (me included) keep > telling people to use this or that error code in an attempt to achieve > some kind of consistency. Also using an error message to distinguish > between reasons for failure makes it impossible to handle the error > other than by visual inspection. Granted, there are currently no code > paths that require this. Yes, so currently, this is rather a cosmetic change. And for that, it is quite intrusive. I think something like this is needed somewhen as part of a bigger overhaul. That's what I called "master plan" last time. That could be that resource handling gets aligned in general, also taking e.g. interrupt resources into account. But only changing error code handling for this function, doesn't seem too useful to me and might even need another change later, then. > One problem with the original patch was also that it didn't actually > convert any existing uses, so there was little chance of anyone noticing Like with the error codes now, there are so many different ways of handling resources that I did not want to mass convert all the drivers without being able to test them. I was hoping for a migration over time with patches from people who really tested what they did. > potential problems. More than a year later this function is used by many > subsystems and a lot of drivers. It just so happened that I observed how > many people just didn't know what error codes to choose and often just > grabbing one randomly. Yes, and concerning resources this needs cleaning on a bigger scale IMO. > By adding devm_ioremap_resource() and having it return ERR_PTR()-encoded > error codes we get rid of all these problems and put the responsibility > for choosing the error code where, in my opinion, it belongs: the > failing function. For completeness, it leaves the problem that people might forget to use ERR_PTR (seen that often in the clk framework). And the change is huge while I can't see any real benefit right now. > > When working with this function, there was also the idea to abstract > > getting the resource away. Which then gave Sascha Hauer and me the > > question, if drivers really have to do this or if this couldn't be done > > by the kernel somehow, i.e. giving the drivers already the resources > > they need, completely prepared. > > I'm not sure I like that very much. That could possibly lead to a new > problem where drivers that need to do something special have to jump > through hoops to achieve something that may otherwise be simple. I assume that drivers are already doing something special :) So, that would turn up in the conversion process. I can't promise that it would really work, it would need some playing around. > Anyway, if people don't think this is a sensible conversion I should > waste no more time on it. On the other hand I have the patch series > ready so I might as well post it for broader review. Working on patches is hardly a waste. Even if not accepted, you gain understanding. Please put me on CC, if you post the patches. Regards, Wolfram
On Wed, 9 Jan 2013 01:19:39 -0800, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Wed, Jan 09, 2013 at 08:07:45AM +0100, Thierry Reding wrote: > > On Sun, Jan 06, 2013 at 11:57:48AM -0800, Dmitry Torokhov wrote: > > > On Sun, Jan 06, 2013 at 08:27:39PM +0100, Thierry Reding wrote: > > > > On Sat, Jan 05, 2013 at 12:06:58AM -0800, Dmitry Torokhov wrote: > > > > > On Sat, Jan 05, 2013 at 01:15:08PM +0530, Laxman Dewangan wrote: > > > > [...] > > > > > > @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) > > > > > > spin_lock_init(&kbc->lock); > > > > > > setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); > > > > > > > > > > > > - res = request_mem_region(res->start, resource_size(res), pdev->name); > > > > > > - if (!res) { > > > > > > - dev_err(&pdev->dev, "failed to request I/O memory\n"); > > > > > > - err = -EBUSY; > > > > > > - goto err_free_mem; > > > > > > - } > > > > > > - > > > > > > - kbc->mmio = ioremap(res->start, resource_size(res)); > > > > > > + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); > > > > > > if (!kbc->mmio) { > > > > > > - dev_err(&pdev->dev, "failed to remap I/O memory\n"); > > > > > > - err = -ENXIO; > > > > > > - goto err_free_mem_region; > > > > > > + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); > > > > > > + return -EADDRNOTAVAIL; > > > > > > > > > > Erm, no, -EBUSY please. > > > > > > > > EADDRNOTAVAIL is the canonical error for devm_request_and_ioremap() > > > > failure. The kerneldoc comment in lib/devres.c even gives a short > > > > example that uses this error code. > > > > > > I am sorry, but I do not consider a function that was added a little > > > over a year ago as a canon. If you look at the uses of EADDRNOTAVAIL it > > > is used predominantly in networking code to indicate that attempted > > > _network_ address is not available. > > > > EBUSY might be misleading, though. devm_request_and_ioremap() can fail > > in both the request_mem_region() and ioremap() calls. Furthermore it'd > > be good to settle on a consistent error-code instead of doing it > > differently depending on subsystem and/or driver. Currently the various > > error codes used are: > > > > EBUSY, EADDRNOTAVAIL, ENXIO, ENOMEM, ENODEV, ENOENT, EINVAL, > > EIO, EFAULT, EADDRINUSE > > > > Also if we can settle on one error code we should follow up with a patch > > to make it consistent across the tree and also update that kerneldoc > > comment. I volunteer to do that if nobody else steps up. I'm also Cc'ing > > Wolfram (the original author), maybe he has some thoughts on this. This really doesn't matter. As far as the driver is concerned if the memory isn't available then it is a failure. Period. Who cares what the reason was. > If you going to change all drivers make devm_request_and_ioremap() > return ERR_PTR()-encoded errors and then we can differentiate what > part of it failed. The ERR_PTR() pattern is actually worse when it comes to reading and understanding code. Us C coders have had beaten into us that the correct test for an invalid pointer is "if (!ptr)". ERR_PTR() is actively dangerous in this regard because it breaks that pattern, but you cannot tell from looking at the API that it is wrong. There are places where it makes sense, but *please* don't use the ERR_PTR pattern unless absolutely necessary. Rarely are the actual error codes important for kernel internal stuff. The error codes only really matter when passing them up to userspace where they have specific meanings. As far as in-kernel stuff goes, the policy for choosing error codes consists of "go look at the errno file and choose one that looks vaguely relevant". g. -- To unsubscribe from this list: send the line "unsubscribe linux-tegra" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/input/keyboard/tegra-kbc.c b/drivers/input/keyboard/tegra-kbc.c index f1d3ba0..c036425 100644 --- a/drivers/input/keyboard/tegra-kbc.c +++ b/drivers/input/keyboard/tegra-kbc.c @@ -618,7 +618,7 @@ static struct tegra_kbc_platform_data *tegra_kbc_dt_parse_pdata( if (!np) return NULL; - pdata = kzalloc(sizeof(*pdata), GFP_KERNEL); + pdata = devm_kzalloc(&pdev->dev, sizeof(*pdata), GFP_KERNEL); if (!pdata) return NULL; @@ -700,33 +700,36 @@ static int tegra_kbc_probe(struct platform_device *pdev) if (!pdata) pdata = tegra_kbc_dt_parse_pdata(pdev); - if (!pdata) + if (!pdata) { + dev_err(&pdev->dev, "Platform data missing\n"); return -EINVAL; - - if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows)) { - err = -EINVAL; - goto err_free_pdata; } + if (!tegra_kbc_check_pin_cfg(pdata, &pdev->dev, &num_rows)) + return -EINVAL; + res = platform_get_resource(pdev, IORESOURCE_MEM, 0); if (!res) { dev_err(&pdev->dev, "failed to get I/O memory\n"); - err = -ENXIO; - goto err_free_pdata; + return -ENXIO; } irq = platform_get_irq(pdev, 0); if (irq < 0) { dev_err(&pdev->dev, "failed to get keyboard IRQ\n"); - err = -ENXIO; - goto err_free_pdata; + return -ENXIO; + } + + kbc = devm_kzalloc(&pdev->dev, sizeof(*kbc), GFP_KERNEL); + if (!kbc) { + dev_err(&pdev->dev, "failed to alloc memory for kbc\n"); + return -ENOMEM; } - kbc = kzalloc(sizeof(*kbc), GFP_KERNEL); - input_dev = input_allocate_device(); - if (!kbc || !input_dev) { - err = -ENOMEM; - goto err_free_mem; + input_dev = devm_input_allocate_device(&pdev->dev); + if (!input_dev) { + dev_err(&pdev->dev, "failed to allocate input device\n"); + return -ENOMEM; } kbc->pdata = pdata; @@ -735,25 +738,16 @@ static int tegra_kbc_probe(struct platform_device *pdev) spin_lock_init(&kbc->lock); setup_timer(&kbc->timer, tegra_kbc_keypress_timer, (unsigned long)kbc); - res = request_mem_region(res->start, resource_size(res), pdev->name); - if (!res) { - dev_err(&pdev->dev, "failed to request I/O memory\n"); - err = -EBUSY; - goto err_free_mem; - } - - kbc->mmio = ioremap(res->start, resource_size(res)); + kbc->mmio = devm_request_and_ioremap(&pdev->dev, res); if (!kbc->mmio) { - dev_err(&pdev->dev, "failed to remap I/O memory\n"); - err = -ENXIO; - goto err_free_mem_region; + dev_err(&pdev->dev, "Cannot request memregion/iomap address\n"); + return -EADDRNOTAVAIL; } - kbc->clk = clk_get(&pdev->dev, NULL); + kbc->clk = devm_clk_get(&pdev->dev, NULL); if (IS_ERR(kbc->clk)) { dev_err(&pdev->dev, "failed to get keyboard clock\n"); - err = PTR_ERR(kbc->clk); - goto err_iounmap; + return PTR_ERR(kbc->clk); } /* @@ -778,9 +772,9 @@ static int tegra_kbc_probe(struct platform_device *pdev) input_dev->close = tegra_kbc_close; err = tegra_kbd_setup_keymap(kbc); - if (err) { + if (err < 0) { dev_err(&pdev->dev, "failed to setup keymap\n"); - goto err_put_clk; + return err; } __set_bit(EV_REP, input_dev->evbit); @@ -790,15 +784,15 @@ static int tegra_kbc_probe(struct platform_device *pdev) err = request_irq(kbc->irq, tegra_kbc_isr, IRQF_NO_SUSPEND | IRQF_TRIGGER_HIGH, pdev->name, kbc); - if (err) { + if (err < 0) { dev_err(&pdev->dev, "failed to request keyboard IRQ\n"); - goto err_put_clk; + return err; } disable_irq(kbc->irq); err = input_register_device(kbc->idev); - if (err) { + if (err < 0) { dev_err(&pdev->dev, "failed to register input device\n"); goto err_free_irq; } @@ -810,46 +804,15 @@ static int tegra_kbc_probe(struct platform_device *pdev) err_free_irq: free_irq(kbc->irq, pdev); -err_put_clk: - clk_put(kbc->clk); -err_iounmap: - iounmap(kbc->mmio); -err_free_mem_region: - release_mem_region(res->start, resource_size(res)); -err_free_mem: - input_free_device(input_dev); - kfree(kbc); -err_free_pdata: - if (!pdev->dev.platform_data) - kfree(pdata); - return err; } static int tegra_kbc_remove(struct platform_device *pdev) { struct tegra_kbc *kbc = platform_get_drvdata(pdev); - struct resource *res; - - platform_set_drvdata(pdev, NULL); free_irq(kbc->irq, pdev); - clk_put(kbc->clk); - input_unregister_device(kbc->idev); - iounmap(kbc->mmio); - res = platform_get_resource(pdev, IORESOURCE_MEM, 0); - release_mem_region(res->start, resource_size(res)); - - /* - * If we do not have platform data attached to the device we - * allocated it ourselves and thus need to free it. - */ - if (!pdev->dev.platform_data) - kfree(kbc->pdata); - - kfree(kbc); - return 0; }
Use devm_* for memory, clock, input device allocation. This reduces code for freeing these resources. Signed-off-by: Laxman Dewangan <ldewangan@nvidia.com> --- Changes from V1: None drivers/input/keyboard/tegra-kbc.c | 93 +++++++++++------------------------- 1 files changed, 28 insertions(+), 65 deletions(-)