netdev - Re: [PATCH net-next v2 3/3] tsnep: Add TSN endpoint Ethernet MAC 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: <YTFAc/vMXTKdFSHL@lunn.ch>
Date:   Thu, 2 Sep 2021 23:21:55 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Gerhard Engleder <gerhard@...leder-embedded.com>
Cc:     David Miller <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        netdev <netdev@...r.kernel.org>, devicetree@...r.kernel.org
Subject: Re: [PATCH net-next v2 3/3] tsnep: Add TSN endpoint Ethernet MAC
 driver

On Thu, Sep 02, 2021 at 10:32:10PM +0200, Gerhard Engleder wrote:
> > > > > +static irqreturn_t tsnep_irq(int irq, void *arg)
> > > > > +{
> > > > > +     struct tsnep_adapter *adapter = arg;
> > > > > +     u32 active = ioread32(adapter->addr + ECM_INT_ACTIVE);
> > > > > +
> > > > > +     /* acknowledge interrupt */
> > > > > +     if (active != 0)
> > > > > +             iowrite32(active, adapter->addr + ECM_INT_ACKNOWLEDGE);
> > > > > +
> > > > > +     /* handle management data interrupt */
> > > > > +     if ((active & ECM_INT_MD) != 0) {
> > > > > +             adapter->md_active = false;
> > > > > +             wake_up_interruptible(&adapter->md_wait);
> > > > > +     }
> > > > > +
> > > > > +     /* handle link interrupt */
> > > > > +     if ((active & ECM_INT_LINK) != 0) {
> > > > > +             if (adapter->netdev->phydev) {
> > > > > +                     struct phy_device *phydev = adapter->netdev->phydev;
> > > > > +                     u32 status = ioread32(adapter->addr + ECM_STATUS);
> > > > > +                     int link = (status & ECM_NO_LINK) ? 0 : 1;
> > > > > +                     u32 speed = status & ECM_SPEED_MASK;
> > > >
> > > > How does PHY link and speed get into this MAC register? Is the MAC
> > > > polling the PHY over the MDIO bus? Is the PHY internal to the MAC and
> > > > it has backdoor access to the PHY status?
> > >
> > > PHY is external. The MAC expects additional signals for link status. These
> > > signals can be derived from RGMII in band signaling of the link status or by
> > > using PHY link and speed LED outputs. The MAC is using the link status for
> > > a quick no link reaction to minimize the impact to real time applications.
> > > EtherCAT for example also uses the link LED output for a no link reaction
> > > within a few microseconds.
> >
> > O.K. This is not the normal Linux way. You normally have the PHY
> > driver tell the PHY core, which then tells the MAC driver. That always
> > works. RGMII in band signaling is not supported by all PHY devices,
> > and the board design would require the LED output are correctly
> > connected, and i guess you need a hacked PHY driver to use the correct
> > LED meanings? Plus i guess you have additional changes in the PHY
> > driver to do fast link down detection?
> 
> Yes, LED outputs must be correctly connected in the board design. LED
> outputs are usually configured with strapping pins, which again require a
> correct board design.

Linux sometime, maybe soon, will be able the control the PHY LEDs, and
probably export them to user space so root can change their meaning.

> Fast link down detection is a hardware property of the selected
> PHY. So far no PHY driver changes were necessary.

Marvell PHYs for example follow 802.3 C40 and default to waiting 750ms
before reporting the link down. You can configure them to only wait
10ms, 20ms or 40ms. So it sounds like you are using a PHY which does
not conform to C40? In general, we probably need to be able to
configure this, for those that do follow C40.

> > I think this needs another DT property to enable using such short
> > cuts, and you should use the Linux way by default.
> 
> Isn't choosing PHY_MAC_INTERRUPT also the Linux way? I preferred it
> over PHY_POLL, because I need the link information directly in the MAC
> anyway. But maybe the speed information is too much and should be provided
> to the MAC.

PHY_MAC_INTERRUPT is just the first step. It means something happened
in the PHY. You need to ask the PHY what? It could be link up or down,
it could be cable diagnostics have finished, the temperature is
getting too hot, whatever can cause the PHY to change state. The PHY
driver will then determine what has actually happened. Some cases, the
MAC does not needed to know. Others the MAC will be told, via the
callback it registered. It gets to know the link speed, up down etc.
That is the Linux way, the complete chain.

> > Also, don't you need a property which tells you to either use RGMII
> > inband, or LED signals?
> 
> No, this decision is done in VHDL/FPGA. No need to consume precious FPGA
> resources for runtime configuration.

You mean you have two ways to synthesis the MAC. You have two
bitstreams. One for LEDs and one of inband RGMII?

> I'm afraid that relying on ACPI is not always an option. x86 CPU modules are
> very often used in industrial automation and the BIOS of the CPU module is
> usually not adapted to the carrier board.

Yes, i've been there. I have managed to get the BIOS customised, but
it is not easy. DT is much easier to use.

> Also other drivers implement a fallback like this. Shall I still
> remove it?

You can keep it. I just don't recommend it, if you can avoid it. But x86...

    Andrew