| Message ID | 20190715084355.9962-1-kai.heng.feng@canonical.com |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | David Miller |
| Headers | show |
| Series | e1000e: Make speed detection on hotplugging cable more reliable | expand |
Dear Kai-Heng, Thank you for the patch. On 7/15/19 10:43 AM, Kai-Heng Feng wrote: > After hotplugging an 1Gbps ethernet cable with 1Gbps link partner, the > MII_BMSR may reports 10Mbps, renders the network rather slow. s/may reports/may report/ s/renders/rendering/ > The issue has much lower fail rate after commit 59653e6497d1 ("e1000e: > Make watchdog use delayed work"), which esssentially introduces some essentially > delay before running the watchdog task. > > But there's still a chance that the hotplugging event and the queued > watchdog task gets run at the same time, then the original issue can be > observed once again. > > So let's use mod_delayed_work() to add a deterministic 1 second delay > before running watchdog task, after an interrupt. I am not clear about the effects for the user. Could you elaborate please? Does the link now come up up to one second later? > Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> Any bug URL? > --- > drivers/net/ethernet/intel/e1000e/netdev.c | 12 ++++++------ > 1 file changed, 6 insertions(+), 6 deletions(-) Kind regards, Paul
at 4:52 PM, Paul Menzel <pmenzel@molgen.mpg.de> wrote: > Dear Kai-Heng, > > > Thank you for the patch. > > On 7/15/19 10:43 AM, Kai-Heng Feng wrote: >> After hotplugging an 1Gbps ethernet cable with 1Gbps link partner, the >> MII_BMSR may reports 10Mbps, renders the network rather slow. > > s/may reports/may report/ > s/renders/rendering/ Apparently English isn’t my mother tongue ;) > >> The issue has much lower fail rate after commit 59653e6497d1 ("e1000e: >> Make watchdog use delayed work"), which esssentially introduces some > > essentially Ok. > >> delay before running the watchdog task. >> >> But there's still a chance that the hotplugging event and the queued >> watchdog task gets run at the same time, then the original issue can be >> observed once again. >> >> So let's use mod_delayed_work() to add a deterministic 1 second delay >> before running watchdog task, after an interrupt. > > I am not clear about the effects for the user. Could you elaborate > please? Does the link now come up up to one second later? Yes, the link will be up on a fixed one second later. The delay varies between 0 to 2 seconds without this patch. > >> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> > > Any bug URL? If maintainers think it’s necessary then I’ll file one. Kai-Heng > >> --- >> drivers/net/ethernet/intel/e1000e/netdev.c | 12 ++++++------ >> 1 file changed, 6 insertions(+), 6 deletions(-) > > > Kind regards, > > Paul
Dear Kai Heng, (with or without hyphen?) On 7/15/19 11:00 AM, Kai Heng Feng wrote: > at 4:52 PM, Paul Menzel <pmenzel@molgen.mpg.de> wrote: >> On 7/15/19 10:43 AM, Kai-Heng Feng wrote: >>> After hotplugging an 1Gbps ethernet cable with 1Gbps link partner, the >>> MII_BMSR may reports 10Mbps, renders the network rather slow. >> >> s/may reports/may report/ >> s/renders/rendering/ > > Apparently English isn’t my mother tongue ;) No problem. Mine neither. >>> The issue has much lower fail rate after commit 59653e6497d1 ("e1000e: >>> Make watchdog use delayed work"), which esssentially introduces some >> >> essentially > > Ok. > >>> delay before running the watchdog task. >>> >>> But there's still a chance that the hotplugging event and the queued >>> watchdog task gets run at the same time, then the original issue can be >>> observed once again. >>> >>> So let's use mod_delayed_work() to add a deterministic 1 second delay >>> before running watchdog task, after an interrupt. >> >> I am not clear about the effects for the user. Could you elaborate >> please? Does the link now come up up to one second later? > > Yes, the link will be up on a fixed one second later. > > The delay varies between 0 to 2 seconds without this patch. Is there no other fix? Regarding booting a system fast (less than six seconds), a fixed one second delay is quite a regression on systems where it worked before. >>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >> >> Any bug URL? > > If maintainers think it’s necessary then I’ll file one. Not necessary, if there is none. I thought you had one in Launchpad or so. Kind regards, Paul
at 5:06 PM, Paul Menzel <pmenzel@molgen.mpg.de> wrote: > Dear Kai Heng, > > > (with or without hyphen?) > > On 7/15/19 11:00 AM, Kai Heng Feng wrote: >> at 4:52 PM, Paul Menzel <pmenzel@molgen.mpg.de> wrote: > >>> On 7/15/19 10:43 AM, Kai-Heng Feng wrote: >>>> After hotplugging an 1Gbps ethernet cable with 1Gbps link partner, the >>>> MII_BMSR may reports 10Mbps, renders the network rather slow. >>> >>> s/may reports/may report/ >>> s/renders/rendering/ >> >> Apparently English isn’t my mother tongue ;) > > No problem. Mine neither. > >>>> The issue has much lower fail rate after commit 59653e6497d1 ("e1000e: >>>> Make watchdog use delayed work"), which esssentially introduces some >>> >>> essentially >> >> Ok. >> >>>> delay before running the watchdog task. >>>> >>>> But there's still a chance that the hotplugging event and the queued >>>> watchdog task gets run at the same time, then the original issue can be >>>> observed once again. >>>> >>>> So let's use mod_delayed_work() to add a deterministic 1 second delay >>>> before running watchdog task, after an interrupt. >>> >>> I am not clear about the effects for the user. Could you elaborate >>> please? Does the link now come up up to one second later? >> >> Yes, the link will be up on a fixed one second later. >> >> The delay varies between 0 to 2 seconds without this patch. > > Is there no other fix? Regarding booting a system fast (less than six > seconds), a fixed one second delay is quite a regression on systems where > it worked before. This only affects when ethernet cable is hot plugged. Kai-Heng > >>>> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> >>> >>> Any bug URL? >> >> If maintainers think it’s necessary then I’ll file one. > > Not necessary, if there is none. I thought you had one in Launchpad or so. > > > Kind regards, > > Paul
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c index e4baa13b3cda..c83bf5349d53 100644 --- a/drivers/net/ethernet/intel/e1000e/netdev.c +++ b/drivers/net/ethernet/intel/e1000e/netdev.c @@ -1780,8 +1780,8 @@ static irqreturn_t e1000_intr_msi(int __always_unused irq, void *data) } /* guard against interrupt when we're going down */ if (!test_bit(__E1000_DOWN, &adapter->state)) - queue_delayed_work(adapter->e1000_workqueue, - &adapter->watchdog_task, 1); + mod_delayed_work(adapter->e1000_workqueue, + &adapter->watchdog_task, HZ); } /* Reset on uncorrectable ECC error */ @@ -1861,8 +1861,8 @@ static irqreturn_t e1000_intr(int __always_unused irq, void *data) } /* guard against interrupt when we're going down */ if (!test_bit(__E1000_DOWN, &adapter->state)) - queue_delayed_work(adapter->e1000_workqueue, - &adapter->watchdog_task, 1); + mod_delayed_work(adapter->e1000_workqueue, + &adapter->watchdog_task, HZ); } /* Reset on uncorrectable ECC error */ @@ -1907,8 +1907,8 @@ static irqreturn_t e1000_msix_other(int __always_unused irq, void *data) hw->mac.get_link_status = true; /* guard against interrupt when we're going down */ if (!test_bit(__E1000_DOWN, &adapter->state)) - queue_delayed_work(adapter->e1000_workqueue, - &adapter->watchdog_task, 1); + mod_delayed_work(adapter->e1000_workqueue, + &adapter->watchdog_task, HZ); } if (!test_bit(__E1000_DOWN, &adapter->state))
After hotplugging an 1Gbps ethernet cable with 1Gbps link partner, the MII_BMSR may reports 10Mbps, renders the network rather slow. The issue has much lower fail rate after commit 59653e6497d1 ("e1000e: Make watchdog use delayed work"), which esssentially introduces some delay before running the watchdog task. But there's still a chance that the hotplugging event and the queued watchdog task gets run at the same time, then the original issue can be observed once again. So let's use mod_delayed_work() to add a deterministic 1 second delay before running watchdog task, after an interrupt. Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com> --- drivers/net/ethernet/intel/e1000e/netdev.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)