From: Heiner Kallweit <hkallweit1@gmail•com>
To: Andrew Lunn <andrew@lunn•ch>,
Russell King - ARM Linux admin <linux@armlinux•org.uk>
Cc: David Miller <davem@davemloft•net>,
f.fainelli@gmail•com, netdev@vger•kernel.org
Subject: Re: [PATCH] net: phy: marvell10g: allow PHY to probe without firmware
Date: Thu, 6 Jun 2019 20:24:23 +0200 [thread overview]
Message-ID: <16971900-e6b9-e4b7-fbf6-9ea2cdb4dc8b@gmail.com> (raw)
In-Reply-To: <20190606124218.GD20899@lunn.ch>
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@armlinux•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
next prev parent reply other threads:[~2019-06-06 18:24 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-06-05 10:43 [PATCH] net: phy: marvell10g: allow PHY to probe without firmware Russell King
2019-06-05 12:37 ` Andrew Lunn
2019-06-06 1:48 ` David Miller
2019-06-06 7:59 ` Russell King - ARM Linux admin
2019-06-06 12:31 ` Andrew Lunn
2019-06-06 12:42 ` Andrew Lunn
2019-06-06 18:24 ` Heiner Kallweit [this message]
2019-06-06 18:36 ` Andrew Lunn
2019-06-06 21:37 ` Russell King - ARM Linux admin
2019-06-10 13:40 ` Heiner Kallweit
2019-06-10 14:13 ` Russell King - ARM Linux admin
2019-06-06 21:16 ` Russell King - ARM Linux admin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=16971900-e6b9-e4b7-fbf6-9ea2cdb4dc8b@gmail.com \
--to=hkallweit1@gmail$(echo .)com \
--cc=andrew@lunn$(echo .)ch \
--cc=davem@davemloft$(echo .)net \
--cc=f.fainelli@gmail$(echo .)com \
--cc=linux@armlinux$(echo .)org.uk \
--cc=netdev@vger$(echo .)kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox