[PATCH 0/2] soundwire: sysfs: expose device number and status

[PATCH 0/2] soundwire: sysfs: expose device number and status

[Pierre-Louis Bossart]

This patchset combines three contributions:

Srinivas Kandagalta suggested creating a device when it's detected on
the bus but not described in platform firmware. I suggested adding the
device number and status to show the difference with 'ghost' devices,
described in firmware but not physically present. Vinod suggested a
simpler way to report the status.

I did not keep Vinod's patch separate since it was using the same
group attribute as the other properties, which prevents status and
device number from being reported if there is no firmware and no
driver.

These patches provide a nice addition for integrators/tests and were
tested on Qualcomm and Intel platforms. Thanks Srinivas for the
initial idea!

Pierre-Louis Bossart (1):
  soundwire: sysfs: add slave status and device number before probe

Srinivas Kandagatla (1):
  soundwire: bus: add enumerated Slave device to device list

 .../ABI/testing/sysfs-bus-soundwire-slave     | 18 ++++++
 drivers/soundwire/bus.c                       |  9 +++
 drivers/soundwire/bus.h                       |  2 +
 drivers/soundwire/bus_type.c                  |  9 +++
 drivers/soundwire/slave.c                     |  6 +-
 drivers/soundwire/sysfs_local.h               |  4 ++
 drivers/soundwire/sysfs_slave.c               | 64 ++++++++++++++++++-
 7 files changed, 109 insertions(+), 3 deletions(-)

-- 
2.25.1

[PATCH 1/2] soundwire: bus: add enumerated Slave device to device list

[Pierre-Louis Bossart]

From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>

Currently Slave devices are only added on startup, either from Device
Tree or ACPI entries. However Slave devices that are physically
present on the bus, but not described in platform firmware, will never
be added to the device list. The user/integrator can only know the
list of devices by looking a dynamic debug logs.

This patch suggests adding a Slave device even where there is no
matching DT or ACPI entry, so that we can see this in sysfs entry.

Initial code from Srinivas. Comments, fixes for ACPI probe and edits
of commit message by Pierre.

Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 drivers/soundwire/bus.c      | 9 +++++++++
 drivers/soundwire/bus.h      | 2 ++
 drivers/soundwire/bus_type.c | 9 +++++++++
 drivers/soundwire/slave.c    | 4 ++--
 4 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/drivers/soundwire/bus.c b/drivers/soundwire/bus.c
index 02574b4bb179..81807b332a12 100644
--- a/drivers/soundwire/bus.c
+++ b/drivers/soundwire/bus.c
@@ -741,6 +741,15 @@ static int sdw_program_device_num(struct sdw_bus *bus)
 
 		if (!found) {
 			/* TODO: Park this device in Group 13 */
+
+			/*
+			 * add Slave device even if there is no platform
+			 * firmware description. There will be no driver probe
+			 * but the user/integration will be able to see the
+			 * device, enumeration status and device number in sysfs
+			 */
+			sdw_slave_add(bus, &id, NULL);
+
 			dev_err(bus->dev, "Slave Entry not found\n");
 		}
 
diff --git a/drivers/soundwire/bus.h b/drivers/soundwire/bus.h
index c53345fbc4c7..fd251c1eb925 100644
--- a/drivers/soundwire/bus.h
+++ b/drivers/soundwire/bus.h
@@ -19,6 +19,8 @@ static inline int sdw_acpi_find_slaves(struct sdw_bus *bus)
 int sdw_of_find_slaves(struct sdw_bus *bus);
 void sdw_extract_slave_id(struct sdw_bus *bus,
 			  u64 addr, struct sdw_slave_id *id);
+int sdw_slave_add(struct sdw_bus *bus, struct sdw_slave_id *id,
+		  struct fwnode_handle *fwnode);
 int sdw_master_device_add(struct sdw_bus *bus, struct device *parent,
 			  struct fwnode_handle *fwnode);
 int sdw_master_device_del(struct sdw_bus *bus);
diff --git a/drivers/soundwire/bus_type.c b/drivers/soundwire/bus_type.c
index 6fba55898cf0..575b9bad99d5 100644
--- a/drivers/soundwire/bus_type.c
+++ b/drivers/soundwire/bus_type.c
@@ -84,6 +84,15 @@ static int sdw_drv_probe(struct device *dev)
 	const struct sdw_device_id *id;
 	int ret;
 
