[PATCH v2] tpm: enable PPI for TPM 2.0

[PATCH v2] tpm: enable PPI for TPM 2.0

[Jarkko Sakkinen]

Enabled PPI interface to the character device sysfs directory accessible
both for 1.x and 2.0 devices.

The ppi group is moved from the platform device directory to the
character device directory. In order to retain backwards compatibility
with the 1.x devices, a symlink is created to the platform device
directory.

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jason Gunthorpe <jason.gunthorpe@obsidianresearch.com>
---
 drivers/char/tpm/tpm-chip.c | 21 +++++++++++++--------
 drivers/char/tpm/tpm.h      | 17 ++++++-----------
 drivers/char/tpm/tpm_ppi.c  | 34 +++++++++++-----------------------
 fs/kernfs/dir.c             |  1 +
 fs/sysfs/group.c            | 42 ++++++++++++++++++++++++++++++++++++++++++
 include/linux/sysfs.h       |  8 ++++++++
 6 files changed, 81 insertions(+), 42 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 283f00a..8a4cfdb 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -26,6 +26,7 @@
 #include <linux/spinlock.h>
 #include <linux/freezer.h>
 #include <linux/major.h>
+#include <linux/kernfs.h>
 #include "tpm.h"
 #include "tpm_eventlog.h"
 
@@ -119,6 +120,9 @@ struct tpm_chip *tpmm_chip_alloc(struct device *dev,
 	chip->dev.class = tpm_class;
 	chip->dev.release = tpm_dev_release;
 	chip->dev.parent = chip->pdev;
+#ifdef CONFIG_ACPI
+	chip->dev.groups = chip->groups;
+#endif
 
 	if (chip->dev_num == 0)
 		chip->dev.devt = MKDEV(MISC_MAJOR, TPM_MINOR);
@@ -181,12 +185,6 @@ static int tpm1_chip_register(struct tpm_chip *chip)
 	if (rc)
 		return rc;
 
-	rc = tpm_add_ppi(chip);
-	if (rc) {
-		tpm_sysfs_del_device(chip);
-		return rc;
-	}
-
 	chip->bios_dir = tpm_bios_log_setup(chip->devname);
 
 	return 0;
@@ -200,8 +198,6 @@ static void tpm1_chip_unregister(struct tpm_chip *chip)
 	if (chip->bios_dir)
 		tpm_bios_log_teardown(chip->bios_dir);
 
-	tpm_remove_ppi(chip);
-
 	tpm_sysfs_del_device(chip);
 }
 
@@ -224,10 +220,17 @@ int tpm_chip_register(struct tpm_chip *chip)
 	if (rc)
 		return rc;
 
+	tpm_add_ppi(chip);
+
 	rc = tpm_dev_add_device(chip);
 	if (rc)
 		goto out_err;
 
+	rc = sysfs_link_group_to_kobj(&chip->pdev->kobj, &chip->dev.kobj,
+				      "ppi");
+	if (rc)
+		goto out_err;
+
 	/* Make the chip available. */
 	spin_lock(&driver_lock);
 	list_add_rcu(&chip->list, &tpm_chip_list);
@@ -262,6 +265,8 @@ void tpm_chip_unregister(struct tpm_chip *chip)
 	spin_unlock(&driver_lock);
 	synchronize_rcu();
 
