2015-06-22 17:25:43

by Jarkko Sakkinen

[permalink] [raw]
Subject: [PATCH v7 0/3] Enable PPI sysfs interface for TPM 2.0

v7:
* Fixed compile error when CONFIG_SYSFS is not enabled.

v6:
* Updated documentation.

v5:
* Removed dangling export of kernfs_remove_by_name_ns() from the sysfs
patch.

v4:
* Use sysfs_remove_link()

v3:
* Fixed to_tpm_chip() macro.
* Split into two patches.
* Renamed sysfs_link_group_to_kobj to sysfs_link_entry_to_kobj
* Only create the "backwards compatibility" symlink for TPM 1.x devices.

Jarkko Sakkinen (3):
sysfs: added sysfs_link_entry_to_kobj()
tpm: move the PPI attributes to character device directory.
tpm: update PPI documentation to address the location change.

Documentation/ABI/testing/sysfs-driver-ppi | 19 +++++++++-----
drivers/char/tpm/tpm-chip.c | 24 +++++++++++------
drivers/char/tpm/tpm.h | 17 +++++-------
drivers/char/tpm/tpm_ppi.c | 34 ++++++++----------------
fs/sysfs/group.c | 42 ++++++++++++++++++++++++++++++
include/linux/sysfs.h | 9 +++++++
6 files changed, 96 insertions(+), 49 deletions(-)

--
2.1.4


2015-06-22 17:25:54

by Jarkko Sakkinen

[permalink] [raw]
Subject: [PATCH v7 1/3] sysfs: added sysfs_link_entry_to_kobj()

Added a new function sysfs_link_group_to_kobj() that adds a symlink
from attribute or group to a kobject. Exported kernfs_remove_by_name_ns
in order to provide a way to remove such symlinks.

Signed-off-by: Jarkko Sakkinen <[email protected]>
---
fs/sysfs/group.c | 42 ++++++++++++++++++++++++++++++++++++++++++
include/linux/sysfs.h | 9 +++++++++
2 files changed, 51 insertions(+)

