Re: [PATCH v5 5/7] PCI: Add support for a function level reset based on _RST method

[Shanker R Donthineni]

Hi Bjorn,

On 6/5/21 3:53 PM, Bjorn Helgaas wrote:
> Mention ACPI in the subject, e.g.,
>
>   PCI: Add support for ACPI _RST reset method
Will change in the next patch.
> On Sun, May 30, 2021 at 12:55:25AM +0530, Amey Narkhede wrote:
>> From: Shanker Donthineni <sdonthineni@xxxxxxxxxx>
>>
>> The _RST is a standard method specified in the ACPI specification. It
>> provides a function level reset when it is described in the acpi_device
>> context associated with PCI-device.
>>
>> Implement a new reset function pci_dev_acpi_reset() for probing RST
>> method and execute if it is defined in the firmware. The ACPI binding
>> information is available only after calling device_add(). To consider
>> _RST method, move pci_init_reset_methods() to end of pci_device_add()
>> and craete two sysfs entries reset & reset_methond from
>> pci_create_sysfs_dev_files()
> s/craete/create/
> s/reset_methond/reset_method/
Will fix it.
>> The default priority of the acpi reset is set to below device-specific
>> and above hardware resets.
> s/acpi/ACPI/
Will fix it.
>> Signed-off-by: Shanker Donthineni <sdonthineni@xxxxxxxxxx>
>> Reviewed-by: Sinan Kaya <okaya@xxxxxxxxxx>
>> ---
>>  drivers/pci/pci-sysfs.c | 23 ++++++++++++++++++++---
>>  drivers/pci/pci.c       | 30 ++++++++++++++++++++++++++++++
>>  drivers/pci/probe.c     |  2 +-
>>  include/linux/pci.h     |  2 +-
>>  4 files changed, 52 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index 04b3d6565..b332d7923 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -1482,12 +1482,30 @@ static const struct attribute_group pci_dev_reset_attr_group = {
>>       .is_visible = pci_dev_reset_attr_is_visible,
>>  };
>>
>> +const struct attribute_group *pci_dev_reset_groups[] = {
>> +     &pci_dev_reset_attr_group,
>> +     &pci_dev_reset_method_attr_group,
>> +     NULL,
>> +};
> These should be static sysfs attributes if possible, e.g., see
> e1d3f3268b0e ("PCI/sysfs: Convert "config" to static attribute").
> pci_create_sysfs_dev_files() will soon be removed completely.
>
>>  int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
>>  {
>> +     int retval;
>> +
>>       if (!sysfs_initialized)
>>               return -EACCES;
>>
>> -     return pci_create_resource_files(pdev);
>> +     retval = pci_create_resource_files(pdev);
>> +     if (retval)
>> +             return retval;
>> +
>> +     retval = device_add_groups(&pdev->dev, pci_dev_reset_groups);
>> +     if (retval) {
>> +             pci_remove_resource_files(pdev);
>> +             return retval;
>> +     }
>> +
>> +     return 0;
>>  }
>>
>>  /**
>> @@ -1501,6 +1519,7 @@ void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
>>       if (!sysfs_initialized)
>>               return;
>>
>> +     device_remove_groups(&pdev->dev, pci_dev_reset_groups);
>>       pci_remove_resource_files(pdev);
>>  }
>>
>> @@ -1594,8 +1613,6 @@ const struct attribute_group *pci_dev_groups[] = {
>>       &pci_dev_group,
>>       &pci_dev_config_attr_group,
>>       &pci_dev_rom_attr_group,
>> -     &pci_dev_reset_attr_group,
>> -     &pci_dev_reset_method_attr_group,
>>       &pci_dev_vpd_attr_group,
>>  #ifdef CONFIG_DMI
>>       &pci_dev_smbios_attr_group,
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index bbed852d9..4a7019d0b 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -5115,6 +5115,35 @@ static void pci_dev_restore(struct pci_dev *dev)
>>               err_handler->reset_done(dev);
>>  }
>>
>> +/**
>> + * pci_dev_acpi_reset - do a function level reset using _RST method
>> + * @dev: device to reset
>> + * @probe: check if _RST method is included in the acpi_device context.
>> + */
>> +static int pci_dev_acpi_reset(struct pci_dev *dev, int probe)
>> +{
>> +#ifdef CONFIG_ACPI
>> +     acpi_handle handle = ACPI_HANDLE(&dev->dev);
>> +
>> +     /* Return -ENOTTY if _RST method is not included in the dev context */
>> +     if (!handle || !acpi_has_method(handle, "_RST"))
>> +             return -ENOTTY;
>> +
>> +     /* Return 0 for probe phase indicating that we can reset this device */
>> +     if (probe)
>> +             return 0;
>> +
>> +     /* Invoke _RST() method to perform a function level reset */
> Superfluous comment.  Actually all the single-line comments here are
> superfluous.
Will remove in the next patch.
>> +     if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) {
>> +             pci_warn(dev, "Failed to reset the device\n");
> The message should mention the type of reset, e.g., "ACPI _RST failed ..."
>
Will change to pci_warn(dev, "ACPI _RST failed\n");
>> +             return -EINVAL;
>> +     }
>> +     return 0;
>> +#else
>> +     return -ENOTTY;
>> +#endif
>> +}
>> +
>>  /*
>>   * The ordering for functions in pci_reset_fn_methods
>>   * is required for reset_methods byte array defined
>> @@ -5122,6 +5151,7 @@ static void pci_dev_restore(struct pci_dev *dev)
>>   */
>>  const struct pci_reset_fn_method pci_reset_fn_methods[] = {
>>       { &pci_dev_specific_reset, .name = "device_specific" },
>> +     { &pci_dev_acpi_reset, .name = "acpi" },
>>       { &pcie_reset_flr, .name = "flr" },
>>       { &pci_af_flr, .name = "af_flr" },
>>       { &pci_pm_reset, .name = "pm" },
>> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
>> index 90fd4f61f..eeab791a0 100644
>> --- a/drivers/pci/probe.c
>> +++ b/drivers/pci/probe.c
>> @@ -2404,7 +2404,6 @@ static void pci_init_capabilities(struct pci_dev *dev)
>>       pci_rcec_init(dev);             /* Root Complex Event Collector */
>>
>>       pcie_report_downtraining(dev);
>> -     pci_init_reset_methods(dev);
>>  }
>>
>>  /*
>> @@ -2495,6 +2494,7 @@ void pci_device_add(struct pci_dev *dev, struct pci_bus *bus)
>>       dev->match_driver = false;
>>       ret = device_add(&dev->dev);
>>       WARN_ON(ret < 0);
>> +     pci_init_reset_methods(dev);
> This is a little sketchy.  We shouldn't be doing device config stuff
> after device_add() because that's when it becomes available for
> drivers to bind to the device.  If we do anything with the device
> after that point, we may interfere with a driver.
The reason I did PCI driver attach/bind is happening from pci_bus_add_device()
after setting 'dev->match_driver = true'. I thought it's safe to update reset
methods after calling device_add() and before driver bind happens.

void pci_bus_add_device(struct pci_dev *dev)
{
    int retval;

    /*
     * Can not put in pci_device_add yet because resources
     * are not assigned yet for some devices.
     */
    pcibios_bus_add_device(dev);
    pci_fixup_device(pci_fixup_final, dev);
    pci_create_sysfs_dev_files(dev);
    pci_proc_attach_device(dev);
    pci_bridge_d3_update(dev);

    dev->match_driver = true;     
    retval = device_attach(&dev->dev);  ---- > PCI driver bind call

