Re: [PATCH 2/2] ASoC: Intel: boards: use software node API in Atom boards

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 6/8/21 11:02 AM, Andy Shevchenko wrote:
> On Tue, Jun 08, 2021 at 10:18:08AM +0200, Hans de Goede wrote:
>> On 6/8/21 12:35 AM, Pierre-Louis Bossart wrote:
>>> From: Heikki Krogerus <heikki.krogerus@linux.intel.com>
>>>
>>> The function device_add_properties() is going to be removed.
>>> Replacing it with software node API equivalents.
> 
> ...
> 
>>> +	device_remove_software_node(priv->codec_dev);
>>
>> This is a problem, nothing guarantees codec_dev not going away before
>> snd_byt_cht_es8316_mc_remove() runs. Although the only thing which I can come up
>> with where this happens is unbinding the i2c-controller driver I still would like
>> us to take this scenario into account.
>>
>> I think it would be better to use device_create_managed_software_node() to tie
>> the lifetime of the swnode to the lifetime of the device, this also removes
>> the need for all the goto err cases (and introducing a remove function in
>> the bytcr_rt5640.c case).
> 
> Which device? If you are telling about codec, the unload-load of the machine
> driver won't be successful or will produce a leak / warning / etc.

Yes I'm talking about the codec, and yes if the codec device goes away before
the machine-driver is unbound things will likely already break. But the
machine driver does not hold any explicit reference on the codec-device,
so this might happen (I guess there might be a reference somewhere inside the
ASoC code once devm_snd_soc_register_card() has returned successfully).

> If you are telling about machine related device, it simply doesn't belong to it.
> 
> Am I got all this right?
> 
>> This does mean that we could end up calling device_create_managed_software_node()
>> on the same device twice, when the machine driver gets unbound + rebound, but
>> that is an already existing issue with our current device_add_properties() usage.
> 
> Yep.
> 
>> We could fix this (in a separate commit since it is an already existing issue)
>> by adding a PROPERTY_ENTRY_BOOL("cht_es8316,swnode-created") property to the
>> properties and checking for that being set with
>> device_property_read_bool(codec, "cht_es8316,swnode-created")
> 
> Not sure it's a good idea, this sounds like a hack.

Right, which is why I also suggested that device_create_managed_software_node()
could be modified to fail when called a second time on the same device, this
is a check which probably would be good to add regardless. More specifically
I guess that set_secondary_fwnode() could be made to return an error when replacing
an existing secondary fwnode with a non NULL value, rather then just replacing it.

>> Or we could move the device_put(codec_dev) to snd_byt_cht_es8316_mc_remove().
> 
> This sounds better.

As I already mentioned I'm not a fan of all the goto err-s these patches
introduce, this won't fix that.

Regards,

Hans