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

Message ID 20171207222145.9769-2-Govinda.Tatti@Oracle.COM
State Changes Requested
Headers show
Series
  • Xen/PCIback: PCI reset using 'reset' SysFS attribute
Related show

Commit Message

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

Comments

Bjorn Helgaas Dec. 8, 2017, 8:24 p.m. | #1
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. | #2
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. | #3
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. | #4
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. | #5
>>>> -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. | #6
[+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. | #7
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. | #8
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

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