diff mbox series

[RFC,v2,net-next,9/9] net: pcs: xpcs: convert to phylink_pcs_ops

Message ID 20210601003325.1631980-10-olteanv@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series Convert xpcs to phylink_pcs_ops | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 2 maintainers not CCed: linux-arm-kernel@lists.infradead.org linux-stm32@st-md-mailman.stormreply.com
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 22 this patch: 22
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 378 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 22 this patch: 22
netdev/header_inline success Link

Commit Message

Vladimir Oltean June 1, 2021, 12:33 a.m. UTC 
From: Vladimir Oltean <vladimir.oltean@nxp.com>

Since all the remaining members of struct mdio_xpcs_ops have direct
equivalents in struct phylink_pcs_ops, it is about time we remove it
altogether.

Since the phylink ops return void, we need to remove the error
propagation from the various xpcs methods and simply print an error
message where appropriate.

Since xpcs_get_state_c73() detects link faults and attempts to reset the
link on its own by calling xpcs_config(), but xpcs_config() now has a
lot of phylink arguments which are not needed and cannot be simply
fabricated by anybody else except phylink, the actual implementation has
been moved into a smaller xpcs_do_config().

The const struct mdio_xpcs_ops *priv->hw->xpcs has been removed, so we
need to look at the struct mdio_xpcs_args pointer now as an indication
whether the port has an XPCS or not.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/common.h  |  3 +-
 drivers/net/ethernet/stmicro/stmmac/hwif.h    |  8 --
 .../ethernet/stmicro/stmmac/stmmac_ethtool.c  |  2 +-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 41 ++------
 .../net/ethernet/stmicro/stmmac/stmmac_mdio.c | 16 +--
 drivers/net/pcs/pcs-xpcs.c                    | 99 +++++++++++--------
 include/linux/pcs/pcs-xpcs.h                  | 11 +--
 7 files changed, 77 insertions(+), 103 deletions(-)

Comments

Russell King (Oracle) June 1, 2021, 12:10 p.m. UTC | #1 
On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
>  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
>  	.validate = stmmac_validate,
> -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> -	.mac_config = stmmac_mac_config,

mac_config is still a required function.
Vladimir Oltean June 2, 2021, 1:43 p.m. UTC | #2 
On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> >  	.validate = stmmac_validate,
> > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > -	.mac_config = stmmac_mac_config,
> 
> mac_config is still a required function.

This is correct, thanks.

VK, would you mind testing again with this extra patch added to the mix?
If it works, I will add it to the series in v3, ordered properly.

-----------------------------[ cut here]-----------------------------
>From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Wed, 2 Jun 2021 16:35:55 +0300
Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
 pcs_ops are provided

