netdev - Re: [PATCH] r8169: do not call rtl8169

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: <74a498d0-343f-46f1-ad95-2651d960d657@gmail.com>
Date: Fri, 18 Apr 2025 23:52:19 +0200
From: Heiner Kallweit <hkallweit1@...il.com>
To: Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>,
 Niklas Cassel <cassel@...nel.org>
Cc: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>,
 nic_swsd@...ltek.com, Andrew Lunn <andrew+netdev@...n.ch>,
 "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
Subject: Re: [PATCH] r8169: do not call rtl8169_down() twice on shutdown

On 18.04.2025 19:19, Manivannan Sadhasivam wrote:
> On Wed, Apr 16, 2025 at 06:03:36PM +0200, Niklas Cassel wrote:
>> Hello Krishna Chaitanya,
>>
>> On Wed, Apr 16, 2025 at 09:15:19PM +0530, Krishna Chaitanya Chundru wrote:
>>> On 4/16/2025 7:43 PM, Niklas Cassel wrote:
>>>>
>>>> So perhaps we should hold off with this patch.
>>>>
>>> I disagree on this, it might be causing issue with net driver, but we
>>> might face some other issues as explained above if we don't call
>>> pci_stop_root_bus().
>>
>> When I wrote hold off with this patch, I meant the patch in $subject,
>> not your patch.
>>
>>
>> When it comes to your patch, I think that the commit log needs to explain
>> why it is so special.
>>
>> Because AFAICT, all other PCIe controller drivers call pci_stop_root_bus()
>> in the .remove() callback, not in the .shutdown() callback.
>>
> 
> And this driver is special because, it has no 'remove()' callback as it
> implements an irqchip controller. So we have to shutdown the devices cleanly in
> the 'shutdown' callback.
> 
Doing proper cleanup in a first place is responsibility of the respective bus
devices (in their shutdown() callback).

Calling pci_stop_root_bus() in the pci controllers shutdown() causes the bus
devices to be removed, hence their remove() is called. Typically devices
don't expect that remove() is called after shutdown(). This may cause issues
if e.g. shutdown() sets a device to D3cold. remove() won't expect that device
is inaccessible.

Another issue is with devices being wake-up sources. If wake-up is enabled,
then devices configure the wake-up in their shutdown() callback. Calling
remove() afterwards may reverse parts of the wake-up configuration.
And I'd expect that most wakeup-capable device disable wakeup in the
remove() callback. So there's a good chance that the proposed change breaks
wake-up.

There maybe other side effects I'm not aware of.

IMO a controller drivers shutdown() shall focus on cleanup up its own resources.

> Also do note that the driver core will not call the 'remove()' callback unless
> the driver as a module is unloaded during poweroff/reboot scenarios. So the
> controller drivers need to properly power down the devices in their 'shutdown()'
> callback IMO.
> 
>> Doing things differently from all other PCIe controller drivers is usually
>> a red flag.
>>
> 
> Yes, even if it is the right thing to do ;) But I agree that the commit message
> needs some improvement.
> 
> - Mani
>