* [PATCH 1/2] PCI: Workaround hardware bugs in ACS functionality by implementing it as a quirk @ 2018-04-11 15:58 James Puthukattukaran 2018-04-11 16:57 ` Sinan Kaya 2018-04-11 17:38 ` Alex Williamson 0 siblings, 2 replies; 4+ messages in thread From: James Puthukattukaran @ 2018-04-11 15:58 UTC (permalink / raw) To: linux-pci; +Cc: Sinan Kaya, Bjorn Helgaas There are bugs in ACS implementations by certain switch vendors that cause ACS violations in perfectly normal accesses. This patch provides the framework for enabling and disabling the functionality at certain points during endpoint bringup in the form of a quirk. Signed-off-by: James Puthukattukaran <james.puthukattukaran@oracle.com> -- -v2: move workaround to pci_bus_read_dev_vendor_id() from pci_bus_check_dev() and move enable_acs_sv to drivers/pci/pci.c -- by Yinghai -v3: add bus->self check for root bus and virtual bus for sriov vfs. -v4: only do workaround for IDT switches -v5: tweak pci_std_enable_acs_sv to deal with unimplemented SV -v6: Added errata verbiage verbatim and resolved patch format issues -v7: changed int to bool for found and idt_workaround declarations. Also added bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=196979 -v8: Rewrote the patch by adding a new acs quirk to keep the workaround out of the main code path -v9: changed function name from pci_dev_specific_fixup_acs_quirk to pci_bus_acs_quirk. Also, tested for FLR and hot reset scenarios and did not see issues where workaround was required. The issue seems to be related only to cold reset/power on situation. -v10: Moved the contents of pci_bus_read_vendor_id into an internal function __pci_bus_read_vendor_id -v11: Split the patch into two patches. The first a general quirk framework. --- drivers/pci/pci.h | 7 +++++++ drivers/pci/probe.c | 33 ++++++++++++++++++++++++++++----- drivers/pci/quirks.c | 41 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 5 deletions(-) diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h index 023f7cf..a9e2a8c 100644 --- a/drivers/pci/pci.h +++ b/drivers/pci/pci.h @@ -387,11 +387,18 @@ struct pci_dev_reset_methods { #ifdef CONFIG_PCI_QUIRKS int pci_dev_specific_reset(struct pci_dev *dev, int probe); +int pci_bus_specific_acs_quirk(struct pci_bus *bus , int devfn, + int enable, bool found); #else static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe) { return -ENOTTY; } +static inline int pci_bus_specific_acs_quirk(struct pc_bus *bus, + int devfn, int enable, bool found) +{ + return -ENOTTY; +} #endif #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64) diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c index ac91b6f..0c93b3e 100644 --- a/drivers/pci/probe.c +++ b/drivers/pci/probe.c @@ -2097,21 +2097,44 @@ static bool pci_bus_wait_crs(struct pci_bus *bus, int devfn, u32 *l, return true; } -bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, +static bool __pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, int timeout) { + + int found = false; + if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l)) - return false; + goto out; /* Some broken boards return 0 or ~0 if a slot is empty: */ if (*l == 0xffffffff || *l == 0x00000000 || *l == 0x0000ffff || *l == 0xffff0000) - return false; + goto out; + found = true; if (pci_bus_crs_vendor_id(*l)) - return pci_bus_wait_crs(bus, devfn, l, timeout); + found = pci_bus_wait_crs(bus, devfn, l, timeout); - return true; +out: + return found; + +} + + +bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, + int timeout) +{ + bool ret; + int enable = -1; + + enable = pci_bus_specific_acs_quirk(bus, devfn, false, false); + + ret = __pci_bus_read_dev_vendor_id(bus, devfn, l, timeout); + + if (enable > 0) + pci_bus_specific_acs_quirk(bus, devfn, enable, ret); + + return ret; } EXPORT_SYMBOL(pci_bus_read_dev_vendor_id); diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c index 26141b1..bf423e3 100644 --- a/drivers/pci/quirks.c +++ b/drivers/pci/quirks.c @@ -4737,3 +4737,44 @@ static void quirk_gpu_hda(struct pci_dev *hda) PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda); DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda); + + + +static const struct pci_dev_acs_quirk{ + u16 vendor; + u16 device; + int (*acs_quirk)(struct pci_bus *bus, int devfn, int enable, bool found); +} pci_dev_acs_quirks[] = { + {0} +}; + +int pci_bus_specific_acs_quirk(struct pci_bus *bus, int devfn, int enable, + bool found) +{ + const struct pci_dev_acs_quirk *i; + struct pci_dev *dev; + int ret; + + if (!bus || !bus->self) + return -ENOTTY; + + dev = bus->self; + + /* + * Allow devices that have HW bugs in the ACS implementations to + * control enabling or disabling that ACS feature here. + */ + for (i = pci_dev_acs_quirks; i->acs_quirk; i++) { + if ((i->vendor == dev->vendor || + i->vendor == (u16)PCI_ANY_ID) && + (i->device == dev->device || + i->device == (u16)PCI_ANY_ID)) { + ret = i->acs_quirk(bus, devfn, enable, found); + if (ret >= 0) + return ret; + } + } + + return -ENOTTY; +} + ^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] PCI: Workaround hardware bugs in ACS functionality by implementing it as a quirk 2018-04-11 15:58 [PATCH 1/2] PCI: Workaround hardware bugs in ACS functionality by implementing it as a quirk James Puthukattukaran @ 2018-04-11 16:57 ` Sinan Kaya 2018-04-11 17:38 ` Alex Williamson 1 sibling, 0 replies; 4+ messages in thread From: Sinan Kaya @ 2018-04-11 16:57 UTC (permalink / raw) To: James Puthukattukaran, linux-pci; +Cc: Bjorn Helgaas On 4/11/2018 11:58 AM, James Puthukattukaran wrote: > > There are bugs in ACS implementations by certain switch vendors that cause ACS violations in perfectly normal accesses. This patch provides the framework for enabling and disabling the functionality at certain points during endpoint bringup in the form of a quirk. > > Signed-off-by: James Puthukattukaran <james.puthukattukaran@oracle.com> Small nits below. Need to see what Bjorn and Alex to say about this. > #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64) > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index ac91b6f..0c93b3e 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -2097,21 +2097,44 @@ static bool pci_bus_wait_crs(struct pci_bus *bus, int devfn, u32 *l, > return true; > } > > -bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, > +static bool __pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, > int timeout) > { There is no point in changing this function contents now. Just change the header in this code. Please revert your other changes in this function. > + > + > +bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, > + int timeout) > +{ > + bool ret; > + int enable = -1; no need for initialization. > + > + enable = pci_bus_specific_acs_quirk(bus, devfn, false, false); > + > + ret = __pci_bus_read_dev_vendor_id(bus, devfn, l, timeout); > + > + if (enable > 0) > + pci_bus_specific_acs_quirk(bus, devfn, enable, ret); > + > + return ret; > } -- Sinan Kaya Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc. Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project. ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] PCI: Workaround hardware bugs in ACS functionality by implementing it as a quirk 2018-04-11 15:58 [PATCH 1/2] PCI: Workaround hardware bugs in ACS functionality by implementing it as a quirk James Puthukattukaran 2018-04-11 16:57 ` Sinan Kaya @ 2018-04-11 17:38 ` Alex Williamson 2018-04-19 15:39 ` James Puthukattukaran 1 sibling, 1 reply; 4+ messages in thread From: Alex Williamson @ 2018-04-11 17:38 UTC (permalink / raw) To: James Puthukattukaran; +Cc: linux-pci, Sinan Kaya, Bjorn Helgaas On Wed, 11 Apr 2018 11:58:43 -0400 James Puthukattukaran <james.puthukattukaran@oracle.com> wrote: > There are bugs in ACS implementations by certain switch vendors that > cause ACS violations in perfectly normal accesses. This patch provides > the framework for enabling and disabling the functionality at certain > points during endpoint bringup in the form of a quirk. > > Signed-off-by: James Puthukattukaran <james.puthukattukaran@oracle.com> > > -- > > -v2: move workaround to pci_bus_read_dev_vendor_id() from > pci_bus_check_dev() > and move enable_acs_sv to drivers/pci/pci.c -- by Yinghai > -v3: add bus->self check for root bus and virtual bus for sriov vfs. > -v4: only do workaround for IDT switches > -v5: tweak pci_std_enable_acs_sv to deal with unimplemented SV > -v6: Added errata verbiage verbatim and resolved patch format issues > -v7: changed int to bool for found and idt_workaround declarations. Also > added bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=196979 > -v8: Rewrote the patch by adding a new acs quirk to keep the workaround > out of the main code path > -v9: changed function name from pci_dev_specific_fixup_acs_quirk to > pci_bus_acs_quirk. Also, tested for FLR and hot reset scenarios > and did > not see issues where workaround was required. The issue seems to be > related only to cold reset/power on situation. > -v10: Moved the contents of pci_bus_read_vendor_id into an internal function > __pci_bus_read_vendor_id > -v11: Split the patch into two patches. The first a general quirk framework. > > --- > > drivers/pci/pci.h | 7 +++++++ > drivers/pci/probe.c | 33 ++++++++++++++++++++++++++++----- > drivers/pci/quirks.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 76 insertions(+), 5 deletions(-) > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > index 023f7cf..a9e2a8c 100644 > --- a/drivers/pci/pci.h > +++ b/drivers/pci/pci.h > @@ -387,11 +387,18 @@ struct pci_dev_reset_methods { > > #ifdef CONFIG_PCI_QUIRKS > int pci_dev_specific_reset(struct pci_dev *dev, int probe); > +int pci_bus_specific_acs_quirk(struct pci_bus *bus , int devfn, > + int enable, bool found); > #else > static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe) > { > return -ENOTTY; > } > +static inline int pci_bus_specific_acs_quirk(struct pc_bus *bus, > + int devfn, int enable, bool found) > +{ > + return -ENOTTY; > +} > #endif > > #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64) > diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c > index ac91b6f..0c93b3e 100644 > --- a/drivers/pci/probe.c > +++ b/drivers/pci/probe.c > @@ -2097,21 +2097,44 @@ static bool pci_bus_wait_crs(struct pci_bus > *bus, int devfn, u32 *l, > return true; > } > > -bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, > +static bool __pci_bus_read_dev_vendor_id(struct pci_bus *bus, int > devfn, u32 *l, > int timeout) > { > + > + int found = false; > + > if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l)) > - return false; > + goto out; > > /* Some broken boards return 0 or ~0 if a slot is empty: */ > if (*l == 0xffffffff || *l == 0x00000000 || > *l == 0x0000ffff || *l == 0xffff0000) > - return false; > + goto out; > > + found = true; > if (pci_bus_crs_vendor_id(*l)) > - return pci_bus_wait_crs(bus, devfn, l, timeout); > + found = pci_bus_wait_crs(bus, devfn, l, timeout); > > - return true; > +out: > + return found; > + > +} The changes to the above function do not change the behavior and are not an improvement to the function, imo. There are no locks to be released, there's no resources to free, there's no reason to have a goto and common exit point. > + > + > +bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, > + int timeout) > +{ > + bool ret; > + int enable = -1; Unnecessary initialization. > + > + enable = pci_bus_specific_acs_quirk(bus, devfn, false, false); The prototype specifies the 3rd arg is an int, not bool. The args are pretty cryptic regardless though, can we rename or create wrappers to make their usage better? This looks like a test_and_clear sort of function and the below looks like a restore (if found?). > + > + ret = __pci_bus_read_dev_vendor_id(bus, devfn, l, timeout); > + > + if (enable > 0) > + pci_bus_specific_acs_quirk(bus, devfn, enable, ret); > + > + return ret; > } > EXPORT_SYMBOL(pci_bus_read_dev_vendor_id); > > diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c > index 26141b1..bf423e3 100644 > --- a/drivers/pci/quirks.c > +++ b/drivers/pci/quirks.c > @@ -4737,3 +4737,44 @@ static void quirk_gpu_hda(struct pci_dev *hda) > PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda); > DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, > PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda); > + > + > + > +static const struct pci_dev_acs_quirk{ > + u16 vendor; > + u16 device; > + int (*acs_quirk)(struct pci_bus *bus, int devfn, int enable, bool found); > +} pci_dev_acs_quirks[] = { > + {0} > +}; > + > +int pci_bus_specific_acs_quirk(struct pci_bus *bus, int devfn, int enable, > + bool found) > +{ > + const struct pci_dev_acs_quirk *i; > + struct pci_dev *dev; > + int ret; > + > + if (!bus || !bus->self) > + return -ENOTTY; > + > + dev = bus->self; > + > + /* > + * Allow devices that have HW bugs in the ACS implementations to > + * control enabling or disabling that ACS feature here. > + */ > + for (i = pci_dev_acs_quirks; i->acs_quirk; i++) { > + if ((i->vendor == dev->vendor || > + i->vendor == (u16)PCI_ANY_ID) && > + (i->device == dev->device || > + i->device == (u16)PCI_ANY_ID)) { > + ret = i->acs_quirk(bus, devfn, enable, found); > + if (ret >= 0) > + return ret; > + } > + } > + > + return -ENOTTY; > +} > + What about a "pci_bus_specific_acs_quirk()" would make something think they need to call it in the presented use case above? The commit log really doesn't shed any light on why it's used here and not elsewhere or specifically what condition this is meant to trap. Should we instead be looking at a pci_bus_specific_read_dev_vendor_id() function? For example: bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, int timeout) { int ret = pci_bus_specific_read_dev_vendor_id(...); if (ret >= 0) return ret > 0; return pci_bus_generic_read_dev_vendor_id(...); } That makes the purpose and call point of the quirk function very well defined and you can reuse the generic code within your quirk without creating obscure calling conventions. Please also fix your sending of patch series, there should be a cover letter with patches referencing the message id for threading. Thanks, Alex ^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH 1/2] PCI: Workaround hardware bugs in ACS functionality by implementing it as a quirk 2018-04-11 17:38 ` Alex Williamson @ 2018-04-19 15:39 ` James Puthukattukaran 0 siblings, 0 replies; 4+ messages in thread From: James Puthukattukaran @ 2018-04-19 15:39 UTC (permalink / raw) To: Alex Williamson; +Cc: linux-pci, Sinan Kaya, Bjorn Helgaas Alex - I have taken your suggestion at the end of to re-architect using a bus specific quirk and will be re-submitting the two patches with the rewrite. thanks James On 04/11/2018 01:38 PM, Alex Williamson wrote: > On Wed, 11 Apr 2018 11:58:43 -0400 > James Puthukattukaran <james.puthukattukaran@oracle.com> wrote: > >> There are bugs in ACS implementations by certain switch vendors that >> cause ACS violations in perfectly normal accesses. This patch provides >> the framework for enabling and disabling the functionality at certain >> points during endpoint bringup in the form of a quirk. >> >> Signed-off-by: James Puthukattukaran <james.puthukattukaran@oracle.com> >> >> -- >> >> -v2: move workaround to pci_bus_read_dev_vendor_id() from >> pci_bus_check_dev() >> and move enable_acs_sv to drivers/pci/pci.c -- by Yinghai >> -v3: add bus->self check for root bus and virtual bus for sriov vfs. >> -v4: only do workaround for IDT switches >> -v5: tweak pci_std_enable_acs_sv to deal with unimplemented SV >> -v6: Added errata verbiage verbatim and resolved patch format issues >> -v7: changed int to bool for found and idt_workaround declarations. Also >> added bugzilla https://bugzilla.kernel.org/show_bug.cgi?id=196979 >> -v8: Rewrote the patch by adding a new acs quirk to keep the workaround >> out of the main code path >> -v9: changed function name from pci_dev_specific_fixup_acs_quirk to >> pci_bus_acs_quirk. Also, tested for FLR and hot reset scenarios >> and did >> not see issues where workaround was required. The issue seems to be >> related only to cold reset/power on situation. >> -v10: Moved the contents of pci_bus_read_vendor_id into an internal function >> __pci_bus_read_vendor_id >> -v11: Split the patch into two patches. The first a general quirk framework. >> >> --- >> >> drivers/pci/pci.h | 7 +++++++ >> drivers/pci/probe.c | 33 ++++++++++++++++++++++++++++----- >> drivers/pci/quirks.c | 41 +++++++++++++++++++++++++++++++++++++++++ >> 3 files changed, 76 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h >> index 023f7cf..a9e2a8c 100644 >> --- a/drivers/pci/pci.h >> +++ b/drivers/pci/pci.h >> @@ -387,11 +387,18 @@ struct pci_dev_reset_methods { >> >> #ifdef CONFIG_PCI_QUIRKS >> int pci_dev_specific_reset(struct pci_dev *dev, int probe); >> +int pci_bus_specific_acs_quirk(struct pci_bus *bus , int devfn, >> + int enable, bool found); >> #else >> static inline int pci_dev_specific_reset(struct pci_dev *dev, int probe) >> { >> return -ENOTTY; >> } >> +static inline int pci_bus_specific_acs_quirk(struct pc_bus *bus, >> + int devfn, int enable, bool found) >> +{ >> + return -ENOTTY; >> +} >> #endif >> >> #if defined(CONFIG_PCI_QUIRKS) && defined(CONFIG_ARM64) >> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c >> index ac91b6f..0c93b3e 100644 >> --- a/drivers/pci/probe.c >> +++ b/drivers/pci/probe.c >> @@ -2097,21 +2097,44 @@ static bool pci_bus_wait_crs(struct pci_bus >> *bus, int devfn, u32 *l, >> return true; >> } >> >> -bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, >> +static bool __pci_bus_read_dev_vendor_id(struct pci_bus *bus, int >> devfn, u32 *l, >> int timeout) >> { >> + >> + int found = false; >> + >> if (pci_bus_read_config_dword(bus, devfn, PCI_VENDOR_ID, l)) >> - return false; >> + goto out; >> >> /* Some broken boards return 0 or ~0 if a slot is empty: */ >> if (*l == 0xffffffff || *l == 0x00000000 || >> *l == 0x0000ffff || *l == 0xffff0000) >> - return false; >> + goto out; >> >> + found = true; >> if (pci_bus_crs_vendor_id(*l)) >> - return pci_bus_wait_crs(bus, devfn, l, timeout); >> + found = pci_bus_wait_crs(bus, devfn, l, timeout); >> >> - return true; >> +out: >> + return found; >> + >> +} > > The changes to the above function do not change the behavior and are > not an improvement to the function, imo. There are no locks to be > released, there's no resources to free, there's no reason to have a > goto and common exit point. > >> + >> + >> +bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, >> + int timeout) >> +{ >> + bool ret; >> + int enable = -1; > > Unnecessary initialization. > >> + >> + enable = pci_bus_specific_acs_quirk(bus, devfn, false, false); > > The prototype specifies the 3rd arg is an int, not bool. The args are > pretty cryptic regardless though, can we rename or create wrappers to > make their usage better? This looks like a test_and_clear sort of > function and the below looks like a restore (if found?). > >> + >> + ret = __pci_bus_read_dev_vendor_id(bus, devfn, l, timeout); >> + >> + if (enable > 0) >> + pci_bus_specific_acs_quirk(bus, devfn, enable, ret); >> + >> + return ret; >> } >> EXPORT_SYMBOL(pci_bus_read_dev_vendor_id); >> >> diff --git a/drivers/pci/quirks.c b/drivers/pci/quirks.c >> index 26141b1..bf423e3 100644 >> --- a/drivers/pci/quirks.c >> +++ b/drivers/pci/quirks.c >> @@ -4737,3 +4737,44 @@ static void quirk_gpu_hda(struct pci_dev *hda) >> PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda); >> DECLARE_PCI_FIXUP_CLASS_FINAL(PCI_VENDOR_ID_NVIDIA, PCI_ANY_ID, >> PCI_CLASS_MULTIMEDIA_HD_AUDIO, 8, quirk_gpu_hda); >> + >> + >> + >> +static const struct pci_dev_acs_quirk{ >> + u16 vendor; >> + u16 device; >> + int (*acs_quirk)(struct pci_bus *bus, int devfn, int enable, bool found); >> +} pci_dev_acs_quirks[] = { >> + {0} >> +}; >> + >> +int pci_bus_specific_acs_quirk(struct pci_bus *bus, int devfn, int enable, >> + bool found) >> +{ >> + const struct pci_dev_acs_quirk *i; >> + struct pci_dev *dev; >> + int ret; >> + >> + if (!bus || !bus->self) >> + return -ENOTTY; >> + >> + dev = bus->self; >> + >> + /* >> + * Allow devices that have HW bugs in the ACS implementations to >> + * control enabling or disabling that ACS feature here. >> + */ >> + for (i = pci_dev_acs_quirks; i->acs_quirk; i++) { >> + if ((i->vendor == dev->vendor || >> + i->vendor == (u16)PCI_ANY_ID) && >> + (i->device == dev->device || >> + i->device == (u16)PCI_ANY_ID)) { >> + ret = i->acs_quirk(bus, devfn, enable, found); >> + if (ret >= 0) >> + return ret; >> + } >> + } >> + >> + return -ENOTTY; >> +} >> + > > What about a "pci_bus_specific_acs_quirk()" would make something think > they need to call it in the presented use case above? The commit log > really doesn't shed any light on why it's used here and not elsewhere > or specifically what condition this is meant to trap. Should we > instead be looking at a pci_bus_specific_read_dev_vendor_id() > function? For example: > > bool pci_bus_read_dev_vendor_id(struct pci_bus *bus, int devfn, u32 *l, int timeout) > { > int ret = pci_bus_specific_read_dev_vendor_id(...); > > if (ret >= 0) > return ret > 0; > > return pci_bus_generic_read_dev_vendor_id(...); > } > > That makes the purpose and call point of the quirk function very well > defined and you can reuse the generic code within your quirk without > creating obscure calling conventions. > > Please also fix your sending of patch series, there should be a cover > letter with patches referencing the message id for threading. Thanks, > > Alex > ^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2018-04-19 15:39 UTC | newest] Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-04-11 15:58 [PATCH 1/2] PCI: Workaround hardware bugs in ACS functionality by implementing it as a quirk James Puthukattukaran 2018-04-11 16:57 ` Sinan Kaya 2018-04-11 17:38 ` Alex Williamson 2018-04-19 15:39 ` James Puthukattukaran
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.