Patchwork [v2,2/4] input: keyboard: tegra: use devm_* for resource allocation

login
register
mail settings
Submitter Laxman Dewangan
Date Jan. 5, 2013, 7:45 a.m.
Message ID <1357371910-3164-3-git-send-email-ldewangan@nvidia.com>
Download mbox | patch
Permalink /patch/209651/
State Not Applicable, archived
Headers show

Comments

Laxman Dewangan - Jan. 5, 2013, 7:45 a.m.
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(-)
Dmitry Torokhov - Jan. 5, 2013, 8:06 a.m.
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.
Laxman Dewangan - Jan. 5, 2013, 11:20 a.m.
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
Dmitry Torokhov - Jan. 5, 2013, 11:18 p.m.
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.
Laxman Dewangan - Jan. 6, 2013, 11 a.m.
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
Thierry Reding - Jan. 6, 2013, 7:27 p.m.
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
Dmitry Torokhov - Jan. 6, 2013, 7:57 p.m.
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.
Thierry Reding - Jan. 9, 2013, 7:07 a.m.
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
Dmitry Torokhov - Jan. 9, 2013, 9:19 a.m.
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.
Thierry Reding - Jan. 9, 2013, 9:23 a.m.
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
Thierry Reding - Jan. 14, 2013, 3:49 p.m.
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
Greg KH - Jan. 14, 2013, 4:16 p.m.
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
Thierry Reding - Jan. 14, 2013, 10:15 p.m.
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
Arnd Bergmann - Jan. 14, 2013, 10:24 p.m.
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
Thierry Reding - Jan. 15, 2013, 6:44 a.m.
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
Wolfram Sang - Jan. 15, 2013, 12:32 p.m.
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.
Wolfram Sang - Jan. 15, 2013, 1:06 p.m.
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
Thierry Reding - Jan. 15, 2013, 3:44 p.m.
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
Wolfram Sang - Jan. 16, 2013, 6:35 a.m.
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
Grant Likely - Feb. 9, 2013, 9:04 a.m.
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

Patch

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;
 }