netdev - Re: [PATCH] net: phy: broadcom: Wire suspend/resume for BCM54612E

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]

Message-ID: <999020b2-692b-4582-8ca0-e19c7b45ee92@gmail.com>
Date: Thu, 2 Nov 2023 20:39:55 -0700
From: Florian Fainelli <f.fainelli@...il.com>
To: Marco von Rosenberg <marcovr@...fnet.de>, Andrew Lunn <andrew@...n.ch>
Cc: Florian Fainelli <florian.fainelli@...adcom.com>,
 Broadcom internal kernel review list
 <bcm-kernel-feedback-list@...adcom.com>,
 Heiner Kallweit <hkallweit1@...il.com>, Russell King
 <linux@...linux.org.uk>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] net: phy: broadcom: Wire suspend/resume for BCM54612E

On 11/2/2023 6:47 PM, Marco von Rosenberg wrote:
> On Wednesday, November 1, 2023 11:06:56 PM CET Andrew Lunn wrote:
>> On Wed, Nov 01, 2023 at 10:42:52PM +0100, Marco von Rosenberg wrote:
>>> On Tuesday, October 31, 2023 1:31:11 AM CET Andrew Lunn wrote:
>>>> Are we talking about a device which as been suspended? The PHY has
>>>> been left running because there is no suspend callback? Something then
>>>> triggers a resume. The bootloader then suspends the active PHY? Linux
>>>> then boots, detects its a resume, so does not touch the hardware
>>>> because there is no resume callback? The suspended PHY is then
>>>> useless.
>>>
>>> Hi Andrew,
>>>
>>> thanks for your feedback. I guess a bit of context is missing here. The
>>> issue has nothing to do with an ordinary suspension of the OS. The main
>>> point is that on initial power-up, the bootloader suspends the PHY before
>>> booting Linux. With a resume callback defined, Linux would call it on
>>> boot and make the PHY usable.
>>
>> Ah, so you rely on phy_attach_direct() calling phy_resume(phydev).
>>
>> This seems an odd way to solve the problem. It was not Linux which
>> suspend the PHY, so using resume is asymmetric.
>>
>> I think soft_reset() or config_init() should be taking the PHY out of
>> suspend.

We have an unconditional call to __phy_resume() in phy_start() and we 
should always have a call to phy_start() regardless of the path though 
you have a point Andrew that we should ensure that by the time 
phy_init_hw() is called we have taken the device out of IDDQ-SR.

> 
> I agree with all of your points. This is just one way which happens to solve
> this specific problem. Of course it might be asymmetric to see the patch as
> a solution to my problem. However is there anything fundamentally wrong with
> adding suspend/resume callbacks? I see some other drivers having these
> callbacks defined and some not (it seems a bit inconsistent throughout the
> drivers in broadcom.c to be honest).
> 
> I'm wondering if I should just omit this whole "motivation" paragraph in the
> commit message and just use the commit message of commit 38b6a9073007 ("net:
> phy: broadcom: Wire suspend/resume for BCM50610 and BCM50610M") as a template.
> I mean, regardless of my motivation, I would say it makes sense for this PHY
> to support suspend and resume.

I would remove the motivation aspect from the paragraph and we could 
also improve the driver a bit to ensure that IDDQ-SR is disabled upon 
config_init(). Other than that your patch is just fine with me. Can you 
re-submit in a few days when net-next opens again?

Thanks!
-- 
Florian