Date:   Mon, 29 Apr 2019 21:07:55 -0400
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Jeff Kirsher <jeffrey.t.kirsher@...el.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, 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.

> @@ -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

Powered by blists - more mailing lists