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

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 --]