Re: [PATCH iwl-next v1 3/6] ixgbe: E610: do not disable LSE on driver down/remove

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
ixgbe: E610: do not disable LSE on driver down/remove

This patch removes the calls to ixgbe_disable_link_status_events() in
ixgbe_down() and ixgbe_remove() for the E610 MAC. The intent is to keep
Link Status Events (LSE) enabled when the interface is down or unloading.

> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 882021c436d0f..83098bf522e42 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
[ ... ]
> @@ -6733,8 +6720,6 @@ void ixgbe_down(struct ixgbe_adapter *adapter)
>  
>  	ixgbe_clean_all_tx_rings(adapter);
>  	ixgbe_clean_all_rx_rings(adapter);
> -	if (adapter->hw.mac.type == ixgbe_mac_e610)
> -		ixgbe_disable_link_status_events(adapter);

[Severity: High]
Does removing this call allow unhandled asynchronous LSE events to permanently
block the ACI mailbox?

If a link event occurs while the driver is down, the firmware can queue an
event in the ACI queue. Upon the next driver load, before the service task
can clear events, commands like ixgbe_get_flash_data() are called.

In ixgbe_aci_send_cmd_execute(), if the EV bit is asserted, the synchronous
response is overwritten by the unhandled asynchronous event from HIDA_2:

        if ((hicr & IXGBE_PF_HICR_EV) && !(hicr & IXGBE_PF_HICR_C))
                for (i = 0; i < IXGBE_ACI_DESC_SIZE_IN_DWORDS; i++)
                        raw_desc[i] = IXGBE_READ_REG(hw, IXGBE_PF_HIDA_2(i));

        if (desc->opcode != cpu_to_le16(opcode) &&
            opcode != ixgbe_aci_opc_get_fw_event)
                return -EIO;

Could this result in an opcode mismatch and return -EIO for all future
commands, causing device initialization to fail?

>  }
>  
>  /**
[ ... ]
> @@ -12080,9 +12065,6 @@ static void ixgbe_remove(struct pci_dev *pdev)
>  	set_bit(__IXGBE_REMOVING, &adapter->state);
>  	cancel_work_sync(&adapter->service_task);
>  
> -	if (adapter->hw.mac.type == ixgbe_mac_e610)
> -		ixgbe_disable_link_status_events(adapter);
> -

[Severity: High]
Can the same mailbox blockage happen here during driver unload?

If LSE is not disabled on remove, a link state change (such as a cable
unplug) after unload could leave the ACI queue with pending events.

Would this cause the same -EIO failure during the subsequent ixgbe_probe()
when the driver is loaded again?

>  	if (adapter->mii_bus)
>  		mdiobus_unregister(adapter->mii_bus);
>