[PATCH 2/5] ASoC: Intel: bdw-rt5677: fix module load/unload issues

[Andy Shevchenko]

On Mon, Jun 22, 2020 at 01:23:04PM -0500, Pierre-Louis Bossart wrote:
> On 6/22/20 1:18 PM, Andy Shevchenko wrote:
> > On Mon, Jun 22, 2020 at 10:42:38AM -0500, Pierre-Louis Bossart wrote:
> > > The mainline code currently prevents modules from being removed.
> > > 
> > > The BE dailink .init() function calls devm_gpiod_get() using the codec
> > > component device as argument. When the machine driver is removed, the
> > > references to the gpiod are not released, and it's not possible to
> > > remove the codec driver module - which is the only entity which could
> > > free the gpiod.
> > > 
> > > This conceptual deadlock can be avoided by invoking gpiod_get() in the
> > > .init() callback, and calling gpiod_put() in the exit() callback.
> > > 
> > > Tested on SAMUS Chromebook with SOF driver.
> > 
> > > +static void bdw_rt5677_exit(struct snd_soc_pcm_runtime *rtd)
> > > +{
> > > +	struct bdw_rt5677_priv *bdw_rt5677 =
> > > +			snd_soc_card_get_drvdata(rtd->card);
> > > +
> > > +	/*
> > > +	 * The .exit() can be reached without going through the .init()
> > > +	 * so explicitly test if the gpiod is valid
> > > +	 */
> > 
> > > +	if (!IS_ERR_OR_NULL(bdw_rt5677->gpio_hp_en))
> > 
> > _OR_NULL is redundant. gpiod_put() is explicitly NULL-aware.
> > 
> > IS_ERR() I suppose can be avoided by using gpiod_get_optional(), if it suits the case.
> > Otherwise it would be questionable why we got error pointer value on ->exit().
> 
> As I explained in the cover letter we can reach this function even if the
> init was not called or was not successful. There are tons of cases which
> reach the same probe_end label in the ASoC core.


> So I explicitly added a test for all possible cases. I don't mind removing
> the _OR_NULL if indeed it's safe, but showing this redundancy also shows an
> intent to deal with such cases.

Thanks for an explanation. I think it's an established practice to have
NULL-aware resource release-like functions.

Do you put always something like below in your code? No.

	if (foo)
		kfree(foo);

-- 
With Best Regards,
Andy Shevchenko