diff mbox series

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

Message ID 20200917141242.9081-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. 17, 2020, 2:12 p.m. UTC
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

Comments

Cezary Rojewski Sept. 18, 2020, 3:22 p.m. UTC | #1
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);
> +}
>
Greg KH Sept. 19, 2020, 2:42 p.m. UTC | #2
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
Cezary Rojewski Sept. 20, 2020, 5:03 p.m. UTC | #3
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
Andy Shevchenko Sept. 21, 2020, 8:09 a.m. UTC | #4
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.
Greg KH Sept. 21, 2020, 8:35 a.m. UTC | #5
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.
> >>>
> >>> 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?

Why are you putting random driver-specific attributes on a device owned
by a different bus?  That can cause problems if you are not careful.

Does the SoC core not provide you with a sound device to do this for
instead?

> 
> >>>    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.

The bus should provide you with the ability to do this, so look into
that for your driver.

But why are you creating a binary sysfs file?  That's only for passing
data raw through the kernel to/from the hardware, with no
parsing/massaging possible.  Is that what is happening here?

I guess if there was documentation, it would be easier to review :)

thanks,

greg k-h
Cezary Rojewski Sept. 21, 2020, 9:13 p.m. UTC | #6
On 2020-09-21 10:35 AM, gregkh@linuxfoundation.org wrote:
> 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:
...

>>
>> 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?
> 
> Why are you putting random driver-specific attributes on a device owned
> by a different bus?  That can cause problems if you are not careful.
> 
> Does the SoC core not provide you with a sound device to do this for
> instead?
> 

I know not about any device that ASoC core provides for me for such
tasks.

And the confusion about ADSP device location stems from incomplete
description coming from ACPI tables. I've had a quite lengthy discussion
about this with Andy.
Re: [PATCH v4 02/13] ASoC: Intel: catpt: Define DSP operations
https://www.spinics.net/lists/alsa-devel/msg114651.html

>>
...

>>>
>>> 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.
> 
> The bus should provide you with the ability to do this, so look into
> that for your driver.
> 
> But why are you creating a binary sysfs file?  That's only for passing
> data raw through the kernel to/from the hardware, with no
> parsing/massaging possible.  Is that what is happening here?
> 
> I guess if there was documentation, it would be easier to review :)
> 
> thanks,
> 
> greg k-h
> 

In v7 I've removed the binary sysfs file, plus the organization has been
simplified - made use of struct device_driver's 'dev_groups' field to
automate creation/ removal process of sysfs files as requested.
Documentation has too been added so things should be clearer.

Thanks,
Czarek
diff mbox series

Patch

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