| Message ID | 20210506153153.30454-6-pali@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: aardvark: Various driver fixes | expand |
On Thu, May 06, 2021 at 05:31:16PM +0200, Pali Rohár wrote: > This is 16-bit register at offset 0x1E. Rename current 'rsvd' struct member > to 'rootcap'. "The 16-bit Root Capabilities register is at offset 0x1e in the PCIe Capability." Please make the commit log complete in itself. In some contexts, the subject line is not visible at the same time. It's fine to repeat the subject in the commit log. > Signed-off-by: Pali Rohár <pali@kernel.org> > Reviewed-by: Marek Behún <kabel@kernel.org> > Fixes: 23a5fba4d941 ("PCI: Introduce PCI bridge emulated config space common logic") > Cc: stable@vger.kernel.org # e0d9d30b7354 ("PCI: pci-bridge-emul: Fix big-endian support") I'm not sure how people would deal with *two* SHA1s. This patch adds functionality, so it's not really fixing a bug in 23a5fba4d941. I see that e0d9d30b7354 came along later and did "s/u16 rsvd/__le16 rsvd/". But it seems like a lot to expect for distros and stable kernel maintainers to interpret this. Personally I think I would omit both Fixes: and the stable tag since these two patches (05 and 06) are just adding functionality. > --- > drivers/pci/pci-bridge-emul.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/pci-bridge-emul.h b/drivers/pci/pci-bridge-emul.h > index b31883022a8e..49bbd37ee318 100644 > --- a/drivers/pci/pci-bridge-emul.h > +++ b/drivers/pci/pci-bridge-emul.h > @@ -54,7 +54,7 @@ struct pci_bridge_emul_pcie_conf { > __le16 slotctl; > __le16 slotsta; > __le16 rootctl; > - __le16 rsvd; > + __le16 rootcap; > __le32 rootsta; > __le32 devcap2; > __le16 devctl2; > -- > 2.20.1 >
On Thursday 06 May 2021 18:10:09 Bjorn Helgaas wrote: > On Thu, May 06, 2021 at 05:31:16PM +0200, Pali Rohár wrote: > > This is 16-bit register at offset 0x1E. Rename current 'rsvd' struct member > > to 'rootcap'. > > "The 16-bit Root Capabilities register is at offset 0x1e in the PCIe > Capability." > > Please make the commit log complete in itself. In some contexts, the > subject line is not visible at the same time. It's fine to repeat the > subject in the commit log. > > > Signed-off-by: Pali Rohár <pali@kernel.org> > > Reviewed-by: Marek Behún <kabel@kernel.org> > > Fixes: 23a5fba4d941 ("PCI: Introduce PCI bridge emulated config space common logic") > > Cc: stable@vger.kernel.org # e0d9d30b7354 ("PCI: pci-bridge-emul: Fix big-endian support") > > I'm not sure how people would deal with *two* SHA1s. I guess that this is fine per stable document as it mention such example: https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html I have already in past sent patches with Fixes:hash1 and CC:stable/hash2 and were taken correctly. > This patch adds functionality, so it's not really fixing a bug in > 23a5fba4d941. I'm not sure what is the correct meaning of Fixes tag. I included it to easily determinate in which commit was introduced member name "rsvd" which should have been named "rootcap". Submitting patches document is not fully clear for me as I understood it that Fixes and CC:stable are two different things. E.g. it mention "Attaching a Fixes: tag does not subvert ... the requirement to Cc: stable@vger.kernel.org on all stable patch candidates." which I understood that patch for backporting needs to have Cc:stable: https://www.kernel.org/doc/html/latest/process/submitting-patches.html But I will change it as needed. Just I did not know what is "the correct way". > I see that e0d9d30b7354 came along later and did > "s/u16 rsvd/__le16 rsvd/". > > But it seems like a lot to expect for distros and stable kernel > maintainers to interpret this. > > Personally I think I would omit both Fixes: and the stable tag since > these two patches (05 and 06) are just adding functionality. > > > --- > > drivers/pci/pci-bridge-emul.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/pci/pci-bridge-emul.h b/drivers/pci/pci-bridge-emul.h > > index b31883022a8e..49bbd37ee318 100644 > > --- a/drivers/pci/pci-bridge-emul.h > > +++ b/drivers/pci/pci-bridge-emul.h > > @@ -54,7 +54,7 @@ struct pci_bridge_emul_pcie_conf { > > __le16 slotctl; > > __le16 slotsta; > > __le16 rootctl; > > - __le16 rsvd; > > + __le16 rootcap; > > __le32 rootsta; > > __le32 devcap2; > > __le16 devctl2; > > -- > > 2.20.1 > >
On Fri, May 07, 2021 at 04:40:15PM +0200, Pali Rohár wrote: > On Thursday 06 May 2021 18:10:09 Bjorn Helgaas wrote: > > On Thu, May 06, 2021 at 05:31:16PM +0200, Pali Rohár wrote: > > > Fixes: 23a5fba4d941 ("PCI: Introduce PCI bridge emulated config space common logic") > > > Cc: stable@vger.kernel.org # e0d9d30b7354 ("PCI: pci-bridge-emul: Fix big-endian support") > > > > I'm not sure how people would deal with *two* SHA1s. > > I guess that this is fine per stable document as it mention such example: > https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html Oh, I see what you mean: the SHA1 on the stable tag is a prerequisite. I had forgotten that, but it makes sense. Thanks! > > This patch adds functionality, so it's not really fixing a bug in > > 23a5fba4d941. > > I'm not sure what is the correct meaning of Fixes tag. I included it to > easily determinate in which commit was introduced member name "rsvd" > which should have been named "rootcap". IMO, the point of "Fixes: X" is to say that "if your kernel includes commit X, you should consider also including this patch because it fixes a problem with X." In this case, the fact that your kernel includes 23a5fba4d941 is not an indication that you should include this patch because this patch doesn't fix a defect in 23a5fba4d941. > Submitting patches document is not fully clear for me as I understood it > that Fixes and CC:stable are two different things. E.g. it mention > "Attaching a Fixes: tag does not subvert ... the requirement to Cc: > stable@vger.kernel.org on all stable patch candidates." which I > understood that patch for backporting needs to have Cc:stable: > https://www.kernel.org/doc/html/latest/process/submitting-patches.html Yes, I think you're right. "Fixes:" by itself just indicates that this fixes a defect in a previous patch. But the fix may not rise to the level of being considered for stable, because historically we only wanted small, critical fixes for stable kernels. I think this patch should omit both Fixes: and the stable tag. If a future patch that depends on this one is a candidate for stable, *that patch* should have a stable tag with this patch as a prerequisite. Useful tidbit I found recently -- a URL like this: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?id=v5.12#n554 is more specific and won't become stale as the doc changes. (If you follow that URL five years later, the doc will have changed so the v5.12 version will be stale, but at least the URL still makes sense in the context of this conversation.) Bjorn
diff --git a/drivers/pci/pci-bridge-emul.h b/drivers/pci/pci-bridge-emul.h index b31883022a8e..49bbd37ee318 100644 --- a/drivers/pci/pci-bridge-emul.h +++ b/drivers/pci/pci-bridge-emul.h @@ -54,7 +54,7 @@ struct pci_bridge_emul_pcie_conf { __le16 slotctl; __le16 slotsta; __le16 rootctl; - __le16 rsvd; + __le16 rootcap; __le32 rootsta; __le32 devcap2; __le16 devctl2;