netdev - Re: Help on PHY not supporting autoneg

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]

Date:   Wed, 30 Nov 2022 16:32:13 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Piergiorgio Beruto <piergiorgio.beruto@...il.com>
Cc:     netdev@...r.kernel.org
Subject: Re: Help on PHY not supporting autoneg

On Wed, Nov 30, 2022 at 01:16:02PM +0100, Piergiorgio Beruto wrote:
> Hello netdev group,
> I am looking for someone willing to help me on a problem I encountered
> while trying to implement a driver for a PHY that does not support
> autoneg. On my reference HW, the PHY is connected to a stmmac via MII.
> 
> This is what I did in the driver:
> - implement get_features to report NO AN support, and the supported link
>   modes.
> - implement the IRQ handling (config_intr and handle_interrupt) to
>   report the link status
> 
> The first problem I encountered is: how to start the PHY? The device
> requires a bit (LINK_CONTROL) to be enabled before trying to bring up
> the link. But I could not find any obvious callback from phylib to
> actually do this. Eventually, I opted for implementing the
> suspend/resume callbacks and set that bit in there. Is that right? Any
> better option?

The MAC driver should call phy_start() in its open function.  That
kicks off the state machine. That results in phy_start_aneg(),
_phy_start_aneg(), phy_config_aneg(), which calls the PHY drivers
config_aneg() method. It does not matter here that the PHY does not
actually support AN, this method still gets called. The driver can
then look at the phydev members and configure the hardware as needed.
 
> With that said, the thing still does not work as I would expect. When I
> ifconfig up my device, here's what happens (the ncn_* prints are just
> debug prinktks I've added to show the problem). See also my comments in
> the log marked with //
> 
> /root # ifconfig eth0 up
> [   26.950557] socfpga-dwmac ff700000.ethernet eth0: Register MEM_TYPE_PAGE_POOL RxQ-0
> [   26.962673] ncn_soft_reset
> [   26.966345] ncn_config_init
> [   26.969168] ncn_config_intr
> [   26.972211] disable IRQs   // OK, this is expected, phylib is resetting the device
> [   26.975062] ncn_resume     // not sure I expected this to be here, but it does not harm
> [   26.977746] socfpga-dwmac ff700000.ethernet eth0: PHY [stmmac-0:08] driver [NCN26000] (irq=49)
> [   26.986861] ncn_config_intr
> [   26.990045] ncn_config_intr ret = 8000, irqs = 0001
> [   26.994958] ncn_handle_interrupt 8000
> [   26.998941] ncn_handle_interrupt 8001
> [   27.002752] link = 1, ret = 0829       // there I get a link UP!
> [   27.016526] dwmac1000: Master AXI performs any burst length
> [   27.022128] socfpga-dwmac ff700000.ethernet eth0: No Safety Features support found
> [   27.029999] socfpga-dwmac ff700000.ethernet eth0: IEEE 1588-2008 Advanced Timestamp supported
> [   27.039425] socfpga-dwmac ff700000.ethernet eth0: registered PTP clock
> [   27.049388] socfpga-dwmac ff700000.ethernet eth0: configuring for phy/mii link mode
> [   27.057155] ncn_resume  // I don't fully understand what happened since the link up, but it seems the MAC is doing more initialization

This looks odd. I would not expect a resume here. Add a WARN_ON(1) to
get a stack trace and see if that helps explain why. My guess would
be, you somehow end up in socfpga_dwmac_resume().

> [   27.061251] ncn_handle_interrupt 8001
> [   27.065100] link = 0, ret = 0809 // here I get a link down (???)
> 
> >From there on, if I read the LINK_CONTROL bit, someone set it to 0 (???)

You can add a WARN_ON(regnum==0) in phy_write() to get a stack trace.

> I've also tried a completely different approach, that is "emulating"
> autoneg by implementing the config_aneg, aneg_done and read_status
> callbacks. If I do this, then my driver works just fine and nobody seems
> to be overwriting register 0.

That is not emulating. The config_aneg() just has a bad name.

If you have not provided a config_aneg() the default implementation is
used, __genphy_config_aneg(), which will call genphy_setup_forced():

int genphy_setup_forced(struct phy_device *phydev)
{
	u16 ctl;

	phydev->pause = 0;
	phydev->asym_pause = 0;

	ctl = mii_bmcr_encode_fixed(phydev->speed, phydev->duplex);

	return phy_modify(phydev, MII_BMCR,
			  ~(BMCR_LOOPBACK | BMCR_ISOLATE | BMCR_PDOWN), ctl);

What exactly is LINK_CONTROL. It is not one of the Linux names for a
bit in BMCR.

    Andrew