Re: [PATCH v3 3/7] tpm_tis: Do not fall back to a hardcoded address for TPM2

Re: [PATCH v3 3/7] tpm_tis: Do not fall back to a hardcoded address for TPM2

[Jarkko Sakkinen]

On Thu, Dec 17, 2015 at 11:23:16AM -0700, Jason Gunthorpe wrote:
> If the ACPI tables do not declare a memory resource for the TPM2
> then do not just fall back to the x86 default base address.
> 
> Also be stricter when checking the ancillary TPM2 ACPI data and error
> out if any of this data is wrong rather than blindly assuming TPM1.
> 
> Fixes: 399235dc6e95 ("tpm, tpm_tis: fix tpm_tis ACPI detection issue with TPM 2.0")
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Tested-by: Wilck, Martin <martin.wilck-RJz4owOZxyXQFUHtdCDX3A@public.gmane.org>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/char/tpm/tpm_tis.c | 48 +++++++++++++++++-----------------------------
>  1 file changed, 18 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index fecd27b45fd1..b2b31f5418ca 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -122,39 +122,11 @@ static inline int is_itpm(struct acpi_device *dev)
>  {
>  	return has_hid(dev, "INTC0102");
>  }
> -
> -static inline int is_fifo(struct acpi_device *dev)
> -{
> -	struct acpi_table_tpm2 *tbl;
> -	acpi_status st;
> -
> -	/* TPM 1.2 FIFO */
> -	if (!has_hid(dev, "MSFT0101"))
> -		return 1;
> -
> -	st = acpi_get_table(ACPI_SIG_TPM2, 1,
> -			    (struct acpi_table_header **) &tbl);
> -	if (ACPI_FAILURE(st)) {
> -		dev_err(&dev->dev, "failed to get TPM2 ACPI table\n");
> -		return 0;
> -	}
> -
> -	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
> -		return 0;
> -
> -	/* TPM 2.0 FIFO */
> -	return 1;
> -}
>  #else
>  static inline int is_itpm(struct acpi_device *dev)
>  {
>  	return 0;
>  }
> -
> -static inline int is_fifo(struct acpi_device *dev)
> -{
> -	return 1;
> -}
>  #endif
>  
>  /* Before we attempt to access the TPM we must see that the valid bit is set.
> @@ -980,11 +952,21 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data)
>  
>  static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
>  {
> +	struct acpi_table_tpm2 *tbl;
> +	acpi_status st;
>  	struct list_head resources;
> -	struct tpm_info tpm_info = tis_default_info;
> +	struct tpm_info tpm_info = {};
>  	int ret;
>  
> -	if (!is_fifo(acpi_dev))
> +	st = acpi_get_table(ACPI_SIG_TPM2, 1,
> +			    (struct acpi_table_header **) &tbl);
> +	if (ACPI_FAILURE(st) || tbl->header.length < sizeof(*tbl)) {
> +		dev_err(&acpi_dev->dev,
> +			FW_BUG "failed to get TPM2 ACPI table\n");
> +		return -EINVAL;
> +	}
> +
> +	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
>  		return -ENODEV;
>  
>  	INIT_LIST_HEAD(&resources);
> @@ -996,6 +978,12 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
>  
>  	acpi_dev_free_resource_list(&resources);
>  
> +	if (tpm_info.start == 0 && tpm_info.len == 0) {
> +		dev_err(&acpi_dev->dev,
> +			FW_BUG "TPM2 ACPI table does not define a memory resource\n");
> +		return -EINVAL;
> +	}
> +

I guess this the only relevant change in this patch? You should propose
removal of is_fifo() as a separate patch if that makes sense. This patch
is now doing orthogonal things.

>  	if (is_itpm(acpi_dev))
>  		itpm = true;
>  
> -- 
> 2.1.4

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH v3 3/7] tpm_tis: Do not fall back to a hardcoded address for TPM2

[Jason Gunthorpe]

On Fri, Dec 18, 2015 at 11:34:32AM +0200, Jarkko Sakkinen wrote:
> > +	st = acpi_get_table(ACPI_SIG_TPM2, 1,
> > +			    (struct acpi_table_header **) &tbl);
> > +	if (ACPI_FAILURE(st) || tbl->header.length < sizeof(*tbl)) {
> > +		dev_err(&acpi_dev->dev,
> > +			FW_BUG "failed to get TPM2 ACPI table\n");
> > +		return -EINVAL;
> > +	}
> > +
> > +	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
> >  		return -ENODEV;
> >  
> >  	INIT_LIST_HEAD(&resources);
> > @@ -996,6 +978,12 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
> >  
> >  	acpi_dev_free_resource_list(&resources);
> >  
> > +	if (tpm_info.start == 0 && tpm_info.len == 0) {
> > +		dev_err(&acpi_dev->dev,
> > +			FW_BUG "TPM2 ACPI table does not define a memory resource\n");
> > +		return -EINVAL;
> > +	}
> > +
> 
> I guess this the only relevant change in this patch? You should propose
> removal of is_fifo() as a separate patch if that makes sense. This patch
> is now doing orthogonal things.

No, the return code changes are relevant too, and are why is_fifo was
best un-inlined.

The patch is fixing all the ACPI data validatation in one go, not just
the resource range, the description notes this.

Jason

Re: [PATCH v3 3/7] tpm_tis: Do not fall back to a hardcoded address for TPM2

[Jarkko Sakkinen]

On Fri, Dec 18, 2015 at 09:51:27AM -0700, Jason Gunthorpe wrote:
> On Fri, Dec 18, 2015 at 11:34:32AM +0200, Jarkko Sakkinen wrote:
> > > +	st = acpi_get_table(ACPI_SIG_TPM2, 1,
> > > +			    (struct acpi_table_header **) &tbl);
> > > +	if (ACPI_FAILURE(st) || tbl->header.length < sizeof(*tbl)) {
> > > +		dev_err(&acpi_dev->dev,
> > > +			FW_BUG "failed to get TPM2 ACPI table\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > > +	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
> > >  		return -ENODEV;
> > >  
> > >  	INIT_LIST_HEAD(&resources);
> > > @@ -996,6 +978,12 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
> > >  
> > >  	acpi_dev_free_resource_list(&resources);
> > >  
> > > +	if (tpm_info.start == 0 && tpm_info.len == 0) {
> > > +		dev_err(&acpi_dev->dev,
> > > +			FW_BUG "TPM2 ACPI table does not define a memory resource\n");
> > > +		return -EINVAL;
> > > +	}
> > > +
> > 
> > I guess this the only relevant change in this patch? You should propose
> > removal of is_fifo() as a separate patch if that makes sense. This patch
> > is now doing orthogonal things.
> 
> No, the return code changes are relevant too, and are why is_fifo was
> best un-inlined.
> 
> The patch is fixing all the ACPI data validatation in one go, not just
> the resource range, the description notes this.

Got you. I think I'm good with this patch.

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

> Jason

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH v3 1/7] tpm_crb: Use the common ACPI definition of struct acpi_tpm2

[Jarkko Sakkinen]

On Thu, Dec 17, 2015 at 11:23:14AM -0700, Jason Gunthorpe wrote:
> include/acpi/actbl2.h is the proper place for these definitions
> and the needed TPM2 ones have been there since
> commit 413d4a6defe0 ("ACPICA: Update TPM2 ACPI table")
> 
> This also drops a couple of le32_to_cpu's for members of this table,
> the existing swapping was not done consistently, and the definitions
> in actbl2.h do not have endianness annotations, declaring that no swap
> is required. Note that the TPM ACPI spec defines all of these
> values to be little endian, both in crb2 and ppi.

I think this patch mixes two separate changes to the driver: removing
l32_to_cpu's and moving to common headers and that is not right.

Even if they should be removed I think what you say about actbl2.h is
wrong. Annotations are probably missing because it is imported code (I'm
happy to be corrected if this is not the case).

/Jarkko

> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Tested-by: Wilck, Martin <martin.wilck-RJz4owOZxyXQFUHtdCDX3A@public.gmane.org>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/char/tpm/tpm.h     |  7 -------
>  drivers/char/tpm/tpm_crb.c | 31 +++++++++----------------------
>  drivers/char/tpm/tpm_tis.c |  2 +-
>  3 files changed, 10 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index 542a80cbfd9c..28b477e8da6a 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -128,13 +128,6 @@ enum tpm2_startup_types {
>  	TPM2_SU_STATE	= 0x0001,
>  };
>  
> -enum tpm2_start_method {
> -	TPM2_START_ACPI = 2,
> -	TPM2_START_FIFO = 6,
> -	TPM2_START_CRB = 7,
> -	TPM2_START_CRB_WITH_ACPI = 8,
> -};
> -
>  struct tpm_chip;
>  
>  struct tpm_vendor_specific {
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 8342cf51ffdc..8dd70696ebe8 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -34,14 +34,6 @@ enum crb_defaults {
>  	CRB_ACPI_START_INDEX = 1,
>  };
>  
> -struct acpi_tpm2 {
> -	struct acpi_table_header hdr;
> -	u16 platform_class;
> -	u16 reserved;
> -	u64 control_area_pa;
> -	u32 start_method;
> -} __packed;
> -
>  enum crb_ca_request {
>  	CRB_CA_REQ_GO_IDLE	= BIT(0),
>  	CRB_CA_REQ_CMD_READY	= BIT(1),
> @@ -207,7 +199,7 @@ static const struct tpm_class_ops tpm_crb = {
>  static int crb_acpi_add(struct acpi_device *device)
>  {
>  	struct tpm_chip *chip;
> -	struct acpi_tpm2 *buf;
> +	struct acpi_table_tpm2 *buf;
>  	struct crb_priv *priv;
>  	struct device *dev = &device->dev;
>  	acpi_status status;
> @@ -217,13 +209,14 @@ static int crb_acpi_add(struct acpi_device *device)
>  
>  	status = acpi_get_table(ACPI_SIG_TPM2, 1,
>  				(struct acpi_table_header **) &buf);
> -	if (ACPI_FAILURE(status)) {
> -		dev_err(dev, "failed to get TPM2 ACPI table\n");
> +	if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
> +		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
>  		return -ENODEV;
>  	}
>  
>  	/* Should the FIFO driver handle this? */
> -	if (buf->start_method == TPM2_START_FIFO)
> +	sm = buf->start_method;
> +	if (sm == ACPI_TPM2_MEMORY_MAPPED)
>  		return -ENODEV;
>  
>  	chip = tpmm_chip_alloc(dev, &tpm_crb);
> @@ -232,11 +225,6 @@ static int crb_acpi_add(struct acpi_device *device)
>  
>  	chip->flags = TPM_CHIP_FLAG_TPM2;
>  
> -	if (buf->hdr.length < sizeof(struct acpi_tpm2)) {
> -		dev_err(dev, "TPM2 ACPI table has wrong size");
> -		return -EINVAL;
> -	}
> -
>  	priv = (struct crb_priv *) devm_kzalloc(dev, sizeof(struct crb_priv),
>  						GFP_KERNEL);
>  	if (!priv) {
> @@ -244,21 +232,20 @@ static int crb_acpi_add(struct acpi_device *device)
>  		return -ENOMEM;
>  	}
>  
> -	sm = le32_to_cpu(buf->start_method);
> -
>  	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
>  	 * report only ACPI start but in practice seems to require both
>  	 * ACPI start and CRB start.
>  	 */
> -	if (sm == TPM2_START_CRB || sm == TPM2_START_FIFO ||
> +	if (sm == ACPI_TPM2_COMMAND_BUFFER || sm == ACPI_TPM2_MEMORY_MAPPED ||
>  	    !strcmp(acpi_device_hid(device), "MSFT0101"))
>  		priv->flags |= CRB_FL_CRB_START;
>  
> -	if (sm == TPM2_START_ACPI || sm == TPM2_START_CRB_WITH_ACPI)
> +	if (sm == ACPI_TPM2_START_METHOD ||
> +	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
>  		priv->flags |= CRB_FL_ACPI_START;
>  
>  	priv->cca = (struct crb_control_area __iomem *)
> -		devm_ioremap_nocache(dev, buf->control_area_pa, 0x1000);
> +		devm_ioremap_nocache(dev, buf->control_address, 0x1000);
>  	if (!priv->cca) {
>  		dev_err(dev, "ioremap of the control area failed\n");
>  		return -ENOMEM;
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 8a3509cb10da..304323bdcaaa 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -135,7 +135,7 @@ static inline int is_fifo(struct acpi_device *dev)
>  		return 0;
>  	}
>  
> -	if (le32_to_cpu(tbl->start_method) != TPM2_START_FIFO)
> +	if (tbl->start_method != ACPI_TPM2_MEMORY_MAPPED)
>  		return 0;
>  
>  	/* TPM 2.0 FIFO */
> -- 
> 2.1.4
> 

------------------------------------------------------------------------------

Re: [PATCH v3 1/7] tpm_crb: Use the common ACPI definition of struct acpi_tpm2

[Jason Gunthorpe]

On Sun, Jan 03, 2016 at 07:09:06PM +0200, Jarkko Sakkinen wrote:
> On Thu, Dec 17, 2015 at 11:23:14AM -0700, Jason Gunthorpe wrote:
> > include/acpi/actbl2.h is the proper place for these definitions
> > and the needed TPM2 ones have been there since
> > commit 413d4a6defe0 ("ACPICA: Update TPM2 ACPI table")
> > 
> > This also drops a couple of le32_to_cpu's for members of this table,
> > the existing swapping was not done consistently, and the definitions
> > in actbl2.h do not have endianness annotations, declaring that no swap
> > is required. Note that the TPM ACPI spec defines all of these
> > values to be little endian, both in crb2 and ppi.
> 
> I think this patch mixes two separate changes to the driver: removing
> l32_to_cpu's and moving to common headers and that is not right.

No, it is only one change: Move to the common kernel way of working
with these structures.

> Even if they should be removed I think what you say about actbl2.h is
> wrong. Annotations are probably missing because it is imported code (I'm
> happy to be corrected if this is not the case).

Edit the comments if you like.

Annotations are missing because BE is not supported at all, nothing to
do with imported code.

Jason

------------------------------------------------------------------------------

Re: [PATCH v3 1/7] tpm_crb: Use the common ACPI definition of struct acpi_tpm2

[Jarkko Sakkinen]

On Mon, Jan 04, 2016 at 11:23:17AM -0700, Jason Gunthorpe wrote:
> On Sun, Jan 03, 2016 at 07:09:06PM +0200, Jarkko Sakkinen wrote:
> > On Thu, Dec 17, 2015 at 11:23:14AM -0700, Jason Gunthorpe wrote:
> > > include/acpi/actbl2.h is the proper place for these definitions
> > > and the needed TPM2 ones have been there since
> > > commit 413d4a6defe0 ("ACPICA: Update TPM2 ACPI table")
> > > 
> > > This also drops a couple of le32_to_cpu's for members of this table,
> > > the existing swapping was not done consistently, and the definitions
> > > in actbl2.h do not have endianness annotations, declaring that no swap
> > > is required. Note that the TPM ACPI spec defines all of these
> > > values to be little endian, both in crb2 and ppi.
> > 
> > I think this patch mixes two separate changes to the driver: removing
> > l32_to_cpu's and moving to common headers and that is not right.
> 
> No, it is only one change: Move to the common kernel way of working
> with these structures.

I'm now cool with this.

> > Even if they should be removed I think what you say about actbl2.h is
> > wrong. Annotations are probably missing because it is imported code (I'm
> > happy to be corrected if this is not the case).
> 
> Edit the comments if you like.
> 
> Annotations are missing because BE is not supported at all, nothing to
> do with imported code.

The code change is OK but the argumentation in the commit messsage is
bogus.

ACPICA will probably do the conversions internally when moved to BE
architecture as I now remember we discussed in December. That's the
only right argument to rip off le32_to_cpu()'s.

Missing annotations are not related of BE support being missing or not.
ACPICA cannot use Linux annotations in it APIs because it is imported
code but can use in the Linux driver for it (drivers/acpi) for
internals.

> Jason

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH v3 2/7] tpm_tis: Disable interrupt auto probing on a per-device basis

[Jarkko Sakkinen]

On Thu, Dec 17, 2015 at 11:23:15AM -0700, Jason Gunthorpe wrote:
> Instead of clearing the global interrupts flag when any device
> does not have an interrupt just pass -1 through tpm_info.irq.
> 
> The only thing that asks for autoprobing is the force=1 path.

Sorry for my ignorance but what does this patch help? Why interrupts
flag is not enough?

> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Tested-by: Wilck, Martin <martin.wilck-RJz4owOZxyXQFUHtdCDX3A@public.gmane.org>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

Did I already give Tested-by's for this series (I did for those that
went into v4.5 pull request)?

