[alsa-devel] [PATCH] ASoC: hda: increment codec device refcount when it is added to the card

[Ranjani Sridharan]

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 at alsa-project.org
> https://mailman.alsa-project.org/mailman/listinfo/alsa-devel