ASoC: Intel: boards: use software node API #2810
Conversation
plbossart
requested review from
dbaluta,
kv2019i,
lgirdwood and
ranj063
as code owners
plbossart
force-pushed
the
fix/device-properties2
branch
from
62f9782 to
8d68b39
Compare
|
turns out the helper is not needed, as mentioned by @jwrdegoede in previous comments. Rework and update (put_device() use was incorrect). |
plbossart
force-pushed
the
fix/device-properties2
branch
from
8d68b39 to
4983c3b
Compare
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
not really familiar with this API, but FWIW LGTM
|
@jwrdegoede any comments? I struggled on the atom drivers since they all use a different structure, but I think this is correct now. |
|
also forgot to ask @andy-shev for feedback. |
Code looks good. Just want to be sure I understand the lifetime constraints. You have a board file (where this code is applied) which lives shorter or longer than codec device. Is it correct? And actually which is the right one (shorter or longer)? In some cases one may use https://elixir.bootlin.com/linux/v5.12-rc5/source/drivers/base/swnode.c#L1076 but with care. |
The platform devices and their drivers (modified here) depend on the functionality provided by the codec device/drivers, so the machine drivers will live for shorter time than the codec ones. The machine platform device is created AFTER we detect the presence of a codec device, and conversely we remove the machine platform device BEFORE removing the codec device. That was the main argument behind NOT using device managed APIs, as Heikki did in his initial patch submitted upstream, as we are adding the properties on the codec device but from the machine driver, so when we remove the machine device the properties are not removed. It's much better to add/remove properties in the same scope, we've been there before and using devm_ creates lots of issues in our load/unload tests. That's what we are doing for SoundWire machine drivers, but for old Baytrail stuff we didn't, so we have to add an explicit remove step in the machine platform driver. |
Right, that explains how above patch is organazed and which APIs it uses. Thanks! To me it's clear and correct solution. |
|
Something's wrong here, there is a refcount error when removing the device software node Not sure what's happening here. full dmesg here |
|
same issue with SoundWire devices, and this was already reported in CI results but the results are PASS @marc-hb @fredoh9 @aiChaoSONG any idea why we pass a test with such a blatant refcount issue? |
Probably because we don't have a culture of "testing the tests" and we're just not trying hard to find issues in general. A test that never reported any failure hasn't been tested and probably does not work, yet for some reasons we consider passing tests as good enough to make even major tests changes. I've been trying to change this "green failure" culture for some time but finally understood it's not anyone's priority. We didn't even have CI for sof-test when I joined, so not all is bad :-) I see the dmesg.txt at that link does not have any "error" keyword, that could be why. What is the log level of this Call Trace? We love extending
Etc., these are just some relevant examples from the top of my head. |
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). > > > > thesofproject#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. > 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). Br, -- heikki From 3d3dca1f80941f8975390bc8f488176d00acef22 Mon Sep 17 00:00:00 2001 From: Heikki Krogerus <heikki.krogerus@linux.intel.com> Date: Tue, 13 Apr 2021 16:28:34 +0300 Subject: [PATCH] software node: Allow node addition to already existing device If the node is added to an already exiting device, the node needs to be also linked to the device separately. This will make sure the reference count is kept in balance also when the node is injected to a device afterwards. Reported-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Fixes: e68d011 ("software node: Introduce device_add_software_node()") Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
plbossart
force-pushed
the
fix/device-properties2
branch
from
4983c3b to
c102eb6
Compare
|
The first patch in this PR was merged by GregKH, so we can merge and contribute the two other patches upstream after v5.13-rc1 |
|
rebased to make SoundWire error go away |
plbossart
force-pushed
the
fix/device-properties2
branch
from
c102eb6 to
72848ae
Compare
The function device_add_properties() is going to be removed. Replacing it with software node API equivalents. Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
plbossart
force-pushed
the
fix/device-properties2
branch
from
72848ae to
33126f3
Compare
|
Rebased on top of v5.13-rc1 to make initial commit go away. This should be good to merge and send upstream now. |
lyakh
left a comment
lyakh
left a comment
There was a problem hiding this comment.
nothing significant, maybe a minor future clean up
The function device_add_properties() is going to be removed. Replacing it with software node API equivalents. Co-developed-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com>
plbossart
force-pushed
the
fix/device-properties2
branch
from
33126f3 to
5f50121
Compare
|
I'll go ahead and merge, this PR has been around for quite some time and I want to send the patches upstream. |
5 participants
Updated from initial upstream contribution by Heikki Krogerius