diff mbox

[tpmdd-devel,04/10] tpm: move the PPI attributes to character device directory.

Message ID 1445020843-9382-5-git-send-email-jarkko.sakkinen@linux.intel.com
State Awaiting Upstream
Headers show

Commit Message

Jarkko Sakkinen Oct. 16, 2015, 6:40 p.m. UTC
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 <jarkko.sakkinen@linux.intel.com>
Reviewed-by: Jason Gunthorpe <jason.gunthorpe@obsidianresearch.com>
Tested-by: Mimi Zohar <zohar@linux.vnet.ibm.com> (on TPM 1.2)
Tested-by: Chris J Arges <chris.j.arges@canonical.com>
Tested-by: Colin Ian King <colin.king@canonical.com>
---
 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(-)

Comments

Jeremiah Mahler Nov. 4, 2015, 6:17 p.m. UTC | #1
Jarkko, all,

On Fri, Oct 16, 2015 at 09:40:23PM +0300, Jarkko Sakkinen wrote:
> 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 <jarkko.sakkinen@linux.intel.com>
> Reviewed-by: Jason Gunthorpe <jason.gunthorpe@obsidianresearch.com>
> Tested-by: Mimi Zohar <zohar@linux.vnet.ibm.com> (on TPM 1.2)
> Tested-by: Chris J Arges <chris.j.arges@canonical.com>
> Tested-by: Colin Ian King <colin.king@canonical.com>
> ---
>  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 1082d4b..f26b0ae 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);
> @@ -182,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;
> @@ -201,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);
>  }
>  
> @@ -225,10 +220,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 = __compat_only_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);
> @@ -263,6 +268,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 39be5ac..36ceb71 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -158,8 +158,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 {
> @@ -182,6 +181,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 */
> @@ -189,7 +190,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)
>  {
> @@ -419,15 +420,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.5.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

The commit for this patch (9b774d5cf2db4) present in the latest
linux-next (20151101+) breaks suspend/resume on an Acer C720 Chromebook.
The computer will successfully suspend but when a resume is attempted
a blank screen is displayed for a few seconds and then it reboots.
Jarkko Sakkinen Nov. 5, 2015, 9:22 a.m. UTC | #2
On Wed, Nov 04, 2015 at 10:17:05AM -0800, Jeremiah Mahler wrote:
> Jarkko, all,
> 
> On Fri, Oct 16, 2015 at 09:40:23PM +0300, Jarkko Sakkinen wrote:
> > 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 <jarkko.sakkinen@linux.intel.com>
> > Reviewed-by: Jason Gunthorpe <jason.gunthorpe@obsidianresearch.com>
> > Tested-by: Mimi Zohar <zohar@linux.vnet.ibm.com> (on TPM 1.2)
> > Tested-by: Chris J Arges <chris.j.arges@canonical.com>
> > Tested-by: Colin Ian King <colin.king@canonical.com>
> > ---
> >  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 1082d4b..f26b0ae 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);
> > @@ -182,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;
> > @@ -201,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);
> >  }
> >  
> > @@ -225,10 +220,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 = __compat_only_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);
> > @@ -263,6 +268,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 39be5ac..36ceb71 100644
> > --- a/drivers/char/tpm/tpm.h
> > +++ b/drivers/char/tpm/tpm.h
> > @@ -158,8 +158,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 {
> > @@ -182,6 +181,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 */
> > @@ -189,7 +190,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)
> >  {
> > @@ -419,15 +420,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.5.0
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> 
> The commit for this patch (9b774d5cf2db4) present in the latest
> linux-next (20151101+) breaks suspend/resume on an Acer C720 Chromebook.
> The computer will successfully suspend but when a resume is attempted
> a blank screen is displayed for a few seconds and then it reboots.

I think I have this same model at home (have to check). If it is, I can
try to reproduce it today when I get back to home.

What kind of environment are you running?
Are you able to acquire kernel logs?

> -- 
> - Jeremiah Mahler

/Jarkko

------------------------------------------------------------------------------
Jarkko Sakkinen Nov. 5, 2015, 11:05 a.m. UTC | #3
On Thu, Nov 05, 2015 at 11:22:55AM +0200, Jarkko Sakkinen wrote:
> On Wed, Nov 04, 2015 at 10:17:05AM -0800, Jeremiah Mahler wrote:
> > Jarkko, all,
> > 
> > On Fri, Oct 16, 2015 at 09:40:23PM +0300, Jarkko Sakkinen wrote:
> > > 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 <jarkko.sakkinen@linux.intel.com>
> > > Reviewed-by: Jason Gunthorpe <jason.gunthorpe@obsidianresearch.com>
> > > Tested-by: Mimi Zohar <zohar@linux.vnet.ibm.com> (on TPM 1.2)
> > > Tested-by: Chris J Arges <chris.j.arges@canonical.com>
> > > Tested-by: Colin Ian King <colin.king@canonical.com>
> > > ---
> > >  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 1082d4b..f26b0ae 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);
> > > @@ -182,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;
> > > @@ -201,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);
> > >  }
> > >  
> > > @@ -225,10 +220,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 = __compat_only_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);
> > > @@ -263,6 +268,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 39be5ac..36ceb71 100644
> > > --- a/drivers/char/tpm/tpm.h
> > > +++ b/drivers/char/tpm/tpm.h
> > > @@ -158,8 +158,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 {
> > > @@ -182,6 +181,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 */
> > > @@ -189,7 +190,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)
> > >  {
> > > @@ -419,15 +420,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.5.0
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > > Please read the FAQ at  http://www.tux.org/lkml/
> > 
> > The commit for this patch (9b774d5cf2db4) present in the latest
> > linux-next (20151101+) breaks suspend/resume on an Acer C720 Chromebook.
> > The computer will successfully suspend but when a resume is attempted
> > a blank screen is displayed for a few seconds and then it reboots.
> 
> I think I have this same model at home (have to check). If it is, I can
> try to reproduce it today when I get back to home.

Yup, it's C720P!

> What kind of environment are you running?
> Are you able to acquire kernel logs?

Both of these questions still apply. Even if I create the same
environment, your logs might have something useful embedded.

Thank you for reporting this issue! I'll try to find a way to reproduce
and fix it at latest for -rc2.

> > -- 
> > - Jeremiah Mahler

/Jarkko

------------------------------------------------------------------------------
Jeremiah Mahler Nov. 5, 2015, 4:47 p.m. UTC | #4
Jarkko,

On Thu, Nov 05, 2015 at 01:05:45PM +0200, Jarkko Sakkinen wrote:
> On Thu, Nov 05, 2015 at 11:22:55AM +0200, Jarkko Sakkinen wrote:
> > On Wed, Nov 04, 2015 at 10:17:05AM -0800, Jeremiah Mahler wrote:
> > > Jarkko, all,
> > > 
[...]
> > > 
> > > The commit for this patch (9b774d5cf2db4) present in the latest
> > > linux-next (20151101+) breaks suspend/resume on an Acer C720 Chromebook.
> > > The computer will successfully suspend but when a resume is attempted
> > > a blank screen is displayed for a few seconds and then it reboots.
> > 
> > I think I have this same model at home (have to check). If it is, I can
> > try to reproduce it today when I get back to home.
> 
> Yup, it's C720P!
> 
> > What kind of environment are you running?
> > Are you able to acquire kernel logs?
> 
> Both of these questions still apply. Even if I create the same
> environment, your logs might have something useful embedded.
> 
> Thank you for reporting this issue! I'll try to find a way to reproduce
> and fix it at latest for -rc2.
> 
> > > -- 
> > > - Jeremiah Mahler
> 
> /Jarkko

I have attached the full dmesg.  The following snippets looked
particularly interesting.

  ...
  [    2.423070] input: PC Speaker as /devices/platform/pcspkr/input/input10
  [    2.453071] ------------[ cut here ]------------
  [    2.453078] WARNING: CPU: 0 PID: 237 at kernel/irq/manage.c:1379 __free_irq+0x90/0x1e0()
  [    2.453079] Trying to free already-free IRQ 6
  [    2.453110] Modules linked in: evdev serio_raw pcspkr cfg80211 sg i915 lpc_ich mfd_core i2c_i801 battery ac snd_hda_codec_realtek ath3k snd_hda_codec_generic i2c_algo_bit btusb drm_kms_helper btrtl btbcm btintel snd_hda_intel bluetooth drm snd_hda_codec snd_hwdep snd_hda_core video snd_pcm rfkill tpm_tis(+) snd_timer tpm snd button i2c_designware_pci i2c_designware_core shpchp i2c_core soundcore autofs4 ext4 crc16 mbcache jbd2 sd_mod ahci libahci fan sdhci_acpi sdhci libata mmc_core xhci_pci scsi_mod xhci_hcd thermal usbcore usb_common
  [    2.453113] CPU: 0 PID: 237 Comm: systemd-udevd Tainted: G        W       4.3.0-rc4+ #271
  [    2.453114] Hardware name: Acer Peppy, BIOS          04/30/2014
  [    2.453117]  ffffffff817288a1 ffffffff812c37a7 ffff880035a6fad0 ffffffff8106d8bd
  [    2.453119]  ffff880100421800 ffff880035a6fb20 ffff880100421800 0000000000000006
  [    2.453121]  ffff8801004218d8 ffffffff8106d93c ffffffff8171e8c8 ffff880000000020
  [    2.453122] Call Trace:
  [    2.453127]  [<ffffffff812c37a7>] ? dump_stack+0x40/0x59
  [    2.453132]  [<ffffffff8106d8bd>] ? warn_slowpath_common+0x7d/0xb0
  [    2.453135]  [<ffffffff8106d93c>] ? warn_slowpath_fmt+0x4c/0x50
  [    2.453140]  [<ffffffffa01dce22>] ? tpm_chip_register+0x142/0x190 [tpm]
  [    2.453142]  [<ffffffff810bd780>] ? __free_irq+0x90/0x1e0
  [    2.453144]  [<ffffffff810bd965>] ? free_irq+0x45/0xb0
  [    2.453148]  [<ffffffff813d8675>] ? release_nodes+0xf5/0x1c0
  [    2.453152]  [<ffffffff813d4c3d>] ? driver_probe_device+0xcd/0x480
  [    2.453155]  [<ffffffff813d506b>] ? __driver_attach+0x7b/0x80
  [    2.453158]  [<ffffffff813d4ff0>] ? driver_probe_device+0x480/0x480
  [    2.453161]  [<ffffffff813d2c4a>] ? bus_for_each_dev+0x5a/0x90
  [    2.453164]  [<ffffffff813d413f>] ? bus_add_driver+0x1df/0x270
  [    2.453166]  [<ffffffffa00d1000>] ? 0xffffffffa00d1000
  [    2.453169]  [<ffffffff813d5807>] ? driver_register+0x57/0xc0
  [    2.453173]  [<ffffffffa00d1025>] ? init_tis+0x25/0x1000 [tpm_tis]
  [    2.453178]  [<ffffffff811510c9>] ? free_pcppages_bulk+0xc9/0x450
  [    2.453179]  [<ffffffffa00d1000>] ? 0xffffffffa00d1000
  [    2.453182]  [<ffffffff81002122>] ? do_one_initcall+0xb2/0x200
  [    2.453185]  [<ffffffff8114953d>] ? do_init_module+0x5b/0x1dc
  [    2.453188]  [<ffffffff810e5637>] ? load_module+0x2197/0x27a0
  [    2.453190]  [<ffffffff810e1ed0>] ? __symbol_put+0x30/0x30
  [    2.453194]  [<ffffffff810e5e20>] ? SyS_finit_module+0x90/0xc0
  [    2.453198]  [<ffffffff815360b6>] ? entry_SYSCALL_64_fastpath+0x16/0x75
  [    2.453200] ---[ end trace 0835dfb15879b441 ]---
  [    2.453212] tpm_tis: probe of 00:08 failed with error -2
  [    2.456012] tpm_inf_pnp 00:08: Found TPM with ID IFX0102
  [    2.544707] usb 1-4: new full-speed USB device number 4 using xhci_hcd
  [    2.571036] __add_probed_i2c_device failed to register device 1-4a
  ...

Notice the "tpm_tis: probe of ... failed with error".  With a working
kernel this probe succeeds.  However, the probe fails, which causes it
to try unload the module which produces the "free already-free IRQ"
trace.  The IRQ message is a secondary issue.  Interrupts were disabled
due to a firmware bug and it is trying to free them anyway.

  ...
  [    2.244779] tpm_tis 00:08: [Firmware Bug]: TPM interrupt not working, polling instead
  ...

So the patch changed something which causes the probe to fail.  I
haven't figured out what that is yet.
Jarkko Sakkinen Nov. 5, 2015, 5:46 p.m. UTC | #5
On Thu, Nov 05, 2015 at 08:47:58AM -0800, Jeremiah Mahler wrote:
> Jarkko,
> 
> On Thu, Nov 05, 2015 at 01:05:45PM +0200, Jarkko Sakkinen wrote:
> > On Thu, Nov 05, 2015 at 11:22:55AM +0200, Jarkko Sakkinen wrote:
> > > On Wed, Nov 04, 2015 at 10:17:05AM -0800, Jeremiah Mahler wrote:
> > > > Jarkko, all,
> > > > 
> [...]
> > > > 
> > > > The commit for this patch (9b774d5cf2db4) present in the latest
> > > > linux-next (20151101+) breaks suspend/resume on an Acer C720 Chromebook.
> > > > The computer will successfully suspend but when a resume is attempted
> > > > a blank screen is displayed for a few seconds and then it reboots.
> > > 
> > > I think I have this same model at home (have to check). If it is, I can
> > > try to reproduce it today when I get back to home.
> > 
> > Yup, it's C720P!
> > 
> > > What kind of environment are you running?
> > > Are you able to acquire kernel logs?
> > 
> > Both of these questions still apply. Even if I create the same
> > environment, your logs might have something useful embedded.
> > 
> > Thank you for reporting this issue! I'll try to find a way to reproduce
> > and fix it at latest for -rc2.
> > 
> > > > -- 
> > > > - Jeremiah Mahler
> > 
> > /Jarkko
> 
> I have attached the full dmesg.  The following snippets looked
> particularly interesting.

I'll look at the logs with time tomorrow. What kind of distribution you
have in your system? Can you also send your kernel config?

I have the same laptop at hand and would like to setup exactly the same
environment.

/Jarkko

------------------------------------------------------------------------------
Jeremiah Mahler Nov. 5, 2015, 6:17 p.m. UTC | #6
Jarkko,

On Thu, Nov 05, 2015 at 07:46:30PM +0200, Jarkko Sakkinen wrote:
> On Thu, Nov 05, 2015 at 08:47:58AM -0800, Jeremiah Mahler wrote:
> > Jarkko,
> > 
> > On Thu, Nov 05, 2015 at 01:05:45PM +0200, Jarkko Sakkinen wrote:
> > > On Thu, Nov 05, 2015 at 11:22:55AM +0200, Jarkko Sakkinen wrote:
> > > > On Wed, Nov 04, 2015 at 10:17:05AM -0800, Jeremiah Mahler wrote:
> > > > > Jarkko, all,
> > > > > 
> > [...]
> > > > > 
> > > > > The commit for this patch (9b774d5cf2db4) present in the latest
> > > > > linux-next (20151101+) breaks suspend/resume on an Acer C720 Chromebook.
> > > > > The computer will successfully suspend but when a resume is attempted
> > > > > a blank screen is displayed for a few seconds and then it reboots.
> > > > 
> > > > I think I have this same model at home (have to check). If it is, I can
> > > > try to reproduce it today when I get back to home.
> > > 
> > > Yup, it's C720P!
> > > 
> > > > What kind of environment are you running?
> > > > Are you able to acquire kernel logs?
> > > 
> > > Both of these questions still apply. Even if I create the same
> > > environment, your logs might have something useful embedded.
> > > 
> > > Thank you for reporting this issue! I'll try to find a way to reproduce
> > > and fix it at latest for -rc2.
> > > 
> > > > > -- 
> > > > > - Jeremiah Mahler
> > > 
> > > /Jarkko
> > 
> > I have attached the full dmesg.  The following snippets looked
> > particularly interesting.
> 
> I'll look at the logs with time tomorrow. What kind of distribution you
> have in your system? Can you also send your kernel config?
> 
I'm running Debian unstable, not sure of the exact version.
Attached is my current .config.

> I have the same laptop at hand and would like to setup exactly the same
> environment.
> 
> /Jarkko

The only difference is that yours is a C720P and mine is a C720.
Hopefully they are similar enough.
Jarkko Sakkinen Nov. 6, 2015, 1:45 p.m. UTC | #7
On Thu, Nov 05, 2015 at 10:17:55AM -0800, Jeremiah Mahler wrote:
> Jarkko,
> 
> On Thu, Nov 05, 2015 at 07:46:30PM +0200, Jarkko Sakkinen wrote:
> > On Thu, Nov 05, 2015 at 08:47:58AM -0800, Jeremiah Mahler wrote:
> > > Jarkko,
> > > 
> > > On Thu, Nov 05, 2015 at 01:05:45PM +0200, Jarkko Sakkinen wrote:
> > > > On Thu, Nov 05, 2015 at 11:22:55AM +0200, Jarkko Sakkinen wrote:
> > > > > On Wed, Nov 04, 2015 at 10:17:05AM -0800, Jeremiah Mahler wrote:
> > > > > > Jarkko, all,
> > > > > > 
> > > [...]
> > > > > > 
> > > > > > The commit for this patch (9b774d5cf2db4) present in the latest
> > I have the same laptop at hand and would like to setup exactly the same
> > environment.
> > 
> > /Jarkko
> 
> The only difference is that yours is a C720P and mine is a C720.
> Hopefully they are similar enough.

The only difference is AFAIK touch screen. I had the laptop lost but
found it today. I'm going to work on this during the weekend with stable
Debian and see if I can reproduce it.

> -- 
> - Jeremiah Mahler

/Jarkko

------------------------------------------------------------------------------
Jeremiah Mahler Nov. 7, 2015, 2:54 a.m. UTC | #8
Jarkko,

On Wed, Nov 04, 2015 at 10:17:05AM -0800, Jeremiah Mahler wrote:
> Jarkko, all,
> 
> On Fri, Oct 16, 2015 at 09:40:23PM +0300, Jarkko Sakkinen wrote:
> > 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 <jarkko.sakkinen@linux.intel.com>
> > Reviewed-by: Jason Gunthorpe <jason.gunthorpe@obsidianresearch.com>
> > Tested-by: Mimi Zohar <zohar@linux.vnet.ibm.com> (on TPM 1.2)
> > Tested-by: Chris J Arges <chris.j.arges@canonical.com>
> > Tested-by: Colin Ian King <colin.king@canonical.com>
> > ---
> >  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(-)
> > 
[...]
> > @@ -225,10 +220,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 = __compat_only_sysfs_link_entry_to_kobj(&chip->pdev->kobj,
> > +							    &chip->dev.kobj,
> > +							    "ppi");
> > +		if (rc)
> > +			goto out_err;
> > +	}
> > +

This new call to __compat_only_sysfs_link_entry_to_kobj fails on an Acer
C720 due to a failed call to kernfs_find_and_get (discussed in a second
message).

[...]
> 
> The commit for this patch (9b774d5cf2db4) present in the latest
> linux-next (20151101+) breaks suspend/resume on an Acer C720 Chromebook.
> The computer will successfully suspend but when a resume is attempted
> a blank screen is displayed for a few seconds and then it reboots.
> 
> -- 
> - Jeremiah Mahler
diff mbox

Patch

diff --git a/drivers/char/tpm/tpm-chip.c b/drivers/char/tpm/tpm-chip.c
index 1082d4b..f26b0ae 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);
@@ -182,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;
@@ -201,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);
 }
 
@@ -225,10 +220,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 = __compat_only_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);
@@ -263,6 +268,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 39be5ac..36ceb71 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -158,8 +158,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 {
@@ -182,6 +181,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 */
@@ -189,7 +190,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)
 {
@@ -419,15 +420,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;
 }