Re: [PATCH v11 1/4] tpm: Remove all uses of drvdata from the TPM Core

From: Jarkko Sakkinen
Date: Tue Apr 19 2016 - 13:21:26 EST

Next message: Mason: "Re: [PATCH] clocksource/drivers/tango-xtal: Fix incorrect test"
Previous message: Steven Rostedt: "Re: [RFC][PATCH 2/4] tracing: Use pid bitmap instead of a pid array for set_event_pid"
In reply to: Jarkko Sakkinen: "Re: [PATCH v11 1/4] tpm: Remove all uses of drvdata from the TPM Core"
Next in thread: Jarkko Sakkinen: "Re: [PATCH v11 1/4] tpm: Remove all uses of drvdata from the TPM Core"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Tue, Apr 19, 2016 at 06:36:22AM -0400, Stefan Berger wrote:
> 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@xxxxxxxxxxxxxxxxxxxx>
> >>
> >>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.

Right, ok sounds justified.

> Stefan
>
> >
> >/Jarkko
> >
> >>Signed-off-by: Jason Gunthorpe <jgunthorpe@xxxxxxxxxxxxxxxxxxxx>
> >>Signed-off-by: Stefan Berger <stefanb@xxxxxxxxxxxxxxxxxx>
> >>---
> >> 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 message: Mason: "Re: [PATCH] clocksource/drivers/tango-xtal: Fix incorrect test"
Previous message: Steven Rostedt: "Re: [RFC][PATCH 2/4] tracing: Use pid bitmap instead of a pid array for set_event_pid"
In reply to: Jarkko Sakkinen: "Re: [PATCH v11 1/4] tpm: Remove all uses of drvdata from the TPM Core"
Next in thread: Jarkko Sakkinen: "Re: [PATCH v11 1/4] tpm: Remove all uses of drvdata from the TPM Core"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]