RE: [PATCH net-next v3 2/5] net: lan743x: read SFP straps from PCI11x1x device

[Thangaraj.S]

Hi Jakub,
Thanks for the review comments.

> -----Original Message-----
> From: Jakub Kicinski <kuba@xxxxxxxxxx>
> Sent: Tuesday, May 12, 2026 7:39 AM
> To: Thangaraj Samynathan - I53494 <Thangaraj.S@xxxxxxxxxxxxx>
> Cc: Jakub Kicinski <kuba@xxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx;
> andrew+netdev@xxxxxxx; davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; Bryan Whitehead - C21958
> <Bryan.Whitehead@xxxxxxxxxxxxx>; UNGLinuxDriver
> <UNGLinuxDriver@xxxxxxxxxxxxx>; linux@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH net-next v3 2/5] net: lan743x: read SFP straps from
> PCI11x1x device
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> This is an AI-generated review of your patch. The human sending this email
> has considered the AI review valid, or at least plausible.
> ---
> net: lan743x: read SFP straps from PCI11x1x device
> 
> This commit reads SFP enable bits from the PCI11x1x strap registers and
> stores the result in a new adapter->is_sfp_support_en flag, with a validation
> message when SFP support appears to be requested without SGMII.
> 
> > diff --git a/drivers/net/ethernet/microchip/lan743x_main.c
> > b/drivers/net/ethernet/microchip/lan743x_main.c
> > index fad4a246e06e..867310dbe9ba 100644
> > --- a/drivers/net/ethernet/microchip/lan743x_main.c
> > +++ b/drivers/net/ethernet/microchip/lan743x_main.c
> > @@ -62,6 +62,12 @@ static void pci11x1x_strap_get_status(struct
> lan743x_adapter *adapter)
> >                       adapter->is_pcs_en = true;
> >               else
> >                       adapter->is_pcs_en = false;
> > +
> > +             if ((strap & STRAP_SFP_USE_EN_) && (strap & STRAP_SFP_EN_))
> > +                     adapter->is_sfp_support_en = true;
> > +             else
> > +                     adapter->is_sfp_support_en = false;
> > +
> 
> [Low]
> Is there a reason STRAP_SFP_USE_EN_ and STRAP_SFP_EN_ are only
> evaluated inside the branch gated by (strap & STRAP_READ_USE_SGMII_EN_)
> (or the A0 workaround)?
> 
> If a board is strapped with STRAP_SFP_USE_EN_=1 but
> STRAP_READ_USE_SGMII_EN_=0, the SFP straps are silently ignored,
> is_sfp_support_en keeps its default of false, and no diagnostic is emitted.
> 
> The two "use strap" gate bits look independent (one for SGMII, one for SFP);
> nesting the SFP decode inside the SGMII decode appears to conflate them.
> The "SFP_EN without SGMII_EN" validation block below will also never fire in
> this configuration because is_sfp_support_en is forced to false.

[Thangaraj Samynathan]  The nesting is intentional. On PCI11x1x, SFP operation requires the PCS/SGMII interface to be active - SFP cannot function
 without it. The STRAP_SFP_USE_EN_/STRAP_SFP_EN_ bits are therefore only meaningful when STRAP_READ_USE_SGMII_EN_ is set (or the A0
 workaround applies). If SGMII is not strapped on, is_sfp_support_en being false is the correct outcome regardless of the SFP strap
 values, since the hardware cannot support SFP in that configuration. The validation message for "SFP_EN without SGMII_EN" covers
 the case where both SGMII and SFP bits are present in the strap register but contradict each other.

> 
> >       } else {
> >               fpga_rev = lan743x_csr_read(adapter, FPGA_REV);
> >               if (fpga_rev) {
> > @@ -73,8 +79,17 @@ static void pci11x1x_strap_get_status(struct
> lan743x_adapter *adapter)
> >                       adapter->is_pcs_en = false;
> >               }
> >       }
> 
> [Low]
> In the else (FPGA/non-strap) branch, adapter->is_sfp_support_en is never
> assigned, while adapter->is_pcs_en is written in every sub-branch.  The code
> relies on the one-time initialization of is_sfp_support_en = false in
> lan743x_pcidev_probe().
> 
> Today pci11x1x_strap_get_status() is only called once from
> lan743x_hardware_init() during probe, so the current behavior is well defined.
> Would it be worth setting adapter->is_sfp_support_en = false in this else
> branch as well to mirror is_pcs_en and keep the function idempotent if it is
> ever called from a reset, resume, or error-recovery path later?

[Thangaraj Samynathan]  The else branch handles the FPGA path where SFP support
is not available. Since SFP is only supported on silicon with valid strap configuration,
is_sfp_support_en is correctly left at its probe-time default of false for this path.
No assignment is needed here.

