[PATCH] PCI: merge slot and bus reset implementations

[PATCH] PCI: merge slot and bus reset implementations

[Raphael Norwitz]

Slot resets are bus resets with additional logic to prevent a device
from being removed during the reset. Currently slot and bus resets have
separate implementations in pci.c, complicating higher level logic. As
discussed on the mailing list, they should be combined into a generic
function which performs an SBR. This change adds a function,
pci_reset_bus_function(), which first attempts a slot reset and then
attempts a bus reset if -ENOTTY is returned, such that there is now a
single device agnostic function to perform an SBR.

This new function is also needed to add SBR reset quirks and therefore
is exposed in pci.h.

Link: https://lkml.org/lkml/2021/3/23/911

Suggested-by: Alex Williamson <alex.williamson@redhat.com>
Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
---
 drivers/pci/pci.c   | 17 +++++++++--------
 include/linux/pci.h |  1 +
 2 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 16a17215f633..12a91af2ade4 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -4982,6 +4982,13 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
 	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
 }
 
+int pci_reset_bus_function(struct pci_dev *dev, int probe)
+{
+	int rc = pci_dev_reset_slot_function(dev, probe);
+
+	return (rc == -ENOTTY) ? pci_parent_bus_reset(dev, probe) : rc;
+}
+
 static void pci_dev_lock(struct pci_dev *dev)
 {
 	pci_cfg_access_lock(dev);
@@ -5102,10 +5109,7 @@ int __pci_reset_function_locked(struct pci_dev *dev)
 	rc = pci_pm_reset(dev, 0);
 	if (rc != -ENOTTY)
 		return rc;
-	rc = pci_dev_reset_slot_function(dev, 0);
-	if (rc != -ENOTTY)
-		return rc;
-	return pci_parent_bus_reset(dev, 0);
+	return pci_reset_bus_function(dev, 0);
 }
 EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
 
@@ -5135,13 +5139,10 @@ int pci_probe_reset_function(struct pci_dev *dev)
 	if (rc != -ENOTTY)
 		return rc;
 	rc = pci_pm_reset(dev, 1);
-	if (rc != -ENOTTY)
-		return rc;
-	rc = pci_dev_reset_slot_function(dev, 1);
 	if (rc != -ENOTTY)
 		return rc;
 
