[tpmdd-devel] Last thing for force=1 rework
diff mbox

Message ID 20151207192258.GA27871@obsidianresearch.com
State New
Headers show

Commit Message

Jason Gunthorpe Dec. 7, 2015, 7:22 p.m. UTC
Jarkko,

The last thing to fix the force=1 stuff is to make similar changes
to tpm_crb. I'm guessing a bit here, can you please look at this and
make any necessary adjustments?

There is a trivial predecessor patch, the complete series is on my
github here:

https://github.com/jgunthorpe/linux/commits/for-jarkko

>From af1ad9fc06dab8d566166a765cb164297566d1fb Mon Sep 17 00:00:00 2001
From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
Date: Mon, 7 Dec 2015 12:18:19 -0700
Subject: [PATCH 7/7] tpm_crb: devm_ioremap_resource

To support the force mode in tpm_tis we need to use resource locking
in tpm_crb as well, via devm_ioremap_resource.

WIP: This version assumes the ACPI stuff is somewhat sane and that
the supplementary addresses (cca, cmd, rsp) all fall within the
struct resource returned by the _CRS. To that end it maps the struct
resource from the _CRS and computes offsets within it. If this is not
the case then the #if 0 approach is needed where each sub resource
is mapped independently.

The light restructuring better aligns crb and tis and makes it easier
to see the that new changes make sense.

Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
---
 drivers/char/tpm/tpm_crb.c | 162 +++++++++++++++++++++++++++++++--------------
 1 file changed, 113 insertions(+), 49 deletions(-)

Comments

Jarkko Sakkinen Dec. 8, 2015, 9:42 a.m. UTC | #1
On Mon, Dec 07, 2015 at 12:22:58PM -0700, Jason Gunthorpe wrote:
> Jarkko,
> 
> The last thing to fix the force=1 stuff is to make similar changes
> to tpm_crb. I'm guessing a bit here, can you please look at this and
> make any necessary adjustments?
> 
> There is a trivial predecessor patch, the complete series is on my
> github here:
> 
> https://github.com/jgunthorpe/linux/commits/for-jarkko

In big picture it looks good to me. I guess this does not pass
checkpatch.pl yet?

> From af1ad9fc06dab8d566166a765cb164297566d1fb Mon Sep 17 00:00:00 2001
> From: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> Date: Mon, 7 Dec 2015 12:18:19 -0700
> Subject: [PATCH 7/7] tpm_crb: devm_ioremap_resource
> 
> To support the force mode in tpm_tis we need to use resource locking
> in tpm_crb as well, via devm_ioremap_resource.
> 
> WIP: This version assumes the ACPI stuff is somewhat sane and that
> the supplementary addresses (cca, cmd, rsp) all fall within the
> struct resource returned by the _CRS. To that end it maps the struct
> resource from the _CRS and computes offsets within it. If this is not
> the case then the #if 0 approach is needed where each sub resource
> is mapped independently.
> 
> The light restructuring better aligns crb and tis and makes it easier
> to see the that new changes make sense.
> 
> Signed-off-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
> ---
>  drivers/char/tpm/tpm_crb.c | 162 +++++++++++++++++++++++++++++++--------------
>  1 file changed, 113 insertions(+), 49 deletions(-)
> 
> diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
> index 0237006dc4d8..131467e64321 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,126 @@ 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);

Can this realize? Is this robustness for a possible FW bug?

> +
> +	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);
> +
> +#if 0
> +		return devm_ioremap_resource(dev, &tmp);
> +#else
> +		return ERR_PTR(-EINVAL);
> +#endif

Does this mean when command buffer and/or response buffer are in
disjoint areas? Have you check from PC Client specification for
constraints how they could be located?

> +	}
> +
> +	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;

Is EINVAL here better than ENODEV?

>  	}
>  
>  	/* Should the FIFO driver handle this? */
> @@ -219,18 +325,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 +341,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);
> +	rc = crb_map_io(device, priv, buf);
>  	if (rc)
>  		return rc;
>  
> -	chip->acpi_dev_handle = device->handle;
> -
> -	rc = tpm2_do_selftest(chip);
> -	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
> 

/Jarkko

------------------------------------------------------------------------------
Go from Idea to Many App Stores Faster with Intel(R) XDK
Give your users amazing mobile app experiences with Intel(R) XDK.
Use one codebase in this all-in-one HTML5 development environment.
Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs.
http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
Jason Gunthorpe Dec. 8, 2015, 4:57 p.m. UTC | #2
On Tue, Dec 08, 2015 at 11:42:09AM +0200, Jarkko Sakkinen wrote:
> In big picture it looks good to me. I guess this does not pass
> checkpatch.pl yet?

