[PATCH] tpm_crb: fix: associate to the correct device

[PATCH] tpm_crb: fix: associate to the correct device

[Jarkko Sakkinen]

At the moment for tpm_crb /proc/iomem shows the HW interface and not the
device using the memory range. This patch fixes the issue by associating
memory mappings to the TPM character device.

The end result is this:

$ cat /proc/iomem|grep MSFT -A2
dbfff000-dbffffff : MSFT0101:00
  dbfff000-dbffffff : tpm0

Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>
---
 drivers/char/tpm/tpm_crb.c | 93 ++++++++++++++++++++++------------------------
 1 file changed, 45 insertions(+), 48 deletions(-)

diff --git a/drivers/char/tpm/tpm_crb.c b/drivers/char/tpm/tpm_crb.c
index 151689d..e0d9fbe 100644
--- a/drivers/char/tpm/tpm_crb.c
+++ b/drivers/char/tpm/tpm_crb.c
@@ -198,30 +198,6 @@ static const struct tpm_class_ops tpm_crb = {
 	.req_complete_val = CRB_STS_COMPLETE,
 };
 
-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;
@@ -256,32 +232,15 @@ static void __iomem *crb_map_res(struct device *dev, struct crb_priv *priv,
 	return priv->iobase + (new_res.start - priv->res.start);
 }
 