> I think the problem is that we don't call acpi_bind_one() until
> device_add().  There's some hackery in pci-acpi.c to deal with a
> similar problem for something else -- see acpi_pci_bridge_d3().
>
> I don't know how to fix this yet.  Here's the call graph that I think
> is relevant:
>
>   pci_scan_single_device
>     pci_scan_device
>       pci_set_of_node
>         dev->dev.of_node = of_pci_find_child_device()  <-- set OF stuff
>     pci_device_add
>       device_add
>         device_platform_notify
>           acpi_platform_notify
>             case KOBJ_ADD:
>               acpi_device_notify
>                 acpi_bind_one
>                   ACPI_COMPANION_SET()       <-- sets ACPI_COMPANION
>       pci_init_reset_methods
>         pci_dev_acpi_reset(PCI_RESET_PROBE)
>           handle = ACPI_HANDLE(&dev->dev)    <-- uses ACPI_COMPANION
>
> I think it's kind of a general problem that we currently don't have
> access to the ACPI stuff until *after* device_add().  I included
> pci_set_of_node() in the graph above because that seems sort of
> like an OF analogue of what acpi_bind_one() is doing.
>
> I would really like to do the ACPI_COMPANION setup earlier, maybe
> at the same time as pci_set_of_node().  But I don't know enough about
> what acpi_bind_one() does -- there's a lot going on in there.
>
Yes, it's a general problem ACPI binding information is available only after
calling device_platform_notify(). There are no exported functions to set
the ACPI_COMPANION like OF_NODE.