diff --git a/fs/sysfs/group.c b/fs/sysfs/group.c
index b400c04..5c72996 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_entry_to_kobj - add a symlink to a kobject pointing to a group or an attribute
+ * @kobj: The kobject containing the group.
+ * @target_kobj: The target kobject.
+ * @target_name: The name of the target group or attribute.
+ */
+int sysfs_link_entry_to_kobj(struct kobject *kobj, struct kobject *target_kobj,
+ const char *target_name)
+{
+ struct kernfs_node *target;
+ struct kernfs_node *entry;
+ 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 = target_kobj->sd;
+ if (target)
+ kernfs_get(target);
+ spin_unlock(&sysfs_symlink_target_lock);
+ if (!target)
+ return -ENOENT;
+
+ entry = kernfs_find_and_get(target_kobj->sd, target_name);
+ if (!entry) {
+ kernfs_put(target);
+ return -ENOENT;
+ }
+
+ link = kernfs_create_link(kobj->sd, target_name, entry);
+ if (IS_ERR(link) && PTR_ERR(link) == -EEXIST)
+ sysfs_warn_dup(kobj->sd, target_name);
+
+ kernfs_put(entry);
+ kernfs_put(target);
+ return IS_ERR(link) ? PTR_ERR(link) : 0;
+}
+EXPORT_SYMBOL_GPL(sysfs_link_entry_to_kobj);
diff --git a/include/linux/sysfs.h b/include/linux/sysfs.h
index 99382c0..bfdf5c7 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_entry_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,13 @@ static inline void sysfs_remove_link_from_group(struct kobject *kobj,
{
}

+static inline int sysfs_link_entry_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

2015-06-22 17:26:48

by Jarkko Sakkinen

[permalink] [raw]
Subject: [PATCH v7 2/3] tpm: move the PPI attributes to character device directory.

Moved PPI attributes to the character device directory. This aligns with
the sysfs guidelines and makes them race free because they are created
atomically with the character device as part of device_register().The
character device and the sysfs attributes appear at the same time to the
user space.

As part of this change we enable PPI attributes also for TPM 2.0
devices. In order to retain backwards compatibility with TPM 1.x
devices, a symlink is created to the platform device directory.

Signed-off-by: Jarkko Sakkinen <[email protected]>
Reviewed-by: Jason Gunthorpe <[email protected]>
---
drivers/char/tpm/tpm-chip.c | 24 ++++++++++++++++--------
drivers/char/tpm/tpm.h | 17 ++++++-----------
drivers/char/tpm/tpm_ppi.c | 34 +++++++++++-----------------------
3 files changed, 33 insertions(+), 42 deletions(-)

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 283f00a..cc1f8f8 100644
--- a/drivers/char/tpm/tpm-chip.c
+++ b/drivers/char/tpm/tpm-chip.c
@@ -119,6 +119,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 +184,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 +197,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 +219,20 @@ 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;

+ if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) {
+ rc = sysfs_link_entry_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 +267,9 @@ void tpm_chip_unregister(struct tpm_chip *chip)
spin_unlock(&driver_lock);
synchronize_rcu();

+ if (!(chip->flags & TPM_CHIP_FLAG_TPM2))
+ sysfs_remove_link(&chip->pdev->kobj, "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..5e7231a 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(d) container_of(d, 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;
}
--
2.1.4

2015-06-22 17:26:13

by Jarkko Sakkinen

[permalink] [raw]
Subject: [PATCH v7 3/3] tpm: update PPI documentation to address the location change.

Updated Documentation/ABI/testing/sysfs-driver-ppi in order to explain
where PPI attributes are located and how backwards compatiblity is
addressed.

Signed-off-by: Jarkko Sakkinen <[email protected]>
---
Documentation/ABI/testing/sysfs-driver-ppi | 19 ++++++++++++-------
1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/Documentation/ABI/testing/sysfs-driver-ppi b/Documentation/ABI/testing/sysfs-driver-ppi
index 7d1435b..9921ef2 100644
--- a/Documentation/ABI/testing/sysfs-driver-ppi
+++ b/Documentation/ABI/testing/sysfs-driver-ppi
@@ -1,4 +1,4 @@
-What: /sys/devices/pnp0/<bus-num>/ppi/
+What: /sys/class/tpm/tpmX/ppi/
Date: August 2012
Kernel Version: 3.6
Contact: [email protected]
@@ -8,9 +8,14 @@ Description:
folder makes sense. The folder path can be got by command
'find /sys/ -name 'pcrs''. For the detail information of PPI,
please refer to the PPI specification from
+
http://www.trustedcomputinggroup.org/

-What: /sys/devices/pnp0/<bus-num>/ppi/version
+ In Linux 4.2 ppi was moved to the character device directory.
+ A symlink from tpmX/device/ppi to tpmX/ppi to provide backwards
+ compatibility.
+
+What: /sys/class/tpm/tpmX/ppi/version
Date: August 2012
Contact: [email protected]
Description:
@@ -18,7 +23,7 @@ Description:
platform.
This file is readonly.

-What: /sys/devices/pnp0/<bus-num>/ppi/request
+What: /sys/class/tpm/tpmX/ppi/request
Date: August 2012
Contact: [email protected]
Description:
@@ -28,7 +33,7 @@ Description:
integer value range from 1 to 160, and 0 means no request.
This file can be read and written.

-What: /sys/devices/pnp0/00:<bus-num>/ppi/response
+What: /sys/class/tpm/tpmX/ppi/response
Date: August 2012
Contact: [email protected]
Description:
@@ -37,7 +42,7 @@ Description:
: <response description>".
This file is readonly.

-What: /sys/devices/pnp0/<bus-num>/ppi/transition_action
+What: /sys/class/tpm/tpmX/ppi/transition_action
Date: August 2012
Contact: [email protected]
Description:
@@ -47,7 +52,7 @@ Description:
description>".
This file is readonly.

-What: /sys/devices/pnp0/<bus-num>/ppi/tcg_operations
+What: /sys/class/tpm/tpmX/ppi/tcg_operations
Date: August 2012
Contact: [email protected]
Description:
@@ -58,7 +63,7 @@ Description:
This attribute is only supported by PPI version 1.2+.
This file is readonly.

-What: /sys/devices/pnp0/<bus-num>/ppi/vs_operations
+What: /sys/class/tpm/tpmX/ppi/vs_operations
Date: August 2012
Contact: [email protected]
Description:
--
2.1.4

2015-06-22 17:30:50

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH v7 1/3] sysfs: added sysfs_link_entry_to_kobj()

On Mon, Jun 22, 2015 at 08:24:50PM +0300, Jarkko Sakkinen wrote:
> Added a new function sysfs_link_group_to_kobj() that adds a symlink
> from attribute or group to a kobject. Exported kernfs_remove_by_name_ns
> in order to provide a way to remove such symlinks.
>
> Signed-off-by: Jarkko Sakkinen <[email protected]>