-	return pci_parent_bus_reset(dev, 1);
+	return pci_reset_bus_function(dev, 1);
 }
 
 /**
diff --git a/include/linux/pci.h b/include/linux/pci.h
index 86c799c97b77..979d54335ac1 100644
--- a/include/linux/pci.h
+++ b/include/linux/pci.h
@@ -1228,6 +1228,7 @@ int pci_probe_reset_bus(struct pci_bus *bus);
 int pci_reset_bus(struct pci_dev *dev);
 void pci_reset_secondary_bus(struct pci_dev *dev);
 void pcibios_reset_secondary_bus(struct pci_dev *dev);
+int pci_reset_bus_function(struct pci_dev *dev, int probe);
 void pci_update_resource(struct pci_dev *dev, int resno);
 int __must_check pci_assign_resource(struct pci_dev *dev, int i);
 int __must_check pci_reassign_resource(struct pci_dev *dev, int i, resource_size_t add_size, resource_size_t align);
-- 
2.20.1

Re: [PATCH] PCI: merge slot and bus reset implementations

[Leon Romanovsky]

On Thu, Apr 01, 2021 at 05:37:16AM +0000, Raphael Norwitz wrote:
> Slot resets are bus resets with additional logic to prevent a device
> from being removed during the reset. Currently slot and bus resets have
> separate implementations in pci.c, complicating higher level logic. As
> discussed on the mailing list, they should be combined into a generic
> function which performs an SBR. This change adds a function,
> pci_reset_bus_function(), which first attempts a slot reset and then
> attempts a bus reset if -ENOTTY is returned, such that there is now a
> single device agnostic function to perform an SBR.
> 
> This new function is also needed to add SBR reset quirks and therefore
> is exposed in pci.h.
> 
> Link: https://lkml.org/lkml/2021/3/23/911
> 
> Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> ---
>  drivers/pci/pci.c   | 17 +++++++++--------
>  include/linux/pci.h |  1 +
>  2 files changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 16a17215f633..12a91af2ade4 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -4982,6 +4982,13 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
>  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
>  }
>  
> +int pci_reset_bus_function(struct pci_dev *dev, int probe)
> +{
> +	int rc = pci_dev_reset_slot_function(dev, probe);
> +
> +	return (rc == -ENOTTY) ? pci_parent_bus_reset(dev, probe) : rc;

The previous coding style is preferable one in the Linux kernel.
int rc = pci_dev_reset_slot_function(dev, probe);
if (rc != -ENOTTY)
  return rc;
return pci_parent_bus_reset(dev, probe);


> +}
> +
>  static void pci_dev_lock(struct pci_dev *dev)
>  {
>  	pci_cfg_access_lock(dev);
> @@ -5102,10 +5109,7 @@ int __pci_reset_function_locked(struct pci_dev *dev)
>  	rc = pci_pm_reset(dev, 0);
>  	if (rc != -ENOTTY)
>  		return rc;
> -	rc = pci_dev_reset_slot_function(dev, 0);
> -	if (rc != -ENOTTY)
> -		return rc;
> -	return pci_parent_bus_reset(dev, 0);
> +	return pci_reset_bus_function(dev, 0);
>  }
>  EXPORT_SYMBOL_GPL(__pci_reset_function_locked);
>  
> @@ -5135,13 +5139,10 @@ int pci_probe_reset_function(struct pci_dev *dev)
>  	if (rc != -ENOTTY)
>  		return rc;
>  	rc = pci_pm_reset(dev, 1);
> -	if (rc != -ENOTTY)
> -		return rc;
> -	rc = pci_dev_reset_slot_function(dev, 1);
>  	if (rc != -ENOTTY)
>  		return rc;
>  
> -	return pci_parent_bus_reset(dev, 1);
> +	return pci_reset_bus_function(dev, 1);
>  }
>  
>  /**
> diff --git a/include/linux/pci.h b/include/linux/pci.h
> index 86c799c97b77..979d54335ac1 100644
> --- a/include/linux/pci.h
> +++ b/include/linux/pci.h
> @@ -1228,6 +1228,7 @@ int pci_probe_reset_bus(struct pci_bus *bus);
>  int pci_reset_bus(struct pci_dev *dev);
>  void pci_reset_secondary_bus(struct pci_dev *dev);
>  void pcibios_reset_secondary_bus(struct pci_dev *dev);
> +int pci_reset_bus_function(struct pci_dev *dev, int probe);
>  void pci_update_resource(struct pci_dev *dev, int resno);
>  int __must_check pci_assign_resource(struct pci_dev *dev, int i);
>  int __must_check pci_reassign_resource(struct pci_dev *dev, int i, resource_size_t add_size, resource_size_t align);
> -- 
> 2.20.1

Re: [PATCH] PCI: merge slot and bus reset implementations

[Alex Williamson]

On Thu, 1 Apr 2021 15:27:37 +0300
Leon Romanovsky <leon@kernel.org> wrote:

> On Thu, Apr 01, 2021 at 05:37:16AM +0000, Raphael Norwitz wrote:
> > Slot resets are bus resets with additional logic to prevent a device
> > from being removed during the reset. Currently slot and bus resets have
> > separate implementations in pci.c, complicating higher level logic. As
> > discussed on the mailing list, they should be combined into a generic
> > function which performs an SBR. This change adds a function,
> > pci_reset_bus_function(), which first attempts a slot reset and then
> > attempts a bus reset if -ENOTTY is returned, such that there is now a
> > single device agnostic function to perform an SBR.
> > 
> > This new function is also needed to add SBR reset quirks and therefore
> > is exposed in pci.h.
> > 
> > Link: https://lkml.org/lkml/2021/3/23/911
> > 
> > Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> > ---
> >  drivers/pci/pci.c   | 17 +++++++++--------
> >  include/linux/pci.h |  1 +
> >  2 files changed, 10 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 16a17215f633..12a91af2ade4 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -4982,6 +4982,13 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
> >  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
> >  }
> >  
> > +int pci_reset_bus_function(struct pci_dev *dev, int probe)
> > +{
> > +	int rc = pci_dev_reset_slot_function(dev, probe);
> > +
> > +	return (rc == -ENOTTY) ? pci_parent_bus_reset(dev, probe) : rc;  
> 
> The previous coding style is preferable one in the Linux kernel.
> int rc = pci_dev_reset_slot_function(dev, probe);
> if (rc != -ENOTTY)
>   return rc;
> return pci_parent_bus_reset(dev, probe);


That'd be news to me, do you have a reference?  I've never seen
complaints for ternaries previously.  Thanks,

Alex

Re: [PATCH] PCI: merge slot and bus reset implementations

[Leon Romanovsky]

On Thu, Apr 01, 2021 at 10:56:16AM -0600, Alex Williamson wrote:
> On Thu, 1 Apr 2021 15:27:37 +0300
> Leon Romanovsky <leon@kernel.org> wrote:
> 
> > On Thu, Apr 01, 2021 at 05:37:16AM +0000, Raphael Norwitz wrote:
> > > Slot resets are bus resets with additional logic to prevent a device
> > > from being removed during the reset. Currently slot and bus resets have
> > > separate implementations in pci.c, complicating higher level logic. As
> > > discussed on the mailing list, they should be combined into a generic
> > > function which performs an SBR. This change adds a function,
> > > pci_reset_bus_function(), which first attempts a slot reset and then
> > > attempts a bus reset if -ENOTTY is returned, such that there is now a
> > > single device agnostic function to perform an SBR.
> > > 
> > > This new function is also needed to add SBR reset quirks and therefore
> > > is exposed in pci.h.
> > > 
> > > Link: https://lkml.org/lkml/2021/3/23/911
> > > 
> > > Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> > > ---
> > >  drivers/pci/pci.c   | 17 +++++++++--------
> > >  include/linux/pci.h |  1 +
> > >  2 files changed, 10 insertions(+), 8 deletions(-)
> > > 
> > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > index 16a17215f633..12a91af2ade4 100644
> > > --- a/drivers/pci/pci.c
> > > +++ b/drivers/pci/pci.c
> > > @@ -4982,6 +4982,13 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
> > >  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
> > >  }
> > >  
> > > +int pci_reset_bus_function(struct pci_dev *dev, int probe)
> > > +{
> > > +	int rc = pci_dev_reset_slot_function(dev, probe);
> > > +
> > > +	return (rc == -ENOTTY) ? pci_parent_bus_reset(dev, probe) : rc;  
> > 
> > The previous coding style is preferable one in the Linux kernel.
> > int rc = pci_dev_reset_slot_function(dev, probe);
> > if (rc != -ENOTTY)
> >   return rc;
> > return pci_parent_bus_reset(dev, probe);
> 
> 
> That'd be news to me, do you have a reference?  I've never seen
> complaints for ternaries previously.  Thanks,

The complaint is not to ternaries, but to the function call as one of
the parameters, that makes it harder to read.

Thanks

> 
> Alex
> 

Re: [PATCH] PCI: merge slot and bus reset implementations

[Alex Williamson]

On Sun, 4 Apr 2021 11:04:32 +0300
Leon Romanovsky <leon@kernel.org> wrote:

> On Thu, Apr 01, 2021 at 10:56:16AM -0600, Alex Williamson wrote:
> > On Thu, 1 Apr 2021 15:27:37 +0300
> > Leon Romanovsky <leon@kernel.org> wrote:
> >   
> > > On Thu, Apr 01, 2021 at 05:37:16AM +0000, Raphael Norwitz wrote:  
> > > > Slot resets are bus resets with additional logic to prevent a device
> > > > from being removed during the reset. Currently slot and bus resets have
> > > > separate implementations in pci.c, complicating higher level logic. As
> > > > discussed on the mailing list, they should be combined into a generic
> > > > function which performs an SBR. This change adds a function,
> > > > pci_reset_bus_function(), which first attempts a slot reset and then
> > > > attempts a bus reset if -ENOTTY is returned, such that there is now a
> > > > single device agnostic function to perform an SBR.
> > > > 
> > > > This new function is also needed to add SBR reset quirks and therefore
> > > > is exposed in pci.h.
> > > > 
> > > > Link: https://lkml.org/lkml/2021/3/23/911
> > > > 
> > > > Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> > > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> > > > ---
> > > >  drivers/pci/pci.c   | 17 +++++++++--------
> > > >  include/linux/pci.h |  1 +
> > > >  2 files changed, 10 insertions(+), 8 deletions(-)
> > > > 
> > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > index 16a17215f633..12a91af2ade4 100644
> > > > --- a/drivers/pci/pci.c
> > > > +++ b/drivers/pci/pci.c
> > > > @@ -4982,6 +4982,13 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
> > > >  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
> > > >  }
> > > >  
> > > > +int pci_reset_bus_function(struct pci_dev *dev, int probe)
> > > > +{
> > > > +	int rc = pci_dev_reset_slot_function(dev, probe);
> > > > +
> > > > +	return (rc == -ENOTTY) ? pci_parent_bus_reset(dev, probe) : rc;    
> > > 
> > > The previous coding style is preferable one in the Linux kernel.
> > > int rc = pci_dev_reset_slot_function(dev, probe);
> > > if (rc != -ENOTTY)
> > >   return rc;
> > > return pci_parent_bus_reset(dev, probe);  
> > 
> > 
> > That'd be news to me, do you have a reference?  I've never seen
> > complaints for ternaries previously.  Thanks,  
> 
> The complaint is not to ternaries, but to the function call as one of
> the parameters, that makes it harder to read.

Sorry, I don't find a function call as a parameter to a ternary to be
extraordinary, nor do I find it to be a discouraged usage model within
the kernel.  This seems like a pretty low bar for hard to read code.

Re: [PATCH] PCI: merge slot and bus reset implementations

[Leon Romanovsky]

On Tue, Apr 06, 2021 at 08:16:26AM -0600, Alex Williamson wrote:
> On Sun, 4 Apr 2021 11:04:32 +0300
> Leon Romanovsky <leon@kernel.org> wrote:
> 
> > On Thu, Apr 01, 2021 at 10:56:16AM -0600, Alex Williamson wrote:
> > > On Thu, 1 Apr 2021 15:27:37 +0300
> > > Leon Romanovsky <leon@kernel.org> wrote:
> > >   
> > > > On Thu, Apr 01, 2021 at 05:37:16AM +0000, Raphael Norwitz wrote:  
> > > > > Slot resets are bus resets with additional logic to prevent a device
> > > > > from being removed during the reset. Currently slot and bus resets have
> > > > > separate implementations in pci.c, complicating higher level logic. As
> > > > > discussed on the mailing list, they should be combined into a generic
> > > > > function which performs an SBR. This change adds a function,
> > > > > pci_reset_bus_function(), which first attempts a slot reset and then
> > > > > attempts a bus reset if -ENOTTY is returned, such that there is now a
> > > > > single device agnostic function to perform an SBR.
> > > > > 
> > > > > This new function is also needed to add SBR reset quirks and therefore
> > > > > is exposed in pci.h.
> > > > > 
> > > > > Link: https://lkml.org/lkml/2021/3/23/911
> > > > > 
> > > > > Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> > > > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > > > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> > > > > ---
> > > > >  drivers/pci/pci.c   | 17 +++++++++--------
> > > > >  include/linux/pci.h |  1 +
> > > > >  2 files changed, 10 insertions(+), 8 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > index 16a17215f633..12a91af2ade4 100644
> > > > > --- a/drivers/pci/pci.c
> > > > > +++ b/drivers/pci/pci.c
> > > > > @@ -4982,6 +4982,13 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
> > > > >  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
> > > > >  }
> > > > >  
> > > > > +int pci_reset_bus_function(struct pci_dev *dev, int probe)
> > > > > +{
> > > > > +	int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > +
> > > > > +	return (rc == -ENOTTY) ? pci_parent_bus_reset(dev, probe) : rc;    
> > > > 
> > > > The previous coding style is preferable one in the Linux kernel.
> > > > int rc = pci_dev_reset_slot_function(dev, probe);
> > > > if (rc != -ENOTTY)
> > > >   return rc;
> > > > return pci_parent_bus_reset(dev, probe);  
> > > 
> > > 
> > > That'd be news to me, do you have a reference?  I've never seen
> > > complaints for ternaries previously.  Thanks,  
> > 
> > The complaint is not to ternaries, but to the function call as one of
> > the parameters, that makes it harder to read.
> 
> Sorry, I don't find a function call as a parameter to a ternary to be
> extraordinary, nor do I find it to be a discouraged usage model within
> the kernel.  This seems like a pretty low bar for hard to read code.

It is up to us where this bar is set.

Thanks

Re: [PATCH] PCI: merge slot and bus reset implementations

[Krzysztof Wilczyński]

[+cc Greg for visibility]

Hello,

[...]
> > > > > The previous coding style is preferable one in the Linux kernel.
> > > > > int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > if (rc != -ENOTTY)
> > > > >   return rc;
> > > > > return pci_parent_bus_reset(dev, probe);  
> > > > 
> > > > 
> > > > That'd be news to me, do you have a reference?  I've never seen
> > > > complaints for ternaries previously.  Thanks,  
> > > 
> > > The complaint is not to ternaries, but to the function call as one of
> > > the parameters, that makes it harder to read.
> > 
> > Sorry, I don't find a function call as a parameter to a ternary to be
> > extraordinary, nor do I find it to be a discouraged usage model within
> > the kernel.  This seems like a pretty low bar for hard to read code.
> 
> It is up to us where this bar is set.

The only person who ever pulled my ear, so to speak, over using ternary
was Greg as a bad style where, especially where it does not need to be
used.

But, I digress.  I humbly think that we should move back on track and
finish review of Raphael's patch.  Would use a ternary here be
a show-stopper?

Krzysztof

Re: [PATCH] PCI: merge slot and bus reset implementations

[Leon Romanovsky]

On Wed, Apr 07, 2021 at 09:49:03AM +0200, Krzysztof Wilczyński wrote:
> [+cc Greg for visibility]
> 
> Hello,
> 
> [...]
> > > > > > The previous coding style is preferable one in the Linux kernel.
> > > > > > int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > > if (rc != -ENOTTY)
> > > > > >   return rc;
> > > > > > return pci_parent_bus_reset(dev, probe);  
> > > > > 
> > > > > 
> > > > > That'd be news to me, do you have a reference?  I've never seen
> > > > > complaints for ternaries previously.  Thanks,  
> > > > 
> > > > The complaint is not to ternaries, but to the function call as one of
> > > > the parameters, that makes it harder to read.
> > > 
> > > Sorry, I don't find a function call as a parameter to a ternary to be
> > > extraordinary, nor do I find it to be a discouraged usage model within
> > > the kernel.  This seems like a pretty low bar for hard to read code.
> > 
> > It is up to us where this bar is set.
> 
> The only person who ever pulled my ear, so to speak, over using ternary
> was Greg as a bad style where, especially where it does not need to be
> used.

Good to hear that I'm not alone.

> 
> But, I digress.  I humbly think that we should move back on track and
> finish review of Raphael's patch.  Would use a ternary here be
> a show-stopper?

Of course no, I would fix it locally when apply.

Thanks

> 
> Krzysztof

Re: [PATCH] PCI: merge slot and bus reset implementations

[ameynarkhede03]

On 21/04/07 10:23AM, Leon Romanovsky wrote:
> On Tue, Apr 06, 2021 at 08:16:26AM -0600, Alex Williamson wrote:
> > On Sun, 4 Apr 2021 11:04:32 +0300
> > Leon Romanovsky <leon@kernel.org> wrote:
> >
> > > On Thu, Apr 01, 2021 at 10:56:16AM -0600, Alex Williamson wrote:
> > > > On Thu, 1 Apr 2021 15:27:37 +0300
> > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > > On Thu, Apr 01, 2021 at 05:37:16AM +0000, Raphael Norwitz wrote:
> > > > > > Slot resets are bus resets with additional logic to prevent a device
> > > > > > from being removed during the reset. Currently slot and bus resets have
> > > > > > separate implementations in pci.c, complicating higher level logic. As
> > > > > > discussed on the mailing list, they should be combined into a generic
> > > > > > function which performs an SBR. This change adds a function,
> > > > > > pci_reset_bus_function(), which first attempts a slot reset and then
> > > > > > attempts a bus reset if -ENOTTY is returned, such that there is now a
> > > > > > single device agnostic function to perform an SBR.
> > > > > >
> > > > > > This new function is also needed to add SBR reset quirks and therefore
> > > > > > is exposed in pci.h.
> > > > > >
> > > > > > Link: https://lkml.org/lkml/2021/3/23/911
> > > > > >
> > > > > > Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> > > > > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > > > > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> > > > > > ---
> > > > > >  drivers/pci/pci.c   | 17 +++++++++--------
> > > > > >  include/linux/pci.h |  1 +
> > > > > >  2 files changed, 10 insertions(+), 8 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > > index 16a17215f633..12a91af2ade4 100644
> > > > > > --- a/drivers/pci/pci.c
> > > > > > +++ b/drivers/pci/pci.c
> > > > > > @@ -4982,6 +4982,13 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
> > > > > >  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
> > > > > >  }
> > > > > >
> > > > > > +int pci_reset_bus_function(struct pci_dev *dev, int probe)
> > > > > > +{
> > > > > > +	int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > > +
> > > > > > +	return (rc == -ENOTTY) ? pci_parent_bus_reset(dev, probe) : rc;
> > > > >
> > > > > The previous coding style is preferable one in the Linux kernel.
> > > > > int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > if (rc != -ENOTTY)
> > > > >   return rc;
> > > > > return pci_parent_bus_reset(dev, probe);
> > > >
> > > >
> > > > That'd be news to me, do you have a reference?  I've never seen
> > > > complaints for ternaries previously.  Thanks,
> > >
> > > The complaint is not to ternaries, but to the function call as one of
> > > the parameters, that makes it harder to read.
> >
> > Sorry, I don't find a function call as a parameter to a ternary to be
> > extraordinary, nor do I find it to be a discouraged usage model within
> > the kernel.  This seems like a pretty low bar for hard to read code.
>
> It is up to us where this bar is set.
>
> Thanks
On the side note there are plenty of places where this pattern is used
though
for example -
kernel/time/clockevents.c:328:
return force ? clockevents_program_min_delta(dev) : -ETIME;

kernel/trace/trace_kprobe.c:233:
return tk ? within_error_injection_list(trace_kprobe_address(tk)) :
       false;

kernel/signal.c:3104:
return oset ? put_compat_sigset(oset, &old_set, sizeof(*oset)) : 0;
etc

Thanks,
Amey

Re: [PATCH] PCI: merge slot and bus reset implementations

[Leon Romanovsky]

On Wed, Apr 07, 2021 at 01:53:56PM +0530, ameynarkhede03@gmail.com wrote:
> On 21/04/07 10:23AM, Leon Romanovsky wrote:
> > On Tue, Apr 06, 2021 at 08:16:26AM -0600, Alex Williamson wrote:
> > > On Sun, 4 Apr 2021 11:04:32 +0300
> > > Leon Romanovsky <leon@kernel.org> wrote:
> > >
> > > > On Thu, Apr 01, 2021 at 10:56:16AM -0600, Alex Williamson wrote:
> > > > > On Thu, 1 Apr 2021 15:27:37 +0300
> > > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > > >
> > > > > > On Thu, Apr 01, 2021 at 05:37:16AM +0000, Raphael Norwitz wrote:
> > > > > > > Slot resets are bus resets with additional logic to prevent a device
> > > > > > > from being removed during the reset. Currently slot and bus resets have
> > > > > > > separate implementations in pci.c, complicating higher level logic. As
> > > > > > > discussed on the mailing list, they should be combined into a generic
> > > > > > > function which performs an SBR. This change adds a function,
> > > > > > > pci_reset_bus_function(), which first attempts a slot reset and then
> > > > > > > attempts a bus reset if -ENOTTY is returned, such that there is now a
> > > > > > > single device agnostic function to perform an SBR.
> > > > > > >
> > > > > > > This new function is also needed to add SBR reset quirks and therefore
> > > > > > > is exposed in pci.h.
> > > > > > >
> > > > > > > Link: https://lkml.org/lkml/2021/3/23/911
> > > > > > >
> > > > > > > Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> > > > > > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > > > > > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> > > > > > > ---
> > > > > > >  drivers/pci/pci.c   | 17 +++++++++--------
> > > > > > >  include/linux/pci.h |  1 +
> > > > > > >  2 files changed, 10 insertions(+), 8 deletions(-)
> > > > > > >
> > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > > > index 16a17215f633..12a91af2ade4 100644
> > > > > > > --- a/drivers/pci/pci.c
> > > > > > > +++ b/drivers/pci/pci.c
> > > > > > > @@ -4982,6 +4982,13 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
> > > > > > >  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
> > > > > > >  }
> > > > > > >
> > > > > > > +int pci_reset_bus_function(struct pci_dev *dev, int probe)
> > > > > > > +{
> > > > > > > +	int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > > > +
> > > > > > > +	return (rc == -ENOTTY) ? pci_parent_bus_reset(dev, probe) : rc;
> > > > > >
> > > > > > The previous coding style is preferable one in the Linux kernel.
> > > > > > int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > > if (rc != -ENOTTY)
> > > > > >   return rc;
> > > > > > return pci_parent_bus_reset(dev, probe);
> > > > >
> > > > >
> > > > > That'd be news to me, do you have a reference?  I've never seen
> > > > > complaints for ternaries previously.  Thanks,
> > > >
> > > > The complaint is not to ternaries, but to the function call as one of
> > > > the parameters, that makes it harder to read.
> > >
> > > Sorry, I don't find a function call as a parameter to a ternary to be
> > > extraordinary, nor do I find it to be a discouraged usage model within
> > > the kernel.  This seems like a pretty low bar for hard to read code.
> >
> > It is up to us where this bar is set.
> >
> > Thanks
> On the side note there are plenty of places where this pattern is used
> though
> for example -
> kernel/time/clockevents.c:328:
> return force ? clockevents_program_min_delta(dev) : -ETIME;
> 
> kernel/trace/trace_kprobe.c:233:
> return tk ? within_error_injection_list(trace_kprobe_address(tk)) :
>        false;
> 
> kernel/signal.c:3104:
> return oset ? put_compat_sigset(oset, &old_set, sizeof(*oset)) : 0;
> etc

Did you look when they were introduced?

Thanks

> 
> Thanks,
> Amey

Re: [PATCH] PCI: merge slot and bus reset implementations

[ameynarkhede03]

On 21/04/07 03:30PM, Leon Romanovsky wrote:
> On Wed, Apr 07, 2021 at 01:53:56PM +0530, ameynarkhede03@gmail.com wrote:
> > On 21/04/07 10:23AM, Leon Romanovsky wrote:
> > > On Tue, Apr 06, 2021 at 08:16:26AM -0600, Alex Williamson wrote:
> > > > On Sun, 4 Apr 2021 11:04:32 +0300
> > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > >
> > > > > On Thu, Apr 01, 2021 at 10:56:16AM -0600, Alex Williamson wrote:
> > > > > > On Thu, 1 Apr 2021 15:27:37 +0300
> > > > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > > > >
> > > > > > > On Thu, Apr 01, 2021 at 05:37:16AM +0000, Raphael Norwitz wrote:
> > > > > > > > Slot resets are bus resets with additional logic to prevent a device
> > > > > > > > from being removed during the reset. Currently slot and bus resets have
> > > > > > > > separate implementations in pci.c, complicating higher level logic. As
> > > > > > > > discussed on the mailing list, they should be combined into a generic
> > > > > > > > function which performs an SBR. This change adds a function,
> > > > > > > > pci_reset_bus_function(), which first attempts a slot reset and then
> > > > > > > > attempts a bus reset if -ENOTTY is returned, such that there is now a
> > > > > > > > single device agnostic function to perform an SBR.
> > > > > > > >
> > > > > > > > This new function is also needed to add SBR reset quirks and therefore
> > > > > > > > is exposed in pci.h.
> > > > > > > >
> > > > > > > > Link: https://lkml.org/lkml/2021/3/23/911
> > > > > > > >
> > > > > > > > Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> > > > > > > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > > > > > > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> > > > > > > > ---
> > > > > > > >  drivers/pci/pci.c   | 17 +++++++++--------
> > > > > > > >  include/linux/pci.h |  1 +
> > > > > > > >  2 files changed, 10 insertions(+), 8 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > > > > index 16a17215f633..12a91af2ade4 100644
> > > > > > > > --- a/drivers/pci/pci.c
> > > > > > > > +++ b/drivers/pci/pci.c
> > > > > > > > @@ -4982,6 +4982,13 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
> > > > > > > >  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
> > > > > > > >  }
> > > > > > > >
> > > > > > > > +int pci_reset_bus_function(struct pci_dev *dev, int probe)
> > > > > > > > +{
> > > > > > > > +	int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > > > > +
> > > > > > > > +	return (rc == -ENOTTY) ? pci_parent_bus_reset(dev, probe) : rc;
> > > > > > >
> > > > > > > The previous coding style is preferable one in the Linux kernel.
> > > > > > > int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > > > if (rc != -ENOTTY)
> > > > > > >   return rc;
> > > > > > > return pci_parent_bus_reset(dev, probe);
> > > > > >
> > > > > >
> > > > > > That'd be news to me, do you have a reference?  I've never seen
> > > > > > complaints for ternaries previously.  Thanks,
> > > > >
> > > > > The complaint is not to ternaries, but to the function call as one of
> > > > > the parameters, that makes it harder to read.
> > > >
> > > > Sorry, I don't find a function call as a parameter to a ternary to be
> > > > extraordinary, nor do I find it to be a discouraged usage model within
> > > > the kernel.  This seems like a pretty low bar for hard to read code.
> > >
> > > It is up to us where this bar is set.
> > >
> > > Thanks
> > On the side note there are plenty of places where this pattern is used
> > though
> > for example -
> > kernel/time/clockevents.c:328:
> > return force ? clockevents_program_min_delta(dev) : -ETIME;
> >
> > kernel/trace/trace_kprobe.c:233:
> > return tk ? within_error_injection_list(trace_kprobe_address(tk)) :
> >        false;
> >
> > kernel/signal.c:3104:
> > return oset ? put_compat_sigset(oset, &old_set, sizeof(*oset)) : 0;
> > etc
>
> Did you look when they were introduced?
>
> Thanks
>
that code trace_kprobe in 2 years old.
If you want more recent example checkout
drivers/pci/controller/pcie-brcmstb.c:1112,1117:
return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
which was introduced 7 months ago.
There are lot of examples in pci.c also.

Thanks,
Amey

Re: [PATCH] PCI: merge slot and bus reset implementations

[Leon Romanovsky]

On Wed, Apr 07, 2021 at 06:36:01PM +0530, ameynarkhede03@gmail.com wrote:
> On 21/04/07 03:30PM, Leon Romanovsky wrote:
> > On Wed, Apr 07, 2021 at 01:53:56PM +0530, ameynarkhede03@gmail.com wrote:
> > > On 21/04/07 10:23AM, Leon Romanovsky wrote:
> > > > On Tue, Apr 06, 2021 at 08:16:26AM -0600, Alex Williamson wrote:
> > > > > On Sun, 4 Apr 2021 11:04:32 +0300
> > > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > > >
> > > > > > On Thu, Apr 01, 2021 at 10:56:16AM -0600, Alex Williamson wrote:
> > > > > > > On Thu, 1 Apr 2021 15:27:37 +0300
> > > > > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > > > > >
> > > > > > > > On Thu, Apr 01, 2021 at 05:37:16AM +0000, Raphael Norwitz wrote:
> > > > > > > > > Slot resets are bus resets with additional logic to prevent a device
> > > > > > > > > from being removed during the reset. Currently slot and bus resets have
> > > > > > > > > separate implementations in pci.c, complicating higher level logic. As
> > > > > > > > > discussed on the mailing list, they should be combined into a generic
> > > > > > > > > function which performs an SBR. This change adds a function,
> > > > > > > > > pci_reset_bus_function(), which first attempts a slot reset and then
> > > > > > > > > attempts a bus reset if -ENOTTY is returned, such that there is now a
> > > > > > > > > single device agnostic function to perform an SBR.
> > > > > > > > >
> > > > > > > > > This new function is also needed to add SBR reset quirks and therefore
> > > > > > > > > is exposed in pci.h.
> > > > > > > > >
> > > > > > > > > Link: https://lkml.org/lkml/2021/3/23/911
> > > > > > > > >
> > > > > > > > > Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> > > > > > > > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > > > > > > > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> > > > > > > > > ---
> > > > > > > > >  drivers/pci/pci.c   | 17 +++++++++--------
> > > > > > > > >  include/linux/pci.h |  1 +
> > > > > > > > >  2 files changed, 10 insertions(+), 8 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > > > > > index 16a17215f633..12a91af2ade4 100644
> > > > > > > > > --- a/drivers/pci/pci.c
> > > > > > > > > +++ b/drivers/pci/pci.c
> > > > > > > > > @@ -4982,6 +4982,13 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
> > > > > > > > >  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
> > > > > > > > >  }
> > > > > > > > >
> > > > > > > > > +int pci_reset_bus_function(struct pci_dev *dev, int probe)
> > > > > > > > > +{
> > > > > > > > > +	int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > > > > > +
> > > > > > > > > +	return (rc == -ENOTTY) ? pci_parent_bus_reset(dev, probe) : rc;
> > > > > > > >
> > > > > > > > The previous coding style is preferable one in the Linux kernel.
> > > > > > > > int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > > > > if (rc != -ENOTTY)
> > > > > > > >   return rc;
> > > > > > > > return pci_parent_bus_reset(dev, probe);
> > > > > > >
> > > > > > >
> > > > > > > That'd be news to me, do you have a reference?  I've never seen
> > > > > > > complaints for ternaries previously.  Thanks,
> > > > > >
> > > > > > The complaint is not to ternaries, but to the function call as one of
> > > > > > the parameters, that makes it harder to read.
> > > > >
> > > > > Sorry, I don't find a function call as a parameter to a ternary to be
> > > > > extraordinary, nor do I find it to be a discouraged usage model within
> > > > > the kernel.  This seems like a pretty low bar for hard to read code.
> > > >
> > > > It is up to us where this bar is set.
> > > >
> > > > Thanks
> > > On the side note there are plenty of places where this pattern is used
> > > though
> > > for example -
> > > kernel/time/clockevents.c:328:
> > > return force ? clockevents_program_min_delta(dev) : -ETIME;
> > >
> > > kernel/trace/trace_kprobe.c:233:
> > > return tk ? within_error_injection_list(trace_kprobe_address(tk)) :
> > >        false;
> > >
> > > kernel/signal.c:3104:
> > > return oset ? put_compat_sigset(oset, &old_set, sizeof(*oset)) : 0;
> > > etc
> >
> > Did you look when they were introduced?
> >
> > Thanks
> >
> that code trace_kprobe in 2 years old.
> If you want more recent example checkout
> drivers/pci/controller/pcie-brcmstb.c:1112,1117:
> return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
> which was introduced 7 months ago.
> There are lot of examples in pci.c also.

Yeah, I know, copy-paste is a powerful tool.

Can we please progress with this patch instead of doing
archaeological research?

Thanks

> 
> Thanks,
> Amey

Re: [PATCH] PCI: merge slot and bus reset implementations

[ameynarkhede03]

On 21/04/07 04:37PM, Leon Romanovsky wrote:
> On Wed, Apr 07, 2021 at 06:36:01PM +0530, ameynarkhede03@gmail.com wrote:
> > On 21/04/07 03:30PM, Leon Romanovsky wrote:
> > > On Wed, Apr 07, 2021 at 01:53:56PM +0530, ameynarkhede03@gmail.com wrote:
> > > > On 21/04/07 10:23AM, Leon Romanovsky wrote:
> > > > > On Tue, Apr 06, 2021 at 08:16:26AM -0600, Alex Williamson wrote:
> > > > > > On Sun, 4 Apr 2021 11:04:32 +0300
> > > > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > > > >
> > > > > > > On Thu, Apr 01, 2021 at 10:56:16AM -0600, Alex Williamson wrote:
> > > > > > > > On Thu, 1 Apr 2021 15:27:37 +0300
> > > > > > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > > > > > >
> > > > > > > > > On Thu, Apr 01, 2021 at 05:37:16AM +0000, Raphael Norwitz wrote:
> > > > > > > > > > Slot resets are bus resets with additional logic to prevent a device
> > > > > > > > > > from being removed during the reset. Currently slot and bus resets have
> > > > > > > > > > separate implementations in pci.c, complicating higher level logic. As
> > > > > > > > > > discussed on the mailing list, they should be combined into a generic
> > > > > > > > > > function which performs an SBR. This change adds a function,
> > > > > > > > > > pci_reset_bus_function(), which first attempts a slot reset and then
> > > > > > > > > > attempts a bus reset if -ENOTTY is returned, such that there is now a
> > > > > > > > > > single device agnostic function to perform an SBR.
> > > > > > > > > >
> > > > > > > > > > This new function is also needed to add SBR reset quirks and therefore
> > > > > > > > > > is exposed in pci.h.
> > > > > > > > > >
> > > > > > > > > > Link: https://lkml.org/lkml/2021/3/23/911
> > > > > > > > > >
> > > > > > > > > > Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> > > > > > > > > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > > > > > > > > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/pci/pci.c   | 17 +++++++++--------
> > > > > > > > > >  include/linux/pci.h |  1 +
> > > > > > > > > >  2 files changed, 10 insertions(+), 8 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > > > > > > index 16a17215f633..12a91af2ade4 100644
> > > > > > > > > > --- a/drivers/pci/pci.c
> > > > > > > > > > +++ b/drivers/pci/pci.c
> > > > > > > > > > @@ -4982,6 +4982,13 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
> > > > > > > > > >  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > +int pci_reset_bus_function(struct pci_dev *dev, int probe)
> > > > > > > > > > +{
> > > > > > > > > > +	int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > > > > > > +
> > > > > > > > > > +	return (rc == -ENOTTY) ? pci_parent_bus_reset(dev, probe) : rc;
> > > > > > > > >
> > > > > > > > > The previous coding style is preferable one in the Linux kernel.
> > > > > > > > > int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > > > > > if (rc != -ENOTTY)
> > > > > > > > >   return rc;
> > > > > > > > > return pci_parent_bus_reset(dev, probe);
> > > > > > > >
> > > > > > > >
> > > > > > > > That'd be news to me, do you have a reference?  I've never seen
> > > > > > > > complaints for ternaries previously.  Thanks,
> > > > > > >
> > > > > > > The complaint is not to ternaries, but to the function call as one of
> > > > > > > the parameters, that makes it harder to read.
> > > > > >
> > > > > > Sorry, I don't find a function call as a parameter to a ternary to be
> > > > > > extraordinary, nor do I find it to be a discouraged usage model within
> > > > > > the kernel.  This seems like a pretty low bar for hard to read code.
> > > > >
> > > > > It is up to us where this bar is set.
> > > > >
> > > > > Thanks
> > > > On the side note there are plenty of places where this pattern is used
> > > > though
> > > > for example -
> > > > kernel/time/clockevents.c:328:
> > > > return force ? clockevents_program_min_delta(dev) : -ETIME;
> > > >
> > > > kernel/trace/trace_kprobe.c:233:
> > > > return tk ? within_error_injection_list(trace_kprobe_address(tk)) :
> > > >        false;
> > > >
> > > > kernel/signal.c:3104:
> > > > return oset ? put_compat_sigset(oset, &old_set, sizeof(*oset)) : 0;
> > > > etc
> > >
> > > Did you look when they were introduced?
> > >
> > > Thanks
> > >
> > that code trace_kprobe in 2 years old.
> > If you want more recent example checkout
> > drivers/pci/controller/pcie-brcmstb.c:1112,1117:
> > return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
> > which was introduced 7 months ago.
> > There are lot of examples in pci.c also.
>
> Yeah, I know, copy-paste is a powerful tool.
>
> Can we please progress with this patch instead of doing
> archaeological research?
>
> Thanks
>
Sorry I didn't understand what you said.


Thanks,
Amey

Re: [PATCH] PCI: merge slot and bus reset implementations

[Raphael Norwitz]

On Wed, Apr 07, 2021 at 04:37:23PM +0300, Leon Romanovsky wrote:
> On Wed, Apr 07, 2021 at 06:36:01PM +0530, ameynarkhede03@gmail.com wrote:
> > On 21/04/07 03:30PM, Leon Romanovsky wrote:
> > > On Wed, Apr 07, 2021 at 01:53:56PM +0530, ameynarkhede03@gmail.com wrote:
> > > > On 21/04/07 10:23AM, Leon Romanovsky wrote:
> > > > > On Tue, Apr 06, 2021 at 08:16:26AM -0600, Alex Williamson wrote:
> > > > > > On Sun, 4 Apr 2021 11:04:32 +0300
> > > > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > > > >
> > > > > > > On Thu, Apr 01, 2021 at 10:56:16AM -0600, Alex Williamson wrote:
> > > > > > > > On Thu, 1 Apr 2021 15:27:37 +0300
> > > > > > > > Leon Romanovsky <leon@kernel.org> wrote:
> > > > > > > >
> > > > > > > > > On Thu, Apr 01, 2021 at 05:37:16AM +0000, Raphael Norwitz wrote:
> > > > > > > > > > Slot resets are bus resets with additional logic to prevent a device
> > > > > > > > > > from being removed during the reset. Currently slot and bus resets have
> > > > > > > > > > separate implementations in pci.c, complicating higher level logic. As
> > > > > > > > > > discussed on the mailing list, they should be combined into a generic
> > > > > > > > > > function which performs an SBR. This change adds a function,
> > > > > > > > > > pci_reset_bus_function(), which first attempts a slot reset and then
> > > > > > > > > > attempts a bus reset if -ENOTTY is returned, such that there is now a
> > > > > > > > > > single device agnostic function to perform an SBR.
> > > > > > > > > >
> > > > > > > > > > This new function is also needed to add SBR reset quirks and therefore
> > > > > > > > > > is exposed in pci.h.
> > > > > > > > > >
> > > > > > > > > > Link: https://urldefense.proofpoint.com/v2/url?u=https-3A__lkml.org_lkml_2021_3_23_911&d=DwIBAg&c=s883GpUCOChKOHiocYtGcg&r=In4gmR1pGzKB8G5p6LUrWqkSMec2L5EtXZow_FZNJZk&m=dn12ruIb9lwgcFMNKBZzri1m3zoTBFlkHnrF48PChs4&s=iEm1FGjLlWUpKJQYMwCHc1crraEzAgN10pCzyEzbrWI&e= 
> > > > > > > > > >
> > > > > > > > > > Suggested-by: Alex Williamson <alex.williamson@redhat.com>
> > > > > > > > > > Signed-off-by: Amey Narkhede <ameynarkhede03@gmail.com>
> > > > > > > > > > Signed-off-by: Raphael Norwitz <raphael.norwitz@nutanix.com>
> > > > > > > > > > ---
> > > > > > > > > >  drivers/pci/pci.c   | 17 +++++++++--------
> > > > > > > > > >  include/linux/pci.h |  1 +
> > > > > > > > > >  2 files changed, 10 insertions(+), 8 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > > > > > > > > > index 16a17215f633..12a91af2ade4 100644
> > > > > > > > > > --- a/drivers/pci/pci.c
> > > > > > > > > > +++ b/drivers/pci/pci.c
> > > > > > > > > > @@ -4982,6 +4982,13 @@ static int pci_dev_reset_slot_function(struct pci_dev *dev, int probe)
> > > > > > > > > >  	return pci_reset_hotplug_slot(dev->slot->hotplug, probe);
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > +int pci_reset_bus_function(struct pci_dev *dev, int probe)
> > > > > > > > > > +{
> > > > > > > > > > +	int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > > > > > > +
> > > > > > > > > > +	return (rc == -ENOTTY) ? pci_parent_bus_reset(dev, probe) : rc;
> > > > > > > > >
> > > > > > > > > The previous coding style is preferable one in the Linux kernel.
> > > > > > > > > int rc = pci_dev_reset_slot_function(dev, probe);
> > > > > > > > > if (rc != -ENOTTY)
> > > > > > > > >   return rc;
> > > > > > > > > return pci_parent_bus_reset(dev, probe);
> > > > > > > >
> > > > > > > >
> > > > > > > > That'd be news to me, do you have a reference?  I've never seen
> > > > > > > > complaints for ternaries previously.  Thanks,
> > > > > > >
> > > > > > > The complaint is not to ternaries, but to the function call as one of
> > > > > > > the parameters, that makes it harder to read.
> > > > > >
> > > > > > Sorry, I don't find a function call as a parameter to a ternary to be
> > > > > > extraordinary, nor do I find it to be a discouraged usage model within
> > > > > > the kernel.  This seems like a pretty low bar for hard to read code.
> > > > >
> > > > > It is up to us where this bar is set.
> > > > >
> > > > > Thanks
> > > > On the side note there are plenty of places where this pattern is used
> > > > though
> > > > for example -
> > > > kernel/time/clockevents.c:328:
> > > > return force ? clockevents_program_min_delta(dev) : -ETIME;
> > > >
> > > > kernel/trace/trace_kprobe.c:233:
> > > > return tk ? within_error_injection_list(trace_kprobe_address(tk)) :
> > > >        false;
> > > >
> > > > kernel/signal.c:3104:
> > > > return oset ? put_compat_sigset(oset, &old_set, sizeof(*oset)) : 0;
> > > > etc
> > >
> > > Did you look when they were introduced?
> > >
> > > Thanks
> > >
> > that code trace_kprobe in 2 years old.
> > If you want more recent example checkout
> > drivers/pci/controller/pcie-brcmstb.c:1112,1117:
> > return pcie->rescal ? brcm_phy_cntl(pcie, 1) : 0;
> > which was introduced 7 months ago.
> > There are lot of examples in pci.c also.
> 
> Yeah, I know, copy-paste is a powerful tool.
> 
> Can we please progress with this patch instead of doing
> archaeological research?
> 
> Thanks
>

I don't have a strong view on the style guidelines being discussed here.

I just sent a V2 replacing the ternary function parameter with your
suggestion.

> > 
> > Thanks,
> > Amey