netdev - Re: [PATCH RFC net-next v2 3/8] net: stmmac: dwmac1000: convert sgmii/rgmii "pcs" to phylink

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: <dz34gg4atjyha5dc7tfgbnmsfku63r7faicyzo3odkllq3bqin@hho3kj5wmaat>
Date: Tue, 11 Jun 2024 15:25:14 +0300
From: Serge Semin <fancer.lancer@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>, 
	Andrew Halaney <ahalaney@...hat.com>
Cc: Andrew Halaney <ahalaney@...hat.com>, 
	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>, netdev@...r.kernel.org, linux-stm32@...md-mailman.stormreply.com, 
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCH RFC net-next v2 3/8] net: stmmac: dwmac1000: convert
 sgmii/rgmii "pcs" to phylink

Hi Russell, Andrew

On Mon, Jun 10, 2024 at 10:19:39AM +0100, Russell King (Oracle) wrote:
> On Wed, Jun 05, 2024 at 03:05:43PM -0500, Andrew Halaney wrote:
> > On Fri, May 31, 2024 at 12:26:25PM GMT, Russell King (Oracle) wrote:
> > > @@ -335,8 +303,12 @@ static int dwmac1000_irq_status(struct mac_device_info *hw,
> > >  

> > >  	dwmac_pcs_isr(ioaddr, GMAC_PCS_BASE, intr_status, x);
> > >  
> > > -	if (intr_status & PCS_RGSMIIIS_IRQ)
> > > -		dwmac1000_rgsmii(ioaddr, x);
> > > +	if (intr_status & PCS_RGSMIIIS_IRQ) {
> > > +		/* TODO Dummy-read to clear the IRQ status */
> > > +		readl(ioaddr + GMAC_RGSMIIIS);
> > 
> > This seems to me that you're doing the TODO here? Maybe I'm
> > misunderstanding... maybe not :)
> 
> Please trim your replies.
> 
> These two lines come from Serge - please ask him why it's marked as a
> TODO. I assume he has a reason. Thanks.

The statement below the "TODO..." comment was supposed to be a
quick-fix of the interrupts flood happening due to the uncleared
RGSMIIIS IRQ flag. Of course dummy-reading in the IRQ handler with no
action required to handle the IRQ wouldn't be the best solution
(despite of having the phylink_pcs_change() called), especially seeing there
is the dwmac_pcs_isr() method, which name implies the PCS IRQ
handling. At least we could have incremented the
stmmac_extra_stats::irq_rgmii_n counter in there. So what I meant TODO here was
to move the RGSMIIIS IRQ handling in dwmac_pcs_isr().

I know that the dwmac_pcs_isr() method has been created around the
cross-IP-cores PCS implementation, but as I mentioned several times
the tx_config_reg[15:0] part of the
GMAC_RGSMIIIS/MAC_PHYIF_Control_Status registers is the same on both
DW GMAC and DW QoS Eth:
tx_config_reg[0]:   LNKMOD
tx_config_reg[1:2]: LNKSPEED
tx_config_reg[3]:   LNKSTS
tx_config_reg[4]:   JABTO (Jabber Timeout, specific to SMII)
tx_config_reg[5]:   FALSCARDET (False Carrier Detected, specific to SMII)

Should we have a DW IP-core-specific getter like
stmmac_ops::pcs_get_config_reg() which would return the
tx_config_reg[15:0] field then we could have cleared the IRQ by just
calling it, we could have had the fields generically
parsed in the dwmac_pcs_isr() handler and in the
phylink_pcs_ops::pcs_get_state(). Thus the entire struct
phylink_pcs_ops definition could be moved to the stmmac_pcs.c module
simplifying the DW GMAC and DW QoS Eth hardware-dependent code.

In this regard there is another change which would be required (and
frankly would make the code simpler). Instead of passing the
CSRs-base address to the
dwmac_pcs_isr()/dwmac_pcs_config()/dwmac_pcs_get_state() methods, we
could pre-define the PCS registers base address as it's done for the
PTP/MMC/EST implementation in the driver. Here is the brief change
description:
1. add stmmac_regs_off::pcs_off field (hwif.h)
2. add stmmac_priv::pcsaddr field (stmmac.h)
3. initialize the stmmac_regs_off::pcs_off field for the DW GMAC and
DW QoS Eth IP-cores in the stmmac_hw array (hwif.c)
4. initialize the stmmac_priv::pcsaddr field in the stmmac_hwif_init()
method as it's done for stmmac_priv::{ptpaddr,mmcaddr,estaddr}.
5. use the PCS-base address in the stmmac_pcs.c module.

As a result (unless I've missed something) we'll be able to move
almost the entire internal PCS implementation to the stmmac_pcs.c
module (except the tx_config_reg[] data getter).

-Serge(y)

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