[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <6b8ec5fe-ca93-d2cf-3060-4f087fcdc85a@denx.de>
Date: Fri, 9 Oct 2020 19:34:10 +0200
From: Marek Vasut <marex@...x.de>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Florian Fainelli <f.fainelli@...il.com>, netdev@...r.kernel.org,
Christoph Niedermaier <cniedermaier@...electronics.com>,
"David S . Miller" <davem@...emloft.net>,
NXP Linux Team <linux-imx@....com>,
Richard Leitner <richard.leitner@...data.com>,
Shawn Guo <shawnguo@...nel.org>, Andrew Lunn <andrew@...n.ch>,
Heiner Kallweit <hkallweit1@...il.com>
Subject: Re: [PATCH] net: fec: Fix phy_device lookup for
phy_reset_after_clk_enable()
On 10/9/20 5:15 PM, Jakub Kicinski wrote:
> On Fri, 9 Oct 2020 09:20:30 +0200 Marek Vasut wrote:
>>> Can you describe your problem in detail?
>>
>> Yes, I tried to do that in the commit message and the extra detailed
>> comment above the code. What exactly do you not understand from that?
>
> Why it's not bound on the first open
It is getting bound on the first open. The problem is in probe(), where
fec_enet_clk_enable(ndev, true) [yes, the name of that function is bad]
calls fec_enet_phy_reset_after_clk_enable() and the ndev->phydev is NULL
while there is already existing instance of that phydev .
So this patch adds this extra look up to get the phydev, which then
permits phy_reset_after_clk_enable() to call phy_device_reset() instead
of returning -ENODEV.
> (I'm guessing it's the first open
> that has the ndev->phydev == NULL? I shouldn't have to guess).
If I had a crystal ball that'd tell me all the review questions up
front, I would write perfect patches with all the feedback sorted out in
V1. Sorry, I don't have one ...
>>> To an untrained eye this looks pretty weird.
>>
>> I see, I'm not quite sure how to address this comment.
>
> If ndev->phydev sometimes is not-NULL on open, then that's a valid
> state to be in. Why not make sure that we're always in that state
> and can depend on ndev->phydev rather than rummaging around for
> the phy_device instance.
Nope, the problem is in probe() in this case.
Powered by blists - more mailing lists