lkml.org 
[lkml]   [2021]   [Mar]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: RFC: sysfs node for Secondary PCI bus reset (PCIe Hot Reset)
From
Date


On 3/3/2021 11:26 PM, Pali Rohár wrote:
> External email: Use caution opening links or attachments
>
>
> On Tuesday 02 March 2021 12:58:29 Alex Williamson wrote:
>> On Mon, 1 Mar 2021 14:28:17 -0600
>> Bjorn Helgaas <helgaas@kernel.org> wrote:
>>
>>> [+cc Alex, reset expert]
>>>
>>> On Mon, Mar 01, 2021 at 06:12:21PM +0100, Pali Rohár wrote:
>>>> Hello!
>>>>
>>>> PCIe card can be reset via in-band Hot Reset signal which can be
>>>> triggered by PCIe bridge via Secondary Bus Reset bit in PCI config
>>>> space.
>>>>
>>>> Kernel already exports sysfs node "reset" for triggering Functional
>>>> Reset of particular function of PCI device. But in some cases Functional
>>>> Reset is not enough and Hot Reset is required.
>>>>
>>>> Following RFC patch exports sysfs node "reset_bus" for PCI bridges which
>>>> triggers Secondary Bus Reset and therefore for PCIe bridges it resets
>>>> connected PCIe card.
>>>>
>>>> What do you think about it?
>>>>
>>>> Currently there is userspace script which can trigger PCIe Hot Reset by
>>>> modifying PCI config space from userspace:
>>>>
>>>> https://alexforencich.com/wiki/en/pcie/hot-reset-linux
>>>>
>>>> But because kernel already provides way how to trigger Functional Reset
>>>> it could provide also way how to trigger PCIe Hot Reset.
>>
>> What that script does and what this does, or what the existing reset
>> attribute does, are very different. The script finds the upstream
>> bridge for a given device, removes the device (ignoring that more than
>> one device might be affected by the bus reset), uses setpci to trigger
>> a secondary bus reset, then rescans devices. The below only triggers
>> the secondary bus reset, neither saving and restoring affected device
>> state like the existing function level reset attribute, nor removing
>> and rescanning as the script does. It simply leaves an entire
>> hierarchy of PCI devices entirely un-programmed yet still has struct
>> pci_devs attached to them for untold future misery.
>>
>> In fact, for the case of a single device affected by the bus reset, as
>> intended by the script, the existing reset attribute will already do
>> that if the device supports no other reset mechanism. There's actually
>> a running LFX mentorship project that aims to allow the user to control
>> the type of reset performed by the existing reset attribute such that a
>> user could force the bus reset behavior over other reset methods.
>
> Hello Alex? Do you have a link for this "reset" project? I'm interesting
> in it as I'm dealing with Compex wifi cards which are causing problems.
>
> For correct initialization I need to issue PCIe Warm Reset for these
> cards (Warm Reset is done via PERST# pin which most linux controller
> drivers controls via GPIO subsystem). And for now there is no way to
> trigger PCIe Warm Reset for particular PCIe device from userspace. As
> there is no userspace <--> kernel API for it.
>
>> There might be some justification for an attribute that actually
>> implements the referenced script correctly, perhaps in kernel we could
>> avoid races with bus rescans, but simply triggering an SBR to quietly
>> de-program all downstream devices with no state restore or device
>> rescan is not it. Any affected device would be unusable. Was this
>> tested? Thanks,
>
> I have tested my change. First I called 'remove' attribute for PCIe
> card, then I called this 'bus_reset' on parent PCIe bridge and later I
> called 'rescan' attribute on bridge. It correctly rested tested ath9k
> card. So I did something similar as in above script. But I agree that
> there are race conditions and basically lot of other calls needs to be
> done to restore state.
>
> So I see that to make it 'usable' we need to do it automatically in
> kernel and also rescan/restore state of PCIe devices behind bridge after
> reset...
But, is save-restore alone going to be enough? I mean what is the state
of the device-driver going to be when the device is going through the
reset process? Isn't remove-rescan the correct thing to do here rather
than save/restore?

- Vidya Sagar
>
>> Alex
>>
>>>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>>>> index 50fcb62d59b5..f5e11c589498 100644
>>>> --- a/drivers/pci/pci-sysfs.c
>>>> +++ b/drivers/pci/pci-sysfs.c
>>>> @@ -1321,6 +1321,30 @@ static ssize_t reset_store(struct device *dev, struct device_attribute *attr,
>>>>
>>>> static DEVICE_ATTR(reset, 0200, NULL, reset_store);
>>>>
>>>> +static ssize_t reset_bus_store(struct device *dev, struct device_attribute *attr,
>>>> + const char *buf, size_t count)
>>>> +{
>>>> + struct pci_dev *pdev = to_pci_dev(dev);
>>>> + unsigned long val;
>>>> + ssize_t result = kstrtoul(buf, 0, &val);
>>>> +
>>>> + if (result < 0)
>>>> + return result;
>>>> +
>>>> + if (val != 1)
>>>> + return -EINVAL;
>>>> +
>>>> + pm_runtime_get_sync(dev);
>>>> + result = pci_bridge_secondary_bus_reset(pdev);
>>>> + pm_runtime_put(dev);
>>>> + if (result < 0)
>>>> + return result;
>>>> +
>>>> + return count;
>>>> +}
>>>> +
>>>> +static DEVICE_ATTR(reset_bus, 0200, NULL, reset_bus_store);
>>>> +
>>>> static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>>>> {
>>>> int retval;
>>>> @@ -1332,8 +1356,15 @@ static int pci_create_capabilities_sysfs(struct pci_dev *dev)
>>>> if (retval)
>>>> goto error;
>>>> }
>>>> + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE) {
>>>> + retval = device_create_file(&dev->dev, &dev_attr_reset_bus);
>>>> + if (retval)
>>>> + goto error_reset_bus;
>>>> + }
>>>> return 0;
>>>>
>>>> +error_reset_bus:
>>>> + device_remove_file(&dev->dev, &dev_attr_reset);
>>>> error:
>>>> pcie_vpd_remove_sysfs_dev_files(dev);
>>>> return retval;
>>>> @@ -1414,6 +1445,8 @@ static void pci_remove_capabilities_sysfs(struct pci_dev *dev)
>>>> device_remove_file(&dev->dev, &dev_attr_reset);
>>>> dev->reset_fn = 0;
>>>> }
>>>> + if (dev->hdr_type == PCI_HEADER_TYPE_BRIDGE)
>>>> + device_remove_file(&dev->dev, &dev_attr_reset_bus);
>>>> }
>>>>
>>>> /**
>>>
>>

\
 
 \ /
  Last update: 2021-03-11 15:26    [W:0.074 / U:0.124 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site