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

[Jakub Kicinski <kuba@kernel.org>]

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.

>  	} 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?

> +
> +	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?

> +
>  	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?

> 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)?

>  #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?

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