Re: [PATCH net-next 3/3] nfp: devlink: allow flashing the device via devlink

[Michal Kubecek <mkubecek@suse.cz>]

On Thu, Feb 14, 2019 at 01:40:46PM -0800, Jakub Kicinski wrote:
> Devlink now allows updating device flash.  Implement this
> callback.
> 
> Compared to ethtool update we no longer have to release
> the networking locks - devlink doesn't take them.
> 
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> ---
...
> diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> index cb9c512abc76..8f189149efc5 100644
> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_ethtool.c
> @@ -1237,11 +1237,8 @@ static int nfp_net_set_channels(struct net_device *netdev,
>  static int
>  nfp_net_flash_device(struct net_device *netdev, struct ethtool_flash *flash)
>  {
> -	const struct firmware *fw;
>  	struct nfp_app *app;
> -	struct nfp_nsp *nsp;
> -	struct device *dev;
> -	int err;
> +	int ret;
>  
>  	if (flash->region != ETHTOOL_FLASH_ALL_REGIONS)
>  		return -EOPNOTSUPP;
> @@ -1250,39 +1247,13 @@ nfp_net_flash_device(struct net_device *netdev, struct ethtool_flash *flash)
>  	if (!app)
>  		return -EOPNOTSUPP;
>  
> -	dev = &app->pdev->dev;
> -
> -	nsp = nfp_nsp_open(app->cpp);
> -	if (IS_ERR(nsp)) {
> -		err = PTR_ERR(nsp);
> -		dev_err(dev, "Failed to access the NSP: %d\n", err);
> -		return err;
> -	}
> -
> -	err = request_firmware_direct(&fw, flash->data, dev);
> -	if (err)
> -		goto exit_close_nsp;
> -
> -	dev_info(dev, "Please be patient while writing flash image: %s\n",
> -		 flash->data);
>  	dev_hold(netdev);
>  	rtnl_unlock();
> -
> -	err = nfp_nsp_write_flash(nsp, fw);
> -	if (err < 0) {
> -		dev_err(dev, "Flash write failed: %d\n", err);
> -		goto exit_rtnl_lock;
> -	}
> -	dev_info(dev, "Finished writing flash image\n");
> -
> -exit_rtnl_lock:
> +	ret = nfp_flash_update_common(app->pf, flash->data, NULL);
>  	rtnl_lock();
>  	dev_put(netdev);
> -	release_firmware(fw);
>  
> -exit_close_nsp:
> -	nfp_nsp_close(nsp);
> -	return err;
> +	return ret;
>  }
>  
>  static const struct ethtool_ops nfp_net_ethtool_ops = {

Out of curiosity: why don't you drop the ethtool_ops callback and let
the fallback introduced in patch 2/3 do the fallback? Is it to preserve
the check of flash->region or to avoid the lookup? My understanding of
the previous patch was that it would allow new drivers providing the
devlink callback to get rid of ethtool_ops one.

Michal Kubecek