Re: [patch net-next v3 3/3] devlink: implement flash status monitoring

From: Jakub Kicinski <jakub.kicinski@netronome.com>
To: David Ahern <dsahern@gmail.com>
Cc: Jiri Pirko <jiri@resnulli.us>,
	netdev@vger.kernel.org, davem@davemloft.net, mlxsw@mellanox.com,
	sthemmin@microsoft.com, saeedm@mellanox.com, leon@kernel.org,
	f.fainelli@gmail.com
Subject: Re: [patch net-next v3 3/3] devlink: implement flash status monitoring
Date: Mon, 10 Jun 2019 10:47:23 -0700	[thread overview]
Message-ID: <20190610104723.66e78254@cakuba.netronome.com> (raw)
In-Reply-To: <249eca9b-e62a-df02-7593-4492daf39183@gmail.com>

On Mon, 10 Jun 2019 11:30:24 -0600, David Ahern wrote:
> On 6/10/19 11:24 AM, Jakub Kicinski wrote:
> > On Mon, 10 Jun 2019 11:09:19 -0600, David Ahern wrote:  
> >> On 6/4/19 7:44 AM, Jiri Pirko wrote:  
> >>> diff --git a/man/man8/devlink-dev.8 b/man/man8/devlink-dev.8
> >>> index 1804463b2321..1021ee8d064c 100644
> >>> --- a/man/man8/devlink-dev.8
> >>> +++ b/man/man8/devlink-dev.8
> >>> @@ -244,6 +244,17 @@ Sets the parameter internal_error_reset of specified devlink device to true.
> >>>  devlink dev reload pci/0000:01:00.0
> >>>  .RS 4
> >>>  Performs hot reload of specified devlink device.
> >>> +.RE
> >>> +.PP
> >>> +devlink dev flash pci/0000:01:00.0 file firmware.bin
> >>> +.RS 4
> >>> +Flashes the specified devlink device with provided firmware file name. If the driver supports it, user gets updates about the flash status. For example:
> >>> +.br
> >>> +Preparing to flash
> >>> +.br
> >>> +Flashing 100%
> >>> +.br
> >>> +Flashing done
> >>>  
> >>>  .SH SEE ALSO
> >>>  .BR devlink (8),    
> >>
> >> something is missing here from a user perspective at least:
> >>
> >> root@mlx-2700-05:~# ./devlink dev
> >> pci/0000:03:00.0
> >>
> >> root@mlx-2700-05:~# ./devlink dev flash pci/0000:03:00.0 file
> >> /lib/firmware/mellanox/mlxsw_spectrum-13.2000.1122.mfa2
> >> devlink answers: No such file or directory
> >>
> >> root@mlx-2700-05:~# ls -l
> >> /lib/firmware/mellanox/mlxsw_spectrum-13.2000.1122.mfa2
> >> -rw-r--r-- 1 cumulus 1001 994184 May 14 22:44
> >> /lib/firmware/mellanox/mlxsw_spectrum-13.2000.1122.mfa2
> >>
> >>
> >> Why the 'no such file' response when the file exists?  
> > 
> > I think the FW loader prepends /lib/firmware to the path (there is a
> > CONFIG_ for the search paths, and / is usually not on it).  Perhaps try:
> > 
> > ./devlink dev flash pci/0000:03:00.0 file mellanox/mlxsw_spectrum-13.2000.1122.mfa2
> >   
> 
> that worked, but if the user specifies fullpath that is confusing. So at
> a minimum the documentation needs to be clear about the paths.
> 
> But, why the path limitation? why not allow a user to load a file from
> any directory? For mlxsw at least the file in /lib/firmware will be
> loaded automagically, so forcing the file to always be in /lib/firmware
> seems counterintuitive when using a command to specify the file to load.

It's the kernel that does this, the request_firmware() API.  It's
documented in both devlink's and ethtool's API.  I was initially
intending to use the file request API directly in devlink, but because
of the requirement to keep compatibility with ethtool that was a no go.

FWIW you can load from any directory, just prefix the file name
with ../../ to get out of /lib/firmware.

I guess we could add some logic into devlink user space to detect that
user does not know about this quirk and fix up the path for them.. 🤔