checkpatch was OK

> > +	/* Detect a 64 bit address on a 32 bit system */
> > +	if (start != tmp.start)
> > +		return ERR_PTR(-EINVAL);
> 
> Can this realize? Is this robustness for a possible FW bug?

Yes, it can happen. resource_size_t is potentially a u32 on some
builds while the ACPI stuff is unconditionally u64.

It isn't a FW bug to require a OS that can handle the 64 bit physical
address, but it means some 32 bit builds of Linux cannot run on that
firmware.

> > +	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);
> > +
> > +#if 0
> > +		return devm_ioremap_resource(dev, &tmp);
> > +#else
> > +		return ERR_PTR(-EINVAL);
> > +#endif
> 
> Does this mean when command buffer and/or response buffer are in
> disjoint areas?

Almost, it means the _CRS does not cover the cca,cmd,rsp buffer
memory. It isn't clear what purpose the _CRS has if the offsets are
given in other ways - it would make sense if the offsets had to live
within the _CRS..

A couple people will need to try this to see how BIOS folks interpret
this.

> Have you check from PC Client specification for constraints how they
> could be located?

I was not able to locate anything.

> >  	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;
> 
> Is EINVAL here better than ENODEV?

Yes, ENODEV supresses some error logging which should not be supressed
in the FW_BUG case.

Jason

------------------------------------------------------------------------------
Go from Idea to Many App Stores Faster with Intel(R) XDK
Give your users amazing mobile app experiences with Intel(R) XDK.
Use one codebase in this all-in-one HTML5 development environment.
Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs.
http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
Jarkko Sakkinen Dec. 8, 2015, 6:20 p.m. UTC | #3
On Tue, Dec 08, 2015 at 09:57:24AM -0700, Jason Gunthorpe wrote:
> On Tue, Dec 08, 2015 at 11:42:09AM +0200, Jarkko Sakkinen wrote:
> > In big picture it looks good to me. I guess this does not pass
> > checkpatch.pl yet?
> 
> checkpatch was OK
> 
> > > +	/* Detect a 64 bit address on a 32 bit system */
> > > +	if (start != tmp.start)
> > > +		return ERR_PTR(-EINVAL);
> > 
> > Can this realize? Is this robustness for a possible FW bug?
> 
> Yes, it can happen. resource_size_t is potentially a u32 on some
> builds while the ACPI stuff is unconditionally u64.
> 
> It isn't a FW bug to require a OS that can handle the 64 bit physical
> address, but it means some 32 bit builds of Linux cannot run on that
> firmware.

Alright.

> > > +	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);
> > > +
> > > +#if 0
> > > +		return devm_ioremap_resource(dev, &tmp);
> > > +#else
> > > +		return ERR_PTR(-EINVAL);
> > > +#endif
> > 
> > Does this mean when command buffer and/or response buffer are in
> > disjoint areas?
> 
> Almost, it means the _CRS does not cover the cca,cmd,rsp buffer
> memory. It isn't clear what purpose the _CRS has if the offsets are
> given in other ways - it would make sense if the offsets had to live
> within the _CRS..
> 
> A couple people will need to try this to see how BIOS folks interpret
> this.
> 
> > Have you check from PC Client specification for constraints how they
> > could be located?
> 
> I was not able to locate anything.

OK, then we cannot assume anything and doing what you do in "#if 0" is
right thing to do from my perspective.

> > >  	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;
> > 
> > Is EINVAL here better than ENODEV?
> 
> Yes, ENODEV supresses some error logging which should not be supressed
> in the FW_BUG case.

Alright.

> Jason

/Jarkko

------------------------------------------------------------------------------
Go from Idea to Many App Stores Faster with Intel(R) XDK
Give your users amazing mobile app experiences with Intel(R) XDK.
Use one codebase in this all-in-one HTML5 development environment.
Design, debug & build mobile apps & 2D/3D high-impact games for multiple OSs.
http://pubads.g.doubleclick.net/gampad/clk?id=254741911&iu=/4140
Jason Gunthorpe Dec. 8, 2015, 9:28 p.m. UTC | #4
On Tue, Dec 08, 2015 at 08:20:37PM +0200, Jarkko Sakkinen wrote:
> On Tue, Dec 08, 2015 at 09:57:24AM -0700, Jason Gunthorpe wrote:
> > > Have you check from PC Client specification for constraints how they
> > > could be located?
> > 
> > I was not able to locate anything.
> 
> OK, then we cannot assume anything and doing what you do in "#if 0" is
> right thing to do from my perspective.