Another approach: It simplifies the code logic if we update reset methods
when creating sysfs entries 'reset' and 'reset_method'. I've verified this
code and getting an expected behavior.

root@jetson:~# cat /sys/bus/pci/devices/0009\:01\:00.0/reset_method
acpi,flr

--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -1427,7 +1427,7 @@ static umode_t pci_dev_reset_method_attr_is_visible(struct kobject *kobj,
 {
        struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));

-       if (!pci_reset_supported(pdev))
+       if (!pci_reset_supported(pdev) && !pci_init_reset_methods(pdev))
                return 0;

        return a->mode;
@@ -1471,7 +1471,7 @@ static umode_t pci_dev_reset_attr_is_visible(struct kobject *kobj,
 {
        struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));

-       if (!pci_reset_supported(pdev))
+       if (!pci_reset_supported(pdev) && !pci_init_reset_methods(pdev))
                return 0;

        return a->mode;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bbed852d977f1..13654255fa3dc 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5115,6 +5115,32 @@ static void pci_dev_restore(struct pci_dev *dev)
                err_handler->reset_done(dev);
 }
+/**
+ * pci_dev_acpi_reset - do a function level reset using _RST method
+ * @dev: device to reset
+ * @probe: check if _RST method is included in the acpi_device context.
+ */
+static int pci_dev_acpi_reset(struct pci_dev *dev, int probe)
+{
+#ifdef CONFIG_ACPI
+       acpi_handle handle = ACPI_HANDLE(&dev->dev);
+
+       if (!handle || !acpi_has_method(handle, "_RST"))
+               return -ENOTTY;
+
+       if (probe)
+               return 0;
+
+       if (ACPI_FAILURE(acpi_evaluate_object(handle, "_RST", NULL, NULL))) {
+               pci_warn(dev, "ACPI _RST failed\n");
+               return -EINVAL;
+       }
+       return 0;
+#else
+       return -ENOTTY;
+#endif
+}
+
 /*
  * The ordering for functions in pci_reset_fn_methods
  * is required for reset_methods byte array defined
@@ -5122,6 +5148,7 @@ static void pci_dev_restore(struct pci_dev *dev)
  */
 const struct pci_reset_fn_method pci_reset_fn_methods[] = {
        { &pci_dev_specific_reset, .name = "device_specific" },
+       { &pci_dev_acpi_reset, .name = "acpi" },
        { &pcie_reset_flr, .name = "flr" },
        { &pci_af_flr, .name = "af_flr" },
        { &pci_pm_reset, .name = "pm" },
@@ -5191,7 +5218,7 @@ EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
  * Stores reset mechanisms supported by device in reset_methods byte array
  * which is a member of struct pci_dev.
  */
-void pci_init_reset_methods(struct pci_dev *dev)
+bool pci_init_reset_methods(struct pci_dev *dev)
 {
        int i, rc;
        u8 prio = PCI_RESET_METHODS_NUM;
@@ -5209,6 +5236,7 @@ void pci_init_reset_methods(struct pci_dev *dev)
                        break;
        }
        memcpy(dev->reset_methods, reset_methods, sizeof(reset_methods));
+       return pci_reset_supported(dev);
 }

 /**
diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h
index 13ec6bd6f4f76..3e871a5a21bbd 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -33,7 +33,7 @@ enum pci_mmap_api {
 int pci_mmap_fits(struct pci_dev *pdev, int resno, struct vm_area_struct *vmai,
                  enum pci_mmap_api mmap_api);

-void pci_init_reset_methods(struct pci_dev *dev);
+bool pci_init_reset_methods(struct pci_dev *dev);
 int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
 int pci_bus_error_reset(struct pci_dev *dev);
-void pci_init_reset_methods(struct pci_dev *dev);
+bool pci_init_reset_methods(struct pci_dev *dev);
 int pci_bridge_secondary_bus_reset(struct pci_dev *dev);
 int pci_bus_error_reset(struct pci_dev *dev);

diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
index 90fd4f61f3802..275a067d7a282 100644
--- a/drivers/pci/probe.c
+++ b/drivers/pci/probe.c
@@ -2404,7 +2404,6 @@ static void pci_init_capabilities(struct pci_dev *dev)
        pci_rcec_init(dev);             /* Root Complex Event Collector */

        pcie_report_downtraining(dev);
-       pci_init_reset_methods(dev);
 }