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

[Ninad Palsule]

Hi Joel,

On 9/11/23 01:03, Joel Stanley wrote:
> On Thu, 7 Sept 2023 at 22:10, Ninad Palsule <ninad@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> > ---
> >   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