lkml.org 
[lkml]   [2007]   [Oct]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRE: TPM driver changes to support multiple locality
    Date
    From
    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/
    \
     
     \ /
      Last update: 2007-10-11 23:11    [W:0.071 / U:0.088 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site