Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
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
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
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.
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
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. -- Alexey
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
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
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
[+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
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
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
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
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
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
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.
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
[+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
Re: [Xen-devel] [PATCH V3 1/2] Drivers/PCI: Export pcie_has_flr() interface
-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