netdev - Re: [PATCH 2/2] net: fec: Create device link between phy dev and mac dev

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: <Y3V5AgBMBOx/ptwx@lunn.ch>
Date:   Thu, 17 Nov 2022 00:57:54 +0100
From:   Andrew Lunn <andrew@...n.ch>
To:     Florian Fainelli <f.fainelli@...il.com>
Cc:     Xiaolei Wang <xiaolei.wang@...driver.com>, hkallweit1@...il.com,
        linux@...linux.org.uk, davem@...emloft.net, edumazet@...gle.com,
        kuba@...nel.org, pabeni@...hat.com, netdev@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] net: fec: Create device link between phy dev and mac
 dev

On Wed, Nov 16, 2022 at 03:27:39PM -0800, Florian Fainelli wrote:
> On 11/16/22 07:07, Andrew Lunn wrote:
> > On Wed, Nov 16, 2022 at 10:43:05PM +0800, Xiaolei Wang wrote:
> > > On imx6sx, there are two fec interfaces, but the external
> > > phys can only be configured by fec0 mii_bus. That means
> > > the fec1 can't work independently, it only work when the
> > > fec0 is active. It is alright in the normal boot since the
> > > fec0 will be probed first. But then the fec0 maybe moved
> > > behind of fec1 in the dpm_list due to various device link.
> 
> Humm, but if FEC1 depends upon its PHY to be available by the FEC0 MDIO bus
> provider, then surely we will need to make sure that FEC0's MDIO bus is
> always functional, and that includes surviving re-ordering as well as any
> sort of run-time power management that can occur.

Runtime PM is solved for the FECs MDIO bus, because there are switches
hanging off it, which have their own life cycle independent of the
MAC. This is something i had to fix many moons ago, when the FEC would
power off the MDIO bus when the interface is admin down, stopping
access to the switch. The mdio read and write functions now do run
time pm get and put as needed.

I've never done suspend/resume with a switch, it is not something
needed in the use cases i've covered.

> > > So in system suspend and resume, we would get the following
> > > warning when configuring the external phy of fec1 via the
> > > fec0 mii_bus due to the inactive of fec0. In order to fix
> > > this issue, we create a device link between phy dev and fec0.
> > > This will make sure that fec0 is always active when fec1
> > > is in active mode.
> 
> Still not clear to me how the proposed fix works, let alone how it does not
> leak device links since there is no device_link_del(), also you are going to
> be creating guaranteed regressions by putting that change in the PHY
> library.

The reference leak is bad, but i think phylib is the correct place to
fix this general issue. It is not specific to the FEC. There are other
boards with dual MAC SoCs and they save a couple of pins by putting
both PHYs on one MDIO bus. Having the link should help better
represent the device tree so that suspend/resume can do stuff in the
right order.

      Andrew