On Fri, 2019-05-31 at 16:28 +0200, Takashi Iwai wrote: > On Fri, 31 May 2019 16:02:30 +0200, > Takashi Iwai wrote: > > > > On Fri, 31 May 2019 15:52:27 +0200, > > Ranjani Sridharan wrote: > > > > > > On Fri, 2019-05-31 at 15:25 +0200, Takashi Iwai wrote: > > > > On Fri, 31 May 2019 15:18:03 +0200, > > > > Ranjani Sridharan wrote: > > > > > > > > > > On Fri, 2019-05-31 at 08:11 +0200, Takashi Iwai wrote: > > > > > > On Thu, 30 May 2019 23:00:10 +0200, > > > > > > Pierre-Louis Bossart wrote: > > > > > > > > > > > > > > > > > > > > > > > > > > > > On 5/30/19 3:18 PM, Ranjani Sridharan wrote: > > > > > > > > Calling snd_device_new() makes the codec devices > > > > > > > > managed by > > > > > > > > the > > > > > > > > card. > > > > > > > > So, when the card is removed, the refcount for the > > > > > > > > codec > > > > > > > > device is decremented and results in the codec device's > > > > > > > > kobject > > > > > > > > being cleaned up if the refcount is 0. But, this leads > > > > > > > > to a > > > > > > > > NULL > > > > > > > > pointer exception while attempting to remove the > > > > > > > > symlinks > > > > > > > > when > > > > > > > > the > > > > > > > > codec driver is released later on. Therefore, increment > > > > > > > > the > > > > > > > > codec > > > > > > > > device's refcount before adding it to the card to > > > > > > > > prevent > > > > > > > > this. > > > > > > > > > > > > > > Ranjani, you should add a bit of context for the rest of > > > > > > > the > > > > > > > list... > > > > > > > > > > > > > > This patch suggest a solution to a set of sightings > > > > > > > occurring > > > > > > > when > > > > > > > removing/adding modules in a loop, and the current > > > > > > > analysis > > > > > > > points > > > > > > > to > > > > > > > a difference between the way the HDMI and HDaudio codecs > > > > > > > are > > > > > > > handled. > > > > > > > > > > > > > > https://github.com/thesofproject/linux/issues/981 > > > > > > > https://github.com/thesofproject/linux/issues/966 > > > > > > > https://github.com/thesofproject/linux/pull/988 > > > > > > > > > > > > > > Since it's not SOF specific it's better to get feedback > > > > > > > directly > > > > > > > from > > > > > > > the large ALSA community/maintainers. We probably want to > > > > > > > focus > > > > > > > on > > > > > > > the > > > > > > > platform-specific/vendor-specific stuff on GitHub and use > > > > > > > the > > > > > > > mailing > > > > > > > list for such framework-level changes. > > > > > > > > > > > > Hm, I still wonder why this doens't happen with the HDA > > > > > > legacy. > > > > > > > > > > > > What is the shortest way to trigger the bug manually > > > > > > without a > > > > > > script? > > > > > > > > > > Hi Takashi, > > > > > > > > > > With SOF, I can reproduce the issue if I just unload the > > > > > sof_pci_dev > > > > > module with rmmod. > > > > > > > > > > Basically, the remove routine for the SOF pci device, > > > > > unregisters > > > > > the > > > > > machine driver and then removes the codec device. So the > > > > > first step > > > > > of > > > > > unregistering the machine driver frees the card which > > > > > decrements > > > > > the > > > > > refcount for the HDA codec's kobject. In the case of HDMI > > > > > codec, > > > > > since > > > > > it is not managed by the card, the refcount is not > > > > > decremented when > > > > > the > > > > > card is removed. > > > > > > > > So it's only about hdac_hdmi codec, or only about hdac_hda > > > > codec? > > > > > > It is only about the hdac_hda codec. > > > > > > > > And why HDMI codec isn't managed by the card...? IOW, isn't it > > > > dangerous -- it means the codec being always removable after > > > > bound to > > > > the card? > > > > > > That is a good point. Probably this needs to be fixed as well. I > > > can > > > include the change for that if you think it is the right thing to > > > do. > > > > > > But I was wondering if it makes sense to increment the refcount > > > when > > > the device is added to the card with snd_device_new()? > > > I'm not sure how it affects the other devices so didnt go down > > > this > > > route. > > > > If you mean really snd_device_new() calls generically, not specific > > to > > snd_hda_codec_device_new() -- then no, something must be wrong. > > > > And even for snd_hda_codec_device_new(), I'm not sure about it. > > > > Actually, who decrements the device refcount at which timing...? > > It'd be helpful if you can clarify it, then we might see a better > > solution or a better explanation. > > Now I'm reading the code again. Is it about the put_device() in > snd_hdac_ext_bus_device_remove()? > > void snd_hdac_ext_bus_device_remove(struct hdac_bus *bus) > { > struct hdac_device *codec, *__codec; > /* > * we need to remove all the codec devices objects created in > the > * snd_hdac_ext_bus_device_init > */ > list_for_each_entry_safe(codec, __codec, &bus->codec_list, > list) { > snd_hdac_device_unregister(codec); > put_device(&codec->dev); > } > } > > I don't figure out why put_device() is needed at this place... Hi Takashi, No, this actually comes at the second step in the case of SOF (ie after the machine driver is unregistered). Actually, I just found out what's causing the issue. It is the call to snd_hda_codec_dev_free() which calls put_device() when snd_card_free() is invoked. So, adding a get_device() in snd_hda_codec_device_new() would make the refcount balanced. On the other hand, removing the put_device() in snd_hda_codec_dev_free() would also address the problem. I'm not sure which would be the preferred route. THanks, Ranjani > > > Takashi > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@xxxxxxxxxxxxxxxx > https://mailman.alsa-project.org/mailman/listinfo/alsa-devel _______________________________________________ Alsa-devel mailing list Alsa-devel@xxxxxxxxxxxxxxxx https://mailman.alsa-project.org/mailman/listinfo/alsa-devel
