Message-ID: <74f5b137ac9af4bbceb7ba08d2de94e972e0fda4.camel@intel.com>
Date:   Thu, 02 May 2019 02:20:24 -0700
From:   Jeff Kirsher <jeffrey.t.kirsher@...el.com>
To:     Jakub Kicinski <jakub.kicinski@...ronome.com>
Cc:     davem@...emloft.net, Alice Michael <alice.michael@...el.com>,
        netdev@...r.kernel.org, nhorman@...hat.com, sassmann@...hat.com,
        Piotr Marczak <piotr.marczak@...el.com>,
        Don Buchholz <donald.buchholz@...el.com>
Subject: Re: [net-next 12/12] i40e: Introduce recovery mode support

On Mon, 2019-04-29 at 21:07 -0400, Jakub Kicinski wrote:
> On Mon, 29 Apr 2019 12:16:28 -0700, Jeff Kirsher wrote:
> > From: Alice Michael <alice.michael@...el.com>
> > 
> > This patch introduces "recovery mode" to the i40e driver. It is
> > part of a new Any2Any idea of upgrading the firmware. In this
> > approach, it is required for the driver to have support for
> > "transition firmware", that is used for migrating from structured
> > to flat firmware image. In this new, very basic mode, i40e driver
> > must be able to handle particular IOCTL calls from the NVM Update
> > Tool and run a small set of AQ commands.
> 
> Could you show us commands that get executed?  I think that'd be much
> quicker for people to parse.
> 
> > Signed-off-by: Alice Michael <alice.michael@...el.com>
> > Signed-off-by: Piotr Marczak <piotr.marczak@...el.com>
> > Tested-by: Don Buchholz <donald.buchholz@...el.com>
> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@...el.com>
> 
> From a cursory look it seems you create a "basic" netdev.  Can this
> netdev pass traffic?
> 
> I'd suggest you implement devlink "limp mode".  Devlink can flash the
> device now.  You can register a devlink instance without registering
> any "minimal" netdevs, and flash with devlink.

Good to know, currently this driver and our other LAN drivers do not have
devlink support, but we are currently working to rectify that.  I am hoping
that we can get devlink support in i40e and other drivers in the 5.3 or 5.4
kernel.  

Alice has updated this patch to add the requested additional information to
the patch description and cleaned up the code not intended for the Linux
kernel driver.  So I will resubmit this series with the updates, once it
goes through our validation checks.

