netdev - RE: [PATCH net-next v2 3/7] net: pcs: xpcs: add 1000BASE-X AN interrupt support

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>] [day] [month] [year] [list]

Message-ID: <014b01d9d01a$7e79aa10$7b6cfe30$@trustnetic.com>
Date: Wed, 16 Aug 2023 16:20:19 +0800
From: Jiawen Wu <jiawenwu@...stnetic.com>
To: "'Russell King \(Oracle\)'" <linux@...linux.org.uk>
Cc: <netdev@...r.kernel.org>,
	<davem@...emloft.net>,
	<edumazet@...gle.com>,
	<kuba@...nel.org>,
	<pabeni@...hat.com>,
	<andrew@...n.ch>,
	<hkallweit1@...il.com>,
	<Jose.Abreu@...opsys.com>,
	<mengyuanlou@...-swift.com>
Subject: RE: [PATCH net-next v2 3/7] net: pcs: xpcs: add 1000BASE-X AN interrupt support

On Tuesday, August 8, 2023 4:47 PM, Jiawen Wu wrote:
> On Tuesday, August 8, 2023 4:21 PM, Russell King (Oracle) wrote:
> > On Tue, Aug 08, 2023 at 10:17:04AM +0800, Jiawen Wu wrote:
> > > Enable CL37 AN complete interrupt for DW XPCS. It requires to clear the
> > > bit(0) [CL37_ANCMPLT_INTR] of VR_MII_AN_INTR_STS after AN completed.
> > >
> > > And there is a quirk for Wangxun devices to enable CL37 AN in backplane
> > > configurations because of the special hardware design.
> >
> > Where is the interrupt handler?
> 
> PCS interrupt is directly connected to the PCI interrupt on the board, so the
> interrupt handler is txgbe_irq_handler() in the ethernet driver.
> 
> >
> > > @@ -759,6 +762,8 @@ static int xpcs_config_aneg_c37_1000basex(struct dw_xpcs *xpcs,
> > >  		return ret;
> > >
> > >  	ret &= ~DW_VR_MII_PCS_MODE_MASK;
> > > +	if (!xpcs->pcs.poll)
> > > +		ret |= DW_VR_MII_AN_INTR_EN;
> >
> > Does this interrupt only work in 1000baseX mode?
> 
> AN interrupt now only be implemented in 1000baseX mode.
> 
> >
> > >  	ret = xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_CTRL, ret);
> > >  	if (ret < 0)
> > >  		return ret;
> > > @@ -1012,6 +1017,17 @@ static int xpcs_get_state_c37_1000basex(struct dw_xpcs *xpcs,
> > >  		if (bmsr < 0)
> > >  			return bmsr;
> > >
> > > +		/* Clear AN complete interrupt */
> > > +		if (!xpcs->pcs.poll) {
> > > +			int an_intr;
> > > +
> > > +			an_intr = xpcs_read(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS);
> > > +			if (an_intr & DW_VR_MII_AN_STS_C37_ANCMPLT_INTR) {
> > > +				an_intr &= ~DW_VR_MII_AN_STS_C37_ANCMPLT_INTR;
> > > +				xpcs_write(xpcs, MDIO_MMD_VEND2, DW_VR_MII_AN_INTR_STS, an_intr);
> > > +			}
> > > +		}
> > > +
> >
> > get_state isn't supposed to be used as a way to acknowledge interrupts,
> > because that will get called quite a bit later after the interrupt has
> > been received.
> 
> I think it's just to clear the AN complete bit here. Actually, ethernet driver
> handle interrupt and call phylink_mac_change() to check PCS state. It does
> get called later, though.
> 
> >
> > As an example of PCS that use interrupts, please see the converted
> > mv88e6xxx PCS, for example:
> >
> >  drivers/net/dsa/mv88e6xxx/pcs-6352.c
> >
> > If the interrupt handler for the PCS is threaded, then it can access
> > the DW_VR_MII_AN_INTR_STS register to acknowledge the interrupt and
> > call phylink_pcs_change() or phylink_mac_change().
> 
> I'll check the usage of this method, thanks.

If interrupt handler is to be implemented in PCS, the codes about interrupts in
txgbe driver needs to be all refactored. This will affect GPIO, SFP... 

Could I refactor these codes in a future patch, and keep the current IRQ structure
in this series?