RE: TPM driver changes to support multiple locality
-Original Message-
From: Randy Dunlap [mailto:[EMAIL PROTECTED]
Sent: Thursday, October 11, 2007 4:02 PM
To: Agarwal, Lomesh
Cc: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org
Subject: Re: TPM driver changes to support multiple locality
On Thu, 11 Oct 2007 15:46:03 -0700 Agarwal, Lomesh wrote:
> Attached is the patch which resolves all the comments.
Inline patches are preferred so that reviewers can comment on them
more easily.
What mail client are you using?
[Agarwal, Lomesh] I am using MS Outlook. Earlier you said inline patches
have problem because of mail client. That's why I sent it as attachment.
The patch has trailing CRs on each line ("DOS mode").
Just verifying: this TPM device has interrupts per locality?
+ /* check if interrupt is meant for this locality */
+ if (check_locality(chip, locality) < 0)
+ return IRQ_NONE;
~
[Agarwal, Lomesh] TPM device has only one interrupt. so on receiving
interrupt driver has to make sure that its meant for its locality.
init_tis() still seems to have some problems.
static int __init init_tis(void)
{
+#define DEVNAME_SIZE 10
+
int rc;
+ if ((locality < 0) || (locality > 4))
+ return PTR_ERR(pdev);
pdev hasn't been set (so it's NULL).
+ pdev = platform_device_register_simple(devname, -1,
NULL, 0);
+ if (IS_ERR(pdev))
return PTR_ERR(pdev);
Error path above needs to call driver_unregister().
[Agarwal, Lomesh] attached is the new patch.
---
~Randy
tpm_tis.c.patch
Description: tpm_tis.c.patch
Re: TPM driver changes to support multiple locality
On Thu, 11 Oct 2007 15:46:03 -0700 Agarwal, Lomesh wrote:
> Attached is the patch which resolves all the comments.
Inline patches are preferred so that reviewers can comment on them
more easily.
What mail client are you using?
The patch has trailing CRs on each line ("DOS mode").
Just verifying: this TPM device has interrupts per locality?
+ /* check if interrupt is meant for this locality */
+ if (check_locality(chip, locality) < 0)
+ return IRQ_NONE;
~
init_tis() still seems to have some problems.
static int __init init_tis(void)
{
+#define DEVNAME_SIZE 10
+
int rc;
+ if ((locality < 0) || (locality > 4))
+ return PTR_ERR(pdev);
pdev hasn't been set (so it's NULL).
+ pdev = platform_device_register_simple(devname, -1, NULL, 0);
+ if (IS_ERR(pdev))
return PTR_ERR(pdev);
Error path above needs to call driver_unregister().
---
~Randy
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: TPM driver changes to support multiple locality
Attached is the patch which resolves all the comments. tpm_tis.c.patch Description: tpm_tis.c.patch
RE: TPM driver changes to support multiple locality
-Original Message-
From: Arjan van de Ven [mailto:[EMAIL PROTECTED]
Sent: Thursday, October 11, 2007 2:41 PM
To: Agarwal, Lomesh
Cc: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org
Subject: Re: TPM driver changes to support multiple locality
On Thu, 11 Oct 2007 11:33:35 -0700
"Agarwal, Lomesh" <[EMAIL PROTECTED]> wrote:
> Below is the patch for TPM driver.
> Comments/suggestions?
please don't top post
>
> --- pristine-linux-2.6.18/drivers/char/tpm/tpm_tis.c 2006-09-19
> 20:42:06.0 -0700
> +++ linux-2.6.18-xen/drivers/char/tpm/tpm_tis.c 2007-10-09
> 15:30:49.0 -0700
or send word wrapped patches
.. or patches against 2 year old kernels for that matter ;)
> enum tis_defaults {
> TIS_MEM_BASE = 0xFED4,
> - TIS_MEM_LEN = 0x5000,
> + TIS_MEM_LEN = 0x1000,
hmmm/// why
[Agarwal, Lomesh] 0x5000 is the length of CSRs for all the localities.
Each locality's CSR size is 0x1000. so 0x1000 is sufficient and will
catch error if driver tries to write to wrong locality.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: TPM driver changes to support multiple locality
On Thu, 11 Oct 2007 11:33:35 -0700
"Agarwal, Lomesh" <[EMAIL PROTECTED]> wrote:
> Below is the patch for TPM driver.
> Comments/suggestions?
please don't top post
>
> --- pristine-linux-2.6.18/drivers/char/tpm/tpm_tis.c 2006-09-19
> 20:42:06.0 -0700
> +++ linux-2.6.18-xen/drivers/char/tpm/tpm_tis.c 2007-10-09
> 15:30:49.0 -0700
or send word wrapped patches
.. or patches against 2 year old kernels for that matter ;)
> enum tis_defaults {
> TIS_MEM_BASE = 0xFED4,
> - TIS_MEM_LEN = 0x5000,
> + TIS_MEM_LEN = 0x1000,
hmmm/// why
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: TPM driver changes to support multiple locality
On Thu, 11 Oct 2007 14:08:17 -0700 Agarwal, Lomesh wrote:
> Here is info. -
>
> This patch adds multiple locality support in tpm_tis driver.
whatever that means. I guess anyone who is familiar with TPM
knows what it means, and others can do research to find out.
Or the patch description could add another 2 lines for it (?).
> drivers/char/tpm/tpm_tis.c | 83 ---
> 1 file changed, 57 insertions(+), 26 deletions(-)
Thanks for that.
> --- pristine-linux-2.6.18/drivers/char/tpm/tpm_tis.c 2006-09-19
> 20:42:06.0 -0700
> +++ linux-2.6.18-xen/drivers/char/tpm/tpm_tis.c 2007-10-11
> 13:28:38.0 -0700
> @@ -56,29 +56,37 @@ enum tis_int_flags {
>
> enum tis_defaults {
> TIS_MEM_BASE = 0xFED4,
> - TIS_MEM_LEN = 0x5000,
> +TIS_LOCALITY_SHIFT = 12,
Indent above uses spaces, not tab.
> + TIS_MEM_LEN = 0x1000,
> TIS_SHORT_TIMEOUT = 750,/* ms */
> TIS_LONG_TIMEOUT = 2000,/* 2 sec */
> };
>
> -#define TPM_ACCESS(l) (0x | ((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) (0x)
> +#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;
Use tab above, not spaces.
> +
> + tpm_access = ioread8(chip->vendor.iobase + TPM_ACCESS(l));
> +
> +/* check if locality is closed */
> +if(tpm_access == 0xFF)
if (
> +return -1;
and use tab(s) for indent, not spaces.
> +
> + 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;
Don't init statics to 0 or NULL (it's done for you).
(same comment as first time)
> +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
> @@ -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)
if (
and use tab(s) to indent, not spaces.
> +return IRQ_NONE;
>
> interrupt = ioread32(chip->vendor.iobase +
>TPM_INT_STATUS(chip->vendor.locality));
> @@ -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);
printk() usually should have a facility level, like
KERN_WARNING:
printk(KERN_WARNING "tpm_tis: failed request_locality %d\n",
locality);
> goto out_err;
> }
>
> @@ -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;
>
RE: TPM driver changes to support multiple locality
ot;\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 = _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(_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(>dev, 0, 0)) != 0) {
platform_device_unregister(pdev);
driver_unregister(_drv);
+ kfree(devname);
}
return rc;
}
@@ -692,7 +721,9 @@ static void __exit cleanup_tis(void)
if (force) {
platform_device_unregister(pdev);
driver_unregister(_drv);
- } else
+ kfree(devname);
+ }
+ else
pnp_unregister_driver(_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:[EMAIL PROTECTED]
Sent: Thursday, October 11, 2007 11:54 AM
To: Agarwal, Lomesh
Cc: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org
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.0 -0700
> +++ linux-2.6.18-xen/drivers/char/tpm/tpm_tis.c 2007-10-09
> 15:30:49.0 -0700
> @@ -56,29 +56,36 @@
>
> enum tis_defaults {
> TIS_MEM_BASE = 0xFED4,
> - TIS_MEM_LEN = 0x5000,
> + TIS_MEM_LEN = 0x1000,
> TIS_SHORT_TIMEOUT = 750,/* ms */
> TIS_LONG_TIMEOUT = 2000,/* 2 sec */
> };
>
> -#define TPM_ACCESS(l) (0x | ((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) (0x)
> +#define TPM_INT_ENABLE(l) (0x0008)
> +#define TPM_INT_VECTOR(l) (0x000C)
> +#define TPM_INT_STATUS(l) (0x0010)
> +#
Re: TPM driver changes to support multiple locality
On Oct 11 2007 11:54, Randy Dunlap wrote: >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). Or, if using quilt on top of the tree, a simple `quilt diff --diffstat --sort` will do all the nice diffing, including stat and use diff -pu. It also preserves diffstat (if not regenerating that) and patch description, so is suited *perfectly* for the task when you don't want to use stgit. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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.0 -0700
> +++ linux-2.6.18-xen/drivers/char/tpm/tpm_tis.c 2007-10-09
> 15:30:49.0 -0700
> @@ -56,29 +56,36 @@
>
> enum tis_defaults {
> TIS_MEM_BASE = 0xFED4,
> - TIS_MEM_LEN = 0x5000,
> + TIS_MEM_LEN = 0x1000,
> TIS_SHORT_TIMEOUT = 750,/* ms */
> TIS_LONG_TIMEOUT = 2000,/* 2 sec */
> };
>
> -#define TPM_ACCESS(l) (0x | ((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) (0x)
> +#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(>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
RE: TPM driver changes to support multiple locality
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 = _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;
+
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(_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(>dev, 0, 0)) != 0) {
platform_device_unregister(pdev);
driver_unregister(_drv);
+ kfree(devname);
}
return rc;
}
@@ -692,7 +720,9 @@
if (force) {
platform_device_unregister(pdev);
driver_unregister(_drv);
- } else
+ kfree(devname);
+ }
+ else
pnp_unregister_driver(_pnp_driver);
}
-Original Message-
From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]
Sent: Wednesday, October 10, 2007 12:46 PM
To: Agarwal, Lomesh
Cc: linux-kernel@vger.kernel.org
Subject: Re: TPM driver changes to support multiple locality
On Tue, 09 Oct 2007 15:51:11 PDT, "Agarwal, Lomesh" said:
> Current TPM driver supports only locality 0. I am planning to add
> support so that it can access any locality. Locality parameter will be
> passed as parameter. Will this change be acceptable? If yes then I
will
> modify the driver and send the patch.
Make sure you extend the API in such a way that older userspace programs
that don't pass the new locality parameter still work correctly.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: TPM driver changes to support multiple locality
, 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;
+
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);
}
-Original Message-
From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED]
Sent: Wednesday, October 10, 2007 12:46 PM
To: Agarwal, Lomesh
Cc: linux-kernel@vger.kernel.org
Subject: Re: TPM driver changes to support multiple locality
On Tue, 09 Oct 2007 15:51:11 PDT, Agarwal, Lomesh said:
Current TPM driver supports only locality 0. I am planning to add
support so that it can access any locality. Locality parameter will be
passed as parameter. Will this change be acceptable? If yes then I
will
modify the driver and send the patch.
Make sure you extend the API in such a way that older userspace programs
that don't pass the new locality parameter still work correctly.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: TPM driver changes to support multiple locality
On Oct 11 2007 11:54, Randy Dunlap wrote: 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). Or, if using quilt on top of the tree, a simple `quilt diff --diffstat --sort` will do all the nice diffing, including stat and use diff -pu. It also preserves diffstat (if not regenerating that) and patch description, so is suited *perfectly* for the task when you don't want to use stgit. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
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.0 -0700
+++ linux-2.6.18-xen/drivers/char/tpm/tpm_tis.c 2007-10-09
15:30:49.0 -0700
@@ -56,29 +56,36 @@
enum tis_defaults {
TIS_MEM_BASE = 0xFED4,
- TIS_MEM_LEN = 0x5000,
+ TIS_MEM_LEN = 0x1000,
TIS_SHORT_TIMEOUT = 750,/* ms */
TIS_LONG_TIMEOUT = 2000,/* 2 sec */
};
-#define TPM_ACCESS(l) (0x | ((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) (0x)
+#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 12s (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
RE: TPM driver changes to support multiple locality
= -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:[EMAIL PROTECTED]
Sent: Thursday, October 11, 2007 11:54 AM
To: Agarwal, Lomesh
Cc: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org
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.0 -0700
+++ linux-2.6.18-xen/drivers/char/tpm/tpm_tis.c 2007-10-09
15:30:49.0 -0700
@@ -56,29 +56,36 @@
enum tis_defaults {
TIS_MEM_BASE = 0xFED4,
- TIS_MEM_LEN = 0x5000,
+ TIS_MEM_LEN = 0x1000,
TIS_SHORT_TIMEOUT = 750,/* ms */
TIS_LONG_TIMEOUT = 2000,/* 2 sec */
};
-#define TPM_ACCESS(l) (0x | ((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) (0x)
+#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
Re: TPM driver changes to support multiple locality
On Thu, 11 Oct 2007 14:08:17 -0700 Agarwal, Lomesh wrote:
Here is info. -
This patch adds multiple locality support in tpm_tis driver.
whatever that means. I guess anyone who is familiar with TPM
knows what it means, and others can do research to find out.
Or the patch description could add another 2 lines for it (?).
drivers/char/tpm/tpm_tis.c | 83 ---
1 file changed, 57 insertions(+), 26 deletions(-)
Thanks for that.
--- pristine-linux-2.6.18/drivers/char/tpm/tpm_tis.c 2006-09-19
20:42:06.0 -0700
+++ linux-2.6.18-xen/drivers/char/tpm/tpm_tis.c 2007-10-11
13:28:38.0 -0700
@@ -56,29 +56,37 @@ enum tis_int_flags {
enum tis_defaults {
TIS_MEM_BASE = 0xFED4,
- TIS_MEM_LEN = 0x5000,
+TIS_LOCALITY_SHIFT = 12,
Indent above uses spaces, not tab.
+ TIS_MEM_LEN = 0x1000,
TIS_SHORT_TIMEOUT = 750,/* ms */
TIS_LONG_TIMEOUT = 2000,/* 2 sec */
};
-#define TPM_ACCESS(l) (0x | ((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) (0x)
+#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;
Use tab above, not spaces.
+
+ tpm_access = ioread8(chip-vendor.iobase + TPM_ACCESS(l));
+
+/* check if locality is closed */
+if(tpm_access == 0xFF)
if (
+return -1;
and use tab(s) for indent, not spaces.
+
+ 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;
Don't init statics to 0 or NULL (it's done for you).
(same comment as first time)
+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
@@ -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)
if (
and use tab(s) to indent, not spaces.
+return IRQ_NONE;
interrupt = ioread32(chip-vendor.iobase +
TPM_INT_STATUS(chip-vendor.locality));
@@ -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);
printk() usually should have a facility level, like
KERN_WARNING:
printk(KERN_WARNING tpm_tis: failed request_locality %d\n,
locality);
goto out_err;
}
@@ -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);
+ }
No braces on single-statement blocks.
+
if (force) {
+
Re: TPM driver changes to support multiple locality
On Thu, 11 Oct 2007 11:33:35 -0700
Agarwal, Lomesh [EMAIL PROTECTED] wrote:
Below is the patch for TPM driver.
Comments/suggestions?
please don't top post
--- pristine-linux-2.6.18/drivers/char/tpm/tpm_tis.c 2006-09-19
20:42:06.0 -0700
+++ linux-2.6.18-xen/drivers/char/tpm/tpm_tis.c 2007-10-09
15:30:49.0 -0700
or send word wrapped patches
.. or patches against 2 year old kernels for that matter ;)
enum tis_defaults {
TIS_MEM_BASE = 0xFED4,
- TIS_MEM_LEN = 0x5000,
+ TIS_MEM_LEN = 0x1000,
hmmm/// why
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: TPM driver changes to support multiple locality
-Original Message-
From: Arjan van de Ven [mailto:[EMAIL PROTECTED]
Sent: Thursday, October 11, 2007 2:41 PM
To: Agarwal, Lomesh
Cc: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org
Subject: Re: TPM driver changes to support multiple locality
On Thu, 11 Oct 2007 11:33:35 -0700
Agarwal, Lomesh [EMAIL PROTECTED] wrote:
Below is the patch for TPM driver.
Comments/suggestions?
please don't top post
--- pristine-linux-2.6.18/drivers/char/tpm/tpm_tis.c 2006-09-19
20:42:06.0 -0700
+++ linux-2.6.18-xen/drivers/char/tpm/tpm_tis.c 2007-10-09
15:30:49.0 -0700
or send word wrapped patches
.. or patches against 2 year old kernels for that matter ;)
enum tis_defaults {
TIS_MEM_BASE = 0xFED4,
- TIS_MEM_LEN = 0x5000,
+ TIS_MEM_LEN = 0x1000,
hmmm/// why
[Agarwal, Lomesh] 0x5000 is the length of CSRs for all the localities.
Each locality's CSR size is 0x1000. so 0x1000 is sufficient and will
catch error if driver tries to write to wrong locality.
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: TPM driver changes to support multiple locality
Attached is the patch which resolves all the comments. tpm_tis.c.patch Description: tpm_tis.c.patch
Re: TPM driver changes to support multiple locality
On Thu, 11 Oct 2007 15:46:03 -0700 Agarwal, Lomesh wrote:
Attached is the patch which resolves all the comments.
Inline patches are preferred so that reviewers can comment on them
more easily.
What mail client are you using?
The patch has trailing CRs on each line (DOS mode).
Just verifying: this TPM device has interrupts per locality?
+ /* check if interrupt is meant for this locality */
+ if (check_locality(chip, locality) 0)
+ return IRQ_NONE;
~
init_tis() still seems to have some problems.
static int __init init_tis(void)
{
+#define DEVNAME_SIZE 10
+
int rc;
+ if ((locality 0) || (locality 4))
+ return PTR_ERR(pdev);
pdev hasn't been set (so it's NULL).
+ pdev = platform_device_register_simple(devname, -1, NULL, 0);
+ if (IS_ERR(pdev))
return PTR_ERR(pdev);
Error path above needs to call driver_unregister().
---
~Randy
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: TPM driver changes to support multiple locality
-Original Message-
From: Randy Dunlap [mailto:[EMAIL PROTECTED]
Sent: Thursday, October 11, 2007 4:02 PM
To: Agarwal, Lomesh
Cc: [EMAIL PROTECTED]; linux-kernel@vger.kernel.org
Subject: Re: TPM driver changes to support multiple locality
On Thu, 11 Oct 2007 15:46:03 -0700 Agarwal, Lomesh wrote:
Attached is the patch which resolves all the comments.
Inline patches are preferred so that reviewers can comment on them
more easily.
What mail client are you using?
[Agarwal, Lomesh] I am using MS Outlook. Earlier you said inline patches
have problem because of mail client. That's why I sent it as attachment.
The patch has trailing CRs on each line (DOS mode).
Just verifying: this TPM device has interrupts per locality?
+ /* check if interrupt is meant for this locality */
+ if (check_locality(chip, locality) 0)
+ return IRQ_NONE;
~
[Agarwal, Lomesh] TPM device has only one interrupt. so on receiving
interrupt driver has to make sure that its meant for its locality.
init_tis() still seems to have some problems.
static int __init init_tis(void)
{
+#define DEVNAME_SIZE 10
+
int rc;
+ if ((locality 0) || (locality 4))
+ return PTR_ERR(pdev);
pdev hasn't been set (so it's NULL).
+ pdev = platform_device_register_simple(devname, -1,
NULL, 0);
+ if (IS_ERR(pdev))
return PTR_ERR(pdev);
Error path above needs to call driver_unregister().
[Agarwal, Lomesh] attached is the new patch.
---
~Randy
tpm_tis.c.patch
Description: tpm_tis.c.patch
RE: TPM driver changes to support multiple locality
There will be no change in API. Driver will accept a parameter "locality" and it will always operate on that locality. By default this parameter will be 0 which is what current driver assumes. Who is the maintainer for this driver? -Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] Sent: Wednesday, October 10, 2007 12:46 PM To: Agarwal, Lomesh Cc: linux-kernel@vger.kernel.org Subject: Re: TPM driver changes to support multiple locality On Tue, 09 Oct 2007 15:51:11 PDT, "Agarwal, Lomesh" said: > Current TPM driver supports only locality 0. I am planning to add > support so that it can access any locality. Locality parameter will be > passed as parameter. Will this change be acceptable? If yes then I will > modify the driver and send the patch. Make sure you extend the API in such a way that older userspace programs that don't pass the new locality parameter still work correctly. - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: TPM driver changes to support multiple locality
On Tue, 09 Oct 2007 15:51:11 PDT, "Agarwal, Lomesh" said: > Current TPM driver supports only locality 0. I am planning to add > support so that it can access any locality. Locality parameter will be > passed as parameter. Will this change be acceptable? If yes then I will > modify the driver and send the patch. Make sure you extend the API in such a way that older userspace programs that don't pass the new locality parameter still work correctly. pgpzLPFWSfif2.pgp Description: PGP signature
Re: TPM driver changes to support multiple locality
On Tue, 09 Oct 2007 15:51:11 PDT, Agarwal, Lomesh said: Current TPM driver supports only locality 0. I am planning to add support so that it can access any locality. Locality parameter will be passed as parameter. Will this change be acceptable? If yes then I will modify the driver and send the patch. Make sure you extend the API in such a way that older userspace programs that don't pass the new locality parameter still work correctly. pgpzLPFWSfif2.pgp Description: PGP signature
RE: TPM driver changes to support multiple locality
There will be no change in API. Driver will accept a parameter locality and it will always operate on that locality. By default this parameter will be 0 which is what current driver assumes. Who is the maintainer for this driver? -Original Message- From: [EMAIL PROTECTED] [mailto:[EMAIL PROTECTED] Sent: Wednesday, October 10, 2007 12:46 PM To: Agarwal, Lomesh Cc: linux-kernel@vger.kernel.org Subject: Re: TPM driver changes to support multiple locality On Tue, 09 Oct 2007 15:51:11 PDT, Agarwal, Lomesh said: Current TPM driver supports only locality 0. I am planning to add support so that it can access any locality. Locality parameter will be passed as parameter. Will this change be acceptable? If yes then I will modify the driver and send the patch. Make sure you extend the API in such a way that older userspace programs that don't pass the new locality parameter still work correctly. - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
TPM driver changes to support multiple locality
Current TPM driver supports only locality 0. I am planning to add support so that it can access any locality. Locality parameter will be passed as parameter. Will this change be acceptable? If yes then I will modify the driver and send the patch. Thanks, Lomesh - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
TPM driver changes to support multiple locality
Current TPM driver supports only locality 0. I am planning to add support so that it can access any locality. Locality parameter will be passed as parameter. Will this change be acceptable? If yes then I will modify the driver and send the patch. Thanks, Lomesh - To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
