Re: [PATCH v3 1/9] cxl/mbox: Move cxl_mem_command construction to helper funcs

[Alison Schofield <alison.schofield@intel.com>]

On Fri, Mar 25, 2022 at 10:27:24AM +0000, Jonathan Cameron wrote:
> On Wed, 23 Mar 2022 18:11:18 -0700
> alison.schofield@intel.com wrote:
> 
> > From: Alison Schofield <alison.schofield@intel.com>
> > 
> > Sanitizing and constructing a cxl_mem_command from a userspace
> > command is part of the validation process prior to submitting
> > the command to a CXL device. Move this work to helper functions:
> > cxl_to_mem_cmd(), cxl_to_mem_cmd_raw().
> > 
> > This declutters cxl_validate_cmd_from_user() in preparation for
> > adding new validation steps.
> > 
> > Signed-off-by: Alison Schofield <alison.schofield@intel.com>
> 
> A few trivial comments inline.
> 
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
> 

Thanks for the review Jonathan -

> > ---

snip
> >  
> > +static int cxl_to_mem_cmd(struct cxl_dev_state *cxlds,
> > +			  const struct cxl_send_command *send_cmd,
> > +			  struct cxl_mem_command *mem_cmd)
> > +{
> > +	const struct cxl_command_info *info;
> > +	struct cxl_mem_command *c;
> > +
> > +	if (send_cmd->flags & ~CXL_MEM_COMMAND_FLAG_MASK)
> > +		return -EINVAL;
> > +
> > +	if (send_cmd->rsvd)
> > +		return -EINVAL;
> > +
> > +	if (send_cmd->in.rsvd || send_cmd->out.rsvd)
> > +		return -EINVAL;
> > +
> > +	/* Convert user's command into the internal representation */
> 
> Not clear which chunk of code this applies to.  Seems like we
> are just getting some addresses here (obviously that applies to original
> code as well) Perhaps move down to where you fill in mem_cmd?

The comment above applies to the line that immediately follow.
ie. senc_cmd->id indexes into the command array.

> 
> > +	c = &cxl_mem_commands[send_cmd->id];
> > +	info = &c->info;
> 
> I don't mind that much either way, but you could do these at
> declaration of the local variables above, before doing the sanity checks.
> 

I err'd on moving things as much 'as is' as possible to keep the
patches easier to review. I like your suggestion and will do it.
(Please like it when you see it in the next version ;))

> > +
snip

> > -
> > -	return 0;
> > +	return rc;
> I haven't read on yet so I'll assume there is more coming in this function as otherwise
> you could just return directly in the two if / else paths.

Yeah, you are seeing some foreshadowing, and you've noted similar in
other patches. I'll clean that up.

Thanks,
Alison

> 
> Thanks,
> 
> Jonathan
> 
> >  }
> >  
> >  int cxl_query_cmd(struct cxl_memdev *cxlmd,
>