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 <[email protected]>
Signed-off-by: Amey Narkhede <[email protected]>
Signed-off-by: Raphael Norwitz <[email protected]>
---
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
On Thu, 1 Apr 2021 15:27:37 +0300
Leon Romanovsky <[email protected]> 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 <[email protected]>
> > Signed-off-by: Amey Narkhede <[email protected]>
> > Signed-off-by: Raphael Norwitz <[email protected]>
> > ---
> > 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
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 <[email protected]>
> Signed-off-by: Amey Narkhede <[email protected]>
> Signed-off-by: Raphael Norwitz <[email protected]>
> ---
> 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
On Thu, Apr 01, 2021 at 10:56:16AM -0600, Alex Williamson wrote:
> On Thu, 1 Apr 2021 15:27:37 +0300
> Leon Romanovsky <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Amey Narkhede <[email protected]>
> > > Signed-off-by: Raphael Norwitz <[email protected]>
> > > ---
> > > 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
>
On Sun, 4 Apr 2021 11:04:32 +0300
Leon Romanovsky <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > Signed-off-by: Amey Narkhede <[email protected]>
> > > > Signed-off-by: Raphael Norwitz <[email protected]>
> > > > ---
> > > > 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.
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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > Signed-off-by: Amey Narkhede <[email protected]>
> > > > > > Signed-off-by: Raphael Norwitz <[email protected]>
> > > > > > ---
> > > > > > 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
On Wed, Apr 07, 2021 at 01:53:56PM +0530, [email protected] 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > Signed-off-by: Amey Narkhede <[email protected]>
> > > > > > > Signed-off-by: Raphael Norwitz <[email protected]>
> > > > > > > ---
> > > > > > > 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
On 21/04/07 04:37PM, Leon Romanovsky wrote:
> On Wed, Apr 07, 2021 at 06:36:01PM +0530, [email protected] wrote:
> > On 21/04/07 03:30PM, Leon Romanovsky wrote:
> > > On Wed, Apr 07, 2021 at 01:53:56PM +0530, [email protected] 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > > > > Signed-off-by: Amey Narkhede <[email protected]>
> > > > > > > > > > Signed-off-by: Raphael Norwitz <[email protected]>
> > > > > > > > > > ---
> > > > > > > > > > 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
On 21/04/07 03:30PM, Leon Romanovsky wrote:
> On Wed, Apr 07, 2021 at 01:53:56PM +0530, [email protected] 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > > Signed-off-by: Amey Narkhede <[email protected]>
> > > > > > > > Signed-off-by: Raphael Norwitz <[email protected]>
> > > > > > > > ---
> > > > > > > > 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
On Wed, Apr 07, 2021 at 06:36:01PM +0530, [email protected] wrote:
> On 21/04/07 03:30PM, Leon Romanovsky wrote:
> > On Wed, Apr 07, 2021 at 01:53:56PM +0530, [email protected] 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > > > Signed-off-by: Amey Narkhede <[email protected]>
> > > > > > > > > Signed-off-by: Raphael Norwitz <[email protected]>
> > > > > > > > > ---
> > > > > > > > > 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
On Tue, Apr 06, 2021 at 08:16:26AM -0600, Alex Williamson wrote:
> On Sun, 4 Apr 2021 11:04:32 +0300
> Leon Romanovsky <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > Signed-off-by: Amey Narkhede <[email protected]>
> > > > > Signed-off-by: Raphael Norwitz <[email protected]>
> > > > > ---
> > > > > 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 Wed, Apr 07, 2021 at 04:37:23PM +0300, Leon Romanovsky wrote:
> On Wed, Apr 07, 2021 at 06:36:01PM +0530, [email protected] wrote:
> > On 21/04/07 03:30PM, Leon Romanovsky wrote:
> > > On Wed, Apr 07, 2021 at 01:53:56PM +0530, [email protected] 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 <[email protected]> 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 <[email protected]> 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 <[email protected]>
> > > > > > > > > > Signed-off-by: Amey Narkhede <[email protected]>
> > > > > > > > > > Signed-off-by: Raphael Norwitz <[email protected]>
> > > > > > > > > > ---
> > > > > > > > > > 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