| Message ID | 20171207222145.9769-2-Govinda.Tatti@Oracle.COM |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Xen/PCIback: PCI reset using 'reset' SysFS attribute | expand |
On Thu, Dec 07, 2017 at 05:21:44PM -0500, Govinda Tatti wrote: > This patch exports pcie_has_flr() and it is being used by Xen pciback > driver to reset (flr/slot/bus) PCI devices based on 'reset' SysFS > attribute. > > Signed-off-by: Govinda Tatti <Govinda.Tatti@Oracle.COM> > --- > v3: -New > > drivers/pci/pci.c | 3 ++- > include/linux/pci.h | 1 + > 2 files changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > index 6078dfc..499e922 100644 > --- a/drivers/pci/pci.c > +++ b/drivers/pci/pci.c > @@ -3872,7 +3872,7 @@ static void pci_flr_wait(struct pci_dev *dev) > * Returns true if the device advertises support for PCIe function level > * resets. > */ > -static bool pcie_has_flr(struct pci_dev *dev) > +bool pcie_has_flr(struct pci_dev *dev) > { > u32 cap; > > @@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) > pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > return cap & PCI_EXP_DEVCAP_FLR; > } > +EXPORT_SYMBOL_GPL(pcie_has_flr); I'd rather change pcie_flr() so you could *always* call it, and it would return 0, -ENOTTY, or whatever, based on whether FLR is supported. Is that feasible? I don't like the "Can I do this? Ok, do this" style of interfaces. It's racy (not really applicable in this case) and seems clunky. > /** > * pcie_flr - initiate a PCIe function level reset > diff --git a/include/linux/pci.h b/include/linux/pci.h > index d16a7c0..44bf2b5 100644 > --- a/include/linux/pci.h > +++ b/include/linux/pci.h > @@ -1089,6 +1089,7 @@ int pcie_get_mps(struct pci_dev *dev); > int pcie_set_mps(struct pci_dev *dev, int mps); > int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed, > enum pcie_link_width *width); > +bool pcie_has_flr(struct pci_dev *dev); > void pcie_flr(struct pci_dev *dev); > int __pci_reset_function(struct pci_dev *dev); > int __pci_reset_function_locked(struct pci_dev *dev); > -- > 2.9.5 >
Thanks Bjorn for your review comments. Please see below for my comments. On 12/8/2017 2:24 PM, Bjorn Helgaas wrote: > On Thu, Dec 07, 2017 at 05:21:44PM -0500, Govinda Tatti wrote: >> This patch exports pcie_has_flr() and it is being used by Xen pciback >> driver to reset (flr/slot/bus) PCI devices based on 'reset' SysFS >> attribute. >> >> Signed-off-by: Govinda Tatti <Govinda.Tatti@Oracle.COM> >> --- >> v3: -New >> >> drivers/pci/pci.c | 3 ++- >> include/linux/pci.h | 1 + >> 2 files changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c >> index 6078dfc..499e922 100644 >> --- a/drivers/pci/pci.c >> +++ b/drivers/pci/pci.c >> @@ -3872,7 +3872,7 @@ static void pci_flr_wait(struct pci_dev *dev) >> * Returns true if the device advertises support for PCIe function level >> * resets. >> */ >> -static bool pcie_has_flr(struct pci_dev *dev) >> +bool pcie_has_flr(struct pci_dev *dev) >> { >> u32 cap; >> >> @@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) >> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); >> return cap & PCI_EXP_DEVCAP_FLR; >> } >> +EXPORT_SYMBOL_GPL(pcie_has_flr); > I'd rather change pcie_flr() so you could *always* call it, and it > would return 0, -ENOTTY, or whatever, based on whether FLR is > supported. Is that feasible? Sure, I will add pcie_has_flr() logic inside pcie_flr() and return appropriate values as suggested by you. Do we still want to retain pcie_has_flr() and its usage inside pci.c?.Otherwise, I will remove it and do required cleanup. Please let me know. Cheers GOVINDA
On Mon, Dec 11, 2017 at 06:29:29PM -0600, Govinda Tatti wrote: > > Thanks Bjorn for your review comments. Please see below for my comments. > > On 12/8/2017 2:24 PM, Bjorn Helgaas wrote: > >On Thu, Dec 07, 2017 at 05:21:44PM -0500, Govinda Tatti wrote: > >>This patch exports pcie_has_flr() and it is being used by Xen pciback > >>driver to reset (flr/slot/bus) PCI devices based on 'reset' SysFS > >>attribute. > >> > >>Signed-off-by: Govinda Tatti <Govinda.Tatti@Oracle.COM> > >>--- > >>v3: -New > >> > >> drivers/pci/pci.c | 3 ++- > >> include/linux/pci.h | 1 + > >> 2 files changed, 3 insertions(+), 1 deletion(-) > >> > >>diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c > >>index 6078dfc..499e922 100644 > >>--- a/drivers/pci/pci.c > >>+++ b/drivers/pci/pci.c > >>@@ -3872,7 +3872,7 @@ static void pci_flr_wait(struct pci_dev *dev) > >> * Returns true if the device advertises support for PCIe function level > >> * resets. > >> */ > >>-static bool pcie_has_flr(struct pci_dev *dev) > >>+bool pcie_has_flr(struct pci_dev *dev) > >> { > >> u32 cap; > >>@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) > >> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > >> return cap & PCI_EXP_DEVCAP_FLR; > >> } > >>+EXPORT_SYMBOL_GPL(pcie_has_flr); > >I'd rather change pcie_flr() so you could *always* call it, and it > >would return 0, -ENOTTY, or whatever, based on whether FLR is > >supported. Is that feasible? > Sure, I will add pcie_has_flr() logic inside pcie_flr() and return > appropriate > values as suggested by you. Do we still want to retain pcie_has_flr() and > its usage inside pci.c?.Otherwise, I will remove it and do required cleanup. If you can restructure the code and remove pcie_has_flr() while retaining the existing behavior of its callers, that would be great. Bjorn
On Fri, Dec 08, 2017 at 02:24:24PM -0600, Bjorn Helgaas wrote: > I'd rather change pcie_flr() so you could *always* call it, and it > would return 0, -ENOTTY, or whatever, based on whether FLR is > supported. Is that feasible? > > I don't like the "Can I do this? Ok, do this" style of interfaces. > It's racy (not really applicable in this case) and seems clunky. I was tempted to change all that for the whole reset sequence but didn't dare to do that because someone probably put some thought into the current scheme. I'd love to get rid of the "can I do this" interfaces entirely, and also change to EOPNOTSUPP for the not supported case.
>>>> -static bool pcie_has_flr(struct pci_dev *dev) >>>> +bool pcie_has_flr(struct pci_dev *dev) >>>> { >>>> u32 cap; >>>> @@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) >>>> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); >>>> return cap & PCI_EXP_DEVCAP_FLR; >>>> } >>>> +EXPORT_SYMBOL_GPL(pcie_has_flr); >>> I'd rather change pcie_flr() so you could *always* call it, and it >>> would return 0, -ENOTTY, or whatever, based on whether FLR is >>> supported. Is that feasible? >> Sure, I will add pcie_has_flr() logic inside pcie_flr() and return >> appropriate >> values as suggested by you. Do we still want to retain pcie_has_flr() and >> its usage inside pci.c?.Otherwise, I will remove it and do required cleanup. > If you can restructure the code and remove pcie_has_flr() while > retaining the existing behavior of its callers, that would be great. I checked the current usage of pcie_has_flr() and pcie_flr(). I have a couple of questions or need some clarification. 1. pcie_has_flr() usage inside pci_probe_reset_function(). This function is only calling pcie_has_flr() but not pcie_flr(). Rest of the code is trying to do specific type of reset except pcie_flr(). rc = pci_dev_specific_reset(dev, 1); if (rc != -ENOTTY) return rc; if (pcie_has_flr(dev)) return 0; rc = pci_af_flr(dev, 1); if (rc != -ENOTTY) return rc; In other-words, I can remove usage of pcie_has_flr() in all other places in pci.c except in above function. 2. W.r.t pcie_flr(), I am planning to return error code. Currently, the following file/modules are calling this function. My plan is to add a check for return code and print a WANRING message if return code is NON-ZERO. I hope this is sufficient for this patch. drivers/crypto/qat/qat_common/adf_aer.c drivers/infiniband/hw/hfi1/chip.c (2 places) drivers/net/ethernet/cavium/liquidio/lio_vf_main.c drivers/net/ethernet/intel/ixgbe/ixgbe_main.c (2 places) drivers/pci/quirks.c (2 places) Cheers GOVINDA
[+cc Christoph] On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote: > > >>>>-static bool pcie_has_flr(struct pci_dev *dev) > >>>>+bool pcie_has_flr(struct pci_dev *dev) > >>>> { > >>>> u32 cap; > >>>>@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) > >>>> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > >>>> return cap & PCI_EXP_DEVCAP_FLR; > >>>> } > >>>>+EXPORT_SYMBOL_GPL(pcie_has_flr); > >>>I'd rather change pcie_flr() so you could *always* call it, and it > >>>would return 0, -ENOTTY, or whatever, based on whether FLR is > >>>supported. Is that feasible? > >>Sure, I will add pcie_has_flr() logic inside pcie_flr() and return > >>appropriate > >>values as suggested by you. Do we still want to retain pcie_has_flr() and > >>its usage inside pci.c?.Otherwise, I will remove it and do required cleanup. > >If you can restructure the code and remove pcie_has_flr() while > >retaining the existing behavior of its callers, that would be great. > I checked the current usage of pcie_has_flr() and pcie_flr(). I have > a couple > of questions or need some clarification. > > 1. pcie_has_flr() usage inside pci_probe_reset_function(). > > This function is only calling pcie_has_flr() but not pcie_flr(). > Rest of the code is trying to do specific type of reset except > pcie_flr(). > > rc = pci_dev_specific_reset(dev, 1); > if (rc != -ENOTTY) > return rc; > if (pcie_has_flr(dev)) > return 0; > rc = pci_af_flr(dev, 1); > if (rc != -ENOTTY) > return rc; > > In other-words, I can remove usage of pcie_has_flr() in all other places > in pci.c except in above function. I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69 ("PCI: Export pcie_flr()"), but revert the restructuring part. Prior to a60a2b73ba69, we had int pcie_flr(struct pci_dev *dev, int probe); like all the other reset methods. AFAICT, the addition of pcie_has_flr() was to optimize the path slightly because when drivers call pcie_flr(), they should already know that their hardware supports FLR. But I don't think that optimization is worth the extra code complexity. If we do need to optimize it, we can check this in the core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET accordingly. Christoph, chime in if I'm missing something here. > 2. W.r.t pcie_flr(), I am planning to return error code. Currently, > the following > file/modules are calling this function. My plan is to add a check > for return > code and print a WANRING message if return code is NON-ZERO. I > hope this is > sufficient for this patch. > > drivers/crypto/qat/qat_common/adf_aer.c > drivers/infiniband/hw/hfi1/chip.c (2 places) > drivers/net/ethernet/cavium/liquidio/lio_vf_main.c > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c (2 places) > drivers/pci/quirks.c (2 places) Checking the return code is probably overkill, since pcie_flr() is void and returns no errors now. The only point of the return value is to tell whether the device supports FLR. If we call it with "probe == 0" there's no useful error to return. Bjorn
On Wed, Dec 13, 2017 at 03:24:21PM -0600, Bjorn Helgaas wrote: > Prior to a60a2b73ba69, we had > > int pcie_flr(struct pci_dev *dev, int probe); > > like all the other reset methods. AFAICT, the addition of > pcie_has_flr() was to optimize the path slightly because when drivers > call pcie_flr(), they should already know that their hardware supports > FLR. But I don't think that optimization is worth the extra code > complexity. If we do need to optimize it, we can check this in the > core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET > accordingly. > > Christoph, chime in if I'm missing something here. Didn't we just have that discussion in another thread a few days ago? I think that the pcie_has_flr was a mistake in retrospective but I think the bool probe API was an even bigger mistake. The only use of it is to hide the reset attribute in sysfs. I'd much rather always have it and have it return EOPNOTSUPP if no reset method is supported. I can send a patch for that if it sounds fine to you.
On Thu, Dec 14, 2017 at 04:52:06AM -0800, Christoph Hellwig wrote: > On Wed, Dec 13, 2017 at 03:24:21PM -0600, Bjorn Helgaas wrote: > > Prior to a60a2b73ba69, we had > > > > int pcie_flr(struct pci_dev *dev, int probe); > > > > like all the other reset methods. AFAICT, the addition of > > pcie_has_flr() was to optimize the path slightly because when drivers > > call pcie_flr(), they should already know that their hardware supports > > FLR. But I don't think that optimization is worth the extra code > > complexity. If we do need to optimize it, we can check this in the > > core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET > > accordingly. > > > > Christoph, chime in if I'm missing something here. > > Didn't we just have that discussion in another thread a few days > ago? Probably, but I didn't have a clear picture of what you were suggesting. > I think that the pcie_has_flr was a mistake in retrospective but I > think the bool probe API was an even bigger mistake. The only use > of it is to hide the reset attribute in sysfs. I'd much rather always > have it and have it return EOPNOTSUPP if no reset method is supported. > > I can send a patch for that if it sounds fine to you. If you can get rid of the whole probe infrastructure, that sounds good to me. Bjorn
Thanks Bjorn and Christophfor your response. Please see below for my comments. On 12/13/2017 3:24 PM, Bjorn Helgaas wrote: > [+cc Christoph] > > On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote: >>>>>> -static bool pcie_has_flr(struct pci_dev *dev) >>>>>> +bool pcie_has_flr(struct pci_dev *dev) >>>>>> { >>>>>> u32 cap; >>>>>> @@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) >>>>>> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); >>>>>> return cap & PCI_EXP_DEVCAP_FLR; >>>>>> } >>>>>> +EXPORT_SYMBOL_GPL(pcie_has_flr); >>>>> I'd rather change pcie_flr() so you could *always* call it, and it >>>>> would return 0, -ENOTTY, or whatever, based on whether FLR is >>>>> supported. Is that feasible? >>>> Sure, I will add pcie_has_flr() logic inside pcie_flr() and return >>>> appropriate >>>> values as suggested by you. Do we still want to retain pcie_has_flr() and >>>> its usage inside pci.c?.Otherwise, I will remove it and do required cleanup. >>> If you can restructure the code and remove pcie_has_flr() while >>> retaining the existing behavior of its callers, that would be great. >> I checked the current usage of pcie_has_flr() and pcie_flr(). I have >> a couple >> of questions or need some clarification. >> >> 1. pcie_has_flr() usage inside pci_probe_reset_function(). >> >> This function is only calling pcie_has_flr() but not pcie_flr(). >> Rest of the code is trying to do specific type of reset except >> pcie_flr(). >> >> rc = pci_dev_specific_reset(dev, 1); >> if (rc != -ENOTTY) >> return rc; >> if (pcie_has_flr(dev)) >> return 0; >> rc = pci_af_flr(dev, 1); >> if (rc != -ENOTTY) >> return rc; >> >> In other-words, I can remove usage of pcie_has_flr() in all other places >> in pci.c except in above function. > I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69 > ("PCI: Export pcie_flr()"), but revert the restructuring part. > > Prior to a60a2b73ba69, we had > > int pcie_flr(struct pci_dev *dev, int probe); > > like all the other reset methods. AFAICT, the addition of > pcie_has_flr() was to optimize the path slightly because when drivers > call pcie_flr(), they should already know that their hardware supports > FLR. But I don't think that optimization is worth the extra code > complexity. If we do need to optimize it, we can check this in the > core during enumeration and set PCI_DEV_FLAGS_NO_FLR_RESET > accordingly. > > Christoph, chime in if I'm missing something here. Not all code paths are aware of FLR capability and also, not using pcie_flr(). For example, arch/powerpc/platforms/powernv/eeh-powernv.c drivers/crypto/cavium/nitrox/nitrox_main.c drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c So, we should consider one of these options. - set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported. - pcie_flr() should return if it is not supported If we modify pcie_flr() to return error codes, then we need to modify all existing modules that are calling this function. Please let me know your preference, so that I can move accordingly. Thanks. Cheers GOVINDA
[+cc Russell, Sinan, Herbert, Srikanth, Derek, Satanand, Felix, Raghu] On Fri, Dec 15, 2017 at 09:48:02AM -0600, Govinda Tatti wrote: > On 12/13/2017 3:24 PM, Bjorn Helgaas wrote: > >On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote: > >>>>>>-static bool pcie_has_flr(struct pci_dev *dev) > >>>>>>+bool pcie_has_flr(struct pci_dev *dev) > >>>>>> { > >>>>>> u32 cap; > >>>>>>@@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) > >>>>>> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); > >>>>>> return cap & PCI_EXP_DEVCAP_FLR; > >>>>>> } > >>>>>>+EXPORT_SYMBOL_GPL(pcie_has_flr); > >>>>>I'd rather change pcie_flr() so you could *always* call it, and > >>>>>it would return 0, -ENOTTY, or whatever, based on whether FLR > >>>>>is supported. Is that feasible? > >>>>Sure, I will add pcie_has_flr() logic inside pcie_flr() and > >>>>return appropriate values as suggested by you. Do we still want > >>>>to retain pcie_has_flr() and its usage inside pci.c?.Otherwise, > >>>>I will remove it and do required cleanup. > >>>If you can restructure the code and remove pcie_has_flr() while > >>>retaining the existing behavior of its callers, that would be > >>>great. > >>I checked the current usage of pcie_has_flr() and pcie_flr(). I > >>have a couple of questions or need some clarification. > >> > >>1. pcie_has_flr() usage inside pci_probe_reset_function(). > >> > >> This function is only calling pcie_has_flr() but not pcie_flr(). > >> Rest of the code is trying to do specific type of reset except > >> pcie_flr(). > >> > >> rc = pci_dev_specific_reset(dev, 1); > >> if (rc != -ENOTTY) > >> return rc; > >> if (pcie_has_flr(dev)) > >> return 0; > >> rc = pci_af_flr(dev, 1); > >> if (rc != -ENOTTY) > >> return rc; > >> > >> In other-words, I can remove usage of pcie_has_flr() in all > >> other places in pci.c except in above function. > >I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69 > >("PCI: Export pcie_flr()"), but revert the restructuring part. > > > >Prior to a60a2b73ba69, we had > > > > int pcie_flr(struct pci_dev *dev, int probe); > > > >like all the other reset methods. AFAICT, the addition of > >pcie_has_flr() was to optimize the path slightly because when > >drivers call pcie_flr(), they should already know that their > >hardware supports FLR. But I don't think that optimization is > >worth the extra code complexity. If we do need to optimize it, we > >can check this in the core during enumeration and set > >PCI_DEV_FLAGS_NO_FLR_RESET accordingly. > Not all code paths are aware of FLR capability and also, not > using pcie_flr(). For example, > > arch/powerpc/platforms/powernv/eeh-powernv.c I assume you're referring to pnv_eeh_do_flr() (which contains code similar to pcie_flr()) and pnv_eeh_do_af_flr() (which has code similar to pci_af_flr()). I agree that those are problematic and would ideally be unified with the PCI core implementations. Powerpc has quite a bit of this sort of special-case code for several reasons, some just historical and some more concrete, so I don't know how feasible this is. > drivers/crypto/cavium/nitrox/nitrox_main.c This has nitrox_reset_device(), which should definitely be replaced with a core interface. > drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c And this has octeon_mbox_process_cmd() which also does a home-grown PCI_EXP_DEVCTL_BCR_FLR request and also should definitely use a core interface. > So, we should consider one of these options. > > - set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported. > - pcie_flr() should return if it is not supported > > If we modify pcie_flr() to return error codes, then we need to modify > all existing modules that are calling this function. Yes, of course. > Please let me know your preference, so that I can move accordingly. Thanks. I think Christoph volunteered to do some restructuring, but I don't know his timeframe. If you can, I would probably wait for that because there's so much overlap here. The other paths that use PCI_EXP_DEVCTL_BCR_FLR are definitely issues and should be fixed, but again should wait for the revised pcie_flr() interface. And if they're not actually required for your Xen issue, they sound like "nice to have" cleanups that will not gate your Xen fixes. I added this to my ever-growing list of cleanups to do. Bjorn
Thanks Bjorn for your response. Please see below for my comments. >> So, we should consider one of these options. >> >> - set PCI_DEV_FLAGS_NO_FLR_RESET if it is not supported. >> - pcie_flr() should return if it is not supported >> >> If we modify pcie_flr() to return error codes, then we need to modify >> all existing modules that are calling this function. > Yes, of course. > >> Please let me know your preference, so that I can move accordingly. Thanks. > I think Christoph volunteered to do some restructuring, but I don't > know his timeframe. If you can, I would probably wait for that > because there's so much overlap here. OK. > > The other paths that use PCI_EXP_DEVCTL_BCR_FLR are definitely issues > and should be fixed, but again should wait for the revised pcie_flr() > interface. And if they're not actually required for your Xen issue, > they sound like "nice to have" cleanups that will not gate your Xen > fixes. I added this to my ever-growing list of cleanups to do. For now, I am planning to use existing pcie_flr() after checking FLR capability inside Xenpciback driver (like other existing pcie_flr() usage). We will switch to revised pcie_flr() once it is available. Cheers GOVINDA
On 16/12/17 05:18, Bjorn Helgaas wrote: > [+cc Russell, Sinan, Herbert, Srikanth, Derek, Satanand, Felix, Raghu] > > On Fri, Dec 15, 2017 at 09:48:02AM -0600, Govinda Tatti wrote: >> On 12/13/2017 3:24 PM, Bjorn Helgaas wrote: >>> On Wed, Dec 13, 2017 at 02:46:57PM -0600, Govinda Tatti wrote: > >>>>>>>> -static bool pcie_has_flr(struct pci_dev *dev) >>>>>>>> +bool pcie_has_flr(struct pci_dev *dev) >>>>>>>> { >>>>>>>> u32 cap; >>>>>>>> @@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) >>>>>>>> pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); >>>>>>>> return cap & PCI_EXP_DEVCAP_FLR; >>>>>>>> } >>>>>>>> +EXPORT_SYMBOL_GPL(pcie_has_flr); > >>>>>>> I'd rather change pcie_flr() so you could *always* call it, and >>>>>>> it would return 0, -ENOTTY, or whatever, based on whether FLR >>>>>>> is supported. Is that feasible? > >>>>>> Sure, I will add pcie_has_flr() logic inside pcie_flr() and >>>>>> return appropriate values as suggested by you. Do we still want >>>>>> to retain pcie_has_flr() and its usage inside pci.c?.Otherwise, >>>>>> I will remove it and do required cleanup. > >>>>> If you can restructure the code and remove pcie_has_flr() while >>>>> retaining the existing behavior of its callers, that would be >>>>> great. > >>>> I checked the current usage of pcie_has_flr() and pcie_flr(). I >>>> have a couple of questions or need some clarification. >>>> >>>> 1. pcie_has_flr() usage inside pci_probe_reset_function(). >>>> >>>> This function is only calling pcie_has_flr() but not pcie_flr(). >>>> Rest of the code is trying to do specific type of reset except >>>> pcie_flr(). >>>> >>>> rc = pci_dev_specific_reset(dev, 1); >>>> if (rc != -ENOTTY) >>>> return rc; >>>> if (pcie_has_flr(dev)) >>>> return 0; >>>> rc = pci_af_flr(dev, 1); >>>> if (rc != -ENOTTY) >>>> return rc; >>>> >>>> In other-words, I can remove usage of pcie_has_flr() in all >>>> other places in pci.c except in above function. > >>> I think we should keep the EXPORT_SYMBOL_GPL() part of a60a2b73ba69 >>> ("PCI: Export pcie_flr()"), but revert the restructuring part. >>> >>> Prior to a60a2b73ba69, we had >>> >>> int pcie_flr(struct pci_dev *dev, int probe); >>> >>> like all the other reset methods. AFAICT, the addition of >>> pcie_has_flr() was to optimize the path slightly because when >>> drivers call pcie_flr(), they should already know that their >>> hardware supports FLR. But I don't think that optimization is >>> worth the extra code complexity. If we do need to optimize it, we >>> can check this in the core during enumeration and set >>> PCI_DEV_FLAGS_NO_FLR_RESET accordingly. > >> Not all code paths are aware of FLR capability and also, not >> using pcie_flr(). For example, >> >> arch/powerpc/platforms/powernv/eeh-powernv.c > > I assume you're referring to pnv_eeh_do_flr() (which contains code similar > to pcie_flr()) and pnv_eeh_do_af_flr() (which has code similar to > pci_af_flr()). I agree that those are problematic and would ideally be > unified with the PCI core implementations. > > Powerpc has quite a bit of this sort of special-case code for several > reasons, some just historical and some more concrete, so I don't know how > feasible this is. It would be lovely if pnv-eeh code used pci_af_flr() but since pnv_eeh_do_flr() uses different config space accessors (not sure why exactly, probably to avoid freezing the entire PHB), it is harder than just trivial change. I'll try and have a deeper look though.
On Fri, Dec 15, 2017 at 12:18:02PM -0600, Bjorn Helgaas wrote: > I think Christoph volunteered to do some restructuring, but I don't > know his timeframe. If you can, I would probably wait for that > because there's so much overlap here. I'll have some time over the holidays. If you need it more urgent than that feel free to take over.
On 12/18/2017 6:26 AM, Christoph Hellwig wrote: > On Fri, Dec 15, 2017 at 12:18:02PM -0600, Bjorn Helgaas wrote: >> I think Christoph volunteered to do some restructuring, but I don't >> know his timeframe. If you can, I would probably wait for that >> because there's so much overlap here. > I'll have some time over the holidays. If you need it more urgent > than that feel free to take over. We will wait for your changes. Thanks Christoph. Cheers GOVINDA
On Mon, Dec 18, 2017 at 04:26:30AM -0800, Christoph Hellwig wrote: > On Fri, Dec 15, 2017 at 12:18:02PM -0600, Bjorn Helgaas wrote: > > I think Christoph volunteered to do some restructuring, but I don't > > know his timeframe. If you can, I would probably wait for that > > because there's so much overlap here. > > I'll have some time over the holidays. If you need it more urgent > than that feel free to take over. > Replying to an old thread.. I noticed pcie_has_flr() has been recently exported in upstream Linux: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d2917f7747805a1f4188672f308d82a8ba01700 Are there more changes / cleanups planned to these interfaces, as mentioned last year? (context: xen-pciback reset/do_flr features upstreaming, which kind of stalled last year when pcie_has_flr() wasn't exported at the time) Thanks, -- Pasi
On 9/9/2018 2:59 PM, Pasi Kärkkäinen wrote: > I noticed pcie_has_flr() has been recently exported in upstream Linux: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d2917f7747805a1f4188672f308d82a8ba01700 > > Are there more changes / cleanups planned to these interfaces, as mentioned last year? > > (context: xen-pciback reset/do_flr features upstreaming, which kind of stalled last year when pcie_has_flr() wasn't exported at the time) Exporting pcie_has_flr() is a very simple change which could have been done by the XEN porting effort. Maybe, the right question is what is so special about XEN reset? What feature PCI core is missing to support XEN FLR reset that caused the effort to stall?
Hi, On Sun, Sep 09, 2018 at 10:33:02PM -0400, Sinan Kaya wrote: > On 9/9/2018 2:59 PM, Pasi Kärkkäinen wrote: > >I noticed pcie_has_flr() has been recently exported in upstream Linux: > >https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d2917f7747805a1f4188672f308d82a8ba01700 > > > >Are there more changes / cleanups planned to these interfaces, as mentioned last year? > > > >(context: xen-pciback reset/do_flr features upstreaming, which kind of stalled last year when pcie_has_flr() wasn't exported at the time) > > Exporting pcie_has_flr() is a very simple change which could have been done > by the XEN porting effort. > > Maybe, the right question is what is so special about XEN reset? > > What feature PCI core is missing to support XEN FLR reset that caused > the effort to stall? > Well one of the reasons probably was because Christoph was planning to deprecate the pcie_has_flr() functionality.. https://lists.xen.org/archives/html/xen-devel/2017-12/msg01057.html https://lists.xen.org/archives/html/xen-devel/2017-12/msg01252.html But now that pcie_has_flr() is exported and available I guess it's fine to continue using it also for xen-pciback :) Thanks, -- Pasi
On 9/10/2018 5:52 AM, Pasi Kärkkäinen wrote: > Hi, > > On Sun, Sep 09, 2018 at 10:33:02PM -0400, Sinan Kaya wrote: >> On 9/9/2018 2:59 PM, Pasi Kärkkäinen wrote: >>> I noticed pcie_has_flr() has been recently exported in upstream Linux: >>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=2d2917f7747805a1f4188672f308d82a8ba01700 >>> >>> Are there more changes / cleanups planned to these interfaces, as mentioned last year? >>> >>> (context: xen-pciback reset/do_flr features upstreaming, which kind of stalled last year when pcie_has_flr() wasn't exported at the time) >> >> Exporting pcie_has_flr() is a very simple change which could have been done >> by the XEN porting effort. >> >> Maybe, the right question is what is so special about XEN reset? >> >> What feature PCI core is missing to support XEN FLR reset that caused >> the effort to stall? >> > > Well one of the reasons probably was because Christoph was planning to deprecate the pcie_has_flr() functionality.. > > https://lists.xen.org/archives/html/xen-devel/2017-12/msg01057.html > https://lists.xen.org/archives/html/xen-devel/2017-12/msg01252.html > > But now that pcie_has_flr() is exported and available I guess it's fine to continue using it also for xen-pciback :) > Yeah, I would go ahead with the implementation. Refactoring can be done independently. > > Thanks, > > -- Pasi > >
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c index 6078dfc..499e922 100644 --- a/drivers/pci/pci.c +++ b/drivers/pci/pci.c @@ -3872,7 +3872,7 @@ static void pci_flr_wait(struct pci_dev *dev) * Returns true if the device advertises support for PCIe function level * resets. */ -static bool pcie_has_flr(struct pci_dev *dev) +bool pcie_has_flr(struct pci_dev *dev) { u32 cap; @@ -3882,6 +3882,7 @@ static bool pcie_has_flr(struct pci_dev *dev) pcie_capability_read_dword(dev, PCI_EXP_DEVCAP, &cap); return cap & PCI_EXP_DEVCAP_FLR; } +EXPORT_SYMBOL_GPL(pcie_has_flr); /** * pcie_flr - initiate a PCIe function level reset diff --git a/include/linux/pci.h b/include/linux/pci.h index d16a7c0..44bf2b5 100644 --- a/include/linux/pci.h +++ b/include/linux/pci.h @@ -1089,6 +1089,7 @@ int pcie_get_mps(struct pci_dev *dev); int pcie_set_mps(struct pci_dev *dev, int mps); int pcie_get_minimum_link(struct pci_dev *dev, enum pci_bus_speed *speed, enum pcie_link_width *width); +bool pcie_has_flr(struct pci_dev *dev); void pcie_flr(struct pci_dev *dev); int __pci_reset_function(struct pci_dev *dev); int __pci_reset_function_locked(struct pci_dev *dev);
This patch exports pcie_has_flr() and it is being used by Xen pciback driver to reset (flr/slot/bus) PCI devices based on 'reset' SysFS attribute. Signed-off-by: Govinda Tatti <Govinda.Tatti@Oracle.COM> --- v3: -New drivers/pci/pci.c | 3 ++- include/linux/pci.h | 1 + 2 files changed, 3 insertions(+), 1 deletion(-)