+	/*
+	 * fw description is mandatory to bind
+	 */
+	if (!dev->fwnode)
+		return -ENODEV;
+
+	if (!IS_ENABLED(CONFIG_ACPI) && !dev->of_node)
+		return -ENODEV;
+
 	id = sdw_get_device_id(slave, drv);
 	if (!id)
 		return -ENODEV;
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 4a250d33de5d..19b012310c29 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -20,8 +20,8 @@ struct device_type sdw_slave_type = {
 	.uevent =	sdw_slave_uevent,
 };
 
-static int sdw_slave_add(struct sdw_bus *bus,
-			 struct sdw_slave_id *id, struct fwnode_handle *fwnode)
+int sdw_slave_add(struct sdw_bus *bus,
+		  struct sdw_slave_id *id, struct fwnode_handle *fwnode)
 {
 	struct sdw_slave *slave;
 	int ret;
-- 
2.25.1

[PATCH 2/2] soundwire: sysfs: add slave status and device number before probe

[Pierre-Louis Bossart]

The MIPI DisCo device properties that are read by the driver from
platform firmware, or hard-coded in the driver, should only be
provided as sysfs entries when a driver probes successfully.

However the device status and device number is updated even when there
is no driver present, and hence can be updated when a Slave device is
detected on the bus without being described in platform firmware and
without any driver registered/probed.

The attr group added for Slave status and device number is not handled
by devm_ routines to avoid errors such as "Resources present before
probing". Since device_del() explicitly removes attribute groups, only
an init function is provided.

Credits to Vinod Koul for the status_show() function, shared in a
separate patch and used as is here. The status table was modified to
remove an unnecessary enum and status_show() is handled in a different
group attribute than what was suggested by Vinod.

Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
Tested-by: Srinivas Kandgatla <srinivas.kandagatla@linaro.org>
Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
---
 .../ABI/testing/sysfs-bus-soundwire-slave     | 18 ++++++
 drivers/soundwire/slave.c                     |  2 +
 drivers/soundwire/sysfs_local.h               |  4 ++
 drivers/soundwire/sysfs_slave.c               | 64 ++++++++++++++++++-
 4 files changed, 87 insertions(+), 1 deletion(-)

diff --git a/Documentation/ABI/testing/sysfs-bus-soundwire-slave b/Documentation/ABI/testing/sysfs-bus-soundwire-slave
index db4c9511d1aa..425adf7b8aec 100644
--- a/Documentation/ABI/testing/sysfs-bus-soundwire-slave
+++ b/Documentation/ABI/testing/sysfs-bus-soundwire-slave
@@ -1,3 +1,21 @@
+What:		/sys/bus/soundwire/devices/sdw:.../dev-status/status
+		/sys/bus/soundwire/devices/sdw:.../dev-status/device_number
+
+Date:		September 2020
+
+Contact:	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
+		Bard Liao <yung-chuan.liao@linux.intel.com>
+		Vinod Koul <vkoul@kernel.org>
+
+Description:	SoundWire Slave status
+
+		These properties report the Slave status, e.g. if it
+		is UNATTACHED or not, and in the latter case show the
+		device_number. This status information is useful to
+		detect devices exposed by platform firmware but not
+		physically present on the bus, and conversely devices
+		not exposed in platform firmware but enumerated.
+
 What:		/sys/bus/soundwire/devices/sdw:.../dev-properties/mipi_revision
 		/sys/bus/soundwire/devices/sdw:.../dev-properties/wake_capable
 		/sys/bus/soundwire/devices/sdw:.../dev-properties/test_mode_capable
diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
index 19b012310c29..ee554a215e44 100644
--- a/drivers/soundwire/slave.c
+++ b/drivers/soundwire/slave.c
@@ -6,6 +6,7 @@
 #include <linux/soundwire/sdw.h>
 #include <linux/soundwire/sdw_type.h>
 #include "bus.h"
+#include "sysfs_local.h"
 
 static void sdw_slave_release(struct device *dev)
 {
@@ -83,6 +84,7 @@ int sdw_slave_add(struct sdw_bus *bus,
 		return ret;
 	}
 	sdw_slave_debugfs_init(slave);
+	sdw_slave_status_sysfs_init(slave);
 
 	return ret;
 }
diff --git a/drivers/soundwire/sysfs_local.h b/drivers/soundwire/sysfs_local.h
index ff60adee3c41..52f2eabf5b4b 100644
--- a/drivers/soundwire/sysfs_local.h
+++ b/drivers/soundwire/sysfs_local.h
@@ -8,6 +8,10 @@
  * SDW sysfs APIs -
  */
 
+/* basic routine to report status of Slave (attachment, dev_num) */
+int sdw_slave_status_sysfs_init(struct sdw_slave *slave);
+
+/* additional device-managed properties reported after driver probe */
 int sdw_slave_sysfs_init(struct sdw_slave *slave);
 int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave);
 
diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c
index f510071b0add..36eaca343601 100644
--- a/drivers/soundwire/sysfs_slave.c
+++ b/drivers/soundwire/sysfs_slave.c
@@ -16,10 +16,14 @@
 
 /*
  * The sysfs for Slave reflects the MIPI description as given
- * in the MIPI DisCo spec
+ * in the MIPI DisCo spec. dev-status properties come directly
+ * from the MIPI SoundWire specification.
  *
  * Base file is device
  *	|---- modalias
+ *	|---- dev-status
+ *		|---- status
+ *		|---- device_number
  *	|---- dev-properties
  *		|---- mipi_revision
  *		|---- wake_capable
@@ -212,3 +216,61 @@ int sdw_slave_sysfs_init(struct sdw_slave *slave)
 
 	return 0;
 }
+
+/*
+ * the status is shown in capital letters for UNATTACHED and RESERVED
+ * on purpose, to alert users to the fact that these status values are
+ * not expected.
+ */
+static const char *const slave_status[] = {
+	[SDW_SLAVE_UNATTACHED] =  "UNATTACHED",
+	[SDW_SLAVE_ATTACHED] = "Attached",
+	[SDW_SLAVE_ALERT] = "Alert",
+	[SDW_SLAVE_RESERVED] = "RESERVED",
+};
+
+static ssize_t status_show(struct device *dev,
+			   struct device_attribute *attr, char *buf)
+{
+	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+
+	return sprintf(buf, "%s\n", slave_status[slave->status]);
+}
+static DEVICE_ATTR_RO(status);
+
+static ssize_t device_number_show(struct device *dev,
+				  struct device_attribute *attr, char *buf)
+{
+	struct sdw_slave *slave = dev_to_sdw_dev(dev);
+
+	if (slave->status == SDW_SLAVE_UNATTACHED)
+		return sprintf(buf, "%s", "N/A");
+	else
+		return sprintf(buf, "%d", slave->dev_num);
+}
+static DEVICE_ATTR_RO(device_number);
+
+static struct attribute *slave_status_attrs[] = {
+	&dev_attr_status.attr,
+	&dev_attr_device_number.attr,
+	NULL,
+};
+
+/*
+ * we don't use ATTRIBUTES_GROUP here since we want to add a subdirectory
+ * for device-status
+ */
+static struct attribute_group sdw_slave_status_attr_group = {
+	.attrs	= slave_status_attrs,
+	.name = "dev-status",
+};
+
+/*
+ * We can't use devm_ here, otherwise resources would be added before
+ * the driver probe. The group is removed in device_del() however.
+ */
+
+int sdw_slave_status_sysfs_init(struct sdw_slave *slave)
+{
+	return device_add_group(&slave->dev, &sdw_slave_status_attr_group);
+}
-- 
2.25.1

Re: [PATCH 2/2] soundwire: sysfs: add slave status and device number before probe

[Greg KH]

