* [PATCH] ALSA: hda - Don't register a cb func if it is registered already
@ 2020-09-30 5:51 Hui Wang
2020-09-30 7:19 ` Jaroslav Kysela
0 siblings, 1 reply; 6+ messages in thread
From: Hui Wang @ 2020-09-30 5:51 UTC (permalink / raw)
To: alsa-devel, tiwai
If the caller of enable_callback_mst() passes a cb func, the callee
function will malloc memory and link this cb func to the list
unconditionally. This will introduce problem if caller is in the
hda_codec_ops.init() since the init() will be repeatedly called in the
codec rt_resume().
So far, the patch_hdmi.c and patch_ca0132.c call enable_callback_mst()
in the hda_codec_ops.init().
Signed-off-by: Hui Wang <hui.wang@canonical.com>
---
sound/pci/hda/hda_jack.c | 14 +++++++++++++-
1 file changed, 13 insertions(+), 1 deletion(-)
diff --git a/sound/pci/hda/hda_jack.c b/sound/pci/hda/hda_jack.c
index 02cc682caa55..ded4813f8b54 100644
--- a/sound/pci/hda/hda_jack.c
+++ b/sound/pci/hda/hda_jack.c
@@ -275,6 +275,18 @@ int snd_hda_jack_detect_state_mst(struct hda_codec *codec,
}
EXPORT_SYMBOL_GPL(snd_hda_jack_detect_state_mst);
+static bool func_is_already_in_callback_list(struct hda_jack_tbl *jack,
+ hda_jack_callback_fn func)
+{
+ struct hda_jack_callback *cb;
+
+ for (cb = jack->callback; cb; cb = cb->next) {
+ if (cb->func == func)
+ return true;
+ }
+ return false;
+}
+
/**
* snd_hda_jack_detect_enable_mst - enable the jack-detection
* @codec: the HDA codec
@@ -297,7 +309,7 @@ snd_hda_jack_detect_enable_callback_mst(struct hda_codec *codec, hda_nid_t nid,
jack = snd_hda_jack_tbl_new(codec, nid, dev_id);
if (!jack)
return ERR_PTR(-ENOMEM);
- if (func) {
+ if (func && !func_is_already_in_callback_list(jack, func)) {
callback = kzalloc(sizeof(*callback), GFP_KERNEL);
if (!callback)
return ERR_PTR(-ENOMEM);
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: hda - Don't register a cb func if it is registered already
2020-09-30 5:51 [PATCH] ALSA: hda - Don't register a cb func if it is registered already Hui Wang
@ 2020-09-30 7:19 ` Jaroslav Kysela
2020-09-30 9:21 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Jaroslav Kysela @ 2020-09-30 7:19 UTC (permalink / raw)
To: Hui Wang, alsa-devel, tiwai
Dne 30. 09. 20 v 7:51 Hui Wang napsal(a):
> If the caller of enable_callback_mst() passes a cb func, the callee
> function will malloc memory and link this cb func to the list
> unconditionally. This will introduce problem if caller is in the
> hda_codec_ops.init() since the init() will be repeatedly called in the
> codec rt_resume().
>
> So far, the patch_hdmi.c and patch_ca0132.c call enable_callback_mst()
> in the hda_codec_ops.init().
Won't be better to handle this double invocation at the callback call time? I
believe that some refcounting and pointing to one allocated callback structure
for all instances is better.
Jaroslav
--
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: hda - Don't register a cb func if it is registered already
2020-09-30 7:19 ` Jaroslav Kysela
@ 2020-09-30 9:21 ` Takashi Iwai
2020-09-30 9:28 ` Jaroslav Kysela
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2020-09-30 9:21 UTC (permalink / raw)
To: Jaroslav Kysela; +Cc: Hui Wang, alsa-devel
On Wed, 30 Sep 2020 09:19:50 +0200,
Jaroslav Kysela wrote:
>
> Dne 30. 09. 20 v 7:51 Hui Wang napsal(a):
> > If the caller of enable_callback_mst() passes a cb func, the callee
> > function will malloc memory and link this cb func to the list
> > unconditionally. This will introduce problem if caller is in the
> > hda_codec_ops.init() since the init() will be repeatedly called in the
> > codec rt_resume().
> >
> > So far, the patch_hdmi.c and patch_ca0132.c call enable_callback_mst()
> > in the hda_codec_ops.init().
>
> Won't be better to handle this double invocation at the callback call time? I
> believe that some refcounting and pointing to one allocated callback structure
> for all instances is better.
IMO, Hui's fix is correct in this case; otherwise it'll result in
endless number of allocations at each time the runtime resume is
performed. So I'm going to take it as is.
For ca0132, though, the simplest fix would be to just move
ca0132_init_unsol() call into patch_ca0132(). But the additional fix
in hda_jack side would be good in anyway.
thanks,
Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: hda - Don't register a cb func if it is registered already
2020-09-30 9:21 ` Takashi Iwai
@ 2020-09-30 9:28 ` Jaroslav Kysela
2020-09-30 9:39 ` Takashi Iwai
0 siblings, 1 reply; 6+ messages in thread
From: Jaroslav Kysela @ 2020-09-30 9:28 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Hui Wang, alsa-devel
Dne 30. 09. 20 v 11:21 Takashi Iwai napsal(a):
> On Wed, 30 Sep 2020 09:19:50 +0200,
> Jaroslav Kysela wrote:
>>
>> Dne 30. 09. 20 v 7:51 Hui Wang napsal(a):
>>> If the caller of enable_callback_mst() passes a cb func, the callee
>>> function will malloc memory and link this cb func to the list
>>> unconditionally. This will introduce problem if caller is in the
>>> hda_codec_ops.init() since the init() will be repeatedly called in the
>>> codec rt_resume().
>>>
>>> So far, the patch_hdmi.c and patch_ca0132.c call enable_callback_mst()
>>> in the hda_codec_ops.init().
>>
>> Won't be better to handle this double invocation at the callback call time? I
>> believe that some refcounting and pointing to one allocated callback structure
>> for all instances is better.
>
> IMO, Hui's fix is correct in this case; otherwise it'll result in
> endless number of allocations at each time the runtime resume is
> performed. So I'm going to take it as is.
I meant to allocate the structure only once with refcounting and multiple
invocation protection. In the proposed change, you lose the bindings. But as
you prefer.
Jaroslav
--
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: hda - Don't register a cb func if it is registered already
2020-09-30 9:28 ` Jaroslav Kysela
@ 2020-09-30 9:39 ` Takashi Iwai
2020-09-30 9:48 ` Jaroslav Kysela
0 siblings, 1 reply; 6+ messages in thread
From: Takashi Iwai @ 2020-09-30 9:39 UTC (permalink / raw)
To: Jaroslav Kysela; +Cc: Hui Wang, alsa-devel
On Wed, 30 Sep 2020 11:28:59 +0200,
Jaroslav Kysela wrote:
>
> Dne 30. 09. 20 v 11:21 Takashi Iwai napsal(a):
> > On Wed, 30 Sep 2020 09:19:50 +0200,
> > Jaroslav Kysela wrote:
> >>
> >> Dne 30. 09. 20 v 7:51 Hui Wang napsal(a):
> >>> If the caller of enable_callback_mst() passes a cb func, the callee
> >>> function will malloc memory and link this cb func to the list
> >>> unconditionally. This will introduce problem if caller is in the
> >>> hda_codec_ops.init() since the init() will be repeatedly called in the
> >>> codec rt_resume().
> >>>
> >>> So far, the patch_hdmi.c and patch_ca0132.c call enable_callback_mst()
> >>> in the hda_codec_ops.init().
> >>
> >> Won't be better to handle this double invocation at the callback call time? I
> >> believe that some refcounting and pointing to one allocated callback structure
> >> for all instances is better.
> >
> > IMO, Hui's fix is correct in this case; otherwise it'll result in
> > endless number of allocations at each time the runtime resume is
> > performed. So I'm going to take it as is.
>
> I meant to allocate the structure only once with refcounting and multiple
> invocation protection. In the proposed change, you lose the
> bindings.
AFAIUC, it won't lose any functionality.
snd_hda_jack_detect_enable_callback() would chain the callback
function if a jack object has been already created, and this
additional check will just prevent the doubly registration of the very
same callback when called multiple times.
thanks,
Takashi
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] ALSA: hda - Don't register a cb func if it is registered already
2020-09-30 9:39 ` Takashi Iwai
@ 2020-09-30 9:48 ` Jaroslav Kysela
0 siblings, 0 replies; 6+ messages in thread
From: Jaroslav Kysela @ 2020-09-30 9:48 UTC (permalink / raw)
To: Takashi Iwai; +Cc: Hui Wang, alsa-devel
Dne 30. 09. 20 v 11:39 Takashi Iwai napsal(a):
> On Wed, 30 Sep 2020 11:28:59 +0200,
> Jaroslav Kysela wrote:
>>
>> Dne 30. 09. 20 v 11:21 Takashi Iwai napsal(a):
>>> On Wed, 30 Sep 2020 09:19:50 +0200,
>>> Jaroslav Kysela wrote:
>>>>
>>>> Dne 30. 09. 20 v 7:51 Hui Wang napsal(a):
>>>>> If the caller of enable_callback_mst() passes a cb func, the callee
>>>>> function will malloc memory and link this cb func to the list
>>>>> unconditionally. This will introduce problem if caller is in the
>>>>> hda_codec_ops.init() since the init() will be repeatedly called in the
>>>>> codec rt_resume().
>>>>>
>>>>> So far, the patch_hdmi.c and patch_ca0132.c call enable_callback_mst()
>>>>> in the hda_codec_ops.init().
>>>>
>>>> Won't be better to handle this double invocation at the callback call time? I
>>>> believe that some refcounting and pointing to one allocated callback structure
>>>> for all instances is better.
>>>
>>> IMO, Hui's fix is correct in this case; otherwise it'll result in
>>> endless number of allocations at each time the runtime resume is
>>> performed. So I'm going to take it as is.
>>
>> I meant to allocate the structure only once with refcounting and multiple
>> invocation protection. In the proposed change, you lose the
>> bindings.
>
> AFAIUC, it won't lose any functionality.
> snd_hda_jack_detect_enable_callback() would chain the callback
> function if a jack object has been already created, and this
> additional check will just prevent the doubly registration of the very
> same callback when called multiple times.
Ok, I see, the lookup is codec / nid / dev_id so in this case the multiple
callback assignments are really not desired. Thanks for the explanation.
Jaroslav
--
Jaroslav Kysela <perex@perex.cz>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2020-09-30 9:49 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-30 5:51 [PATCH] ALSA: hda - Don't register a cb func if it is registered already Hui Wang
2020-09-30 7:19 ` Jaroslav Kysela
2020-09-30 9:21 ` Takashi Iwai
2020-09-30 9:28 ` Jaroslav Kysela
2020-09-30 9:39 ` Takashi Iwai
2020-09-30 9:48 ` Jaroslav Kysela
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.