RE: [PATCH] net/ixgbe: Treat 1G Cu SFPs as 1G SX on the X550 devices

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Thursday, 14 April 2022 19.07
> 
> 14/04/2022 17:43, Stephen Hemminger:
> > On Thu, 14 Apr 2022 15:11:46 +0000
> > Jeff Daly <jeffd@silicom-usa.com> wrote:
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 14/04/2022 14:13, Wang, Haiyue:
> > > > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > > > 14/04/2022 03:31, Wang, Haiyue:
> > > > > > > From: jeffd@silicom-usa.com <jeffd@silicom-usa.com>
> > > > > > > > From: Stephen Douthit <stephend@silicom-usa.com>
> > > > > > > >
> > > > > > > > 1G Cu SFPs are not officially supported on the X552/X553
> family
> > > > > > > > of devices but treat them as 1G SX modules since they
> usually
> > > > > > > > work.  Print a warning though since support isn't
> validated,
> > > > > > > > similar to what already happens for other unofficially
> supported
> > > > > > > > SFPs enabled via the allow_unsupported_sfps parameter
> inherited
> > > > from the mainline Linux driver.
> > > > > > > >
> > > > > > > > Signed-off-by: Stephen Douthit <stephend@silicom-usa.com>
> > > > > > > > Signed-off-by: Jeff Daly <jeffd@silicom-usa.com>
> > > > > > > > ---
> > > > > > > >  drivers/net/ixgbe/base/ixgbe_x550.c | 14 +++++++++++++-
> > > > > > > >  1 file changed, 13 insertions(+), 1 deletion(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > > > b/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > > > index 8810d1658e..8d1bc6c80d 100644
> > > > > > > > --- a/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > > > +++ b/drivers/net/ixgbe/base/ixgbe_x550.c
> > > > > > > > @@ -1538,9 +1538,21 @@ STATIC s32
> > > > > > > > ixgbe_supported_sfp_modules_X550em(struct ixgbe_hw *hw,
> bool
> > > > > > *linear)
> > > > > > >
> > > > > > > NACK.
> > > > > > >
> > > > > > > As for 1G Cu SFP treating it as 1G SX, some 1G-Base-T SFP
> modules
> > > > > > > require the use of RX_ILOS and some Intel Ethernet products
> don't
> > > > support that.
> > > > > >
> > > > > > So what is the solution?
> > > > > >
> > > > > > > And the DPDK keeps the same design with kernel.
> > > > > >
> > > > > > It should not be a justification for limiting DPDK features.
> > > > >
> > > > > Um, this is upstream version driver to keep the same behavior.
> > > > >
> > > > > There are also some kind of custom release ...
> > > >
> > > > I don't understand.
> > > > Upstream DPDK (and Linux) must support a maximum of hardware and
> > > > setup.
> > > > Why rejecting adding such compatibility?
> > > >
> > >
> > > so, I will ask a question directly in case people just aren't
> inclined to make a suggestion
> > > (and perhaps this should be also directed to the Linux kernel
> driver mailing list), but
> > > if there's a driver option: module_param(allow_unsupported_sfp,
> uint, 0) to allow
> > > enabling non-official support of some SFPs, then I can't image that
> it wouldn't also be
> > > acceptable to add: module_param(cu_sfp_as sx, uint, 0) to be able
> to select whether
> > > to enable this specific handling as well?
> > >
> > > if a patch of this nature is acceptable to Linux driver
> maintainers, then it would also be
> > > here as well according to your explanation of the NACK,  correct?
> >
> > Makes sense for DPDK to have a similar option to enable (at your own
> risk) SFP's.
> > But:
> >    - there is no equivalent of module_params in DPDK; you will have
> to think of something
> 
> We have devargs which is supposed to be used per port with the option -
> a.
> In future, I would like to use devargs with another option
> which is not necessarily tight to a port, so it can be per-driver.
> The devargs syntax already allows to configure a driver, example:
> 	class=eth/driver=foo,param=bar

I have a very strong preference for per-driver options over per-port options!

Per-port options are difficult to work with when the application detects the hardware configuration at runtime. E.g. when using modular hardware, where different types of NICs may be plugged into a NIC module slot.

> 
> >    - should print message that when enabled the driver is no longer
> supported.
> 
> It could be supported by Silicom.

There's more to "supported by" than meets the eye: When an ODM designs products using Intel chips, some sort of customer support from Intel field application engineers is expected by the ODM. We cannot expect Silicom to provide design support to anyone but their own customers. E.g. if the NIC is behaving weird at the hardware bring-up phase, where it might be any type of problem, Silicom will not be able to provide the kind of support required. My point is: There is a difference between community support and customer support.

Let me throw up an idea for consideration... I'm trying to think out of the box here, so please forgive me if I'm stepping on anyone's toes with this suggestion:

If Intel doesn't want to take on the responsibility and support for this feature graciously donated by Silicom (which is obviously Intel's own decision to make), but the DPDK community thinks the feature is beneficial, perhaps Silicom could be accepted as the maintainer of this part of the driver? The driver would still come with a big fat disclaimer saying that this feature is not supported by Intel, but maintained by Silicom, who also provides community support for it.

The worst case alternative is a fork or separate add-on patch set offered by the donor. This has certainly happened to other projects. Don't get me wrong, we are not there at all regarding this feature! I'm just wondering if we can make the DPDK project even more inclusive, so we can avoid forks and add-on patch sets now and in the future.

-Morten