2023-03-30 09:19:34

by Sit, Michael Wei Hong

[permalink] [raw]
Subject: [PATCH net v5 0/3] Fix PHY handle no longer parsing

After the fixed link support was introduced, it is observed that PHY
no longer attach to the MAC properly. So we introduce a helper
function to determine if the MAC should expect to connect to a PHY
and proceed accordingly.

Michael Sit Wei Hong (3):
net: phylink: add phylink_expects_phy() method
net: stmmac: check if MAC needs to attach to a PHY
net: stmmac: remove redundant fixup to support fixed-link mode

.../net/ethernet/stmicro/stmmac/dwmac-intel.c | 1 -
.../net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
drivers/net/phy/phylink.c | 19 +++++++++++++++++++
include/linux/phylink.h | 1 +
4 files changed, 23 insertions(+), 2 deletions(-)

v2: Initialize fwnode before using the variable
v3: Introduced phylink_expects_phy() method as suggested by Russell King
remove xpcs_an_inband fixup instead of moving the fixed-link check
as suggested by Andrew Lunn
v4: Modify phylink_expects_phy() to check for more complete set of
conditions when no PHY is needed and return false if met.
v5: Enhance phylink_expects_phy() description.
--
2.34.1


2023-03-30 09:19:36

by Sit, Michael Wei Hong

[permalink] [raw]
Subject: [PATCH net v5 1/3] net: phylink: add phylink_expects_phy() method

Provide phylink_expects_phy() to allow MAC drivers to check if it
is expecting a PHY to attach to. Since fixed-linked setups do not
need to attach to a PHY.

Provides a boolean value as to if the MAC should expect a PHY.
Returns true if a PHY is expected.

Reviewed-by: Russell King (Oracle) <[email protected]>
Signed-off-by: Michael Sit Wei Hong <[email protected]>
---
drivers/net/phy/phylink.c | 19 +++++++++++++++++++
include/linux/phylink.h | 1 +
2 files changed, 20 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 1a2f074685fa..30c166b33468 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -1586,6 +1586,25 @@ void phylink_destroy(struct phylink *pl)
}
EXPORT_SYMBOL_GPL(phylink_destroy);

