netdev - Re: [PATCH v4] drivers/net/phy: add driver for Microchip LAN867x 10BASE-T1S PHY

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: <13cd631b-d368-44c0-a977-55c8a05e396d@lunn.ch>
Date:   Fri, 21 Apr 2023 14:39:24 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Parthiban.Veerasooran@...rochip.com
Cc:     ramon.nordin.rodriguez@...roamp.se, hkallweit1@...il.com,
        linux@...linux.org.uk, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
        olteanv@...il.com, Jan.Huber@...rochip.com
Subject: Re: [PATCH v4] drivers/net/phy: add driver for Microchip LAN867x
 10BASE-T1S PHY

> Do you think it makes sense to probe the driver based on the Revision 
> number as well? because we have two more revisions in the pipeline and 
> each revision has different settings for the initial configuration. If 
> you agree with this, as per the current datasheet this PHY revision is 2 
> means Rev.B1. If you like, I would suggest to use the PHY_ID define as 
> below,

Do the current settings not work at all on future silicon? The words
'optimal performance' have been used when describing what this magic
does. Which suggest it should work, but not optimally?

Unless this is going to cause the magic smoke to escape, i say leave
it as it is until you add explicit support for those revisions.

> > +       /* None of the interrupts in the lan867x phy seem relevant.
> > +        * Other phys inspect the link status and call phy_trigger_machine
> > +        * in the interrupt handler.
> > +        * This phy does not support link status, and thus has no interrupt
> > +        * for it either.
> > +        * So we'll just disable all interrupts on the chip.
> > +        */
> I could see the below in the datasheet section 4.7.
> 
> When the device is in a reset state, the IRQ_N interrupt pin is 
> high-impedance and will be pulled high through an
> external pull-up resistor. Once all device reset sources are deasserted, 
> the device will begin its internal initialization.
> The device will assert the Reset Complete (RESETC) bit in the Status 2 
> (STS2) register to indicate that it has
> completed its internal initialization and is ready for configuration. As 
> the Reset Complete status is non-maskable, the
> IRQ_N pin will always be asserted and driven low following a device reset
> 
> Do you think it makes sense to clear/acknowledge the "reset_done" 
> interrupt by reading the Status 2 register in the interrupt routine?

When interrupt support is added, you should do this. It is normal to
clear all pending interrupt before enabling interrupts, since you
don't want to handle stale interrupts.

The only potential issue here is when somebody has a board which mixes
multiple different PHYs using a shared interrupt. This PHY is going to
block all other PHYs, which maybe do have working interrupt
support. But there is no regression here, it has never worked. So it
is not something which the first version of the driver needs to
handle.

	Andrew