diff mbox series

[v8,09/14] ASoC: Intel: catpt: Simple sysfs attributes

Message ID 20200923122508.3360-10-cezary.rojewski@intel.com (mailing list archive)
State Superseded
Headers show
Series ASoC: Intel: Catpt - Lynx and Wildcat point | expand

Commit Message

Cezary Rojewski Sept. 23, 2020, 12:25 p.m. UTC
Add sysfs entries for displaying version of FW currently in use as well
as dumping full FW information including build and log-providers hashes.

Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
---

Changes in v7:
- fixed licence header for fs.c
- renamed fs.c to sysfs.c to better match its purpose
- added documentation within Documentation/ABI/testing for entries
  exposed by catpt
- bin_attribute fw_build replaced by attribute fw_info:
  fw_info contains full FW information and after successful handshake,
  it's always available (stored in driver data) so no need to invoke
  GET_FW_VERSION IPC again, just dump the stored information
- rather than manually creating and removing sysfs files, now makes use
  of dev_groups member of struct device_driver 

Changes in v6:
- functions declaration and usage now part of this patch instead of
  being separated from it

Changes in v2:
- fixed size provided to memcpy() in fw_build_read() as reported by Mark

 .../ABI/testing/sysfs-bus-pci-devices-catpt   | 16 ++++++
 sound/soc/intel/catpt/core.h                  |  1 +
 sound/soc/intel/catpt/device.c                |  1 +
 sound/soc/intel/catpt/sysfs.c                 | 57 +++++++++++++++++++
 4 files changed, 75 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-devices-catpt
 create mode 100644 sound/soc/intel/catpt/sysfs.c

Comments

Andy Shevchenko Sept. 23, 2020, 1:34 p.m. UTC | #1
On Wed, Sep 23, 2020 at 02:25:03PM +0200, Cezary Rojewski wrote:
> Add sysfs entries for displaying version of FW currently in use as well
> as dumping full FW information including build and log-providers hashes.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>
> ---
> 
> Changes in v7:
> - fixed licence header for fs.c
> - renamed fs.c to sysfs.c to better match its purpose
> - added documentation within Documentation/ABI/testing for entries
>   exposed by catpt
> - bin_attribute fw_build replaced by attribute fw_info:
>   fw_info contains full FW information and after successful handshake,
>   it's always available (stored in driver data) so no need to invoke
>   GET_FW_VERSION IPC again, just dump the stored information
> - rather than manually creating and removing sysfs files, now makes use
>   of dev_groups member of struct device_driver 
> 
> Changes in v6:
> - functions declaration and usage now part of this patch instead of
>   being separated from it
> 
> Changes in v2:
> - fixed size provided to memcpy() in fw_build_read() as reported by Mark
> 
>  .../ABI/testing/sysfs-bus-pci-devices-catpt   | 16 ++++++
>  sound/soc/intel/catpt/core.h                  |  1 +
>  sound/soc/intel/catpt/device.c                |  1 +
>  sound/soc/intel/catpt/sysfs.c                 | 57 +++++++++++++++++++
>  4 files changed, 75 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-bus-pci-devices-catpt
>  create mode 100644 sound/soc/intel/catpt/sysfs.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci-devices-catpt b/Documentation/ABI/testing/sysfs-bus-pci-devices-catpt
> new file mode 100644
> index 000000000000..8a200f4eefbd
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-catpt
> @@ -0,0 +1,16 @@
> +What:		/sys/devices/pci0000:00/<dev>/fw_version
> +Date:		September 2020
> +Contact:	Cezary Rojewski <cezary.rojewski@intel.com>
> +Description:
> +		Version of AudioDSP firmware ASoC catpt driver is
> +		communicating with.
> +		Format: %d.%d.%d.%d, type:major:minor:build.
> +
> +What:		/sys/devices/pci0000:00/<dev>/fw_info
> +Date:		September 2020
> +Contact:	Cezary Rojewski <cezary.rojewski@intel.com>
> +Description:
> +		Detailed AudioDSP firmware build information including
> +		build hash and log-providers hash. This information is
> +		obtained during initial handshake with firmware.
> +		Format: %s.
> diff --git a/sound/soc/intel/catpt/core.h b/sound/soc/intel/catpt/core.h
> index a29b4c0232cb..19eda125ee59 100644
> --- a/sound/soc/intel/catpt/core.h
> +++ b/sound/soc/intel/catpt/core.h
> @@ -14,6 +14,7 @@
>  #include "registers.h"

