| Message ID | 265c48b1ae2d6ebfed21f41bc1bfd2c2b98a1257.1648601710.git.alison.schofield@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Do not allow set-partition immediate mode | expand |
On Tue, Mar 29, 2022 at 6:28 PM <alison.schofield@intel.com> wrote: > > From: Alison Schofield <alison.schofield@intel.com> > > Previously, handle_mailbox_cmd_from_user(), constructed the mailbox > command and dispatched it to the hardware. The construction work > has moved to the validation path. > > handle_mailbox_cmd_from_user() now expects a fully validated > mbox param. Make it's caller, cxl_send_cmd(), deliver it. Update > the comments and dereferencing of the new mbox parameter. It's not clear to me that handle_mailbox_cmd_from_user() has reason to exist as a helper vs being open coded in cxl_send_cmd() after this, but that can be a follow-on cleanup. > > Signed-off-by: Alison Schofield <alison.schofield@intel.com> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > --- > drivers/cxl/core/mbox.c | 54 +++++++++++++++++------------------------ > 1 file changed, 22 insertions(+), 32 deletions(-) > > diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c > index 654e61a77eb3..b0abda67d7da 100644 > --- a/drivers/cxl/core/mbox.c > +++ b/drivers/cxl/core/mbox.c > @@ -417,8 +417,7 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, > /** > * handle_mailbox_cmd_from_user() - Dispatch a mailbox command for userspace. > * @cxlds: The device data for the operation > - * @cmd: The validated command. > - * @in_payload: Pointer to userspace's input payload. > + * @mbox_cmd: The validated mailbox command. > * @out_payload: Pointer to userspace's output payload. > * @size_out: (Input) Max payload size to copy out. > * (Output) Payload size hardware generated. > @@ -433,59 +432,48 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, > * * %-EINTR - Mailbox acquisition interrupted. > * * %-EXXX - Transaction level failures. > * > - * Creates the appropriate mailbox command and dispatches it on behalf of a > - * userspace request. The input and output payloads are copied between > - * userspace. > + * Dispatches a mailbox command on behalf of a userspace request. > + * The output payload is copied to userspace. > * > * See cxl_send_cmd(). > */ > static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds, > - const struct cxl_mem_command *cmd, > - u64 in_payload, u64 out_payload, > - s32 *size_out, u32 *retval) > + struct cxl_mbox_cmd *mbox_cmd, > + u64 out_payload, s32 *size_out, > + u32 *retval) > { > struct device *dev = cxlds->dev; > - struct cxl_mbox_cmd mbox_cmd; > int rc; > > - rc = cxl_to_mbox_cmd(cxlds, &mbox_cmd, cmd->opcode, cmd->info.size_in, > - cmd->info.size_out, in_payload); > - if (rc) > - return rc; > - > dev_dbg(dev, > "Submitting %s command for user\n" > "\topcode: %x\n" > "\tsize: %zx\n", > - cxl_mem_opcode_to_name(mbox_cmd.opcode), > - mbox_cmd.opcode, mbox_cmd.size_in); > + cxl_mem_opcode_to_name(mbox_cmd->opcode), > + mbox_cmd->opcode, mbox_cmd->size_in); > > - rc = cxlds->mbox_send(cxlds, &mbox_cmd); > + rc = cxlds->mbox_send(cxlds, mbox_cmd); > if (rc) > - goto out; > + return rc; > > /* > * @size_out contains the max size that's allowed to be written back out > * to userspace. While the payload may have written more output than > * this it will have to be ignored. > */ > - if (mbox_cmd.size_out) { > - dev_WARN_ONCE(dev, mbox_cmd.size_out > *size_out, > + if (mbox_cmd->size_out) { > + dev_WARN_ONCE(dev, mbox_cmd->size_out > *size_out, > "Invalid return size\n"); > if (copy_to_user(u64_to_user_ptr(out_payload), > - mbox_cmd.payload_out, mbox_cmd.size_out)) { > - rc = -EFAULT; > - goto out; > + mbox_cmd->payload_out, mbox_cmd->size_out)) { > + return -EFAULT; > } > } > > - *size_out = mbox_cmd.size_out; > - *retval = mbox_cmd.return_code; > + *size_out = mbox_cmd->size_out; > + *retval = mbox_cmd->return_code; > > -out: > - kvfree(mbox_cmd.payload_in); > - kvfree(mbox_cmd.payload_out); > - return rc; > + return 0; > } > > int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s) > @@ -506,9 +494,11 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s) > if (rc) > return rc; > > - rc = handle_mailbox_cmd_from_user(cxlds, &c, send.in.payload, > - send.out.payload, &send.out.size, > - &send.retval); > + rc = handle_mailbox_cmd_from_user(cxlds, &mbox_cmd, send.out.payload, > + &send.out.size, &send.retval); > + > + kvfree(mbox_cmd.payload_in); > + kvfree(mbox_cmd.payload_out); It's not obvious that these are paired with the allocations in cxl_to_mbox_cmd(). How about rename cxl_to_mbox_cmd() to cxl_mbox_cmd_ctor(), and wrap these kvfree() calls in a helper called cxl_mbox_cmd_dtor()? Otherwise, these are only minor comments, so you can add: Reviewed-by: Dan Williams <dan.j.williams@intel.com>
diff --git a/drivers/cxl/core/mbox.c b/drivers/cxl/core/mbox.c index 654e61a77eb3..b0abda67d7da 100644 --- a/drivers/cxl/core/mbox.c +++ b/drivers/cxl/core/mbox.c @@ -417,8 +417,7 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, /** * handle_mailbox_cmd_from_user() - Dispatch a mailbox command for userspace. * @cxlds: The device data for the operation - * @cmd: The validated command. - * @in_payload: Pointer to userspace's input payload. + * @mbox_cmd: The validated mailbox command. * @out_payload: Pointer to userspace's output payload. * @size_out: (Input) Max payload size to copy out. * (Output) Payload size hardware generated. @@ -433,59 +432,48 @@ int cxl_query_cmd(struct cxl_memdev *cxlmd, * * %-EINTR - Mailbox acquisition interrupted. * * %-EXXX - Transaction level failures. * - * Creates the appropriate mailbox command and dispatches it on behalf of a - * userspace request. The input and output payloads are copied between - * userspace. + * Dispatches a mailbox command on behalf of a userspace request. + * The output payload is copied to userspace. * * See cxl_send_cmd(). */ static int handle_mailbox_cmd_from_user(struct cxl_dev_state *cxlds, - const struct cxl_mem_command *cmd, - u64 in_payload, u64 out_payload, - s32 *size_out, u32 *retval) + struct cxl_mbox_cmd *mbox_cmd, + u64 out_payload, s32 *size_out, + u32 *retval) { struct device *dev = cxlds->dev; - struct cxl_mbox_cmd mbox_cmd; int rc; - rc = cxl_to_mbox_cmd(cxlds, &mbox_cmd, cmd->opcode, cmd->info.size_in, - cmd->info.size_out, in_payload); - if (rc) - return rc; - dev_dbg(dev, "Submitting %s command for user\n" "\topcode: %x\n" "\tsize: %zx\n", - cxl_mem_opcode_to_name(mbox_cmd.opcode), - mbox_cmd.opcode, mbox_cmd.size_in); + cxl_mem_opcode_to_name(mbox_cmd->opcode), + mbox_cmd->opcode, mbox_cmd->size_in); - rc = cxlds->mbox_send(cxlds, &mbox_cmd); + rc = cxlds->mbox_send(cxlds, mbox_cmd); if (rc) - goto out; + return rc; /* * @size_out contains the max size that's allowed to be written back out * to userspace. While the payload may have written more output than * this it will have to be ignored. */ - if (mbox_cmd.size_out) { - dev_WARN_ONCE(dev, mbox_cmd.size_out > *size_out, + if (mbox_cmd->size_out) { + dev_WARN_ONCE(dev, mbox_cmd->size_out > *size_out, "Invalid return size\n"); if (copy_to_user(u64_to_user_ptr(out_payload), - mbox_cmd.payload_out, mbox_cmd.size_out)) { - rc = -EFAULT; - goto out; + mbox_cmd->payload_out, mbox_cmd->size_out)) { + return -EFAULT; } } - *size_out = mbox_cmd.size_out; - *retval = mbox_cmd.return_code; + *size_out = mbox_cmd->size_out; + *retval = mbox_cmd->return_code; -out: - kvfree(mbox_cmd.payload_in); - kvfree(mbox_cmd.payload_out); - return rc; + return 0; } int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s) @@ -506,9 +494,11 @@ int cxl_send_cmd(struct cxl_memdev *cxlmd, struct cxl_send_command __user *s) if (rc) return rc; - rc = handle_mailbox_cmd_from_user(cxlds, &c, send.in.payload, - send.out.payload, &send.out.size, - &send.retval); + rc = handle_mailbox_cmd_from_user(cxlds, &mbox_cmd, send.out.payload, + &send.out.size, &send.retval); + + kvfree(mbox_cmd.payload_in); + kvfree(mbox_cmd.payload_out); if (rc) return rc;