netdev - RE: [PATCH net-next v3] Add Mellanox BlueField Gigabit Ethernet driver

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: <CH2PR12MB3895318D3802419175BA42C3D7E10@CH2PR12MB3895.namprd12.prod.outlook.com>
Date:   Wed, 18 Nov 2020 14:52:50 +0000
From:   Asmaa Mnebhi <asmaa@...dia.com>
To:     Andrew Lunn <andrew@...n.ch>,
        David Thompson <davthompson@...dia.com>
CC:     "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>
Subject: RE: [PATCH net-next v3] Add Mellanox BlueField Gigabit Ethernet
 driver

Hi Andrew,

> > +static int mlxbf_gige_phy_enable_interrupt(struct phy_device *phydev)
> > +{
> > +	int err = 0;
> > +
> > +	if (phydev->drv->ack_interrupt)
> > +		err = phydev->drv->ack_interrupt(phydev);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	phydev->interrupts = PHY_INTERRUPT_ENABLED;
> > +	if (phydev->drv->config_intr)
> > +		err = phydev->drv->config_intr(phydev);
> > +
> > +	return err;
> > +}

We actually just need to make sure here, the PHY INT register is still configured properly. mlxbf_gige_phy_enable_interrupt is called right after phy_start because the PHY INT controller/status register gets cleared out after phy_start due to the call to __phy_resume. Because it gets cleared, the PHY no longer reports any interrupts.

> > +
> > +static int mlxbf_gige_phy_disable_interrupt(struct phy_device
> > +*phydev) {
> > +	int err = 0;
> > +
> > +	if (phydev->drv->ack_interrupt)
> > +		err = phydev->drv->ack_interrupt(phydev);
> > +	if (err < 0)
> > +		return err;
> > +
> > +	phydev->interrupts = PHY_INTERRUPT_DISABLED;
> > +	if (phydev->drv->config_intr)
> > +		err = phydev->drv->config_intr(phydev);
> > +
> > +	return err;
> > +}
> 
We need to call mlxbf_gige_phy_disable_interrupt from the stop routine because if we leave it enabled, and decide to call the ndo_open routine after, the interrupt coming from the PHY will be discarded until the PHY state machine is setup properly (via phy_start).

> This is, erm, interesting.
> 
> > +irqreturn_t mlxbf_gige_mdio_handle_phy_interrupt(int irq, void
> > +*dev_id) {
> > +	struct phy_device *phydev;
> > +	struct mlxbf_gige *priv;
> > +	u32 val;
> > +
> > +	priv = dev_id;
> > +	phydev = priv->netdev->phydev;
> > +
> > +	/* Check if this interrupt is from PHY device.
> > +	 * Return if it is not.
> > +	 */
> > +	val = readl(priv->gpio_io +
> > +			MLXBF_GIGE_GPIO_CAUSE_OR_CAUSE_EVTEN0);
> > +	if (!(val & MLXBF_GIGE_CAUSE_OR_CAUSE_EVTEN0_MASK))
> > +		return IRQ_NONE;
> > +
> > +	phy_mac_interrupt(phydev);
> > +
> > +	/* Clear interrupt when done, otherwise, no further interrupt
> > +	 * will be triggered.
> > +	 */
> > +	val = readl(priv->gpio_io +
> > +			MLXBF_GIGE_GPIO_CAUSE_OR_CLRCAUSE);
> > +	val |= MLXBF_GIGE_CAUSE_OR_CLRCAUSE_MASK;
> > +	writel(val, priv->gpio_io +
> > +			MLXBF_GIGE_GPIO_CAUSE_OR_CLRCAUSE);
> > +
> > +	/* Make sure to clear the PHY device interrupt */
> > +	if (phydev->drv->ack_interrupt)
> > +		phydev->drv->ack_interrupt(phydev);
> > +
> > +	phydev->interrupts = PHY_INTERRUPT_ENABLED;
> > +	if (phydev->drv->config_intr)
> > +		phydev->drv->config_intr(phydev);
> 

Yes mlxbf_gige_mdio_handle_phy_interrupt is used to check whether the interrupt is coming from GPIO12 (which is set in HW as the PHY INT_N pin). There is one HW interrupt line (here defined as MLXBF_GIGE_PHY_INT_N) shared among all the GPIOs and other components (like I2C).  
Now you mentioned that we should not be copying core PHY code. Ack_interrupt needs to be called here to make sure we clear the interrupt after clearing the Mellanox HW interrupt MLXBF_GIGE_PHY_INT_N. and config_intr needs to be called because phy_mac_interrupt causes the PHY device to clear the PHY INT enable bits. 

> And more interesting code.
> 
> We have to find a better way to do this, you should not by copying core PHY
> code into a MAC driver.
> 
> So it seems to me, the PHY interrupt you request is not actually a PHY
> interrupt. It looks more like an interrupt controller. The EVTEN0 suggests that
> there could be multiple interrupts here, of which one if the PHY? This is more
> a generic GPIO block which can do interrupts when the pins are configured as
> inputs? Or is it an interrupt controller with multiple interrupts?
> 
> Once you model this correctly in Linux, you can probably remove all the
> interesting code.
> 
>     Andrew