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

From: "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
To: "Rojewski, Cezary" <cezary.rojewski@intel.com>
Cc: "pierre-louis.bossart@linux.intel.com"
	<pierre-louis.bossart@linux.intel.com>,
	"alsa-devel@alsa-project.org" <alsa-devel@alsa-project.org>,
	"andriy.shevchenko@linux.intel.com"
	<andriy.shevchenko@linux.intel.com>,
	"Kaczmarski, Filip" <filip.kaczmarski@intel.com>,
	"N, Harshapriya" <harshapriya.n@intel.com>,
	"Barlik, Marcin" <marcin.barlik@intel.com>,
	"zwisler@google.com" <zwisler@google.com>,
	"lgirdwood@gmail.com" <lgirdwood@gmail.com>,
	"tiwai@suse.com" <tiwai@suse.com>,
	"Proborszcz, Filip" <filip.proborszcz@intel.com>,
	"broonie@kernel.org" <broonie@kernel.org>,
	"amadeuszx.slawinski@linux.intel.com"
	<amadeuszx.slawinski@linux.intel.com>,
	"Wasko, Michal" <michal.wasko@intel.com>,
	"cujomalainey@chromium.org" <cujomalainey@chromium.org>,
	"Hejmowski, Krzysztof" <krzysztof.hejmowski@intel.com>,
	"Papierkowski, Piotr \(Habana\)" <ppapierkowski@habana.ai>,
	"Gopal, Vamshi Krishna" <vamshi.krishna.gopal@intel.com>
Subject: Re: [PATCH v6 09/14] ASoC: Intel: catpt: Simple sysfs attributes
Date: Mon, 21 Sep 2020 10:35:14 +0200	[thread overview]
Message-ID: <20200921083514.GA3151537@kroah.com> (raw)
In-Reply-To: <dfdb29d1c5f04d88970757e1ec9596d9@intel.com>

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