| Message ID | 20151207192258.GA27871@obsidianresearch.com |
|---|---|
| State | New |
| Headers | show |
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
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
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
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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 ------------------------------------------------------------------------------
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)