On Wed, Sep 16, 2020 at 03:15:04PM -0500, Pierre-Louis Bossart wrote:
> The MIPI DisCo device properties that are read by the driver from
> platform firmware, or hard-coded in the driver, should only be
> provided as sysfs entries when a driver probes successfully.
> 
> However the device status and device number is updated even when there
> is no driver present, and hence can be updated when a Slave device is
> detected on the bus without being described in platform firmware and
> without any driver registered/probed.
> 
> The attr group added for Slave status and device number is not handled
> by devm_ routines to avoid errors such as "Resources present before
> probing". Since device_del() explicitly removes attribute groups, only
> an init function is provided.
> 
> Credits to Vinod Koul for the status_show() function, shared in a
> separate patch and used as is here. The status table was modified to
> remove an unnecessary enum and status_show() is handled in a different
> group attribute than what was suggested by Vinod.
> 
> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com>
> Reviewed-by: Rander Wang <rander.wang@linux.intel.com>
> Reviewed-by: Guennadi Liakhovetski <guennadi.liakhovetski@linux.intel.com>
> Tested-by: Srinivas Kandgatla <srinivas.kandagatla@linaro.org>
> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> ---
>  .../ABI/testing/sysfs-bus-soundwire-slave     | 18 ++++++
>  drivers/soundwire/slave.c                     |  2 +
>  drivers/soundwire/sysfs_local.h               |  4 ++
>  drivers/soundwire/sysfs_slave.c               | 64 ++++++++++++++++++-
>  4 files changed, 87 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-soundwire-slave b/Documentation/ABI/testing/sysfs-bus-soundwire-slave
> index db4c9511d1aa..425adf7b8aec 100644
> --- a/Documentation/ABI/testing/sysfs-bus-soundwire-slave
> +++ b/Documentation/ABI/testing/sysfs-bus-soundwire-slave
> @@ -1,3 +1,21 @@
> +What:		/sys/bus/soundwire/devices/sdw:.../dev-status/status
> +		/sys/bus/soundwire/devices/sdw:.../dev-status/device_number
> +
> +Date:		September 2020
> +
> +Contact:	Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
> +		Bard Liao <yung-chuan.liao@linux.intel.com>
> +		Vinod Koul <vkoul@kernel.org>
> +
> +Description:	SoundWire Slave status
> +
> +		These properties report the Slave status, e.g. if it
> +		is UNATTACHED or not, and in the latter case show the
> +		device_number. This status information is useful to
> +		detect devices exposed by platform firmware but not
> +		physically present on the bus, and conversely devices
> +		not exposed in platform firmware but enumerated.
> +
>  What:		/sys/bus/soundwire/devices/sdw:.../dev-properties/mipi_revision
>  		/sys/bus/soundwire/devices/sdw:.../dev-properties/wake_capable
>  		/sys/bus/soundwire/devices/sdw:.../dev-properties/test_mode_capable
> diff --git a/drivers/soundwire/slave.c b/drivers/soundwire/slave.c
> index 19b012310c29..ee554a215e44 100644
> --- a/drivers/soundwire/slave.c
> +++ b/drivers/soundwire/slave.c
> @@ -6,6 +6,7 @@
>  #include <linux/soundwire/sdw.h>
>  #include <linux/soundwire/sdw_type.h>
>  #include "bus.h"
> +#include "sysfs_local.h"
>  
>  static void sdw_slave_release(struct device *dev)
>  {
> @@ -83,6 +84,7 @@ int sdw_slave_add(struct sdw_bus *bus,
>  		return ret;
>  	}
>  	sdw_slave_debugfs_init(slave);
> +	sdw_slave_status_sysfs_init(slave);
>  
>  	return ret;
>  }
> diff --git a/drivers/soundwire/sysfs_local.h b/drivers/soundwire/sysfs_local.h
> index ff60adee3c41..52f2eabf5b4b 100644
> --- a/drivers/soundwire/sysfs_local.h
> +++ b/drivers/soundwire/sysfs_local.h
> @@ -8,6 +8,10 @@
>   * SDW sysfs APIs -
>   */
>  
> +/* basic routine to report status of Slave (attachment, dev_num) */
> +int sdw_slave_status_sysfs_init(struct sdw_slave *slave);
> +
> +/* additional device-managed properties reported after driver probe */
>  int sdw_slave_sysfs_init(struct sdw_slave *slave);
>  int sdw_slave_sysfs_dpn_init(struct sdw_slave *slave);
>  
> diff --git a/drivers/soundwire/sysfs_slave.c b/drivers/soundwire/sysfs_slave.c
> index f510071b0add..36eaca343601 100644
> --- a/drivers/soundwire/sysfs_slave.c
> +++ b/drivers/soundwire/sysfs_slave.c
> @@ -16,10 +16,14 @@
>  
>  /*
>   * The sysfs for Slave reflects the MIPI description as given
> - * in the MIPI DisCo spec
> + * in the MIPI DisCo spec. dev-status properties come directly
> + * from the MIPI SoundWire specification.
>   *
>   * Base file is device
>   *	|---- modalias
> + *	|---- dev-status
> + *		|---- status
> + *		|---- device_number
>   *	|---- dev-properties
>   *		|---- mipi_revision
>   *		|---- wake_capable
> @@ -212,3 +216,61 @@ int sdw_slave_sysfs_init(struct sdw_slave *slave)
>  
>  	return 0;
>  }
> +
> +/*
> + * the status is shown in capital letters for UNATTACHED and RESERVED
> + * on purpose, to alert users to the fact that these status values are
> + * not expected.
> + */
> +static const char *const slave_status[] = {
> +	[SDW_SLAVE_UNATTACHED] =  "UNATTACHED",
> +	[SDW_SLAVE_ATTACHED] = "Attached",
> +	[SDW_SLAVE_ALERT] = "Alert",
> +	[SDW_SLAVE_RESERVED] = "RESERVED",
> +};
> +
> +static ssize_t status_show(struct device *dev,
> +			   struct device_attribute *attr, char *buf)
> +{
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +
> +	return sprintf(buf, "%s\n", slave_status[slave->status]);
> +}
> +static DEVICE_ATTR_RO(status);
> +
> +static ssize_t device_number_show(struct device *dev,
> +				  struct device_attribute *attr, char *buf)
> +{
> +	struct sdw_slave *slave = dev_to_sdw_dev(dev);
> +
> +	if (slave->status == SDW_SLAVE_UNATTACHED)
> +		return sprintf(buf, "%s", "N/A");
> +	else
> +		return sprintf(buf, "%d", slave->dev_num);
> +}
> +static DEVICE_ATTR_RO(device_number);
> +
> +static struct attribute *slave_status_attrs[] = {
> +	&dev_attr_status.attr,
> +	&dev_attr_device_number.attr,
> +	NULL,
> +};
> +
> +/*
> + * we don't use ATTRIBUTES_GROUP here since we want to add a subdirectory
> + * for device-status
> + */
> +static struct attribute_group sdw_slave_status_attr_group = {
> +	.attrs	= slave_status_attrs,
> +	.name = "dev-status",
> +};
> +
> +/*
> + * We can't use devm_ here, otherwise resources would be added before
> + * the driver probe. The group is removed in device_del() however.
> + */
> +
> +int sdw_slave_status_sysfs_init(struct sdw_slave *slave)
> +{
> +	return device_add_group(&slave->dev, &sdw_slave_status_attr_group);

DOesn't this race with userspace?  Why not make this part of the default
set of device attributes and have the driver core create them
automatically?

thanks,

greg k-h

Re: [PATCH 2/2] soundwire: sysfs: add slave status and device number before probe

[Pierre-Louis Bossart]

Thanks for the review Greg,

>> +int sdw_slave_status_sysfs_init(struct sdw_slave *slave)
>> +{
>> +	return device_add_group(&slave->dev, &sdw_slave_status_attr_group);
> 
> DOesn't this race with userspace?  Why not make this part of the default
> set of device attributes and have the driver core create them
> automatically?

What did you mean by 'default set of device attributes', would you mind 
providing a pointer to an example so that I can look into this?

What we have in this patch is added by the SoundWire core but you're 
probably thinking of something else. thanks for enlightening the rest of 
us:-)

Re: [PATCH 2/2] soundwire: sysfs: add slave status and device number before probe

[Greg KH]

On Thu, Sep 17, 2020 at 07:54:50AM -0500, Pierre-Louis Bossart wrote:
> Thanks for the review Greg,
> 
> > > +int sdw_slave_status_sysfs_init(struct sdw_slave *slave)
> > > +{
> > > +	return device_add_group(&slave->dev, &sdw_slave_status_attr_group);
> > 
> > DOesn't this race with userspace?  Why not make this part of the default
> > set of device attributes and have the driver core create them
> > automatically?
> 
> What did you mean by 'default set of device attributes', would you mind
> providing a pointer to an example so that I can look into this?

Set the "groups" pointer in any one of the normal driver core structures
(struct device_type, struct device, struct device_driver), or the
"bus_groups", "dev_groups", or "drv_groups" pointer in struct bus_type.

that should give you something to go on :)

thanks,

greg k-h