The pcs_config method from struct phylink_pcs_ops does everything that
the mac_config method from struct phylink_mac_ops used to do in the
legacy approach of driving a MAC PCS. So allow drivers to not implement
the mac_config method if there is nothing to do. Keep the method
required for setups that do not provide pcs_ops.

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/phy/phylink.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 96d8e88b4e46..a8842c6ce3a2 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
 static void phylink_mac_config(struct phylink *pl,
 			       const struct phylink_link_state *state)
 {
+	if (!pl->mac_ops->mac_config)
+		return;
+
 	phylink_dbg(pl,
 		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
 		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
@@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
 
 	ASSERT_RTNL();
 
+	/* The mac_ops::mac_config method may be absent only if the
+	 * pcs_ops are present.
+	 */
+	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
+		return;
+
 	phylink_info(pl, "configuring for %s/%s link mode\n",
 		     phylink_an_mode_str(pl->cur_link_an_mode),
 		     phy_modes(pl->link_config.interface));
-----------------------------[ cut here]-----------------------------
Russell King (Oracle) June 2, 2021, 1:47 p.m. UTC | #3 
On Wed, Jun 02, 2021 at 04:43:21PM +0300, Vladimir Oltean wrote:
> On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> > On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > >  	.validate = stmmac_validate,
> > > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > > -	.mac_config = stmmac_mac_config,
> > 
> > mac_config is still a required function.
> 
> This is correct, thanks.
> 
> VK, would you mind testing again with this extra patch added to the mix?
> If it works, I will add it to the series in v3, ordered properly.
> 
> -----------------------------[ cut here]-----------------------------
> From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Wed, 2 Jun 2021 16:35:55 +0300
> Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
>  pcs_ops are provided
> 
> The pcs_config method from struct phylink_pcs_ops does everything that
> the mac_config method from struct phylink_mac_ops used to do in the
> legacy approach of driving a MAC PCS. So allow drivers to not implement
> the mac_config method if there is nothing to do. Keep the method
> required for setups that do not provide pcs_ops.
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/phy/phylink.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 96d8e88b4e46..a8842c6ce3a2 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
>  static void phylink_mac_config(struct phylink *pl,
>  			       const struct phylink_link_state *state)
>  {
> +	if (!pl->mac_ops->mac_config)
> +		return;
> +
>  	phylink_dbg(pl,
>  		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
>  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
> @@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
>  
>  	ASSERT_RTNL();
>  
> +	/* The mac_ops::mac_config method may be absent only if the
> +	 * pcs_ops are present.
> +	 */
> +	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
> +		return;
> +
>  	phylink_info(pl, "configuring for %s/%s link mode\n",
>  		     phylink_an_mode_str(pl->cur_link_an_mode),
>  		     phy_modes(pl->link_config.interface));

I would rather we didn't do that - I suspect your case is not the
common scenario, so please add a dummy function to stmmac instead.
Vladimir Oltean June 2, 2021, 2:02 p.m. UTC | #4 
On Wed, Jun 02, 2021 at 02:47:49PM +0100, Russell King (Oracle) wrote:
> On Wed, Jun 02, 2021 at 04:43:21PM +0300, Vladimir Oltean wrote:
> > On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> > > On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> > > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > > >  	.validate = stmmac_validate,
> > > > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > > > -	.mac_config = stmmac_mac_config,
> > > 
> > > mac_config is still a required function.
> > 
> > This is correct, thanks.
> > 
> > VK, would you mind testing again with this extra patch added to the mix?
> > If it works, I will add it to the series in v3, ordered properly.
> > 
> > -----------------------------[ cut here]-----------------------------
> > From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > Date: Wed, 2 Jun 2021 16:35:55 +0300
> > Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
> >  pcs_ops are provided
> > 
> > The pcs_config method from struct phylink_pcs_ops does everything that
> > the mac_config method from struct phylink_mac_ops used to do in the
> > legacy approach of driving a MAC PCS. So allow drivers to not implement
> > the mac_config method if there is nothing to do. Keep the method
> > required for setups that do not provide pcs_ops.
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  drivers/net/phy/phylink.c | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> > 
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 96d8e88b4e46..a8842c6ce3a2 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
> >  static void phylink_mac_config(struct phylink *pl,
> >  			       const struct phylink_link_state *state)
> >  {
> > +	if (!pl->mac_ops->mac_config)
> > +		return;
> > +
> >  	phylink_dbg(pl,
> >  		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
> >  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
> > @@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
> >  
> >  	ASSERT_RTNL();
> >  
> > +	/* The mac_ops::mac_config method may be absent only if the
> > +	 * pcs_ops are present.
> > +	 */
> > +	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
> > +		return;
> > +
> >  	phylink_info(pl, "configuring for %s/%s link mode\n",
> >  		     phylink_an_mode_str(pl->cur_link_an_mode),
> >  		     phy_modes(pl->link_config.interface));
> 
> I would rather we didn't do that - I suspect your case is not the
> common scenario, so please add a dummy function to stmmac instead.

Ok, in that case the only delta to be applied should be:

-----------------------------[ cut here]-----------------------------
>From 998569108392befc591f790e46fe5dcd1d0b6278 Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Wed, 2 Jun 2021 17:00:12 +0300
Subject: [PATCH] fixup! net: pcs: xpcs: convert to phylink_pcs_ops

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index d5685a74f3b7..704aa91b145a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1000,6 +1000,12 @@ static void stmmac_validate(struct phylink_config *config,
 		xpcs_validate(priv->hw->xpcs, supported, state);
 }
 
+static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
+			      const struct phylink_link_state *state)
+{
+	/* Nothing to do, xpcs_config() handles everything */
+}
+
 static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
 {
 	struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
-----------------------------[ cut here]-----------------------------
Wong Vee Khee June 2, 2021, 2:43 p.m. UTC | #5 
On Wed, Jun 02, 2021 at 05:02:33PM +0300, Vladimir Oltean wrote:
> On Wed, Jun 02, 2021 at 02:47:49PM +0100, Russell King (Oracle) wrote:
> > On Wed, Jun 02, 2021 at 04:43:21PM +0300, Vladimir Oltean wrote:
> > > On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> > > > On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> > > > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > > > >  	.validate = stmmac_validate,
> > > > > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > > > > -	.mac_config = stmmac_mac_config,
> > > > 
> > > > mac_config is still a required function.
> > > 
> > > This is correct, thanks.
> > > 
> > > VK, would you mind testing again with this extra patch added to the mix?
> > > If it works, I will add it to the series in v3, ordered properly.
> > > 
> > > -----------------------------[ cut here]-----------------------------
> > > From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
> > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > Date: Wed, 2 Jun 2021 16:35:55 +0300
> > > Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
> > >  pcs_ops are provided
> > > 
> > > The pcs_config method from struct phylink_pcs_ops does everything that
> > > the mac_config method from struct phylink_mac_ops used to do in the
> > > legacy approach of driving a MAC PCS. So allow drivers to not implement
> > > the mac_config method if there is nothing to do. Keep the method
> > > required for setups that do not provide pcs_ops.
> > > 
> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > ---
> > >  drivers/net/phy/phylink.c | 9 +++++++++
> > >  1 file changed, 9 insertions(+)
> > > 
> > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > index 96d8e88b4e46..a8842c6ce3a2 100644
> > > --- a/drivers/net/phy/phylink.c
> > > +++ b/drivers/net/phy/phylink.c
> > > @@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
> > >  static void phylink_mac_config(struct phylink *pl,
> > >  			       const struct phylink_link_state *state)
> > >  {
> > > +	if (!pl->mac_ops->mac_config)
> > > +		return;
> > > +
> > >  	phylink_dbg(pl,
> > >  		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
> > >  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
> > > @@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
> > >  
> > >  	ASSERT_RTNL();
> > >  
> > > +	/* The mac_ops::mac_config method may be absent only if the
> > > +	 * pcs_ops are present.
> > > +	 */
> > > +	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
> > > +		return;
> > > +
> > >  	phylink_info(pl, "configuring for %s/%s link mode\n",
> > >  		     phylink_an_mode_str(pl->cur_link_an_mode),
> > >  		     phy_modes(pl->link_config.interface));
> > 
> > I would rather we didn't do that - I suspect your case is not the
> > common scenario, so please add a dummy function to stmmac instead.
> 
> Ok, in that case the only delta to be applied should be:
> 
> -----------------------------[ cut here]-----------------------------
> >From 998569108392befc591f790e46fe5dcd1d0b6278 Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Wed, 2 Jun 2021 17:00:12 +0300
> Subject: [PATCH] fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index d5685a74f3b7..704aa91b145a 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1000,6 +1000,12 @@ static void stmmac_validate(struct phylink_config *config,
>  		xpcs_validate(priv->hw->xpcs, supported, state);
>  }
>  
> +static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
> +			      const struct phylink_link_state *state)
> +{
> +	/* Nothing to do, xpcs_config() handles everything */
> +}
> +
>  static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
>  {
>  	struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
> -----------------------------[ cut here]-----------------------------

No luck.. I am still seeing the same kernel panic on my side with the
additional patch applied:

[   12.513652] intel-eth-pci 0000:00:1e.4 enp0s30f4: FPE workqueue start
[   12.520079] intel-eth-pci 0000:00:1e.4 enp0s30f4: configuring for inband/sgmii link mode
[   12.528141] BUG: kernel NULL pointer dereference, address: 0000000000000000
[   12.535072] #PF: supervisor instruction fetch in kernel mode
[   12.540719] #PF: error_code(0x0010) - not-present page
[   12.545843] PGD 0 P4D 0
[   12.548382] Oops: 0010 [#1] PREEMPT SMP NOPTI
[   12.552733] CPU: 3 PID: 2657 Comm: connmand Tainted: G     U            5.13.0-rc3-intel-lts #79
[   12.561485] Hardware name: Intel Corporation Tiger Lake Client Platform/TigerLake U DDR4 SODIMM RVP, BIOS TGLIFUI1.R00.3373.AF0.2009230546 09/23/2020
[   12.574803] RIP: 0010:0x0
[   12.577436] Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6.
[   12.584280] RSP: 0018:ffffaa05404efb78 EFLAGS: 00010246
[   12.589489] RAX: 0000000000000000 RBX: ffffa0fb91758c00 RCX: 0000000000000000
[   12.596600] RDX: ffffaa05404efba0 RSI: 0000000000000002 RDI: ffffa0facc63c4d8
[   12.603711] RBP: ffffaa05404efba0 R08: 0000000000000000 R09: ffffaa05404ef888
[   12.610825] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
[   12.617935] R13: ffffa0facc6388c0 R14: 0000000000000006 R15: 0000000000000001
[   12.625040] FS:  00007f4a6d5b87c0(0000) GS:ffffa0fc57f80000(0000) knlGS:0000000000000000
[   12.633096] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   12.638825] CR2: ffffffffffffffd6 CR3: 000000010e3f6004 CR4: 0000000000770ee0
[   12.645932] PKRU: 55555554
[   12.648641] Call Trace:
[   12.651095]  phylink_major_config+0x5e/0x1a0 [phylink]
[   12.656219]  phylink_start+0x204/0x2c0 [phylink]
[   12.660833]  stmmac_open+0x3d0/0x9f0 [stmmac]
[   12.665186]  __dev_open+0xe7/0x180
[   12.668594]  __dev_change_flags+0x174/0x1d0
[   12.672773]  ? __thaw_task+0x40/0x40
[   12.676348]  ? arch_stack_walk+0x9e/0xf0
[   12.680266]  dev_change_flags+0x21/0x60
[   12.684100]  devinet_ioctl+0x5e8/0x750
[   12.687847]  inet_ioctl+0x190/0x1c0
[   12.691335]  ? dev_ioctl+0x26d/0x4c0
[   12.694907]  sock_do_ioctl+0x44/0x140
[   12.698569]  ? alloc_empty_file+0x61/0xb0
[   12.702572]  sock_ioctl+0x22c/0x320
[   12.706061]  __x64_sys_ioctl+0x80/0xb0
[   12.709808]  do_syscall_64+0x42/0x80
[   12.713381]  entry_SYSCALL_64_after_hwframe+0x44/0xae
[   12.718425] RIP: 0033:0x7f4a6db054bb
[   12.721995] Code: 0f 1e fa 48 8b 05 c5 69 0c 00 64 c7 00 26 00 00 00 48 c7 c0 ff ff ff ff c3 66 0f 1f 44 00 00 f3 0f 1e fa b8 10 00 00 00 0f 05 <48> 3d 01 f0 ff ff 73 01 c3 48 8b 0d 95 69 0c 00 f7 d8 64 89 01 48
[   12.740656] RSP: 002b:00007ffdeab00da8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
[   12.748194] RAX: ffffffffffffffda RBX: 0000562741681100 RCX: 00007f4a6db054bb
[   12.755300] RDX: 00007ffdeab00db0 RSI: 0000000000008914 RDI: 0000000000000010
[   12.762405] RBP: 0000000000000010 R08: 0000562741681100 R09: 0000000000000000
[   12.769510] R10: 000056274162e010 R11: 0000000000000246 R12: 0000000000000000
[   12.776613] R13: 00007ffdeab00db0 R14: 0000000000000001 R15: 00007ffdeab014f0
[   12.783721] Modules linked in: bluetooth ecryptfs hid_sensor_incl_3d hid_sensor_magn_3d hid_sensor_gyro_3d hid_sensor_als hid_sensor_accel_3d hid_sensor_trigger hid_sensor_iio_common hid_sensor_custom hid_sensor_hub intel_ishtp_loader intel_ishtp_hid mxl_gpy x86_pkg_temp_thermal ax88179_178a dwmac_intel usbnet mii stmmac dwc3 atkbd kvm_intel pcs_xpcs mei_wdt phylink mei_hdcp udc_core kvm libps2 libphy mei_me i915 irqbypass spi_pxa2xx_platform intel_rapl_msr pcspkr dw_dmac wdat_wdt i2c_i801 dw_dmac_core i2c_smbus intel_ish_ipc mei intel_ishtp dwc3_pci thermal tpm_crb tpm_tis tpm_tis_core intel_pmc_core parport_pc i8042 parport tpm sch_fq_codel uhid fuse configfs snd_hda_intel snd_intel_dspcfg snd_intel_sdw_acpi snd_hda_codec snd_hda_core snd_pcm snd_timer snd soundcore
[   12.851290] CR2: 0000000000000000
[   12.854603] ---[ end trace 52b9ca69be06d1e3 ]---


My git log:-
dc25bc46886e (HEAD -> master) phy: maxlinear: add Maxlinear GPY115/21x/24x driver
d059f32fcef1 fixup! net: pcs: xpcs: convert to phylink_pcs_ops
1a1a9c7267c1 net: pcs: xpcs: convert to phylink_pcs_ops
ec25f3c80108 net: pcs: xpcs: convert to mdio_device
912a23a5768a net: pcs: xpcs: use mdiobus_c45_addr in xpcs_{read,write}
af2bd0a4e32c net: pcs: xpcs: export xpcs_probe
7e4eea51132f net: pcs: xpcs: export xpcs_config_eee
37d5e086fc83 net: pcs: xpcs: export xpcs_validate
d58487edb1a8 net: pcs: xpcs: make the checks related to the PHY interface mode stateless
2f0eba94af8d net: pcs: xpcs: there is only one PHY ID
67148dd06896 net: pcs: xpcs: delete shim definition for mdio_xpcs_get_ops()
5fe8e519e44f (origin/master, origin/HEAD) Merge git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next

HTH,
 VK
Vladimir Oltean June 2, 2021, 2:57 p.m. UTC | #6 
On Wed, Jun 02, 2021 at 10:43:36PM +0800, Wong Vee Khee wrote:
> On Wed, Jun 02, 2021 at 05:02:33PM +0300, Vladimir Oltean wrote:
> > On Wed, Jun 02, 2021 at 02:47:49PM +0100, Russell King (Oracle) wrote:
> > > On Wed, Jun 02, 2021 at 04:43:21PM +0300, Vladimir Oltean wrote:
> > > > On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> > > > > On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> > > > > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > > > > >  	.validate = stmmac_validate,
> > > > > > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > > > > > -	.mac_config = stmmac_mac_config,
> > > > > 
> > > > > mac_config is still a required function.
> > > > 
> > > > This is correct, thanks.
> > > > 
> > > > VK, would you mind testing again with this extra patch added to the mix?
> > > > If it works, I will add it to the series in v3, ordered properly.
> > > > 
> > > > -----------------------------[ cut here]-----------------------------
> > > > From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
> > > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > Date: Wed, 2 Jun 2021 16:35:55 +0300
> > > > Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
> > > >  pcs_ops are provided
> > > > 
> > > > The pcs_config method from struct phylink_pcs_ops does everything that
> > > > the mac_config method from struct phylink_mac_ops used to do in the
> > > > legacy approach of driving a MAC PCS. So allow drivers to not implement
> > > > the mac_config method if there is nothing to do. Keep the method
> > > > required for setups that do not provide pcs_ops.
> > > > 
> > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > ---
> > > >  drivers/net/phy/phylink.c | 9 +++++++++
> > > >  1 file changed, 9 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > index 96d8e88b4e46..a8842c6ce3a2 100644
> > > > --- a/drivers/net/phy/phylink.c
> > > > +++ b/drivers/net/phy/phylink.c
> > > > @@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
> > > >  static void phylink_mac_config(struct phylink *pl,
> > > >  			       const struct phylink_link_state *state)
> > > >  {
> > > > +	if (!pl->mac_ops->mac_config)
> > > > +		return;
> > > > +
> > > >  	phylink_dbg(pl,
> > > >  		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
> > > >  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
> > > > @@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
> > > >  
> > > >  	ASSERT_RTNL();
> > > >  
> > > > +	/* The mac_ops::mac_config method may be absent only if the
> > > > +	 * pcs_ops are present.
> > > > +	 */
> > > > +	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
> > > > +		return;
> > > > +
> > > >  	phylink_info(pl, "configuring for %s/%s link mode\n",
> > > >  		     phylink_an_mode_str(pl->cur_link_an_mode),
> > > >  		     phy_modes(pl->link_config.interface));
> > > 
> > > I would rather we didn't do that - I suspect your case is not the
> > > common scenario, so please add a dummy function to stmmac instead.
> > 
> > Ok, in that case the only delta to be applied should be:
> > 
> > -----------------------------[ cut here]-----------------------------
> > >From 998569108392befc591f790e46fe5dcd1d0b6278 Mon Sep 17 00:00:00 2001
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > Date: Wed, 2 Jun 2021 17:00:12 +0300
> > Subject: [PATCH] fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index d5685a74f3b7..704aa91b145a 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -1000,6 +1000,12 @@ static void stmmac_validate(struct phylink_config *config,
> >  		xpcs_validate(priv->hw->xpcs, supported, state);
> >  }
> >  
> > +static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
> > +			      const struct phylink_link_state *state)
> > +{
> > +	/* Nothing to do, xpcs_config() handles everything */
> > +}
> > +
> >  static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
> >  {
> >  	struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
> > -----------------------------[ cut here]-----------------------------
> 
> No luck.. I am still seeing the same kernel panic on my side with the
> additional patch applied:
> 
> [   12.513652] intel-eth-pci 0000:00:1e.4 enp0s30f4: FPE workqueue start
> [   12.520079] intel-eth-pci 0000:00:1e.4 enp0s30f4: configuring for inband/sgmii link mode
> [   12.528141] BUG: kernel NULL pointer dereference, address: 0000000000000000
> [   12.535072] #PF: supervisor instruction fetch in kernel mode
> [   12.540719] #PF: error_code(0x0010) - not-present page
> [   12.545843] PGD 0 P4D 0
> [   12.548382] Oops: 0010 [#1] PREEMPT SMP NOPTI
> [   12.552733] CPU: 3 PID: 2657 Comm: connmand Tainted: G     U            5.13.0-rc3-intel-lts #79
> [   12.561485] Hardware name: Intel Corporation Tiger Lake Client Platform/TigerLake U DDR4 SODIMM RVP, BIOS TGLIFUI1.R00.3373.AF0.2009230546 09/23/2020
> [   12.574803] RIP: 0010:0x0
> [   12.577436] Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6.
> [   12.584280] RSP: 0018:ffffaa05404efb78 EFLAGS: 00010246
> [   12.589489] RAX: 0000000000000000 RBX: ffffa0fb91758c00 RCX: 0000000000000000
> [   12.596600] RDX: ffffaa05404efba0 RSI: 0000000000000002 RDI: ffffa0facc63c4d8
> [   12.603711] RBP: ffffaa05404efba0 R08: 0000000000000000 R09: ffffaa05404ef888
> [   12.610825] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
> [   12.617935] R13: ffffa0facc6388c0 R14: 0000000000000006 R15: 0000000000000001
> [   12.625040] FS:  00007f4a6d5b87c0(0000) GS:ffffa0fc57f80000(0000) knlGS:0000000000000000
> [   12.633096] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   12.638825] CR2: ffffffffffffffd6 CR3: 000000010e3f6004 CR4: 0000000000770ee0
> [   12.645932] PKRU: 55555554
> [   12.648641] Call Trace:
> [   12.651095]  phylink_major_config+0x5e/0x1a0 [phylink]
> [   12.656219]  phylink_start+0x204/0x2c0 [phylink]
> [   12.660833]  stmmac_open+0x3d0/0x9f0 [stmmac]
> [   12.665186]  __dev_open+0xe7/0x180
> [   12.668594]  __dev_change_flags+0x174/0x1d0
> [   12.672773]  ? __thaw_task+0x40/0x40
> [   12.676348]  ? arch_stack_walk+0x9e/0xf0
> [   12.680266]  dev_change_flags+0x21/0x60
> [   12.684100]  devinet_ioctl+0x5e8/0x750
> [   12.687847]  inet_ioctl+0x190/0x1c0
> [   12.691335]  ? dev_ioctl+0x26d/0x4c0
> [   12.694907]  sock_do_ioctl+0x44/0x140
> [   12.698569]  ? alloc_empty_file+0x61/0xb0
> [   12.702572]  sock_ioctl+0x22c/0x320
> [   12.706061]  __x64_sys_ioctl+0x80/0xb0
> [   12.709808]  do_syscall_64+0x42/0x80
> [   12.713381]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> 
> My git log:-
> dc25bc46886e (HEAD -> master) phy: maxlinear: add Maxlinear GPY115/21x/24x driver
> d059f32fcef1 fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> 1a1a9c7267c1 net: pcs: xpcs: convert to phylink_pcs_ops
> ec25f3c80108 net: pcs: xpcs: convert to mdio_device
> 912a23a5768a net: pcs: xpcs: use mdiobus_c45_addr in xpcs_{read,write}
> af2bd0a4e32c net: pcs: xpcs: export xpcs_probe
> 7e4eea51132f net: pcs: xpcs: export xpcs_config_eee
> 37d5e086fc83 net: pcs: xpcs: export xpcs_validate
> d58487edb1a8 net: pcs: xpcs: make the checks related to the PHY interface mode stateless
> 2f0eba94af8d net: pcs: xpcs: there is only one PHY ID
> 67148dd06896 net: pcs: xpcs: delete shim definition for mdio_xpcs_get_ops()
> 5fe8e519e44f (origin/master, origin/HEAD) Merge git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next

Yikes, stupid me, I forgot to add this piece to the delta, the new
callback wasn't doing anything, but it also didn't warn me that the
static function is unused in my build:

-----------------------------[ cut here]-----------------------------
>From 7a2b56091fd480ae1018ac964756368904a41a0c Mon Sep 17 00:00:00 2001
From: Vladimir Oltean <vladimir.oltean@nxp.com>
Date: Wed, 2 Jun 2021 17:54:58 +0300
Subject: [PATCH] fixup! fixup! net: pcs: xpcs: convert to phylink_pcs_ops

Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 704aa91b145a..5b5edfdccab3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1137,6 +1137,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
 
 static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
 	.validate = stmmac_validate,
+	.mac_config = stmmac_mac_config,
 	.mac_link_down = stmmac_mac_link_down,
 	.mac_link_up = stmmac_mac_link_up,
 };
-----------------------------[ cut here]-----------------------------
Wong Vee Khee June 2, 2021, 3:10 p.m. UTC | #7 
On Wed, Jun 02, 2021 at 05:57:30PM +0300, Vladimir Oltean wrote:
> On Wed, Jun 02, 2021 at 10:43:36PM +0800, Wong Vee Khee wrote:
> > On Wed, Jun 02, 2021 at 05:02:33PM +0300, Vladimir Oltean wrote:
> > > On Wed, Jun 02, 2021 at 02:47:49PM +0100, Russell King (Oracle) wrote:
> > > > On Wed, Jun 02, 2021 at 04:43:21PM +0300, Vladimir Oltean wrote:
> > > > > On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> > > > > > On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> > > > > > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > > > > > >  	.validate = stmmac_validate,
> > > > > > > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > > > > > > -	.mac_config = stmmac_mac_config,
> > > > > > 
> > > > > > mac_config is still a required function.
> > > > > 
> > > > > This is correct, thanks.
> > > > > 
> > > > > VK, would you mind testing again with this extra patch added to the mix?
> > > > > If it works, I will add it to the series in v3, ordered properly.
> > > > > 
> > > > > -----------------------------[ cut here]-----------------------------
> > > > > From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
> > > > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > Date: Wed, 2 Jun 2021 16:35:55 +0300
> > > > > Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
> > > > >  pcs_ops are provided
> > > > > 
> > > > > The pcs_config method from struct phylink_pcs_ops does everything that
> > > > > the mac_config method from struct phylink_mac_ops used to do in the
> > > > > legacy approach of driving a MAC PCS. So allow drivers to not implement
> > > > > the mac_config method if there is nothing to do. Keep the method
> > > > > required for setups that do not provide pcs_ops.
> > > > > 
> > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > ---
> > > > >  drivers/net/phy/phylink.c | 9 +++++++++
> > > > >  1 file changed, 9 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > > index 96d8e88b4e46..a8842c6ce3a2 100644
> > > > > --- a/drivers/net/phy/phylink.c
> > > > > +++ b/drivers/net/phy/phylink.c
> > > > > @@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
> > > > >  static void phylink_mac_config(struct phylink *pl,
> > > > >  			       const struct phylink_link_state *state)
> > > > >  {
> > > > > +	if (!pl->mac_ops->mac_config)
> > > > > +		return;
> > > > > +
> > > > >  	phylink_dbg(pl,
> > > > >  		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
> > > > >  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
> > > > > @@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
> > > > >  
> > > > >  	ASSERT_RTNL();
> > > > >  
> > > > > +	/* The mac_ops::mac_config method may be absent only if the
> > > > > +	 * pcs_ops are present.
> > > > > +	 */
> > > > > +	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
> > > > > +		return;
> > > > > +
> > > > >  	phylink_info(pl, "configuring for %s/%s link mode\n",
> > > > >  		     phylink_an_mode_str(pl->cur_link_an_mode),
> > > > >  		     phy_modes(pl->link_config.interface));
> > > > 
> > > > I would rather we didn't do that - I suspect your case is not the
> > > > common scenario, so please add a dummy function to stmmac instead.
> > > 
> > > Ok, in that case the only delta to be applied should be:
> > > 
> > > -----------------------------[ cut here]-----------------------------
> > > >From 998569108392befc591f790e46fe5dcd1d0b6278 Mon Sep 17 00:00:00 2001
> > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > Date: Wed, 2 Jun 2021 17:00:12 +0300
> > > Subject: [PATCH] fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > > 
> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++++
> > >  1 file changed, 6 insertions(+)
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > index d5685a74f3b7..704aa91b145a 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > @@ -1000,6 +1000,12 @@ static void stmmac_validate(struct phylink_config *config,
> > >  		xpcs_validate(priv->hw->xpcs, supported, state);
> > >  }
> > >  
> > > +static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
> > > +			      const struct phylink_link_state *state)
> > > +{
> > > +	/* Nothing to do, xpcs_config() handles everything */
> > > +}
> > > +
> > >  static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
> > >  {
> > >  	struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
> > > -----------------------------[ cut here]-----------------------------
> > 
> > No luck.. I am still seeing the same kernel panic on my side with the
> > additional patch applied:
> > 
> > [   12.513652] intel-eth-pci 0000:00:1e.4 enp0s30f4: FPE workqueue start
> > [   12.520079] intel-eth-pci 0000:00:1e.4 enp0s30f4: configuring for inband/sgmii link mode
> > [   12.528141] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > [   12.535072] #PF: supervisor instruction fetch in kernel mode
> > [   12.540719] #PF: error_code(0x0010) - not-present page
> > [   12.545843] PGD 0 P4D 0
> > [   12.548382] Oops: 0010 [#1] PREEMPT SMP NOPTI
> > [   12.552733] CPU: 3 PID: 2657 Comm: connmand Tainted: G     U            5.13.0-rc3-intel-lts #79
> > [   12.561485] Hardware name: Intel Corporation Tiger Lake Client Platform/TigerLake U DDR4 SODIMM RVP, BIOS TGLIFUI1.R00.3373.AF0.2009230546 09/23/2020
> > [   12.574803] RIP: 0010:0x0
> > [   12.577436] Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6.
> > [   12.584280] RSP: 0018:ffffaa05404efb78 EFLAGS: 00010246
> > [   12.589489] RAX: 0000000000000000 RBX: ffffa0fb91758c00 RCX: 0000000000000000
> > [   12.596600] RDX: ffffaa05404efba0 RSI: 0000000000000002 RDI: ffffa0facc63c4d8
> > [   12.603711] RBP: ffffaa05404efba0 R08: 0000000000000000 R09: ffffaa05404ef888
> > [   12.610825] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
> > [   12.617935] R13: ffffa0facc6388c0 R14: 0000000000000006 R15: 0000000000000001
> > [   12.625040] FS:  00007f4a6d5b87c0(0000) GS:ffffa0fc57f80000(0000) knlGS:0000000000000000
> > [   12.633096] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > [   12.638825] CR2: ffffffffffffffd6 CR3: 000000010e3f6004 CR4: 0000000000770ee0
> > [   12.645932] PKRU: 55555554
> > [   12.648641] Call Trace:
> > [   12.651095]  phylink_major_config+0x5e/0x1a0 [phylink]
> > [   12.656219]  phylink_start+0x204/0x2c0 [phylink]
> > [   12.660833]  stmmac_open+0x3d0/0x9f0 [stmmac]
> > [   12.665186]  __dev_open+0xe7/0x180
> > [   12.668594]  __dev_change_flags+0x174/0x1d0
> > [   12.672773]  ? __thaw_task+0x40/0x40
> > [   12.676348]  ? arch_stack_walk+0x9e/0xf0
> > [   12.680266]  dev_change_flags+0x21/0x60
> > [   12.684100]  devinet_ioctl+0x5e8/0x750
> > [   12.687847]  inet_ioctl+0x190/0x1c0
> > [   12.691335]  ? dev_ioctl+0x26d/0x4c0
> > [   12.694907]  sock_do_ioctl+0x44/0x140
> > [   12.698569]  ? alloc_empty_file+0x61/0xb0
> > [   12.702572]  sock_ioctl+0x22c/0x320
> > [   12.706061]  __x64_sys_ioctl+0x80/0xb0
> > [   12.709808]  do_syscall_64+0x42/0x80
> > [   12.713381]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> > 
> > My git log:-
> > dc25bc46886e (HEAD -> master) phy: maxlinear: add Maxlinear GPY115/21x/24x driver
> > d059f32fcef1 fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > 1a1a9c7267c1 net: pcs: xpcs: convert to phylink_pcs_ops
> > ec25f3c80108 net: pcs: xpcs: convert to mdio_device
> > 912a23a5768a net: pcs: xpcs: use mdiobus_c45_addr in xpcs_{read,write}
> > af2bd0a4e32c net: pcs: xpcs: export xpcs_probe
> > 7e4eea51132f net: pcs: xpcs: export xpcs_config_eee
> > 37d5e086fc83 net: pcs: xpcs: export xpcs_validate
> > d58487edb1a8 net: pcs: xpcs: make the checks related to the PHY interface mode stateless
> > 2f0eba94af8d net: pcs: xpcs: there is only one PHY ID
> > 67148dd06896 net: pcs: xpcs: delete shim definition for mdio_xpcs_get_ops()
> > 5fe8e519e44f (origin/master, origin/HEAD) Merge git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next
> 
> Yikes, stupid me, I forgot to add this piece to the delta, the new
> callback wasn't doing anything, but it also didn't warn me that the
> static function is unused in my build:
> 
> -----------------------------[ cut here]-----------------------------
> >From 7a2b56091fd480ae1018ac964756368904a41a0c Mon Sep 17 00:00:00 2001
> From: Vladimir Oltean <vladimir.oltean@nxp.com>
> Date: Wed, 2 Jun 2021 17:54:58 +0300
> Subject: [PATCH] fixup! fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> 
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> ---
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 704aa91b145a..5b5edfdccab3 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -1137,6 +1137,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
>  
>  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
>  	.validate = stmmac_validate,
> +	.mac_config = stmmac_mac_config,
>  	.mac_link_down = stmmac_mac_link_down,
>  	.mac_link_up = stmmac_mac_link_up,
>  };
> -----------------------------[ cut here]-----------------------------

It works on my setup now after applying the last fixup patch! :)

 VK
Vladimir Oltean June 2, 2021, 3:14 p.m. UTC | #8 
On Wed, Jun 02, 2021 at 11:10:56PM +0800, Wong Vee Khee wrote:
> On Wed, Jun 02, 2021 at 05:57:30PM +0300, Vladimir Oltean wrote:
> > On Wed, Jun 02, 2021 at 10:43:36PM +0800, Wong Vee Khee wrote:
> > > On Wed, Jun 02, 2021 at 05:02:33PM +0300, Vladimir Oltean wrote:
> > > > On Wed, Jun 02, 2021 at 02:47:49PM +0100, Russell King (Oracle) wrote:
> > > > > On Wed, Jun 02, 2021 at 04:43:21PM +0300, Vladimir Oltean wrote:
> > > > > > On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> > > > > > > On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> > > > > > > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > > > > > > >  	.validate = stmmac_validate,
> > > > > > > > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > > > > > > > -	.mac_config = stmmac_mac_config,
> > > > > > > 
> > > > > > > mac_config is still a required function.
> > > > > > 
> > > > > > This is correct, thanks.
> > > > > > 
> > > > > > VK, would you mind testing again with this extra patch added to the mix?
> > > > > > If it works, I will add it to the series in v3, ordered properly.
> > > > > > 
> > > > > > -----------------------------[ cut here]-----------------------------
> > > > > > From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
> > > > > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > > Date: Wed, 2 Jun 2021 16:35:55 +0300
> > > > > > Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
> > > > > >  pcs_ops are provided
> > > > > > 
> > > > > > The pcs_config method from struct phylink_pcs_ops does everything that
> > > > > > the mac_config method from struct phylink_mac_ops used to do in the
> > > > > > legacy approach of driving a MAC PCS. So allow drivers to not implement
> > > > > > the mac_config method if there is nothing to do. Keep the method
> > > > > > required for setups that do not provide pcs_ops.
> > > > > > 
> > > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > > ---
> > > > > >  drivers/net/phy/phylink.c | 9 +++++++++
> > > > > >  1 file changed, 9 insertions(+)
> > > > > > 
> > > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > > > index 96d8e88b4e46..a8842c6ce3a2 100644
> > > > > > --- a/drivers/net/phy/phylink.c
> > > > > > +++ b/drivers/net/phy/phylink.c
> > > > > > @@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
> > > > > >  static void phylink_mac_config(struct phylink *pl,
> > > > > >  			       const struct phylink_link_state *state)
> > > > > >  {
> > > > > > +	if (!pl->mac_ops->mac_config)
> > > > > > +		return;
> > > > > > +
> > > > > >  	phylink_dbg(pl,
> > > > > >  		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
> > > > > >  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
> > > > > > @@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
> > > > > >  
> > > > > >  	ASSERT_RTNL();
> > > > > >  
> > > > > > +	/* The mac_ops::mac_config method may be absent only if the
> > > > > > +	 * pcs_ops are present.
> > > > > > +	 */
> > > > > > +	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
> > > > > > +		return;
> > > > > > +
> > > > > >  	phylink_info(pl, "configuring for %s/%s link mode\n",
> > > > > >  		     phylink_an_mode_str(pl->cur_link_an_mode),
> > > > > >  		     phy_modes(pl->link_config.interface));
> > > > > 
> > > > > I would rather we didn't do that - I suspect your case is not the
> > > > > common scenario, so please add a dummy function to stmmac instead.
> > > > 
> > > > Ok, in that case the only delta to be applied should be:
> > > > 
> > > > -----------------------------[ cut here]-----------------------------
> > > > >From 998569108392befc591f790e46fe5dcd1d0b6278 Mon Sep 17 00:00:00 2001
> > > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > Date: Wed, 2 Jun 2021 17:00:12 +0300
> > > > Subject: [PATCH] fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > > > 
> > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > ---
> > > >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++++
> > > >  1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > index d5685a74f3b7..704aa91b145a 100644
> > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > @@ -1000,6 +1000,12 @@ static void stmmac_validate(struct phylink_config *config,
> > > >  		xpcs_validate(priv->hw->xpcs, supported, state);
> > > >  }
> > > >  
> > > > +static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
> > > > +			      const struct phylink_link_state *state)
> > > > +{
> > > > +	/* Nothing to do, xpcs_config() handles everything */
> > > > +}
> > > > +
> > > >  static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
> > > >  {
> > > >  	struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
> > > > -----------------------------[ cut here]-----------------------------
> > > 
> > > No luck.. I am still seeing the same kernel panic on my side with the
> > > additional patch applied:
> > > 
> > > [   12.513652] intel-eth-pci 0000:00:1e.4 enp0s30f4: FPE workqueue start
> > > [   12.520079] intel-eth-pci 0000:00:1e.4 enp0s30f4: configuring for inband/sgmii link mode
> > > [   12.528141] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > [   12.535072] #PF: supervisor instruction fetch in kernel mode
> > > [   12.540719] #PF: error_code(0x0010) - not-present page
> > > [   12.545843] PGD 0 P4D 0
> > > [   12.548382] Oops: 0010 [#1] PREEMPT SMP NOPTI
> > > [   12.552733] CPU: 3 PID: 2657 Comm: connmand Tainted: G     U            5.13.0-rc3-intel-lts #79
> > > [   12.561485] Hardware name: Intel Corporation Tiger Lake Client Platform/TigerLake U DDR4 SODIMM RVP, BIOS TGLIFUI1.R00.3373.AF0.2009230546 09/23/2020
> > > [   12.574803] RIP: 0010:0x0
> > > [   12.577436] Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6.
> > > [   12.584280] RSP: 0018:ffffaa05404efb78 EFLAGS: 00010246
> > > [   12.589489] RAX: 0000000000000000 RBX: ffffa0fb91758c00 RCX: 0000000000000000
> > > [   12.596600] RDX: ffffaa05404efba0 RSI: 0000000000000002 RDI: ffffa0facc63c4d8
> > > [   12.603711] RBP: ffffaa05404efba0 R08: 0000000000000000 R09: ffffaa05404ef888
> > > [   12.610825] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
> > > [   12.617935] R13: ffffa0facc6388c0 R14: 0000000000000006 R15: 0000000000000001
> > > [   12.625040] FS:  00007f4a6d5b87c0(0000) GS:ffffa0fc57f80000(0000) knlGS:0000000000000000
> > > [   12.633096] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > [   12.638825] CR2: ffffffffffffffd6 CR3: 000000010e3f6004 CR4: 0000000000770ee0
> > > [   12.645932] PKRU: 55555554
> > > [   12.648641] Call Trace:
> > > [   12.651095]  phylink_major_config+0x5e/0x1a0 [phylink]
> > > [   12.656219]  phylink_start+0x204/0x2c0 [phylink]
> > > [   12.660833]  stmmac_open+0x3d0/0x9f0 [stmmac]
> > > [   12.665186]  __dev_open+0xe7/0x180
> > > [   12.668594]  __dev_change_flags+0x174/0x1d0
> > > [   12.672773]  ? __thaw_task+0x40/0x40
> > > [   12.676348]  ? arch_stack_walk+0x9e/0xf0
> > > [   12.680266]  dev_change_flags+0x21/0x60
> > > [   12.684100]  devinet_ioctl+0x5e8/0x750
> > > [   12.687847]  inet_ioctl+0x190/0x1c0
> > > [   12.691335]  ? dev_ioctl+0x26d/0x4c0
> > > [   12.694907]  sock_do_ioctl+0x44/0x140
> > > [   12.698569]  ? alloc_empty_file+0x61/0xb0
> > > [   12.702572]  sock_ioctl+0x22c/0x320
> > > [   12.706061]  __x64_sys_ioctl+0x80/0xb0
> > > [   12.709808]  do_syscall_64+0x42/0x80
> > > [   12.713381]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > 
> > > My git log:-
> > > dc25bc46886e (HEAD -> master) phy: maxlinear: add Maxlinear GPY115/21x/24x driver
> > > d059f32fcef1 fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > > 1a1a9c7267c1 net: pcs: xpcs: convert to phylink_pcs_ops
> > > ec25f3c80108 net: pcs: xpcs: convert to mdio_device
> > > 912a23a5768a net: pcs: xpcs: use mdiobus_c45_addr in xpcs_{read,write}
> > > af2bd0a4e32c net: pcs: xpcs: export xpcs_probe
> > > 7e4eea51132f net: pcs: xpcs: export xpcs_config_eee
> > > 37d5e086fc83 net: pcs: xpcs: export xpcs_validate
> > > d58487edb1a8 net: pcs: xpcs: make the checks related to the PHY interface mode stateless
> > > 2f0eba94af8d net: pcs: xpcs: there is only one PHY ID
> > > 67148dd06896 net: pcs: xpcs: delete shim definition for mdio_xpcs_get_ops()
> > > 5fe8e519e44f (origin/master, origin/HEAD) Merge git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next
> > 
> > Yikes, stupid me, I forgot to add this piece to the delta, the new
> > callback wasn't doing anything, but it also didn't warn me that the
> > static function is unused in my build:
> > 
> > -----------------------------[ cut here]-----------------------------
> > >From 7a2b56091fd480ae1018ac964756368904a41a0c Mon Sep 17 00:00:00 2001
> > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > Date: Wed, 2 Jun 2021 17:54:58 +0300
> > Subject: [PATCH] fixup! fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 704aa91b145a..5b5edfdccab3 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -1137,6 +1137,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
> >  
> >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> >  	.validate = stmmac_validate,
> > +	.mac_config = stmmac_mac_config,
> >  	.mac_link_down = stmmac_mac_link_down,
> >  	.mac_link_up = stmmac_mac_link_up,
> >  };
> > -----------------------------[ cut here]-----------------------------
> 
> It works on my setup now after applying the last fixup patch! :)

Thanks again for testing.
Just for clarity, "it works" includes traffic at 10/100/1000, right?
If I'm not mistaken, you tested with sgmii and MLO_INBAND_AN, correct?
On the sja1105 I am testing with sgmii and no MLO_INBAND_AN, so not
exactly the same setup (that, plus my hardware needs a few more changes
still).
Wong Vee Khee June 2, 2021, 3:28 p.m. UTC | #9 
On Wed, Jun 02, 2021 at 06:14:47PM +0300, Vladimir Oltean wrote:
> On Wed, Jun 02, 2021 at 11:10:56PM +0800, Wong Vee Khee wrote:
> > On Wed, Jun 02, 2021 at 05:57:30PM +0300, Vladimir Oltean wrote:
> > > On Wed, Jun 02, 2021 at 10:43:36PM +0800, Wong Vee Khee wrote:
> > > > On Wed, Jun 02, 2021 at 05:02:33PM +0300, Vladimir Oltean wrote:
> > > > > On Wed, Jun 02, 2021 at 02:47:49PM +0100, Russell King (Oracle) wrote:
> > > > > > On Wed, Jun 02, 2021 at 04:43:21PM +0300, Vladimir Oltean wrote:
> > > > > > > On Tue, Jun 01, 2021 at 01:10:33PM +0100, Russell King (Oracle) wrote:
> > > > > > > > On Tue, Jun 01, 2021 at 03:33:25AM +0300, Vladimir Oltean wrote:
> > > > > > > > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > > > > > > > >  	.validate = stmmac_validate,
> > > > > > > > > -	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
> > > > > > > > > -	.mac_config = stmmac_mac_config,
> > > > > > > > 
> > > > > > > > mac_config is still a required function.
> > > > > > > 
> > > > > > > This is correct, thanks.
> > > > > > > 
> > > > > > > VK, would you mind testing again with this extra patch added to the mix?
> > > > > > > If it works, I will add it to the series in v3, ordered properly.
> > > > > > > 
> > > > > > > -----------------------------[ cut here]-----------------------------
> > > > > > > From a79863027998451c73d5bbfaf1b77cf6097a110c Mon Sep 17 00:00:00 2001
> > > > > > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > > > Date: Wed, 2 Jun 2021 16:35:55 +0300
> > > > > > > Subject: [PATCH] net: phylink: allow the mac_config method to be missing if
> > > > > > >  pcs_ops are provided
> > > > > > > 
> > > > > > > The pcs_config method from struct phylink_pcs_ops does everything that
> > > > > > > the mac_config method from struct phylink_mac_ops used to do in the
> > > > > > > legacy approach of driving a MAC PCS. So allow drivers to not implement
> > > > > > > the mac_config method if there is nothing to do. Keep the method
> > > > > > > required for setups that do not provide pcs_ops.
> > > > > > > 
> > > > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > > > ---
> > > > > > >  drivers/net/phy/phylink.c | 9 +++++++++
> > > > > > >  1 file changed, 9 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > > > > > > index 96d8e88b4e46..a8842c6ce3a2 100644
> > > > > > > --- a/drivers/net/phy/phylink.c
> > > > > > > +++ b/drivers/net/phy/phylink.c
> > > > > > > @@ -415,6 +415,9 @@ static void phylink_resolve_flow(struct phylink_link_state *state)
> > > > > > >  static void phylink_mac_config(struct phylink *pl,
> > > > > > >  			       const struct phylink_link_state *state)
> > > > > > >  {
> > > > > > > +	if (!pl->mac_ops->mac_config)
> > > > > > > +		return;
> > > > > > > +
> > > > > > >  	phylink_dbg(pl,
> > > > > > >  		    "%s: mode=%s/%s/%s/%s adv=%*pb pause=%02x link=%u an=%u\n",
> > > > > > >  		    __func__, phylink_an_mode_str(pl->cur_link_an_mode),
> > > > > > > @@ -1192,6 +1195,12 @@ void phylink_start(struct phylink *pl)
> > > > > > >  
> > > > > > >  	ASSERT_RTNL();
> > > > > > >  
> > > > > > > +	/* The mac_ops::mac_config method may be absent only if the
> > > > > > > +	 * pcs_ops are present.
> > > > > > > +	 */
> > > > > > > +	if (WARN_ON_ONCE(!pl->mac_ops->mac_config && !pl->pcs_ops))
> > > > > > > +		return;
> > > > > > > +
> > > > > > >  	phylink_info(pl, "configuring for %s/%s link mode\n",
> > > > > > >  		     phylink_an_mode_str(pl->cur_link_an_mode),
> > > > > > >  		     phy_modes(pl->link_config.interface));
> > > > > > 
> > > > > > I would rather we didn't do that - I suspect your case is not the
> > > > > > common scenario, so please add a dummy function to stmmac instead.
> > > > > 
> > > > > Ok, in that case the only delta to be applied should be:
> > > > > 
> > > > > -----------------------------[ cut here]-----------------------------
> > > > > >From 998569108392befc591f790e46fe5dcd1d0b6278 Mon Sep 17 00:00:00 2001
> > > > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > Date: Wed, 2 Jun 2021 17:00:12 +0300
> > > > > Subject: [PATCH] fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > > > > 
> > > > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > > > ---
> > > > >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > > 
> > > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > > index d5685a74f3b7..704aa91b145a 100644
> > > > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > > > @@ -1000,6 +1000,12 @@ static void stmmac_validate(struct phylink_config *config,
> > > > >  		xpcs_validate(priv->hw->xpcs, supported, state);
> > > > >  }
> > > > >  
> > > > > +static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
> > > > > +			      const struct phylink_link_state *state)
> > > > > +{
> > > > > +	/* Nothing to do, xpcs_config() handles everything */
> > > > > +}
> > > > > +
> > > > >  static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
> > > > >  {
> > > > >  	struct stmmac_fpe_cfg *fpe_cfg = priv->plat->fpe_cfg;
> > > > > -----------------------------[ cut here]-----------------------------
> > > > 
> > > > No luck.. I am still seeing the same kernel panic on my side with the
> > > > additional patch applied:
> > > > 
> > > > [   12.513652] intel-eth-pci 0000:00:1e.4 enp0s30f4: FPE workqueue start
> > > > [   12.520079] intel-eth-pci 0000:00:1e.4 enp0s30f4: configuring for inband/sgmii link mode
> > > > [   12.528141] BUG: kernel NULL pointer dereference, address: 0000000000000000
> > > > [   12.535072] #PF: supervisor instruction fetch in kernel mode
> > > > [   12.540719] #PF: error_code(0x0010) - not-present page
> > > > [   12.545843] PGD 0 P4D 0
> > > > [   12.548382] Oops: 0010 [#1] PREEMPT SMP NOPTI
> > > > [   12.552733] CPU: 3 PID: 2657 Comm: connmand Tainted: G     U            5.13.0-rc3-intel-lts #79
> > > > [   12.561485] Hardware name: Intel Corporation Tiger Lake Client Platform/TigerLake U DDR4 SODIMM RVP, BIOS TGLIFUI1.R00.3373.AF0.2009230546 09/23/2020
> > > > [   12.574803] RIP: 0010:0x0
> > > > [   12.577436] Code: Unable to access opcode bytes at RIP 0xffffffffffffffd6.
> > > > [   12.584280] RSP: 0018:ffffaa05404efb78 EFLAGS: 00010246
> > > > [   12.589489] RAX: 0000000000000000 RBX: ffffa0fb91758c00 RCX: 0000000000000000
> > > > [   12.596600] RDX: ffffaa05404efba0 RSI: 0000000000000002 RDI: ffffa0facc63c4d8
> > > > [   12.603711] RBP: ffffaa05404efba0 R08: 0000000000000000 R09: ffffaa05404ef888
> > > > [   12.610825] R10: 0000000000000001 R11: 0000000000000001 R12: 0000000000000001
> > > > [   12.617935] R13: ffffa0facc6388c0 R14: 0000000000000006 R15: 0000000000000001
> > > > [   12.625040] FS:  00007f4a6d5b87c0(0000) GS:ffffa0fc57f80000(0000) knlGS:0000000000000000
> > > > [   12.633096] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> > > > [   12.638825] CR2: ffffffffffffffd6 CR3: 000000010e3f6004 CR4: 0000000000770ee0
> > > > [   12.645932] PKRU: 55555554
> > > > [   12.648641] Call Trace:
> > > > [   12.651095]  phylink_major_config+0x5e/0x1a0 [phylink]
> > > > [   12.656219]  phylink_start+0x204/0x2c0 [phylink]
> > > > [   12.660833]  stmmac_open+0x3d0/0x9f0 [stmmac]
> > > > [   12.665186]  __dev_open+0xe7/0x180
> > > > [   12.668594]  __dev_change_flags+0x174/0x1d0
> > > > [   12.672773]  ? __thaw_task+0x40/0x40
> > > > [   12.676348]  ? arch_stack_walk+0x9e/0xf0
> > > > [   12.680266]  dev_change_flags+0x21/0x60
> > > > [   12.684100]  devinet_ioctl+0x5e8/0x750
> > > > [   12.687847]  inet_ioctl+0x190/0x1c0
> > > > [   12.691335]  ? dev_ioctl+0x26d/0x4c0
> > > > [   12.694907]  sock_do_ioctl+0x44/0x140
> > > > [   12.698569]  ? alloc_empty_file+0x61/0xb0
> > > > [   12.702572]  sock_ioctl+0x22c/0x320
> > > > [   12.706061]  __x64_sys_ioctl+0x80/0xb0
> > > > [   12.709808]  do_syscall_64+0x42/0x80
> > > > [   12.713381]  entry_SYSCALL_64_after_hwframe+0x44/0xae
> > > > 
> > > > My git log:-
> > > > dc25bc46886e (HEAD -> master) phy: maxlinear: add Maxlinear GPY115/21x/24x driver
> > > > d059f32fcef1 fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > > > 1a1a9c7267c1 net: pcs: xpcs: convert to phylink_pcs_ops
> > > > ec25f3c80108 net: pcs: xpcs: convert to mdio_device
> > > > 912a23a5768a net: pcs: xpcs: use mdiobus_c45_addr in xpcs_{read,write}
> > > > af2bd0a4e32c net: pcs: xpcs: export xpcs_probe
> > > > 7e4eea51132f net: pcs: xpcs: export xpcs_config_eee
> > > > 37d5e086fc83 net: pcs: xpcs: export xpcs_validate
> > > > d58487edb1a8 net: pcs: xpcs: make the checks related to the PHY interface mode stateless
> > > > 2f0eba94af8d net: pcs: xpcs: there is only one PHY ID
> > > > 67148dd06896 net: pcs: xpcs: delete shim definition for mdio_xpcs_get_ops()
> > > > 5fe8e519e44f (origin/master, origin/HEAD) Merge git://git.kernel.org/pub/scm/linux/kernel/git/pablo/nf-next
> > > 
> > > Yikes, stupid me, I forgot to add this piece to the delta, the new
> > > callback wasn't doing anything, but it also didn't warn me that the
> > > static function is unused in my build:
> > > 
> > > -----------------------------[ cut here]-----------------------------
> > > >From 7a2b56091fd480ae1018ac964756368904a41a0c Mon Sep 17 00:00:00 2001
> > > From: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > Date: Wed, 2 Jun 2021 17:54:58 +0300
> > > Subject: [PATCH] fixup! fixup! net: pcs: xpcs: convert to phylink_pcs_ops
> > > 
> > > Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
> > > ---
> > >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > index 704aa91b145a..5b5edfdccab3 100644
> > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > > @@ -1137,6 +1137,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
> > >  
> > >  static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
> > >  	.validate = stmmac_validate,
> > > +	.mac_config = stmmac_mac_config,
> > >  	.mac_link_down = stmmac_mac_link_down,
> > >  	.mac_link_up = stmmac_mac_link_up,
> > >  };
> > > -----------------------------[ cut here]-----------------------------
> > 
> > It works on my setup now after applying the last fixup patch! :)
> 
> Thanks again for testing.
> Just for clarity, "it works" includes traffic at 10/100/1000, right?
> If I'm not mistaken, you tested with sgmii and MLO_INBAND_AN, correct?
> On the sja1105 I am testing with sgmii and no MLO_INBAND_AN, so not
> exactly the same setup (that, plus my hardware needs a few more changes
> still).

Yes, I tested by performing ping on different link speeds (10/100/1000M),
with SGMII and Auto-negotiation enabled. Link speed is changed manually
using ethtool.

 VK
diff mbox series

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/common.h b/drivers/net/ethernet/stmicro/stmmac/common.h
index 4bcd1d340766..8a83f9e1e95b 100644
--- a/drivers/net/ethernet/stmicro/stmmac/common.h
+++ b/drivers/net/ethernet/stmicro/stmmac/common.h
@@ -503,8 +503,7 @@  struct mac_device_info {
 	const struct stmmac_hwtimestamp *ptp;
 	const struct stmmac_tc_ops *tc;
 	const struct stmmac_mmc_ops *mmc;
-	const struct mdio_xpcs_ops *xpcs;
-	struct mdio_xpcs_args *xpcs_args;
+	struct mdio_xpcs_args *xpcs;
 	struct mii_regs mii;	/* MII register Addresses */
 	struct mac_link link;
 	void __iomem *pcsr;     /* vpointer to device CSRs */
diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.h b/drivers/net/ethernet/stmicro/stmmac/hwif.h
index 5014b260844b..91f7592a0189 100644
--- a/drivers/net/ethernet/stmicro/stmmac/hwif.h
+++ b/drivers/net/ethernet/stmicro/stmmac/hwif.h
@@ -612,14 +612,6 @@  struct stmmac_mmc_ops {
 #define stmmac_mmc_read(__priv, __args...) \
 	stmmac_do_void_callback(__priv, mmc, read, __args)
 
-/* XPCS callbacks */
-#define stmmac_xpcs_config(__priv, __args...) \
-	stmmac_do_callback(__priv, xpcs, config, __args)
-#define stmmac_xpcs_get_state(__priv, __args...) \
-	stmmac_do_callback(__priv, xpcs, get_state, __args)
-#define stmmac_xpcs_link_up(__priv, __args...) \
-	stmmac_do_callback(__priv, xpcs, link_up, __args)
-
 struct stmmac_regs_off {
 	u32 ptp_off;
 	u32 mmc_off;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
index 050576ee704d..d0ce608b81c3 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_ethtool.c
@@ -721,7 +721,7 @@  static int stmmac_ethtool_op_set_eee(struct net_device *dev,
 			    "Setting EEE tx-lpi is not supported\n");
 
 	if (priv->hw->xpcs) {
-		ret = xpcs_config_eee(priv->hw->xpcs_args,
+		ret = xpcs_config_eee(priv->hw->xpcs,
 				      priv->plat->mult_fact_100ns,
 				      edata->eee_enabled);
 		if (ret)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 426c8f891f5a..d5685a74f3b7 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -997,29 +997,7 @@  static void stmmac_validate(struct phylink_config *config,
 
 	/* If PCS is supported, check which modes it supports. */
 	if (priv->hw->xpcs)
-		xpcs_validate(priv->hw->xpcs_args, supported, state);
-}
-
-static void stmmac_mac_pcs_get_state(struct phylink_config *config,
-				     struct phylink_link_state *state)
-{
-	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
-
-	state->link = 0;
-	stmmac_xpcs_get_state(priv, priv->hw->xpcs_args, state);
-}
-
-static void stmmac_mac_config(struct phylink_config *config, unsigned int mode,
-			      const struct phylink_link_state *state)
-{
-	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
-
-	stmmac_xpcs_config(priv, priv->hw->xpcs_args, state);
-}
-
-static void stmmac_mac_an_restart(struct phylink_config *config)
-{
-	/* Not Supported */
+		xpcs_validate(priv->hw->xpcs, supported, state);
 }
 
 static void stmmac_fpe_link_state_handle(struct stmmac_priv *priv, bool is_up)
@@ -1061,8 +1039,6 @@  static void stmmac_mac_link_up(struct phylink_config *config,
 	struct stmmac_priv *priv = netdev_priv(to_net_dev(config->dev));
 	u32 ctrl;
 
-	stmmac_xpcs_link_up(priv, priv->hw->xpcs_args, speed, interface);
-
 	ctrl = readl(priv->ioaddr + MAC_CTRL_REG);
 	ctrl &= ~priv->hw->link.speed_mask;
 
@@ -1155,9 +1131,6 @@  static void stmmac_mac_link_up(struct phylink_config *config,
 
 static const struct phylink_mac_ops stmmac_phylink_mac_ops = {
 	.validate = stmmac_validate,
-	.mac_pcs_get_state = stmmac_mac_pcs_get_state,
-	.mac_config = stmmac_mac_config,
-	.mac_an_restart = stmmac_mac_an_restart,
 	.mac_link_down = stmmac_mac_link_down,
 	.mac_link_up = stmmac_mac_link_up,
 };
@@ -1234,6 +1207,7 @@  static int stmmac_init_phy(struct net_device *dev)
 
 static int stmmac_phy_setup(struct stmmac_priv *priv)
 {
+	struct stmmac_mdio_bus_data *mdio_bus_data = priv->plat->mdio_bus_data;
 	struct fwnode_handle *fwnode = of_fwnode_handle(priv->plat->phylink_node);
 	int mode = priv->plat->phy_interface;
 	struct phylink *phylink;
@@ -1241,8 +1215,7 @@  static int stmmac_phy_setup(struct stmmac_priv *priv)
 	priv->phylink_config.dev = &priv->dev->dev;
 	priv->phylink_config.type = PHYLINK_NETDEV;
 	priv->phylink_config.pcs_poll = true;
-	priv->phylink_config.ovr_an_inband =
-		priv->plat->mdio_bus_data->xpcs_an_inband;
+	priv->phylink_config.ovr_an_inband = mdio_bus_data->xpcs_an_inband;
 
 	if (!fwnode)
 		fwnode = dev_fwnode(priv->device);
@@ -1252,6 +1225,12 @@  static int stmmac_phy_setup(struct stmmac_priv *priv)
 	if (IS_ERR(phylink))
 		return PTR_ERR(phylink);
 
+	if (mdio_bus_data->has_xpcs) {
+		struct mdio_xpcs_args *xpcs = priv->hw->xpcs;
+
+		phylink_set_pcs(phylink, &xpcs->pcs);
+	}
+
 	priv->phylink = phylink;
 	return 0;
 }
@@ -3652,7 +3631,7 @@  int stmmac_open(struct net_device *dev)
 	if (priv->hw->pcs != STMMAC_PCS_TBI &&
 	    priv->hw->pcs != STMMAC_PCS_RTBI &&
 	    (!priv->hw->xpcs ||
-	     xpcs_get_an_mode(priv->hw->xpcs_args, mode) != DW_AN_C73)) {
+	     xpcs_get_an_mode(priv->hw->xpcs, mode) != DW_AN_C73)) {
 		ret = stmmac_init_phy(dev);
 		if (ret) {
 			netdev_err(priv->dev,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
index 9b4bf78d2eaa..6312a152c8ad 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c
@@ -444,14 +444,6 @@  int stmmac_mdio_register(struct net_device *ndev)
 		max_addr = PHY_MAX_ADDR;
 	}
 
-	if (mdio_bus_data->has_xpcs) {
-		priv->hw->xpcs = mdio_xpcs_get_ops();
-		if (!priv->hw->xpcs) {
-			err = -ENODEV;
-			goto bus_register_fail;
-		}
-	}
-
 	if (mdio_bus_data->needs_reset)
 		new_bus->reset = &stmmac_mdio_reset;
 
@@ -526,11 +518,11 @@  int stmmac_mdio_register(struct net_device *ndev)
 				continue;
 			}
 
-			priv->hw->xpcs_args = xpcs;
+			priv->hw->xpcs = xpcs;
 			break;
 		}
 
-		if (!priv->hw->xpcs_args) {
+		if (!priv->hw->xpcs) {
 			dev_warn(dev, "No XPCS found\n");
 			err = -ENODEV;
 			goto no_xpcs_found;
@@ -563,8 +555,8 @@  int stmmac_mdio_unregister(struct net_device *ndev)
 		return 0;
 
 	if (priv->hw->xpcs) {
-		mdio_device_free(priv->hw->xpcs_args->mdiodev);
-		xpcs_destroy(priv->hw->xpcs_args);
+		mdio_device_free(priv->hw->xpcs->mdiodev);
+		xpcs_destroy(priv->hw->xpcs);
 	}
 
 	mdiobus_unregister(priv->mii);
diff --git a/drivers/net/pcs/pcs-xpcs.c b/drivers/net/pcs/pcs-xpcs.c
index 3e09850b8318..f1092771ab70 100644
--- a/drivers/net/pcs/pcs-xpcs.c
+++ b/drivers/net/pcs/pcs-xpcs.c
@@ -100,6 +100,9 @@ 
 /* VR MII EEE Control 1 defines */
 #define DW_VR_MII_EEE_TRN_LPI		BIT(0)	/* Transparent Mode Enable */
 
+#define phylink_pcs_to_xpcs(pl_pcs) \
+	container_of((pl_pcs), struct mdio_xpcs_args, pcs)
+
 static const int xpcs_usxgmii_features[] = {
 	ETHTOOL_LINK_MODE_Pause_BIT,
 	ETHTOOL_LINK_MODE_Asym_Pause_BIT,
@@ -448,7 +451,7 @@  static int xpcs_get_max_usxgmii_speed(const unsigned long *supported)
 	return max;
 }
 
-static int xpcs_config_usxgmii(struct mdio_xpcs_args *xpcs, int speed)
+static void xpcs_config_usxgmii(struct mdio_xpcs_args *xpcs, int speed)
 {
 	int ret, speed_sel;
 
@@ -473,33 +476,40 @@  static int xpcs_config_usxgmii(struct mdio_xpcs_args *xpcs, int speed)
 		break;
 	default:
 		/* Nothing to do here */
-		return -EINVAL;
+		return;
 	}
 
 	ret = xpcs_read_vpcs(xpcs, MDIO_CTRL1);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	ret = xpcs_write_vpcs(xpcs, MDIO_CTRL1, ret | DW_USXGMII_EN);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	ret = xpcs_read(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	ret &= ~DW_USXGMII_SS_MASK;
 	ret |= speed_sel | DW_USXGMII_FULL;
 
 	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, MDIO_CTRL1, ret);
 	if (ret < 0)
-		return ret;
+		goto out;
 
 	ret = xpcs_read_vpcs(xpcs, MDIO_CTRL1);
 	if (ret < 0)
-		return ret;
+		goto out;
+
+	ret = xpcs_write_vpcs(xpcs, MDIO_CTRL1, ret | DW_USXGMII_RST);
+	if (ret < 0)
+		goto out;
+
+	return;
 
-	return xpcs_write_vpcs(xpcs, MDIO_CTRL1, ret | DW_USXGMII_RST);
+out:
+	pr_err("%s: XPCS access returned %pe\n", __func__, ERR_PTR(ret));
 }
 
 static int _xpcs_config_aneg_c73(struct mdio_xpcs_args *xpcs,
@@ -829,19 +839,19 @@  static int xpcs_config_aneg_c37_sgmii(struct mdio_xpcs_args *xpcs)
 	return xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_DIG_CTRL1, ret);
 }
 
-static int xpcs_config(struct mdio_xpcs_args *xpcs,
-		       const struct phylink_link_state *state)
+static int xpcs_do_config(struct mdio_xpcs_args *xpcs,
+			  phy_interface_t interface, unsigned int mode)
 {
 	const struct xpcs_compat *compat;
 	int ret;
 
-	compat = xpcs_find_compat(xpcs->id, state->interface);
+	compat = xpcs_find_compat(xpcs->id, interface);
 	if (!compat)
 		return -ENODEV;
 
 	switch (compat->an_mode) {
 	case DW_AN_C73:
-		if (state->an_enabled) {
+		if (phylink_autoneg_inband(mode)) {
 			ret = xpcs_config_aneg_c73(xpcs, compat);
 			if (ret)
 				return ret;
@@ -859,6 +869,16 @@  static int xpcs_config(struct mdio_xpcs_args *xpcs,
 	return 0;
 }
 
+static int xpcs_config(struct phylink_pcs *pcs, unsigned int mode,
+		       phy_interface_t interface,
+		       const unsigned long *advertising,
+		       bool permit_pause_to_mac)
+{
+	struct mdio_xpcs_args *xpcs = phylink_pcs_to_xpcs(pcs);
+
+	return xpcs_do_config(xpcs, interface, mode);
+}
+
 static int xpcs_get_state_c73(struct mdio_xpcs_args *xpcs,
 			      struct phylink_link_state *state,
 			      const struct xpcs_compat *compat)
@@ -877,7 +897,7 @@  static int xpcs_get_state_c73(struct mdio_xpcs_args *xpcs,
 
 		state->link = 0;
 
-		return xpcs_config(xpcs, state);
+		return xpcs_do_config(xpcs, state->interface, MLO_AN_INBAND);
 	}
 
 	if (state->an_enabled && xpcs_aneg_done_c73(xpcs, state, compat)) {
@@ -934,41 +954,45 @@  static int xpcs_get_state_c37_sgmii(struct mdio_xpcs_args *xpcs,
 	return 0;
 }
 
-static int xpcs_get_state(struct mdio_xpcs_args *xpcs,
-			  struct phylink_link_state *state)
+static void xpcs_get_state(struct phylink_pcs *pcs,
+			   struct phylink_link_state *state)
 {
+	struct mdio_xpcs_args *xpcs = phylink_pcs_to_xpcs(pcs);
 	const struct xpcs_compat *compat;
 	int ret;
 
 	compat = xpcs_find_compat(xpcs->id, state->interface);
 	if (!compat)
-		return -ENODEV;
+		return;
 
 	switch (compat->an_mode) {
 	case DW_AN_C73:
 		ret = xpcs_get_state_c73(xpcs, state, compat);
-		if (ret)
-			return ret;
+		if (ret) {
+			pr_err("xpcs_get_state_c73 returned %pe\n",
+			       ERR_PTR(ret));
+			return;
+		}
 		break;
 	case DW_AN_C37_SGMII:
 		ret = xpcs_get_state_c37_sgmii(xpcs, state);
-		if (ret)
-			return ret;
+		if (ret) {
+			pr_err("xpcs_get_state_c37_sgmii returned %pe\n",
+			       ERR_PTR(ret));
+		}
 		break;
 	default:
-		return -1;
+		return;
 	}
-
-	return 0;
 }
 
-static int xpcs_link_up(struct mdio_xpcs_args *xpcs, int speed,
-			phy_interface_t interface)
+static void xpcs_link_up(struct phylink_pcs *pcs, unsigned int mode,
+			 phy_interface_t interface, int speed, int duplex)
 {
+	struct mdio_xpcs_args *xpcs = phylink_pcs_to_xpcs(pcs);
+
 	if (interface == PHY_INTERFACE_MODE_USXGMII)
 		return xpcs_config_usxgmii(xpcs, speed);
-
-	return 0;
 }
 
 static u32 xpcs_get_id(struct mdio_xpcs_args *xpcs)
@@ -1009,6 +1033,12 @@  static u32 xpcs_get_id(struct mdio_xpcs_args *xpcs)
 	return 0xffffffff;
 }
 
+static const struct phylink_pcs_ops xpcs_phylink_ops = {
+	.pcs_config = xpcs_config,
+	.pcs_get_state = xpcs_get_state,
+	.pcs_link_up = xpcs_link_up,
+};
+
 struct mdio_xpcs_args *xpcs_create(struct mdio_device *mdiodev,
 				   phy_interface_t interface)
 {
@@ -1039,6 +1069,9 @@  struct mdio_xpcs_args *xpcs_create(struct mdio_device *mdiodev,
 			goto out;
 		}
 
+		xpcs->pcs.ops = &xpcs_phylink_ops;
+		xpcs->pcs.poll = true;
+
 		ret = xpcs_soft_reset(xpcs, compat);
 		if (ret)
 			goto out;
@@ -1061,16 +1094,4 @@  void xpcs_destroy(struct mdio_xpcs_args *xpcs)
 }
 EXPORT_SYMBOL_GPL(xpcs_destroy);
 
-static struct mdio_xpcs_ops xpcs_ops = {
-	.config = xpcs_config,
-	.get_state = xpcs_get_state,
-	.link_up = xpcs_link_up,
-};
-
-struct mdio_xpcs_ops *mdio_xpcs_get_ops(void)
-{
-	return &xpcs_ops;
-}
-EXPORT_SYMBOL_GPL(mdio_xpcs_get_ops);
-
 MODULE_LICENSE("GPL v2");
diff --git a/include/linux/pcs/pcs-xpcs.h b/include/linux/pcs/pcs-xpcs.h
index 57a199393d63..0860a5b59f10 100644
--- a/include/linux/pcs/pcs-xpcs.h
+++ b/include/linux/pcs/pcs-xpcs.h
@@ -19,19 +19,10 @@  struct xpcs_id;
 struct mdio_xpcs_args {
 	struct mdio_device *mdiodev;
 	const struct xpcs_id *id;
-};
-
-struct mdio_xpcs_ops {
-	int (*config)(struct mdio_xpcs_args *xpcs,
-		      const struct phylink_link_state *state);
-	int (*get_state)(struct mdio_xpcs_args *xpcs,
-			 struct phylink_link_state *state);
-	int (*link_up)(struct mdio_xpcs_args *xpcs, int speed,
-		       phy_interface_t interface);
+	struct phylink_pcs pcs;
 };
 
 int xpcs_get_an_mode(struct mdio_xpcs_args *xpcs, phy_interface_t interface);
-struct mdio_xpcs_ops *mdio_xpcs_get_ops(void);
 void xpcs_validate(struct mdio_xpcs_args *xpcs, unsigned long *supported,
 		   struct phylink_link_state *state);
 int xpcs_config_eee(struct mdio_xpcs_args *xpcs, int mult_fact_100ns,