[PATCH] ASoC: wm8962: Convert to devm_input_allocate_device()

[PATCH] ASoC: wm8962: Convert to devm_input_allocate_device()

[Mark Brown]

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 sound/soc/codecs/wm8962.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c
index 50b0983..1cee9b3 100644
--- a/sound/soc/codecs/wm8962.c
+++ b/sound/soc/codecs/wm8962.c
@@ -3189,7 +3189,7 @@ static void wm8962_init_beep(struct snd_soc_codec *codec)
 	struct wm8962_priv *wm8962 = snd_soc_codec_get_drvdata(codec);
 	int ret;
 
-	wm8962->beep = input_allocate_device();
+	wm8962->beep = devm_input_allocate_device(codec->dev);
 	if (!wm8962->beep) {
 		dev_err(codec->dev, "Failed to allocate beep device\n");
 		return;
@@ -3210,7 +3210,6 @@ static void wm8962_init_beep(struct snd_soc_codec *codec)
 
 	ret = input_register_device(wm8962->beep);
 	if (ret != 0) {
-		input_free_device(wm8962->beep);
 		wm8962->beep = NULL;
 		dev_err(codec->dev, "Failed to register beep device\n");
 	}
@@ -3227,7 +3226,6 @@ static void wm8962_free_beep(struct snd_soc_codec *codec)
 	struct wm8962_priv *wm8962 = snd_soc_codec_get_drvdata(codec);
 
 	device_remove_file(codec->dev, &dev_attr_beep);
-	input_unregister_device(wm8962->beep);
 	cancel_work_sync(&wm8962->beep_work);
 	wm8962->beep = NULL;
 
-- 
1.7.10.4

Re: [PATCH] ASoC: wm8962: Convert to devm_input_allocate_device()

[Leon Romanovsky]

Hi Mark,

On Thu, Dec 20, 2012 at 4:18 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  sound/soc/codecs/wm8962.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
>
> diff --git a/sound/soc/codecs/wm8962.c b/sound/soc/codecs/wm8962.c
> index 50b0983..1cee9b3 100644
> --- a/sound/soc/codecs/wm8962.c
> +++ b/sound/soc/codecs/wm8962.c
> @@ -3210,7 +3210,6 @@ static void wm8962_init_beep(struct snd_soc_codec *codec)
>
>         ret = input_register_device(wm8962->beep);
>         if (ret != 0) {
> -               input_free_device(wm8962->beep);
>                 wm8962->beep = NULL;
>                 dev_err(codec->dev, "Failed to register beep device\n");
>         }

I'm not sure if this patch is correct.
According to comment of input_free_memory()
(http://lxr.free-electrons.com/source/drivers/input/input.c#L1825) you
need to call it if input_register_device failed.


--
Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | leon@leon.nu

Re: [PATCH] ASoC: wm8962: Convert to devm_input_allocate_device()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 852 bytes --]

On Wed, Apr 24, 2013 at 04:50:48PM +0300, Leon Romanovsky wrote:
> On Thu, Dec 20, 2012 at 4:18 PM, Mark Brown

> >         ret = input_register_device(wm8962->beep);
> >         if (ret != 0) {
> > -               input_free_device(wm8962->beep);
> >                 wm8962->beep = NULL;
> >                 dev_err(codec->dev, "Failed to register beep device\n");
> >         }

> I'm not sure if this patch is correct.
> According to comment of input_free_memory()
> (http://lxr.free-electrons.com/source/drivers/input/input.c#L1825) you
> need to call it if input_register_device failed.

This seems like a bug that should be fixed on the API side, it's really
not what you'd expect a devm function to do, and if you are explicitly
freeing a devm allocated object there's an expectation that you need to
call a corresponding devm cleanup function.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: wm8962: Convert to devm_input_allocate_device()

[Leon Romanovsky]

Hi Mark,
On Thu, Apr 25, 2013 at 3:52 PM, Mark Brown <broonie@kernel.org> wrote:
> On Wed, Apr 24, 2013 at 04:50:48PM +0300, Leon Romanovsky wrote:
>> On Thu, Dec 20, 2012 at 4:18 PM, Mark Brown
>
>> >         ret = input_register_device(wm8962->beep);
>> >         if (ret != 0) {
>> > -               input_free_device(wm8962->beep);
>> >                 wm8962->beep = NULL;
>> >                 dev_err(codec->dev, "Failed to register beep device\n");
>> >         }
>
>> I'm not sure if this patch is correct.
>> According to comment of input_free_memory()
>> (http://lxr.free-electrons.com/source/drivers/input/input.c#L1825) you
>> need to call it if input_register_device failed.
>
> This seems like a bug that should be fixed on the API side, it's really
> not what you'd expect a devm function to do, and if you are explicitly
> freeing a devm allocated object there's an expectation that you need to
> call a corresponding devm cleanup function.

I can't agree with you about the "expectation" - device memory can be in use.
This code will free memory after all references will be freed.
Additionally it is up-to developer to decide what to do if
input_register_device call failed.
According to the code base
(http://lxr.free-electrons.com/ident?i=input_free_device)
the patch is not correct.

Thanks.

--
Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | leon@leon.nu

Re: [PATCH] ASoC: wm8962: Convert to devm_input_allocate_device()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1312 bytes --]

On Sun, Apr 28, 2013 at 08:00:29AM +0300, Leon Romanovsky wrote:
> On Thu, Apr 25, 2013 at 3:52 PM, Mark Brown <broonie@kernel.org> wrote:

> > This seems like a bug that should be fixed on the API side, it's really
> > not what you'd expect a devm function to do, and if you are explicitly
> > freeing a devm allocated object there's an expectation that you need to
> > call a corresponding devm cleanup function.

> I can't agree with you about the "expectation" - device memory can be in use.
> This code will free memory after all references will be freed.
> Additionally it is up-to developer to decide what to do if
> input_register_device call failed.
> According to the code base
> (http://lxr.free-electrons.com/ident?i=input_free_device)
> the patch is not correct.

Right, but what I'm saying is that this is not how a devm_ API is
expected to behave and is therefore at best error prone on two fronts -
both due to the fact that you need to clean up explicitly even if the
resource is not actually managed and also due to the fact that the
regular free function is being used rather than a devm_ one when
explicit deallocation is done.  Neither of these things is normal for a
devm_ API.  The behaviour you're describing says that the managed
function should actually be registration not allocation.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: wm8962: Convert to devm_input_allocate_device()

[Leon Romanovsky]

On Sun, Apr 28, 2013 at 12:47 PM, Mark Brown <broonie@kernel.org> wrote:
> On Sun, Apr 28, 2013 at 08:00:29AM +0300, Leon Romanovsky wrote:
>> On Thu, Apr 25, 2013 at 3:52 PM, Mark Brown <broonie@kernel.org> wrote:
>
>> > This seems like a bug that should be fixed on the API side, it's really
>> > not what you'd expect a devm function to do, and if you are explicitly
>> > freeing a devm allocated object there's an expectation that you need to
>> > call a corresponding devm cleanup function.
>
>> I can't agree with you about the "expectation" - device memory can be in use.
>> This code will free memory after all references will be freed.
>> Additionally it is up-to developer to decide what to do if
>> input_register_device call failed.
>> According to the code base
>> (http://lxr.free-electrons.com/ident?i=input_free_device)
>> the patch is not correct.
>
> Right, but what I'm saying is that this is not how a devm_ API is
> expected to behave and is therefore at best error prone on two fronts -
> both due to the fact that you need to clean up explicitly even if the
> resource is not actually managed and also due to the fact that the
> regular free function is being used rather than a devm_ one when
> explicit deallocation is done.  Neither of these things is normal for a
> devm_ API.  The behaviour you're describing says that the managed
> function should actually be registration not allocation.
I think the reason of our misunderstanding is due to the name of
input_free_device call. From the code, it is device destroy function,
and the freeing itself done as an error handling of
input_register_device
(http://lxr.free-electrons.com/source/drivers/input/input.c#L2114).

How do you think we need to proceed? Do I need to send patches with
explicit call to input_free_device function?

--
Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | leon@leon.nu

Re: [PATCH] ASoC: wm8962: Convert to devm_input_allocate_device()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 656 bytes --]

On Sun, Apr 28, 2013 at 09:32:18PM +0300, Leon Romanovsky wrote:

> I think the reason of our misunderstanding is due to the name of
> input_free_device call. From the code, it is device destroy function,
> and the freeing itself done as an error handling of
> input_register_device
> (http://lxr.free-electrons.com/source/drivers/input/input.c#L2114).

> How do you think we need to proceed? Do I need to send patches with
> explicit call to input_free_device function?

I really think the input API needs to be looked at here, this is all way
too error prone.  Calling input_free_device() on something allocated
using devm_ looks like an error itself...

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [alsa-devel] [PATCH] ASoC: wm8962: Convert to devm_input_allocate_device()

[Leon Romanovsky]

On Mon, Apr 29, 2013 at 1:19 PM, Mark Brown <broonie@kernel.org> wrote:
> On Sun, Apr 28, 2013 at 09:32:18PM +0300, Leon Romanovsky wrote:
>
>> I think the reason of our misunderstanding is due to the name of
>> input_free_device call. From the code, it is device destroy function,
>> and the freeing itself done as an error handling of
>> input_register_device
>> (http://lxr.free-electrons.com/source/drivers/input/input.c#L2114).
>
>> How do you think we need to proceed? Do I need to send patches with
>> explicit call to input_free_device function?
>
> I really think the input API needs to be looked at here, this is all way
> too error prone.  Calling input_free_device() on something allocated
> using devm_ looks like an error itself...
In general, I agree with you, but I think we both agree that the
current patch is not working as expected.
The problem is that you allocated device with devm_ and later at the
code you tried to register it, but failed. In this case no one will
call to devres_destroy, because it is done at unregister stage only.

I see two possible solutions:
1. short one - fix your patches
2. long one - add input_free_device code into input_register_device
call (http://lxr.free-electrons.com/source/drivers/input/input.c#L2114).


--
Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | leon@leon.nu

Re: [alsa-devel] [PATCH] ASoC: wm8962: Convert to devm_input_allocate_device()

[Dmitry Torokhov]

On Mon, Apr 29, 2013 at 08:47:37PM +0300, Leon Romanovsky wrote:
> On Mon, Apr 29, 2013 at 1:19 PM, Mark Brown <broonie@kernel.org> wrote:
> > On Sun, Apr 28, 2013 at 09:32:18PM +0300, Leon Romanovsky wrote:
> >
> >> I think the reason of our misunderstanding is due to the name of
> >> input_free_device call. From the code, it is device destroy function,
> >> and the freeing itself done as an error handling of
> >> input_register_device
> >> (http://lxr.free-electrons.com/source/drivers/input/input.c#L2114).
> >
> >> How do you think we need to proceed? Do I need to send patches with
> >> explicit call to input_free_device function?
> >
> > I really think the input API needs to be looked at here, this is all way
> > too error prone.  Calling input_free_device() on something allocated
> > using devm_ looks like an error itself...
> In general, I agree with you, but I think we both agree that the
> current patch is not working as expected.
> The problem is that you allocated device with devm_ and later at the
> code you tried to register it, but failed. In this case no one will
> call to devres_destroy, because it is done at unregister stage only.
> 
> I see two possible solutions:
> 1. short one - fix your patches
> 2. long one - add input_free_device code into input_register_device
> call (http://lxr.free-electrons.com/source/drivers/input/input.c#L2114).
> 

The rules are pretty straightforward:

1. If you are using devm_input_allocate_device() you do not need to call
input_free_device() nor input_unregister_device()  - the core will
create a devres structure for freeing the device and if
input_register_device() succeeds it will also add a 2nd devres for
unregistering. This way the "normal" unwind is a 2-step process with
device is "half alive" and being able to survive input_event() calls
from IRQ handlers if they are still alive.

IOW  it should all "just work".

2. If you are using input_allocate_device() then you need to call
input_free_device() until you called input_register_device(), afterward
input_unregister_device() should be called.

Thanks.

-- 
Dmitry

Re: [alsa-devel] [PATCH] ASoC: wm8962: Convert to devm_input_allocate_device()

[Dmitry Torokhov]

On Mon, Apr 29, 2013 at 11:07:28AM -0700, Dmitry Torokhov wrote:
> On Mon, Apr 29, 2013 at 08:47:37PM +0300, Leon Romanovsky wrote:
> > On Mon, Apr 29, 2013 at 1:19 PM, Mark Brown <broonie@kernel.org> wrote:
> > > On Sun, Apr 28, 2013 at 09:32:18PM +0300, Leon Romanovsky wrote:
> > >
> > >> I think the reason of our misunderstanding is due to the name of
> > >> input_free_device call. From the code, it is device destroy function,
> > >> and the freeing itself done as an error handling of
> > >> input_register_device
> > >> (http://lxr.free-electrons.com/source/drivers/input/input.c#L2114).
> > >
> > >> How do you think we need to proceed? Do I need to send patches with
> > >> explicit call to input_free_device function?
> > >
> > > I really think the input API needs to be looked at here, this is all way
> > > too error prone.  Calling input_free_device() on something allocated
> > > using devm_ looks like an error itself...
> > In general, I agree with you, but I think we both agree that the
> > current patch is not working as expected.
> > The problem is that you allocated device with devm_ and later at the
> > code you tried to register it, but failed. In this case no one will
> > call to devres_destroy, because it is done at unregister stage only.
> > 
> > I see two possible solutions:
> > 1. short one - fix your patches
> > 2. long one - add input_free_device code into input_register_device
> > call (http://lxr.free-electrons.com/source/drivers/input/input.c#L2114).
> > 
> 
> The rules are pretty straightforward:
> 
> 1. If you are using devm_input_allocate_device() you do not need to call
> input_free_device() nor input_unregister_device()  - the core will
> create a devres structure for freeing the device and if
> input_register_device() succeeds it will also add a 2nd devres for
> unregistering. This way the "normal" unwind is a 2-step process with
> device is "half alive" and being able to survive input_event() calls
> from IRQ handlers if they are still alive.
> 
> IOW  it should all "just work".
> 
> 2. If you are using input_allocate_device() then you need to call
> input_free_device() until you called input_register_device(), afterward
> input_unregister_device() should be called.
> 

BTW, looking at Marks patch it looks good to me.

-- 
Dmitry

Re: [alsa-devel] [PATCH] ASoC: wm8962: Convert to devm_input_allocate_device()

[Leon Romanovsky]

On Mon, Apr 29, 2013 at 9:19 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Mon, Apr 29, 2013 at 11:07:28AM -0700, Dmitry Torokhov wrote:
>> On Mon, Apr 29, 2013 at 08:47:37PM +0300, Leon Romanovsky wrote:
>> > On Mon, Apr 29, 2013 at 1:19 PM, Mark Brown <broonie@kernel.org> wrote:
>> > > On Sun, Apr 28, 2013 at 09:32:18PM +0300, Leon Romanovsky wrote:
>> > >
>> > >> I think the reason of our misunderstanding is due to the name of
>> > >> input_free_device call. From the code, it is device destroy function,
>> > >> and the freeing itself done as an error handling of
>> > >> input_register_device
>> > >> (http://lxr.free-electrons.com/source/drivers/input/input.c#L2114).
>> > >
>> > >> How do you think we need to proceed? Do I need to send patches with
>> > >> explicit call to input_free_device function?
>> > >
>> > > I really think the input API needs to be looked at here, this is all way
>> > > too error prone.  Calling input_free_device() on something allocated
>> > > using devm_ looks like an error itself...
>> > In general, I agree with you, but I think we both agree that the
>> > current patch is not working as expected.
>> > The problem is that you allocated device with devm_ and later at the
>> > code you tried to register it, but failed. In this case no one will
>> > call to devres_destroy, because it is done at unregister stage only.
>> >
>> > I see two possible solutions:
>> > 1. short one - fix your patches
>> > 2. long one - add input_free_device code into input_register_device
>> > call (http://lxr.free-electrons.com/source/drivers/input/input.c#L2114).
>> >
>>
>> The rules are pretty straightforward:
>>
>> 1. If you are using devm_input_allocate_device() you do not need to call
>> input_free_device() nor input_unregister_device()  - the core will
>> create a devres structure for freeing the device and if
>> input_register_device() succeeds it will also add a 2nd devres for
>> unregistering. This way the "normal" unwind is a 2-step process with
>> device is "half alive" and being able to survive input_event() calls
>> from IRQ handlers if they are still alive.
>>
>> IOW  it should all "just work".
>>
>> 2. If you are using input_allocate_device() then you need to call
>> input_free_device() until you called input_register_device(), afterward
>> input_unregister_device() should be called.
>>
>
> BTW, looking at Marks patch it looks good to me.
Dmitry, thanks for the explanation.

Mark, please take my apologies, I was mislead by the following comment
on the code:
http://lxr.free-electrons.com/source/drivers/input/input.c#L1825
1822  * input_free_device - free memory occupied by input_dev structure
1823  * @dev: input device to free
1824  *
1825  * This function should only be used if input_register_device()
1826  * was not called yet or if it failed.

>
> --
> Dmitry



--
Leon Romanovsky | Independent Linux Consultant
        www.leon.nu | leon@leon.nu

Re: [PATCH] ASoC: wm8962: Convert to devm_input_allocate_device()

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 214 bytes --]

On Mon, Apr 29, 2013 at 09:52:34PM +0300, Leon Romanovsky wrote:

> Mark, please take my apologies, I was mislead by the following comment
> on the code:

No problem, thanks for taking the time to check into this.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]