| Message ID | 20221116015637.3299664-1-ming4.li@intel.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | a4ff8e7a71601321f7bf7b58ede664dc0d774274 |
| Headers | show |
| Series | [V2,1/1] PCI/DOE: Fix maximum data object length miscalculation | expand |
On Wed, 16 Nov 2022 09:56:37 +0800 Li Ming <ming4.li@intel.com> wrote: > The value of data object length 0x0 indicates 2^18 dwords being > transferred. This patch adjusts the value of data object length for the > above case on both sending side and receiving side. > > Besides, it is unnecessary to check whether length is greater than > SZ_1M while receiving a response data object, because length from LENGTH > field of data object header, max value is 2^18. > > Signed-off-by: Li Ming <ming4.li@intel.com> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> Thanks, > --- > v1->v2: > * Fix the value of PCI_DOE_MAX_LENGTH > * Moving length checking closer to transferring process > * Add a missing bracket > * Adjust patch description > --- > drivers/pci/doe.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > index e402f05068a5..66d9ab288646 100644 > --- a/drivers/pci/doe.c > +++ b/drivers/pci/doe.c > @@ -29,6 +29,9 @@ > #define PCI_DOE_FLAG_CANCEL 0 > #define PCI_DOE_FLAG_DEAD 1 > > +/* Max data object length is 2^18 dwords */ > +#define PCI_DOE_MAX_LENGTH (1 << 18) > + > /** > * struct pci_doe_mb - State for a single DOE mailbox > * > @@ -107,6 +110,7 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, > { > struct pci_dev *pdev = doe_mb->pdev; > int offset = doe_mb->cap_offset; > + size_t length; > u32 val; > int i; > > @@ -123,15 +127,20 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, > if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) > return -EIO; > > + /* Length is 2 DW of header + length of payload in DW */ > + length = 2 + task->request_pl_sz / sizeof(u32); > + if (length > PCI_DOE_MAX_LENGTH) > + return -EIO; > + if (length == PCI_DOE_MAX_LENGTH) > + length = 0; > + > /* Write DOE Header */ > val = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID, task->prot.vid) | > FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE, task->prot.type); > pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, val); > - /* Length is 2 DW of header + length of payload in DW */ > pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, > FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, > - 2 + task->request_pl_sz / > - sizeof(u32))); > + length)); > for (i = 0; i < task->request_pl_sz / sizeof(u32); i++) > pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, > task->request_pl[i]); > @@ -178,7 +187,10 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas > pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0); > > length = FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, val); > - if (length > SZ_1M || length < 2) > + /* A value of 0x0 indicates max data object length */ > + if (!length) > + length = PCI_DOE_MAX_LENGTH; > + if (length < 2) > return -EIO; > > /* First 2 dwords have already been read */
On Wed, Nov 16, 2022 at 09:56:37AM +0800, Li Ming wrote: > The value of data object length 0x0 indicates 2^18 dwords being > transferred. A spec reference would be nice, e.g.: "According to PCIe r6.0.1 sec 6.30.1 table 6-29, the value of ..." > This patch adjusts the value of data object length for the > above case on both sending side and receiving side. Nit: Generally phrases like "This patch ..." should be avoided. Use imperative mood instead, e.g.: "Adjust the value ..." > Signed-off-by: Li Ming <ming4.li@intel.com> Reviewed-by: Lukas Wunner <lukas@wunner.de> Thanks, Lukas
On Wed, Nov 16, 2022 at 09:56:37AM +0800, Li Ming wrote: > The value of data object length 0x0 indicates 2^18 dwords being > transferred. This patch adjusts the value of data object length for the > above case on both sending side and receiving side. > > Besides, it is unnecessary to check whether length is greater than > SZ_1M while receiving a response data object, because length from LENGTH > field of data object header, max value is 2^18. > > Signed-off-by: Li Ming <ming4.li@intel.com> Applied with Reviewed-by from Jonathan and Lukas, thank you very much to all of you! I touched up the commit log; let me know if I made anything worse: PCI/DOE: Fix maximum data object length miscalculation Per PCIe r6.0, sec 6.30.1, a data object Length of 0x0 indicates 2^18 DWORDs (256K DW or 1MB) being transferred. Adjust the value of data object length for this case on both sending side and receiving side. Don't bother checking whether Length is greater than SZ_1M because all values of the 18-bit Length field are valid, and it is impossible to represent anything larger than SZ_1M: 0x00000 256K DW (1M bytes) 0x00001 1 DW (4 bytes) ... 0x3ffff 256K-1 DW (1M - 4 bytes) > --- > v1->v2: > * Fix the value of PCI_DOE_MAX_LENGTH > * Moving length checking closer to transferring process > * Add a missing bracket > * Adjust patch description > --- > drivers/pci/doe.c | 20 ++++++++++++++++---- > 1 file changed, 16 insertions(+), 4 deletions(-) > > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c > index e402f05068a5..66d9ab288646 100644 > --- a/drivers/pci/doe.c > +++ b/drivers/pci/doe.c > @@ -29,6 +29,9 @@ > #define PCI_DOE_FLAG_CANCEL 0 > #define PCI_DOE_FLAG_DEAD 1 > > +/* Max data object length is 2^18 dwords */ > +#define PCI_DOE_MAX_LENGTH (1 << 18) > + > /** > * struct pci_doe_mb - State for a single DOE mailbox > * > @@ -107,6 +110,7 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, > { > struct pci_dev *pdev = doe_mb->pdev; > int offset = doe_mb->cap_offset; > + size_t length; > u32 val; > int i; > > @@ -123,15 +127,20 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, > if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) > return -EIO; > > + /* Length is 2 DW of header + length of payload in DW */ > + length = 2 + task->request_pl_sz / sizeof(u32); > + if (length > PCI_DOE_MAX_LENGTH) > + return -EIO; > + if (length == PCI_DOE_MAX_LENGTH) > + length = 0; > + > /* Write DOE Header */ > val = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID, task->prot.vid) | > FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE, task->prot.type); > pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, val); > - /* Length is 2 DW of header + length of payload in DW */ > pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, > FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, > - 2 + task->request_pl_sz / > - sizeof(u32))); > + length)); > for (i = 0; i < task->request_pl_sz / sizeof(u32); i++) > pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, > task->request_pl[i]); > @@ -178,7 +187,10 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas > pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0); > > length = FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, val); > - if (length > SZ_1M || length < 2) > + /* A value of 0x0 indicates max data object length */ > + if (!length) > + length = PCI_DOE_MAX_LENGTH; > + if (length < 2) > return -EIO; > > /* First 2 dwords have already been read */ > -- > 2.25.1 >
On Wed, Nov 16, 2022 at 12:02:50PM -0600, Bjorn Helgaas wrote: > On Wed, Nov 16, 2022 at 09:56:37AM +0800, Li Ming wrote: > > The value of data object length 0x0 indicates 2^18 dwords being > > transferred. This patch adjusts the value of data object length for the > > above case on both sending side and receiving side. > > > > Besides, it is unnecessary to check whether length is greater than > > SZ_1M while receiving a response data object, because length from LENGTH > > field of data object header, max value is 2^18. > > > > Signed-off-by: Li Ming <ming4.li@intel.com> > > Applied with Reviewed-by from Jonathan and Lukas, thank you very much > to all of you! Oh, I forgot to mention, this is on a pci/doe branch, but if you need to include this with other CXL work, just let me know, add my: Acked-by: Bjorn Helgaas <bhelgaas@google.com> and I'll drop it on my side. Bjorn
On Wed, Nov 16, 2022 at 12:02:50PM -0600, Bjorn Helgaas wrote: > On Wed, Nov 16, 2022 at 09:56:37AM +0800, Li Ming wrote: > > The value of data object length 0x0 indicates 2^18 dwords being > > transferred. This patch adjusts the value of data object length for the > > above case on both sending side and receiving side. > > > > Besides, it is unnecessary to check whether length is greater than > > SZ_1M while receiving a response data object, because length from LENGTH > > field of data object header, max value is 2^18. > > > > Signed-off-by: Li Ming <ming4.li@intel.com> > > Applied with Reviewed-by from Jonathan and Lukas, thank you very much > to all of you! > > I touched up the commit log; let me know if I made anything worse: Jonathan mentioned that a Fixes tag might make sense. If you agree: Fixes: 9d24322e887b ("PCI/DOE: Add DOE mailbox support functions") Cc: stable@vger.kernel.org # v6.0+ Thanks! Lukas
On Wed, Nov 16, 2022 at 07:56:10PM +0100, Lukas Wunner wrote: > On Wed, Nov 16, 2022 at 12:02:50PM -0600, Bjorn Helgaas wrote: > > On Wed, Nov 16, 2022 at 09:56:37AM +0800, Li Ming wrote: > > > The value of data object length 0x0 indicates 2^18 dwords being > > > transferred. This patch adjusts the value of data object length for the > > > above case on both sending side and receiving side. > > > > > > Besides, it is unnecessary to check whether length is greater than > > > SZ_1M while receiving a response data object, because length from LENGTH > > > field of data object header, max value is 2^18. > > > > > > Signed-off-by: Li Ming <ming4.li@intel.com> > > > > Applied with Reviewed-by from Jonathan and Lukas, thank you very much > > to all of you! > > > > I touched up the commit log; let me know if I made anything worse: > > Jonathan mentioned that a Fixes tag might make sense. If you agree: > > Fixes: 9d24322e887b ("PCI/DOE: Add DOE mailbox support functions") > Cc: stable@vger.kernel.org # v6.0+ Sure, sorry I missed that. Although I don't see it on the list, so maybe it was a side-band comment. Added in any case. Bjorn
diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c index e402f05068a5..66d9ab288646 100644 --- a/drivers/pci/doe.c +++ b/drivers/pci/doe.c @@ -29,6 +29,9 @@ #define PCI_DOE_FLAG_CANCEL 0 #define PCI_DOE_FLAG_DEAD 1 +/* Max data object length is 2^18 dwords */ +#define PCI_DOE_MAX_LENGTH (1 << 18) + /** * struct pci_doe_mb - State for a single DOE mailbox * @@ -107,6 +110,7 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, { struct pci_dev *pdev = doe_mb->pdev; int offset = doe_mb->cap_offset; + size_t length; u32 val; int i; @@ -123,15 +127,20 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb, if (FIELD_GET(PCI_DOE_STATUS_ERROR, val)) return -EIO; + /* Length is 2 DW of header + length of payload in DW */ + length = 2 + task->request_pl_sz / sizeof(u32); + if (length > PCI_DOE_MAX_LENGTH) + return -EIO; + if (length == PCI_DOE_MAX_LENGTH) + length = 0; + /* Write DOE Header */ val = FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_VID, task->prot.vid) | FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_1_TYPE, task->prot.type); pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, val); - /* Length is 2 DW of header + length of payload in DW */ pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, FIELD_PREP(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, - 2 + task->request_pl_sz / - sizeof(u32))); + length)); for (i = 0; i < task->request_pl_sz / sizeof(u32); i++) pci_write_config_dword(pdev, offset + PCI_DOE_WRITE, task->request_pl[i]); @@ -178,7 +187,10 @@ static int pci_doe_recv_resp(struct pci_doe_mb *doe_mb, struct pci_doe_task *tas pci_write_config_dword(pdev, offset + PCI_DOE_READ, 0); length = FIELD_GET(PCI_DOE_DATA_OBJECT_HEADER_2_LENGTH, val); - if (length > SZ_1M || length < 2) + /* A value of 0x0 indicates max data object length */ + if (!length) + length = PCI_DOE_MAX_LENGTH; + if (length < 2) return -EIO; /* First 2 dwords have already been read */
The value of data object length 0x0 indicates 2^18 dwords being transferred. This patch adjusts the value of data object length for the above case on both sending side and receiving side. Besides, it is unnecessary to check whether length is greater than SZ_1M while receiving a response data object, because length from LENGTH field of data object header, max value is 2^18. Signed-off-by: Li Ming <ming4.li@intel.com> --- v1->v2: * Fix the value of PCI_DOE_MAX_LENGTH * Moving length checking closer to transferring process * Add a missing bracket * Adjust patch description --- drivers/pci/doe.c | 20 ++++++++++++++++---- 1 file changed, 16 insertions(+), 4 deletions(-)