From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mgwym02.jp.fujitsu.com (mgwym02.jp.fujitsu.com [211.128.242.41]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ml01.01.org (Postfix) with ESMTPS id E30C221E70D32 for ; Tue, 29 Aug 2017 17:25:41 -0700 (PDT) Received: from m3050.s.css.fujitsu.com (msm.b.css.fujitsu.com [10.134.21.208]) by yt-mxoi2.gw.nic.fujitsu.com (Postfix) with ESMTP id 5B723AC00E9 for ; Wed, 30 Aug 2017 09:28:17 +0900 (JST) Date: Wed, 30 Aug 2017 09:23:40 +0900 From: Yasunori Goto Subject: Re: [PATCH] libnvdimm: clean up command definitions In-Reply-To: References: <20170829190859.F14D.E1E9C6FF@jp.fujitsu.com> Message-Id: <20170830092318.30E3.E1E9C6FF@jp.fujitsu.com> MIME-Version: 1.0 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Errors-To: linux-nvdimm-bounces@lists.01.org Sender: "Linux-nvdimm" To: Dan Williams Cc: "linux-kernel@vger.kernel.org" , "linux-nvdimm@lists.01.org" List-ID: > On Tue, Aug 29, 2017 at 3:09 AM, Yasunori Goto wrote: > >> On Mon, Aug 28, 2017 at 6:03 PM, Yasunori Goto wrote: > >> >> On Mon, Aug 28, 2017 at 1:50 PM, Jerry Hoemann wrote: > >> >> > > >> >> > On Mon, Aug 28, 2017 at 08:45:32AM -0700, Dan Williams wrote: > >> >> >> Remove the command payloads that do not have an associated libnvdimm > >> >> >> ioctl. I.e. remove the payloads that would only ever be carried in the > >> >> >> ND_CMD_CALL envelope. This prevents userspace from growing unnecessary > >> >> >> dependencies on this kernel header when userspace already has everything > >> >> >> it needs to craft and send these commands. > >> >> > > >> >> > Userspace needs to include linux/ndctl.h to make the call as > >> >> > that is where nd_cmd_pkg is defined. > >> >> > > >> >> > So you want to have some structures defined in ndctl.h and other > >> >> > defined in the to be created libndctl-nfit.h? Plus a third header > >> >> > file for the HPE non-root calls? > >> >> > >> >> Yes. ndctl.h exports the ioctl command payloads, everything that goes > >> >> inside of ND_CMD_CALL is defined by userspace headers. The > >> >> libndctl-nfit.h header is proposed as a place to land vendor agnostic > >> >> NFIT-defined payloads, and any vendor specific definitions would > >> >> remain internal to libndctl as they are today. > >> >> > >> >> > Will libndctl-nfit.h be generally available and installed? > >> >> > >> >> Yes, that's the plan. > >> >> > >> >> > Will it be clean so that other applications can use it to get these > >> >> > definitions? Or will it be loaded w/ a bunch of stuff only useful > >> >> > to your ndctl command? > >> >> > >> >> Yes, that's the plan. It's a bug if libndctl-nfit.h is not generically > >> >> clean for issuing the NFIT root device commands via some ND_CMD_CALL > >> >> helpers from the base libndctl library. > >> >> > >> >> In other words libndctl-nfit.h defines the payload and libndctl > >> >> defines some general helpers for issuing commands. > >> > > >> > Maybe I don't understand your idea yet, let me confirm it. > >> > > >> > Certainly, current acpi driver does not need these definitions. > >> > But, I think nfit_test.ko will need them to emulate these features. > >> > > >> > Do you intend that libndctl-nfit.h should be defined at "include/uapi/linux/" > >> > directory? > >> > Otherwise, it should be defined at "tools/testing/nvdimm/" or > >> > "tools/testing/nvdimm/test" ? > >> > >> nfit_test will need its own internal / private copy of these payloads > >> in tools/testing/nvdimm/test so it can emulate how the bios behaves. > >> The include/uapi/linux directory is for user to kernel interface > >> definitions and these command payloads are purely an interface to bios > >> / firmware. > >> > > > > Hmm, > > > > I'm trying to make libndctl-nfit.h under tools/testing/nvdimm/nfit/, > > and trying to move NFIT_CMD_TRANSLATE_SPA from drivers/acpi/nfit/core.c to it. > > > > But I noticed that drivers/acpi/nfit/core.c still uses the definition > > in acpi_nfit_init_dsms(). > > > > ---- > > static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) > > { > > struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc; > > const guid_t *guid = to_nfit_uuid(NFIT_DEV_BUS); > > > > : > > > > dsm_mask = > > (1 << ND_CMD_ARS_CAP) | > > (1 << ND_CMD_ARS_START) | > > (1 << ND_CMD_ARS_STATUS) | > > (1 << ND_CMD_CLEAR_ERROR) | > > (1 << NFIT_CMD_TRANSLATE_SPA) | > > (1 << NFIT_CMD_ARS_INJECT_SET) | > > (1 << NFIT_CMD_ARS_INJECT_CLEAR) | > > (1 << NFIT_CMD_ARS_INJECT_GET); > > for_each_set_bit(i, &dsm_mask, BITS_PER_LONG) > > if (acpi_check_dsm(adev->handle, guid, 1, 1ULL << i)) > > set_bit(i, &nd_desc->bus_dsm_mask); > > --- > > > > Though it seems to be necessary yet, > > do you have any idea to remove it? > > Here's the end state I'm looking for... > > 1/ drivers/acpi/nfit/core.c keeps its current definitions of the > NFIT_CMD_ numbers since it needs to validate the DSM command number > mask > > 2/ tools/testing/nvdimm/test/nfit.h grows private definitions of only > the command payloads that it emulates > > 3/ ndctl in userspace grows a new ndctl/libndctl-nfit.h with all the > payloads defined in ACPI. Ideally we can deprecate all the payload > definitions in ndctl.h and only use the ND_CMD_CALL ioctl plus the > ndctl/libndctl-nfit.h definitions. Although, since label reading has > moved away from DSMs we'll always need the separate > ND_CMD_{GET,SET}_CONFIG_DATA ioctls. The libndctl-nfit.h header should > be listed in pkginclude_HEADERS. Hmm, Ok. I'll try. Thanks for your explanation. _______________________________________________ Linux-nvdimm mailing list Linux-nvdimm@lists.01.org https://lists.01.org/mailman/listinfo/linux-nvdimm From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751730AbdH3A21 (ORCPT ); Tue, 29 Aug 2017 20:28:27 -0400 Received: from mgwkm03.jp.fujitsu.com ([202.219.69.170]:55402 "EHLO mgwkm03.jp.fujitsu.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751240AbdH3A20 (ORCPT ); Tue, 29 Aug 2017 20:28:26 -0400 X-SecurityPolicyCheck: OK by SHieldMailChecker v2.5.2 X-SHieldMailCheckerPolicyVersion: FJ-ISEC-20170217-enc X-SHieldMailCheckerMailID: 02ec5f9edaef4792aa2b3ea02efbcb12 Date: Wed, 30 Aug 2017 09:23:40 +0900 From: Yasunori Goto To: Dan Williams Subject: Re: [PATCH] libnvdimm: clean up command definitions Cc: Jerry Hoemann , "linux-nvdimm@lists.01.org" , "linux-kernel@vger.kernel.org" In-Reply-To: References: <20170829190859.F14D.E1E9C6FF@jp.fujitsu.com> Message-Id: <20170830092318.30E3.E1E9C6FF@jp.fujitsu.com> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" Content-Transfer-Encoding: 7bit X-Mailer: Becky! ver. 2.73 [ja] X-TM-AS-MML: disable Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org > On Tue, Aug 29, 2017 at 3:09 AM, Yasunori Goto wrote: > >> On Mon, Aug 28, 2017 at 6:03 PM, Yasunori Goto wrote: > >> >> On Mon, Aug 28, 2017 at 1:50 PM, Jerry Hoemann wrote: > >> >> > > >> >> > On Mon, Aug 28, 2017 at 08:45:32AM -0700, Dan Williams wrote: > >> >> >> Remove the command payloads that do not have an associated libnvdimm > >> >> >> ioctl. I.e. remove the payloads that would only ever be carried in the > >> >> >> ND_CMD_CALL envelope. This prevents userspace from growing unnecessary > >> >> >> dependencies on this kernel header when userspace already has everything > >> >> >> it needs to craft and send these commands. > >> >> > > >> >> > Userspace needs to include linux/ndctl.h to make the call as > >> >> > that is where nd_cmd_pkg is defined. > >> >> > > >> >> > So you want to have some structures defined in ndctl.h and other > >> >> > defined in the to be created libndctl-nfit.h? Plus a third header > >> >> > file for the HPE non-root calls? > >> >> > >> >> Yes. ndctl.h exports the ioctl command payloads, everything that goes > >> >> inside of ND_CMD_CALL is defined by userspace headers. The > >> >> libndctl-nfit.h header is proposed as a place to land vendor agnostic > >> >> NFIT-defined payloads, and any vendor specific definitions would > >> >> remain internal to libndctl as they are today. > >> >> > >> >> > Will libndctl-nfit.h be generally available and installed? > >> >> > >> >> Yes, that's the plan. > >> >> > >> >> > Will it be clean so that other applications can use it to get these > >> >> > definitions? Or will it be loaded w/ a bunch of stuff only useful > >> >> > to your ndctl command? > >> >> > >> >> Yes, that's the plan. It's a bug if libndctl-nfit.h is not generically > >> >> clean for issuing the NFIT root device commands via some ND_CMD_CALL > >> >> helpers from the base libndctl library. > >> >> > >> >> In other words libndctl-nfit.h defines the payload and libndctl > >> >> defines some general helpers for issuing commands. > >> > > >> > Maybe I don't understand your idea yet, let me confirm it. > >> > > >> > Certainly, current acpi driver does not need these definitions. > >> > But, I think nfit_test.ko will need them to emulate these features. > >> > > >> > Do you intend that libndctl-nfit.h should be defined at "include/uapi/linux/" > >> > directory? > >> > Otherwise, it should be defined at "tools/testing/nvdimm/" or > >> > "tools/testing/nvdimm/test" ? > >> > >> nfit_test will need its own internal / private copy of these payloads > >> in tools/testing/nvdimm/test so it can emulate how the bios behaves. > >> The include/uapi/linux directory is for user to kernel interface > >> definitions and these command payloads are purely an interface to bios > >> / firmware. > >> > > > > Hmm, > > > > I'm trying to make libndctl-nfit.h under tools/testing/nvdimm/nfit/, > > and trying to move NFIT_CMD_TRANSLATE_SPA from drivers/acpi/nfit/core.c to it. > > > > But I noticed that drivers/acpi/nfit/core.c still uses the definition > > in acpi_nfit_init_dsms(). > > > > ---- > > static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) > > { > > struct nvdimm_bus_descriptor *nd_desc = &acpi_desc->nd_desc; > > const guid_t *guid = to_nfit_uuid(NFIT_DEV_BUS); > > > > : > > > > dsm_mask = > > (1 << ND_CMD_ARS_CAP) | > > (1 << ND_CMD_ARS_START) | > > (1 << ND_CMD_ARS_STATUS) | > > (1 << ND_CMD_CLEAR_ERROR) | > > (1 << NFIT_CMD_TRANSLATE_SPA) | > > (1 << NFIT_CMD_ARS_INJECT_SET) | > > (1 << NFIT_CMD_ARS_INJECT_CLEAR) | > > (1 << NFIT_CMD_ARS_INJECT_GET); > > for_each_set_bit(i, &dsm_mask, BITS_PER_LONG) > > if (acpi_check_dsm(adev->handle, guid, 1, 1ULL << i)) > > set_bit(i, &nd_desc->bus_dsm_mask); > > --- > > > > Though it seems to be necessary yet, > > do you have any idea to remove it? > > Here's the end state I'm looking for... > > 1/ drivers/acpi/nfit/core.c keeps its current definitions of the > NFIT_CMD_ numbers since it needs to validate the DSM command number > mask > > 2/ tools/testing/nvdimm/test/nfit.h grows private definitions of only > the command payloads that it emulates > > 3/ ndctl in userspace grows a new ndctl/libndctl-nfit.h with all the > payloads defined in ACPI. Ideally we can deprecate all the payload > definitions in ndctl.h and only use the ND_CMD_CALL ioctl plus the > ndctl/libndctl-nfit.h definitions. Although, since label reading has > moved away from DSMs we'll always need the separate > ND_CMD_{GET,SET}_CONFIG_DATA ioctls. The libndctl-nfit.h header should > be listed in pkginclude_HEADERS. Hmm, Ok. I'll try. Thanks for your explanation.