[PATCH V2 1/1] PCI/DOE: Fix maximum data object length miscalculation

[PATCH V2 1/1] PCI/DOE: Fix maximum data object length miscalculation

[Li Ming]

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(-)

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

Re: [PATCH V2 1/1] PCI/DOE: Fix maximum data object length miscalculation

[Jonathan Cameron]

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 */

Re: [PATCH V2 1/1] PCI/DOE: Fix maximum data object length miscalculation

[Lukas Wunner]

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

Re: [PATCH V2 1/1] PCI/DOE: Fix maximum data object length miscalculation

[Bjorn Helgaas]

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
> 

Re: [PATCH V2 1/1] PCI/DOE: Fix maximum data object length miscalculation

[Bjorn Helgaas]

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

Re: [PATCH V2 1/1] PCI/DOE: Fix maximum data object length miscalculation

[Lukas Wunner]

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

Re: [PATCH V2 1/1] PCI/DOE: Fix maximum data object length miscalculation

[Bjorn Helgaas]

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