lkml.org 
[lkml]   [2021]   [Jun]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH v7 2/8] PCI: Add new array for keeping track of ordering of reset methods
    On Mon, Jun 21, 2021 at 10:45:18PM +0530, Amey Narkhede wrote:
    > On 21/06/21 10:02AM, Shanker R Donthineni wrote:
    > > On 6/18/21 12:22 PM, Amey Narkhede wrote:
    > > > I wonder if this would be easier if dev->reset_methods[] contained
    > > > indices into pci_reset_fn_methods[], highest priority first, with the
    > > > priority being determined when dev->reset_methods[] is updated. For
    > > > example:
    > > >
    > > > const struct pci_reset_fn_method pci_reset_fn_methods[] = {
    > > > { }, # 0
    > > > { &pci_dev_specific_reset, .name = "device_specific" }, # 1
    > > > { &pci_dev_acpi_reset, .name = "acpi" }, # 2
    > > > { &pcie_reset_flr, .name = "flr" }, # 3
    > > > { &pci_af_flr, .name = "af_flr" }, # 4
    > > > { &pci_pm_reset, .name = "pm" }, # 5
    > > > { &pci_reset_bus_function, .name = "bus" }, # 6
    > > > };
    > > >
    > > > dev->reset_methods[] = [1, 2, 3, 4, 5, 6]
    > > > means all reset methods are supported, in the default priority
    > > > order
    > > >
    > > > dev->reset_methods[] = [1, 0, 0, 0, 0, 0]
    > > > means only pci_dev_specific_reset is supported
    > > >
    > > > dev->reset_methods[] = [3, 5, 0, 0, 0, 0]
    > > > means pcie_reset_flr and pci_pm_reset are supported, in that
    > > > priority order
    > >
    > > What about keeping two bitmap fields 'resets_supported' and
    > > 'resets_enabled' in pci_dev object and mange it through sysfs and
    > > probe helper function. We can avoid two loops multiple paces and
    > > takes only 2Bytes of memory to keep track resets.
    > >
    > > resets_supported  ---> initialized during pci_dev setup
    > > resets_enabled ---> Exposed to userspace through sysfs by default set to resets_supported
    > >
    > > include/linux/pci.h:
    > > ------------------------
    > > /* Different types of PCI resets possible, lower number is higher priority */
    > > #define PCI_RESET_METHOD_DEVSPEC     0
    > > #define PCI_RESET_METHOD_ACPI            1
    > > #define PCI_RESET_METHOD_FLR              2
    > > #define PCI_RESET_METHOD_Af_FLR         3
    > > #define PCI_RESET_METHOD_PM               4
    > > #define PCI_RESET_METHOD_BUS             5
    > > #define PCI_RESET_METHOD_MAX            6
    > >
    > > struct pci_dev {
    > >     ...
    > >         u8              resets_supported;
    > >         u8              resets_enabled;
    > > };
    > >
    > > static inline bool pci_reset_supported(struct pci_dev *dev)
    > > {
    > >         return !!(dev->resets_supported);
    > > }
    > >
    > >
    > > drivers/pci/pci.c:
    > > --------------------
    > > const struct pci_reset_fn_method pci_reset_fn_methods[PCI_RESET_METHOD_MAX] = {
    > >         [PCI_RESET_METHOD_DEVSPEC] = { &pci_dev_specific_reset,
    > >                                                                    .name = "device_specific" },
    > >         [PCI_RESET_METHOD_ACPI] = { &pci_dev_acpi_reset, .name = "acpi" },
    > >         [PCI_RESET_METHOD_FLR] = { &pcie_reset_flr, .name = "flr" },
    > >         [PCI_RESET_METHOD_Af_FLR] = { &pci_af_flr, .name = "af_flr" },
    > >         [PCI_RESET_METHOD_PM] = { &pci_pm_reset, .name = "pm" },
    > >         [PCI_RESET_METHOD_BUS] = { &pci_reset_bus_function, .name = "bus" }
    > > };
    > >
    > >
    > > void pci_init_reset_methods(struct pci_dev *dev)
    > > {
    > >         int i, rc;
    > >
    > >         BUILD_BUG_ON(ARRAY_SIZE(pci_reset_fn_methods) != PCI_RESET_METHOD_MAX);
    > >         might_sleep();
    > >
    > >         for (i = 0; i < PCI_RESET_METHOD_MAX; i++) {
    > >                 rc = pci_reset_fn_methods[i].reset_fn(dev, PCI_RESET_PROBE);
    > >                 if (!rc)
    > >                         dev->resets_supported |= BIT(i);
    > >                 else if (rc != -ENOTTY)
    > >                         break;
    > >         }
    > >         dev->resets_enabled = dev->resets_supported;
    > > }
    > >
    > > int __pci_reset_function_locked(struct pci_dev *dev)
    > > {
    > >         int i, rc = -ENOTTY;
    > >
    > >         might_sleep();
    > >
    > >         for (i = 0; i < PCI_RESET_METHOD_MAX; i++) {
    > >                 if (dev->resets_enabled & BIT(i)) {
    > >                         rc = pci_reset_fn_methods[i].reset_fn(dev, 0);
    > >                         if (rc != -ENOTTY)
    > >                                 return rc;
    > >                 }
    > >         }
    > >
    > >         if (rc == -ENOTTY)
    > >                 pci_warn(dev, "No reset happened reason %s\n",
    > >                          !!dev->resets_supported ?
    > >                          "disabled by user" : "not supported");
    > >
    > >         return rc;
    > > }
    > >
    > > drivers/pci/pci-sysfs.c
    > > ----------------------------
    > > static ssize_t reset_method_store(struct device *dev,
    > >                                   struct device_attribute *attr,
    > >                                   const char *buf, size_t count)
    > > {
    > >         struct pci_dev *pdev = to_pci_dev(dev);
    > >         u8 resets_enabled = 0;
    > > ...
    > >         if (sysfs_streq(options, "default")) {
    > >                 pdev->resets_enabled = pdev->resets_supported;
    > >                 goto set_reset_methods;
    > >         }
    > >
    > >         while ((name = strsep(&options, ",")) != NULL) {
    > >                 if (sysfs_streq(name, ""))
    > >                         continue;
    > >                 name = strim(name);
    > >
    > >                 for (i = 0; i < PCI_RESET_METHOD_MAX; i++) {
    > >                         if ((pdev->resets_supported & BIT(i)) &&
    > >                             sysfs_streq(name, pci_reset_fn_methods[i].name)) {
    > >                                 resets_enabled |= BIT(i);
    > >                                 break;
    > >                         }
    > >                 }
    > > ...
    > >         }
    > >
    > > set_reset_methods:
    > >         kfree(options);
    > >         pdev->resets_enabled =  resets_enabled;
    > >         return count;
    > > }
    > >
    > > static ssize_t reset_method_show(struct device *dev,
    > >                                  struct device_attribute *attr,
    > >                                  char *buf)
    > > {
    > >         struct pci_dev *pdev = to_pci_dev(dev);
    > >         ssize_t len = 0;
    > >         int i;
    > >
    > >         for (i = 0; i < PCI_RESET_METHOD_MAX; i++) {
    > >                 if (pdev->resets_enabled & BIT(i))
    > >                         len += sysfs_emit_at(buf, len, "%s%s",
    > >                                              len ? "," : "",
    > >                                              pci_reset_fn_methods[i].name);
    > >         }
    > >         len += sysfs_emit_at(buf, len, len ? "\n" : "");
    > >
    > >         return len;
    > > }
    > >
    > Thank you for the idea.
    > Actually that would be coming full circle because Alex, Raphael and
    > I tried similar approach earlier while prototyping for v2 but this
    > implementation does look better than what I had at that time.

    I thought part of the point of this series was to allow the user to
    change the *order* of reset types. I don't think we can control the
    ordering if we only keep a bit (or even two) per reset type.

    \
     
     \ /
      Last update: 2021-06-21 20:39    [W:3.797 / U:0.188 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site