> 
> > @@ -13904,6 +14007,134 @@ void i40e_set_fec_in_flags(u8 fec_cfg, u32
> > *flags)
> >  		*flags &= ~(I40E_FLAG_RS_FEC | I40E_FLAG_BASE_R_FEC);
> >  }
> >  
> > +/**
> > + * i40e_check_recovery_mode - check if we are running transition
> > firmware
> > + * @pf: board private structure
> > + *
> > + * Check registers indicating the firmware runs in recovery mode. Sets
> > the
> > + * appropriate driver state.
> > + *
> > + * Returns true if the recovery mode was detected, false otherwise
> > + **/
> > +static bool i40e_check_recovery_mode(struct i40e_pf *pf)
> > +{
> > +	u32 val = rd32(&pf->hw, I40E_GL_FWSTS);
> > +
> > +	if (val & I40E_GL_FWSTS_FWS1B_MASK) {
> > +		dev_notice(&pf->pdev->dev, "Firmware recovery mode
> > detected. Limiting functionality.\n");
> > +		dev_notice(&pf->pdev->dev, "Refer to the Intel(R) Ethernet
> > Adapters and Devices User Guide for details on firmware recovery
> > mode.\n");
> > +		set_bit(__I40E_RECOVERY_MODE, pf->state);
> > +
> > +		return true;
> > +	}
> > +	if (test_and_clear_bit(__I40E_RECOVERY_MODE, pf->state))
> > +		dev_info(&pf->pdev->dev, "Reinitializing in normal mode
> > with full functionality.\n");
> > +
> > +	return false;
> > +}
> > +
> > +/**
> > + * i40e_init_recovery_mode - initialize subsystems needed in recovery
> > mode
> > + * @pf: board private structure
> > + * @hw: ptr to the hardware info
> > + *
> > + * This function does a minimal setup of all subsystems needed for
> > running
> > + * recovery mode.
> > + *
> > + * Returns 0 on success, negative on failure
> > + **/
> > +static int i40e_init_recovery_mode(struct i40e_pf *pf, struct i40e_hw
> > *hw)
> > +{
> > +	struct i40e_vsi *vsi;
> > +	int err;
> > +	int v_idx;
> > +
> > +#ifdef HAVE_PCI_ERS
> > +	pci_save_state(pf->pdev);
> > +#endif
> > +
> > +	/* set up periodic task facility */
> > +	timer_setup(&pf->service_timer, i40e_service_timer, 0);
> > +	pf->service_timer_period = HZ;
> > +
> > +	INIT_WORK(&pf->service_task, i40e_service_task);
> > +	clear_bit(__I40E_SERVICE_SCHED, pf->state);
> > +
> > +	err = i40e_init_interrupt_scheme(pf);
> > +	if (err)
> > +		goto err_switch_setup;
> > +
> > +	/* The number of VSIs reported by the FW is the minimum guaranteed
> > +	 * to us; HW supports far more and we share the remaining pool with
> > +	 * the other PFs. We allocate space for more than the guarantee
> > with
> > +	 * the understanding that we might not get them all later.
> > +	 */
> > +	if (pf->hw.func_caps.num_vsis < I40E_MIN_VSI_ALLOC)
> > +		pf->num_alloc_vsi = I40E_MIN_VSI_ALLOC;
> > +	else
> > +		pf->num_alloc_vsi = pf->hw.func_caps.num_vsis;
> > +
> > +	/* Set up the vsi struct and our local tracking of the MAIN PF vsi.
> > */
> > +	pf->vsi = kcalloc(pf->num_alloc_vsi, sizeof(struct i40e_vsi *),
> > +			  GFP_KERNEL);
> > +	if (!pf->vsi) {
> > +		err = -ENOMEM;
> > +		goto err_switch_setup;
> > +	}
> > +
> > +	/* We allocate one VSI which is needed as absolute minimum
> > +	 * in order to register the netdev
> > +	 */
> > +	v_idx = i40e_vsi_mem_alloc(pf, I40E_VSI_MAIN);
> > +	if (v_idx < 0)
> > +		goto err_switch_setup;
> > +	pf->lan_vsi = v_idx;
> > +	vsi = pf->vsi[v_idx];
> > +	if (!vsi)
> > +		goto err_switch_setup;
> > +	vsi->alloc_queue_pairs = 1;
> > +	err = i40e_config_netdev(vsi);
> > +	if (err)
> > +		goto err_switch_setup;
> > +	err = register_netdev(vsi->netdev);
> > +	if (err)
> > +		goto err_switch_setup;
> > +	vsi->netdev_registered = true;
> > +	i40e_dbg_pf_init(pf);
> > +
> > +	err = i40e_setup_misc_vector_for_recovery_mode(pf);
> > +	if (err)
> > +		goto err_switch_setup;
> > +
> > +	/* tell the firmware that we're starting */
> > +	i40e_send_version(pf);
> > +
> > +	/* since everything's happy, start the service_task timer */
> > +	mod_timer(&pf->service_timer,
> > +		  round_jiffies(jiffies + pf->service_timer_period));
> > +
> > +	return 0;
> > +
> > +err_switch_setup:
> > +	i40e_reset_interrupt_capability(pf);
> > +	del_timer_sync(&pf->service_timer);
> > +#ifdef NOT_FOR_UPSTREAM
> 
> Delightful :)
> 
> > +	dev_warn(&pf->pdev->dev, "previous errors forcing module to load in
> > debug mode\n");
> > +	i40e_dbg_pf_init(pf);
> > +	set_bit(__I40E_DEBUG_MODE, pf->state);
> > +	return 0;
> > +#else
> > +	i40e_shutdown_adminq(hw);
> > +	iounmap(hw->hw_addr);
> > +	pci_disable_pcie_error_reporting(pf->pdev);
> > +	pci_release_mem_regions(pf->pdev);
> > +	pci_disable_device(pf->pdev);
> > +	kfree(pf);
> > +
> > +	return err;
> > +#endif
> > +}
> > +
> >  /**
> >   * i40e_probe - Device initialization routine
> >   * @pdev: PCI device information struct


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)

Powered by blists - more mailing lists