Re: [PATCH] net/ixgbe: Limit SDP3 check of TX_DISABLE to appropriate devices

[Thomas Monjalon <thomas@monjalon.net>]

12/05/2022 19:01, Jeff Daly:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 11/05/2022 13:43, Zhang, Qi Z:
> > > From: Jeff Daly <jeffd@silicom-usa.com>
> > > >
> > > > 1ca05831b9b added a check that SDP3 (used as a TX_DISABLE output to
> > > > the SFP cage on these cards) is not asserted to avoid incorrectly
> > > > reporting link up when the SFP's laser is turned off.
> > > >
> > > > ff8162cb957 limited this workaround to fiber ports
> > > >
> > > > This patch:
> > > > * Adds devarg 'fiber_sdp3_no_tx_disable' not all fiber ixgbe devs use
> > > >   SDP3 as TX_DISABLE
> > > >
> > > > Fixes: 1ca05831b9b ("net/ixgbe: fix link status")
> > > > Fixes: ff8162cb957 ("net/ixgbe: fix link status")
> > > > Cc: stable@dpdk.org
> > > >
> > > > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > >
> > > Acked-by: Qi Zhang <qi.z.zhang@intel.com>
> > >
> > > Applied to dpdk-next-net-intel.
> > 
> > There is a lack of context in this description.
> > I don't know what SDP3 and TX_DISABLE refers to.
> > Please make more complete sentences, thanks.
> > 
> 
> I don't want to sound obtuse here, but this is a fix to a specific Intel NIC driver.  Any symbols or abbreviations or definitions used in a device driver are almost always in the manual.  While SDP3 means something specific to the Intel 82599 and X550 (and probably others), it probably doesn't appear in a driver from Marvell for example.  So, in this case [S]oftware [D]efined [P]ins [3] (out of 0-3) is specifically talking about the Intel X550.  I'm familiar enough with the hardware to recognize that, but if I was to look at a Marvell driver and saw something I didn't recognize like that, I'd be checking the Marvell manual.

Of course we can spend hours checking code and manuals.
But if we need to look to a lot of commits,
it is a lot more convenient to have a summary of this information
in the commit log.

> What I'm describing here is the fact that the TX_DISABLE signal (a signal defined in the SFP spec) from the NIC as implemented by the Software Defined Pin (3) by many (most?) implementations that are using this Intel driver, is not specifically the *only* use of SDP3.  A later patch limited the check (correctly) to fiber implementations (which is the only thing that makes sense), and *this* patch adds a module switch for platforms to disable this check in the event that they (as they are perfectly allowed to) don't use SDP3 as TX_DISABLE.

Thank you for the explanation.