| Message ID | 1427891332-16709-1-git-send-email-jarkko.sakkinen@linux.intel.com |
|---|---|
| State | Superseded |
| Headers | show |
On Wed, Apr 01, 2015 at 03:28:52PM +0300, Jarkko Sakkinen wrote: > Added PPI interface to the character device. PPI interface is also kept > in the pdev for backwards compatibility. Could you look at just completely moving the PPI interface to the char dev and then adding a symlink from the pdev? That would be really ideal. symlinks have the advantage that they actually fully fix the lifetime issues. This seems doable, if we replace the ppi_attrs group with a bunch of calls to sysfs_create_link it should work ? > +static struct tpm_chip *ppi_dev_to_chip(struct device *dev) > +{ > + struct tpm_chip *chip = dev_get_drvdata(dev); > + > + if (chip == NULL) > + chip = to_tpm_chip(chip); > + > + return chip; > +} If symlinks don't work out, we should probably just set the drvdata on the tpm_chip itself to avoid this. > + if (!(chip->flags & TPM_CHIP_FLAG_PPI)) > + return -EINVAL; Hum, I don't think the PPI files should be created if there is no PPI support.. > +void __init tpm_ppi_init(struct class *tpm_class) > +{ > + tpm_class->dev_groups = tpm_groups; > } So this shouldn't be unconditional. Also, ultimately PPI can't just claim the dev_groups, other parts of the driver will need to add groups too. I think it makes more sense to do struct attribute_group *tpm_ppi_get_sysfs(struct tpm_chip *chip) { } And take care of building the list in the caller. And tpm_ppi_get_sysfs should be called after the driver is readied but before adding the device. Jason ------------------------------------------------------------------------------ Dive into the World of Parallel Programming The Go Parallel Website, sponsored by Intel and developed in partnership with Slashdot Media, is your hub for all things parallel software development, from weekly thought leadership blogs to news, videos, case studies, tutorials and more. Take a look and join the conversation now. http://goparallel.sourceforge.net/
On Wed, Apr 01, 2015 at 12:19:25PM -0600, Jason Gunthorpe wrote: > On Wed, Apr 01, 2015 at 03:28:52PM +0300, Jarkko Sakkinen wrote: > > Added PPI interface to the character device. PPI interface is also kept > > in the pdev for backwards compatibility. > > Could you look at just completely moving the PPI interface to the char > dev and then adding a symlink from the pdev? That would be really > ideal. > > symlinks have the advantage that they actually fully fix the lifetime > issues. > > This seems doable, if we replace the ppi_attrs group with a bunch of > calls to sysfs_create_link it should work ? If we follow the pattern in [1] by the book, how would you use sysfs_create_link()? To be more specific, how would you get the driver core to create the symlinks for you? If we decide not to follow [1] by the book, then this might be doable (thinking off my head, that's the reason why I use *might be* instead of *is*). Wouldn't we get non-racy behavior if sysfs_create_link()'s are executed after device_initialize() and before device_add()? > > +static struct tpm_chip *ppi_dev_to_chip(struct device *dev) > > +{ > > + struct tpm_chip *chip = dev_get_drvdata(dev); > > + > > + if (chip == NULL) > > + chip = to_tpm_chip(chip); > > + > > + return chip; > > +} > > If symlinks don't work out, we should probably just set the drvdata on > the tpm_chip itself to avoid this. I'll experiment with this. Thanks for the comment. > > + if (!(chip->flags & TPM_CHIP_FLAG_PPI)) > > + return -EINVAL; > > Hum, I don't think the PPI files should be created if there is no PPI > support.. Again, following [1] by the book. And again, I think we could just as well do our sysfs stuff in-between device_initialize() and device_add() and get the non-racy behavior. > > +void __init tpm_ppi_init(struct class *tpm_class) > > +{ > > + tpm_class->dev_groups = tpm_groups; > > } > > So this shouldn't be unconditional. > > Also, ultimately PPI can't just claim the dev_groups, other parts of > the driver will need to add groups too. > > I think it makes more sense to do > > struct attribute_group *tpm_ppi_get_sysfs(struct tpm_chip *chip) > { > } > > And take care of building the list in the caller. > > And tpm_ppi_get_sysfs should be called after the driver is readied but > before adding the device. I don't think this would matter. Things could be refactored when more sysfs attributes are needed. > Jason /Jarkko [1] http://kroah.com/log/blog/2013/06/26/how-to-create-a-sysfs-file-correctly/ ------------------------------------------------------------------------------ BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT Develop your own process in accordance with the BPMN 2 standard Learn Process modeling best practices with Bonita BPM through live exercises http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_ source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF
Gave this some rethought :) On Wed, Apr 08, 2015 at 10:26:07AM +0300, Jarkko Sakkinen wrote: > On Wed, Apr 01, 2015 at 12:19:25PM -0600, Jason Gunthorpe wrote: > > On Wed, Apr 01, 2015 at 03:28:52PM +0300, Jarkko Sakkinen wrote: > > > Added PPI interface to the character device. PPI interface is also kept > > > in the pdev for backwards compatibility. > > > > Could you look at just completely moving the PPI interface to the char > > dev and then adding a symlink from the pdev? That would be really > > ideal. > > > > symlinks have the advantage that they actually fully fix the lifetime > > issues. > > > > This seems doable, if we replace the ppi_attrs group with a bunch of > > calls to sysfs_create_link it should work ? > > If we follow the pattern in [1] by the book, how would you use > sysfs_create_link()? To be more specific, how would you get the driver > core to create the symlinks for you? > > If we decide not to follow [1] by the book, then this might be doable > (thinking off my head, that's the reason why I use *might be* instead > of *is*). Wouldn't we get non-racy behavior if sysfs_create_link()'s > are executed after device_initialize() and before device_add()? Here I tend to lean towards for creating a separate set of attributes instead. I would keep the legacy stuff completely separated of the sysfs attributes for the character devices and not do any clever things in the sysfs. > > > +static struct tpm_chip *ppi_dev_to_chip(struct device *dev) > > > +{ > > > + struct tpm_chip *chip = dev_get_drvdata(dev); > > > + > > > + if (chip == NULL) > > > + chip = to_tpm_chip(chip); > > > + > > > + return chip; > > > +} > > > > If symlinks don't work out, we should probably just set the drvdata on > > the tpm_chip itself to avoid this. > > I'll experiment with this. Thanks for the comment. > > > > + if (!(chip->flags & TPM_CHIP_FLAG_PPI)) > > > + return -EINVAL; > > > > Hum, I don't think the PPI files should be created if there is no PPI > > support.. > > Again, following [1] by the book. And again, I think we could just as > well do our sysfs stuff in-between device_initialize() and device_add() > and get the non-racy behavior. I do not think it would be a bad idea to always create them when the kernel is compiled with CONFIG_ACPI. Maybe it would be abetter idea to return -ENOSYS? Device Model in the Linux kernel seems to recommend through the defaults APIs a flat set of attributes for each device node. > > > +void __init tpm_ppi_init(struct class *tpm_class) > > > +{ > > > + tpm_class->dev_groups = tpm_groups; > > > } > > > > So this shouldn't be unconditional. > > > > Also, ultimately PPI can't just claim the dev_groups, other parts of > > the driver will need to add groups too. > > > > I think it makes more sense to do > > > > struct attribute_group *tpm_ppi_get_sysfs(struct tpm_chip *chip) > > { > > } > > > > And take care of building the list in the caller. > > > > And tpm_ppi_get_sysfs should be called after the driver is readied but > > before adding the device. > > I don't think this would matter. Things could be refactored when more > sysfs attributes are needed. > > > Jason /Jarkko ------------------------------------------------------------------------------ BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT Develop your own process in accordance with the BPMN 2 standard Learn Process modeling best practices with Bonita BPM through live exercises http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_ source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF
On Wed, Apr 08, 2015 at 10:26:07AM +0300, Jarkko Sakkinen wrote: > On Wed, Apr 01, 2015 at 12:19:25PM -0600, Jason Gunthorpe wrote: > > On Wed, Apr 01, 2015 at 03:28:52PM +0300, Jarkko Sakkinen wrote: > > > Added PPI interface to the character device. PPI interface is also kept > > > in the pdev for backwards compatibility. > > > > Could you look at just completely moving the PPI interface to the char > > dev and then adding a symlink from the pdev? That would be really > > ideal. > > > > symlinks have the advantage that they actually fully fix the lifetime > > issues. > > > > This seems doable, if we replace the ppi_attrs group with a bunch of > > calls to sysfs_create_link it should work ? > > If we follow the pattern in [1] by the book, how would you use > sysfs_create_link()? To be more specific, how would you get the driver > core to create the symlinks for you? The driver core does not create symlinks, it creates the real files, which is the tpm_class->dev_groups part of your patch. That is fine.. The symlinks replace the broken legacy files under the platform_device. These are already racy, and different versions of the kernel have had different kind of races here. It wasn't until your 'tpm: fix call order in tpm-chip.c' that the ordering here started to make any kind of sense. So, I'm inclined to say the legacy paths don't much matter. They have rarely worked race free so nothing out there can be depending on them. I'd rather see the legacy paths be turned into symlinkes because that means we can close the use-after-free oops possibility the current code has, and that is a more serious bug than the user space race which has always existed anyhow. > If we decide not to follow [1] by the book, then this might be doable > (thinking off my head, that's the reason why I use *might be* instead > of *is*). Wouldn't we get non-racy behavior if sysfs_create_link()'s > are executed after device_initialize() and before device_add()? That would at least preserve the latest semantic that the uevent is created after all the sysfs is in place. It is the best we can do. Since this seems to address the race problem why do you think this is not worthwhile? > > > + if (!(chip->flags & TPM_CHIP_FLAG_PPI)) > > > + return -EINVAL; > > > > Hum, I don't think the PPI files should be created if there is no PPI > > support.. > > Again, following [1] by the book. And again, I think we could just as > well do our sysfs stuff in-between device_initialize() and device_add() > and get the non-racy behavior. Not relying on the class default seems reasonable for ppi to me. > I do not think it would be a bad idea to always create them when the > kernel is compiled with CONFIG_ACPI. Maybe it would be abetter idea to > return -ENOSYS? This is not really consistent with other uses of sysfs, user space tooling has a harder time detecting ENOSYS than it does a file that doesn't exist. It is also a change from the current PPI behavior, so I don't think we should do this without a very good reason. > Device Model in the Linux kernel seems to recommend > through the defaults APIs a flat set of attributes for each device > node. No, that is just the defaults scheme, there are other ways to create attributes that are conditional based on device capabilities. Greg notes: > Sometimes you don't have control over the driver either, or want > different sysfs files for different devices controlled by your driver ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ drivers/firewire/core-device.c has an example of this, the config_rom_attributes are pruned to only expose the ones that actually exist using the struct device groups scheme. Jason ------------------------------------------------------------------------------ BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT Develop your own process in accordance with the BPMN 2 standard Learn Process modeling best practices with Bonita BPM through live exercises http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_ source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF
On Wed, 2015-04-08 at 10:28 -0600, Jason Gunthorpe wrote: > On Wed, Apr 08, 2015 at 10:26:07AM +0300, Jarkko Sakkinen wrote: > > On Wed, Apr 01, 2015 at 12:19:25PM -0600, Jason Gunthorpe wrote: > > > On Wed, Apr 01, 2015 at 03:28:52PM +0300, Jarkko Sakkinen wrote: > > > > Added PPI interface to the character device. PPI interface is also kept > > > > in the pdev for backwards compatibility. > > > > > > Could you look at just completely moving the PPI interface to the char > > > dev and then adding a symlink from the pdev? That would be really > > > ideal. > > > > > > symlinks have the advantage that they actually fully fix the lifetime > > > issues. > > > > > > This seems doable, if we replace the ppi_attrs group with a bunch of > > > calls to sysfs_create_link it should work ? > > > > If we follow the pattern in [1] by the book, how would you use > > sysfs_create_link()? To be more specific, how would you get the driver > > core to create the symlinks for you? > > The driver core does not create symlinks, it creates the real files, > which is the tpm_class->dev_groups part of your patch. That is fine.. > > The symlinks replace the broken legacy files under the > platform_device. These are already racy, and different versions of the > kernel have had different kind of races here. It wasn't until your > 'tpm: fix call order in tpm-chip.c' that the ordering here started to > make any kind of sense. > > So, I'm inclined to say the legacy paths don't much matter. They have > rarely worked race free so nothing out there can be depending on > them. > > I'd rather see the legacy paths be turned into symlinkes because that > means we can close the use-after-free oops possibility the current > code has, and that is a more serious bug than the user space race > which has always existed anyhow. OK, I'll consider doing this for the next iteration. > > If we decide not to follow [1] by the book, then this might be doable > > (thinking off my head, that's the reason why I use *might be* instead > > of *is*). Wouldn't we get non-racy behavior if sysfs_create_link()'s > > are executed after device_initialize() and before device_add()? > > That would at least preserve the latest semantic that the uevent is > created after all the sysfs is in place. It is the best we can > do. > > Since this seems to address the race problem why do you think this is > not worthwhile? > > > > > + if (!(chip->flags & TPM_CHIP_FLAG_PPI)) > > > > + return -EINVAL; > > > > > > Hum, I don't think the PPI files should be created if there is no PPI > > > support.. > > > > Again, following [1] by the book. And again, I think we could just as > > well do our sysfs stuff in-between device_initialize() and device_add() > > and get the non-racy behavior. > > Not relying on the class default seems reasonable for ppi to me. > > > I do not think it would be a bad idea to always create them when the > > kernel is compiled with CONFIG_ACPI. Maybe it would be abetter idea to > > return -ENOSYS? > > This is not really consistent with other uses of sysfs, user space > tooling has a harder time detecting ENOSYS than it does a file that > doesn't exist. > > It is also a change from the current PPI behavior, so I don't think we > should do this without a very good reason. Agreed. > > Device Model in the Linux kernel seems to recommend > > through the defaults APIs a flat set of attributes for each device > > node. > > No, that is just the defaults scheme, there are other ways to > create attributes that are conditional based on device capabilities. > > Greg notes: > > > Sometimes you don't have control over the driver either, or want > > different sysfs files for different devices controlled by your driver > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > drivers/firewire/core-device.c has an example of this, the > config_rom_attributes are pruned to only expose the ones that actually > exist using the struct device groups scheme. Thanks for noting this. > Jason OK based on this discussion I'll iterate the patch. /Jarkko ------------------------------------------------------------------------------ BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT Develop your own process in accordance with the BPMN 2 standard Learn Process modeling best practices with Bonita BPM through live exercises http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_ source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF
diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c index 283f00a..44f39be 100644 --- a/drivers/char/tpm/tpm-chip.c +++ b/drivers/char/tpm/tpm-chip.c @@ -181,12 +181,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 +194,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,6 +216,10 @@ int tpm_chip_register(struct tpm_chip *chip) if (rc) return rc; + rc = tpm_add_ppi(chip); + if (rc) + goto out_err; + rc = tpm_dev_add_device(chip); if (rc) goto out_err; @@ -237,6 +233,7 @@ int tpm_chip_register(struct tpm_chip *chip) return 0; out_err: + tpm_remove_ppi(chip); tpm1_chip_unregister(chip); return rc; } @@ -262,7 +259,8 @@ void tpm_chip_unregister(struct tpm_chip *chip) spin_unlock(&driver_lock); synchronize_rcu(); - tpm1_chip_unregister(chip); tpm_dev_del_device(chip); + tpm_remove_ppi(chip); + tpm1_chip_unregister(chip); } EXPORT_SYMBOL_GPL(tpm_chip_unregister); diff --git a/drivers/char/tpm/tpm-interface.c b/drivers/char/tpm/tpm-interface.c index e85d341..aa813ff 100644 --- a/drivers/char/tpm/tpm-interface.c +++ b/drivers/char/tpm/tpm-interface.c @@ -1038,6 +1038,7 @@ static int __init tpm_init(void) return rc; } + tpm_ppi_init(tpm_class); return 0; } diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h index f8319a0..65872a5 100644 --- a/drivers/char/tpm/tpm.h +++ b/drivers/char/tpm/tpm.h @@ -182,7 +182,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) { @@ -411,6 +411,7 @@ void tpm_sysfs_del_device(struct tpm_chip *chip); int tpm_pcr_read_dev(struct tpm_chip *chip, int pcr_idx, u8 *res_buf); +void tpm_ppi_init(struct class *tpm_class); #ifdef CONFIG_ACPI extern int tpm_add_ppi(struct tpm_chip *chip); extern void tpm_remove_ppi(struct tpm_chip *chip); diff --git a/drivers/char/tpm/tpm_ppi.c b/drivers/char/tpm/tpm_ppi.c index 6ca9b5d..517a8b2 100644 --- a/drivers/char/tpm/tpm_ppi.c +++ b/drivers/char/tpm/tpm_ppi.c @@ -50,10 +50,23 @@ tpm_eval_dsm(acpi_handle ppi_handle, int func, acpi_object_type type, func, argv4, type); } +static struct tpm_chip *ppi_dev_to_chip(struct device *dev) +{ + struct tpm_chip *chip = dev_get_drvdata(dev); + + if (chip == NULL) + chip = to_tpm_chip(chip); + + return chip; +} + 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 = ppi_dev_to_chip(dev); + + if (!(chip->flags & TPM_CHIP_FLAG_PPI)) + return -EINVAL; return scnprintf(buf, PAGE_SIZE, "%s\n", chip->ppi_version); } @@ -63,7 +76,10 @@ 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 = ppi_dev_to_chip(dev); + + if (!(chip->flags & TPM_CHIP_FLAG_PPI)) + return -EINVAL; obj = tpm_eval_dsm(chip->acpi_dev_handle, TPM_PPI_FN_GETREQ, ACPI_TYPE_PACKAGE, NULL); @@ -100,7 +116,10 @@ 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 = ppi_dev_to_chip(dev); + + if (!(chip->flags & TPM_CHIP_FLAG_PPI)) + return -EINVAL; /* * the function to submit TPM operation request to pre-os environment @@ -156,7 +175,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 = ppi_dev_to_chip(dev); static char *info[] = { "None", @@ -166,6 +185,9 @@ static ssize_t tpm_show_ppi_transition_action(struct device *dev, "Error", }; + if (!(chip->flags & TPM_CHIP_FLAG_PPI)) + return -EINVAL; + /* * PPI spec defines params[3].type as empty package, but some platforms * (e.g. Capella with PPI 1.0) need integer/string/buffer type, so for @@ -197,7 +219,10 @@ 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 = ppi_dev_to_chip(dev); + + if (!(chip->flags & TPM_CHIP_FLAG_PPI)) + return -EINVAL; obj = tpm_eval_dsm(chip->acpi_dev_handle, TPM_PPI_FN_GETRSP, ACPI_TYPE_PACKAGE, NULL); @@ -251,7 +276,7 @@ cleanup: return status; } -static ssize_t show_ppi_operations(acpi_handle dev_handle, char *buf, u32 start, +static ssize_t show_ppi_operations(struct tpm_chip *chip, char *buf, u32 start, u32 end) { int i; @@ -268,14 +293,17 @@ static ssize_t show_ppi_operations(acpi_handle dev_handle, char *buf, u32 start, "User not required", }; - if (!acpi_check_dsm(dev_handle, tpm_ppi_uuid, TPM_PPI_REVISION_ID, - 1 << TPM_PPI_FN_GETOPR)) + if (!(chip->flags & TPM_CHIP_FLAG_PPI)) + return -EINVAL; + + if (!acpi_check_dsm(chip->acpi_dev_handle, tpm_ppi_uuid, + TPM_PPI_REVISION_ID, 1 << TPM_PPI_FN_GETOPR)) return -EPERM; tmp.integer.type = ACPI_TYPE_INTEGER; for (i = start; i <= end; i++) { tmp.integer.value = i; - obj = tpm_eval_dsm(dev_handle, TPM_PPI_FN_GETOPR, + obj = tpm_eval_dsm(chip->acpi_dev_handle, TPM_PPI_FN_GETOPR, ACPI_TYPE_INTEGER, &argv); if (!obj) { return -ENOMEM; @@ -296,22 +324,57 @@ 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 = ppi_dev_to_chip(dev); - return show_ppi_operations(chip->acpi_dev_handle, buf, 0, - PPI_TPM_REQ_MAX); + if (!(chip->flags & TPM_CHIP_FLAG_PPI)) + return -EINVAL; + + return show_ppi_operations(chip, buf, 0, PPI_TPM_REQ_MAX); } 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 = ppi_dev_to_chip(dev); + + if (!(chip->flags & TPM_CHIP_FLAG_PPI)) + return -EINVAL; + + return show_ppi_operations(chip, buf, PPI_VS_REQ_START, PPI_VS_REQ_END); +} + +static DEVICE_ATTR(ppi_version, S_IRUGO, tpm_show_ppi_version, NULL); +static DEVICE_ATTR(ppi_request, S_IRUGO | S_IWUSR | S_IWGRP, + tpm_show_ppi_request, tpm_store_ppi_request); +static DEVICE_ATTR(ppi_transition_action, S_IRUGO, + tpm_show_ppi_transition_action, NULL); +static DEVICE_ATTR(ppi_response, S_IRUGO, tpm_show_ppi_response, NULL); +static DEVICE_ATTR(ppi_tcg_operations, S_IRUGO, tpm_show_ppi_tcg_operations, + NULL); +static DEVICE_ATTR(ppi_vs_operations, S_IRUGO, tpm_show_ppi_vs_operations, + NULL); + +static struct attribute *tpm_attrs[] = { + &dev_attr_ppi_version.attr, + &dev_attr_ppi_request.attr, + &dev_attr_ppi_transition_action.attr, + &dev_attr_ppi_response.attr, + &dev_attr_ppi_tcg_operations.attr, + &dev_attr_ppi_vs_operations.attr, + NULL, +}; +ATTRIBUTE_GROUPS(tpm); - return show_ppi_operations(chip->acpi_dev_handle, buf, PPI_VS_REQ_START, - PPI_VS_REQ_END); +void __init tpm_ppi_init(struct class *tpm_class) +{ + tpm_class->dev_groups = tpm_groups; } +/* Legacy attributes and operations for backwards compatibility. These are + * only for TPM 1.x and are attached to the platform device. + */ + static DEVICE_ATTR(version, S_IRUGO, tpm_show_ppi_version, NULL); static DEVICE_ATTR(request, S_IRUGO | S_IWUSR | S_IWGRP, tpm_show_ppi_request, tpm_store_ppi_request); @@ -327,8 +390,10 @@ static struct attribute *ppi_attrs[] = { &dev_attr_transition_action.attr, &dev_attr_response.attr, &dev_attr_tcg_operations.attr, - &dev_attr_vs_operations.attr, NULL, + &dev_attr_vs_operations.attr, + NULL, }; + static struct attribute_group ppi_attr_grp = { .name = "ppi", .attrs = ppi_attrs @@ -337,7 +402,7 @@ static struct attribute_group ppi_attr_grp = { int tpm_add_ppi(struct tpm_chip *chip) { union acpi_object *obj; - int rc; + int rc = 0; if (!chip->acpi_dev_handle) return 0; @@ -356,7 +421,8 @@ int tpm_add_ppi(struct tpm_chip *chip) ACPI_FREE(obj); } - rc = sysfs_create_group(&chip->pdev->kobj, &ppi_attr_grp); + if (!(chip->flags & TPM_CHIP_FLAG_TPM2)) + rc = sysfs_create_group(&chip->pdev->kobj, &ppi_attr_grp); if (!rc) chip->flags |= TPM_CHIP_FLAG_PPI; @@ -366,6 +432,9 @@ int tpm_add_ppi(struct tpm_chip *chip) void tpm_remove_ppi(struct tpm_chip *chip) { + if (chip->flags & TPM_CHIP_FLAG_TPM2) + return; + if (chip->flags & TPM_CHIP_FLAG_PPI) sysfs_remove_group(&chip->pdev->kobj, &ppi_attr_grp); }
Added PPI interface to the character device. PPI interface is also kept in the pdev for backwards compatibility. Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- drivers/char/tpm/tpm-chip.c | 16 +++--- drivers/char/tpm/tpm-interface.c | 1 + drivers/char/tpm/tpm.h | 3 +- drivers/char/tpm/tpm_ppi.c | 105 ++++++++++++++++++++++++++++++++------- 4 files changed, 97 insertions(+), 28 deletions(-)