Re: [PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions

[Jerry Hoemann <jerry.hoemann@hpe.com>]

On Tue, Dec 08, 2015 at 06:10:20PM -0800, Dan Williams wrote:
> On Wed, Dec 2, 2015 at 1:05 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote:
> > Add ioctl command ND_CMD_CALL_DSM to acpi_nfit_ctl and __nd_ioctl which
> > allow kernel to call a nvdimm's _DSM as a passthru without using the
> > marshaling code of the nd_cmd_desc.
> >
> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com>
> > ---
> >  drivers/acpi/nfit.c  | 109 ++++++++++++++++++++++++++++++++-------------------
> >  drivers/nvdimm/bus.c |  61 +++++++++++++++++++++-------
> >  2 files changed, 115 insertions(+), 55 deletions(-)
> >
> 
> In general I'd like to see this patch remove the need to sprinkle "if
> (dsm_call)" throughout the implementation ... specific examples below:


 The current code is exporting a very different interface
 for calling _DSM from the pass thru I'm proposing.

 The current code explicitly knows the calling structure of each _DSM function.
 It knows the number and size of each input and output field.  For variable
 size output functions the current kernel code knows which field describes
 the size of the output.  The current code knows the dsm_mask for the _DSM.

 For the pass thru that I'm proposing the kernel wouldn't need to know any
 of this.  The information needed to make the _DSM call would be passed in by
 the caller.  [ Yes, there are cases where some calls are made from within
 the kernel.  But it would be those callers who use the data that needs to
 know about the data. ]

 So the use of dsm_call marks where there is a fundamental difference
 in the two approaches.  This is one reason why I didn't try to integrate
 these functions in my original submittal.

 You previously expressed an interest in converting the user application
 to use a pass thru mode and deprecate the current ioctl.  Is this something
 you're still interested in doing?

 If yes, the work we need to do to integrate these two different approaches
 will just need to be undone.  Further if we deprecate the current IOCTLs,
 then the nd_cmd_desc tables and related nd_cmd_in_size and nd_cmd_out_size
 could then be removed.



> 
> > diff --git a/drivers/acpi/nfit.c b/drivers/acpi/nfit.c
> > index c1b8d03..e509145 100644
> > --- a/drivers/acpi/nfit.c
> > +++ b/drivers/acpi/nfit.c
> > @@ -75,7 +75,11 @@ static int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc,

 ...

> > +       __u64 rev = 1, func = cmd;
> 
> Why __u64 and not int for these?  acpi_evaluate_dsm() takes an int for
> both so a bigger type here will be truncated down.


ACPI defines arguments to a _DSM as 64 bit quantities. We want the interface
exported to the user to follow the ACPI spec.  The variables above collect
the value of rev or func from the two different sources (wrapper or legacy)
and then passes to acpi_evaluate_dsm which defines the parameters as simple
ints.

So, going from user interface to call of acpi_evaluate_dsm there will be
a truncation somewhere.

Looking at acpi_evaluate_dsm(), it uses union acpi_object and fills in
.integer.value for both rev and func.  These are defined as u64.

So patching acpi_evaluate_dsm to make the rev and func parameters u64 might
be do'able, but we'd still have potential sign issues with other callers
to acpi_evaluate_dsm which look to be using simple ints in the call.

Do you want me to look at patching acpi_evaluate_dsm (and possibly
its callers) as part of this patch set?


...



> >
> >         /* fail write commands (when read-only) */
> >         if (read_only)
> > -               switch (ioctl_cmd) {
> > -               case ND_IOCTL_VENDOR:
> > -               case ND_IOCTL_SET_CONFIG_DATA:
> > -               case ND_IOCTL_ARS_START:
> > +               switch (cmd) {
> > +               case ND_CMD_VENDOR:
> > +               case ND_CMD_SET_CONFIG_DATA:
> > +               case ND_CMD_ARS_START:
> > +               case ND_CMD_CALL_DSM:
> 
> I agree with your comment in the cover letter that this change should
> be a separate patch.

  Will do.

> 
> It bothers me that we'll block all ND_CMD_CALL_DSM in the read_only
> case.  Let's leave ND_CMD_CALL_DSM out of this selection for now.

  Not thrilled here either, but it is the conservative approach for
  the kernel.

  Since ND_CMD_CALL_DSM is a pass thru,  the kernel
  doesn't have the knowledge whether the call being made
  is "read only" or not.  Having ND_CMD_CALL_DSM in
  the switch doesn't prevent the user from making such
  calls, it only requires s/he opens the device for write.


-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

--