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

From: Shanker Donthineni <sdonthineni@nvidia.com>

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()

The default priority of the acpi reset is set to below device-specific
and above hardware resets.

Signed-off-by: Shanker Donthineni <sdonthineni@nvidia.com>
Reviewed-by: Sinan Kaya <okaya@kernel.org>
---
 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(-)

[+cc Rafael in case you have any ideas about acpi_bind_one() below]

Mention ACPI in the subject, e.g.,

  PCI: Add support for ACPI _RST reset method

On Sun, May 30, 2021 at 12:55:25AM +0530, Amey Narkhede wrote:
> From: Shanker Donthineni <sdonthineni@nvidia.com>
> 
> 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/

> The default priority of the acpi reset is set to below device-specific
> and above hardware resets.

s/acpi/ACPI/

> Signed-off-by: Shanker Donthineni <sdonthineni@nvidia.com>
> Reviewed-by: Sinan Kaya <okaya@kernel.org>
> ---
>  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.

> +	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 ..."

> +		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.

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.

>  }
>  
>  struct pci_dev *pci_scan_single_device(struct pci_bus *bus, int devfn)
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 6e9bc4f9c..a7f063da2 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -49,7 +49,7 @@
>  			       PCI_STATUS_SIG_TARGET_ABORT | \
>  			       PCI_STATUS_PARITY)
>  
> -#define PCI_RESET_METHODS_NUM 5
> +#define PCI_RESET_METHODS_NUM 6
>  
>  /*
>   * The PCI interface treats multi-function devices as independent
> -- 
> 2.31.1
>

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@nvidia.com>
>>
>> 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@nvidia.com>
>> Reviewed-by: Sinan Kaya <okaya@kernel.org>
>> ---
>>  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);
 }

Hi Bjorn,

On 6/5/21 3:53 PM, Bjorn Helgaas wrote:
> 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.
>
> 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:
I've refactored pci_dev_acpi_reset() to avoid dependency on acpi_bind_one().
It can be called any time after creating pci_dev object. The code logic is
not exactly same as acpi_pci_bridge_d3() but similar flow. No need to set
ACPI_COMPANION since it would be updated eventually after probing the
reset methods.

Please review the below code and provide suggestions for the next step.

Updated patch:

[PATCH v5 5/7] PCI: Add support for ACPI _RST reset method

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(). Since the
ACPI_COMPANION was not done before calling pci_init_reset_methods(),
use acpi_pci_find_companion() to know the ACPI binding.

The default priority of the ACPI reset is set to below device-specific
and above hardware resets.

Signed-off-by: Shanker Donthineni <sdonthineni@nvidia.com>
Suggested-by: Alex Williamson <alex.williamson@redhat.com>
Reviewed-by: Sinan Kaya <okaya@kernel.org>
---
 drivers/pci/pci-acpi.c | 30 ++++++++++++++++++++++++++++++
 drivers/pci/pci.c      |  1 +
 drivers/pci/pci.h      |  6 ++++++
 include/linux/pci.h    |  2 +-
 4 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
index 36bc23e217592..c344c33f5c910 100644
--- a/drivers/pci/pci-acpi.c
+++ b/drivers/pci/pci-acpi.c
@@ -934,6 +934,36 @@ static pci_power_t acpi_pci_choose_state(struct pci_dev *pdev)

 static struct acpi_device *acpi_pci_find_companion(struct device *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.
+ */
+int pci_dev_acpi_reset(struct pci_dev *dev, int probe)
+{
+       acpi_handle handle = ACPI_HANDLE(&dev->dev);
+
+       /* Find out ACPI_HANDLE if not available in the device context */
+       if (!handle) {
+               handle = acpi_device_handle(acpi_pci_find_companion(&dev->dev));
+               if (!handle)
+                       return -ENOTTY;
+       }
+
+       if (!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;
+}
+
 static bool acpi_pci_bridge_d3(struct pci_dev *dev)
 {
        const struct fwnode_handle *fwnode;
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index bbed852d977f1..5726d120b70a2 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -5122,6 +5122,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/pci.h b/drivers/pci/pci.h
index 13ec6bd6f4f76..f3974ed1a99c2 100644
--- a/drivers/pci/pci.h
+++ b/drivers/pci/pci.h
@@ -703,7 +703,13 @@ static inline int pci_aer_raw_clear_status(struct pci_dev *dev) { return -EINVAL
 #ifdef CONFIG_ACPI
 int pci_acpi_program_hp_params(struct pci_dev *dev);
 extern const struct attribute_group pci_dev_acpi_attr_group;
+int pci_dev_acpi_reset(struct pci_dev *dev, int probe);
 #else
+static inline int pci_dev_acpi_reset(struct pci_dev *dev, int probe)
+{
+       return -ENOTTY;
+}
+
 static inline int pci_acpi_program_hp_params(struct pci_dev *dev)
 {
        return -ENODEV;
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 6e9bc4f9cdab4..a7f063da2fe5f 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -49,7 +49,7 @@
                               PCI_STATUS_SIG_TARGET_ABORT | \
                               PCI_STATUS_PARITY)

-#define PCI_RESET_METHODS_NUM 5
+#define PCI_RESET_METHODS_NUM 6

Hi Bjorn,

On 6/5/21 3:53 PM, Bjorn Helgaas wrote:
> 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.
>
> 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().

Thanks for pointing to reference code. I've added a new patch for setting
ACPI_COMPANION before device_add(). Please review the updated patch
series v7 'Expose and manage PCI device reset' and provide feedback.

-Shanker