linux-kernel - Re: [PATCH 0/4] Expose and manage PCI device reset

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20210313020221.GA14334@raphael-debian-dev>
Date:   Sat, 13 Mar 2021 02:02:27 +0000
From:   Raphael Norwitz <raphael.norwitz@...anix.com>
To:     Amey Narkhede <ameynarkhede03@...il.com>
CC:     Alex Williamson <alex.williamson@...hat.com>,
        "bhelgaas@...gle.com" <bhelgaas@...gle.com>,
        "linux-pci@...r.kernel.org" <linux-pci@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Raphael Norwitz <raphael.norwitz@...anix.com>
Subject: Re: [PATCH 0/4] Expose and manage PCI device reset

On Sat, Mar 13, 2021 at 12:10:38AM +0530, Amey Narkhede wrote:
> On 21/03/12 11:20AM, Alex Williamson wrote:
> > On Fri, 12 Mar 2021 23:04:48 +0530
> > ameynarkhede03@...il.com wrote:
> >
> > > From: Amey Narkhede <ameynarkhede03@...il.com>
> > >
> > > PCI and PCIe devices may support a number of possible reset mechanisms
> > > for example Function Level Reset (FLR) provided via Advanced Feature or
> > > PCIe capabilities, Power Management reset, bus reset, or device specific reset.
> > > Currently the PCI subsystem creates a policy prioritizing these reset methods
> > > which provides neither visibility nor control to userspace.
> > >
> > > Expose the reset methods available per device to userspace, via sysfs
> > > and allow an administrative user or device owner to have ability to
> > > manage per device reset method priorities or exclusions.
> > > This feature aims to allow greater control of a device for use cases
> > > as device assignment, where specific device or platform issues may
> > > interact poorly with a given reset method, and for which device specific
> > > quirks have not been developed.
> > >
> > > Suggested-by: Alex Williamson <alex.williamson@...hat.com>
> > > Reviewed-by: Alex Williamson <alex.williamson@...hat.com>
> > > Reviewed-by: Raphael Norwitz <raphael.norwitz@...anix.com>
> >
> > Reviews/Acks/Sign-off-by from others (aside from Tested/Reported-by)
> > really need to be explicit, IMO.  This is a common issue for new
> > developers, but it really needs to be more formal.  I wouldn't claim to
> > be able to speak for Raphael and interpret his comments so far as his
> > final seal of approval.
> >
> > Also in the patches, all Sign-offs/Reviews/Acks need to be above the
> > triple dash '---' line.  Anything between that line and the beginning
> > of the diff is discarded by tools.  People will often use that for
> > difference between version since it will be discarded on commit.
> > Likewise, the cover letter is not committed, so Review-by there are
> > generally not done.  I generally make my Sign-off last in the chain and
> > maintainers will generally add theirs after that.  This makes for a
> > chain where someone can read up from the bottom to see how this commit
> > entered the kernel.  Reviews, Acks, and whatnot will therefore usually
> > be collected above the author posting the patch.
> >
> > Since this is a v1 patch and it's likely there will be more revisions,
> > rather than send a v2 immediately with corrections, I'd probably just
> > reply to the cover letter retracting Raphael's Review-by for him to
> > send his own and noting that you'll fix the commit reviews formatting,
> > but will wait for a bit for further comments before sending a new
> > version.
> >
> > No big deal, nice work getting it sent out.  Thanks,
> >
> > Alex
> >
> Raphael sent me the email with
> Reviewed-by: Raphael Norwitz <raphael.norwitz@...anix.com> that
> is why I included it.
> So basically in v2 I should reorder tags such that Sign-off will be
> the last. Did I get that right? Or am I missing something?
>

Just to confirm, I did send

Reviewed-by: Raphael Norwitz <raphael.norwitz@...anix.com>

for the latest version and I'm happy to have it on this series.

> Thanks,
> Amey
> 
> > > Amey Narkhede (4):
> > >   PCI: Refactor pcie_flr to follow calling convention of other reset
> > >     methods
> > >   PCI: Add new bitmap for keeping track of supported reset mechanisms
> > >   PCI: Remove reset_fn field from pci_dev
> > >   PCI/sysfs: Allow userspace to query and set device reset mechanism
> > >
> > >  Documentation/ABI/testing/sysfs-bus-pci       |  15 ++
> > >  drivers/crypto/cavium/nitrox/nitrox_main.c    |   4 +-
> > >  drivers/crypto/qat/qat_common/adf_aer.c       |   2 +-
> > >  drivers/infiniband/hw/hfi1/chip.c             |   4 +-
> > >  drivers/net/ethernet/broadcom/bnxt/bnxt.c     |   2 +-
> > >  .../ethernet/cavium/liquidio/lio_vf_main.c    |   4 +-
> > >  .../ethernet/cavium/liquidio/octeon_mailbox.c |   2 +-
> > >  drivers/net/ethernet/freescale/enetc/enetc.c  |   2 +-
> > >  .../ethernet/freescale/enetc/enetc_pci_mdio.c |   2 +-
> > >  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |   4 +-
> > >  drivers/pci/pci-sysfs.c                       |  68 +++++++-
> > >  drivers/pci/pci.c                             | 160 ++++++++++--------
> > >  drivers/pci/pci.h                             |  11 +-
> > >  drivers/pci/pcie/aer.c                        |  12 +-
> > >  drivers/pci/probe.c                           |   4 +-
> > >  drivers/pci/quirks.c                          |  17 +-
> > >  include/linux/pci.h                           |  17 +-
> > >  17 files changed, 213 insertions(+), 117 deletions(-)
> > >
> > > --
> > > 2.30.2
> > >
> >