Re: [PATCH] ASoC: Intel: Handle device properties with software node API

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

On 4/13/21 9:05 AM, Heikki Krogerus wrote:
> On Tue, Apr 13, 2021 at 03:20:45PM +0300, Heikki Krogerus wrote:
>> On Mon, Apr 12, 2021 at 03:36:20PM -0500, Pierre-Louis Bossart wrote:
>>> I took the code and split it in two for BYT/CHT (modified to remove devm_)
>>> and SoundWire parts (added as is).
>>>
>>> https://github.com/thesofproject/linux/pull/2810
>>>
>>> Both cases result in a refcount error on device_remove_sof when removing the
>>> platform device. I don't understand the code well enough to figure out what
>>> happens, but it's likely a case of the software node being removed twice?
>>
>> Right. Because you are injecting the node to an already existing
>> device, the node does not get linked with the device in sysfs. That
>> would increment the reference count in a normal case. It all happens
>> in the function software_node_notify(). Driver core calls it when a
>> device is added and also when it's removed. In this case it is only
>> called when it's removed.
>>
>> I think the best way to handle this now is to simply not decrementing
>> the ref count when you add the properties, so don't call
>> fwnode_handle_put() there (but add a comment explaining why you are
>> not calling it).
> 
> No, sorry... That's just too hacky. Let's not do that after all.
> 
> We can also fix this in the software node code. I'm attaching a patch
> that should make it possible to inject the software nodes also
> afterwards safely. This is definitely also not without its problems,
> but we can go with that if it works. Let me know.

I tested manually on bytcr w/ RT5640 and used the SOF CI farm to test 
the SoundWire cases, I don't see any issues with your patch. The 
refcount issue is gone and the module load/unload cycles don't report 
any problems.

Would you queue it for 5.13-rc1, or is this too late already?

>> For a better solution you would call device_reprobe() after you have
>> injected the software node, but before that you need to modify
>> device_reprobe() so it calls device_platform_notify() (which it really
>> should call in any case). But this should probable be done later,
>> separately.
>>
>> thanks,
>>
>> P.S.
>>
>> Have you guys considered the possibility of describing the connections
>> between all these components by using one of the methods that we now
>> have for that in kernel, for example device graph? It can now be
>> used also with software nodes (OF graph and ACPI device graph are of
>> course already fully supported).

I must admit I've never heard of a 'device graph'. Any pointers or APIs 
you can think of?
It's a good comment since we are planning to rework the SOF clients and 
machine driver handling.