Message-ID: <20201113131252.743c1226@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Date:   Fri, 13 Nov 2020 13:12:52 -0800
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jacob Keller <jacob.e.keller@...el.com>
Cc:     netdev@...r.kernel.org, Jiri Pirko <jiri@...dia.com>,
        Michael Chan <michael.chan@...adcom.com>,
        Shannon Nelson <snelson@...sando.io>,
        Saeed Mahameed <saeedm@...dia.com>,
        Boris Pismenny <borisp@...dia.com>,
        Bin Luo <luobin9@...wei.com>
Subject: Re: [net-next] devlink: move request_firmware out of driver

On Thu, 12 Nov 2020 16:01:42 -0800 Jacob Keller wrote:
> All drivers which implement the devlink flash update support, with the
> exception of netdevsim, use either request_firmware or
> request_firmware_direct to locate the firmware file. Rather than having
> each driver do this separately as part of its .flash_update
> implementation, perform the request_firmware within net/core/devlink.c
> 
> Replace the file_name paramter in the struct devlink_flash_update_params
> with a pointer to the fw object.
> 
> Use request_firmware rather than request_firmware_direct. Although most
> Linux distributions today do not have the fallback mechanism
> implemented, only about half the drivers used the _direct request, as
> compared to the generic request_firmware. In the event that
> a distribution does support the fallback mechanism, the devlink flash
> update ought to be able to use it to provide the firmware contents. For
> distributions which do not support the fallback userspace mechanism,
> there should be essentially no difference between request_firmware and
> request_firmware_direct.

Thanks for working on this!

> This is a follow to the discussion that took place at [1]. After reading
> through the docs for request_firmware vs request_firmware_direct, I believe
> that the net/core/devlink.c should be using request_firmware. While it is
> true that no distribution supports this today, it seems like we shouldn't
> rule it out entirely here. I'm willing to change this if we think it's not
> worth bothering to support it.
> 
> Note that I have only compile-tested the drivers other than ice, as I do not
> have hw for them. The only tricky transformation was in the bnxt driver
> which shares code with the ethtool implementation. The rest were pretty
> straight forward transformations.
> 
> One other thing came to my attention while working on this and while
> discussing the ice devlink support with colleagues: the userspace devlink
> program doesn't really indicate that the flash file must be located in the
> firmware search path (usually /lib/firmware/*).

It's in the man page, same as ethool.

> It is probably worth some effort to make the userspace tool error out
> more clearly when the file can't be found.

Can do, although the path is configurable AFAIR through some kconfig,
and an extack from the kernel would probably be as informative?

Or are you thinking of doing something like copying/linking the file to
/lib/firmware automatically if user provides path relative to CWD?


../drivers/net/ethernet/intel/ice/ice_devlink.c:250:17: warning: unused variable ‘dev’ [-Wunused-variable]
  250 |  struct device *dev = &pf->pdev->dev;
      |                 ^~~
../drivers/net/ethernet/qlogic/qed/qed_mcp.c:510:9: warning: context imbalance in '_qed_mcp_cmd_and_union' - unexpected unlock
../drivers/net/ethernet/mellanox/mlx5/core/devlink.c: In function ‘mlx5_devlink_flash_update’:
../drivers/net/ethernet/mellanox/mlx5/core/devlink.c:16:25: warning: unused variable ‘fw’ [-Wunused-variable]
   16 |  const struct firmware *fw;
      |                         ^~

Powered by blists - more mailing lists