[Intel-gfx] [PATCH v4] drm/i915/dsb: DSB code refactoring

[Luca Coelho]

On Tue, 2023-10-31 at 09:15 +0000, Manna, Animesh wrote:
> 
> > -----Original Message-----
> > From: Luca Coelho <luca at coelho.fi>
> > Sent: Tuesday, October 31, 2023 1:14 PM
> > To: Manna, Animesh <animesh.manna at intel.com>; intel-
> > gfx at lists.freedesktop.org
> > Cc: Nikula, Jani <jani.nikula at intel.com>
> > Subject: Re: [Intel-gfx] [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 at intel.com>
> > > Signed-off-by: Animesh Manna <animesh.manna at intel.com>
> > > ---
> > 
> > [...]
> > > +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. ;)

--
Cheers,
Luca.