> -----Original Message-----
> From: Luca Coelho <luca@xxxxxxxxx>
> Sent: Tuesday, October 31, 2023 3:35 PM
> To: Manna, Animesh <animesh.manna@xxxxxxxxx>; intel-
> gfx@xxxxxxxxxxxxxxxxxxxxx
> Cc: Nikula, Jani <jani.nikula@xxxxxxxxx>
> Subject: Re: [PATCH v4] drm/i915/dsb: DSB code refactoring
>
> On Tue, 2023-10-31 at 09:15 +0000, Manna, Animesh wrote:
> >
> > > -----Original Message-----
> > > From: Luca Coelho <luca@xxxxxxxxx>
> > > Sent: Tuesday, October 31, 2023 1:14 PM
> > > To: Manna, Animesh <animesh.manna@xxxxxxxxx>; intel-
> > > gfx@xxxxxxxxxxxxxxxxxxxxx
> > > Cc: Nikula, Jani <jani.nikula@xxxxxxxxx>
> > > Subject: Re: [PATCH v4] drm/i915/dsb: DSB code
> > > refactoring
> > >
> > > On Fri, 2023-10-27 at 17:27 +0530, Animesh Manna wrote:
> > > > Refactor DSB implementation to be compatible with Xe driver.
> > > >
> > > > v1: RFC version.
> > > > v2: Make intel_dsb structure opaque from external usage. [Jani]
> > > > v3: Rebased on latest.
> > > > v4:
> > > > - Add boundary check in dsb_buffer_memset(). [Luca]
> > > > - Use size_t instead of u32. [Luca]
> > > >
> > > > Cc: Jani Nikula <jani.nikula@xxxxxxxxx>
> > > > Signed-off-by: Animesh Manna <animesh.manna@xxxxxxxxx>
> > > > ---
> > >
> > > [...]
> > > > +void intel_dsb_buffer_memset(struct intel_dsb_buffer *dsb_buf,
> > > > +u32 idx, u32 val, size_t size) {
> > > > + if ((idx > dsb_buf->buf_size / 4) || (size > dsb_buf->buf_size -
> > > > +idx
> > > > +* 4))
> > >
> > > You actually don't need the first expression. This expression
> > > should
> > > enough:
> > >
> > > dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf->cmd_buf)
> >
> > Here size is in bytes, but idx is index of 32 bytes array. So, the
> > above expression would be,
> >
> > dsb_buf->buf_size <= (idx * sizeof(*dsb_buf->cmd_buf) + size)
>
> Oh, you're right, of course.
>
>
> > The same is done with 2nd expression but agree to use sizeof() instead of
> magic number 4.
> >
> > The first expression is added if idx is big number and due to overflow the
> above check can pass which is not correct. Please let me know your thoughts,
> if you are not ok will drop maybe.
>
> If you're worried about overflow when you're multiplying by 4, then you can
> just do it the opposite way, still with a single expression:
>
> dsb_buf->buf_size / sizeof(*dsb_buf->cmd_buf) <= idx + size /
> sizeof(*dsb_buf->cmd_buf)
>
> Or, taking advantage of the fact that both buf_size and size need to be
> divided by sizeof(), we could something like:
>
> idx > (dsb_buf->buf_size - size) / sizeof(*dsb_buf->cmd_buf)
>
> ...but we're bike-shedding. I don't think the number of expressions or the
> complexity of the expressions matter much here, unless you're really in a
> hotpath, in which case you should add an unlikely() or so.
>
> I'll leave it to you.
>
>
> > >
> > > > + return;
> > >
> > > Blindly returning here doesn't solve the problem, it just hides it.
> > > I think the best would be to use WARN_ON() instead of if.
> > >
> > > So:
> > > WARN_ON(dsb_buf->buf_size <= (idx + size) * sizeof(*dsb_buf-
> > > > cmd_buf));
> >
> > I will add the WARN_ON().
>
> This is the part that I actually think is important. ;)
Thanks for review, have floated v5 after adding the changes.
Regards,
Animesh
>
> --
> Cheers,
> Luca.
