[alsa-devel] [PATCH] ASoC: wm8962: Convert to devm_input_allocate_device()
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;
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
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.
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
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.
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
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...
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
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:
- short one - fix your patches
- 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.
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:
- short one - fix your patches
- 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:
- 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".
- 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.
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:
- short one - fix your patches
- 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:
- 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".
- 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
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.
participants (4)
-
Dmitry Torokhov -
Leon Romanovsky -
Mark Brown -
Mark Brown