Hmmm... is this *really* necessary? If linking from the parent kobj
doesn't make a fundamental functional difference, I don't think this
is a good idea. If linking to the parent doesn't work, why doesn't
it? Shouldn't that already be a different kobj then? I'd really like
to keep groups as a dumb container of simple attrs.

Thanks.

--
tejun

2015-06-22 17:53:17

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH v7 1/3] sysfs: added sysfs_link_entry_to_kobj()

On Mon, Jun 22, 2015 at 01:30:39PM -0400, Tejun Heo wrote:
> On Mon, Jun 22, 2015 at 08:24:50PM +0300, Jarkko Sakkinen wrote:
> > Added a new function sysfs_link_group_to_kobj() that adds a symlink
> > from attribute or group to a kobject. Exported kernfs_remove_by_name_ns
> > in order to provide a way to remove such symlinks.
> >
> > Signed-off-by: Jarkko Sakkinen <[email protected]>
>
> Hmmm... is this *really* necessary? If linking from the parent kobj
> doesn't make a fundamental functional difference, I don't think this
> is a good idea. If linking to the parent doesn't work, why doesn't
> it? Shouldn't that already be a different kobj then? I'd really like
> to keep groups as a dumb container of simple attrs.

TPM is undergoing a migration of core attributes from the
platform_device to the core's struct device.

The only purpose of the symlink was to provide userspace
compatability with the old location.

Jason

2015-06-22 18:01:59

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH v7 1/3] sysfs: added sysfs_link_entry_to_kobj()

On Mon, Jun 22, 2015 at 11:52:53AM -0600, Jason Gunthorpe wrote:
> On Mon, Jun 22, 2015 at 01:30:39PM -0400, Tejun Heo wrote:
> > On Mon, Jun 22, 2015 at 08:24:50PM +0300, Jarkko Sakkinen wrote:
> > > Added a new function sysfs_link_group_to_kobj() that adds a symlink
> > > from attribute or group to a kobject. Exported kernfs_remove_by_name_ns
> > > in order to provide a way to remove such symlinks.
> > >
> > > Signed-off-by: Jarkko Sakkinen <[email protected]>
> >
> > Hmmm... is this *really* necessary? If linking from the parent kobj
> > doesn't make a fundamental functional difference, I don't think this
> > is a good idea. If linking to the parent doesn't work, why doesn't
> > it? Shouldn't that already be a different kobj then? I'd really like
> > to keep groups as a dumb container of simple attrs.
>
> TPM is undergoing a migration of core attributes from the
> platform_device to the core's struct device.
>
> The only purpose of the symlink was to provide userspace
> compatability with the old location.

Ah, yeah, that's painful. Can you please briefly explain why it
wasn't necessary before? Are you merging multiple devices into one?

Thanks.

--
tejun

2015-06-23 12:43:08

by Sakkinen, Jarkko

[permalink] [raw]
Subject: Re: [PATCH v7 1/3] sysfs: added sysfs_link_entry_to_kobj()

On Mon, Jun 22, 2015 at 02:01:54PM -0400, Tejun Heo wrote:
> On Mon, Jun 22, 2015 at 11:52:53AM -0600, Jason Gunthorpe wrote:
> > On Mon, Jun 22, 2015 at 01:30:39PM -0400, Tejun Heo wrote:
> > > On Mon, Jun 22, 2015 at 08:24:50PM +0300, Jarkko Sakkinen wrote:
> > > > Added a new function sysfs_link_group_to_kobj() that adds a symlink
> > > > from attribute or group to a kobject. Exported kernfs_remove_by_name_ns
> > > > in order to provide a way to remove such symlinks.
> > > >
> > > > Signed-off-by: Jarkko Sakkinen <[email protected]>
> > >
> > > Hmmm... is this *really* necessary? If linking from the parent kobj
> > > doesn't make a fundamental functional difference, I don't think this
> > > is a good idea. If linking to the parent doesn't work, why doesn't
> > > it? Shouldn't that already be a different kobj then? I'd really like
> > > to keep groups as a dumb container of simple attrs.
> >
> > TPM is undergoing a migration of core attributes from the
> > platform_device to the core's struct device.
> >
> > The only purpose of the symlink was to provide userspace
> > compatability with the old location.
>
> Ah, yeah, that's painful. Can you please briefly explain why it
> wasn't necessary before? Are you merging multiple devices into one?

