[Intel-wired-lan] [PATCH net v3] e1000e: Disable TSO on i219-LM card to increase speed

[Palczewski, Mateusz]

Hi,
Sorry for late response 
> Hi,
> 
> On Wed, Feb 22, 2023 at 10:22 PM Paul Menzel <pmenzel at molgen.mpg.de> wrote:
> >
> > [Cc: +Kai-Heng]
> 
> Thanks for adding me to Cc list.
> 
> Please add me to the Cc list if there's next revision.
Sure, will do.
> 
> >
> >
> > Dear Mateusz, dear Sebastian,
> >
> >
> > Thank you for the patch.
> >
> > Am 22.02.23 um 15:07 schrieb Mateusz Palczewski:
> > > From: Sebastian Basierski <sebastianx.basierski at intel.com>
> > >
> > > While using i219-LM card currently it was only possible to achieve 
> > > about 60% of maximum speed due to regression introduced in Linux 5.8-rc1.
> > > This was caused by TSO not being disabled by default despite commit
> > > f29801030ac6 ("e1000e: Disable TSO for buffer overrun workaround") 
> > > implementation. Fix that by disabling TSO during driver probe.
> 
> Does that mean "watchdog_timer" isn't scheduled?

I think it is, but for some reason forcing TSO to be disabled in e1000_watchdog_task() does work only after reloading the driver on server and not right away after it was booted.
I believe with the solution posted in the new patch it should work right away.
 
> 
> >
> > Can the code added by the referenced commit then be removed?

You are right, I will prepare new patch version with removal of the code that disables TSO in watchdog task and instead does this during probe.

> >
> > Also the questions from the discussion of v2(?) was not answered, why 
> > the conditions in the if statement of the code added by commit
> > f29801030ac6 where not true.
> >
> >      /* disable TSO for pcie and 10/100 speeds, to avoid
> >       * some hardware issues
> >       */
> >      if (!(adapter->flags & FLAG_TSO_FORCE)) {
> 
> Yea, my idea was to take FLAG_TSO_FORCE into consideration hence the adding the change to this if block.
> 
> Maybe someone still wants to enable TSO despite of the downside?

By disabling TSO during probe we are not shutting it down completly, if a user wants to use it anyway despite speed decrease it can be done manually with ethtool. 

> 
> Kai-Heng
> 
> >
> > Missing Fixes: tag below.
Thanks, will add that in v4 version. 
> >
> > > Signed-off-by: Sebastian Basierski <sebastianx.basierski at intel.com>
> > > Signed-off-by: Mateusz Palczewski <mateusz.palczewski at intel.com>
> > > ---
> > >   v3: Changed the patch to disable TSO during the probe
> > >   v2: Fixed commit message and comment inside the code
> > > ---
> > >   drivers/net/ethernet/intel/e1000e/netdev.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c 
> > > b/drivers/net/ethernet/intel/e1000e/netdev.c
> > > index 04acd1a992fa..863796acf8d7 100644
> > > --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> > > +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> > > @@ -7529,6 +7529,14 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> > >                           NETIF_F_RXCSUM |
> > >                           NETIF_F_HW_CSUM);
> > >
> > > +      /* Disable TSO for i219 to avoid transfer speed
> > > +       * being capped at 60%.
> > > +       */
> > > +     if (hw->mac.type == e1000_pch_spt) {
> > > +             netdev->features &= ~NETIF_F_TSO;
> > > +             netdev->features &= ~NETIF_F_TSO6;
> > > +     }
> > > +
> > >       /* Set user-changeable features (subset of all device features) */
> > >       netdev->hw_features = netdev->features;
> > >       netdev->hw_features |= NETIF_F_RXFCS;
> >
> >
> > Kind regards,
> >
> > Paul
>