netdev - Re: [PATCH RFC net-next v2 10/17] net: stmmac: Introduce internal PCS offset-based CSR access

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <57xxremctndaz7rfmuyw3rjuz3hi7tntbaghvqda5uxngku7pl@rsekr4b5gbzr>
Date: Wed, 3 Jul 2024 21:03:41 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Halaney <ahalaney@...hat.com>, Vinod Koul <vkoul@...nel.org>, 
	Alexandre Torgue <alexandre.torgue@...s.st.com>, Jose Abreu <joabreu@...opsys.com>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, 
	Maxime Coquelin <mcoquelin.stm32@...il.com>, Alexei Starovoitov <ast@...nel.org>, 
	Jesper Dangaard Brouer <hawk@...nel.org>, John Fastabend <john.fastabend@...il.com>, 
	Daniel Borkmann <daniel@...earbox.net>, linux-arm-kernel@...ts.infradead.org, 
	linux-stm32@...md-mailman.stormreply.com, bpf@...r.kernel.org, netdev@...r.kernel.org, 
	linux-arm-msm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC net-next v2 10/17] net: stmmac: Introduce internal
 PCS offset-based CSR access

On Fri, Jun 28, 2024 at 03:54:13PM +0100, Russell King (Oracle) wrote:
> On Mon, Jun 24, 2024 at 04:26:27PM +0300, Serge Semin wrote:
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> > index 80eb72bc6311..d0bcebe87ee8 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac-qcom-ethqos.c
> > @@ -633,7 +633,7 @@ static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos)
> >  			      RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
> >  			      RGMII_IO_MACRO_CONFIG2);
> >  		ethqos_set_serdes_speed(ethqos, SPEED_2500);
> > -		stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 0, 0, 0);
> > +		stmmac_pcs_ctrl_ane(priv, priv->pcsaddr, 0, 0, 0);
> >  		break;
> >  	case SPEED_1000:
> >  		val &= ~ETHQOS_MAC_CTRL_PORT_SEL;
> > @@ -641,12 +641,12 @@ static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos)
> >  			      RGMII_CONFIG2_RGMII_CLK_SEL_CFG,
> >  			      RGMII_IO_MACRO_CONFIG2);
> >  		ethqos_set_serdes_speed(ethqos, SPEED_1000);
> > -		stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 1, 0, 0);
> > +		stmmac_pcs_ctrl_ane(priv, priv->pcsaddr, 1, 0, 0);
> >  		break;
> >  	case SPEED_100:
> >  		val |= ETHQOS_MAC_CTRL_PORT_SEL | ETHQOS_MAC_CTRL_SPEED_MODE;
> >  		ethqos_set_serdes_speed(ethqos, SPEED_1000);
> > -		stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 1, 0, 0);
> > +		stmmac_pcs_ctrl_ane(priv, priv->pcsaddr, 1, 0, 0);
> >  		break;
> >  	case SPEED_10:
> >  		val |= ETHQOS_MAC_CTRL_PORT_SEL;
> > @@ -656,7 +656,7 @@ static int ethqos_configure_sgmii(struct qcom_ethqos *ethqos)
> >  					 SGMII_10M_RX_CLK_DVDR),
> >  			      RGMII_IO_MACRO_CONFIG);
> >  		ethqos_set_serdes_speed(ethqos, SPEED_1000);
> > -		stmmac_pcs_ctrl_ane(priv, priv->ioaddr, 1, 0, 0);
> > +		stmmac_pcs_ctrl_ane(priv, priv->pcsaddr, 1, 0, 0);
> >  		break;
> >  	}
> >  
> 

> I think a better preparatory patch (given what you do in future patches)
> would be to change all of these to:
> 
> 	ethqos_pcs_set_inband(priv, {false | true});
> 
> which would be:
> 
> static void ethqos_pcs_set_inband(struct stmmac_priv *priv, bool enable)
> {
> 	stmmac_pcs_ctrl_ane(priv, priv->ioaddr, enable, 0, 0);
> }
> 
> which then means this patch becomes a single line, and your subsequent
> patch just has to replace stmmac_pcs_ctrl_ane() with its open-coded
> equivalent.

Why not. We can introduce one more preparation patch as you suggest.

> 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/hwif.c b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > index 84fd57b76fad..3666893acb69 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/hwif.c
> > @@ -6,6 +6,7 @@
> >  
> >  #include "common.h"
> >  #include "stmmac.h"
> > +#include "stmmac_pcs.h"
> >  #include "stmmac_ptp.h"
> >  #include "stmmac_est.h"
> >  
> > @@ -116,6 +117,7 @@ static const struct stmmac_hwif_entry {
> >  	const void *tc;
> >  	const void *mmc;
> >  	const void *est;
> > +	const void *pcs;
> 

> I'm not a fan of void pointers. common.h includes linux/phylink.h, which
> will define struct phylink_pcs_ops, so there is no reason not to declare
> this as:
> 
> 	const struct phylink_pcs_ops *pcs;

So am I. But in this case we have all the ops fields defined as voids.
So I just followed the local convention.

Anyway, I failed to find out the reason of using void pointers here.
So locally, in one of my cleanup patchesets, I've got them converted
to the typed pointers. I can submit that patch as another preparation
patch to this series. Then we can use the pointer to phylink_pcs_ops
with no doubts. What do you think?

-Serge(y)

> 
> -- 
> RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
> FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!