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

[Greg Kroah-Hartman <gregkh@linuxfoundation.org>]

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?

> +/*
> + * The sysfs for properties reflects the MIPI description as given
> + * in the MIPI DisCo spec
> + *
> + * 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 :)

> +
> +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?

> +/*
> + * Slave sysfs
> + */
> +
> +/*
> + * The sysfs for Slave reflects the MIPI description as given
> + * in the MIPI DisCo spec
> + *
> + * Base file is device
> + *	|---- mipi_revision
> + *	|---- wake_capable
> + *	|---- test_mode_capable
> + *	|---- simple_clk_stop_capable
> + *	|---- clk_stop_timeout
> + *	|---- ch_prep_timeout
> + *	|---- reset_behave
> + *	|---- high_PHY_capable
> + *	|---- paging_support
> + *	|---- bank_delay_support
> + *	|---- p15_behave
> + *	|---- master_count
> + *	|---- source_ports
> + *	|---- sink_ports
> + *	|---- dp0
> + *		|---- max_word
> + *		|---- min_word
> + *		|---- words
> + *		|---- flow_controlled
> + *		|---- simple_ch_prep_sm
> + *		|---- device_interrupts
> + *	|---- dpN
> + *		|---- max_word
> + *		|---- min_word
> + *		|---- words
> + *		|---- type
> + *		|---- max_grouping
> + *		|---- simple_ch_prep_sm
> + *		|---- ch_prep_timeout
> + *		|---- device_interrupts
> + *		|---- max_ch
> + *		|---- min_ch
> + *		|---- ch
> + *		|---- ch_combinations
> + *		|---- modes
> + *		|---- max_async_buffer
> + *		|---- block_pack_mode
> + *		|---- port_encoding
> + *		|---- bus_min_freq
> + *		|---- bus_max_freq
> + *		|---- bus_freq
> + *		|---- max_freq
> + *		|---- min_freq
> + *		|---- freq
> + *		|---- prep_ch_behave
> + *		|---- glitchless
> + *
> + */
> +struct sdw_slave_sysfs {
> +	struct sdw_slave *slave;
> +};

Same here, why are you using kobjects for this "device"?  Shouldn't you
use a real struct device for dpX?


> +
> +#define SLAVE_ATTR(type)					\
> +static ssize_t type##_show(struct device *dev,			\
> +		struct device_attribute *attr, char *buf)	\
> +{								\
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);		\
> +	return sprintf(buf, "0x%x\n", slave->prop.type);	\
> +}								\
> +static DEVICE_ATTR_RO(type)

Oh wait, you are?  I'm confused, is this a real device or not?

> +
> +SLAVE_ATTR(mipi_revision);
> +SLAVE_ATTR(wake_capable);
> +SLAVE_ATTR(test_mode_capable);
> +SLAVE_ATTR(clk_stop_mode1);
> +SLAVE_ATTR(simple_clk_stop_capable);
> +SLAVE_ATTR(clk_stop_timeout);
> +SLAVE_ATTR(ch_prep_timeout);
> +SLAVE_ATTR(reset_behave);
> +SLAVE_ATTR(high_PHY_capable);
> +SLAVE_ATTR(paging_support);
> +SLAVE_ATTR(bank_delay_support);
> +SLAVE_ATTR(p15_behave);
> +SLAVE_ATTR(master_count);
> +SLAVE_ATTR(source_ports);
> +
> +static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
> +			     char *buf)
> +{
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +
> +	return sdw_slave_modalias(slave, buf, 256);
> +}
> +static DEVICE_ATTR_RO(modalias);
> +
> +static struct attribute *slave_dev_attrs[] = {
> +	&dev_attr_mipi_revision.attr,
> +	&dev_attr_wake_capable.attr,
> +	&dev_attr_test_mode_capable.attr,
> +	&dev_attr_clk_stop_mode1.attr,
> +	&dev_attr_simple_clk_stop_capable.attr,
> +	&dev_attr_clk_stop_timeout.attr,
> +	&dev_attr_ch_prep_timeout.attr,
> +	&dev_attr_reset_behave.attr,
> +	&dev_attr_high_PHY_capable.attr,
> +	&dev_attr_paging_support.attr,
> +	&dev_attr_bank_delay_support.attr,
> +	&dev_attr_p15_behave.attr,
> +	&dev_attr_master_count.attr,
> +	&dev_attr_source_ports.attr,
> +	&dev_attr_modalias.attr,
> +	NULL,
> +};
> +
> +static struct attribute_group sdw_slave_dev_attr_group = {
> +	.attrs	= slave_dev_attrs,
> +};
> +
> +const struct attribute_group *sdw_slave_dev_attr_groups[] = {
> +	&sdw_slave_dev_attr_group,
> +	NULL
> +};
> diff --git a/include/linux/soundwire/sdw.h b/include/linux/soundwire/sdw.h
> index e752db72a045..fce502d08fef 100644
> --- a/include/linux/soundwire/sdw.h
> +++ b/include/linux/soundwire/sdw.h
> @@ -308,6 +308,15 @@ int sdw_master_read_prop(struct sdw_bus *bus);
>  int sdw_slave_read_prop(struct sdw_slave *slave);
>  
>  /*
> + * SDW sysfs APIs
> + */
> +struct sdw_slave_sysfs;
> +struct sdw_master_sysfs;
> +
> +int sdw_sysfs_bus_init(struct sdw_bus *bus);

How are you handling the sysfs files showing up "after" the device is
registered and userspace not seeing them?  Have you tried using libudev
to get the attributes?  Does this all work properly?  I think with the
use of a kobject that breaks, and that's not good.


> +void sdw_sysfs_bus_exit(struct sdw_bus *bus);
> +
> +/*
>   * SDW Slave Structures and APIs
>   */
>  
> @@ -363,6 +372,7 @@ struct sdw_slave_ops {
>   * @bus: Bus handle
>   * @ops: Slave callback ops
>   * @prop: Slave properties
> + * @sysfs: Sysfs interface
>   * @node: node for bus list
>   * @port_ready: Port ready completion flag for each Slave port
>   * @dev_num: Device Number assigned by Bus
> @@ -374,6 +384,7 @@ struct sdw_slave {
>  	struct sdw_bus *bus;
>  	const struct sdw_slave_ops *ops;
>  	struct sdw_slave_prop prop;
> +	struct sdw_slave_sysfs *sysfs;

So a differently reference counted device is hanging off of this?
That's the big objection to using a kobject here and should have been a
clue that this isn't ok.

>  	struct list_head node;
>  	struct completion *port_ready;
>  	u16 dev_num;
> @@ -453,6 +464,7 @@ struct sdw_master_ops {
>   * @msg_lock: message lock
>   * @ops: Master callback ops
>   * @prop: Master properties
> + * @sysfs: Bus sysfs
>   * @defer_msg: Defer message
>   * @clk_stop_timeout: Clock stop timeout computed
>   */
> @@ -465,6 +477,7 @@ struct sdw_bus {
>  	struct mutex msg_lock;
>  	const struct sdw_master_ops *ops;
>  	struct sdw_master_prop prop;
> +	struct sdw_master_sysfs *sysfs;

Same here.

thanks,

greg k-h