I've been working on for a while on TPM 2.0 protocol support and wanted
to do things right from the beginning for TPM 2.0 level devices.

The first thing was to introduce a device class for TPM devices (both
1.x and 2.0), which I did when the baseline support for TPM 2.0 landed
in 4.0.

The second is to introduce *necessary* sysfs attributes in the correct
place so that they are created and destroyed race free. All sysfs
attributes for TPM 1.x devices have been placed in the pdev directory.

For major part of the attributes I decided not to add them at all for
TPM 2.0 devices because they break all the conventions suggested in
sysfs-rules.txt and more importantly you can acquire the data that they
contain by using /dev/tpm0 and TPM protocol (either 1.x or 2.0).

The PPI interface is necessary because it is used to take the ownership
of the device. The solution that I'm presenting in this patch set is
something that I just considered "least harm" when considering backwards
compatibility and raciness.

> Thanks.
>
> --
> tejun

/Jarkko

2015-06-23 16:19:48

by Jason Gunthorpe

[permalink] [raw]
Subject: Re: [PATCH v7 1/3] sysfs: added sysfs_link_entry_to_kobj()

On Mon, Jun 22, 2015 at 02:01:54PM -0400, Tejun Heo wrote:
> > TPM is undergoing a migration of core attributes from the
> > platform_device to the core's struct device.
> >
> > The only purpose of the symlink was to provide userspace
> > compatability with the old location.
>
> Ah, yeah, that's painful. Can you please briefly explain why it
> wasn't necessary before? Are you merging multiple devices into one?

Prior to the clean up work starting each and every driver was adding
sysfs files on its own to the platform_device and we had no core
struct tpm.struct device all.

The first cleanup moved all the sysfs creation into core code, but
kept the use of the platform device. A second cleanup added the struct
device and removed the misc_device abuse.

The next round was hoped to bring the core code into alignment with
everything else in the kernel:
- Create sysfs files at the right time so userspace isn't racey
- Don't stomp the drvdata of the platform_device in core code
- Have the understandable lifetime model that most of the rest
of the kernel has

The concept was to move the sysfs files to the natural location on the
core's struct tpm.struct device and leave a symlink behind on the
platform_device for compat.

We could also just move the sysfs file and ignore the compat symlink,
but it is not clear to me if that is an OK UAPI break for sysfs?

Jason

2015-07-02 17:04:20

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH v7 1/3] sysfs: added sysfs_link_entry_to_kobj()

Hello, Jason.

On Tue, Jun 23, 2015 at 10:19:32AM -0600, Jason Gunthorpe wrote:
> The concept was to move the sysfs files to the natural location on the
> core's struct tpm.struct device and leave a symlink behind on the
> platform_device for compat.

I see. The only problem I see is that this might get abused. Can you
clearly mark the function that it's not intended to be used in
anything new, e.g. by renaming it to sth like
__compat_only_sysfs_link_entry_to_kobj().

> We could also just move the sysfs file and ignore the compat symlink,
> but it is not clear to me if that is an OK UAPI break for sysfs?

We shouldn't if it has actual chance of disturbing userland.

Thanks.

--
tejun

2015-07-03 11:12:44

by Jarkko Sakkinen

[permalink] [raw]
Subject: Re: [PATCH v7 1/3] sysfs: added sysfs_link_entry_to_kobj()



On 07/02/2015 08:04 PM, Tejun Heo wrote:
> Hello, Jason.
>
> On Tue, Jun 23, 2015 at 10:19:32AM -0600, Jason Gunthorpe wrote:
>> The concept was to move the sysfs files to the natural location on the
>> core's struct tpm.struct device and leave a symlink behind on the
>> platform_device for compat.
>
> I see. The only problem I see is that this might get abused. Can you
> clearly mark the function that it's not intended to be used in
> anything new, e.g. by renaming it to sth like
> __compat_only_sysfs_link_entry_to_kobj().


Great! We will do that. Thanks for taking time and understanding what
we are dealing with. We are trying to fix all the bad behavior from TPM
subsystem and bring up to the todays standards so that next gen TPM 2.0
devices will have a solid platform.

>> We could also just move the sysfs file and ignore the compat symlink,
>> but it is not clear to me if that is an OK UAPI break for sysfs?
>
> We shouldn't if it has actual chance of disturbing userland.
>
> Thanks.

/Jarkko