>  struct catpt_dev;
> +extern const struct attribute_group *catpt_attr_groups[];

Grouping these are not okay — they are different by meaning, just put blank
line in between.

>  void catpt_sram_init(struct resource *sram, u32 start, u32 size);
>  void catpt_sram_free(struct resource *sram);
> diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c
> index c02d46e5bc81..2d53efa656b1 100644
> --- a/sound/soc/intel/catpt/device.c
> +++ b/sound/soc/intel/catpt/device.c
> @@ -338,6 +338,7 @@ static struct platform_driver catpt_acpi_driver = {
>  		.name = "intel_catpt",
>  		.acpi_match_table = catpt_ids,
>  		.pm = &catpt_dev_pm,
> +		.dev_groups = catpt_attr_groups,
>  	},
>  };
>  module_platform_driver(catpt_acpi_driver);
> diff --git a/sound/soc/intel/catpt/sysfs.c b/sound/soc/intel/catpt/sysfs.c
> new file mode 100644
> index 000000000000..f38562071a04
> --- /dev/null
> +++ b/sound/soc/intel/catpt/sysfs.c
> @@ -0,0 +1,57 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +//
> +// Copyright(c) 2020 Intel Corporation. All rights reserved.
> +//
> +// Author: Cezary Rojewski <cezary.rojewski@intel.com>
> +//
> +
> +#include <linux/pm_runtime.h>
> +#include "core.h"
> +
> +static ssize_t fw_version_show(struct device *dev,
> +			       struct device_attribute *attr, char *buf)
> +{
> +	struct catpt_dev *cdev = dev_get_drvdata(dev);
> +	struct catpt_fw_version version;
> +	int ret;

> +	pm_runtime_get_sync(cdev->dev);
> +
> +	ret = catpt_ipc_get_fw_version(cdev, &version);
> +
> +	pm_runtime_mark_last_busy(cdev->dev);
> +	pm_runtime_put_autosuspend(cdev->dev);

Is it subject to change at run-time?

> +	if (ret)
> +		return CATPT_IPC_ERROR(ret);
> +
> +	return sprintf(buf, "%d.%d.%d.%d\n", version.type, version.major,
> +		       version.minor, version.build);
> +}

> +

This blank line is not needed.

> +static DEVICE_ATTR_RO(fw_version);
> +
> +static ssize_t fw_info_show(struct device *dev,
> +			    struct device_attribute *attr, char *buf)
> +{
> +	struct catpt_dev *cdev = dev_get_drvdata(dev);
> +
> +	return sprintf(buf, "%s\n", cdev->ipc.config.fw_info);
> +}

> +

This blank line is not needed.

> +static DEVICE_ATTR_RO(fw_info);
> +
> +static struct attribute *catpt_attrs[] = {
> +	&dev_attr_fw_version.attr,
> +	&dev_attr_fw_info.attr,
> +	NULL
> +};
> +
> +static const struct attribute_group catpt_attr_group = {
> +	.attrs = catpt_attrs,
> +};
> +
> +const struct attribute_group *catpt_attr_groups[] = {
> +	&catpt_attr_group,
> +	NULL
> +};
> -- 
> 2.17.1
>
Greg KH Sept. 23, 2020, 5:17 p.m. UTC | #2
On Wed, Sep 23, 2020 at 02:25:03PM +0200, Cezary Rojewski wrote:
> Add sysfs entries for displaying version of FW currently in use as well
> as dumping full FW information including build and log-providers hashes.
> 
> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com>

