Re: igb fc (aka flowcontrol) issue...

Eric Joyner wrote this message on Sun, Aug 01, 2021 at 21:10 -0700:
> On Sun, Aug 1, 2021 at 6:59 PM John-Mark Gurney <jmg@funkthat.com> wrote:
> 
> > I have a dual port igb card:
> > igb0: <Intel(R) PRO/1000 82576> port 0x2020-0x203f mem
> > 0xd1020000-0xd103ffff,0xd0c00000-0xd0ffffff,0xd1044000-0xd1047fff irq 17 at
> > device 0.0 on pci3
> > igb0: Using 1024 TX descriptors and 1024 RX descriptors
> > igb0: Using 4 RX queues 4 TX queues
> > igb0: Using MSI-X interrupts with 5 vectors
> > igb0: Ethernet address: xxxx
> > igb0: netmap queues/slots: TX 4/1024, RX 4/1024
> > igb1: <Intel(R) PRO/1000 82576> port 0x2000-0x201f mem
> > 0xd1000000-0xd101ffff,0xd0800000-0xd0bfffff,0xd1040000-0xd1043fff irq 18 at
> > device 0.1 on pci3
> > igb1: Using 1024 TX descriptors and 1024 RX descriptors
> > igb1: Using 4 RX queues 4 TX queues
> > igb1: Using MSI-X interrupts with 5 vectors
> > igb1: Ethernet address: xxxx
> > igb1: netmap queues/slots: TX 4/1024, RX 4/1024
> >
> > And I have observed an issue with it when adjusting flow control, it
> > seems to adjust both ports at the same time:
> > # sysctl dev.igb.0.fc
> > dev.igb.0.fc: 3
> > # sysctl dev.igb.1.fc
> > dev.igb.1.fc: 3
> > # sysctl dev.igb.0.fc=0
> > dev.igb.0.fc: 3 -> 0
> > # sysctl dev.igb.1.fc
> > dev.igb.1.fc: 0
> >
> > Is this correct behavior?
> >
> > Also, the fc sysctl is not documented, I propose the following:
> > diff --git a/share/man/man4/em.4 b/share/man/man4/em.4
> > index a1fa22c..bad82e9 100644
> > --- a/share/man/man4/em.4
> > +++ b/share/man/man4/em.4
> > @@ -265,6 +265,26 @@ If
> >  is non-zero, this tunable limits the maximum delay in which a transmit
> >  interrupt is generated.
> >  .El
> > +.Sh SYSCTL VARIABLES
> > +The following variable is availabel:
> > +.Bl -tag -width "dev.em.X.fc"
> > +.It Va dev.em.X.fc
> > +This sysctl sets the flow control for the card (this means both ports
> > +on a dual port card).  The values are as follows:
> > +.Bl -tag -width "XX" -offset "XXXX" -compact
> > +.It 0
> > +Disabled
> > +.It 1
> > +Process received pause frames, do not transmit them.
> > +.It 2
> > +Send pause frames, but to not process received frames.
> > +.It 3
> > +Process and send pause frames.
> > +.It 4
> > +No software override, use EEPROM configuration.
> > +.El
> > +.El
> > +Note: That the variable is available for igb as well.
> >  .Sh FILES
> >  .Bl -tag -width /dev/led/em*
> >  .It Pa /dev/led/em*
> 
> I was looking at the hardware specs to see a mention of it, but I didn't
> find anything. But looking at the code, I do see that in if_em.c's
> em_set_flowcntl(), the value "input" used in the output of the sysctl is a
> "static int". So it seems like if you set the value to 0, you're going to
> read 0 from it, even on the second port. That looks like a bug that needs
> to be fixed.

Yeah, it looks like reading the status of the flowcontrol doesn't work.  It
just reads whatever was last set.

change intput to:
int input;
input = adapter->fc;

does fix the sysctl problem of reading.

But, it does appear that adapter->fc does not get initalized anywhere,
which means reading it won't be correct until it's set the first time.
And even then, it's initalized to 0, so if you first set it to zero,
it won't change anything, because it doesn't have the correct state in
the var, and only does the set if the value changed...

So w/o additional work, you have to turn it on first, before you can
turn if off...

> And for the man page, that sysctl doesn't accept "4" as a valid value
> (unless you're going to add that?). I wouldn't put that flow control sets
> the value for both ports on the card, either. But otherwise, that seems
> reasonable to me.

Well, I was just going by the comment in e1000_phy.c:
         *  other:  No software override.  The flow control configuration
         *          in the EEPROM is used.

and I picked 4 for other.  But now that I look at the code, the code
doesn't implement that case.  I'll drop that case then.

I added that note because that was the observed behavior...  I'll
drop it as it's been tracked down to a sysctl implemention bug.  (I
wanted the patch to be what I was going to commit so there wasn't
confusion.)

-- 
  John-Mark Gurney				Voice: +1 415 225 5579

     "All that I will do, has been done, All that I have, has not."