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 20:24:23 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Andrew Lunn <andrew@...n.ch>,
        Russell King - ARM Linux admin <linux@...linux.org.uk>
Cc:     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 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.
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.
But I didn't dig deep enough into a possible solution yet.

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.

> Please could you be more specific about what changes Heiner made which
> causes this patch problems?
> 
>        Thanks
> 	Andrew
> 
Heiner