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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190610104723.66e78254@cakuba.netronome.com>
Date:   Mon, 10 Jun 2019 10:47:23 -0700
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     David Ahern <dsahern@...il.com>
Cc:     Jiri Pirko <jiri@...nulli.us>, netdev@...r.kernel.org,
        davem@...emloft.net, mlxsw@...lanox.com, sthemmin@...rosoft.com,
        saeedm@...lanox.com, leon@...nel.org, f.fainelli@...il.com
Subject: Re: [patch net-next v3 3/3] devlink: implement flash status
 monitoring

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@...-2700-05:~# ./devlink dev
> >> pci/0000:03:00.0
> >>
> >> root@...-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@...-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.. 🤔