From: Stefan Berger <stefanb@linux.vnet.ibm.com>
To: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Cc: tpmdd-devel@lists.sourceforge.net,
jgunthorpe@obsidianresearch.com,
linux-security-module@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH v11 1/4] tpm: Remove all uses of drvdata from the TPM Core
Date: Tue, 19 Apr 2016 06:36:22 -0400 [thread overview]
Message-ID: <57160A26.5040805@linux.vnet.ibm.com> (raw)
In-Reply-To: <20160419101214.GA11574@intel.com>
On 04/19/2016 06:12 AM, Jarkko Sakkinen wrote:
> On Mon, Apr 18, 2016 at 01:26:13PM -0400, Stefan Berger wrote:
>> From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>>
>> 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.
> Can you give me a quick recap why this patch was mandatory to make the
> patch set work? Which regression in the earlier versions of the patch
> set this fixes?
The below patch removes usage of dev_get_drvdata() for retrieving the
chip. With Christophe's series dropping the priv field I now can use the
drvdata to store proxy_dev rather than re-introducing the priv field in
the chip structure. Besides that it doesn't seem necessary to use the
drvdata field to get from the chip to the device if a simple
container_of can do it.
Stefan
>
> /Jarkko
>
>> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
>> Signed-off-by: Stefan Berger <stefanb@linux.vnet.ibm.com>
>> ---
>> 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 5bc530c..7e2c9cf 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);
>> @@ -276,14 +274,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));
>>
>> @@ -297,10 +291,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.
>> @@ -323,24 +357,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);
>>
>> @@ -362,8 +392,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 7cba092..080dade 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 a7c3473..b46cf70 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 8bc6fb8..508e8e0 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);
>>
>> --
>> 2.4.3
>>
next prev parent reply other threads:[~2016-04-19 10:36 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-04-18 17:26 [PATCH v11 0/4] Multi-instance vTPM proxy driver Stefan Berger
2016-04-18 17:26 ` [PATCH v11 1/4] tpm: Remove all uses of drvdata from the TPM Core Stefan Berger
2016-04-19 10:12 ` Jarkko Sakkinen
2016-04-19 10:36 ` Stefan Berger [this message]
2016-04-19 17:06 ` Jason Gunthorpe
2016-04-20 5:33 ` Jarkko Sakkinen
2016-04-19 17:21 ` Jarkko Sakkinen
2016-04-22 15:06 ` Jarkko Sakkinen
2016-04-25 19:13 ` Stefan Berger
2016-05-10 13:55 ` [BUG, bisect] " Jeremiah Mahler
2016-05-10 14:38 ` Jarkko Sakkinen
[not found] ` <201605101505.u4AF5nHd032349@d03av04.boulder.ibm.com>
2016-05-11 1:26 ` [tpmdd-devel] " Jeremiah Mahler
[not found] ` <201605111027.u4BARtQ4021061@d01av01.pok.ibm.com>
2016-05-11 14:29 ` Jeremiah Mahler
2016-05-11 15:20 ` Stefan Berger
2016-04-18 17:26 ` [PATCH v11 2/4] tpm: Introduce TPM_CHIP_FLAG_VIRTUAL Stefan Berger
2016-04-22 16:46 ` Jarkko Sakkinen
2016-04-18 17:26 ` [PATCH v11 3/4] tpm: Proxy driver for supporting multiple emulated TPMs Stefan Berger
2016-04-22 16:47 ` Jarkko Sakkinen
2016-04-18 17:26 ` [PATCH v11 4/4] tpm: Add documentation for the tpm_vtpm_proxy device driver Stefan Berger
2016-04-22 16:47 ` Jarkko Sakkinen
2016-04-22 16:54 ` [PATCH v11 0/4] Multi-instance vTPM proxy driver Jarkko Sakkinen
2016-04-26 9:28 ` [tpmdd-devel] " Jarkko Sakkinen
2016-04-26 11:30 ` Stefan Berger
2016-04-27 12:56 ` Jarkko Sakkinen
2016-04-27 16:27 ` Stefan Berger
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=57160A26.5040805@linux.vnet.ibm.com \
--to=stefanb@linux.vnet.ibm.com \
--cc=jarkko.sakkinen@linux.intel.com \
--cc=jgunthorpe@obsidianresearch.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=tpmdd-devel@lists.sourceforge.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome