| 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
| ||
|
Date: Sat, 5 Jun 2021 19:04:05 -0500
From: Shanker R Donthineni <sdonthineni@...dia.com>
To: Bjorn Helgaas <helgaas@...nel.org>,
Amey Narkhede <ameynarkhede03@...il.com>
CC: Bjorn Helgaas <bhelgaas@...gle.com>, <alex.williamson@...hat.com>,
Raphael Norwitz <raphael.norwitz@...anix.com>,
<linux-pci@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
<kw@...ux.com>, Sinan Kaya <okaya@...nel.org>,
"Rafael J. Wysocki" <rjw@...ysocki.net>
Subject: Re: [PATCH v5 5/7] PCI: Add support for a function level reset based
on _RST method
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@...dia.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@...dia.com>
>> Reviewed-by: Sinan Kaya <okaya@...nel.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@...son:~# 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);
}
Powered by blists - more mailing lists