I'd like to see some results from real systems to decide what the
final version looks like, ie if it is a FW_BUG or not, or maybe tpm2
crb doesn't have a struct resource, or maybe it has multiple who knows.

Let me know when you get a chance to run this and I can finalize
things..

Jason

------------------------------------------------------------------------------
Jarkko Sakkinen Dec. 9, 2015, 6:42 a.m. UTC | #5
On Tue, Dec 08, 2015 at 02:28:23PM -0700, Jason Gunthorpe wrote:
> On Tue, Dec 08, 2015 at 08:20:37PM +0200, Jarkko Sakkinen wrote:
> > On Tue, Dec 08, 2015 at 09:57:24AM -0700, Jason Gunthorpe wrote:
> > > > Have you check from PC Client specification for constraints how they
> > > > could be located?
> > > 
> > > I was not able to locate anything.
> > 
> > OK, then we cannot assume anything and doing what you do in "#if 0" is
> > right thing to do from my perspective.
> 
> I'd like to see some results from real systems to decide what the
> final version looks like, ie if it is a FW_BUG or not, or maybe tpm2
> crb doesn't have a struct resource, or maybe it has multiple who knows.
> 
> Let me know when you get a chance to run this and I can finalize
> things..

OK

> Jason

/Jarkko

------------------------------------------------------------------------------
Jarkko Sakkinen Dec. 9, 2015, 9:45 p.m. UTC | #6
On Wed, Dec 09, 2015 at 08:42:21AM +0200, Jarkko Sakkinen wrote:
> On Tue, Dec 08, 2015 at 02:28:23PM -0700, Jason Gunthorpe wrote:
> > On Tue, Dec 08, 2015 at 08:20:37PM +0200, Jarkko Sakkinen wrote:
> > > On Tue, Dec 08, 2015 at 09:57:24AM -0700, Jason Gunthorpe wrote:
> > > > > Have you check from PC Client specification for constraints how they
> > > > > could be located?
> > > > 
> > > > I was not able to locate anything.
> > > 
> > > OK, then we cannot assume anything and doing what you do in "#if 0" is
> > > right thing to do from my perspective.
> > 
> > I'd like to see some results from real systems to decide what the
> > final version looks like, ie if it is a FW_BUG or not, or maybe tpm2
> > crb doesn't have a struct resource, or maybe it has multiple who knows.
> > 
> > Let me know when you get a chance to run this and I can finalize
> > things..
> 
> OK

When I modprobe tpm_tis force=1 I get:

[ 2547.438038] tpm_tis tpm_tis: 2.0 TPM (device-id 0x1A, rev-id 16)
[ 2547.750326] tpm_tis MSFT0101:00: can't request region for resource
[mem 0xfed40000-0xfed44fff]
[ 2547.750334] tpm_tis: probe of MSFT0101:00 failed with error -16

Without force everything works as expected.

/Jarkko

------------------------------------------------------------------------------
Jason Gunthorpe Dec. 9, 2015, 9:49 p.m. UTC | #7
On Wed, Dec 09, 2015 at 11:45:40PM +0200, Jarkko Sakkinen wrote:

> When I modprobe tpm_tis force=1 I get:
> 
> [ 2547.438038] tpm_tis tpm_tis: 2.0 TPM (device-id 0x1A, rev-id 16)
> [ 2547.750326] tpm_tis MSFT0101:00: can't request region for resource
> [mem 0xfed40000-0xfed44fff]
> [ 2547.750334] tpm_tis: probe of MSFT0101:00 failed with error -16

This is expected and fine, I'll add your Tested-by for the tis stuff

> Without force everything works as expected.

I'm looking for some results that use tpm_crb ..

Jason

------------------------------------------------------------------------------
Jarkko Sakkinen Dec. 10, 2015, 9:40 a.m. UTC | #8
On Wed, Dec 09, 2015 at 02:49:55PM -0700, Jason Gunthorpe wrote:
> On Wed, Dec 09, 2015 at 11:45:40PM +0200, Jarkko Sakkinen wrote:
> 
> > When I modprobe tpm_tis force=1 I get:
> > 
> > [ 2547.438038] tpm_tis tpm_tis: 2.0 TPM (device-id 0x1A, rev-id 16)
> > [ 2547.750326] tpm_tis MSFT0101:00: can't request region for resource
> > [mem 0xfed40000-0xfed44fff]
> > [ 2547.750334] tpm_tis: probe of MSFT0101:00 failed with error -16
> 
> This is expected and fine, I'll add your Tested-by for the tis stuff

Sorry to ask a stupid question but why this is expected and fine?

> > Without force everything works as expected.
> 
> I'm looking for some results that use tpm_crb ..

OK, I'll try that out.

