Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Raphael Norwitz <raphael.norwitz@nutanix.com>
To:     Leon Romanovsky <leon@kernel.org>
CC:     "ameynarkhede03@gmail.com" <ameynarkhede03@gmail.com>,
        Raphael Norwitz <raphael.norwitz@nutanix.com>,
        "AlexWilliamson@archlinux" <AlexWilliamson@archlinux>,
        "alex.williamson@redhat.com" <alex.williamson@redhat.com>,
        "bhelgaas@google.com" <bhelgaas@google.com>,
        "linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH] PCI: merge slot and bus reset implementations
Thread-Topic: [PATCH] PCI: merge slot and bus reset implementations
Thread-Index: AQHXJrkTV5QyceIJbEKfiTx8AA1Ka6qfl1qAgABLDwCABCJuAIADjJIAgAEfDICAABDMAIAARPkAgAAJ14CAAAjEgIAB5myA
Date:   Thu, 8 Apr 2021 18:38:30 +0000
Message-ID: <20210408183821.GA12374@raphael-debian-dev>
References: <20210401053656.16065-1-raphael.norwitz@nutanix.com>
 <YGW8Oe9jn+n9sVsw@unreal> <20210401105616.71156d08@omen>
 <YGlzEA5HL6ZvNsB8@unreal> <20210406081626.31f19c0f@x1.home.shazbot.org>
 <YG1eBUY0vCTV+Za/@unreal> <20210407082356.53subv4np2fx777x@archlinux>
 <YG2l+AbQW1N0bbQ9@unreal> <20210407130601.aleyww5d5mttitry@archlinux>
 <YG21k6/4QQNrw41S@unreal>
In-Reply-To: <YG21k6/4QQNrw41S@unreal>
Accept-Language: en-US
Content-Language: en-US
user-agent: Mutt/1.10.1 (2018-07-13)
Content-Type: text/plain; charset="us-ascii"
Content-ID: <8F8269E7421E524FBFDFD50C75E76F5D@namprd02.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 133f8fe5-80c2-453a-ef4f-08d8fabd8221
X-MS-Exchange-CrossTenant-originalarrivaltime: 08 Apr 2021 18:38:31.0281
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: bb047546-786f-4de1-bd75-24e5b6f79043
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: OIdW+kM2QkozwOAwXQnn6pHXldfwAekS2iaS2PK4ZdnX1y7Ye3p9wXPo9lyDwHnQ3UAk3it9/BQcPMKbnLyAXDm5vrQ3pP/3CwyaWfJOg5k=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN6PR02MB5280
X-Proofpoint-GUID: DvQoNGi09CTo8fYKRqs-i-j7HLEsIRRI
X-Proofpoint-ORIG-GUID: DvQoNGi09CTo8fYKRqs-i-j7HLEsIRRI
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:6.0.391,18.0.761
 definitions=2021-04-08_04:2021-04-08,2021-04-08 signatures=0
X-Proofpoint-Spam-Reason: safe
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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 wr=
ote:
> > > > 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 wro=
te:
> > > > > > > > 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 pre=
vent 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 func=
tion,
> > > > > > > > > > pci_reset_bus_function(), which first attempts a slot r=
eset 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 quirk=
s and therefore
> > > > > > > > > > is exposed in pci.h.
> > > > > > > > > >
> > > > > > > > > > Link: https://urldefense.proofpoint.com/v2/url?u=3Dhttp=
s-3A__lkml.org_lkml_2021_3_23_911&d=3DDwIBAg&c=3Ds883GpUCOChKOHiocYtGcg&r=
=3DIn4gmR1pGzKB8G5p6LUrWqkSMec2L5EtXZow_FZNJZk&m=3Ddn12ruIb9lwgcFMNKBZzri1m=
3zoTBFlkHnrF48PChs4&s=3DiEm1FGjLlWUpKJQYMwCHc1crraEzAgN10pCzyEzbrWI&e=3D=20
> > > > > > > > > >
> > > > > > > > > > Suggested-by: Alex Williamson <alex.williamson@redhat.c=
om>
> > > > > > > > > > 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_fu=
nction(struct pci_dev *dev, int probe)
> > > > > > > > > >  	return pci_reset_hotplug_slot(dev->slot->hotplug, pro=
be);
> > > > > > > > > >  }
> > > > > > > > > >
> > > > > > > > > > +int pci_reset_bus_function(struct pci_dev *dev, int pr=
obe)
> > > > > > > > > > +{
> > > > > > > > > > +	int rc =3D pci_dev_reset_slot_function(dev, probe);
> > > > > > > > > > +
> > > > > > > > > > +	return (rc =3D=3D -ENOTTY) ? pci_parent_bus_reset(dev=
, probe) : rc;
> > > > > > > > >
> > > > > > > > > The previous coding style is preferable one in the Linux =
kernel.
> > > > > > > > > int rc =3D pci_dev_reset_slot_function(dev, probe);
> > > > > > > > > if (rc !=3D -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 a=
s 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 u=
sed
> > > > 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.
>=20
> Yeah, I know, copy-paste is a powerful tool.
>=20
> Can we please progress with this patch instead of doing
> archaeological research?
>=20
> 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.

> >=20
> > Thanks,
> > Amey=