+/**
+ * phylink_expects_phy() - Determine if phylink expects a phy to be attached
+ * @pl: a pointer to a &struct phylink returned from phylink_create()
+ *
+ * When using fixed-link mode, or in-band mode with 1000base-X or 2500base-X,
+ * no PHY is needed.
+ *
+ * Returns true if phylink will be expecting a PHY.
+ */
+bool phylink_expects_phy(struct phylink *pl)
+{
+ if (pl->cfg_link_an_mode == MLO_AN_FIXED ||
+ (pl->cfg_link_an_mode == MLO_AN_INBAND &&
+ phy_interface_mode_is_8023z(pl->link_config.interface)))
+ return false;
+ return true;
+}
+EXPORT_SYMBOL_GPL(phylink_expects_phy);
+
static void phylink_phy_change(struct phy_device *phydev, bool up)
{
struct phylink *pl = phydev->phylink;
diff --git a/include/linux/phylink.h b/include/linux/phylink.h
index c492c26202b5..637698ed5cb6 100644
--- a/include/linux/phylink.h
+++ b/include/linux/phylink.h
@@ -574,6 +574,7 @@ struct phylink *phylink_create(struct phylink_config *, struct fwnode_handle *,
phy_interface_t iface,
const struct phylink_mac_ops *mac_ops);
void phylink_destroy(struct phylink *);
+bool phylink_expects_phy(struct phylink *pl);

int phylink_connect_phy(struct phylink *, struct phy_device *);
int phylink_of_phy_connect(struct phylink *, struct device_node *, u32 flags);
--
2.34.1

2023-03-30 09:19:37

by Sit, Michael Wei Hong

[permalink] [raw]
Subject: [PATCH net v5 2/3] net: stmmac: check if MAC needs to attach to a PHY

After the introduction of the fixed-link support, the MAC driver
no longer attempt to scan for a PHY to attach to. This causes the
non fixed-link setups to stop working.

Using the phylink_expects_phy() to check and determine if the MAC
should expect and attach a PHY.

Fixes: ab21cf920928 ("net: stmmac: make mdio register skips PHY scanning for fixed-link")
Signed-off-by: Michael Sit Wei Hong <[email protected]>
Signed-off-by: Lai Peter Jun Ann <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 8f543c3ab5c5..41f0f3b74933 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1135,6 +1135,7 @@ static int stmmac_init_phy(struct net_device *dev)
{
struct stmmac_priv *priv = netdev_priv(dev);
struct fwnode_handle *fwnode;
+ bool phy_needed;
int ret;

fwnode = of_fwnode_handle(priv->plat->phylink_node);
@@ -1144,10 +1145,11 @@ static int stmmac_init_phy(struct net_device *dev)
if (fwnode)
ret = phylink_fwnode_phy_connect(priv->phylink, fwnode, 0);

+ phy_needed = phylink_expects_phy(priv->phylink);
/* Some DT bindings do not set-up the PHY handle. Let's try to
* manually parse it
*/
- if (!fwnode || ret) {
+ if (!fwnode || phy_needed || ret) {
int addr = priv->plat->phy_addr;
struct phy_device *phydev;

--
2.34.1

2023-03-30 09:20:06

by Sit, Michael Wei Hong

[permalink] [raw]
Subject: [PATCH net v5 3/3] net: stmmac: remove redundant fixup to support fixed-link mode

Currently, intel_speed_mode_2500() will fix-up xpcs_an_inband
to 1 if the underlying controller has a max speed of 1000Mbps.
The value has been initialized and modified if it is
a fixed-linked setup earlier.

This patch removes the fix-up to allow for fixed-linked setup
support. In stmmac_phy_setup(), ovr_an_inband is set based on
the value of xpcs_an_inband. Which in turn will return an
error in phylink_parse_mode() where MLO_AN_FIXED and
ovr_an_inband are both set.

Fixes: c82386310d95 ("stmmac: intel: prepare to support 1000BASE-X phy interface setting")
Signed-off-by: Michael Sit Wei Hong <[email protected]>
---
drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c | 1 -
1 file changed, 1 deletion(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
index 7deb1f817dac..6db87184bf75 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-intel.c
@@ -251,7 +251,6 @@ static void intel_speed_mode_2500(struct net_device *ndev, void *intel_data)
priv->plat->mdio_bus_data->xpcs_an_inband = false;
} else {
priv->plat->max_speed = 1000;
- priv->plat->mdio_bus_data->xpcs_an_inband = true;
}
}

--
2.34.1

2023-03-31 08:35:07

by patchwork-bot+netdevbpf

[permalink] [raw]
Subject: Re: [PATCH net v5 0/3] Fix PHY handle no longer parsing

Hello:

This series was applied to netdev/net.git (main)
by David S. Miller <[email protected]>:

On Thu, 30 Mar 2023 17:14:01 +0800 you wrote:
> After the fixed link support was introduced, it is observed that PHY
> no longer attach to the MAC properly. So we introduce a helper
> function to determine if the MAC should expect to connect to a PHY
> and proceed accordingly.
>
> Michael Sit Wei Hong (3):
> net: phylink: add phylink_expects_phy() method
> net: stmmac: check if MAC needs to attach to a PHY
> net: stmmac: remove redundant fixup to support fixed-link mode
>
> [...]

Here is the summary with links:
- [net,v5,1/3] net: phylink: add phylink_expects_phy() method
https://git.kernel.org/netdev/net/c/653a180957a8
- [net,v5,2/3] net: stmmac: check if MAC needs to attach to a PHY
https://git.kernel.org/netdev/net/c/fe2cfbc96803
- [net,v5,3/3] net: stmmac: remove redundant fixup to support fixed-link mode
https://git.kernel.org/netdev/net/c/6fc21a6ed595

You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html


2023-04-03 21:26:26

by Martin Blumenstingl

[permalink] [raw]
Subject: RE [PATCH net v5 2/3] net: stmmac: check if MAC needs to attach to a PHY

Hello,

[...]
> @@ -1144,10 +1145,11 @@ static int stmmac_init_phy(struct net_device *dev)
> if (fwnode)
> ret = phylink_fwnode_phy_connect(priv->phylink, fwnode, 0);
>
> + phy_needed = phylink_expects_phy(priv->phylink);
> /* Some DT bindings do not set-up the PHY handle. Let's try to
> * manually parse it
> */
> - if (!fwnode || ret) {
> + if (!fwnode || phy_needed || ret) {
Unfortunately this breaks Ethernet on my X96 Air board (the .dts file
can be found upstream in:
arch/arm64/boot/dts/amlogic/meson-sm1-x96-air-gbit.dts)

Working boot-log:
# dmesg | grep dwmac
[ 3.699961] meson8b-dwmac ff3f0000.ethernet: IRQ eth_wake_irq not found
[ 3.700944] meson8b-dwmac ff3f0000.ethernet: IRQ eth_lpi not found
[ 3.707196] meson8b-dwmac ff3f0000.ethernet: PTP uses main clock
[ 3.713688] meson8b-dwmac ff3f0000.ethernet: User ID: 0x11, Synopsys ID: 0x37
[ 3.720201] meson8b-dwmac ff3f0000.ethernet: DWMAC1000
[ 3.725387] meson8b-dwmac ff3f0000.ethernet: DMA HW capability register supported
[ 3.732832] meson8b-dwmac ff3f0000.ethernet: RX Checksum Offload Engine supported
[ 3.740301] meson8b-dwmac ff3f0000.ethernet: COE Type 2
[ 3.745491] meson8b-dwmac ff3f0000.ethernet: TX Checksum insertion supported
[ 3.752504] meson8b-dwmac ff3f0000.ethernet: Wake-Up On Lan supported
[ 3.758993] meson8b-dwmac ff3f0000.ethernet: Normal descriptors
[ 3.764813] meson8b-dwmac ff3f0000.ethernet: Ring mode enabled
[ 3.770629] meson8b-dwmac ff3f0000.ethernet: Enable RX Mitigation via HW Watchdog Timer
[ 13.565781] meson8b-dwmac ff3f0000.ethernet end0: renamed from eth0
[ 14.036061] meson8b-dwmac ff3f0000.ethernet end0: Register MEM_TYPE_PAGE_POOL RxQ-0
[ 14.255617] meson8b-dwmac ff3f0000.ethernet end0: PHY [mdio_mux-0.0:00] driver [RTL8211F Gigabit Ethernet] (irq=33)
[ 14.265404] meson8b-dwmac ff3f0000.ethernet end0: No Safety Features support found
[ 14.267977] meson8b-dwmac ff3f0000.ethernet end0: PTP not supported by HW
[ 14.275723] meson8b-dwmac ff3f0000.ethernet end0: configuring for phy/rgmii-txid link mode
[ 17.394262] meson8b-dwmac ff3f0000.ethernet end0: Link is Up - 1Gbps/Full - flow control rx/tx

Non-working boot-log:
# dmesg | grep dwmac
[ 3.730072] meson8b-dwmac ff3f0000.ethernet: IRQ eth_wake_irq not found
[ 3.731053] meson8b-dwmac ff3f0000.ethernet: IRQ eth_lpi not found
[ 3.737303] meson8b-dwmac ff3f0000.ethernet: PTP uses main clock
[ 3.743795] meson8b-dwmac ff3f0000.ethernet: User ID: 0x11, Synopsys ID: 0x37
[ 3.750311] meson8b-dwmac ff3f0000.ethernet: DWMAC1000
[ 3.755498] meson8b-dwmac ff3f0000.ethernet: DMA HW capability register supported
[ 3.762944] meson8b-dwmac ff3f0000.ethernet: RX Checksum Offload Engine supported
[ 3.770412] meson8b-dwmac ff3f0000.ethernet: COE Type 2
[ 3.775603] meson8b-dwmac ff3f0000.ethernet: TX Checksum insertion supported
[ 3.782615] meson8b-dwmac ff3f0000.ethernet: Wake-Up On Lan supported
[ 3.789106] meson8b-dwmac ff3f0000.ethernet: Normal descriptors
[ 3.794924] meson8b-dwmac ff3f0000.ethernet: Ring mode enabled
[ 3.800738] meson8b-dwmac ff3f0000.ethernet: Enable RX Mitigation via HW Watchdog Timer
[ 13.052942] meson8b-dwmac ff3f0000.ethernet end0: renamed from eth0
[ 13.594285] meson8b-dwmac ff3f0000.ethernet end0: Register MEM_TYPE_PAGE_POOL RxQ-0
[ 13.825578] meson8b-dwmac ff3f0000.ethernet end0: PHY [mdio_mux-0.0:00] driver [RTL8211F Gigabit Ethernet] (irq=33)
[ 13.831358] meson8b-dwmac ff3f0000.ethernet end0: no phy found
[ 13.836229] meson8b-dwmac ff3f0000.ethernet end0: __stmmac_open: Cannot attach to PHY (error: -19)

Reverting this patch fixes that problem.

I think the reason is a logic error in the patch:
As you can see the PHY is found and attached (my understanding is
that this happens through phylink_fwnode_phy_connect()). But we now
also go to that if block below even fwnode != NULL && ret == 0 (which
indicates that phylink_fwnode_phy_connect() was successful). Inside
that if block priv->plat->phy_addr then has the default value (-1)
that was set in stmmac_probe_config_dt().

I am running out of time for today. Could you please look into this
and follow up with a patch (on top of this one, as this one has
already been applied) that considers your original issues as well as
the case of my board (I suspect that all Amlogic boards that are
supported upstream are affected)? Please keep me Cc'ed so I can test
your additional patch and then add my Tested-by.


Thank you!
Martin

2023-04-05 08:56:05

by Shahab Vahedi

[permalink] [raw]
Subject: Re: [PATCH net v5 2/3] net: stmmac: check if MAC needs to attach to a PHY

Same happens on ARC HSDK [1]:

# dmesg | grep stmmaceth
stmmaceth f0008000.ethernet: use coherent DMA ops
stmmaceth f0008000.ethernet: IRQ eth_wake_irq not found
stmmaceth f0008000.ethernet: IRQ eth_lpi not found
stmmaceth f0008000.ethernet: PTP uses main clock
stmmaceth f0008000.ethernet: User ID: 0x10, Synopsys ID: 0x37
stmmaceth f0008000.ethernet: DWMAC1000
stmmaceth f0008000.ethernet: DMA HW capability register supported
stmmaceth f0008000.ethernet: RX Checksum Offload Engine supported
stmmaceth f0008000.ethernet: COE Type 2
stmmaceth f0008000.ethernet: TX Checksum insertion supported
stmmaceth f0008000.ethernet: Normal descriptors
stmmaceth f0008000.ethernet: Ring mode enabled
stmmaceth f0008000.ethernet: Enable RX Mitigation via HW Watchdog Timer
stmmaceth f0008000.ethernet: device MAC address 7e:14:df:5f:b8:78
stmmaceth f0008000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
stmmaceth f0008000.ethernet eth0: PHY [stmmac-0:00] driver [Micrel KSZ9031 Gigabit PHY] (irq=POLL)
stmmaceth f0008000.ethernet eth0: no phy found
stmmaceth f0008000.ethernet eth0: __stmmac_open: Cannot attach to PHY (error: -19)

After reverting this patch:

# dmesg | grep stmmaceth
stmmaceth f0008000.ethernet: use coherent DMA ops
stmmaceth f0008000.ethernet: IRQ eth_wake_irq not found
stmmaceth f0008000.ethernet: IRQ eth_lpi not found
stmmaceth f0008000.ethernet: PTP uses main clock
stmmaceth f0008000.ethernet: User ID: 0x10, Synopsys ID: 0x37
stmmaceth f0008000.ethernet: DWMAC1000
stmmaceth f0008000.ethernet: DMA HW capability register supported
stmmaceth f0008000.ethernet: RX Checksum Offload Engine supported
stmmaceth f0008000.ethernet: COE Type 2
stmmaceth f0008000.ethernet: TX Checksum insertion supported
stmmaceth f0008000.ethernet: Normal descriptors
stmmaceth f0008000.ethernet: Ring mode enabled
stmmaceth f0008000.ethernet: Enable RX Mitigation via HW Watchdog Timer
stmmaceth f0008000.ethernet: device MAC address 26:05:ea:c0:66:16
stmmaceth f0008000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
stmmaceth f0008000.ethernet eth0: PHY [stmmac-0:00] driver [Micrel KSZ9031
Gigabit PHY] (irq=POLL)
stmmaceth f0008000.ethernet eth0: No Safety Features support found
stmmaceth f0008000.ethernet eth0: PTP not supported by HW
stmmaceth f0008000.ethernet eth0: configuring for phy/rgmii-id link mode
stmmaceth f0008000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off

[1]
arch/arc/boot/dts/hsdk.dts

--
Shahab

2023-04-05 09:21:26

by Shahab Vahedi

[permalink] [raw]
Subject: Re: [PATCH net v5 2/3] net: stmmac: check if MAC needs to attach to a PHY

Fixing the CC header. Apologies for the inconvenience.

------

Same happens on ARC HSDK [1]:

# dmesg | grep stmmaceth
stmmaceth f0008000.ethernet: use coherent DMA ops
stmmaceth f0008000.ethernet: IRQ eth_wake_irq not found
stmmaceth f0008000.ethernet: IRQ eth_lpi not found
stmmaceth f0008000.ethernet: PTP uses main clock
stmmaceth f0008000.ethernet: User ID: 0x10, Synopsys ID: 0x37
stmmaceth f0008000.ethernet: DWMAC1000
stmmaceth f0008000.ethernet: DMA HW capability register supported
stmmaceth f0008000.ethernet: RX Checksum Offload Engine supported
stmmaceth f0008000.ethernet: COE Type 2
stmmaceth f0008000.ethernet: TX Checksum insertion supported
stmmaceth f0008000.ethernet: Normal descriptors
stmmaceth f0008000.ethernet: Ring mode enabled
stmmaceth f0008000.ethernet: Enable RX Mitigation via HW Watchdog Timer
stmmaceth f0008000.ethernet: device MAC address 7e:14:df:5f:b8:78
stmmaceth f0008000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
stmmaceth f0008000.ethernet eth0: PHY [stmmac-0:00] driver [Micrel KSZ9031 Gigabit PHY] (irq=POLL)
stmmaceth f0008000.ethernet eth0: no phy found
stmmaceth f0008000.ethernet eth0: __stmmac_open: Cannot attach to PHY (error: -19)

After reverting this patch:

# dmesg | grep stmmaceth
stmmaceth f0008000.ethernet: use coherent DMA ops
stmmaceth f0008000.ethernet: IRQ eth_wake_irq not found
stmmaceth f0008000.ethernet: IRQ eth_lpi not found
stmmaceth f0008000.ethernet: PTP uses main clock
stmmaceth f0008000.ethernet: User ID: 0x10, Synopsys ID: 0x37
stmmaceth f0008000.ethernet: DWMAC1000
stmmaceth f0008000.ethernet: DMA HW capability register supported
stmmaceth f0008000.ethernet: RX Checksum Offload Engine supported
stmmaceth f0008000.ethernet: COE Type 2
stmmaceth f0008000.ethernet: TX Checksum insertion supported
stmmaceth f0008000.ethernet: Normal descriptors
stmmaceth f0008000.ethernet: Ring mode enabled
stmmaceth f0008000.ethernet: Enable RX Mitigation via HW Watchdog Timer
stmmaceth f0008000.ethernet: device MAC address 26:05:ea:c0:66:16
stmmaceth f0008000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
stmmaceth f0008000.ethernet eth0: PHY [stmmac-0:00] driver [Micrel KSZ9031
Gigabit PHY] (irq=POLL)
stmmaceth f0008000.ethernet eth0: No Safety Features support found
stmmaceth f0008000.ethernet eth0: PTP not supported by HW
stmmaceth f0008000.ethernet eth0: configuring for phy/rgmii-id link mode
stmmaceth f0008000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off

[1]
arch/arc/boot/dts/hsdk.dts

--
Shahab

2023-04-12 06:47:15

by Maxime Chevallier

[permalink] [raw]
Subject: Re: [PATCH net v5 1/3] net: phylink: add phylink_expects_phy() method

Hello everyone,

On Thu, 30 Mar 2023 17:14:02 +0800
Michael Sit Wei Hong <[email protected]> wrote:

> Provide phylink_expects_phy() to allow MAC drivers to check if it
> is expecting a PHY to attach to. Since fixed-linked setups do not
> need to attach to a PHY.
>
> Provides a boolean value as to if the MAC should expect a PHY.
> Returns true if a PHY is expected.

I'm currently working on the TSE rework for dwmac_socfpga, and I
noticed one regression since this patch, when using an SFP, see details
below :

> Reviewed-by: Russell King (Oracle) <[email protected]>
> Signed-off-by: Michael Sit Wei Hong <[email protected]>
> ---
> drivers/net/phy/phylink.c | 19 +++++++++++++++++++
> include/linux/phylink.h | 1 +
> 2 files changed, 20 insertions(+)
>
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 1a2f074685fa..30c166b33468 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -1586,6 +1586,25 @@ void phylink_destroy(struct phylink *pl)
> }
> EXPORT_SYMBOL_GPL(phylink_destroy);
>
> +/**
> + * phylink_expects_phy() - Determine if phylink expects a phy to be
> attached
> + * @pl: a pointer to a &struct phylink returned from phylink_create()
> + *
> + * When using fixed-link mode, or in-band mode with 1000base-X or
> 2500base-X,
> + * no PHY is needed.
> + *
> + * Returns true if phylink will be expecting a PHY.
> + */
> +bool phylink_expects_phy(struct phylink *pl)
> +{
> + if (pl->cfg_link_an_mode == MLO_AN_FIXED ||
> + (pl->cfg_link_an_mode == MLO_AN_INBAND &&
> + phy_interface_mode_is_8023z(pl->link_config.interface)))

From the discussion, at one point Russell mentionned [1] :
"If there's a sfp bus, then we don't expect a PHY from the MAC driver
(as there can only be one PHY attached), and as phylink_expects_phy()
is for the MAC driver to use, we should be returning false if
pl->sfp_bus != NULL."

This makes sense and indeed adding the relevant check solves the issue.

Am I correct in assuming this was an unintentional omission from this
patch, or was the pl->sfp_bus check dropped on purpose ?

> + return false;
> + return true;
> +}
> +EXPORT_SYMBOL_GPL(phylink_expects_phy);

Thanks,

Maxime

[1] :
https://lore.kernel.org/netdev/[email protected]/

2023-04-12 07:16:26

by Sit, Michael Wei Hong

[permalink] [raw]
Subject: RE: [PATCH net v5 1/3] net: phylink: add phylink_expects_phy() method



> -----Original Message-----
> From: Maxime Chevallier <[email protected]>
> Sent: Wednesday, April 12, 2023 4:38 PM
> To: Sit, Michael Wei Hong <[email protected]>
> Cc: Giuseppe Cavallaro <[email protected]>; Alexandre
> Torgue <[email protected]>; Jose Abreu
> <[email protected]>; David S . Miller <[email protected]>;
> Eric Dumazet <[email protected]>; Jakub Kicinski
> <[email protected]>; Paolo Abeni <[email protected]>; Maxime
> Coquelin <[email protected]>; Ong, Boon Leong
> <[email protected]>; [email protected]; linux-
> [email protected]; linux-arm-
> [email protected]; [email protected];
> [email protected]; [email protected]; [email protected];
> Looi, Hong Aun <[email protected]>; Voon, Weifeng
> <[email protected]>; Lai, Peter Jun Ann
> <[email protected]>; [email protected]
> Subject: Re: [PATCH net v5 1/3] net: phylink: add
> phylink_expects_phy() method
>
> Hello everyone,
>
> On Thu, 30 Mar 2023 17:14:02 +0800
> Michael Sit Wei Hong <[email protected]> wrote:
>
> > Provide phylink_expects_phy() to allow MAC drivers to check if it is
> > expecting a PHY to attach to. Since fixed-linked setups do not need
> to
> > attach to a PHY.
> >
> > Provides a boolean value as to if the MAC should expect a PHY.
> > Returns true if a PHY is expected.
>
> I'm currently working on the TSE rework for dwmac_socfpga, and I
> noticed one regression since this patch, when using an SFP, see
> details below :
>
> > Reviewed-by: Russell King (Oracle) <[email protected]>
> > Signed-off-by: Michael Sit Wei Hong
> <[email protected]>
> > ---
> > drivers/net/phy/phylink.c | 19 +++++++++++++++++++
> > include/linux/phylink.h | 1 +
> > 2 files changed, 20 insertions(+)
> >
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 1a2f074685fa..30c166b33468 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -1586,6 +1586,25 @@ void phylink_destroy(struct phylink
> *pl) }
> > EXPORT_SYMBOL_GPL(phylink_destroy);
> >
> > +/**
> > + * phylink_expects_phy() - Determine if phylink expects a phy to
> be
> > attached
> > + * @pl: a pointer to a &struct phylink returned from
> phylink_create()
> > + *
> > + * When using fixed-link mode, or in-band mode with 1000base-X
> or
> > 2500base-X,
> > + * no PHY is needed.
> > + *
> > + * Returns true if phylink will be expecting a PHY.
> > + */
> > +bool phylink_expects_phy(struct phylink *pl) {
> > + if (pl->cfg_link_an_mode == MLO_AN_FIXED ||
> > + (pl->cfg_link_an_mode == MLO_AN_INBAND &&
> > + phy_interface_mode_is_8023z(pl->link_config.interface)))
>
> From the discussion, at one point Russell mentionned [1] :
> "If there's a sfp bus, then we don't expect a PHY from the MAC
> driver (as there can only be one PHY attached), and as
> phylink_expects_phy() is for the MAC driver to use, we should be
> returning false if
> pl->sfp_bus != NULL."
>
> This makes sense and indeed adding the relevant check solves the
> issue.
>
> Am I correct in assuming this was an unintentional omission from
> this patch, or was the pl->sfp_bus check dropped on purpose ?
>
> > + return false;
> > + return true;
> > +}
> > +EXPORT_SYMBOL_GPL(phylink_expects_phy);
>
> Thanks,
>
> Maxime
>

Russell also did mention:
" The reason for the extra "&& !pl->sfp_bus" in phylink_attach_phy()
is to allow SFPs to connect to the MAC using inband mode with
1000base-X and 2500base-X interface modes. These are not for the
MAC-side of things though."

So I thought that the check can be dropped. I do not have any SFP hardware
to test, if adding the check make sense, you can send us a patch so we can
test it out.
> [1] :
> https://lore.kernel.org/netdev/[email protected]
> rg.uk/