| 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 |
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 >
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>
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.
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 --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 +};
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