From patchwork Tue Apr 12 17:54:40 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jason Gunthorpe X-Patchwork-Id: 609518 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from lists.sourceforge.net (lists.sourceforge.net [216.34.181.88]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 3qkvj441b7z9t3n for ; Wed, 13 Apr 2016 03:55:04 +1000 (AEST) Received: from localhost ([127.0.0.1] helo=sfs-ml-3.v29.ch3.sourceforge.com) by sfs-ml-3.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1aq2Wf-0007t4-Mc; Tue, 12 Apr 2016 17:54:57 +0000 Received: from sog-mx-4.v43.ch3.sourceforge.com ([172.29.43.194] helo=mx.sourceforge.net) by sfs-ml-3.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1aq2We-0007sw-M1 for tpmdd-devel@lists.sourceforge.net; Tue, 12 Apr 2016 17:54:56 +0000 Received-SPF: pass (sog-mx-4.v43.ch3.sourceforge.com: domain of obsidianresearch.com designates 184.70.90.242 as permitted sender) client-ip=184.70.90.242; envelope-from=jgunthorpe@obsidianresearch.com; helo=quartz.orcorp.ca; Received: from quartz.orcorp.ca ([184.70.90.242]) by sog-mx-4.v43.ch3.sourceforge.com with esmtps (TLSv1:AES128-SHA:128) (Exim 4.76) id 1aq2Wc-0002tU-Uj for tpmdd-devel@lists.sourceforge.net; Tue, 12 Apr 2016 17:54:56 +0000 Received: from [10.0.0.160] (helo=jggl.edm.orcorp.ca) by quartz.orcorp.ca with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84) (envelope-from ) id 1aq2WO-00033w-66; Tue, 12 Apr 2016 11:54:40 -0600 Received: from jgg by jggl.edm.orcorp.ca with local (Exim 4.84) (envelope-from ) id 1aq2WO-0003tU-2R; Tue, 12 Apr 2016 11:54:40 -0600 Date: Tue, 12 Apr 2016 11:54:40 -0600 From: Jason Gunthorpe To: Stefan Berger Message-ID: <20160412175439.GB5759@obsidianresearch.com> References: <57068198.6050407@linux.vnet.ibm.com> <20160411084358.GB11322@intel.com> <20160411181403.GB371@obsidianresearch.com> <201604111933.u3BJXErj001305@d03av03.boulder.ibm.com> <20160411202811.GA3663@obsidianresearch.com> <201604112030.u3BKUeJQ017181@d01av04.pok.ibm.com> <20160411205718.GC3663@obsidianresearch.com> <201604112213.u3BMD3i5028908@d03av02.boulder.ibm.com> <20160412000500.GC5861@obsidianresearch.com> <201604121512.u3CFCULH013445@d01av05.pok.ibm.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <201604121512.u3CFCULH013445@d01av05.pok.ibm.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-Broken-Reverse-DNS: no host name found for IP address 10.0.0.160 X-Spam-Score: -1.4 (-) X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. -1.5 SPF_CHECK_PASS SPF reports sender host as permitted sender for sender-domain -0.0 SPF_HELO_PASS SPF: HELO matches SPF record -0.0 SPF_PASS SPF: sender matches SPF record -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature 0.2 AWL AWL: Adjusted score from AWL reputation of From: address X-Headers-End: 1aq2Wc-0002tU-Uj Cc: tpmdd-devel@lists.sourceforge.net, Christophe Ricard Subject: Re: [tpmdd-devel] [PATCH v9 2/4] tpm: Proxy driver for supporting multiple emulated TPMs X-BeenThere: tpmdd-devel@lists.sourceforge.net X-Mailman-Version: 2.1.9 Precedence: list List-Id: Tpm Device Driver maintainance List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: tpmdd-devel-bounces@lists.sourceforge.net On Tue, Apr 12, 2016 at 11:18:20AM -0400, Stefan Berger wrote: > > Stick it before the TPM_CHIP_FLAG_VIRTUAL patch and throw away most of > > that patch. Just add two flag checks into tpm_add/del_legacy_sysfs() > Here's the v10 series now. If you want a better text for that patch, > please let me know. Sure. I noticed a few other things, full tidied patch below, thanks for taking this on. > I had to extend it to tpm-interface.c > [2]https://github.com/stefanberger/linux/commits/vtpm-driver.v10 Yes, that looks right, thanks > The patches pass my test suite and tpm tis seems to also work fine. > [3]https://github.com/stefanberger/linux-vtpm-tests > > + rc = __compat_only_sysfs_link_entry_to_kobj( > > + &chip->dev.parent->kobj, &chip->dev.kobj, "ppi"); > > + if (rc && rc != -ENOENT) > > + goto out_err; > I wonder whether it is necessary to goto out_err or not just return rc > here? Correct Good catch on the tpm_pm_ functions too > Also, for tpm0 I now find the following two pcrs entries, as well as > all the rest of course: > /sys/devices/pnp0/00:05/tpm/tpm0/pcrs > /sys/devices/pnp0/00:05/pcrs > Is this intentionately? Yes, the expectation is that the legacy path '/sys/devices/pnp0/00:05/pcrs' is a symlink to the new path '/sys/devices/pnp0/00:05/tpm/tpm0/pcrs' I assume you saw that all the symlinks were working? The way the tpm core created attributes on the parent device was never correct, so this finally fixes it. Someday we can drop the legacy symlinks. BTW, I just noticed this needs a fix: +#define VTPM_PROXY_IOC_NEW_DEV _IOW(0xa1, 0x00, struct vtpm_proxy_new_dev) Should be _IOWR since the struct is bidirectional >From b09dd2f8d2c5242102649cd0473f161919af87fd Mon Sep 17 00:00:00 2001 From: Jason Gunthorpe Date: Tue, 12 Apr 2016 11:48:21 -0600 Subject: [PATCH] tpm: Remove all uses of drvdata from the TPM Core The final thing preventing this was the way the sysfs files were attached to the pdev. Follow the approach developed for ppi and move the sysfs files to the chip->dev with symlinks from the pdev for compatibility. Everything in the core now sanely uses container_of to get the chip. Signed-off-by: Jason Gunthorpe Signed-off-by: Stefan Berger --- drivers/char/tpm/tpm-chip.c | 73 ++++++++++++++++++++++++++++------------ drivers/char/tpm/tpm-interface.c | 7 ++-- drivers/char/tpm/tpm-sysfs.c | 61 ++++++++++++++------------------- drivers/char/tpm/tpm.h | 10 +++--- 4 files changed, 84 insertions(+), 67 deletions(-) diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index f62c8518cd68..018a898de5b0 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -170,9 +170,7 @@ struct tpm_chip *tpm_chip_alloc(struct device *dev, chip->dev.class = tpm_class; chip->dev.release = tpm_dev_release; chip->dev.parent = dev; -#ifdef CONFIG_ACPI chip->dev.groups = chip->groups; -#endif if (chip->dev_num == 0) chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR); @@ -275,14 +273,10 @@ static void tpm_del_char_device(struct tpm_chip *chip) static int tpm1_chip_register(struct tpm_chip *chip) { - int rc; - if (chip->flags & TPM_CHIP_FLAG_TPM2) return 0; - rc = tpm_sysfs_add_device(chip); - if (rc) - return rc; + tpm_sysfs_add_device(chip); chip->bios_dir = tpm_bios_log_setup(dev_name(&chip->dev)); @@ -296,10 +290,50 @@ static void tpm1_chip_unregister(struct tpm_chip *chip) if (chip->bios_dir) tpm_bios_log_teardown(chip->bios_dir); +} + +static void tpm_del_legacy_sysfs(struct tpm_chip *chip) +{ + struct attribute **i; + + if (chip->flags & TPM_CHIP_FLAG_TPM2) + return; - tpm_sysfs_del_device(chip); + sysfs_remove_link(&chip->dev.parent->kobj, "ppi"); + + for (i = chip->groups[0]->attrs; *i != NULL; ++i) + sysfs_remove_link(&chip->dev.parent->kobj, (*i)->name); } +/* For compatibility with legacy sysfs paths we provide symlinks from the + * parent dev directory to selected names within the tpm chip directory. Old + * kernel versions created these files directly under the parent. + */ +static int tpm_add_legacy_sysfs(struct tpm_chip *chip) +{ + struct attribute **i; + int rc; + + if (chip->flags & TPM_CHIP_FLAG_TPM2) + return 0; + + rc = __compat_only_sysfs_link_entry_to_kobj( + &chip->dev.parent->kobj, &chip->dev.kobj, "ppi"); + if (rc && rc != -ENOENT) + return rc; + + /* All the names from tpm-sysfs */ + for (i = chip->groups[0]->attrs; *i != NULL; ++i) { + rc = __compat_only_sysfs_link_entry_to_kobj( + &chip->dev.parent->kobj, &chip->dev.kobj, (*i)->name); + if (rc) { + tpm_del_legacy_sysfs(chip); + return rc; + } + } + + return 0; +} /* * tpm_chip_register() - create a character device for the TPM chip * @chip: TPM chip to use. @@ -322,24 +356,20 @@ int tpm_chip_register(struct tpm_chip *chip) tpm_add_ppi(chip); rc = tpm_add_char_device(chip); - if (rc) - goto out_err; + if (rc) { + tpm1_chip_unregister(chip); + return rc; + } chip->flags |= TPM_CHIP_FLAG_REGISTERED; - if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) { - rc = __compat_only_sysfs_link_entry_to_kobj( - &chip->dev.parent->kobj, &chip->dev.kobj, "ppi"); - if (rc && rc != -ENOENT) { - tpm_chip_unregister(chip); - return rc; - } + rc = tpm_add_legacy_sysfs(chip); + if (rc) { + tpm_chip_unregister(chip); + return rc; } return 0; -out_err: - tpm1_chip_unregister(chip); - return rc; } EXPORT_SYMBOL_GPL(tpm_chip_register); @@ -361,8 +391,7 @@ void tpm_chip_unregister(struct tpm_chip *chip) if (!(chip->flags & TPM_CHIP_FLAG_REGISTERED)) return; - if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) - sysfs_remove_link(&chip->dev.parent->kobj, "ppi"); + tpm_del_legacy_sysfs(chip); tpm1_chip_unregister(chip); tpm_del_char_device(chip); diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index 7cba092e3069..080dade5e80f 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -432,12 +432,11 @@ static const struct tpm_input_header tpm_getcap_header = { .ordinal = TPM_ORD_GET_CAP }; -ssize_t tpm_getcap(struct device *dev, __be32 subcap_id, cap_t *cap, +ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap, const char *desc) { struct tpm_cmd_t tpm_cmd; int rc; - struct tpm_chip *chip = dev_get_drvdata(dev); tpm_cmd.header.in = tpm_getcap_header; if (subcap_id == CAP_VERSION_1_1 || subcap_id == CAP_VERSION_1_2) { @@ -935,7 +934,7 @@ static struct tpm_input_header savestate_header = { */ int tpm_pm_suspend(struct device *dev) { - struct tpm_chip *chip = dev_get_drvdata(dev); + struct tpm_chip *chip = to_tpm_chip(dev); struct tpm_cmd_t cmd; int rc, try; @@ -996,7 +995,7 @@ EXPORT_SYMBOL_GPL(tpm_pm_suspend); */ int tpm_pm_resume(struct device *dev) { - struct tpm_chip *chip = dev_get_drvdata(dev); + struct tpm_chip *chip = to_tpm_chip(dev); if (chip == NULL) return -ENODEV; diff --git a/drivers/char/tpm/tpm-sysfs.c b/drivers/char/tpm/tpm-sysfs.c index a7c3473c3421..b46cf70c8b16 100644 --- a/drivers/char/tpm/tpm-sysfs.c +++ b/drivers/char/tpm/tpm-sysfs.c @@ -36,7 +36,7 @@ static ssize_t pubek_show(struct device *dev, struct device_attribute *attr, int i, rc; char *str = buf; - struct tpm_chip *chip = dev_get_drvdata(dev); + struct tpm_chip *chip = to_tpm_chip(dev); tpm_cmd.header.in = tpm_readpubek_header; err = tpm_transmit_cmd(chip, &tpm_cmd, READ_PUBEK_RESULT_SIZE, @@ -92,9 +92,9 @@ static ssize_t pcrs_show(struct device *dev, struct device_attribute *attr, ssize_t rc; int i, j, num_pcrs; char *str = buf; - struct tpm_chip *chip = dev_get_drvdata(dev); + struct tpm_chip *chip = to_tpm_chip(dev); - rc = tpm_getcap(dev, TPM_CAP_PROP_PCR, &cap, + rc = tpm_getcap(chip, TPM_CAP_PROP_PCR, &cap, "attempting to determine the number of PCRS"); if (rc) return 0; @@ -119,8 +119,8 @@ static ssize_t enabled_show(struct device *dev, struct device_attribute *attr, cap_t cap; ssize_t rc; - rc = tpm_getcap(dev, TPM_CAP_FLAG_PERM, &cap, - "attempting to determine the permanent enabled state"); + rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_PERM, &cap, + "attempting to determine the permanent enabled state"); if (rc) return 0; @@ -135,8 +135,8 @@ static ssize_t active_show(struct device *dev, struct device_attribute *attr, cap_t cap; ssize_t rc; - rc = tpm_getcap(dev, TPM_CAP_FLAG_PERM, &cap, - "attempting to determine the permanent active state"); + rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_PERM, &cap, + "attempting to determine the permanent active state"); if (rc) return 0; @@ -151,8 +151,8 @@ static ssize_t owned_show(struct device *dev, struct device_attribute *attr, cap_t cap; ssize_t rc; - rc = tpm_getcap(dev, TPM_CAP_PROP_OWNER, &cap, - "attempting to determine the owner state"); + rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_PROP_OWNER, &cap, + "attempting to determine the owner state"); if (rc) return 0; @@ -167,8 +167,8 @@ static ssize_t temp_deactivated_show(struct device *dev, cap_t cap; ssize_t rc; - rc = tpm_getcap(dev, TPM_CAP_FLAG_VOL, &cap, - "attempting to determine the temporary state"); + rc = tpm_getcap(to_tpm_chip(dev), TPM_CAP_FLAG_VOL, &cap, + "attempting to determine the temporary state"); if (rc) return 0; @@ -180,11 +180,12 @@ static DEVICE_ATTR_RO(temp_deactivated); static ssize_t caps_show(struct device *dev, struct device_attribute *attr, char *buf) { + struct tpm_chip *chip = to_tpm_chip(dev); cap_t cap; ssize_t rc; char *str = buf; - rc = tpm_getcap(dev, TPM_CAP_PROP_MANUFACTURER, &cap, + rc = tpm_getcap(chip, TPM_CAP_PROP_MANUFACTURER, &cap, "attempting to determine the manufacturer"); if (rc) return 0; @@ -192,8 +193,8 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr, be32_to_cpu(cap.manufacturer_id)); /* Try to get a TPM version 1.2 TPM_CAP_VERSION_INFO */ - rc = tpm_getcap(dev, CAP_VERSION_1_2, &cap, - "attempting to determine the 1.2 version"); + rc = tpm_getcap(chip, CAP_VERSION_1_2, &cap, + "attempting to determine the 1.2 version"); if (!rc) { str += sprintf(str, "TCG version: %d.%d\nFirmware version: %d.%d\n", @@ -203,7 +204,7 @@ static ssize_t caps_show(struct device *dev, struct device_attribute *attr, cap.tpm_version_1_2.revMinor); } else { /* Otherwise just use TPM_STRUCT_VER */ - rc = tpm_getcap(dev, CAP_VERSION_1_1, &cap, + rc = tpm_getcap(chip, CAP_VERSION_1_1, &cap, "attempting to determine the 1.1 version"); if (rc) return 0; @@ -222,7 +223,7 @@ static DEVICE_ATTR_RO(caps); static ssize_t cancel_store(struct device *dev, struct device_attribute *attr, const char *buf, size_t count) { - struct tpm_chip *chip = dev_get_drvdata(dev); + struct tpm_chip *chip = to_tpm_chip(dev); if (chip == NULL) return 0; @@ -234,7 +235,7 @@ static DEVICE_ATTR_WO(cancel); static ssize_t durations_show(struct device *dev, struct device_attribute *attr, char *buf) { - struct tpm_chip *chip = dev_get_drvdata(dev); + struct tpm_chip *chip = to_tpm_chip(dev); if (chip->duration[TPM_LONG] == 0) return 0; @@ -251,7 +252,7 @@ static DEVICE_ATTR_RO(durations); static ssize_t timeouts_show(struct device *dev, struct device_attribute *attr, char *buf) { - struct tpm_chip *chip = dev_get_drvdata(dev); + struct tpm_chip *chip = to_tpm_chip(dev); return sprintf(buf, "%d %d %d %d [%s]\n", jiffies_to_usecs(chip->timeout_a), @@ -281,24 +282,12 @@ static const struct attribute_group tpm_dev_group = { .attrs = tpm_dev_attrs, }; -int tpm_sysfs_add_device(struct tpm_chip *chip) +void tpm_sysfs_add_device(struct tpm_chip *chip) { - int err; - err = sysfs_create_group(&chip->dev.parent->kobj, - &tpm_dev_group); - - if (err) - dev_err(&chip->dev, - "failed to create sysfs attributes, %d\n", err); - return err; -} - -void tpm_sysfs_del_device(struct tpm_chip *chip) -{ - /* The sysfs routines rely on an implicit tpm_try_get_ops, this - * function is called before ops is null'd and the sysfs core - * synchronizes this removal so that no callbacks are running or can - * run again + /* The sysfs routines rely on an implicit tpm_try_get_ops, device_del + * is called before ops is null'd and the sysfs core synchronizes this + * removal so that no callbacks are running or can run again */ - sysfs_remove_group(&chip->dev.parent->kobj, &tpm_dev_group); + WARN_ON(chip->groups_cnt != 0); + chip->groups[chip->groups_cnt++] = &tpm_dev_group; } diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index 8bc6fb85fb38..508e8e00c9c1 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -168,9 +168,9 @@ struct tpm_chip { struct dentry **bios_dir; -#ifdef CONFIG_ACPI - const struct attribute_group *groups[2]; + const struct attribute_group *groups[3]; unsigned int groups_cnt; +#ifdef CONFIG_ACPI acpi_handle acpi_dev_handle; char ppi_version[TPM_PPI_VERSION_LEN + 1]; #endif /* CONFIG_ACPI */ @@ -471,7 +471,8 @@ extern dev_t tpm_devt; extern const struct file_operations tpm_fops; extern struct idr dev_nums_idr; -ssize_t tpm_getcap(struct device *, __be32, cap_t *, const char *); +ssize_t tpm_getcap(struct tpm_chip *chip, __be32 subcap_id, cap_t *cap, + const char *desc); ssize_t tpm_transmit(struct tpm_chip *chip, const char *buf, size_t bufsiz); ssize_t tpm_transmit_cmd(struct tpm_chip *chip, void *cmd, int len, @@ -496,8 +497,7 @@ extern struct tpm_chip *tpmm_chip_alloc(struct device *pdev, extern int tpm_chip_register(struct tpm_chip *chip); extern void tpm_chip_unregister(struct tpm_chip *chip); -int tpm_sysfs_add_device(struct tpm_chip *chip); -void tpm_sysfs_del_device(struct tpm_chip *chip); +void tpm_sysfs_add_device(struct tpm_chip *chip); int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);