/Jarkko

> ---
>  drivers/char/tpm/tpm_tis.c | 18 ++++++++++--------
>  1 file changed, 10 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 304323bdcaaa..fecd27b45fd1 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -69,7 +69,11 @@ enum tis_defaults {
>  struct tpm_info {
>  	unsigned long start;
>  	unsigned long len;
> -	unsigned int irq;
> +	/* irq > 0 means: use irq $irq;
> +	 * irq = 0 means: autoprobe for an irq;
> +	 * irq = -1 means: no irq support
> +	 */
> +	int irq;
>  };
>  
>  static struct tpm_info tis_default_info = {
> @@ -807,7 +811,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
>  	/* INTERRUPT Setup */
>  	init_waitqueue_head(&chip->vendor.read_queue);
>  	init_waitqueue_head(&chip->vendor.int_queue);
> -	if (interrupts) {
> +	if (interrupts && tpm_info->irq != -1) {
>  		if (tpm_info->irq) {
>  			tpm_tis_probe_irq_single(chip, intmask, IRQF_SHARED,
>  						 tpm_info->irq);
> @@ -895,9 +899,9 @@ static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
>  
>  #ifdef CONFIG_PNP
>  static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
> -				      const struct pnp_device_id *pnp_id)
> +			    const struct pnp_device_id *pnp_id)
>  {
> -	struct tpm_info tpm_info = tis_default_info;
> +	struct tpm_info tpm_info = {};
>  	acpi_handle acpi_dev_handle = NULL;
>  
>  	tpm_info.start = pnp_mem_start(pnp_dev, 0);
> @@ -906,7 +910,7 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
>  	if (pnp_irq_valid(pnp_dev, 0))
>  		tpm_info.irq = pnp_irq(pnp_dev, 0);
>  	else
> -		interrupts = false;
> +		tpm_info.irq = -1;
>  
>  #ifdef CONFIG_ACPI
>  	if (pnp_acpi_device(pnp_dev)) {
> @@ -984,6 +988,7 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
>  		return -ENODEV;
>  
>  	INIT_LIST_HEAD(&resources);
> +	tpm_info.irq = -1;
>  	ret = acpi_dev_get_resources(acpi_dev, &resources, tpm_check_resource,
>  				     &tpm_info);
>  	if (ret < 0)
> @@ -991,9 +996,6 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
>  
>  	acpi_dev_free_resource_list(&resources);
>  
> -	if (!tpm_info.irq)
> -		interrupts = false;
> -
>  	if (is_itpm(acpi_dev))
>  		itpm = true;
>  
> -- 
> 2.1.4
> 

------------------------------------------------------------------------------

Re: [PATCH v3 2/7] tpm_tis: Disable interrupt auto probing on a per-device basis

[Jason Gunthorpe]

On Sun, Jan 03, 2016 at 07:20:40PM +0200, Jarkko Sakkinen wrote:
> On Thu, Dec 17, 2015 at 11:23:15AM -0700, Jason Gunthorpe wrote:
> > Instead of clearing the global interrupts flag when any device
> > does not have an interrupt just pass -1 through tpm_info.irq.
> > 
> > The only thing that asks for autoprobing is the force=1 path.
> 
> Sorry for my ignorance but what does this patch help? Why interrupts
> flag is not enough?

It is wrong for a driver's probe function to change global state, and
very wrong to change a module option.

> > Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > Tested-by: Wilck, Martin <martin.wilck-RJz4owOZxyXQFUHtdCDX3A@public.gmane.org>
> > Tested-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> 
> Did I already give Tested-by's for this series (I did for those that
> went into v4.5 pull request)?

You said you tested the crb stuff, which is this series... Did you
test something else?

Jason

------------------------------------------------------------------------------

Re: [PATCH v3 2/7] tpm_tis: Disable interrupt auto probing on a per-device basis

[Jarkko Sakkinen]

On Mon, Jan 04, 2016 at 11:24:42AM -0700, Jason Gunthorpe wrote:
> On Sun, Jan 03, 2016 at 07:20:40PM +0200, Jarkko Sakkinen wrote:
> > On Thu, Dec 17, 2015 at 11:23:15AM -0700, Jason Gunthorpe wrote:
> > > Instead of clearing the global interrupts flag when any device
> > > does not have an interrupt just pass -1 through tpm_info.irq.
> > > 
> > > The only thing that asks for autoprobing is the force=1 path.
> > 
> > Sorry for my ignorance but what does this patch help? Why interrupts
> > flag is not enough?
> 
> It is wrong for a driver's probe function to change global state, and
> very wrong to change a module option.

I disagree about global state in some sense but module options should
stay static. That's much better argument.

> > > Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> > > Tested-by: Wilck, Martin <martin.wilck-RJz4owOZxyXQFUHtdCDX3A@public.gmane.org>
> > > Tested-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> > 
> > Did I already give Tested-by's for this series (I did for those that
> > went into v4.5 pull request)?
> 
> You said you tested the crb stuff, which is this series... Did you
> test something else?

Checked my emails. Right I did run my tests for this (both with FIFO
and CRB driver). It was bit "stormy" before Christmas with all kinds
of small patches so I wasn't sure.

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

> Jason

/Jarkko

------------------------------------------------------------------------------

Re: [PATCH v3 4/7] tpm_tis: Use devm_ioremap_resource

[Jarkko Sakkinen]

On Thu, Dec 17, 2015 at 11:23:17AM -0700, Jason Gunthorpe wrote:
> This does a request_resource under the covers which means tis holds a
> lock on the memory range it is using so other drivers cannot grab it.
> When doing probing it is important to ensure that other drivers are
> not using the same range before tis starts touching it.
> 
> To do this flow the actual struct resource from the device right
> through to devm_ioremap_resource. This ensures all the proper resource
> meta-data is carried down.

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Tested-by: Wilck, Martin <martin.wilck-RJz4owOZxyXQFUHtdCDX3A@public.gmane.org>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/char/tpm/tpm_tis.c | 29 ++++++++++++++++-------------
>  1 file changed, 16 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index b2b31f5418ca..856fb35e574c 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -67,8 +67,7 @@ enum tis_defaults {
>  };
>  
>  struct tpm_info {
> -	unsigned long start;
> -	unsigned long len;
> +	struct resource res;
>  	/* irq > 0 means: use irq $irq;
>  	 * irq = 0 means: autoprobe for an irq;
>  	 * irq = -1 means: no irq support
> @@ -77,8 +76,11 @@ struct tpm_info {
>  };
>  
>  static struct tpm_info tis_default_info = {
> -	.start = TIS_MEM_BASE,
> -	.len = TIS_MEM_LEN,
> +	.res = {
> +		.start = TIS_MEM_BASE,
> +		.end = TIS_MEM_BASE + TIS_MEM_LEN - 1,
> +		.flags = IORESOURCE_MEM,
> +	},
>  	.irq = 0,
>  };
>  
> @@ -692,7 +694,7 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
>  	chip->acpi_dev_handle = acpi_dev_handle;
>  #endif
>  
> -	chip->vendor.iobase = devm_ioremap(dev, tpm_info->start, tpm_info->len);
> +	chip->vendor.iobase = devm_ioremap_resource(dev, &tpm_info->res);
>  	if (!chip->vendor.iobase)
>  		return -EIO;
>  
> @@ -875,9 +877,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
>  {
>  	struct tpm_info tpm_info = {};
>  	acpi_handle acpi_dev_handle = NULL;
> +	struct resource *res;
>  
> -	tpm_info.start = pnp_mem_start(pnp_dev, 0);
> -	tpm_info.len = pnp_mem_len(pnp_dev, 0);
> +	res = pnp_get_resource(pnp_dev, IORESOURCE_MEM, 0);
> +	if (!res)
> +		return -ENODEV;
> +	tpm_info.res = *res;
>  
>  	if (pnp_irq_valid(pnp_dev, 0))
>  		tpm_info.irq = pnp_irq(pnp_dev, 0);
> @@ -940,12 +945,10 @@ static int tpm_check_resource(struct acpi_resource *ares, void *data)
>  	struct tpm_info *tpm_info = (struct tpm_info *) data;
>  	struct resource res;
>  
> -	if (acpi_dev_resource_interrupt(ares, 0, &res)) {
> +	if (acpi_dev_resource_interrupt(ares, 0, &res))
>  		tpm_info->irq = res.start;
> -	} else if (acpi_dev_resource_memory(ares, &res)) {
> -		tpm_info->start = res.start;
> -		tpm_info->len = resource_size(&res);
> -	}
> +	else if (acpi_dev_resource_memory(ares, &res))
> +		tpm_info->res = res;
>  
>  	return 1;
>  }
> @@ -978,7 +981,7 @@ static int tpm_tis_acpi_init(struct acpi_device *acpi_dev)
>  
>  	acpi_dev_free_resource_list(&resources);
>  
> -	if (tpm_info.start == 0 && tpm_info.len == 0) {
> +	if (resource_type(&tpm_info.res) != IORESOURCE_MEM) {
>  		dev_err(&acpi_dev->dev,
>  			FW_BUG "TPM2 ACPI table does not define a memory resource\n");
>  		return -EINVAL;
> -- 
> 2.1.4
> 

------------------------------------------------------------------------------

Re: [PATCH v3 5/7] tpm_tis: Clean up the force=1 module parameter

[Jarkko Sakkinen]

On Thu, Dec 17, 2015 at 11:23:18AM -0700, Jason Gunthorpe wrote:
> The TPM core has long assumed that every device has a driver attached,
> however the force path was attaching the TPM core outside of a driver
> context. This isn't generally reliable as the user could detatch the
> driver using sysfs or something, but commit b8b2c7d845d5 ("base/platform:
> assert that dev_pm_domain callbacks are called unconditionally")
> forced the issue by leaving the driver pointer NULL if there is
> no probe.
> 
> Rework the TPM setup to create a platform device with resources and
> then allow the driver core to naturally bind and probe it through the
> normal mechanisms. All this structure is needed anyhow to enable TPM
> for OF environments.
> 
> Finally, since the entire flow is changing convert the init/exit to use
> the modern ifdef-less coding style when possible
> 
> Reported-by: "Wilck, Martin" <martin.wilck@ts.fujitsu.com>
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Tested-by: Wilck, Martin <martin.wilck@ts.fujitsu.com>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
> ---
>  drivers/char/tpm/tpm_tis.c | 173 +++++++++++++++++++++++++++------------------
>  1 file changed, 106 insertions(+), 67 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_tis.c b/drivers/char/tpm/tpm_tis.c
> index 856fb35e574c..12aa96a69b6c 100644
> --- a/drivers/char/tpm/tpm_tis.c
> +++ b/drivers/char/tpm/tpm_tis.c
> @@ -60,7 +60,6 @@ enum tis_int_flags {
>  };
>  
>  enum tis_defaults {
> -	TIS_MEM_BASE = 0xFED40000,
>  	TIS_MEM_LEN = 0x5000,
>  	TIS_SHORT_TIMEOUT = 750,	/* ms */
>  	TIS_LONG_TIMEOUT = 2000,	/* 2 sec */
> @@ -75,15 +74,6 @@ struct tpm_info {
>  	int irq;
>  };
>  
> -static struct tpm_info tis_default_info = {
> -	.res = {
> -		.start = TIS_MEM_BASE,
> -		.end = TIS_MEM_BASE + TIS_MEM_LEN - 1,
> -		.flags = IORESOURCE_MEM,
> -	},
> -	.irq = 0,
> -};
> -
>  /* Some timeout values are needed before it is known whether the chip is
>   * TPM 1.0 or TPM 2.0.
>   */
> @@ -695,8 +685,8 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
>  #endif
>  
>  	chip->vendor.iobase = devm_ioremap_resource(dev, &tpm_info->res);
> -	if (!chip->vendor.iobase)
> -		return -EIO;
> +	if (IS_ERR(chip->vendor.iobase))
> +		return PTR_ERR(chip->vendor.iobase);

Isn't this a regression in the descendig patch in this series?

>  	/* Maximum timeouts */
>  	chip->vendor.timeout_a = TIS_TIMEOUT_A_MAX;
> @@ -825,7 +815,6 @@ out_err:
>  	return rc;
>  }
>  
> -#ifdef CONFIG_PM_SLEEP
>  static void tpm_tis_reenable_interrupts(struct tpm_chip *chip)
>  {
>  	u32 intmask;
> @@ -867,11 +856,9 @@ static int tpm_tis_resume(struct device *dev)
>  
>  	return 0;
>  }
> -#endif
>  
>  static SIMPLE_DEV_PM_OPS(tpm_tis_pm, tpm_pm_suspend, tpm_tis_resume);
>  
> -#ifdef CONFIG_PNP
>  static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
>  			    const struct pnp_device_id *pnp_id)
>  {
> @@ -889,14 +876,12 @@ static int tpm_tis_pnp_init(struct pnp_dev *pnp_dev,
>  	else
>  		tpm_info.irq = -1;
>  
> -#ifdef CONFIG_ACPI
>  	if (pnp_acpi_device(pnp_dev)) {
>  		if (is_itpm(pnp_acpi_device(pnp_dev)))
>  			itpm = true;
>  
> -		acpi_dev_handle = pnp_acpi_device(pnp_dev)->handle;
> +		acpi_dev_handle = ACPI_HANDLE(&pnp_dev->dev);
>  	}
> -#endif
>  
>  	return tpm_tis_init(&pnp_dev->dev, &tpm_info, acpi_dev_handle);
>  }
> @@ -937,7 +922,6 @@ static struct pnp_driver tis_pnp_driver = {
>  module_param_string(hid, tpm_pnp_tbl[TIS_HID_USR_IDX].id,
>  		    sizeof(tpm_pnp_tbl[TIS_HID_USR_IDX].id), 0444);
>  MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to probe");
> -#endif
>  
>  #ifdef CONFIG_ACPI
>  static int tpm_check_resource(struct acpi_resource *ares, void *data)
> @@ -1024,80 +1008,135 @@ static struct acpi_driver tis_acpi_driver = {
>  };
>  #endif
>  
> +static struct platform_device *force_pdev;
> +
> +static int tpm_tis_plat_probe(struct platform_device *pdev)
> +{
> +	struct tpm_info tpm_info = {};
> +	struct resource *res;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (res == NULL) {
> +		dev_err(&pdev->dev, "no memory resource defined\n");
> +		return -ENODEV;
> +	}
> +	tpm_info.res = *res;
> +
> +	res = platform_get_resource(pdev, IORESOURCE_IRQ, 0);
> +	if (res) {
> +		tpm_info.irq = res->start;
> +	} else {
> +		if (pdev == force_pdev)
> +			tpm_info.irq = -1;
> +		else
> +			/* When forcing auto probe the IRQ */
> +			tpm_info.irq = 0;
> +	}
> +
> +	return tpm_tis_init(&pdev->dev, &tpm_info, NULL);
> +}
> +
> +static int tpm_tis_plat_remove(struct platform_device *pdev)
> +{
> +	struct tpm_chip *chip = dev_get_drvdata(&pdev->dev);
> +
> +	tpm_chip_unregister(chip);
> +	tpm_tis_remove(chip);
> +
> +	return 0;
> +}
> +
>  static struct platform_driver tis_drv = {
> +	.probe = tpm_tis_plat_probe,
> +	.remove = tpm_tis_plat_remove,
>  	.driver = {
>  		.name		= "tpm_tis",
>  		.pm		= &tpm_tis_pm,
>  	},
>  };
>  
> -static struct platform_device *pdev;
> -
>  static bool force;
> +#ifdef CONFIG_X86
>  module_param(force, bool, 0444);
>  MODULE_PARM_DESC(force, "Force device probe rather than using ACPI entry");
> +#endif
> +
> +static int tpm_tis_force_device(void)
> +{
> +	struct platform_device *pdev;
> +	static const struct resource x86_resources[] = {
> +		{
> +			.start = 0xFED40000,
> +			.end = 0xFED40000 + TIS_MEM_LEN - 1,
> +			.flags = IORESOURCE_MEM,
> +		},
> +	};
> +
> +	if (!force)
> +		return 0;
> +
> +	/* The driver core will match the name tpm_tis of the device to
> +	 * the tpm_tis platform driver and complete the setup via
> +	 * tpm_tis_plat_probe
> +	 */
> +	pdev = platform_device_register_simple("tpm_tis", -1, x86_resources,
> +					       ARRAY_SIZE(x86_resources));
> +	if (IS_ERR(pdev))
> +		return PTR_ERR(pdev);
> +	force_pdev = pdev;
> +
> +	return 0;
> +}
> +
>  static int __init init_tis(void)
>  {
>  	int rc;
> -#ifdef CONFIG_PNP
> -	if (!force) {
> -		rc = pnp_register_driver(&tis_pnp_driver);
> -		if (rc)
> -			return rc;
> -	}
> -#endif
> +
> +	rc = tpm_tis_force_device();
> +	if (rc)
> +		goto err_force;
> +
> +	rc = platform_driver_register(&tis_drv);
> +	if (rc)
> +		goto err_platform;
> +
>  #ifdef CONFIG_ACPI
> -	if (!force) {
> -		rc = acpi_bus_register_driver(&tis_acpi_driver);
> -		if (rc) {
> -#ifdef CONFIG_PNP
> -			pnp_unregister_driver(&tis_pnp_driver);
> -#endif
> -			return rc;
> -		}
> -	}
> +	rc = acpi_bus_register_driver(&tis_acpi_driver);
> +	if (rc)
> +		goto err_acpi;
>  #endif
> -	if (!force)
> -		return 0;
>  
> -	rc = platform_driver_register(&tis_drv);
> -	if (rc < 0)
> -		return rc;
> -	pdev = platform_device_register_simple("tpm_tis", -1, NULL, 0);
> -	if (IS_ERR(pdev)) {
> -		rc = PTR_ERR(pdev);
> -		goto err_dev;
> +	if (IS_ENABLED(CONFIG_PNP)) {
> +		rc = pnp_register_driver(&tis_pnp_driver);
> +		if (rc)
> +			goto err_pnp;
>  	}
> -	rc = tpm_tis_init(&pdev->dev, &tis_default_info, NULL);
> -	if (rc)
> -		goto err_init;
> +
>  	return 0;
> -err_init:
> -	platform_device_unregister(pdev);
> -err_dev:
> -	platform_driver_unregister(&tis_drv);
> +
> +err_pnp:
> +#ifdef CONFIG_ACPI
> +	acpi_bus_unregister_driver(&tis_acpi_driver);
> +err_acpi:
> +#endif
> +	platform_device_unregister(force_pdev);
> +err_platform:
> +	if (force_pdev)
> +		platform_device_unregister(force_pdev);
> +err_force:
>  	return rc;
>  }
>  
>  static void __exit cleanup_tis(void)
>  {
> -	struct tpm_chip *chip;
> -#if defined(CONFIG_PNP) || defined(CONFIG_ACPI)
> -	if (!force) {
> +	pnp_unregister_driver(&tis_pnp_driver);
>  #ifdef CONFIG_ACPI
> -		acpi_bus_unregister_driver(&tis_acpi_driver);
> -#endif
> -#ifdef CONFIG_PNP
> -		pnp_unregister_driver(&tis_pnp_driver);
> -#endif
> -		return;
> -	}
> +	acpi_bus_unregister_driver(&tis_acpi_driver);
>  #endif
> -	chip = dev_get_drvdata(&pdev->dev);
> -	tpm_chip_unregister(chip);
> -	tpm_tis_remove(chip);
> -	platform_device_unregister(pdev);
>  	platform_driver_unregister(&tis_drv);
> +
> +	if (force_pdev)
> +		platform_device_unregister(force_pdev);
>  }
>  
>  module_init(init_tis);
> -- 
> 2.1.4
> 

/Jarkko

Re: [PATCH v3 5/7] tpm_tis: Clean up the force=1 module parameter

[Jason Gunthorpe]

On Sun, Jan 03, 2016 at 07:26:50PM +0200, Jarkko Sakkinen wrote:
> > @@ -695,8 +685,8 @@ static int tpm_tis_init(struct device *dev, struct tpm_info *tpm_info,
> >  #endif
> >  
> >  	chip->vendor.iobase = devm_ioremap_resource(dev, &tpm_info->res);
> > -	if (!chip->vendor.iobase)
> > -		return -EIO;
> > +	if (IS_ERR(chip->vendor.iobase))
> > +		return PTR_ERR(chip->vendor.iobase);
> 
> Isn't this a regression in the descendig patch in this series?

Oh yes, good catch.

The fix from Martin got rebased onto the wrong patch by accident.

Jason

Re: [PATCH v3 6/7] tpm_crb: Drop le32_to_cpu(ioread32(..))

[Jarkko Sakkinen]

On Thu, Dec 17, 2015 at 11:23:19AM -0700, Jason Gunthorpe wrote:
> ioread32 and readl are defined to read from PCI style memory, ie little
> endian and return the result in host order. On platforms where a
> swap is required ioread32/readl do the swap internally (eg see ppc).

Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>

/Jarkko

> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/char/tpm/tpm_crb.c | 22 +++++++++++-----------
>  1 file changed, 11 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 8dd70696ebe8..0237006dc4d8 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -89,7 +89,7 @@ static u8 crb_status(struct tpm_chip *chip)
>  	struct crb_priv *priv = chip->vendor.priv;
>  	u8 sts = 0;
>  
> -	if ((le32_to_cpu(ioread32(&priv->cca->start)) & CRB_START_INVOKE) !=
> +	if ((ioread32(&priv->cca->start) & CRB_START_INVOKE) !=
>  	    CRB_START_INVOKE)
>  		sts |= CRB_STS_COMPLETE;
>  
> @@ -105,7 +105,7 @@ static int crb_recv(struct tpm_chip *chip, u8 *buf, size_t count)
>  	if (count < 6)
>  		return -EIO;
>  
> -	if (le32_to_cpu(ioread32(&priv->cca->sts)) & CRB_CA_STS_ERROR)
> +	if (ioread32(&priv->cca->sts) & CRB_CA_STS_ERROR)
>  		return -EIO;
>  
>  	memcpy_fromio(buf, priv->rsp, 6);
> @@ -141,11 +141,11 @@ static int crb_send(struct tpm_chip *chip, u8 *buf, size_t len)
>  	struct crb_priv *priv = chip->vendor.priv;
>  	int rc = 0;
>  
> -	if (len > le32_to_cpu(ioread32(&priv->cca->cmd_size))) {
> +	if (len > ioread32(&priv->cca->cmd_size)) {
>  		dev_err(&chip->dev,
>  			"invalid command count value %x %zx\n",
>  			(unsigned int) len,
> -			(size_t) le32_to_cpu(ioread32(&priv->cca->cmd_size)));
> +			(size_t) ioread32(&priv->cca->cmd_size));
>  		return -E2BIG;
>  	}
>  
> @@ -181,7 +181,7 @@ static void crb_cancel(struct tpm_chip *chip)
>  static bool crb_req_canceled(struct tpm_chip *chip, u8 status)
>  {
>  	struct crb_priv *priv = chip->vendor.priv;
> -	u32 cancel = le32_to_cpu(ioread32(&priv->cca->cancel));
> +	u32 cancel = ioread32(&priv->cca->cancel);
>  
>  	return (cancel & CRB_CANCEL_INVOKE) == CRB_CANCEL_INVOKE;
>  }
> @@ -251,10 +251,10 @@ static int crb_acpi_add(struct acpi_device *device)
>  		return -ENOMEM;
>  	}
>  
> -	pa = ((u64) le32_to_cpu(ioread32(&priv->cca->cmd_pa_high)) << 32) |
> -		(u64) le32_to_cpu(ioread32(&priv->cca->cmd_pa_low));
> -	priv->cmd = devm_ioremap_nocache(dev, pa,
> -					 ioread32(&priv->cca->cmd_size));
> +	pa = ((u64)ioread32(&priv->cca->cmd_pa_high) << 32) |
> +	     (u64)ioread32(&priv->cca->cmd_pa_low);
> +	priv->cmd =
> +	    devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->cmd_size));
>  	if (!priv->cmd) {
>  		dev_err(dev, "ioremap of the command buffer failed\n");
>  		return -ENOMEM;
> @@ -262,8 +262,8 @@ static int crb_acpi_add(struct acpi_device *device)
>  
>  	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
>  	pa = le64_to_cpu(pa);
> -	priv->rsp = devm_ioremap_nocache(dev, pa,
> -					 ioread32(&priv->cca->rsp_size));
> +	priv->rsp =
> +	    devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->rsp_size));
>  	if (!priv->rsp) {
>  		dev_err(dev, "ioremap of the response buffer failed\n");
>  		return -ENOMEM;
> -- 
> 2.1.4
> 

------------------------------------------------------------------------------

Re: [PATCH v3 7/7] tpm_crb: Use devm_ioremap_resource

[Jarkko Sakkinen]

On Thu, Dec 17, 2015 at 11:23:20AM -0700, Jason Gunthorpe wrote:
> To support the force mode in tpm_tis we need to use resource locking
> in tpm_crb as well, via devm_ioremap_resource.
> 
> The light restructuring better aligns crb and tis and makes it easier
> to see the that new changes make sense.

This patch mixes two items: restructing and actual change.

It might make the code cleaner but this patch harder to accept because
it contains more code changes are therefore more risks of regressions.
Tested code is better than clean code.

You should propose clean ups on smaller scale (patches) and not and not
hidden into other changes.

/Jarkko

> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>
> Tested-by: Jarkko Sakkinen <jarkko.sakkinen-VuQAYsv1563Yd54FQh9/CA@public.gmane.org>
> ---
>  drivers/char/tpm/tpm_crb.c | 157 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 108 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 0237006dc4d8..1f844cc63016 100644
> --- a/drivers/char/tpm/tpm_crb.c
> +++ b/drivers/char/tpm/tpm_crb.c
> @@ -77,6 +77,8 @@ enum crb_flags {
>  
>  struct crb_priv {
>  	unsigned int flags;
> +	struct resource res;
> +	void __iomem *iobase;
>  	struct crb_control_area __iomem *cca;
>  	u8 __iomem *cmd;
>  	u8 __iomem *rsp;
> @@ -196,22 +198,121 @@ static const struct tpm_class_ops tpm_crb = {
>  	.req_complete_val = CRB_STS_COMPLETE,
>  };
>  
> -static int crb_acpi_add(struct acpi_device *device)
> +static int crb_init(struct acpi_device *device, struct crb_priv *priv)
>  {
>  	struct tpm_chip *chip;
> +	int rc;
> +
> +	chip = tpmm_chip_alloc(&device->dev, &tpm_crb);
> +	if (IS_ERR(chip))
> +		return PTR_ERR(chip);
> +
> +	chip->vendor.priv = priv;
> +	chip->acpi_dev_handle = device->handle;
> +	chip->flags = TPM_CHIP_FLAG_TPM2;
> +
> +	rc = tpm_get_timeouts(chip);
> +	if (rc)
> +		return rc;
> +
> +	rc = tpm2_do_selftest(chip);
> +	if (rc)
> +		return rc;
> +
> +	return tpm_chip_register(chip);
> +}
> +
> +static int crb_check_resource(struct acpi_resource *ares, void *data)
> +{
> +	struct crb_priv *priv = data;
> +	struct resource res;
> +
> +	if (acpi_dev_resource_memory(ares, &res))
> +		priv->res = res;
> +
> +	return 1;
> +}
> +
> +static void __iomem *crb_access(struct device *dev, struct crb_priv *priv,
> +				u64 start, u32 size)
> +{
> +	struct resource tmp = {};
> +
> +	tmp.start = start;
> +	tmp.end = start + size - 1;
> +	tmp.flags = IORESOURCE_MEM;
> +
> +	/* Detect a 64 bit address on a 32 bit system */
> +	if (start != tmp.start)
> +		return ERR_PTR(-EINVAL);
> +
> +	if (!resource_contains(&priv->res, &tmp)) {
> +		dev_err(dev,
> +			FW_BUG "TPM2 ACPI sub resource %pR is not in the device's region of %pR\n",
> +			&tmp, &priv->res);
> +		return ERR_PTR(-EINVAL);
> +	}
> +
> +	return priv->iobase + (tmp.start - priv->res.start);
> +}
> +
> +static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
> +		      struct acpi_table_tpm2 *buf)
> +{
> +	struct list_head resources;
> +	struct device *dev = &device->dev;
> +	u64 pa;
> +	int ret;
> +
> +	INIT_LIST_HEAD(&resources);
> +	ret = acpi_dev_get_resources(device, &resources, crb_check_resource,
> +				     priv);
> +	if (ret < 0)
> +		return ret;
> +	acpi_dev_free_resource_list(&resources);
> +
> +	if (resource_type(&priv->res) != IORESOURCE_MEM) {
> +		dev_err(dev,
> +			FW_BUG "TPM2 ACPI table does not define a memory resource\n");
> +		return -EINVAL;
> +	}
> +
> +	priv->iobase = devm_ioremap_resource(dev, &priv->res);
> +	if (IS_ERR(priv->iobase))
> +		return PTR_ERR(priv->iobase);
> +
> +	priv->cca = crb_access(dev, priv, buf->control_address, 0x1000);
> +	if (IS_ERR(priv->cca))
> +		return PTR_ERR(priv->cca);
> +
> +	pa = ((u64)ioread32(&priv->cca->cmd_pa_high) << 32) |
> +	     (u64)ioread32(&priv->cca->cmd_pa_low);
> +	priv->cmd = crb_access(dev, priv, pa, ioread32(&priv->cca->cmd_size));
> +	if (IS_ERR(priv->cmd))
> +		return PTR_ERR(priv->cmd);
> +
> +	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
> +	pa = le64_to_cpu(pa);
> +	priv->rsp = crb_access(dev, priv, pa, ioread32(&priv->cca->rsp_size));
> +	if (IS_ERR(priv->rsp))
> +		return PTR_ERR(priv->rsp);
> +	return 0;
> +}
> +
> +static int crb_acpi_add(struct acpi_device *device)
> +{
>  	struct acpi_table_tpm2 *buf;
>  	struct crb_priv *priv;
>  	struct device *dev = &device->dev;
>  	acpi_status status;
>  	u32 sm;
> -	u64 pa;
>  	int rc;
>  
>  	status = acpi_get_table(ACPI_SIG_TPM2, 1,
>  				(struct acpi_table_header **) &buf);
>  	if (ACPI_FAILURE(status) || buf->header.length < sizeof(*buf)) {
>  		dev_err(dev, FW_BUG "failed to get TPM2 ACPI table\n");
> -		return -ENODEV;
> +		return -EINVAL;
>  	}
>  
>  	/* Should the FIFO driver handle this? */
> @@ -219,18 +320,9 @@ static int crb_acpi_add(struct acpi_device *device)
>  	if (sm == ACPI_TPM2_MEMORY_MAPPED)
>  		return -ENODEV;
>  
> -	chip = tpmm_chip_alloc(dev, &tpm_crb);
> -	if (IS_ERR(chip))
> -		return PTR_ERR(chip);
> -
> -	chip->flags = TPM_CHIP_FLAG_TPM2;
> -
> -	priv = (struct crb_priv *) devm_kzalloc(dev, sizeof(struct crb_priv),
> -						GFP_KERNEL);
> -	if (!priv) {
> -		dev_err(dev, "failed to devm_kzalloc for private data\n");
> +	priv = devm_kzalloc(dev, sizeof(struct crb_priv), GFP_KERNEL);
> +	if (!priv)
>  		return -ENOMEM;
> -	}
>  
>  	/* The reason for the extra quirk is that the PTT in 4th Gen Core CPUs
>  	 * report only ACPI start but in practice seems to require both
> @@ -244,44 +336,11 @@ static int crb_acpi_add(struct acpi_device *device)
>  	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
>  		priv->flags |= CRB_FL_ACPI_START;
>  
> -	priv->cca = (struct crb_control_area __iomem *)
> -		devm_ioremap_nocache(dev, buf->control_address, 0x1000);
> -	if (!priv->cca) {
> -		dev_err(dev, "ioremap of the control area failed\n");
> -		return -ENOMEM;
> -	}
> -
> -	pa = ((u64)ioread32(&priv->cca->cmd_pa_high) << 32) |
> -	     (u64)ioread32(&priv->cca->cmd_pa_low);
> -	priv->cmd =
> -	    devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->cmd_size));
> -	if (!priv->cmd) {
> -		dev_err(dev, "ioremap of the command buffer failed\n");
> -		return -ENOMEM;
> -	}
> -
> -	memcpy_fromio(&pa, &priv->cca->rsp_pa, 8);
> -	pa = le64_to_cpu(pa);
> -	priv->rsp =
> -	    devm_ioremap_nocache(dev, pa, ioread32(&priv->cca->rsp_size));
> -	if (!priv->rsp) {
> -		dev_err(dev, "ioremap of the response buffer failed\n");
> -		return -ENOMEM;
> -	}
> -
> -	chip->vendor.priv = priv;
> -
> -	rc = tpm_get_timeouts(chip);
> -	if (rc)
> -		return rc;
> -
> -	chip->acpi_dev_handle = device->handle;
> -
> -	rc = tpm2_do_selftest(chip);
> +	rc = crb_map_io(device, priv, buf);
>  	if (rc)
>  		return rc;
>  
> -	return tpm_chip_register(chip);
> +	return crb_init(device, priv);
>  }
>  
>  static int crb_acpi_remove(struct acpi_device *device)
> -- 
> 2.1.4
> 

------------------------------------------------------------------------------

Re: [PATCH v3 7/7] tpm_crb: Use devm_ioremap_resource

[Jason Gunthorpe]

On Sun, Jan 03, 2016 at 07:32:13PM +0200, Jarkko Sakkinen wrote:
> On Thu, Dec 17, 2015 at 11:23:20AM -0700, Jason Gunthorpe wrote:
> > To support the force mode in tpm_tis we need to use resource locking
> > in tpm_crb as well, via devm_ioremap_resource.
> > 
> > The light restructuring better aligns crb and tis and makes it easier
> > to see the that new changes make sense.
> 
> This patch mixes two items: restructing and actual change.

Do you have a suggestion how to split this?

Everything is done for a reason, it isn't just churn for the sake of
churn.

Jason

------------------------------------------------------------------------------

Re: [PATCH v3 7/7] tpm_crb: Use devm_ioremap_resource

[Jarkko Sakkinen]

On Mon, Jan 04, 2016 at 11:30:25AM -0700, Jason Gunthorpe wrote:
> On Sun, Jan 03, 2016 at 07:32:13PM +0200, Jarkko Sakkinen wrote:
> > On Thu, Dec 17, 2015 at 11:23:20AM -0700, Jason Gunthorpe wrote:
> > > To support the force mode in tpm_tis we need to use resource locking
> > > in tpm_crb as well, via devm_ioremap_resource.
> > > 
> > > The light restructuring better aligns crb and tis and makes it easier
> > > to see the that new changes make sense.
> > 
> > This patch mixes two items: restructing and actual change.
> 
> Do you have a suggestion how to split this?
> 
> Everything is done for a reason, it isn't just churn for the sake of
> churn.

I'll appy this to master.

> Jason

/Jarkko