Re: [PATCH 3/3] net: phy: mediatek-ge: do not disable EEE advertisement

[Arınç ÜNAL]

On 20.03.2024 23:53, Russell King (Oracle) wrote:
> On Wed, Mar 20, 2024 at 11:40:56PM +0300, Arınç ÜNAL wrote:
> > On 18.03.2024 10:46, Arınç ÜNAL via B4 Relay wrote:
> > Can I get an opinion on this? Is it actually possible that the PHY driver
> > would start probing after the DSA subdriver? On the console logs for the
> > DSA subdriver, I can see that the name of the PHY driver will appear, which
> > makes me believe the PHY driver would actually never probe after the DSA
> > subdriver.
> >
> > [    4.402641] mt7530-mdio mdio-bus:1f wan (uninitialized): PHY [mt7530-0:00] driver [MediaTek MT7530 PHY] (irq=POLL)
> > [    4.420392] mt7530-mdio mdio-bus:1f lan0 (uninitialized): PHY [mt7530-0:01] driver [MediaTek MT7530 PHY] (irq=POLL)
> > [    4.437791] mt7530-mdio mdio-bus:1f lan1 (uninitialized): PHY [mt7530-0:02] driver [MediaTek MT7530 PHY] (irq=POLL)
> > [    4.455096] mt7530-mdio mdio-bus:1f lan2 (uninitialized): PHY [mt7530-0:03] driver [MediaTek MT7530 PHY] (irq=POLL)
> > [    4.472422] mt7530-mdio mdio-bus:1f lan3 (uninitialized): PHY [mt7530-0:04] driver [MediaTek MT7530 PHY] (irq=POLL)
> >
> > I don't want to submit a bugfix to the net tree if the bug won't ever
> > happen in real life.
>
> It would be really great if you could tell us which bug fixes you're
> submitting are for a real problem that you or a user have encountered,
> and which are down to essentially code inspection and things that
> "aren't correct". Basically, don't do this.

I agree. Patch 1 fixes a real problem, patch 2 "fixes" a problem found with
code inspection. Though, it would be great if you could review patch 2.

> It isn't true that the PHY specific driver will be probed before DSA
> initialises - consider the case where the DSA driver is built-in but
> the PHY specific driver is modular and on the not-yet-mounted rootfs.
> That would result in the generic PHY driver being used even when the
> PHY specific driver were to be loaded later - and thus only basic
> standard 802.3 PHY behaviour will be supported.
>
> That's not specific to mt7530, it applies to everything that uses
> phylib. It isn't something that really warrants "bug fixing" in each
> and every driver.

That makes sense. But there's a special case with the MT7530 DSA subdriver
and mediatek-ge driver. The PHY driver is needed for the PHYs to function
properly. So the DSA subdriver forces mediatek-ge to be selected [1]. So
the PHY driver could only be compiled as a module when the DSA subdriver is
also compiled so. And that designates mediatek-ge as a dependency for the
DSA subdriver, if I understand correctly.

[1] https://git.kernel.org/pub/scm/linux/kernel/git/netdev/net-next.git/commit/?id=fb4bb62aaac715e50c7c007714af19a2698db88b