Much nicer:

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cezary Rojewski Sept. 23, 2020, 6:31 p.m. UTC | #3
On 2020-09-23 3:34 PM, Andy Shevchenko wrote:
> On Wed, Sep 23, 2020 at 02:25:03PM +0200, Cezary Rojewski wrote:
>> Add sysfs entries for displaying version of FW currently in use as well
>> as dumping full FW information including build and log-providers hashes.

...

>>   #include "registers.h"
> 
>>   struct catpt_dev;
>> +extern const struct attribute_group *catpt_attr_groups[];
> 
> Grouping these are not okay — they are different by meaning, just put blank
> line in between.
> 

Sure, ack.

...

>> +#include <linux/pm_runtime.h>
>> +#include "core.h"
>> +
>> +static ssize_t fw_version_show(struct device *dev,
>> +			       struct device_attribute *attr, char *buf)
>> +{
>> +	struct catpt_dev *cdev = dev_get_drvdata(dev);
>> +	struct catpt_fw_version version;
>> +	int ret;
> 
>> +	pm_runtime_get_sync(cdev->dev);
>> +
>> +	ret = catpt_ipc_get_fw_version(cdev, &version);
>> +
>> +	pm_runtime_mark_last_busy(cdev->dev);
>> +	pm_runtime_put_autosuspend(cdev->dev);
> 
> Is it subject to change at run-time?
> 

No it does not. However, I do not intent to have the fw_version occupy
memory for device's drvdata (i.e. send the IPC internally and store it
inside struct catpt_dev). So, I'd rather wake the device, dump the
version and leave the bytes alone.

One could think about statics but then again, how many times this sysfs
file is going to get read anyway?
It's more readable and simple this way, losing nothing in return TBH.

>> +	if (ret)
>> +		return CATPT_IPC_ERROR(ret);
>> +
>> +	return sprintf(buf, "%d.%d.%d.%d\n", version.type, version.major,
>> +		       version.minor, version.build);
>> +}
> 
>> +
> 
> This blank line is not needed.
> 

Ack.

>> +static DEVICE_ATTR_RO(fw_version);
>> +
>> +static ssize_t fw_info_show(struct device *dev,
>> +			    struct device_attribute *attr, char *buf)
>> +{
>> +	struct catpt_dev *cdev = dev_get_drvdata(dev);
>> +
>> +	return sprintf(buf, "%s\n", cdev->ipc.config.fw_info);
>> +}
> 
>> +
> 
> This blank line is not needed.
> 

Ack.
Andy Shevchenko Sept. 24, 2020, 2:24 p.m. UTC | #4
On Wed, Sep 23, 2020 at 06:31:08PM +0000, Rojewski, Cezary wrote:
> On 2020-09-23 3:34 PM, Andy Shevchenko wrote:
> > On Wed, Sep 23, 2020 at 02:25:03PM +0200, Cezary Rojewski wrote:

...

> >> +	pm_runtime_get_sync(cdev->dev);
> >> +
> >> +	ret = catpt_ipc_get_fw_version(cdev, &version);
> >> +
> >> +	pm_runtime_mark_last_busy(cdev->dev);
> >> +	pm_runtime_put_autosuspend(cdev->dev);
> > 
> > Is it subject to change at run-time?
> 
> No it does not. However, I do not intent to have the fw_version occupy
> memory for device's drvdata (i.e. send the IPC internally and store it
> inside struct catpt_dev). So, I'd rather wake the device, dump the
> version and leave the bytes alone.
> 
> One could think about statics but then again, how many times this sysfs
> file is going to get read anyway?
> It's more readable and simple this way, losing nothing in return TBH.

For regular user perhaps few times or from time to time, but for dump syzkaller
type of fuzzers it may be thousands per second...

Greg is happy anyway, so choose yourself the best one you think of.
diff mbox series

Patch

