Re: [Intel-xe] [PATCH] drm/xe: remove header variable from parse_g2h_msg

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Aug 01, 2023 at 03:37:43AM +0000, Matthew Brost wrote:
> On Mon, Jul 31, 2023 at 02:08:06PM -0400, Rodrigo Vivi wrote:
> > On Fri, Jul 28, 2023 at 09:02:11PM -0700, Matthew Brost wrote:
> > > The header variable is unused, remove it.
> > > 
> > > Suggested-by: Oded Gabbay <ogabbay@kernel.org>
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_guc_ct.c | 3 +--
> > >  1 file changed, 1 insertion(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/xe/xe_guc_ct.c b/drivers/gpu/drm/xe/xe_guc_ct.c
> > > index d322eadbe75a..6c757efa88b8 100644
> > > --- a/drivers/gpu/drm/xe/xe_guc_ct.c
> > > +++ b/drivers/gpu/drm/xe/xe_guc_ct.c
> > > @@ -829,12 +829,11 @@ static int parse_g2h_response(struct xe_guc_ct *ct, u32 *msg, u32 len)
> > >  static int parse_g2h_msg(struct xe_guc_ct *ct, u32 *msg, u32 len)
> > >  {
> > >  	struct xe_device *xe = ct_to_xe(ct);
> > > -	u32 header, hxg, origin, type;
> > > +	u32 hxg, origin, type;
> > >  	int ret;
> > >  
> > >  	lockdep_assert_held(&ct->lock);
> > >  
> > > -	header = msg[0];
> > 
> > Shouldn't we be parsing the header and checking for the len?
> > against corrupted messages?
> > 
> 
> Yep and we do. From g2h_read:
> 
>  955         /* Read header */
>  956         xe_map_memcpy_from(xe, msg, &g2h->cmds, sizeof(u32) * g2h->info.head,
>  957                            sizeof(u32));
>  958         len = FIELD_GET(GUC_CTB_MSG_0_NUM_DWORDS, msg[0]) + GUC_CTB_MSG_MIN_LEN;
>  959         if (len > avail) {
>  960                 drm_err(&xe->drm,
>  961                         "G2H channel broken on read, avail=%d, len=%d, reset required\n",
>  962                         avail, len);
>  963                 g2h->info.broken = true;
>  964
>  965                 return -EPROTO;
>  966         }

oh, indeed! and that comes before.
So, let's kill this useless header.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> Matt
> 
> > something like on i915's intel_guc_ct.c:
> > 
> >   /* message len with header */
> >         len = FIELD_GET(GUC_CTB_MSG_0_NUM_DWORDS, header) + GUC_CTB_MSG_MIN_LEN;
> >  	if (unlikely(len > (u32)available)) {
> >                 CT_ERROR(ct, "Incomplete message %*ph %*ph %*ph\n",
> > 	                 4, &header,
> >                          4 * (head + available - 1 > size ?
> >                               size - head : available - 1), &cmds[head],
> > 	                 4 * (head + available - 1 > size ?
> >                               available - 1 - size + head : 0), &cmds[0]);
> >                 desc->status |= GUC_CTB_STATUS_UNDERFLOW;
> > 	        goto corrupted;
> >         }
> > 
> > 
> > >  	hxg = msg[1];
> > >  
> > >  	origin = FIELD_GET(GUC_HXG_MSG_0_ORIGIN, hxg);
> > > -- 
> > > 2.34.1
> > >