Re: [PATCH] PCI: merge slot and bus reset implementations
On Wed, Apr 07, 2021 at 04:37:23PM +0300, Leon Romanovsky wrote: > On Wed, Apr 07, 2021 at 06:36:01PM +0530, ameynarkhed...@gmail.com wrote: > > On 21/04/07 03:30PM, Leon Romanovsky wrote: > > > On Wed, Apr 07, 2021 at 01:53:56PM +0530, ameynarkhed...@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 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 wrote: > > > > > > > > > > > > > > > > > On Thu, Apr 01, 2021 at 05:37:16AM +, 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=DwIBAg=s883GpUCOChKOHiocYtGcg=In4gmR1pGzKB8G5p6LUrWqkSMec2L5EtXZow_FZNJZk=dn12ruIb9lwgcFMNKBZzri1m3zoTBFlkHnrF48PChs4=iEm1FGjLlWUpKJQYMwCHc1crraEzAgN10pCzyEzbrWI= > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Suggested-by: Alex Williamson > > > > > > > > > > Signed-off-by: Amey Narkhede > > > > > > > > > > Signed-off-by: Raphael Norwitz > > > > > > > > > > --- > > > > > > > > > > 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, _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
Re: [PATCH] PCI: merge slot and bus reset implementations
On 21/04/07 04:37PM, Leon Romanovsky wrote: > On Wed, Apr 07, 2021 at 06:36:01PM +0530, ameynarkhed...@gmail.com wrote: > > On 21/04/07 03:30PM, Leon Romanovsky wrote: > > > On Wed, Apr 07, 2021 at 01:53:56PM +0530, ameynarkhed...@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 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 wrote: > > > > > > > > > > > > > > > > > On Thu, Apr 01, 2021 at 05:37:16AM +, 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 > > > > > > > > > > Signed-off-by: Amey Narkhede > > > > > > > > > > Signed-off-by: Raphael Norwitz > > > > > > > > > > --- > > > > > > > > > > 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, _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
Re: [PATCH] PCI: merge slot and bus reset implementations
On Wed, Apr 07, 2021 at 06:36:01PM +0530, ameynarkhed...@gmail.com wrote: > On 21/04/07 03:30PM, Leon Romanovsky wrote: > > On Wed, Apr 07, 2021 at 01:53:56PM +0530, ameynarkhed...@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 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 wrote: > > > > > > > > > > > > > > > On Thu, Apr 01, 2021 at 05:37:16AM +, 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 > > > > > > > > > Signed-off-by: Amey Narkhede > > > > > > > > > Signed-off-by: Raphael Norwitz > > > > > > > > > --- > > > > > > > > > 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, _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
On 21/04/07 03:30PM, Leon Romanovsky wrote: > On Wed, Apr 07, 2021 at 01:53:56PM +0530, ameynarkhed...@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 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 wrote: > > > > > > > > > > > > > On Thu, Apr 01, 2021 at 05:37:16AM +, 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 > > > > > > > > Signed-off-by: Amey Narkhede > > > > > > > > Signed-off-by: Raphael Norwitz > > > > > > > > --- > > > > > > > > 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, _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
On Wed, Apr 07, 2021 at 01:53:56PM +0530, ameynarkhed...@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 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 wrote: > > > > > > > > > > > On Thu, Apr 01, 2021 at 05:37:16AM +, 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 > > > > > > > Signed-off-by: Amey Narkhede > > > > > > > Signed-off-by: Raphael Norwitz > > > > > > > --- > > > > > > > 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, _set, sizeof(*oset)) : 0; > etc Did you look when they were introduced? Thanks > > Thanks, > Amey
Re: [PATCH] PCI: merge slot and bus reset implementations
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 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 wrote: > > > > > > > > > On Thu, Apr 01, 2021 at 05:37:16AM +, 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 > > > > > > Signed-off-by: Amey Narkhede > > > > > > Signed-off-by: Raphael Norwitz > > > > > > --- > > > > > > 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, _set, sizeof(*oset)) : 0; etc Thanks, Amey
Re: [PATCH] PCI: merge slot and bus reset implementations
On Tue, Apr 06, 2021 at 08:16:26AM -0600, Alex Williamson wrote: > On Sun, 4 Apr 2021 11:04:32 +0300 > Leon Romanovsky 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 wrote: > > > > > > > On Thu, Apr 01, 2021 at 05:37:16AM +, 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 > > > > > Signed-off-by: Amey Narkhede > > > > > Signed-off-by: Raphael Norwitz > > > > > --- > > > > > 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
On Sun, 4 Apr 2021 11:04:32 +0300 Leon Romanovsky 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 wrote: > > > > > On Thu, Apr 01, 2021 at 05:37:16AM +, 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 > > > > Signed-off-by: Amey Narkhede > > > > Signed-off-by: Raphael Norwitz > > > > --- > > > > 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
On Thu, Apr 01, 2021 at 10:56:16AM -0600, Alex Williamson wrote: > On Thu, 1 Apr 2021 15:27:37 +0300 > Leon Romanovsky wrote: > > > On Thu, Apr 01, 2021 at 05:37:16AM +, 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 > > > Signed-off-by: Amey Narkhede > > > Signed-off-by: Raphael Norwitz > > > --- > > > 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
On Thu, Apr 01, 2021 at 05:37:16AM +, 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 > Signed-off-by: Amey Narkhede > Signed-off-by: Raphael Norwitz > --- > 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
On Thu, 1 Apr 2021 15:27:37 +0300 Leon Romanovsky wrote: > On Thu, Apr 01, 2021 at 05:37:16AM +, 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 > > Signed-off-by: Amey Narkhede > > Signed-off-by: Raphael Norwitz > > --- > > 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
[PATCH] PCI: merge slot and bus reset implementations
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 Signed-off-by: Amey Narkhede Signed-off-by: Raphael Norwitz --- 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