| Message ID | 20200917141242.9081-10-cezary.rojewski@intel.com |
|---|---|
| State | New |
| Headers | show |
| Series | ASoC: Intel: Catpt - Lynx and Wildcat point | expand |
On 2020-09-17 4:12 PM, Cezary Rojewski wrote: > Add sysfs entries for displaying version of FW currently in use as well > as binary dump of entire version info, including build and log providers > hashes. > > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > --- > > 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 > +Greg KH Greg, would you mind taking a look at these sysfs entries added for new catpt driver (Audio DSP driver for Haswell and Broadwell machines)? Link to opening post for the series: [PATCH v6 00/14] ASoC: Intel: Catpt - Lynx and Wildcat point https://www.spinics.net/lists/alsa-devel/msg115765.html Let me give you a quick introduction to the catpt's fs code: During power-up sequence a handshake is made between host (kernel device driver) and DSP (firmware) side. Two sysfs entries are generated which expose running DSP firmware version and its build info - information obtained during said handshake. Much like devices (such as those of PCI-type) expose sysfs entries for their easy identification, catpt provides entries to identify DSP FW it is dealing with. Thanks, Czarek > sound/soc/intel/catpt/core.h | 3 ++ > sound/soc/intel/catpt/device.c | 6 +++ > sound/soc/intel/catpt/fs.c | 79 ++++++++++++++++++++++++++++++++++ > 3 files changed, 88 insertions(+) > create mode 100644 sound/soc/intel/catpt/fs.c > > diff --git a/sound/soc/intel/catpt/core.h b/sound/soc/intel/catpt/core.h > index a29b4c0232cb..1f0f1ac92341 100644 > --- a/sound/soc/intel/catpt/core.h > +++ b/sound/soc/intel/catpt/core.h > @@ -155,6 +155,9 @@ int catpt_store_module_states(struct catpt_dev *cdev, struct dma_chan *chan); > int catpt_store_memdumps(struct catpt_dev *cdev, struct dma_chan *chan); > int catpt_coredump(struct catpt_dev *cdev); > > +int catpt_sysfs_create(struct catpt_dev *cdev); > +void catpt_sysfs_remove(struct catpt_dev *cdev); > + > #include <sound/memalloc.h> > #include <uapi/sound/asound.h> > > diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c > index 7c7ddbabaf55..e9b7c1f474e0 100644 > --- a/sound/soc/intel/catpt/device.c > +++ b/sound/soc/intel/catpt/device.c > @@ -184,6 +184,10 @@ static int catpt_probe_components(struct catpt_dev *cdev) > goto board_err; > } > > + ret = catpt_sysfs_create(cdev); > + if (ret) > + goto board_err; > + > /* reflect actual ADSP state in pm_runtime */ > pm_runtime_set_active(cdev->dev); > > @@ -292,6 +296,8 @@ static int catpt_acpi_remove(struct platform_device *pdev) > catpt_sram_free(&cdev->iram); > catpt_sram_free(&cdev->dram); > > + catpt_sysfs_remove(cdev); > + > return 0; > } > > diff --git a/sound/soc/intel/catpt/fs.c b/sound/soc/intel/catpt/fs.c > new file mode 100644 > index 000000000000..d73493687f4a > --- /dev/null > +++ b/sound/soc/intel/catpt/fs.c > @@ -0,0 +1,79 @@ > +// SPDX-License-Identifier: GPL-2.0-pcm > +// > +// 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_build_read(struct file *filp, struct kobject *kobj, > + struct bin_attribute *bin_attr, char *buf, > + loff_t off, size_t count) > +{ > + struct catpt_dev *cdev = dev_get_drvdata(kobj_to_dev(kobj)); > + 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); > + > + memcpy(buf, &version, sizeof(version)); > + return count; > +} > + > +static BIN_ATTR_RO(fw_build, sizeof(struct catpt_fw_version)); > + > +int catpt_sysfs_create(struct catpt_dev *cdev) > +{ > + int ret; > + > + ret = sysfs_create_file(&cdev->dev->kobj, &dev_attr_fw_version.attr); > + if (ret) > + return ret; > + > + ret = sysfs_create_bin_file(&cdev->dev->kobj, &bin_attr_fw_build); > + if (ret) { > + sysfs_remove_file(&cdev->dev->kobj, &dev_attr_fw_version.attr); > + return ret; > + } > + > + return 0; > +} > + > +void catpt_sysfs_remove(struct catpt_dev *cdev) > +{ > + sysfs_remove_bin_file(&cdev->dev->kobj, &bin_attr_fw_build); > + sysfs_remove_file(&cdev->dev->kobj, &dev_attr_fw_version.attr); > +} >
On Fri, Sep 18, 2020 at 03:22:13PM +0000, Rojewski, Cezary wrote: > On 2020-09-17 4:12 PM, Cezary Rojewski wrote: > > Add sysfs entries for displaying version of FW currently in use as well > > as binary dump of entire version info, including build and log providers > > hashes. > > > > Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> > > --- > > > > 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 > > > > +Greg KH > > Greg, would you mind taking a look at these sysfs entries added for new > catpt driver (Audio DSP driver for Haswell and Broadwell machines)? Why me? > Link to opening post for the series: > [PATCH v6 00/14] ASoC: Intel: Catpt - Lynx and Wildcat point > https://www.spinics.net/lists/alsa-devel/msg115765.html Does lore.kernel.org handle alsa-devel yet? > > Let me give you a quick introduction to the catpt's fs code: > During power-up sequence a handshake is made between host (kernel device > driver) and DSP (firmware) side. Two sysfs entries are generated which > expose running DSP firmware version and its build info - information > obtained during said handshake. > > Much like devices (such as those of PCI-type) expose sysfs entries for > their easy identification, catpt provides entries to identify DSP FW it > is dealing with. No Documentation/ABI/ entry for these new devices explaining what they do and are for? That would be a good first step, and has always been a requirement for sysfs files. Do that and resend the series and cc: me and ask for my review and I will be glad to give it. Oh, a few notes below: > > sound/soc/intel/catpt/core.h | 3 ++ > > sound/soc/intel/catpt/device.c | 6 +++ > > sound/soc/intel/catpt/fs.c | 79 ++++++++++++++++++++++++++++++++++ > > 3 files changed, 88 insertions(+) > > create mode 100644 sound/soc/intel/catpt/fs.c > > > > diff --git a/sound/soc/intel/catpt/core.h b/sound/soc/intel/catpt/core.h > > index a29b4c0232cb..1f0f1ac92341 100644 > > --- a/sound/soc/intel/catpt/core.h > > +++ b/sound/soc/intel/catpt/core.h > > @@ -155,6 +155,9 @@ int catpt_store_module_states(struct catpt_dev *cdev, struct dma_chan *chan); > > int catpt_store_memdumps(struct catpt_dev *cdev, struct dma_chan *chan); > > int catpt_coredump(struct catpt_dev *cdev); > > > > +int catpt_sysfs_create(struct catpt_dev *cdev); > > +void catpt_sysfs_remove(struct catpt_dev *cdev); > > + > > #include <sound/memalloc.h> > > #include <uapi/sound/asound.h> > > > > diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c > > index 7c7ddbabaf55..e9b7c1f474e0 100644 > > --- a/sound/soc/intel/catpt/device.c > > +++ b/sound/soc/intel/catpt/device.c > > @@ -184,6 +184,10 @@ static int catpt_probe_components(struct catpt_dev *cdev) > > goto board_err; > > } > > > > + ret = catpt_sysfs_create(cdev); > > + if (ret) > > + goto board_err; Why are you calling a specific function to do all of this? Why not provide a default_groups pointer which allows the driver core to automatically create/destroy the sysfs files for you in a race-free manner with userspace? That's the recommended way, you should never have to manually create files. > > + > > /* reflect actual ADSP state in pm_runtime */ > > pm_runtime_set_active(cdev->dev); > > > > @@ -292,6 +296,8 @@ static int catpt_acpi_remove(struct platform_device *pdev) > > catpt_sram_free(&cdev->iram); > > catpt_sram_free(&cdev->dram); > > > > + catpt_sysfs_remove(cdev); > > + > > return 0; > > } > > > > diff --git a/sound/soc/intel/catpt/fs.c b/sound/soc/intel/catpt/fs.c > > new file mode 100644 > > index 000000000000..d73493687f4a > > --- /dev/null > > +++ b/sound/soc/intel/catpt/fs.c > > @@ -0,0 +1,79 @@ > > +// SPDX-License-Identifier: GPL-2.0-pcm What is "GPL-2.0-pcm" for a SPDX identifier? We don't have that listed in LICENSES, do we? Did this pass the spdxcheck tool in scripts? thanks, greg k-h
On 2020-09-19 4:42 PM, gregkh@linuxfoundation.org wrote: > On Fri, Sep 18, 2020 at 03:22:13PM +0000, Rojewski, Cezary wrote: >> On 2020-09-17 4:12 PM, Cezary Rojewski wrote: >>> Add sysfs entries for displaying version of FW currently in use as well >>> as binary dump of entire version info, including build and log providers >>> hashes. >>> >>> Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> >>> --- >>> >>> 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 >>> >> >> +Greg KH >> >> Greg, would you mind taking a look at these sysfs entries added for new >> catpt driver (Audio DSP driver for Haswell and Broadwell machines)? > > Why me? > Andy (CC'ed) suggested that it's best if sysfs code is routed through you. Given your input, I believe he was right. >> Link to opening post for the series: >> [PATCH v6 00/14] ASoC: Intel: Catpt - Lynx and Wildcat point >> https://www.spinics.net/lists/alsa-devel/msg115765.html > > Does lore.kernel.org handle alsa-devel yet? > Believe it does as alsa-devel archive can be found on lore.kernel.org. Not really the guy to answer integration questions, though. >> >> Let me give you a quick introduction to the catpt's fs code: >> During power-up sequence a handshake is made between host (kernel device >> driver) and DSP (firmware) side. Two sysfs entries are generated which >> expose running DSP firmware version and its build info - information >> obtained during said handshake. >> >> Much like devices (such as those of PCI-type) expose sysfs entries for >> their easy identification, catpt provides entries to identify DSP FW it >> is dealing with. > > No Documentation/ABI/ entry for these new devices explaining what they > do and are for? That would be a good first step, and has always been a > requirement for sysfs files. Do that and resend the series and cc: me > and ask for my review and I will be glad to give it. > > Oh, a few notes below: > Well, that's just one device driver targeting basically single device available in two flavors. Lack of Documentation/ABI/<sysfs-doc> for solution has been noted though. Will add in v7. As this device is available on /sys/bus/pci0000:00/<dev> is the name for upcoming file: sysfs-bus-pci-devices-catpt ok? Or, would you prevent a different, more explicit one? >>> sound/soc/intel/catpt/core.h | 3 ++ >>> sound/soc/intel/catpt/device.c | 6 +++ >>> sound/soc/intel/catpt/fs.c | 79 ++++++++++++++++++++++++++++++++++ >>> 3 files changed, 88 insertions(+) >>> create mode 100644 sound/soc/intel/catpt/fs.c >>> >>> diff --git a/sound/soc/intel/catpt/core.h b/sound/soc/intel/catpt/core.h >>> index a29b4c0232cb..1f0f1ac92341 100644 >>> --- a/sound/soc/intel/catpt/core.h >>> +++ b/sound/soc/intel/catpt/core.h >>> @@ -155,6 +155,9 @@ int catpt_store_module_states(struct catpt_dev *cdev, struct dma_chan *chan); >>> int catpt_store_memdumps(struct catpt_dev *cdev, struct dma_chan *chan); >>> int catpt_coredump(struct catpt_dev *cdev); >>> >>> +int catpt_sysfs_create(struct catpt_dev *cdev); >>> +void catpt_sysfs_remove(struct catpt_dev *cdev); >>> + >>> #include <sound/memalloc.h> >>> #include <uapi/sound/asound.h> >>> >>> diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c >>> index 7c7ddbabaf55..e9b7c1f474e0 100644 >>> --- a/sound/soc/intel/catpt/device.c >>> +++ b/sound/soc/intel/catpt/device.c >>> @@ -184,6 +184,10 @@ static int catpt_probe_components(struct catpt_dev *cdev) >>> goto board_err; >>> } >>> >>> + ret = catpt_sysfs_create(cdev); >>> + if (ret) >>> + goto board_err; > > Why are you calling a specific function to do all of this? Why not > provide a default_groups pointer which allows the driver core to > automatically create/destroy the sysfs files for you in a race-free > manner with userspace? > > That's the recommended way, you should never have to manually create > files. > > Thanks, that's something new. As this is simple device-driver, I believe you meant usage of sysfs_(add|remove)_group() or their "device" equivalents: device_(add|remove)_group(), is that correct? Haven't found any usage of default_group within /sound/ subsystem what cannot be said about the functions I've just mentioned. Feel free to correct me if I'm wrong about this. >>> + >>> /* reflect actual ADSP state in pm_runtime */ >>> pm_runtime_set_active(cdev->dev); >>> >>> @@ -292,6 +296,8 @@ static int catpt_acpi_remove(struct platform_device *pdev) >>> catpt_sram_free(&cdev->iram); >>> catpt_sram_free(&cdev->dram); >>> >>> + catpt_sysfs_remove(cdev); >>> + >>> return 0; >>> } >>> >>> diff --git a/sound/soc/intel/catpt/fs.c b/sound/soc/intel/catpt/fs.c >>> new file mode 100644 >>> index 000000000000..d73493687f4a >>> --- /dev/null >>> +++ b/sound/soc/intel/catpt/fs.c >>> @@ -0,0 +1,79 @@ >>> +// SPDX-License-Identifier: GPL-2.0-pcm > > What is "GPL-2.0-pcm" for a SPDX identifier? We don't have that listed > in LICENSES, do we? > > Did this pass the spdxcheck tool in scripts? > > thanks, > > greg k-h > Well, after s/pcm/only/ is does : ) Mark already mentioned this mistake, my bad for letting it into v6.. Thanks for your input, much appreciated. Czarek
On Sun, Sep 20, 2020 at 05:03:00PM +0000, Rojewski, Cezary wrote: > On 2020-09-19 4:42 PM, gregkh@linuxfoundation.org wrote: > > On Fri, Sep 18, 2020 at 03:22:13PM +0000, Rojewski, Cezary wrote: > >> On 2020-09-17 4:12 PM, Cezary Rojewski wrote: > >>> Add sysfs entries for displaying version of FW currently in use as well > >>> as binary dump of entire version info, including build and log providers > >>> hashes. ... > >> Greg, would you mind taking a look at these sysfs entries added for new > >> catpt driver (Audio DSP driver for Haswell and Broadwell machines)? > > > > Why me? > > > > Andy (CC'ed) suggested that it's best if sysfs code is routed through you. > Given your input, I believe he was right. 'routed' probably is not what I meant, rather 'reviewed'. Because I remember that sysfs got new support for attributes thru certain members of the driver structure. Also I'm not sure I know the policy about binary attributes (it's possible that my knowledge is interfering with sysctl binary attributes that are no-no nowadays). Anyway, thanks for looking into this! > >> Link to opening post for the series: > >> [PATCH v6 00/14] ASoC: Intel: Catpt - Lynx and Wildcat point > >> https://www.spinics.net/lists/alsa-devel/msg115765.html > > > > Does lore.kernel.org handle alsa-devel yet? > > > > Believe it does as alsa-devel archive can be found on lore.kernel.org. > Not really the guy to answer integration questions, though. ... > >>> + ret = catpt_sysfs_create(cdev); > >>> + if (ret) > >>> + goto board_err; > > > > Why are you calling a specific function to do all of this? Why not > > provide a default_groups pointer which allows the driver core to > > automatically create/destroy the sysfs files for you in a race-free > > manner with userspace? > > > > That's the recommended way, you should never have to manually create > > files. > > Thanks, that's something new. As this is simple device-driver, I believe > you meant usage of sysfs_(add|remove)_group() or their "device" > equivalents: device_(add|remove)_group(), is that correct? Haven't found > any usage of default_group within /sound/ subsystem what cannot be said > about the functions I've just mentioned. > > Feel free to correct me if I'm wrong about this. It's a member of the struct device_driver, if I'm not mistaken.
diff --git a/sound/soc/intel/catpt/core.h b/sound/soc/intel/catpt/core.h index a29b4c0232cb..1f0f1ac92341 100644 --- a/sound/soc/intel/catpt/core.h +++ b/sound/soc/intel/catpt/core.h @@ -155,6 +155,9 @@ int catpt_store_module_states(struct catpt_dev *cdev, struct dma_chan *chan); int catpt_store_memdumps(struct catpt_dev *cdev, struct dma_chan *chan); int catpt_coredump(struct catpt_dev *cdev); +int catpt_sysfs_create(struct catpt_dev *cdev); +void catpt_sysfs_remove(struct catpt_dev *cdev); + #include <sound/memalloc.h> #include <uapi/sound/asound.h> diff --git a/sound/soc/intel/catpt/device.c b/sound/soc/intel/catpt/device.c index 7c7ddbabaf55..e9b7c1f474e0 100644 --- a/sound/soc/intel/catpt/device.c +++ b/sound/soc/intel/catpt/device.c @@ -184,6 +184,10 @@ static int catpt_probe_components(struct catpt_dev *cdev) goto board_err; } + ret = catpt_sysfs_create(cdev); + if (ret) + goto board_err; + /* reflect actual ADSP state in pm_runtime */ pm_runtime_set_active(cdev->dev); @@ -292,6 +296,8 @@ static int catpt_acpi_remove(struct platform_device *pdev) catpt_sram_free(&cdev->iram); catpt_sram_free(&cdev->dram); + catpt_sysfs_remove(cdev); + return 0; } diff --git a/sound/soc/intel/catpt/fs.c b/sound/soc/intel/catpt/fs.c new file mode 100644 index 000000000000..d73493687f4a --- /dev/null +++ b/sound/soc/intel/catpt/fs.c @@ -0,0 +1,79 @@ +// SPDX-License-Identifier: GPL-2.0-pcm +// +// 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_build_read(struct file *filp, struct kobject *kobj, + struct bin_attribute *bin_attr, char *buf, + loff_t off, size_t count) +{ + struct catpt_dev *cdev = dev_get_drvdata(kobj_to_dev(kobj)); + 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); + + memcpy(buf, &version, sizeof(version)); + return count; +} + +static BIN_ATTR_RO(fw_build, sizeof(struct catpt_fw_version)); + +int catpt_sysfs_create(struct catpt_dev *cdev) +{ + int ret; + + ret = sysfs_create_file(&cdev->dev->kobj, &dev_attr_fw_version.attr); + if (ret) + return ret; + + ret = sysfs_create_bin_file(&cdev->dev->kobj, &bin_attr_fw_build); + if (ret) { + sysfs_remove_file(&cdev->dev->kobj, &dev_attr_fw_version.attr); + return ret; + } + + return 0; +} + +void catpt_sysfs_remove(struct catpt_dev *cdev) +{ + sysfs_remove_bin_file(&cdev->dev->kobj, &bin_attr_fw_build); + sysfs_remove_file(&cdev->dev->kobj, &dev_attr_fw_version.attr); +}
Add sysfs entries for displaying version of FW currently in use as well as binary dump of entire version info, including build and log providers hashes. Signed-off-by: Cezary Rojewski <cezary.rojewski@intel.com> --- 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 sound/soc/intel/catpt/core.h | 3 ++ sound/soc/intel/catpt/device.c | 6 +++ sound/soc/intel/catpt/fs.c | 79 ++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+) create mode 100644 sound/soc/intel/catpt/fs.c