Patchwork [V3,1/2] Drivers/PCI: Export pcie_has_flr() interface

login
register
mail settings
Submitter Govinda Tatti
Date Dec. 7, 2017, 10:21 p.m.
Message ID <20171207222145.9769-2-Govinda.Tatti@Oracle.COM>
Download mbox | patch
Permalink /patch/10101195/
State New
Headers show

Comments

Govinda Tatti - Dec. 7, 2017, 10:21 p.m.
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(-)
Bjorn Helgaas - Dec. 8, 2017, 8:24 p.m.
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
>
Govinda Tatti - Dec. 12, 2017, 12:29 a.m.
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
Bjorn Helgaas - Dec. 12, 2017, 12:59 a.m.
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
Christoph Hellwig - Dec. 12, 2017, 3:07 p.m.
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.
Govinda Tatti - Dec. 13, 2017, 8:46 p.m.
>>>> -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
Bjorn Helgaas - Dec. 13, 2017, 9:24 p.m.
[+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
Christoph Hellwig - Dec. 14, 2017, 12:52 p.m.
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.
Bjorn Helgaas - Dec. 15, 2017, 12:24 a.m.
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
Govinda Tatti - Dec. 15, 2017, 3:48 p.m.
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
Bjorn Helgaas - Dec. 15, 2017, 6:18 p.m.
[+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
Govinda Tatti - Dec. 15, 2017, 8:01 p.m.
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
Alexey Kardashevskiy - Dec. 18, 2017, 3:09 a.m.
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.
Christoph Hellwig - Dec. 18, 2017, 12:26 p.m.
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.
Govinda Tatti - Dec. 18, 2017, 5:22 p.m.
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

Patch

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);