Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753845AbbLJAYs (ORCPT ); Wed, 9 Dec 2015 19:24:48 -0500 Received: from g9t5009.houston.hp.com ([15.240.92.67]:59917 "EHLO g9t5009.houston.hp.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750849AbbLJAYq (ORCPT ); Wed, 9 Dec 2015 19:24:46 -0500 Date: Wed, 9 Dec 2015 17:24:42 -0700 From: Jerry Hoemann To: Dan Williams Cc: Ross Zwisler , "Rafael J. Wysocki" , Len Brown , "Elliott, Robert (Persistent Memory)" , jmoyer , Dmitry Krivenok , Linda Knippers , "linux-nvdimm@lists.01.org" , Linux ACPI , "linux-kernel@vger.kernel.org" Subject: Re: [PATCH v3 3/3] nvdimm: Add IOCTL pass thru functions Message-ID: <20151210002442.GA104756@tevye.fc.hp.com> Reply-To: Jerry.Hoemann@hpe.com References: MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5008 Lines: 130 On Tue, Dec 08, 2015 at 06:10:20PM -0800, Dan Williams wrote: > On Wed, Dec 2, 2015 at 1:05 PM, Jerry Hoemann 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 > > --- > > 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-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/