RE: TPM driver changes to support multiple locality

[Agarwal, Lomesh]

Here is info. -

This patch adds multiple locality support in tpm_tis driver.
drivers/char/tpm/tpm_tis.c |   83 ++++++++++++++++++++++++-----------
 1 file changed, 57 insertions(+), 26 deletions(-)

--- pristine-linux-2.6.18/drivers/char/tpm/tpm_tis.c	2006-09-19
20:42:06.000000000 -0700
+++ linux-2.6.18-xen/drivers/char/tpm/tpm_tis.c	2007-10-11
13:28:38.000000000 -0700
@@ -56,29 +56,37 @@ enum tis_int_flags {
 
 enum tis_defaults {
 	TIS_MEM_BASE = 0xFED40000,
-	TIS_MEM_LEN = 0x5000,
+    TIS_LOCALITY_SHIFT = 12,
+	TIS_MEM_LEN = 0x1000,
 	TIS_SHORT_TIMEOUT = 750,	/* ms */
 	TIS_LONG_TIMEOUT = 2000,	/* 2 sec */
 };
 
-#define	TPM_ACCESS(l)			(0x0000 | ((l) << 12))
-#define	TPM_INT_ENABLE(l)		(0x0008 | ((l) << 12))
-#define	TPM_INT_VECTOR(l)		(0x000C | ((l) << 12))
-#define	TPM_INT_STATUS(l)		(0x0010 | ((l) << 12))
-#define	TPM_INTF_CAPS(l)		(0x0014 | ((l) << 12))
-#define	TPM_STS(l)			(0x0018 | ((l) << 12))
-#define	TPM_DATA_FIFO(l)		(0x0024 | ((l) << 12))
+#define	TPM_ACCESS(l)			(0x0000)
+#define	TPM_INT_ENABLE(l)		(0x0008)
+#define	TPM_INT_VECTOR(l)		(0x000C)
+#define	TPM_INT_STATUS(l)		(0x0010)
+#define	TPM_INTF_CAPS(l)		(0x0014)
+#define	TPM_STS(l)			(0x0018)
+#define	TPM_DATA_FIFO(l)		(0x0024)
 
-#define	TPM_DID_VID(l)			(0x0F00 | ((l) << 12))
-#define	TPM_RID(l)			(0x0F04 | ((l) << 12))
+#define	TPM_DID_VID(l)			(0x0F00)
+#define	TPM_RID(l)			(0x0F04)
 
 static LIST_HEAD(tis_chips);
 static DEFINE_SPINLOCK(tis_lock);
 
 static int check_locality(struct tpm_chip *chip, int l)
 {
-	if ((ioread8(chip->vendor.iobase + TPM_ACCESS(l)) &
-	     (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) ==
+    unsigned char tpm_access;
+
+	tpm_access = ioread8(chip->vendor.iobase + TPM_ACCESS(l));
+
+    /* check if locality is closed */
+    if(tpm_access == 0xFF)
+        return -1;
+
+	if ((tpm_access & (TPM_ACCESS_ACTIVE_LOCALITY |
TPM_ACCESS_VALID)) ==
 	    (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID))
 		return chip->vendor.locality = l;
 
@@ -251,10 +259,14 @@ static int tpm_tis_recv(struct tpm_chip 
 
 out:
 	tpm_tis_ready(chip);
-	release_locality(chip, chip->vendor.locality, 0);
+	release_locality(chip, chip->vendor.locality, 1);
 	return size;
 }
 
+static int locality = 0;
+module_param(locality, int, 0444);
+MODULE_PARM_DESC(locality, "TPM Locality To access");
+
 /*
  * If interrupts are used (signaled by an irq set in the vendor
structure)
  * tpm.c can skip polling for the data to be available as the interrupt
is
@@ -266,7 +278,7 @@ static int tpm_tis_send(struct tpm_chip 
 	size_t count = 0;
 	u32 ordinal;
 
-	if (request_locality(chip, 0) < 0)
+	if (request_locality(chip, locality) < 0)
 		return -EBUSY;
 
 	status = tpm_tis_status(chip);
@@ -326,7 +338,7 @@ static int tpm_tis_send(struct tpm_chip 
 	return len;
 out_err:
 	tpm_tis_ready(chip);
-	release_locality(chip, chip->vendor.locality, 0);
+	release_locality(chip, chip->vendor.locality, 1);
 	return rc;
 }
 
@@ -401,7 +413,10 @@ static irqreturn_t tis_int_handler(int i
 {
 	struct tpm_chip *chip = (struct tpm_chip *) dev_id;
 	u32 interrupt;
-	int i;
+
+        /* check if interrupt is meant for this locality */
+        if(check_locality(chip, locality) < 0)
+            return IRQ_NONE;
 
 	interrupt = ioread32(chip->vendor.iobase +
 			     TPM_INT_STATUS(chip->vendor.locality));
@@ -411,10 +426,6 @@ static irqreturn_t tis_int_handler(int i
 
 	if (interrupt & TPM_INTF_DATA_AVAIL_INT)
 		wake_up_interruptible(&chip->vendor.read_queue);
-	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
-		for (i = 0; i < 5; i++)
-			if (check_locality(chip, i) >= 0)
-				break;
 	if (interrupt &
 	    (TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_STS_VALID_INT |
 	     TPM_INTF_CMD_READY_INT))
@@ -440,7 +451,7 @@ static int tpm_tis_init(struct device *d
 	struct tpm_chip *chip;
 
 	if (!start)
-		start = TIS_MEM_BASE;
+		start = TIS_MEM_BASE | (locality << TIS_LOCALITY_SHIFT);
 	if (!len)
 		len = TIS_MEM_LEN;
 
@@ -490,8 +501,9 @@ static int tpm_tis_init(struct device *d
 	if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
 		dev_dbg(dev, "\tData Avail Int Support\n");
 
-	if (request_locality(chip, 0) != 0) {
-		rc = -ENODEV;
+	if (request_locality(chip, locality) < 0) {
+		rc = -EBUSY;
+		printk("tpm_tis: failed request_locality %d\n",
locality);
 		goto out_err;
 	}
 
@@ -582,9 +594,11 @@ static int tpm_tis_init(struct device *d
 
 	tpm_get_timeouts(chip);
 	tpm_continue_selftest(chip);
+	release_locality(chip, chip->vendor.locality, 1);
 
 	return 0;
 out_err:
+	release_locality(chip, chip->vendor.locality, 1);
 	if (chip->vendor.iobase)
 		iounmap(chip->vendor.iobase);
 	tpm_remove_hardware(chip->dev);
@@ -636,7 +650,7 @@ module_param_string(hid, tpm_pnp_tbl[TIS
 MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to
probe");
 
 static struct device_driver tis_drv = {
-	.name = "tpm_tis",
+	.name = "",
 	.bus = &platform_bus_type,
 	.owner = THIS_MODULE,
 	.suspend = tpm_pm_suspend,
@@ -648,19 +662,34 @@ static struct platform_device *pdev;
 static int force;
 module_param(force, bool, 0444);
 MODULE_PARM_DESC(force, "Force device probe rather than using ACPI
entry");
+
+static char *devname;
+
 static int __init init_tis(void)
 {
+#define DEVNAME_SIZE 10
+
 	int rc;
 
+	if ((locality < 0) || (locality > 4)) {
+		return PTR_ERR(pdev);
+	}
+
 	if (force) {
+		devname = kmalloc(DEVNAME_SIZE, GFP_KERNEL);
+        	scnprintf(devname, DEVNAME_SIZE, "%s%d", "tpm_tis",
locality);
+
+		tis_drv.name = devname;
 		rc = driver_register(&tis_drv);
 		if (rc < 0)
 			return rc;
-		if
(IS_ERR(pdev=platform_device_register_simple("tpm_tis", -1, NULL, 0)))
+
+		if (IS_ERR(pdev=platform_device_register_simple(devname,
-1, NULL, 0)))
 			return PTR_ERR(pdev);
 		if((rc=tpm_tis_init(&pdev->dev, 0, 0)) != 0) {
 			platform_device_unregister(pdev);
 			driver_unregister(&tis_drv);
+			kfree(devname);
 		}
 		return rc;
 	}
@@ -692,7 +721,9 @@ static void __exit cleanup_tis(void)
 	if (force) {
 		platform_device_unregister(pdev);
 		driver_unregister(&tis_drv);
-	} else
+		kfree(devname);
+	}
+	else 
 		pnp_unregister_driver(&tis_pnp_driver);
 }

I don't have scripts/checkpatch.pl in my linux tree. Where can I get
one?
Devname is being used in 2 functions. That's why it is global. Locality
parameter is initialized with 0 just to be safe. Are all the global
variables in driver is guaranteed to be init 0? Even if it is it doesn't
hurt to init it.

-----Original Message-----
From: Randy Dunlap [mailto:randy.dunlap@xxxxxxxxxx] 
Sent: Thursday, October 11, 2007 11:54 AM
To: Agarwal, Lomesh
Cc: Valdis.Kletnieks@xxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: TPM driver changes to support multiple locality

On Thu, 11 Oct 2007 11:33:35 -0700 Agarwal, Lomesh wrote:

> Below is the patch for TPM driver.
> Comments/suggestions?

Observe/use kernel coding style.
Run the patch thru scripts/checkpatch.pl and check its suggestions.
Use "diffstat -p1 -w70" and put that summary near the top of the
patch (after the patch description).

Use -p option of diff to generate the diff so that reviewers
can see the function name that patch blocks apply to.

Use tabs instead of spaces for indenting.
More below.

> --- pristine-linux-2.6.18/drivers/char/tpm/tpm_tis.c	2006-09-19
> 20:42:06.000000000 -0700
> +++ linux-2.6.18-xen/drivers/char/tpm/tpm_tis.c	2007-10-09
> 15:30:49.000000000 -0700
> @@ -56,29 +56,36 @@
>  
>  enum tis_defaults {
>  	TIS_MEM_BASE = 0xFED40000,
> -	TIS_MEM_LEN = 0x5000,
> +	TIS_MEM_LEN = 0x1000,
>  	TIS_SHORT_TIMEOUT = 750,	/* ms */
>  	TIS_LONG_TIMEOUT = 2000,	/* 2 sec */
>  };
>  
> -#define	TPM_ACCESS(l)			(0x0000 | ((l) << 12))
> -#define	TPM_INT_ENABLE(l)		(0x0008 | ((l) << 12))
> -#define	TPM_INT_VECTOR(l)		(0x000C | ((l) << 12))
> -#define	TPM_INT_STATUS(l)		(0x0010 | ((l) << 12))
> -#define	TPM_INTF_CAPS(l)		(0x0014 | ((l) << 12))
> -#define	TPM_STS(l)			(0x0018 | ((l) << 12))
> -#define	TPM_DATA_FIFO(l)		(0x0024 | ((l) << 12))
> +#define	TPM_ACCESS(l)			(0x0000)
> +#define	TPM_INT_ENABLE(l)		(0x0008)
> +#define	TPM_INT_VECTOR(l)		(0x000C)
> +#define	TPM_INT_STATUS(l)		(0x0010)
> +#define	TPM_INTF_CAPS(l)		(0x0014)
> +#define	TPM_STS(l)			(0x0018)
> +#define	TPM_DATA_FIFO(l)		(0x0024)
>  
> -#define	TPM_DID_VID(l)			(0x0F00 | ((l) << 12))
> -#define	TPM_RID(l)			(0x0F04 | ((l) << 12))
> +#define	TPM_DID_VID(l)			(0x0F00)
> +#define	TPM_RID(l)			(0x0F04)
>  
>  static LIST_HEAD(tis_chips);
>  static DEFINE_SPINLOCK(tis_lock);
>  
>  static int check_locality(struct tpm_chip *chip, int l)
>  {
> -	if ((ioread8(chip->vendor.iobase + TPM_ACCESS(l)) &
> -	     (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID)) ==
> +    unsigned char tpm_access;
> +

Indent above/below the same amount (1 tab, not spaces).

> +	tpm_access = ioread8(chip->vendor.iobase + TPM_ACCESS(l));
> +
> +    /* check if locality is closed */
> +	if(tpm_access == 0xFF)

	if (

> +        return -1;
> +
> +	if((tpm_access & (TPM_ACCESS_ACTIVE_LOCALITY |

	if ((

> TPM_ACCESS_VALID)) ==
>  	    (TPM_ACCESS_ACTIVE_LOCALITY | TPM_ACCESS_VALID))
>  		return chip->vendor.locality = l;
>  
> @@ -251,10 +258,14 @@
>  
>  out:
>  	tpm_tis_ready(chip);
> -	release_locality(chip, chip->vendor.locality, 0);
> +	release_locality(chip, chip->vendor.locality, 1);
>  	return size;
>  }
>  
> +static int locality = 0;

No need to init this to 0.

> +module_param(locality, int, 0444);
> +MODULE_PARM_DESC(locality, "TPM Locality To access");
> +
>  /*
>   * If interrupts are used (signaled by an irq set in the vendor
> structure)
>   * tpm.c can skip polling for the data to be available as the
interrupt
> is
> @@ -266,7 +277,7 @@
>  	size_t count = 0;
>  	u32 ordinal;
>  
> -	if (request_locality(chip, 0) < 0)
> +	if (request_locality(chip, locality) < 0)
>  		return -EBUSY;
>  
>  	status = tpm_tis_status(chip);
> @@ -326,7 +337,7 @@
>  	return len;
>  out_err:
>  	tpm_tis_ready(chip);
> -	release_locality(chip, chip->vendor.locality, 0);
> +	release_locality(chip, chip->vendor.locality, 1);
>  	return rc;
>  }
>  
> @@ -401,7 +412,10 @@
>  {
>  	struct tpm_chip *chip = (struct tpm_chip *) dev_id;
>  	u32 interrupt;
> -	int i;
> +
> +        /* check if interrupt is meant for this locality */
> +        if(check_locality(chip, locality) < 0)
> +                return IRQ_NONE;
>  

Use same indenting as surrounding code.

>  	interrupt = ioread32(chip->vendor.iobase +
>  			     TPM_INT_STATUS(chip->vendor.locality));
> @@ -411,10 +425,6 @@
>  
>  	if (interrupt & TPM_INTF_DATA_AVAIL_INT)
>  		wake_up_interruptible(&chip->vendor.read_queue);
> -	if (interrupt & TPM_INTF_LOCALITY_CHANGE_INT)
> -		for (i = 0; i < 5; i++)
> -			if (check_locality(chip, i) >= 0)
> -				break;
>  	if (interrupt &
>  	    (TPM_INTF_LOCALITY_CHANGE_INT | TPM_INTF_STS_VALID_INT |
>  	     TPM_INTF_CMD_READY_INT))
> @@ -440,7 +450,7 @@
>  	struct tpm_chip *chip;
>  
>  	if (!start)
> -		start = TIS_MEM_BASE;
> +		start = TIS_MEM_BASE | (locality << 12);

Looks like all of those "<< 12"s (mostly in header file) could use
some helpers.

>  	if (!len)
>  		len = TIS_MEM_LEN;
>  
> @@ -490,8 +500,9 @@
>  	if (intfcaps & TPM_INTF_DATA_AVAIL_INT)
>  		dev_dbg(dev, "\tData Avail Int Support\n");
>  
> -	if (request_locality(chip, 0) != 0) {
> -		rc = -ENODEV;
> +	if (request_locality(chip, locality) < 0) {
> +		rc = -EBUSY;
> +		printk("failed request_locality\n");

Use some prefix identity string that tells the use what module
failed here.

>  		goto out_err;
>  	}
>  
> @@ -582,9 +593,11 @@
>  
>  	tpm_get_timeouts(chip);
>  	tpm_continue_selftest(chip);
> +	release_locality(chip, chip->vendor.locality, 1);
>  
>  	return 0;
>  out_err:
> +	release_locality(chip, chip->vendor.locality, 1);
>  	if (chip->vendor.iobase)
>  		iounmap(chip->vendor.iobase);
>  	tpm_remove_hardware(chip->dev);
> @@ -636,7 +649,7 @@
>  MODULE_PARM_DESC(hid, "Set additional specific HID for this driver to
> probe");
>  
>  static struct device_driver tis_drv = {
> -	.name = "tpm_tis",
> +	.name = "",
>  	.bus = &platform_bus_type,
>  	.owner = THIS_MODULE,
>  	.suspend = tpm_pm_suspend,
> @@ -648,19 +661,34 @@
>  static int force;
>  module_param(force, bool, 0444);
>  MODULE_PARM_DESC(force, "Force device probe rather than using ACPI
> entry");
> +
> +static char *devname = NULL;

No need to init to 0 or NULL.

But why is devname global here?
could it be in the function below or is it used later?
A quick scan finds only local function use below...


> +
>  static int __init init_tis(void)
>  {
> +#define DEVNAME_SIZE 10
> +
>  	int rc;
>  
> +	if ((locality < 0) || (locality > 4)) {
> +		return PTR_ERR(pdev);
> +	}
> +
>  	if (force) {
> +		devname = kmalloc(DEVNAME_SIZE, GFP_KERNEL);
> +        	scnprintf(devname, DEVNAME_SIZE, "%s%d", "tpm_tis",
> locality);
> +
> +		tis_drv.name = devname;
>  		rc = driver_register(&tis_drv);
>  		if (rc < 0)
>  			return rc;
> -		if
> (IS_ERR(pdev=platform_device_register_simple("tpm_tis", -1, NULL, 0)))
> +
> +		if (IS_ERR(pdev=platform_device_register_simple(devname,
> -1, NULL, 0)))
>  			return PTR_ERR(pdev);
>  		if((rc=tpm_tis_init(&pdev->dev, 0, 0)) != 0) {
>  			platform_device_unregister(pdev);
>  			driver_unregister(&tis_drv);
> +			kfree(devname);
>  		}
>  		return rc;
>  	}
> @@ -692,7 +720,9 @@
>  	if (force) {
>  		platform_device_unregister(pdev);
>  		driver_unregister(&tis_drv);
> -	} else
> +		kfree(devname);
> +	}
> +	else 
>  		pnp_unregister_driver(&tis_pnp_driver);
>  }

---
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/