* [PATCH] PCI: merge slot and bus reset implementations @ 2021-04-01 5:37 Raphael Norwitz 2021-04-01 12:27 ` Leon Romanovsky 0 siblings, 1 reply; 12+ messages in thread From: Raphael Norwitz @ 2021-04-01 5:37 UTC (permalink / raw) To: bhelgaas Cc: linux-pci, linux-kernel, alex.williamson, ameynarkhede03, 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 ^ permalink raw reply related [flat|nested] 12+ messages in thread
* Re: [PATCH] PCI: merge slot and bus reset implementations 2021-04-01 5:37 [PATCH] PCI: merge slot and bus reset implementations Raphael Norwitz @ 2021-04-01 12:27 ` Leon Romanovsky 2021-04-01 16:56 ` Alex Williamson 0 siblings, 1 reply; 12+ messages in thread From: Leon Romanovsky @ 2021-04-01 12:27 UTC (permalink / raw) To: Raphael Norwitz Cc: bhelgaas, linux-pci, linux-kernel, alex.williamson, ameynarkhede03 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] PCI: merge slot and bus reset implementations 2021-04-01 12:27 ` Leon Romanovsky @ 2021-04-01 16:56 ` Alex Williamson 2021-04-04 8:04 ` Leon Romanovsky 0 siblings, 1 reply; 12+ messages in thread From: Alex Williamson @ 2021-04-01 16:56 UTC (permalink / raw) To: Leon Romanovsky Cc: Raphael Norwitz, bhelgaas, linux-pci, linux-kernel, ameynarkhede03 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] PCI: merge slot and bus reset implementations 2021-04-01 16:56 ` Alex Williamson @ 2021-04-04 8:04 ` Leon Romanovsky 2021-04-06 14:16 ` Alex Williamson 0 siblings, 1 reply; 12+ messages in thread From: Leon Romanovsky @ 2021-04-04 8:04 UTC (permalink / raw) To: Alex Williamson Cc: Raphael Norwitz, bhelgaas, linux-pci, linux-kernel, ameynarkhede03 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 > ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] PCI: merge slot and bus reset implementations 2021-04-04 8:04 ` Leon Romanovsky @ 2021-04-06 14:16 ` Alex Williamson 2021-04-07 7:23 ` Leon Romanovsky 0 siblings, 1 reply; 12+ messages in thread From: Alex Williamson @ 2021-04-06 14:16 UTC (permalink / raw) To: Leon Romanovsky Cc: Raphael Norwitz, bhelgaas, linux-pci, linux-kernel, ameynarkhede03 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. ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] PCI: merge slot and bus reset implementations 2021-04-06 14:16 ` Alex Williamson @ 2021-04-07 7:23 ` Leon Romanovsky 2021-04-07 8:23 ` ameynarkhede03 0 siblings, 1 reply; 12+ messages in thread From: Leon Romanovsky @ 2021-04-07 7:23 UTC (permalink / raw) To: Alex Williamson Cc: Raphael Norwitz, bhelgaas, linux-pci, linux-kernel, ameynarkhede03 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] PCI: merge slot and bus reset implementations 2021-04-07 7:23 ` Leon Romanovsky @ 2021-04-07 8:23 ` ameynarkhede03 2021-04-07 12:30 ` Leon Romanovsky 0 siblings, 1 reply; 12+ messages in thread From: ameynarkhede03 @ 2021-04-07 8:23 UTC (permalink / raw) To: Leon Romanovsky Cc: Raphael Norwitz, Alex Williamson, bhelgaas@google.com <bhelgaas@google.com>,linux-pci@vger.kernel.org, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] PCI: merge slot and bus reset implementations 2021-04-07 8:23 ` ameynarkhede03 @ 2021-04-07 12:30 ` Leon Romanovsky 2021-04-07 13:06 ` ameynarkhede03 0 siblings, 1 reply; 12+ messages in thread From: Leon Romanovsky @ 2021-04-07 12:30 UTC (permalink / raw) To: ameynarkhede03 Cc: Raphael Norwitz, Alex Williamson, bhelgaas@google.com <bhelgaas@google.com>,linux-pci@vger.kernel.org, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] PCI: merge slot and bus reset implementations 2021-04-07 12:30 ` Leon Romanovsky @ 2021-04-07 13:06 ` ameynarkhede03 2021-04-07 13:37 ` Leon Romanovsky 0 siblings, 1 reply; 12+ messages in thread From: ameynarkhede03 @ 2021-04-07 13:06 UTC (permalink / raw) To: Leon Romanovsky Cc: Raphael Norwitz, AlexWilliamson, alex.williamson, bhelgaas, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] PCI: merge slot and bus reset implementations 2021-04-07 13:06 ` ameynarkhede03 @ 2021-04-07 13:37 ` Leon Romanovsky 2021-04-07 13:43 ` ameynarkhede03 2021-04-08 18:38 ` Raphael Norwitz 0 siblings, 2 replies; 12+ messages in thread From: Leon Romanovsky @ 2021-04-07 13:37 UTC (permalink / raw) To: ameynarkhede03 Cc: Raphael Norwitz, AlexWilliamson, alex.williamson, bhelgaas, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] PCI: merge slot and bus reset implementations 2021-04-07 13:37 ` Leon Romanovsky @ 2021-04-07 13:43 ` ameynarkhede03 2021-04-08 18:38 ` Raphael Norwitz 1 sibling, 0 replies; 12+ messages in thread From: ameynarkhede03 @ 2021-04-07 13:43 UTC (permalink / raw) To: Leon Romanovsky Cc: Raphael Norwitz, alex.williamson, bhelgaas, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [PATCH] PCI: merge slot and bus reset implementations 2021-04-07 13:37 ` Leon Romanovsky 2021-04-07 13:43 ` ameynarkhede03 @ 2021-04-08 18:38 ` Raphael Norwitz 1 sibling, 0 replies; 12+ messages in thread From: Raphael Norwitz @ 2021-04-08 18:38 UTC (permalink / raw) To: Leon Romanovsky Cc: ameynarkhede03, Raphael Norwitz, AlexWilliamson, alex.williamson, bhelgaas, linux-pci, linux-kernel 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 ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2021-04-08 18:38 UTC | newest] Thread overview: 12+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-04-01 5:37 [PATCH] PCI: merge slot and bus reset implementations Raphael Norwitz 2021-04-01 12:27 ` Leon Romanovsky 2021-04-01 16:56 ` Alex Williamson 2021-04-04 8:04 ` Leon Romanovsky 2021-04-06 14:16 ` Alex Williamson 2021-04-07 7:23 ` Leon Romanovsky 2021-04-07 8:23 ` ameynarkhede03 2021-04-07 12:30 ` Leon Romanovsky 2021-04-07 13:06 ` ameynarkhede03 2021-04-07 13:37 ` Leon Romanovsky 2021-04-07 13:43 ` ameynarkhede03 2021-04-08 18:38 ` Raphael Norwitz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).