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: <9e9854d5-1722-40f2-b343-97cf9b23a977@gmail.com>
Date: Sat, 19 Apr 2025 12:18:54 +0200
From: Heiner Kallweit <hkallweit1@...il.com>
To: Bjorn Helgaas <bhelgaas@...gle.com>
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, Niklas Cassel <cassel@...nel.org>,
 Manivannan Sadhasivam <manivannan.sadhasivam@...aro.org>
Subject: Re: [PATCH] r8169: do not call rtl8169_down() twice on shutdown

On 18.04.2025 23:52, Heiner Kallweit wrote:
> 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
>>
> 
+Bjorn, as this is primarily a PCI topic