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

[Pierre-Louis Bossart]

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.

> 
>> +		gpiod_put(bdw_rt5677->gpio_hp_en);
>> +}
>