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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 13 Apr 2021 10:47:49 -0500
From:   Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To:     Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc:     Cezary Rojewski <cezary.rojewski@...el.com>,
        Kai Vehmanen <kai.vehmanen@...ux.intel.com>,
        Jie Yang <yang.jie@...ux.intel.com>,
        linux-kernel@...r.kernel.org, Takashi Iwai <tiwai@...e.com>,
        Liam Girdwood <liam.r.girdwood@...ux.intel.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Mark Brown <broonie@...nel.org>, alsa-devel@...a-project.org,
        Bard Liao <yung-chuan.liao@...ux.intel.com>
Subject: Re: [PATCH] ASoC: Intel: Handle device properties with software node
 API



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.