From: Simon Horman <[email protected]> Sent: Wednesday, July 8, 2026 7:02 PM
>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 >--- Hi Simon, yeah i saw most of these on sashiko local run and i believe we should not be worried about them. I see that sashiko does not understand some of the FW/NIC <-> driver communication concepts for this series >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? No, removing this call does the opposite - it let driver to handle events which without that step would not be received at all. > >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. That's not true and actually it's opposite to the idea of async events. Events are not queued when driver is down, that's the period when it may required to be notified by some sort of event, like eg link state change so simply its send then "before the service task can clear events" they are handled in an async way, not synchronously by servise task > >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: It still bases on assumption that for some reason event has not been handled That's mislesading, handling mechanism is implemented and events are handled one by one. HIDA_2 corresponds to the cmd which is proceesed in given moment. > > 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? I really do not see such possibility > >> } >> >> /** >[ ... ] >> @@ -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? That's even more odd. Event is not something what is kept to be send between driver reloads / FW reinit > >> if (adapter->mii_bus) >> mdiobus_unregister(adapter->mii_bus); >>
