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@xxxxxxxxx>
>> ---
>> 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
>>
