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

[Heikki Krogerus <heikki.krogerus@linux.intel.com>]

On Tue, Apr 13, 2021 at 10:47:49AM -0500, Pierre-Louis Bossart wrote:
> 
> 
> 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?

I'll send it out now. Let's see what happens.

thanks,

> > > 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.

-- 
heikki