| Message ID | 20210312173452.3855-1-ameynarkhede03@gmail.com |
|---|---|
| Headers | show |
| Series | Expose and manage PCI device reset | expand |
On 21/03/12 11:20AM, Alex Williamson wrote: > On Fri, 12 Mar 2021 23:04:48 +0530 > ameynarkhede03@gmail.com wrote: > > > From: Amey Narkhede <ameynarkhede03@gmail.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@redhat.com> > > Reviewed-by: Alex Williamson <alex.williamson@redhat.com> > > Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.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@nutanix.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? 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 > > >
Hi Amey, Thank you for sending the series over! [...] > > 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@nutanix.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? [...] I am not sure about the messages outside of the mailing list between you, Alex and Raphael, as normally conversation and any reviews would happen here (on the mailing list, that is), but as long as everyone involved is on the same page, then every should be fine. In terms of how to format the patch, have a look at the following, especially before you send another version, as there are some good tips and recommendations there (including how to order things): https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com/ Krzysztof
On 21/03/12 07:58PM, Krzysztof Wilczyński wrote: > Hi Amey, > > Thank you for sending the series over! > > [...] > > > 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@nutanix.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? > [...] > > I am not sure about the messages outside of the mailing list between > you, Alex and Raphael, as normally conversation and any reviews would > happen here (on the mailing list, that is), but as long as everyone > involved is on the same page, then every should be fine. > > In terms of how to format the patch, have a look at the following, > especially before you send another version, as there are some good tips > and recommendations there (including how to order things): > > https://lore.kernel.org/linux-pci/20171026223701.GA25649@bhelgaas-glaptop.roam.corp.google.com/ > > Krzysztof Basically whole thing boils down to I'm not good at handling terminal email clients. I'll surely keep those points mentioned by Bjorn in my mind. Thanks, Amey
Hi Amey, [...] > Basically whole thing boils down to I'm not good at handling terminal > email clients. I'll surely keep those points mentioned by Bjorn > in my mind. [...] No worries. Thunderbird works fine with Google Mail and can send plain text e-mails too, if you get tired of Mutt etc. By the way, don't immediately send v2 quite yet. Allow people some time to review first version. Well, unless you deem that you need to do it, that is. Krzysztof
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@gmail.com wrote: > > > > > From: Amey Narkhede <ameynarkhede03@gmail.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@redhat.com> > > > Reviewed-by: Alex Williamson <alex.williamson@redhat.com> > > > Reviewed-by: Raphael Norwitz <raphael.norwitz@nutanix.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@nutanix.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@nutanix.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 > > > > >
On Fri, Mar 12, 2021 at 11:04:48PM +0530, ameynarkhede03@gmail.com wrote: > From: Amey Narkhede <ameynarkhede03@gmail.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. Sorry, are we talking about specific devices/flows/applications that must have this functionality or about theoretical use case? Thanks