Re: [PATCH v3 0/9] Do not allow set-partition immediate mode

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue, 29 Mar 2022 18:24:50 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Fri, Mar 25, 2022 at 3:34 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Wed, 23 Mar 2022 18:11:17 -0700
> > alison.schofield@intel.com wrote:
> >  
> > > From: Alison Schofield <alison.schofield@intel.com>
> > >
> > > Blocking immediate mode in set-partition info triggered a
> > > refactoring of the send path of userspace mailbox commands.
> > >
> > > The v1 to address the issue was a single patch [1] that inserted
> > > a new immediate mode check in the send path where the payload was
> > > available for examining. That was not in a validation function.
> > >
> > > The v2 patchset [2] added refactoring of the send path so that
> > > validation work can all spawn from cxl_validate_cmd_from_user().
> > >
> > > Here, v3 offers a finer level of refactoring:
> > >
> > > Patches 1-7: Refactor existing code so that all validation work
> > >       can spawn from cxl_validate_cmd_from_user().
> > >
> > >       The movement intends to cleanly rip the work of building a
> > >       mailbox command away from handle_mailbox_command_from_user()
> > >       and give it to cxl_validate_cmd_from_user().  
> >
> > This makes me wonder a bit if
> > cxl_validate_cmd_from_user() is an appropriate name given
> > it now does more than validation?  
> 
> Yeah, perhaps as a follow on, I think there's renaming possible to
> make it clear what the difference is between:
> 
> cxl_send_cmd
> cxl_mbox_send_cmd
> cxl_pci_mbox_send
> cxl_validate_cmd_from_user
> handle_mailbox_cmd_from_user
> 
> ---
> 
> cxl_send_cmd is really just a helper for the ioctl path so perhaps:
> s/cxl_query_cmd/cxl_ioctl_query/
> s/cxl_send_cmd/cxl_ioctl_send/
> 
> cxl_mbox_send_cmd() is the path that kernel-internal users take that
> don't need to do any copy from user work to pass buffers to
> cxl_pci_mbox_send() (which needs kernel buffers from
> memcpy_{to,from}io()), so perhaps:
> s/cxl_mbox_send_cmd/cxl_internal_cmd_send/
> 
> cxl_pci_mbox_send seems ok if the other function names change.
> 
> cxl_validate_cmd_from_user and handle_mailbox_cmd_from_user are really
> doing three distinct operations. Check the payload for correctness,
> check the command against the submit policy, and construct a kernel
> command buffer from the user buffer. So how about:
> 
> cxl_ioctl_send_checks() (inspired by submit_bio_checks())
> cxl_ioctl_send_to_kbuf()
> 
> ...whatever we pick I think it's ok if this is done after this
> reorganization completes with the current names.

Agreed. Keep this series clean, but then circle back to cleanup
the naming is a good approach.

Jonathan