-static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
-		      struct acpi_table_tpm2 *buf)
+static int crb_map_io(struct device *dev, u64 cca_pa, struct crb_priv *priv)
 {
-	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_map_res(dev, priv, buf->control_address, 0x1000);
+	priv->cca = crb_map_res(dev, priv, cca_pa, 0x1000);
 	if (IS_ERR(priv->cca))
 		return PTR_ERR(priv->cca);
 
@@ -297,11 +256,41 @@ static int crb_map_io(struct acpi_device *device, struct crb_priv *priv,
 	return PTR_ERR_OR_ZERO(priv->rsp);
 }
 
+static int crb_init(struct acpi_device *device, u64 cca_pa,
+		    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 = crb_map_io(&chip->dev, cca_pa, priv);
+	if (rc)
+		return rc;
+
+	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_acpi_add(struct acpi_device *device)
 {
 	struct acpi_table_tpm2 *buf;
 	struct crb_priv *priv;
 	struct device *dev = &device->dev;
+	struct list_head resources;
 	acpi_status status;
 	u32 sm;
 	int rc;
@@ -322,6 +311,18 @@ static int crb_acpi_add(struct acpi_device *device)
 	if (!priv)
 		return -ENOMEM;
 
+	INIT_LIST_HEAD(&resources);
+	rc = acpi_dev_get_resources(device, &resources, crb_check_resource,
+				    priv);
+	if (rc < 0)
+		return rc;
+	acpi_dev_free_resource_list(&resources);
+
+	if (resource_type(&priv->res) != IORESOURCE_MEM) {
+		dev_err(dev, FW_BUG "The ACPI device does not have a memory resource\n");
+		return -EINVAL;
+	}
+
 	/* 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.
@@ -334,11 +335,7 @@ static int crb_acpi_add(struct acpi_device *device)
 	    sm == ACPI_TPM2_COMMAND_BUFFER_WITH_START_METHOD)
 		priv->flags |= CRB_FL_ACPI_START;
 
-	rc = crb_map_io(device, priv, buf);
-	if (rc)
-		return rc;
-
-	return crb_init(device, priv);
+	return crb_init(device, buf->control_address, priv);
 }
 
 static int crb_acpi_remove(struct acpi_device *device)
-- 
2.7.0

Re: [tpmdd-devel] [PATCH] tpm_crb: fix: associate to the correct device

[Jason Gunthorpe]

On Wed, Feb 17, 2016 at 01:23:31PM +0200, Jarkko Sakkinen wrote:
> At the moment for tpm_crb /proc/iomem shows the HW interface and not the
> device using the memory range. This patch fixes the issue by associating
> memory mappings to the TPM character device.
> 
> The end result is this:
> 
> $ cat /proc/iomem|grep MSFT -A2
> dbfff000-dbffffff : MSFT0101:00
>   dbfff000-dbffffff : tpm0

Hmmm... This large patch basically changes devm_ioremap so it uses the
tpm chip dev not the acpi_dev - which seems reasonable - however I
notice lots of drivers do use the parent device..

However, it will change the unwind ordering on remove.. And when we
check if tpm_crb is OK on that point I see this bug:

static int crb_acpi_remove(struct acpi_device *device)
{
[..]
        tpm_chip_unregister(chip);
        if (chip->flags & TPM_CHIP_FLAG_TPM2)
	                tpm2_shutdown(chip, TPM2_SU_CLEAR);

It is illegal to reference chip after tpm_chip_unregister, and it is
very illegal to try to send a command to an unregistered tpm. This
will conflict with my other patches to fix ops locking.

Further, it probably conflicts with this patch, since the iomapping is
associated with chip, it risks being torn down by tpm_chip_unregister
when it triggers the devm unwind.

Something like tpm2_shutdown belongs in the core code, add it to
tpm_chip_unregister, and do that before this patch..

Ideally this would have been fewer lines, not sure why the acpi stuff
had to move out of crb_map_io, for instance..

Does tpm_tis need the same fix?

> +	INIT_LIST_HEAD(&resources);
> +	rc = acpi_dev_get_resources(device, &resources, crb_check_resource,
> +				    priv);
> +	if (rc < 0)
> +		return rc;
> +	acpi_dev_free_resource_list(&resources);

I wonder if the acpi_dev_free_resource_list is what kill's the
resource name? Maybe that needs to be delayed until after the
ioremap, which is best done with this back in crb_map_io  ??

Thanks for looking at this stuff,
Jason

Re: [tpmdd-devel] [PATCH] tpm_crb: fix: associate to the correct device

[Jarkko Sakkinen]

On Thu, Feb 18, 2016 at 11:03:31AM -0700, Jason Gunthorpe wrote:
> On Wed, Feb 17, 2016 at 01:23:31PM +0200, Jarkko Sakkinen wrote:
> > At the moment for tpm_crb /proc/iomem shows the HW interface and not the
> > device using the memory range. This patch fixes the issue by associating
> > memory mappings to the TPM character device.
> > 
> > The end result is this:
> > 
> > $ cat /proc/iomem|grep MSFT -A2
> > dbfff000-dbffffff : MSFT0101:00
> >   dbfff000-dbffffff : tpm0
> 
> Hmmm... This large patch basically changes devm_ioremap so it uses the
> tpm chip dev not the acpi_dev - which seems reasonable - however I
> notice lots of drivers do use the parent device..
> 
> However, it will change the unwind ordering on remove.. And when we
> check if tpm_crb is OK on that point I see this bug:
> 
> static int crb_acpi_remove(struct acpi_device *device)
> {
> [..]
>         tpm_chip_unregister(chip);
>         if (chip->flags & TPM_CHIP_FLAG_TPM2)
> 	                tpm2_shutdown(chip, TPM2_SU_CLEAR);

Good catch, thanks!

/Jarkko

Re: [tpmdd-devel] [PATCH] tpm_crb: fix: associate to the correct device

[Jarkko Sakkinen]

On Thu, Feb 18, 2016 at 11:03:31AM -0700, Jason Gunthorpe wrote:
> > +	INIT_LIST_HEAD(&resources);
> > +	rc = acpi_dev_get_resources(device, &resources, crb_check_resource,
> > +				    priv);
> > +	if (rc < 0)
> > +		return rc;
> > +	acpi_dev_free_resource_list(&resources);
> 
> I wonder if the acpi_dev_free_resource_list is what kill's the
> resource name? Maybe that needs to be delayed until after the
> ioremap, which is best done with this back in crb_map_io  ??
> 
> Thanks for looking at this stuff,

I had almost forgotten this comment. I'll try this out as soon as
possible. Might well be the reason.

> Jason

/Jarkko