| Message ID | 20200930055146.5665-1-hui.wang@canonical.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f4794c6064a83d2c57b264bd299c367d172d1044 |
| Headers | show |
| Series | ALSA: hda - Don't register a cb func if it is registered already | expand |
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
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
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
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
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
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);
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(-)