On Mon, Dec 16, 2024 at 06:53:10AM +0100, Przemek Kitszel wrote: > On 12/16/24 06:14, Chia-Lin Kao (AceLan) wrote: > > When booting with a dock connected, the igc driver can get stuck for ~40 > > seconds if PCIe link is lost during initialization. > > > > This happens because the driver access device after EECD register reads > > return all F's, indicating failed reads. Consequently, hw->hw_addr is set > > to NULL, which impacts subsequent rd32() reads. This leads to the driver > > hanging in igc_get_hw_semaphore_i225(), as the invalid hw->hw_addr > > prevents retrieving the expected value. > > Than you very much for the patch and the analysis! > > > > > To address this, a validation check is added for the EECD register read > > result. If all F's are returned, indicating PCIe link loss, the driver > > will return -ENXIO immediately. This avoids the 40-second hang and > > It is not clear from the patch what part of the driver will return > -ENXIO, you have put -ENODEV in the patch, but it's ignored anyway. I was thinking of using -ENODEV or -ENXIO, and I forgot to generate the patch again after I changed it to -ENXIO in the commit message. I'll fix this in v2. > > > significantly improves boot time when using a dock with an igc NIC. > > > > [ 0.911913] igc 0000:70:00.0: enabling device (0000 -> 0002) > > [ 0.912386] igc 0000:70:00.0: PTM enabled, 4ns granularity > > [ 1.571098] igc 0000:70:00.0 (unnamed net_device) (uninitialized): PCIe > > link lost, device now detached > > [ 43.449095] igc_get_hw_semaphore_i225: igc 0000:70:00.0 (unnamed > > net_device) (uninitialized): Driver can't access device - SMBI bit is set. > > [ 43.449186] igc 0000:70:00.0: probe with driver igc failed with error -13 > > [ 46.345701] igc 0000:70:00.0: enabling device (0000 -> 0002) > > [ 46.345777] igc 0000:70:00.0: PTM enabled, 4ns granularity > > > > Would be best if you could also attach the sequence after your fix. Sure
> Please add a Fixes: tag. I'm not sure which commit should I add Fixes to. > Please make [PATCH iwl-net] as a subject prefix. Please CC Vitaly. igc is an ethernet driver, should I also add iwl-net tag? > (But please also wait a day prior to sending v2 for more feedback). > > > Signed-off-by: Chia-Lin Kao (AceLan) <[email protected]> > > --- > > drivers/net/ethernet/intel/igc/igc_base.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/drivers/net/ethernet/intel/igc/igc_base.c > > b/drivers/net/ethernet/intel/igc/igc_base.c > > index 9fae8bdec2a7..54ce60280765 100644 > > --- a/drivers/net/ethernet/intel/igc/igc_base.c > > +++ b/drivers/net/ethernet/intel/igc/igc_base.c > > @@ -68,6 +68,10 @@ static s32 igc_init_nvm_params_base(struct igc_hw *hw) > > This function is used only in igc_get_invariants_base(), which ignores > the return value you have added. I would expect it to propagate instead. You are right, looks like the patch fixes the issue accidentally. Return earlier in igc_get_invariants_base() skipping the rest of the settings. The part impacts the behavior is nvm->word_size will be 0. And then in igc_get_hw_semaphore_i225() the timeout value will become 1. So that we won't hang for 40 seconds to wait for the timeout. This patch is not perfect, I need to figure out another way to address the issue better. Please let me know if you got any ideas. Thanks. > > > u32 eecd = rd32(IGC_EECD); > > u16 size; > > + /* failed to read reg and got all F's */ > > + if (!(~eecd)) > > + return -ENODEV; > > + > > size = FIELD_GET(IGC_EECD_SIZE_EX_MASK, eecd); > > /* Added to a constant, "size" becomes the left-shift value >