> 
> > +
> > +     if (adapter->is_pci11x1x && !adapter->is_pcs_en &&
> > +         adapter->is_sfp_support_en) {
> > +             netif_err(adapter, drv, adapter->netdev,
> > +                       "Invalid EEPROM configuration: SFP_EN strap specified
> without SGMII_EN strap\n");
> > +     }
> 
> [Medium]
> The commit message describes this as:
> 
>     Add a validation check to ensure PCS is enabled when SFP support
>     is requested, as SFP operation requires the PCS interface.
> 
> Does this block actually enforce that invariant?  It logs the invalid combination
> but leaves adapter->is_sfp_support_en set to true and does not propagate an
> error out of pci11x1x_strap_get_status() or lan743x_hardware_init().
> 
> Later patches in the same series (for example "net: lan743x: Add PCS/XPCS
> support for SFP on PCI11x1x") gate lan743x_pcs_mdiobus_init() and the
> swnodes/SFP platform-device registration on is_sfp_support_en alone,
> without an is_pcs_en check.  With a misconfigured EEPROM the driver would
> then attempt PCS/XPCS MDIO operations on hardware whose PCS is disabled.
> 
> Would it be preferable to either force adapter->is_sfp_support_en = false
> when the invalid combination is detected, or return an error from
> pci11x1x_strap_get_status() so lan743x_hardware_init() can fail probe?
> 
[Thangaraj Samynathan - I53494]  The check is intentionally a warning 
the misconfiguration is an EEPROM issue outside the driver's control, and the Ethernet
controller remains functional without SFP. Failing probe entirely over an SFP strap
misconfiguration would make the NIC unusable unnecessarily. However, you are right
that leaving is_sfp_support_en = true after the warning is unsafe. We will force
adapter->is_sfp_support_en = false when the invalid combination is detected, ensuring
all subsequent SFP/PCS/XPCS paths are safely skipped.
> > +
> >       netif_dbg(adapter, drv, adapter->netdev,
> >                 "PCS I/F %s\n",
> > str_enable_disable(adapter->is_pcs_en));
> > +     netif_dbg(adapter, drv, adapter->netdev,
> > +               "SFP support %s\n",
> > + str_enable_disable(adapter->is_sfp_support_en));
> >  }
> 
> [Low]
> The commit message says:
> 
>     Refactor debug logging to use the str_enable_disable() helper for
>     consistency with modern kernel standards.
> 
> Is this description accurate?  The existing "PCS I/F %s" netif_dbg() already
> uses str_enable_disable() prior to this patch (it was introduced by the earlier
> "net: lan743x: rename is_sgmii_en to is_pcs_en" commit).
> 
> This patch only adds a new netif_dbg() for is_sfp_support_en using the same
> helper; no existing debug line is refactored.  Could the bullet be reworded to
> reflect that a new debug line was added rather than an existing one
> refactored?
> 

[Thangaraj Samynathan - I53494] Agreed. Will fix in next revision.

> > diff --git a/drivers/net/ethernet/microchip/lan743x_main.h
> > b/drivers/net/ethernet/microchip/lan743x_main.h
> > index f0fa0580b04e..26c30dc2e55c 100644
> > --- a/drivers/net/ethernet/microchip/lan743x_main.h
> > +++ b/drivers/net/ethernet/microchip/lan743x_main.h
> > @@ -37,6 +37,8 @@
> >
> >  #define STRAP_READ                   (0x0C)
> >  #define STRAP_READ_USE_SGMII_EN_     BIT(22)
> > +#define STRAP_SFP_USE_EN_              BIT(31)
> > +#define STRAP_SFP_EN_                  BIT(15)
> >  #define STRAP_READ_SGMII_EN_         BIT(6)
> 
> [Low]
> The two new STRAP_SFP_*_ macros use spaces for alignment while every
> surrounding STRAP_* definition uses tabs.  Was this intentional, or should they
> be tab-aligned to match the rest of the file (and avoid a checkpatch warning)?

[Thangaraj Samynathan] Agreed. Will be fixed in next revision.

> 
> >  #define STRAP_READ_SGMII_REFCLK_     BIT(5)
> >  #define STRAP_READ_SGMII_2_5G_               BIT(4)
> > @@ -1081,6 +1083,7 @@ struct lan743x_adapter {
> >       u8                      max_tx_channels;
> >       u8                      used_tx_channels;
> >       u8                      max_vector_count;
> > +     bool                    is_sfp_support_en;
> 
> [Low]
> Same question here: the new is_sfp_support_en field uses spaces for
> alignment while the surrounding struct members use tabs.  Should this be tab-
> aligned for consistency with the rest of struct lan743x_adapter?

 [Thangaraj Samynathan] Agreed. Will be fixed in next revision.

> 
> >
> >  #define LAN743X_ADAPTER_FLAG_OTP             BIT(0)
> >       u32                     flags;
> --
> pw-bot: cr