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);
+}