+	kernfs_remove_by_name(chip->pdev->kobj.sd, "ppi");
+
 	tpm1_chip_unregister(chip);
 	tpm_dev_del_device(chip);
 }
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index f8319a0..3f96025 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -151,8 +151,7 @@ struct tpm_vendor_specific {
 
 enum tpm_chip_flags {
 	TPM_CHIP_FLAG_REGISTERED	= BIT(0),
-	TPM_CHIP_FLAG_PPI		= BIT(1),
-	TPM_CHIP_FLAG_TPM2		= BIT(2),
+	TPM_CHIP_FLAG_TPM2		= BIT(1),
 };
 
 struct tpm_chip {
@@ -175,6 +174,8 @@ struct tpm_chip {
 	struct dentry **bios_dir;
 
 #ifdef CONFIG_ACPI
+	const struct attribute_group *groups[2];
+	unsigned int groups_cnt;
 	acpi_handle acpi_dev_handle;
 	char ppi_version[TPM_PPI_VERSION_LEN + 1];
 #endif /* CONFIG_ACPI */
@@ -182,7 +183,7 @@ struct tpm_chip {
 	struct list_head list;
 };
 
-#define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
+#define to_tpm_chip(n) container_of(dev, struct tpm_chip, dev)
 
 static inline void tpm_chip_put(struct tpm_chip *chip)
 {
@@ -412,15 +413,9 @@ void tpm_sysfs_del_device(struct tpm_chip *chip);
 int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, u8 *res_buf);
 
 #ifdef CONFIG_ACPI
-extern int tpm_add_ppi(struct tpm_chip *chip);
-extern void tpm_remove_ppi(struct tpm_chip *chip);
+extern void tpm_add_ppi(struct tpm_chip *chip);
 #else
-static inline int tpm_add_ppi(struct tpm_chip *chip)
-{
-	return 0;
-}
-
-static inline void tpm_remove_ppi(struct tpm_chip *chip)
+static inline void tpm_add_ppi(struct tpm_chip *chip)
 {
 }
 #endif
diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c
index 6ca9b5d..692a2c6 100644
--- a/drivers/char/tpm/tpm_ppi.c
+++ b/drivers/char/tpm/tpm_ppi.c
@@ -53,7 +53,7 @@ tpm_eval_dsm(acpi_handle ppi_handle, int func, acpi_object_type type,
 static ssize_t tpm_show_ppi_version(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 scnprintf(buf, PAGE_SIZE, "%s\n", chip->ppi_version);
 }
@@ -63,7 +63,7 @@ static ssize_t tpm_show_ppi_request(struct device *dev,
 {
 	ssize_t size = -EINVAL;
 	union acpi_object *obj;
-	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct tpm_chip *chip = to_tpm_chip(dev);
 
 	obj = tpm_eval_dsm(chip->acpi_dev_handle, TPM_PPI_FN_GETREQ,
 			   ACPI_TYPE_PACKAGE, NULL);
@@ -100,7 +100,7 @@ static ssize_t tpm_store_ppi_request(struct device *dev,
 	int func = TPM_PPI_FN_SUBREQ;
 	union acpi_object *obj, tmp;
 	union acpi_object argv4 = ACPI_INIT_DSM_ARGV4(1, &tmp);
-	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct tpm_chip *chip = to_tpm_chip(dev);
 
 	/*
 	 * the function to submit TPM operation request to pre-os environment
@@ -156,7 +156,7 @@ static ssize_t tpm_show_ppi_transition_action(struct device *dev,
 		.buffer.length = 0,
 		.buffer.pointer = NULL
 	};
-	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct tpm_chip *chip = to_tpm_chip(dev);
 
 	static char *info[] = {
 		"None",
@@ -197,7 +197,7 @@ static ssize_t tpm_show_ppi_response(struct device *dev,
 	acpi_status status = -EINVAL;
 	union acpi_object *obj, *ret_obj;
 	u64 req, res;
-	struct tpm_chip *chip = dev_get_drvdata(dev);
+	struct tpm_chip *chip = to_tpm_chip(dev);
 
 	obj = tpm_eval_dsm(chip->acpi_dev_handle, TPM_PPI_FN_GETRSP,
 			   ACPI_TYPE_PACKAGE, NULL);
@@ -296,7 +296,7 @@ static ssize_t tpm_show_ppi_tcg_operations(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 show_ppi_operations(chip->acpi_dev_handle, buf, 0,
 				   PPI_TPM_REQ_MAX);
@@ -306,7 +306,7 @@ static ssize_t tpm_show_ppi_vs_operations(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 show_ppi_operations(chip->acpi_dev_handle, buf, PPI_VS_REQ_START,
 				   PPI_VS_REQ_END);
@@ -334,17 +334,16 @@ static struct attribute_group ppi_attr_grp = {
 	.attrs = ppi_attrs
 };
 
-int tpm_add_ppi(struct tpm_chip *chip)
+void tpm_add_ppi(struct tpm_chip *chip)
 {
 	union acpi_object *obj;
-	int rc;
 
 	if (!chip->acpi_dev_handle)
-		return 0;
+		return;
 
 	if (!acpi_check_dsm(chip->acpi_dev_handle, tpm_ppi_uuid,
 			    TPM_PPI_REVISION_ID, 1 << TPM_PPI_FN_VERSION))
-		return 0;
+		return;
 
 	/* Cache PPI version string. */
 	obj = acpi_evaluate_dsm_typed(chip->acpi_dev_handle, tpm_ppi_uuid,
@@ -356,16 +355,5 @@ int tpm_add_ppi(struct tpm_chip *chip)
 		ACPI_FREE(obj);
 	}
 
-	rc = sysfs_create_group(&chip->pdev->kobj, &ppi_attr_grp);
-
-	if (!rc)
-		chip->flags |= TPM_CHIP_FLAG_PPI;
-
-	return rc;
-}
-
-void tpm_remove_ppi(struct tpm_chip *chip)
-{
-	if (chip->flags & TPM_CHIP_FLAG_PPI)
-		sysfs_remove_group(&chip->pdev->kobj, &ppi_attr_grp);
+	chip->groups[chip->groups_cnt++] = &ppi_attr_grp;
 }
diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
index 6acc964..749cea3 100644
--- a/fs/kernfs/dir.c
+++ b/fs/kernfs/dir.c
@@ -1225,6 +1225,7 @@ int kernfs_remove_by_name_ns(struct kernfs_node *parent, const char *name,
 	else
 		return -ENOENT;
 }
+EXPORT_SYMBOL_GPL(kernfs_remove_by_name_ns);
 
 /**
  * kernfs_rename_ns - move and rename a kernfs_node
diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index b400c04..25267d9 100644
--- a/fs/sysfs/group.c
+++ b/fs/sysfs/group.c
@@ -352,3 +352,45 @@ void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
 	}
 }
 EXPORT_SYMBOL_GPL(sysfs_remove_link_from_group);
+
+/**
+ * sysfs_link_group_to_kobj - add a symlink to a kobject pointing to a group
+ * @kobj:		The kobject containing the group.
+ * @target_kobj:	The target kobject.
+ * @target_name:	Name of the target group.
+ */
+int sysfs_link_group_to_kobj(struct kobject *kobj, struct kobject *target_kobj,
+			     const char *target_name)
+{
+	struct kernfs_node *target_kobj_sd;
+	struct kernfs_node *target_grp;
+	struct kernfs_node *link;
+
+	/*
+	 * We don't own @target_kobj and it may be removed at any time.
+	 * Synchronize using sysfs_symlink_target_lock. See sysfs_remove_dir()
+	 * for details.
+	 */
+	spin_lock(&sysfs_symlink_target_lock);
+	target_kobj_sd = target_kobj->sd;
+	if (target_kobj_sd)
+		kernfs_get(target_kobj_sd);
+	spin_unlock(&sysfs_symlink_target_lock);
+	if (!target_kobj_sd)
+		return -ENOENT;
+
+	target_grp = kernfs_find_and_get(target_kobj->sd, target_name);
+	if (!target_grp) {
+		kernfs_put(target_kobj_sd);
+		return -ENOENT;
+	}
+
+	link = kernfs_create_link(kobj->sd, target_name, target_grp);
+	if (IS_ERR(link) && PTR_ERR(link) == -EEXIST)
+		sysfs_warn_dup(kobj->sd, target_name);
+
+	kernfs_put(target_grp);
+	kernfs_put(target_kobj_sd);
+	return IS_ERR(link) ? PTR_ERR(link) : 0;
+}
+EXPORT_SYMBOL_GPL(sysfs_link_group_to_kobj);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 99382c0..7f2963e 100644
--- a/include/linux/sysfs.h
+++ b/include/linux/sysfs.h
@@ -264,6 +264,8 @@ int sysfs_add_link_to_group(struct kobject *kobj, const char *group_name,
 			    struct kobject *target, const char *link_name);
 void sysfs_remove_link_from_group(struct kobject *kobj, const char *group_name,
 				  const char *link_name);
+int sysfs_link_group_to_kobj(struct kobject *kobj, struct kobject *target_kobj,
+			     const char *target_name);
 
 void sysfs_notify(struct kobject *kobj, const char *dir, const char *attr);
 
@@ -436,6 +438,12 @@ static inline void sysfs_remove_link_from_group(struct kobject *kobj,
 {
 }
 
+int sysfs_link_group_to_kobj(struct kobject *kobj, struct kobject *target_kobj,
+			     const char *target_name)
+{
+	return 0;
+}
+
 static inline void sysfs_notify(struct kobject *kobj, const char *dir,
 				const char *attr)
 {
-- 
2.1.4

Re: [PATCH v2] tpm: enable PPI for TPM 2.0

[Guenter Roeck]

On Tue, Apr 21, 2015 at 09:30:55PM +0300, Jarkko Sakkinen wrote:
> Enabled PPI interface to the character device sysfs directory accessible
> both for 1.x and 2.0 devices.
> 
> The ppi group is moved from the platform device directory to the
> character device directory. In order to retain backwards compatibility
> with the 1.x devices, a symlink is created to the platform device
> directory.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jason.gunthorpe@obsidianresearch.com>
> ---
>  drivers/char/tpm/tpm-chip.c | 21 +++++++++++++--------
>  drivers/char/tpm/tpm.h      | 17 ++++++-----------
>  drivers/char/tpm/tpm_ppi.c  | 34 +++++++++++-----------------------
>  fs/kernfs/dir.c             |  1 +
>  fs/sysfs/group.c            | 42 ++++++++++++++++++++++++++++++++++++++++++
>  include/linux/sysfs.h       |  8 ++++++++

Shouldn't the sysfs changes be a separate patch ?

Guenter

Re: [PATCH v2] tpm: enable PPI for TPM 2.0

[Jason Gunthorpe]

On Tue, Apr 21, 2015 at 09:30:55PM +0300, Jarkko Sakkinen wrote:
> Enabled PPI interface to the character device sysfs directory accessible
> both for 1.x and 2.0 devices.
> 
> The ppi group is moved from the platform device directory to the
> character device directory. In order to retain backwards compatibility
> with the 1.x devices, a symlink is created to the platform device
> directory.
> 
> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>

> Reviewed-by: Jason Gunthorpe <jason.gunthorpe@obsidianresearch.com>

Jumping the gun a bit, there :)
But yes, the TPM bits still look OK to me.

Ah, don't forget to update Documentation/ABI/testing/sysfs-driver-ppi

> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index 6acc964..749cea3 100644
> +++ b/fs/kernfs/dir.c
> @@ -1225,6 +1225,7 @@ int kernfs_remove_by_name_ns(struct kernfs_node *parent, const char *name,
>  	else
>  		return -ENOENT;
>  }
> +EXPORT_SYMBOL_GPL(kernfs_remove_by_name_ns);

??
I don't see this being called

> +/**
> + * sysfs_link_group_to_kobj - add a symlink to a kobject pointing to a group
> + * @kobj:		The kobject containing the group.
> + * @target_kobj:	The target kobject.
> + * @target_name:	Name of the target group.
> + */
> +int sysfs_link_group_to_kobj(struct kobject *kobj, struct kobject *target_kobj,
> +			     const char *target_name)

Agree with Guenter, separate patch.

I liked the signature I suggested earlier:

sysfs_link_group_to_kobj(struct kobject *from_kobj,
			 const char *from_name,
                         struct kobject *target_kobj,
                         const char *target_name)

'link_group' may be too specific a name, I think this would work to
establish a link to any file in a sysfs directory? Which is good,
we'll need that for future TPM patches that relocate the other sysfs
files..

Jason

Re: [PATCH v2] tpm: enable PPI for TPM 2.0

[Greg Kroah-Hartman]

On Tue, Apr 21, 2015 at 09:30:55PM +0300, Jarkko Sakkinen wrote:
> -#define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
> +#define to_tpm_chip(n) container_of(dev, struct tpm_chip, dev)

That doesn't look correct to me...

thanks,

greg k-h

Re: [PATCH v2] tpm: enable PPI for TPM 2.0

[Jarkko Sakkinen]

On Tue, Apr 21, 2015 at 09:52:14PM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 21, 2015 at 09:30:55PM +0300, Jarkko Sakkinen wrote:
> > -#define to_tpm_chip(n) container_of(n, struct tpm_chip, vendor)
> > +#define to_tpm_chip(n) container_of(dev, struct tpm_chip, dev)
> 
> That doesn't look correct to me...

Oops, no it doesn't (things worked though because the local variable
where this was used was named as dev). Thanks for pointing this out.

> thanks,
> 
> greg k-h

/Jarkko

Re: [PATCH v2] tpm: enable PPI for TPM 2.0

[Jarkko Sakkinen]

On Tue, Apr 21, 2015 at 11:58:33AM -0700, Guenter Roeck wrote:
> On Tue, Apr 21, 2015 at 09:30:55PM +0300, Jarkko Sakkinen wrote:
> > Enabled PPI interface to the character device sysfs directory accessible
> > both for 1.x and 2.0 devices.
> > 
> > The ppi group is moved from the platform device directory to the
> > character device directory. In order to retain backwards compatibility
> > with the 1.x devices, a symlink is created to the platform device
> > directory.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > Reviewed-by: Jason Gunthorpe <jason.gunthorpe@obsidianresearch.com>
> > ---
> >  drivers/char/tpm/tpm-chip.c | 21 +++++++++++++--------
> >  drivers/char/tpm/tpm.h      | 17 ++++++-----------
> >  drivers/char/tpm/tpm_ppi.c  | 34 +++++++++++-----------------------
> >  fs/kernfs/dir.c             |  1 +
> >  fs/sysfs/group.c            | 42 ++++++++++++++++++++++++++++++++++++++++++
> >  include/linux/sysfs.h       |  8 ++++++++
> 
> Shouldn't the sysfs changes be a separate patch ?

Yes, I think it would make sense. I'll refine this as a patch set.
Thanks.

> Guenter

/Jarkko

Re: [PATCH v2] tpm: enable PPI for TPM 2.0

[Jarkko Sakkinen]

On Tue, Apr 21, 2015 at 01:33:38PM -0600, Jason Gunthorpe wrote:
> On Tue, Apr 21, 2015 at 09:30:55PM +0300, Jarkko Sakkinen wrote:
> > Enabled PPI interface to the character device sysfs directory accessible
> > both for 1.x and 2.0 devices.
> > 
> > The ppi group is moved from the platform device directory to the
> > character device directory. In order to retain backwards compatibility
> > with the 1.x devices, a symlink is created to the platform device
> > directory.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> > Reviewed-by: Jason Gunthorpe <jason.gunthorpe@obsidianresearch.com>
> 
> Jumping the gun a bit, there :)
> But yes, the TPM bits still look OK to me.
> Ah, don't forget to update Documentation/ABI/testing/sysfs-driver-ppi

Right. Is there a rigid naming convention for these files? I don't see
any obvious pattern and there's no documentation for file names in here:

https://www.kernel.org/doc/Documentation/ABI/README

The legacy TPM 1.x sysfs attributes are in a file named as sysfs-class-tpm.

Maybe we could:

* Rename existing sysfs-class-tpm as sysfs-class-tpm1
* Rename sysfs-driver-ppi as sysfs-class-tpm

What do you think? I don't know exactly what to do here. That is the
reason I haven't updated the documentation yet.

> > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> > index 6acc964..749cea3 100644
> > +++ b/fs/kernfs/dir.c
> > @@ -1225,6 +1225,7 @@ int kernfs_remove_by_name_ns(struct kernfs_node *parent, const char *name,
> >  	else
> >  		return -ENOENT;
> >  }
> > +EXPORT_SYMBOL_GPL(kernfs_remove_by_name_ns);
> 
> ??
> I don't see this being called

My bad, it is clutter in the patch file. I'll remove it.

> > +/**
> > + * sysfs_link_group_to_kobj - add a symlink to a kobject pointing to a group
> > + * @kobj:		The kobject containing the group.
> > + * @target_kobj:	The target kobject.
> > + * @target_name:	Name of the target group.
> > + */
> > +int sysfs_link_group_to_kobj(struct kobject *kobj, struct kobject *target_kobj,
> > +			     const char *target_name)
> 
> Agree with Guenter, separate patch.
> 
> I liked the signature I suggested earlier:
> 
> sysfs_link_group_to_kobj(struct kobject *from_kobj,
> 			 const char *from_name,
>                          struct kobject *target_kobj,
>                          const char *target_name)
> 
> 'link_group' may be too specific a name, I think this would work to
> establish a link to any file in a sysfs directory? Which is good,
> we'll need that for future TPM patches that relocate the other sysfs
> files..

I think it should.

> Jason.

/Jarkko

Re: [tpmdd-devel] [PATCH v2] tpm: enable PPI for TPM 2.0

[Jarkko Sakkinen]

On Wed, Apr 22, 2015 at 08:57:55AM +0300, Jarkko Sakkinen wrote:
> On Tue, Apr 21, 2015 at 01:33:38PM -0600, Jason Gunthorpe wrote:
> > On Tue, Apr 21, 2015 at 09:30:55PM +0300, Jarkko Sakkinen wrote:
> > > Enabled PPI interface to the character device sysfs directory accessible
> > > both for 1.x and 2.0 devices.
> > > 
> > > The ppi group is moved from the platform device directory to the
> > > character device directory. In order to retain backwards compatibility
> > > with the 1.x devices, a symlink is created to the platform device
> > > directory.
> > > 
> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> > 
> > > Reviewed-by: Jason Gunthorpe <jason.gunthorpe@obsidianresearch.com>
> > 
> > Jumping the gun a bit, there :)
> > But yes, the TPM bits still look OK to me.
> > Ah, don't forget to update Documentation/ABI/testing/sysfs-driver-ppi
> 
> Right. Is there a rigid naming convention for these files? I don't see
> any obvious pattern and there's no documentation for file names in here:
> 
> https://www.kernel.org/doc/Documentation/ABI/README
> 
> The legacy TPM 1.x sysfs attributes are in a file named as sysfs-class-tpm.
> 
> Maybe we could:
> 
> * Rename existing sysfs-class-tpm as sysfs-class-tpm1
> * Rename sysfs-driver-ppi as sysfs-class-tpm
> 
> What do you think? I don't know exactly what to do here. That is the
> reason I haven't updated the documentation yet.
> 
> > > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> > > index 6acc964..749cea3 100644
> > > +++ b/fs/kernfs/dir.c
> > > @@ -1225,6 +1225,7 @@ int kernfs_remove_by_name_ns(struct kernfs_node *parent, const char *name,
> > >  	else
> > >  		return -ENOENT;
> > >  }
> > > +EXPORT_SYMBOL_GPL(kernfs_remove_by_name_ns);
> > 
> > ??
> > I don't see this being called
> 
> My bad, it is clutter in the patch file. I'll remove it.

In fact this is needed because the driver uses kernfs_remove_by_name,
which is an inline function that uses kernfs_remove_by_name_ns
internally.

> > > +/**
> > > + * sysfs_link_group_to_kobj - add a symlink to a kobject pointing to a group
> > > + * @kobj:		The kobject containing the group.
> > > + * @target_kobj:	The target kobject.
> > > + * @target_name:	Name of the target group.
> > > + */
> > > +int sysfs_link_group_to_kobj(struct kobject *kobj, struct kobject *target_kobj,
> > > +			     const char *target_name)
> > 
> > Agree with Guenter, separate patch.
> > 
> > I liked the signature I suggested earlier:
> > 
> > sysfs_link_group_to_kobj(struct kobject *from_kobj,
> > 			 const char *from_name,
> >                          struct kobject *target_kobj,
> >                          const char *target_name)
> > 
> > 'link_group' may be too specific a name, I think this would work to
> > establish a link to any file in a sysfs directory? Which is good,
> > we'll need that for future TPM patches that relocate the other sysfs
> > files..
> 
> I think it should.
> 
> > Jason.
> 
> /Jarkko

/Jarkko

Re: [tpmdd-devel] [PATCH v2] tpm: enable PPI for TPM 2.0

[Jason Gunthorpe]

On Wed, Apr 22, 2015 at 07:38:55PM +0300, Jarkko Sakkinen wrote:
> > > > +EXPORT_SYMBOL_GPL(kernfs_remove_by_name_ns);
> > > 
> > > ??
> > > I don't see this being called
> > 
> > My bad, it is clutter in the patch file. I'll remove it.
> 
> In fact this is needed because the driver uses kernfs_remove_by_name,
> which is an inline function that uses kernfs_remove_by_name_ns
> internally.

Hum, that probably needs a sysfs_ analog - the paired removal
function for sysfs_link_group_to_kobj ?

Jason

Re: [tpmdd-devel] [PATCH v2] tpm: enable PPI for TPM 2.0

[Greg Kroah-Hartman]

On Wed, Apr 22, 2015 at 11:22:43AM -0600, Jason Gunthorpe wrote:
> On Wed, Apr 22, 2015 at 07:38:55PM +0300, Jarkko Sakkinen wrote:
> > > > > +EXPORT_SYMBOL_GPL(kernfs_remove_by_name_ns);
> > > > 
> > > > ??
> > > > I don't see this being called
> > > 
> > > My bad, it is clutter in the patch file. I'll remove it.
> > 
> > In fact this is needed because the driver uses kernfs_remove_by_name,
> > which is an inline function that uses kernfs_remove_by_name_ns
> > internally.
> 
> Hum, that probably needs a sysfs_ analog - the paired removal
> function for sysfs_link_group_to_kobj ?

Be very careful using that sysfs function, you shouldn't be doing that
within a driver subsystem unless you know what you are doing :)

greg k-h

Re: [PATCH v2] tpm: enable PPI for TPM 2.0

[Jarkko Sakkinen]

On Tue, Apr 21, 2015 at 01:33:38PM -0600, Jason Gunthorpe wrote:
> On Tue, Apr 21, 2015 at 09:30:55PM +0300, Jarkko Sakkinen wrote:
> > Enabled PPI interface to the character device sysfs directory accessible
> > both for 1.x and 2.0 devices.
> > 
> > The ppi group is moved from the platform device directory to the
> > character device directory. In order to retain backwards compatibility
> > with the 1.x devices, a symlink is created to the platform device
> > directory.
> > 
> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> 
> > Reviewed-by: Jason Gunthorpe <jason.gunthorpe@obsidianresearch.com>
> 
> Jumping the gun a bit, there :)
> But yes, the TPM bits still look OK to me.
> 
> Ah, don't forget to update Documentation/ABI/testing/sysfs-driver-ppi
> 
> > diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> > index 6acc964..749cea3 100644
> > +++ b/fs/kernfs/dir.c
> > @@ -1225,6 +1225,7 @@ int kernfs_remove_by_name_ns(struct kernfs_node *parent, const char *name,
> >  	else
> >  		return -ENOENT;
> >  }
> > +EXPORT_SYMBOL_GPL(kernfs_remove_by_name_ns);
> 
> ??
> I don't see this being called
> 
> > +/**
> > + * sysfs_link_group_to_kobj - add a symlink to a kobject pointing to a group
> > + * @kobj:		The kobject containing the group.
> > + * @target_kobj:	The target kobject.
> > + * @target_name:	Name of the target group.
> > + */
> > +int sysfs_link_group_to_kobj(struct kobject *kobj, struct kobject *target_kobj,
> > +			     const char *target_name)
> 
> Agree with Guenter, separate patch.
> 
> I liked the signature I suggested earlier:
> 
> sysfs_link_group_to_kobj(struct kobject *from_kobj,
> 			 const char *from_name,
>                          struct kobject *target_kobj,
>                          const char *target_name)
> 
> 'link_group' may be too specific a name, I think this would work to
> establish a link to any file in a sysfs directory? Which is good,
> we'll need that for future TPM patches that relocate the other sysfs
> files..

BTW, which sysfs attributes would need to be relocated? Other than PPI
at least we are not interested to port for TPM 2.0 because you can just
as well get the data using TPM2 protocol and /dev/tpm0.

> Jason

/Jarkko