Re: [alsa-devel] [PATCH v5 10/15] soundwire: Add sysfs for SoundWire DisCo properties

[Greg Kroah-Hartman]

On Fri, Dec 22, 2017 at 01:56:56PM +0530, Vinod Koul wrote:
> On Wed, Dec 13, 2017 at 05:59:22PM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Dec 13, 2017 at 10:22:46PM +0530, Vinod Koul wrote:
> > > On Wed, Dec 13, 2017 at 05:28:21PM +0100, Greg Kroah-Hartman wrote:
> > > > On Wed, Dec 13, 2017 at 03:24:30PM +0530, Vinod Koul wrote:
> > > > > On Wed, Dec 13, 2017 at 10:15:37AM +0100, Greg Kroah-Hartman wrote:
> > > > > > On Wed, Dec 06, 2017 at 09:17:06PM +0530, Vinod Koul wrote:
> > > > > > > It helps to read the properties for understanding and debugging
> > > > > > > systems, so add sysfs files for SoundWire DisCo properties.
> > > > > > > 
> > > > > > > TODO: Add ABI files for sysfs
> > > > > > 
> > > > > > Is this TODO done?
> > > > > 
> > > > > Nope sorry not yet. But before this get merged will add
> > > > > 
> > > > > > > + * Base file is:
> > > > > > > + *		properties
> > > > > > > + *		|---- interface-revision
> > > > > > > + *		|---- master-count
> > > > > > > + *		|---- link-N
> > > > > > > + *		      |---- clock-stop-modes
> > > > > > > + *		      |---- max-clock-frequency
> > > > > > > + *		      |---- clock-frequencies
> > > > > > > + *		      |---- default-frame-rows
> > > > > > > + *		      |---- default-frame-cols
> > > > > > > + *		      |---- dynamic-frame-shape
> > > > > > > + *		      |---- command-error-threshold
> > > > > > > + */
> > > > > > 
> > > > > > Why nest them so deep?  Anyway, that's not really an issue I guess, it's
> > > > > > your ABI, not mine :)
> > > > > 
> > > > > well it gives us a hierarchical view. We have N links...
> > > > 
> > > > That's fine, then make it a real 'struct device' if you want to have a
> > > > reference counted object.  Tie it to your bus, and you are good to go.
> > > > Don't use a raw kobject as that totaly breaks the device heirachy in the
> > > > kernel as well as preventing any of these attributes from being accessed
> > > > by userspace libraries (i.e. libudev.)
> > > > 
> > > > > > > +
> > > > > > > +struct sdw_master_sysfs {
> > > > > > > +	struct kobject kobj;
> > > > > > > +	struct sdw_bus *bus;
> > > > > > 
> > > > > > Huh?  Why do you need to use kobjects?
> > > > > > 
> > > > > > When you switch from using a 'struct device' and hang a kobject off of
> > > > > > it, that's a huge signal that something is wrong here.  That kobject
> > > > > > will now no longer be part of the device "chain" in the system, uevents
> > > > > > will get odd, and other strange things can happen.
> > > > > > 
> > > > > > Why can't you just use "normal" attributes attached to the device?  You
> > > > > > shouldn't need a kobject here.  What am I missing?
> > > > > 
> > > > > Okay my understanding might be incorrect then. So we can have N links in
> > > > > the system and each link would have a kobject for "link-N". Not sure how
> > > > > device attributes would do link-N/clock-stop-modes and so on, if they can
> > > > > let me know how and I will surely change that.
> > > > 
> > > > You can create a subdirectory for attributes quite easily.  If you don't
> > > > want to make it a "full" object, and all you care about is the
> > > > subdirectory, then do it that way.  Otherwise use a 'struct device'
> > > > please.
> > > 
> > > Okay thanks this makes sense, yes all we do care about is creating
> > > subdirectories and attributes under them. So in that sense we don't care
> > > much about adding kobjects, it was means to the end.
> > > 
> > > So do you mind pointing an example, I though the way for that was kobjects
> > > by looking at few examples I saw.
> > 
> > Create a dynamic attribute group on the fly and initialize it and set
> > the name to the name you want for the subdirectory.  I know it's done in
> > the kernel in some places, dig around :)
> 
> Hey Greg,
> 
> So I have spent couple of days on this but don't have a solution.
> 
> I can create a subdirectory using dynamic attribute group by giving it a
> name. Works as advertised:). But I am unable to create subdirectory under
> newly created subdir. The properties defined by MIPI follow a certain
> hierarchy so we need subdirectories to represent that.

Yes, you can't create directories 2 levels deep, sorry, didn't realize
you wanted to do that.

> Second issue am facing with these is getting the sdw_bus context. I tried
> adding it but nothing looks elegant. I tried making sdw_attributes on top of
> struct attribute and pass around the bus but doesn't seem to work for me.
> 
> On the contrary, the current design of creating kobjects looked more
> cleaner. It has been used in bunch of other places, I double checked ref
> counting and cleanup looks okay to me. Btw I tested with libudev as well,
> its works well as we do send the event using kobject_uevent().

Where has it been used in other parts of the kernel like this?  Time to
go yell at people :)

And the issue isn't that you will not catch the uevent, but that this is
an attribute of the "parent" device, which is now not a device, but a
"raw" kobject on no bus at all.

So, just create these as real struct devices, with a different "type"
and have them all live on the bus properly.  Look at how the greybus
code does it as one example (USB is another one, but it's much more
complex.)

thanks,

greg k-h