diff --git a/Documentation/ABI/testing/sysfs-bus-pci-devices-catpt b/Documentation/ABI/testing/sysfs-bus-pci-devices-catpt
new file mode 100644
index 000000000000..8a200f4eefbd
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-bus-pci-devices-catpt
@@ -0,0 +1,16 @@ 
+What:		/sys/devices/pci0000:00/<dev>/fw_version
+Date:		September 2020
+Contact:	Cezary Rojewski <cezary.rojewski@intel.com>
+Description:
+		Version of AudioDSP firmware ASoC catpt driver is
+		communicating with.
+		Format: %d.%d.%d.%d, type:major:minor:build.
+
+What:		/sys/devices/pci0000:00/<dev>/fw_info
+Date:		September 2020
+Contact:	Cezary Rojewski <cezary.rojewski@intel.com>
+Description:
+		Detailed AudioDSP firmware build information including
+		build hash and log-providers hash. This information is
+		obtained during initial handshake with firmware.
+		Format: %s.
diff --git a/sound/soc/intel/catpt/core.h b/sound/soc/intel/catpt/core.h
index a29b4c0232cb..19eda125ee59 100644
--- a/sound/soc/intel/catpt/core.h
+++ b/sound/soc/intel/catpt/core.h
@@ -14,6 +14,7 @@ 
 #include "registers.h"
 
 struct catpt_dev;
+extern const struct attribute_group *catpt_attr_groups[];
 
 void catpt_sram_init(struct resource *sram, u32 start, u32 size);
 void catpt_sram_free(struct resource *sram);
diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c
index c02d46e5bc81..2d53efa656b1 100644
--- a/sound/soc/intel/catpt/device.c
+++ b/sound/soc/intel/catpt/device.c
@@ -338,6 +338,7 @@  static struct platform_driver catpt_acpi_driver = {
 		.name = "intel_catpt",
 		.acpi_match_table = catpt_ids,
 		.pm = &catpt_dev_pm,
+		.dev_groups = catpt_attr_groups,
 	},
 };
 module_platform_driver(catpt_acpi_driver);
diff --git a/sound/soc/intel/catpt/sysfs.c b/sound/soc/intel/catpt/sysfs.c
new file mode 100644
index 000000000000..f38562071a04
--- /dev/null
+++ b/sound/soc/intel/catpt/sysfs.c
@@ -0,0 +1,57 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+//
+// Copyright(c) 2020 Intel Corporation. All rights reserved.
+//
+// Author: Cezary Rojewski <cezary.rojewski@intel.com>
+//
+
+#include <linux/pm_runtime.h>
+#include "core.h"
+
+static ssize_t fw_version_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct catpt_dev *cdev = dev_get_drvdata(dev);
+	struct catpt_fw_version version;
+	int ret;
+
+	pm_runtime_get_sync(cdev->dev);
+
+	ret = catpt_ipc_get_fw_version(cdev, &version);
+
+	pm_runtime_mark_last_busy(cdev->dev);
+	pm_runtime_put_autosuspend(cdev->dev);
+
+	if (ret)
+		return CATPT_IPC_ERROR(ret);
+
+	return sprintf(buf, "%d.%d.%d.%d\n", version.type, version.major,
+		       version.minor, version.build);
+}
+
+static DEVICE_ATTR_RO(fw_version);
+
+static ssize_t fw_info_show(struct device *dev,
+			    struct device_attribute *attr, char *buf)
+{
+	struct catpt_dev *cdev = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%s\n", cdev->ipc.config.fw_info);
+}
+
+static DEVICE_ATTR_RO(fw_info);
+
+static struct attribute *catpt_attrs[] = {
+	&dev_attr_fw_version.attr,
+	&dev_attr_fw_info.attr,
+	NULL
+};
+
+static const struct attribute_group catpt_attr_group = {
+	.attrs = catpt_attrs,
+};
+
+const struct attribute_group *catpt_attr_groups[] = {
+	&catpt_attr_group,
+	NULL
+};