Re: [PATCH net v3 3/3] bnxt_en: Re-write the BARs following any type of PCIe errors

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 1 · Medium: 2 · 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).

- [High] The new guard `if (!pci_is_enabled(pdev) &&
  pci_enable_device(pdev))` is a non-atomic check-then-act.
- [Medium] In bnxt_io_slot_reset()
  (drivers/net/ethernet/broadcom/bnxt/bnxt.c), the slot-reset handler…
- [Medium] The commit says it disables memory decode and bus mastering
  before rewriting the BARs 'so the device doesn't decode a half-updated…

--

bnxt_en: Re-write the BARs following any type of PCIe errors

With this patch, bnxt_io_slot_reset() zeroes and restores the BARs after
every kind of PCIe error, not only fatal ones, and the 5 second wait after
the reset is now unconditional. Memory decode and bus mastering are turned
off while the BARs are rewritten, and pci_enable_device() is deferred until
after the restore. pci_enable_device() in both bnxt_io_slot_reset() and
bnxt_fw_reset_task() is now guarded by pci_is_enabled().

> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 9ea7e172787e2..5bd817479d642 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -15547,7 +15547,7 @@ static void bnxt_fw_reset_task(struct work_struct *work)
>  		if (test_and_clear_bit(BNXT_STATE_FW_ACTIVATE_RESET, &bp->state) &&
>  		    !test_bit(BNXT_STATE_FW_ACTIVATE, &bp->state))
>  			bnxt_dl_remote_reload(bp);
> -		if (pci_enable_device(bp->pdev)) {
> +		if (!pci_is_enabled(bp->pdev) && pci_enable_device(bp->pdev)) {

[Severity: High]

The commit message says bnxt_fw_reset_task() may be re-enabling the device
at the same time as bnxt_io_slot_reset(), and that this guard keeps
enable_cnt balanced. Is the check-then-enable atomic with respect to
bnxt_io_slot_reset()?

In the ENABLE_DEV state, bnxt_fw_reset_task() holds neither netdev_lock
nor any other lock that bnxt_io_slot_reset() takes. So both contexts can
see pci_is_enabled() == 0 before either one reaches the increment:

drivers/pci/pci.c:pci_enable_device_flags() {
    ...
	pci_update_current_state(dev, dev->current_state);

	if (atomic_inc_return(&dev->enable_cnt) > 1)
		return 0;		/* already enabled */
    ...
}

pci_update_current_state() does a config read before the increment, so
the window is wider than it looks.

If both callers get into pci_enable_device(), enable_cnt ends up at 2.
bnxt_io_error_detected(), bnxt_fw_reset_close() and bnxt_cleanup_pci()
each drop a single reference, behind a pci_is_enabled() check:

	if (pci_is_enabled(bp->pdev))
		pci_disable_device(bp->pdev);

drivers/pci/pci.c:pci_disable_device() {
    ...
	if (atomic_dec_return(&dev->enable_cnt) != 0)
		return;
    ...
}

In that case do_pci_disable_device() never runs. Can bus mastering then
stay on while bnxt_free_ctx_mem() and bnxt_free_mem() free DMA memory? Can
the device also stay enabled after unbind?

One way to reach this: bnxt_io_error_detected() returns DISCONNECT because
BNXT_STATE_IN_FW_RESET is already set. A sibling function then votes
NEED_RESET, merge_result() upgrades the result, and slot_reset is still
delivered to this PF while fw_reset_task is active.

Do the two recovery flows need real serialization instead? Some options:
take netdev_lock around ENABLE_DEV, cancel or flush fw_reset_task, or skip
slot_reset recovery when error_detected aborted.

[ ... ]

> @@ -17608,10 +17608,8 @@ static pci_ers_result_t bnxt_io_error_detected(struct pci_dev *pdev,
>  	 * so we disable bus master to prevent any potential bad DMAs before
>  	 * freeing kernel memory.
>  	 */
> -	if (state == pci_channel_io_frozen) {
> -		set_bit(BNXT_STATE_PCI_CHANNEL_IO_FROZEN, &bp->state);
> +	if (state == pci_channel_io_frozen)
>  		bnxt_fw_fatal_close(bp);
> -	}

[Severity: Medium]

BNXT_STATE_PCI_CHANNEL_IO_FROZEN was only set on the path where
bnxt_io_error_detected() actually quiesced the device. Once it is gone,
how does bnxt_io_slot_reset() know whether error_detected took ownership?

Earlier in bnxt_io_error_detected(), this happens when a firmware reset is
already in progress:

	if (test_and_set_bit(BNXT_STATE_IN_FW_RESET, &bp->state)) {
		netdev_err(bp->dev, "Firmware reset already in progress\n");
		abort = true;
	}

	if (abort || state == pci_channel_io_perm_failure) {
		netdev_unlock(netdev);
		return PCI_ERS_RESULT_DISCONNECT;
	}

That path returns without calling __bnxt_close_nic(),
bnxt_fw_fatal_close() or pci_disable_device().

Suppose a sibling function under the same bridge, such as the other PF of
a dual-port adapter, votes NEED_RESET. merge_result() then upgrades the
result:

drivers/pci/pcie/err.c:merge_result() {
    ...
	case PCI_ERS_RESULT_DISCONNECT:
		if (new == PCI_ERS_RESULT_NEED_RESET)
			orig = PCI_ERS_RESULT_NEED_RESET;
    ...
}

report_slot_reset() then calls bnxt_io_slot_reset() on this PF anyway.

bnxt_io_slot_reset() now always clears master and memory decode, zeroes
the BARs and calls pci_restore_state(). The device is still enabled, so
the new pci_is_enabled() guard skips the enable and execution continues.
bnxt_try_recover_fw(), bnxt_hwrm_func_reset(), bnxt_ulp_irq_stop() and
bnxt_clear_int_mode() all run, and BNXT_STATE_IN_FW_RESET is cleared,
while bnxt_fw_reset_task() still owns the device.

Before this patch the BARs were not rewritten on this path. Going by the
commit message, MMIO returned all-ones and recovery stopped at "Firmware
not ready".

Now it can return PCI_ERS_RESULT_RECOVERED, which leads to
bnxt_io_resume()->bnxt_open(). Meanwhile the OPENING state in
bnxt_fw_reset_task() may also call bnxt_open(). It can also take rtnl
there and then block on the netdev_lock that slot_reset holds.

Should error_detected record whether it quiesced the device, so that
slot_reset can bail out in the abort case (or cancel or flush
fw_reset_task)?

[ ... ]

> @@ -17641,65 +17639,80 @@ static pci_ers_result_t bnxt_io_slot_reset(struct pci_dev *pdev)

[ ... ]

> +	/* Upon PCIe error, our device internal logic that latches to
> +	 * BAR value is getting reset and will restore only upon
> +	 * rewriting the BARs.
> +	 *
> +	 * As pci_restore_state() does not re-write the BARs if the
> +	 * value is same as saved value earlier, driver needs to
> +	 * write the BARs to 0 to force restore.
> +	 */
> +	pci_clear_master(pdev);
> +	pci_read_config_word(pdev, PCI_COMMAND, &cmd);
> +	cmd &= ~PCI_COMMAND_MEMORY;
> +	pci_write_config_word(pdev, PCI_COMMAND, cmd);
>  

[ ... ]

> +	bnxt_clear_bars(pdev);
> +	pci_restore_state(pdev);

[Severity: Medium]

According to the commit message, decode and bus mastering are turned off
here "so the device doesn't decode a half-updated address". The commit
message also expects bnxt_fw_reset_task() to run at the same time. Does
the guarantee still hold in that case?

The ENABLE_DEV state in bnxt_fw_reset_task() shares no lock with
bnxt_io_slot_reset(), and it does this:

		if (!pci_is_enabled(bp->pdev) && pci_enable_device(bp->pdev)) {
			...
		}
		pci_set_master(bp->pdev);
		bp->fw_reset_state = BNXT_FW_RESET_STATE_POLL_FW;
		fallthrough;

pci_enable_device() can set PCI_COMMAND_MEMORY, and pci_set_master() sets
PCI_COMMAND_MASTER.

Suppose that lands after the PCI_COMMAND_MEMORY clear above but before
pci_restore_state() finishes. Wouldn't the function then decode memory
with the BARs zeroed by bnxt_clear_bars()? It would keep decoding while
the BARs are rewritten one dword at a time:

drivers/pci/pci.c:pci_restore_config_space() {
    ...
		pci_restore_config_space_range(pdev, 10, 15, false);
		/* Restore BARs before the command register. */
		pci_restore_config_space_range(pdev, 4, 9, false);
		pci_restore_config_space_range(pdev, 0, 3, false);
    ...
}

For a 64-bit BAR, the upper dword could be correct while the lower dword
is still zero, with bus master enabled.

The reverse can also happen. The POLL_FW_DOWN and POLL_FW states read
BAR0 through bnxt_fw_health_readl() and bnxt_hwrm_poll() without
netdev_lock. If those reads land while slot_reset has decode turned off,
would they get Unsupported Request completions and all-ones data that the
reset state machine reads as firmware status?

The new pci_is_enabled() checks only change enable_cnt bookkeeping. They
don't order the Command register and BAR writes between the two paths.

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