netdev - Re: [PATCH net-next] 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: <20200811200652.GD2141651@lunn.ch>
Date:   Tue, 11 Aug 2020 22:06:52 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Asmaa Mnebhi <asmaa@...dia.com>
Cc:     David Thompson <dthompson@...lanox.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "kuba@...nel.org" <kuba@...nel.org>,
        Jiri Pirko <jiri@...lanox.com>,
        Asmaa Mnebhi <Asmaa@...lanox.com>
Subject: Re: [PATCH net-next] Add Mellanox BlueField Gigabit Ethernet driver

On Tue, Aug 11, 2020 at 07:53:35PM +0000, Asmaa Mnebhi wrote:
> Hi Andrew,
> 
> Thanks again for your feedback.
> 
> > > +	/* Finally 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 & priv->phy_int_gpio_mask))
> > > +		return IRQ_NONE;
> > > +
> > > +	/* Clear interrupt when done, otherwise, no further interrupt
> > > +	 * will be triggered.
> > > +	 * Writing 0x1 to the clear cause register also clears the
> > > +	 * following registers:
> > > +	 * cause_gpio_arm_coalesce0
> > > +	 * cause_rsh_coalesce0
> > > +	 */
> > > +	val = readl(priv->gpio_io +
> > > +			MLXBF_GIGE_GPIO_CAUSE_OR_CLRCAUSE);
> > > +	val |= priv->phy_int_gpio_mask;
> > > +	writel(val, priv->gpio_io +
> > > +			MLXBF_GIGE_GPIO_CAUSE_OR_CLRCAUSE);
> > 
> > Shoudn't there be a call into the PHY driver at this point?
> > 
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> > 
> > So these last three functions seem to be an interrupt controller?  So why not
> > model it as a Linux interrupt controller?
> 
> Apologies for the confusion. The plan is to remove support to the polling and instead support the HW interrupt as follows (from the probe):
> irq = platform_get_irq(pdev, MLXBF_GIGE_PHY_INT_N);
>          if (irq < 0) {
>                  dev_err(dev, "Failed to retrieve irq 0x%x\n", irq);
>                  return -ENODEV;
>          }
>          priv->mdiobus->irq[phy_addr] = irq;

O.K, that is one way to do it. The other is via the MAC driver calling
phy_mac_interrupt().

> I guess my question is should we model it as a linux interrupt
> controller rather than use phy_connect_direct ?

It seems like there are other interrupt sources, not just the PHY. Do
you plan to use any of them? It can be easier to debug issues if you
have an interrupt controller, can see counters in /proc/interrupts,
etc. Also, if you need to export the lines to some other driver,
e.g. SFP, it is easier to do when there is an interrupt controller.

> Using phy_connect_direct to register my interrupt handler, I have
> encountered a particular issue where the PHY interrupt is triggered
> before the phy link status bit (reg 0x1 of the PHY device) is set to
> 1 (indicating link is up).

So the hardware is broken :-(

What about the other way, link down? Same problem?

Polling is probably your best bet, since it is robust against broken
interrupts. If i remember correctly, this is an off the shelf 1G PHY?
Microchip? Is there an errata for this? Maybe the errata suggests a
work around?

     Andrew