Message-ID: <817cf0ff-5ecd-f6b1-d9b9-cf6b2473ed23@oss.qualcomm.com>
Date: Sun, 20 Apr 2025 04:48:24 +0530
From: Krishna Chaitanya Chundru <krishna.chundru@....qualcomm.com>
To: Heiner Kallweit <hkallweit1@...il.com>,
        Bjorn Helgaas <bhelgaas@...gle.com>
Cc: 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 4/19/2025 3:48 PM, Heiner Kallweit wrote:
> 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.
>>
Lets say controller driver in the shut down callback keeps PCIe device
state in D3cold without removing PCIe devices. Then the PCIe client
drivers which are not registered with the shutdown callback assumes PCIe
link is still accessible and initiates some transfers or there may be
on ongoing transfers also which can result in some system errors like
soc error etc which can hang the device.

The patch which I submitted in the qcom pcie controller, removes the
PCIe devices first before turning off the PCIe link. All this
info needs to be in the commit text, I will update it in the next
version.
>> 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.
>>
After shutdown, the system will restart right why we need to enable 
wakeup in shutdown as after restart it will be like fresh boot to system
Correct me if I was wrong here.

I want to understand, why shutdown of the PCIe endpoint drivers in this
case rtl18169 shutdown will be called before PCIe controller shutdown,
AFAIK, the shutdown execution order will be same as probe order.

The problem PCI patch is trying to do is, not every PCIe drivers are
registering with shutdown callback, in that case if PCIe controller
driver if it cleans up its resources in shutdown and the PCIe drivers
which don't have the shutdown callback doesn't know that link was
down and can continue data transfers which will cause system errors like
noc error.

- Krishna Chaitanya.
>> 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
> 

Powered by blists - more mailing lists