[PATCH 1/1] PCI/DOE: adjust data object length

[PATCH 1/1] PCI/DOE: adjust data object length

[Li Ming]

The value of data object length 0x0 indicates 2^18 dwords being
transferred, it is introduced in PCIe r6.0,sec 6.30.1. 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>
---
 drivers/pci/doe.c | 21 +++++++++++++++++----
 1 file changed, 17 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
index e402f05068a5..204cbc570f63 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	(2 << 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;
+	u32 length;
 	u32 val;
 	int i;
 
@@ -128,10 +132,12 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
 		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 */
+	length = 2 + task->request_pl_sz / sizeof(u32);
+	if (length == PCI_DOE_MAX_LENGTH)
+		length = 0;
 	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 +184,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 */
@@ -520,8 +529,12 @@ int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
 	/*
 	 * DOE requests must be a whole number of DW and the response needs to
 	 * be big enough for at least 1 DW
+	 *
+	 * Max data object length is 1MB, and data object header occupies 8B,
+	 * thus request_pl_sz should not be greater than 1MB - 8B.
 	 */
-	if (task->request_pl_sz % sizeof(u32) ||
+	if (task->request_pl_sz > SZ_1M - 8 ||
+	    task->request_pl_sz % sizeof(u32) ||
 	    task->response_pl_sz < sizeof(u32))
 		return -EINVAL;
 
-- 
2.25.1

Re: [PATCH 1/1] PCI/DOE: adjust data object length

[Bjorn Helgaas]

On Wed, Nov 09, 2022 at 10:20:44AM +0800, Li Ming wrote:
> The value of data object length 0x0 indicates 2^18 dwords being
> transferred, it is introduced in PCIe r6.0,sec 6.30.1. 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>
> ---
>  drivers/pci/doe.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index e402f05068a5..204cbc570f63 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	(2 << 18)

2 ^  18 == 262144
2 << 18 == 524288

>  /**
>   * 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;
> +	u32 length;
>  	u32 val;
>  	int i;
>  
> @@ -128,10 +132,12 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
>  		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 */
> +	length = 2 + task->request_pl_sz / sizeof(u32);
> +	if (length == PCI_DOE_MAX_LENGTH)
> +		length = 0;

Do you check for overflow anywhere?  What if length is
PCI_DOE_MAX_LENGTH + 1?

>  	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 +184,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 */
> @@ -520,8 +529,12 @@ int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
>  	/*
>  	 * DOE requests must be a whole number of DW and the response needs to
>  	 * be big enough for at least 1 DW
> +	 *
> +	 * Max data object length is 1MB, and data object header occupies 8B,
> +	 * thus request_pl_sz should not be greater than 1MB - 8B.
>  	 */
> -	if (task->request_pl_sz % sizeof(u32) ||
> +	if (task->request_pl_sz > SZ_1M - 8 ||
> +	    task->request_pl_sz % sizeof(u32) ||

Oh, I see, this looks like the check for overflow.  It would be nice
if it were expressed in terms of PCI_DOE_MAX_LENGTH somehow.

It would also be nice, but maybe not practical, to have it closer to
the FIELD_PREP above so it's more obvious that we never try to encode
something too big.

>  	    task->response_pl_sz < sizeof(u32))
>  		return -EINVAL;
>  
> -- 
> 2.25.1
> 

Re: [PATCH 1/1] PCI/DOE: adjust data object length

[Li, Ming]

On 11/10/2022 1:52 AM, Bjorn Helgaas wrote:
> On Wed, Nov 09, 2022 at 10:20:44AM +0800, Li Ming wrote:
>> The value of data object length 0x0 indicates 2^18 dwords being
>> transferred, it is introduced in PCIe r6.0,sec 6.30.1. 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>
>> ---
>>  drivers/pci/doe.c | 21 +++++++++++++++++----
>>  1 file changed, 17 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
>> index e402f05068a5..204cbc570f63 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	(2 << 18)
> 
> 2 ^  18 == 262144
> 2 << 18 == 524288
> 
Thanks for your review, I will fix it in next version.

>>  /**
>>   * 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;
>> +	u32 length;
>>  	u32 val;
>>  	int i;
>>  
>> @@ -128,10 +132,12 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
>>  		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 */
>> +	length = 2 + task->request_pl_sz / sizeof(u32);
>> +	if (length == PCI_DOE_MAX_LENGTH)
>> +		length = 0;
> 
> Do you check for overflow anywhere?  What if length is
> PCI_DOE_MAX_LENGTH + 1?
> 
>>  	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 +184,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 */
>> @@ -520,8 +529,12 @@ int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
>>  	/*
>>  	 * DOE requests must be a whole number of DW and the response needs to
>>  	 * be big enough for at least 1 DW
>> +	 *
>> +	 * Max data object length is 1MB, and data object header occupies 8B,
>> +	 * thus request_pl_sz should not be greater than 1MB - 8B.
>>  	 */
>> -	if (task->request_pl_sz % sizeof(u32) ||
>> +	if (task->request_pl_sz > SZ_1M - 8 ||
>> +	    task->request_pl_sz % sizeof(u32) ||
> 
> Oh, I see, this looks like the check for overflow.  It would be nice
> if it were expressed in terms of PCI_DOE_MAX_LENGTH somehow.
> 
> It would also be nice, but maybe not practical, to have it closer to
> the FIELD_PREP above so it's more obvious that we never try to encode
> something too big.
> 
here is the beginning of a task, and starting to check task->request_pl_sz, so I put request_pl_sz overflow checking here.
do you mean that we keep task->request_pl_sz % sizeof(u32) here and move request_pl_sz overflow checking to closer to the FIELD_PREP above?

Thanks
Ming

>>  	    task->response_pl_sz < sizeof(u32))
>>  		return -EINVAL;
>>  
>> -- 
>> 2.25.1
>>

Re: [PATCH 1/1] PCI/DOE: adjust data object length

[Bjorn Helgaas]

On Thu, Nov 10, 2022 at 09:27:52AM +0800, Li, Ming wrote:
> On 11/10/2022 1:52 AM, Bjorn Helgaas wrote:
> > On Wed, Nov 09, 2022 at 10:20:44AM +0800, Li Ming wrote:
> >> The value of data object length 0x0 indicates 2^18 dwords being
> >> transferred, it is introduced in PCIe r6.0,sec 6.30.1. 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>
> >> ---
> >>  drivers/pci/doe.c | 21 +++++++++++++++++----
> >>  1 file changed, 17 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> >> index e402f05068a5..204cbc570f63 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	(2 << 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;
> >> +	u32 length;
> >>  	u32 val;
> >>  	int i;
> >>  
> >> @@ -128,10 +132,12 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
> >>  		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 */
> >> +	length = 2 + task->request_pl_sz / sizeof(u32);
> >> +	if (length == PCI_DOE_MAX_LENGTH)
> >> +		length = 0;
> > 
> > Do you check for overflow anywhere?  What if length is
> > PCI_DOE_MAX_LENGTH + 1?
> > 
> >>  	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 +184,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 */
> >> @@ -520,8 +529,12 @@ int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
> >>  	/*
> >>  	 * DOE requests must be a whole number of DW and the response needs to
> >>  	 * be big enough for at least 1 DW
> >> +	 *
> >> +	 * Max data object length is 1MB, and data object header occupies 8B,
> >> +	 * thus request_pl_sz should not be greater than 1MB - 8B.
> >>  	 */
> >> -	if (task->request_pl_sz % sizeof(u32) ||
> >> +	if (task->request_pl_sz > SZ_1M - 8 ||
> >> +	    task->request_pl_sz % sizeof(u32) ||
> > 
> > Oh, I see, this looks like the check for overflow.  It would be nice
> > if it were expressed in terms of PCI_DOE_MAX_LENGTH somehow.
> > 
> > It would also be nice, but maybe not practical, to have it closer to
> > the FIELD_PREP above so it's more obvious that we never try to encode
> > something too big.
> > 
> here is the beginning of a task, and starting to check
> task->request_pl_sz, so I put request_pl_sz overflow checking here.
>
> do you mean that we keep task->request_pl_sz % sizeof(u32) here and
> move request_pl_sz overflow checking to closer to the FIELD_PREP
> above?

Yes, that's what I meant.

I think the more important thing is to do the check using
PCI_DOE_MAX_LENGTH if possible so the connection is obvious and
consistent.

Re: [PATCH 1/1] PCI/DOE: adjust data object length

[Jonathan Cameron]

On Wed,  9 Nov 2022 10:20:44 +0800
Li Ming <ming4.li@intel.com> wrote:

> The value of data object length 0x0 indicates 2^18 dwords being
> transferred, it is introduced in PCIe r6.0,sec 6.30.1. This patch

Was introduced prior to that in the DOE ECN, so perhaps just drop "introduced".

I'm not sure why we missed that little detail of the spec originally so good to fix this up.
Probably deserves a fixes tag though it would be very hard to hit
with the only protocol we currently have upstream.

Other than what Bjorn pointed out and the missing bracket the
Robot found, looks good to me.

> 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>
> ---
>  drivers/pci/doe.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index e402f05068a5..204cbc570f63 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	(2 << 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;
> +	u32 length;
>  	u32 val;
>  	int i;
>  
> @@ -128,10 +132,12 @@ static int pci_doe_send_req(struct pci_doe_mb *doe_mb,
>  		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 */
> +	length = 2 + task->request_pl_sz / sizeof(u32);
> +	if (length == PCI_DOE_MAX_LENGTH)
> +		length = 0;
>  	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 +184,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 */
> @@ -520,8 +529,12 @@ int pci_doe_submit_task(struct pci_doe_mb *doe_mb, struct pci_doe_task *task)
>  	/*
>  	 * DOE requests must be a whole number of DW and the response needs to
>  	 * be big enough for at least 1 DW
> +	 *
> +	 * Max data object length is 1MB, and data object header occupies 8B,
> +	 * thus request_pl_sz should not be greater than 1MB - 8B.
>  	 */
> -	if (task->request_pl_sz % sizeof(u32) ||
> +	if (task->request_pl_sz > SZ_1M - 8 ||
> +	    task->request_pl_sz % sizeof(u32) ||
>  	    task->response_pl_sz < sizeof(u32))
>  		return -EINVAL;
>