Re: [PATCH net v3 2/3] bnxt_en: Fix driver init in kdump kernel

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] bnxt_kdump_reset() (drivers/net/ethernet/broadcom/bnxt/bnxt.c)
  checks that config space is reachable only after the FLR, never before…

--

bnxt_en: Fix driver init in kdump kernel

This commit adds bnxt_kdump_reset() and calls it from bnxt_init_one() in
the kdump kernel. It disables bus master and memory decode, saves PCI
state, issues an FLR, polls config space for up to 5 more seconds, clears
the BARs and restores the saved PCI state, so a PCIe switch in synthetic
mode sees the BARs reprogrammed.

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 8ce8a82d34530..9ea7e172787e2 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -17119,6 +17119,43 @@ static void bnxt_clear_bars(struct pci_dev *pdev)
>  		pci_write_config_dword(pdev, off, 0);
>  }
>  
> +/* Clear any pending DMA transactions from crash kernel while loading driver in
> + * capture kernel.
> + */
> +static int bnxt_kdump_reset(struct pci_dev *pdev)
> +{
> +	int rc, i;
> +	u16 cmd;
> +
> +	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +	cmd &= ~(PCI_COMMAND_MASTER | PCI_COMMAND_MEMORY);
> +	pci_write_config_word(pdev, PCI_COMMAND, cmd);
> +
> +	if (pci_save_state(pdev))
> +		dev_warn(&pdev->dev, "Failed to save PCI state, PCI restore may be incomplete\n");
> +
> +	rc = pcie_flr(pdev);
> +	if (rc)
> +		dev_warn(&pdev->dev, "pcie_flr() failed (rc: %d), trying to continue\n",
> +			 rc);
> +
> +	/* In case device is not returning CRS, wait 5 seconds longer */
> +	for (i = 0; i < 50; i++) {
> +		pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +		if (!PCI_POSSIBLE_ERROR(cmd))
> +			break;
> +		msleep(100);
> +	}
> +	if (PCI_POSSIBLE_ERROR(cmd)) {
> +		dev_err(&pdev->dev, "PCI config space inaccessible after FLR, aborting\n");
> +		return -ENODEV;
> +	}
> +
> +	bnxt_clear_bars(pdev);
> +	pci_restore_state(pdev);
> +	return 0;

[Severity: Medium]
What happens in bnxt_kdump_reset() if the function is not answering config
reads when the kdump kernel probes it? One example is a chip reset that is
still running.

The first PCI_COMMAND read at the top of the function is not checked with
PCI_POSSIBLE_ERROR(). In that case cmd would be 0xffff, and the masked
value gets written back. pci_save_state() also does not check what it
reads:

drivers/pci/pci.c:pci_save_state() {
	for (i = 0; i < 16; i++) {
		pci_read_config_dword(dev, i * 4, &dev->saved_config_space[i]);
		...
	}
	dev->state_saved = true;
	...
}

So it returns 0 with an all-ones snapshot, and the "Failed to save PCI
state" warning does not fire.

pcie_flr() would then fail its pci_dev_config_accessible() check. It
returns -ENOTTY without doing the reset, and the code carries on.

If the device starts responding during the new 5 second poll,
bnxt_clear_bars() zeroes BAR0-5. pci_restore_state() then writes back
every saved dword that differs from the live value, plus the PCIe control
registers:

  - BAR0-5 and the ROM BAR get 0xffffffff instead of the assigned
    addresses
  - PCI_COMMAND becomes 0xffff, which enables memory/IO decode and bus
    master on a function that was never FLR'd
  - BIST gets 0xff
  - pci_restore_pcie_state() writes DEVCTL as 0xffff, which includes
    PCI_EXP_DEVCTL_BCR_FLR

bnxt_kdump_reset() still returns 0 in this case. bnxt_init_one() then goes
on to bnxt_init_board() and maps BARs the device no longer decodes.

Could this cause the same UR/AER errors behind the PCIe switch that this
patch is meant to fix?

Would it make sense to check PCI_POSSIBLE_ERROR(cmd) after the first read,
or to validate the saved header before restoring it? The code could then
abort or skip the save/restore.

The later patch in this series, "bnxt_en: Re-write the BARs following any
type of PCIe errors", does not change bnxt_kdump_reset(). So this seems to
remain at the end of the series.

> +}
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005204246.3822563-1-michael.chan%40broadcom.com