Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1757755AbXJKVIa (ORCPT ); Thu, 11 Oct 2007 17:08:30 -0400 Received: (majordomo@vger.kernel.org) by vger.kernel.org id S1754438AbXJKVIX (ORCPT ); Thu, 11 Oct 2007 17:08:23 -0400 Received: from mga03.intel.com ([143.182.124.21]:43390 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753831AbXJKVIW convert rfc822-to-8bit (ORCPT ); Thu, 11 Oct 2007 17:08:22 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="4.21,261,1188802800"; d="scan'208";a="296940075" X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT Subject: RE: TPM driver changes to support multiple locality Date: Thu, 11 Oct 2007 14:08:17 -0700 Message-ID: In-Reply-To: <20071011115429.92326cbd.randy.dunlap@oracle.com> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: TPM driver changes to support multiple locality Thread-Index: AcgMOErs7theSRDFTumFLgeE5CCG2gAEc1iA References: <27383.1192045583@turing-police.cc.vt.edu> <20071011115429.92326cbd.randy.dunlap@oracle.com> From: "Agarwal, Lomesh" To: "Randy Dunlap" Cc: , X-OriginalArrivalTime: 11 Oct 2007 21:08:20.0042 (UTC) FILETIME=[D9C022A0:01C80C4A] Sender: linux-kernel-owner@vger.kernel.org X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 13681 Lines: 491 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@oracle.com] Sent: Thursday, October 11, 2007 11:54 AM To: Agarwal, Lomesh Cc: Valdis.Kletnieks@vt.edu; 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.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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/