Re: [PATCH v1 1/2] fsi: sbefifo: Remove write's max command length

[Ninad Palsule <ninad@linux.ibm.com>]

Hi Joel,

On 9/11/23 01:03, Joel Stanley wrote:
> On Thu, 7 Sept 2023 at 22:10, Ninad Palsule <ninad@linux.ibm.com> wrote:
>> This commit removes max command length check in the user write path.
>> This is required to support images larger than 1MB. This should not
>> create any issues as read path does not have this check either.
>>
>> As per the original design cronus server was suppose to break up the
>> image into 1MB pieces but it requires restructuring of the driver.
> When you say "driver" you mean the kernel driver, or userspace?
>
> This isn't a great justification for removing a bounds check.
I have improved the comment. Added length check back.
>
>> Today driver sends EOT message on each write request so we will have to
>> send it only after all pieces are sent which requires large change hence
>> we decided to remove this check.
> This paragraph could be clearer. Could you try rephrasing?
>
> Assuming we want to make this change, what is the expected maximum
> transfer? Could we instead make the check be that value (3MB?).
Added length check back. I am using 4MB for some cushion.

Thanks for the review.

~Ninad

>> Testing:
>>    Loaded 3 MB image through cronus server.
>>
>> Signed-off-by: Ninad Palsule <ninad@linux.ibm.com>
>> ---
>>   drivers/fsi/fsi-sbefifo.c | 3 ---
>>   1 file changed, 3 deletions(-)
>>
>> diff --git a/drivers/fsi/fsi-sbefifo.c b/drivers/fsi/fsi-sbefifo.c
>> index 9912b7a6a4b9..b771dff27f7f 100644
>> --- a/drivers/fsi/fsi-sbefifo.c
>> +++ b/drivers/fsi/fsi-sbefifo.c
>> @@ -113,7 +113,6 @@ enum sbe_state
>>   #define SBEFIFO_TIMEOUT_IN_RSP         1000
>>
>>   /* Other constants */
>> -#define SBEFIFO_MAX_USER_CMD_LEN       (0x100000 + PAGE_SIZE)
>>   #define SBEFIFO_RESET_MAGIC            0x52534554 /* "RSET" */
>>
>>   struct sbefifo {
>> @@ -870,8 +869,6 @@ static ssize_t sbefifo_user_write(struct file *file, const char __user *buf,
>>          if (!user)
>>                  return -EINVAL;
>>          sbefifo = user->sbefifo;
>> -       if (len > SBEFIFO_MAX_USER_CMD_LEN)
>> -               return -EINVAL;
>>          if (len & 3)
>>                  return -EINVAL;
>>
>> --
>> 2.39.2
>>