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 for Android: free password hash cracker in your pocket

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

Message-ID: <20201118030950.GB1804098@lunn.ch>
Date:   Wed, 18 Nov 2020 04:09:50 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     David Thompson <davthompson@...dia.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        Asmaa Mnebhi <asmaa@...dia.com>
Subject: Re: [PATCH net-next v3] Add Mellanox BlueField Gigabit Ethernet
 driver

Hi David

> +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;
> +}
> +
> +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;
> +}

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);

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