netdev - Re: [PATCH] net: phy: marvell10g: allow PHY to probe without firmware

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:   Thu, 6 Jun 2019 22:16:25 +0100
From:   Russell King - ARM Linux admin <linux@...linux.org.uk>
To:     Heiner Kallweit <hkallweit1@...il.com>
Cc:     Andrew Lunn <andrew@...n.ch>, David Miller <davem@...emloft.net>,
        f.fainelli@...il.com, netdev@...r.kernel.org
Subject: Re: [PATCH] net: phy: marvell10g: allow PHY to probe without firmware

On Thu, Jun 06, 2019 at 08:24:23PM +0200, Heiner Kallweit wrote:
> On 06.06.2019 14:42, Andrew Lunn wrote:
> > On Thu, Jun 06, 2019 at 08:59:19AM +0100, Russell King - ARM Linux admin wrote:
> >> On Wed, Jun 05, 2019 at 06:48:27PM -0700, David Miller wrote:
> >>> From: Russell King <rmk+kernel@...linux.org.uk>
> >>> Date: Wed, 05 Jun 2019 11:43:16 +0100
> >>>
> >>>> +	    (state == PHY_UP || state == PHY_RESUMING)) {
> >>>
> >>> drivers/net/phy/marvell10g.c: In function ‘mv3310_link_change_notify’:
> >>> drivers/net/phy/marvell10g.c:268:35: error: ‘PHY_RESUMING’ undeclared (first use in this function); did you mean ‘RPM_RESUMING’?
> >>>       (state == PHY_UP || state == PHY_RESUMING)) {
> >>>                                    ^~~~~~~~~~~~
> >>>                                    RPM_RESUMING
> >>> drivers/net/phy/marvell10g.c:268:35: note: each undeclared identifier is reported only once for each function it appears in
> >>> At top level:
> >>> drivers/net/phy/marvell10g.c:262:13: warning: ‘mv3310_link_change_notify’ defined but not used [-Wunused-function]
> >>>  static void mv3310_link_change_notify(struct phy_device *phydev)
> >>>              ^~~~~~~~~~~~~~~~~~~~~~~~~
> >>
> >> Hmm. Looks like Heiner's changes in net-next _totally_ screw this
> >> approach - it's not just about PHY_RESUMING being removed, it's
> >> also about the link change notifier being moved. :(
> > 
> > Hi Russell
> > 
> > The link change notifier still seems to be called, and it is still
> > part of the phy_driver structure.
> > 
> Before my change the link change notifier didn't do what the name states.
> It was an "I'm going to run the state machine now, and something may
> have changed or not" callback.

Sure, but you've made an unexpected semantic change to the callback,
which means patches can be applied (even back-ported) that may depend
on one behaviour or the other, which will end up introducing bugs.

> Still we have state changes happening outside the state machine and
> therefore not calling the link change notifier. This brings me to the
> second point:
> 
> I don't like too much state changes outside control of the state machine,
> like in phy_start / phy_stop / phy_error. I think it would be better
> if a state change request is sent to the state machine, and the state
> machine decides whether the requested transition is allowed.

I do like to waste my time creating patches _after_ a discussion about
how this should be handled, writing code, getting people to test, and
then submitting the patches.  It makes me feel like a valued individual
when I end up wasting both my time and others time like that.

However, there is now a bigger issue - the person who reported the
issue has now programmed firmware into their PHYs, so can no longer
test.  So, whatever we now come up with has to be just submitted
without testing that it actually works.

> Coming to the use case of keeping the link down if the firmware isn't
> loaded. I'm not sure whether the firmware is needed for all modes, or
> whether e.g. basic modes like 100BaseT work also w/o firmware.
> Instead of manually changing the state it may be better to remove all
> modes needing the firmware from supported and advertising bitmap in
> the config_init callback.
> Then modes not needing the firmware can still be used, and if no mode
> remains then nothing is advertised and the link stays down anyway.

We've lost the ability to determine that now.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up