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@xxxxxxx> 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@xxxxxxx>
> > ---
> > 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
-----------------------------------------------------------------------------
--
--
To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