> Jason

/Jarkko

------------------------------------------------------------------------------
Jason Gunthorpe Dec. 10, 2015, 6:12 p.m. UTC | #9
On Thu, Dec 10, 2015 at 11:40:23AM +0200, Jarkko Sakkinen wrote:
> On Wed, Dec 09, 2015 at 02:49:55PM -0700, Jason Gunthorpe wrote:
> > On Wed, Dec 09, 2015 at 11:45:40PM +0200, Jarkko Sakkinen wrote:
> > 
> > > When I modprobe tpm_tis force=1 I get:
> > > 
> > > [ 2547.438038] tpm_tis tpm_tis: 2.0 TPM (device-id 0x1A, rev-id 16)
> > > [ 2547.750326] tpm_tis MSFT0101:00: can't request region for resource
> > > [mem 0xfed40000-0xfed44fff]
> > > [ 2547.750334] tpm_tis: probe of MSFT0101:00 failed with error -16
> > 
> > This is expected and fine, I'll add your Tested-by for the tis stuff
> 
> Sorry to ask a stupid question but why this is expected and fine?

The new version for force doesn't inhibit auto detection, so the
force'd device attaches first:

> > > [ 2547.438038] tpm_tis tpm_tis: 2.0 TPM (device-id 0x1A, rev-id 16)

Then auto probing runs, and is blocked because the force driver took
over the address range first:

> > > [ 2547.750326] tpm_tis MSFT0101:00: can't request region for resource
> > > [mem 0xfed40000-0xfed44fff]

Using force when pnp is already working for the device is a user
mistake and something worth logging.

Jason

------------------------------------------------------------------------------
Jarkko Sakkinen Dec. 12, 2015, 4:19 p.m. UTC | #10
On Thu, Dec 10, 2015 at 11:40:23AM +0200, Jarkko Sakkinen wrote:
> On Wed, Dec 09, 2015 at 02:49:55PM -0700, Jason Gunthorpe wrote:
> > On Wed, Dec 09, 2015 at 11:45:40PM +0200, Jarkko Sakkinen wrote:
> > 
> > > When I modprobe tpm_tis force=1 I get:
> > > 
> > > [ 2547.438038] tpm_tis tpm_tis: 2.0 TPM (device-id 0x1A, rev-id 16)
> > > [ 2547.750326] tpm_tis MSFT0101:00: can't request region for resource
> > > [mem 0xfed40000-0xfed44fff]
> > > [ 2547.750334] tpm_tis: probe of MSFT0101:00 failed with error -16
> > 
> > This is expected and fine, I'll add your Tested-by for the tis stuff
> 
> Sorry to ask a stupid question but why this is expected and fine?
> 
> > > Without force everything works as expected.
> > 
> > I'm looking for some results that use tpm_crb ..
> 
> OK, I'll try that out.

First I did 'git pull https://github.com/jgunthorpe/linux for-jarkko'
on top of my master and compiled the kernel.

Then I run my own smoke tests:

jsakkine at jsakkine-tpm1 in ~/devel/tpm2-scripts (master‚óŹ)
$ sudo python -m unittest -v tpm2-smoke-test
test_seal_with_auth (tpm2-smoke-test.SmokeTest) ... ok
test_seal_with_policy (tpm2-smoke-test.SmokeTest) ... ok
test_seal_with_policy_script (tpm2-smoke-test.SmokeTest) ... ok
test_seal_with_too_long_auth (tpm2-smoke-test.SmokeTest) ... ok
test_unseal_with_wrong_auth (tpm2-smoke-test.SmokeTest) ... ok
test_unseal_with_wrong_policy (tpm2-smoke-test.SmokeTest) ... ok

----------------------------------------------------------------------
Ran 6 tests in 28.453s

OK

jsakkine at jsakkine-tpm1 in ~/devel/tpm2-scripts (master‚óŹ)
$ ./keyctl-smoke.sh
878930893

LGTM

/Jarkko

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

Patch
diff mbox

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 0237006dc4d8..131467e64321 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,126 @@  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);
+
+#if 0
+		return devm_ioremap_resource(dev, &tmp);
+#else
+		return ERR_PTR(-EINVAL);
+#endif
+	}
+
+	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 +325,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 +341,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);
+	rc = crb_map_io(device, priv, buf);
 	if (rc)
 		return rc;
 
-	chip->acpi_dev_handle = device->handle;
-
-	rc = tpm2_do_selftest(chip);
-	if (rc)
-		return rc;
-
-	return tpm_chip_register(chip);
+	return crb_init(device, priv);
 }
 
 static int crb_acpi_remove(struct acpi_device *device)