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

["Rojewski, Cezary" <cezary.rojewski@intel.com>]

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