[PATCH] tpm: Stop taking over the non-unique lpc bus PCI ID, Also timer, stack and enum fixes

[Kylene Hall]

This patch is against the 2.6.12-rc2 kernel source.  It changes the tpm 
drivers from defining a pci driver structure to a format similar to the 
drivers/char/watchdog/i8xx_tco driver.  This is necessary because the 
lpc_bus only has one PCI ID and claiming that ID in the pci driver probe 
process prevents other drivers from finding their hardware.  This patch 
also fixes numerous problems that were pointed out with timer 
manipulations, large stack objects, lack of enums and defined constants.

Still lingering:

How can I receive Hotplug and ACPI events without being a PCI driver?

The first person to the lpc bus needs to call pci_enable_device and the 
last to leave one should call pci_disable_device, how as a device driver 
on this bus do I know if I am the first or last one and need to make the 
appropriate call?

Thanks,
Kylie Hall

Signed-off-by: Kylene Hall <[EMAIL PROTECTED]>
---
diff -uprN linux-2.6.12-rc2/drivers/char/tpm/tpm_atmel.c 
linux-2.6.12-rc2-tpmdd/drivers/char/tpm/tpm_atmel.c
--- linux-2.6.12-rc2/drivers/char/tpm/tpm_atmel.c   2005-04-15 
16:31:21.0 -0500
+++ linux-2.6.12-rc2-tpmdd/drivers/char/tpm/tpm_atmel.c 2005-04-15 
16:26:17.0 -0500
@@ -22,17 +22,22 @@
 #include "tpm.h"
 
 /* Atmel definitions */
-#defineTPM_ATML_BASE   0x400
+enum {
+   TPM_ATML_BASE = 0x400
+};
 
 /* write status bits */
-#defineATML_STATUS_ABORT   0x01
-#defineATML_STATUS_LASTBYTE0x04
-
+enum {
+   ATML_STATUS_ABORT = 0x01,
+   ATML_STATUS_LASTBYTE = 0x04
+};
 /* read status bits */
-#defineATML_STATUS_BUSY0x01
-#defineATML_STATUS_DATA_AVAIL  0x02
-#defineATML_STATUS_REWRITE 0x04
-
+enum {
+   ATML_STATUS_BUSY = 0x01,
+   ATML_STATUS_DATA_AVAIL = 0x02,
+   ATML_STATUS_REWRITE = 0x04,
+   ATML_STATUS_READY = 0x08
+};
 
 static int tpm_atml_recv(struct tpm_chip *chip, u8 * buf, size_t count)
 {
@@ -48,8 +53,7 @@ static int tpm_atml_recv(struct tpm_chip
for (i = 0; i < 6; i++) {
status = inb(chip->vendor->base + 1);
if ((status & ATML_STATUS_DATA_AVAIL) == 0) {
-   dev_err(&chip->pci_dev->dev,
-   "error reading header\n");
+   dev_err(chip->dev, "error reading header\n");
return -EIO;
}
*buf++ = inb(chip->vendor->base);
@@ -60,13 +64,12 @@ static int tpm_atml_recv(struct tpm_chip
size = be32_to_cpu(*native_size);
 
if (count < size) {
-   dev_err(&chip->pci_dev->dev,
+   dev_err(chip->dev,
"Recv size(%d) less than available space\n", size);
for (; i < size; i++) { /* clear the waiting data anyway */
status = inb(chip->vendor->base + 1);
if ((status & ATML_STATUS_DATA_AVAIL) == 0) {
-   dev_err(&chip->pci_dev->dev,
-   "error reading data\n");
+   dev_err(chip->dev, "error reading data\n");
return -EIO;
}
}
@@ -77,8 +80,7 @@ static int tpm_atml_recv(struct tpm_chip
for (; i < size; i++) {
status = inb(chip->vendor->base + 1);
if ((status & ATML_STATUS_DATA_AVAIL) == 0) {
-   dev_err(&chip->pci_dev->dev,
-   "error reading data\n");
+   dev_err(chip->dev, "error reading data\n");
return -EIO;
}
*buf++ = inb(chip->vendor->base);
@@ -87,7 +89,7 @@ static int tpm_atml_recv(struct tpm_chip
/* make sure data available is gone */
status = inb(chip->vendor->base + 1);
if (status & ATML_STATUS_DATA_AVAIL) {
-   dev_err(&chip->pci_dev->dev, "data available is stuck\n");
+   dev_err(chip->dev, "data available is stuck\n");
return -EIO;
}
 
@@ -98,9 +100,9 @@ static int tpm_atml_send(struct tpm_chip
 {
int i;
 
-   dev_dbg(&chip->pci_dev->dev, "tpm_atml_send: ");
+   dev_dbg(chip->dev, "tpm_atml_send: ");
for (i = 0; i < count; i++) {
-   dev_dbg(&chip->pci_dev->dev, "0x%x(%d) ", buf[i], buf[i]);
+   dev_dbg(chip->dev, "0x%x(%d) ", buf[i], buf[i]);
outb(buf[i], chip->vendor->base);
}
 
@@ -112,6 +114,27 @@ static void tpm_atml_cancel(struct tpm_c
outb(ATML_STATUS_ABORT, chip->vendor->base

Re: [PATCH] char/tpm: use msleep(), clean-up timers, fix typo

[Kylene Hall]

I have tested this patch and agree that using msleep is the right.  Please 
apply this patch to the tpm driver.  One hunk might fail b/c the 
typo has been fixed already.

Thanks,
Kylie Hall

On Fri, 11 Mar 2005, Nishanth Aravamudan wrote:

> Not sure what happened to the original mail, but I'm not seeing it
> yet...
> 
> On Wed, Mar 09, 2005 at 04:42:01PM -0800, Greg KH wrote:
> > ChangeSet 1.2035, 2005/03/09 10:12:19-08:00, [EMAIL PROTECTED]
> > 
> > [PATCH] Add TPM hardware enablement driver
> > 
> > This patch is a device driver to enable new hardware.  The new hardware is
> > the TPM chip as described by specifications at
> > .  The TPM chip will enable you to
> > use hardware to securely store and protect your keys and personal data.
> > To use the chip according to the specification, you will need the Trusted
> > Software Stack (TSS) of which an implementation for Linux is available at:
> > .
> 
> Here is a patch that removes all callers of schedule_timeout() as I
> previously mentioned:
> 
> Description: The TPM driver unnecessarily uses timers when it simply
> needs to maintain a maximum delay via time_before(). msleep() is used
> instead of schedule_timeout() to guarantee the task delays as expected.
> While compile-testing, I found a typo in the driver, using tpm_chp
> instead of tpm_chip. Remove the now unused timer callback function and
> change TPM_TIMEOUT's units to milliseconds. Patch is compile-tested.
> 
> Signed-off-by: Nishanth Aravamudan <[EMAIL PROTECTED]>
> 
> diff -urpN 2.6.11-v/drivers/char/tpm/tpm.c 2.6.11/drivers/char/tpm/tpm.c
> --- 2.6.11-v/drivers/char/tpm/tpm.c   2005-03-10 10:50:00.0 -0800
> +++ 2.6.11/drivers/char/tpm/tpm.c 2005-03-10 11:00:50.0 -0800
> @@ -19,7 +19,7 @@
>   * 
>   * Note, the TPM chip is not interrupt driven (only polling)
>   * and can have very long timeouts (minutes!). Hence the unusual
> - * calls to schedule_timeout.
> + * calls to msleep.
>   *
>   */
>  
> @@ -52,14 +52,6 @@ static void user_reader_timeout(unsigned
>   up(&chip->buffer_mutex);
>  }
>  
> -void tpm_time_expired(unsigned long ptr)
> -{
> - int *exp = (int *) ptr;
> - *exp = 1;
> -}
> -
> -EXPORT_SYMBOL_GPL(tpm_time_expired);
> -
>  /*
>   * Initialize the LPC bus and enable the TPM ports
>   */
> @@ -135,6 +127,7 @@ static ssize_t tpm_transmit(struct tpm_c
>   ssize_t len;
>   u32 count;
>   __be32 *native_size;
> + unsigned long stop;
>  
>   native_size = (__force __be32 *) (buf + 2);
>   count = be32_to_cpu(*native_size);
> @@ -155,28 +148,16 @@ static ssize_t tpm_transmit(struct tpm_c
>   return len;
>   }
>  
> - down(&chip->timer_manipulation_mutex);
> - chip->time_expired = 0;
> - init_timer(&chip->device_timer);
> - chip->device_timer.function = tpm_time_expired;
> - chip->device_timer.expires = jiffies + 2 * 60 * HZ;
> - chip->device_timer.data = (unsigned long) &chip->time_expired;
> - add_timer(&chip->device_timer);
> - up(&chip->timer_manipulation_mutex);
> -
> + stop = jiffies + 2 * 60 * HZ;
>   do {
>   u8 status = inb(chip->vendor->base + 1);
>   if ((status & chip->vendor->req_complete_mask) ==
>   chip->vendor->req_complete_val) {
> - down(&chip->timer_manipulation_mutex);
> - del_singleshot_timer_sync(&chip->device_timer);
> - up(&chip->timer_manipulation_mutex);
>   goto out_recv;
>   }
> - set_current_state(TASK_UNINTERRUPTIBLE);
> - schedule_timeout(TPM_TIMEOUT);
> + msleep(TPM_TIMEOUT); /* CHECK */
>   rmb();
> - } while (!chip->time_expired);
> + } while (time_before(jiffies, stop));
>  
>  
>   chip->vendor->cancel(chip);
> @@ -219,7 +200,7 @@ static ssize_t show_pcrs(struct device *
>   int i, j, index, num_pcrs;
>   char *str = buf;
>  
> - struct tpm_chp *chip =
> + struct tpm_chip *chip =
>   pci_get_drvdata(container_of(dev, struct pci_dev, dev));
>   if (chip == NULL)
>   return -ENODEV;
> @@ -450,10 +431,8 @@ ssize_t tpm_write(struct file * file, co
>  
>   /* cannot perform a write until the read has cleared
>  either via tpm_read or a user_read_timer timeout */
> - while (atomic_read(&chip->data_pending) != 0) {
> - set_current_state(TASK_UNINTERRUPTIBLE);
> - schedule_timeout(TPM_TIMEOUT);
> - }
> + while (atomic_read(&chip->data_pending) != 0)
> + msleep(TPM_TIMEOUT);
>  
>   down(&chip->buffer_mutex);
>  
> diff -urpN 2.6.11-v/drivers/char/tpm/tpm.h 2.6.11/drivers/char/tpm/tpm.h
> --- 2.6.11-v/drivers/char/tpm/tpm.h   2005-03-10 10:50:00.0 -0800
> +++ 2.6.11/drivers/char/tpm/tpm.h 2005-03-10 10:58:12.0 -0800
> @@ -24,7 +24,7 @@
>  #include 
>  #includ

Re: [PATCH] Add TPM hardware enablement driver

[Kylene Hall]

The patch at the bottom addresses a number of the concerns raised in this 
email along with a couple of other comments which this generated regarding 
not needing __force and the need for a MAINTAINERS entry.

Signed-off-by: Kylene Hall <[EMAIL PROTECTED]>

On Wed, 9 Mar 2005, Jeff Garzik wrote:

> Greg KH wrote:

> > +#defineTPM_MINOR   224 /* officially assigned
> > */
> > +
> > +#defineTPM_BUFSIZE 2048
> > +
> > +/* PCI configuration addresses */
> > +#definePCI_GEN_PMCON_1 0xA0
> > +#definePCI_GEN1_DEC0xE4
> > +#definePCI_LPC_EN  0xE6
> > +#definePCI_GEN2_DEC0xEC
> 
> enums preferred to #defines, as these provide more type information, and are
> more visible in a debugger.

fixed

> > +static LIST_HEAD(tpm_chip_list);
> > +static spinlock_t driver_lock = SPIN_LOCK_UNLOCKED;
> > +static int dev_mask[32];
> 
> don't use '32', create a constant and use that constant instead.
fixed

> > +   tpm_write_index(0x0D, 0x55);/* unlock 4F */
> > +   tpm_write_index(0x0A, 0x00);/* int disable */
> > +   tpm_write_index(0x08, base);/* base addr lo */
> > +   tpm_write_index(0x09, (base & 0xFF00) >> 8);/* base addr
> > hi */
> > +   tpm_write_index(0x0D, 0xAA);/* lock 4F */
> 
> please define symbol names for the TPM registers

fixed 


> > +   down(&chip->timer_manipulation_mutex);
> > +   chip->time_expired = 0;
> > +   init_timer(&chip->device_timer);
> > +   chip->device_timer.function = tpm_time_expired;
> > +   chip->device_timer.expires = jiffies + 2 * 60 * HZ;
> > +   chip->device_timer.data = (unsigned long) &chip->time_expired;
> > +   add_timer(&chip->device_timer);
> 
> very wrong.  you init_timer() when you initialize 'chip'... once.  then during
> the device lifetime you add/mod/del the timer.
> 
> calling init_timer() could lead to corruption of state.

fixed

> > +static u8 cap_pcr[] = {
> > +   0, 193, /* TPM_TAG_RQU_COMMAND */
> > +   0, 0, 0, 22,/* length */
> > +   0, 0, 0, 101,   /* TPM_ORD_GetCapability */
> > +   0, 0, 0, 5,
> > +   0, 0, 0, 4,
> > +   0, 0, 1, 1
> > +};
> 
> const

fixed


> > +static u8 pcrread[] = {
> > +   0, 193, /* TPM_TAG_RQU_COMMAND */
> > +   0, 0, 0, 14,/* length */
> > +   0, 0, 0, 21,/* TPM_ORD_PcrRead */
> > +   0, 0, 0, 0  /* PCR index */
> > +};
> 
> const

fixed

> > +
> > +   struct tpm_chp *chip =
> > +   pci_get_drvdata(container_of(dev, struct pci_dev, dev));
> 
> use to_pci_dev()

fixed

> > +#define  READ_PUBEK_RESULT_SIZE 314
> > +static u8 readpubek[] = {
> > +   0, 193, /* TPM_TAG_RQU_COMMAND */
> > +   0, 0, 0, 30,/* length */
> > +   0, 0, 0, 124,   /* TPM_ORD_ReadPubek */
> > +};
> > +
> > +static ssize_t show_pubek(struct device *dev, char *buf)
> > +{
> > +   u8 data[READ_PUBEK_RESULT_SIZE];
> 
> massive obj on stack

fixed

> > +   struct tpm_chip *chip =
> > +   pci_get_drvdata(container_of(dev, struct pci_dev, dev));
> 
> to_pci_dev()

fixed

> > +static u8 cap_version[] = {
> > +   0, 193, /* TPM_TAG_RQU_COMMAND */
> > +   0, 0, 0, 18,/* length */
> > +   0, 0, 0, 101,   /* TPM_ORD_GetCapability */
> > +   0, 0, 0, 6,
> > +   0, 0, 0, 0
> > +};
> 
> const

fixed

> > +static u8 cap_manufacturer[] = {
> > +   0, 193, /* TPM_TAG_RQU_COMMAND */
> > +   0, 0, 0, 22,/* length */
> > +   0, 0, 0, 101,   /* TPM_ORD_GetCapability */
> > +   0, 0, 0, 5,
> > +   0, 0, 0, 4,
> > +   0, 0, 1, 3
> > +};
> 
> const

fixed

> 
> > +static ssize_t show_caps(struct device *dev, char *buf)
> > +{
> > +   u8 data[READ_PUBEK_RESULT_SIZE];
> 
> massive obj on stack

fixed

> > +   struct tpm_chip *chip =
> > +   pci_get_drvdata(container_of(dev, struct pci_dev, dev));
> 
> to_pci_dev()

fixed

> > +   chip->data_buffer = kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
> > +   if (chip->data_buffer == NULL) {
> > +   chip->num_opens--;
> > +   pci_dev_put(chip->pci_dev);
> > +   return -ENOMEM;
> > +   }
> 
> what is the purpose of this pci_dev_get/put?  attempting to

Re: [tpmdd-devel] Re: [PATCH 1/1] tpm: update tpm sysfs file ownership - updated version

[Kylene Hall]

On Wed, 2005-02-09 at 16:04, Chris Wright wrote:
> > +#define TPM_DEVICE_ATTRS { \
> > +   __ATTR(pubek, S_IRUGO, show_pubek, NULL), \
> > +   __ATTR(pcrs, S_IRUGO, show_pcrs, NULL), \
> > +   __ATTR(caps, S_IRUGO, show_caps, NULL), \
> > +   __ATTR(cancel, S_IWUSR | S_IWGRP, NULL, store_cancel) }
> 
> This doesn't look like the right way to go.  
> 
> > +
> >  struct tpm_chip;
> >  
> >  struct tpm_vendor_specific {
> > @@ -42,6 +54,7 @@ struct tpm_vendor_specific {
> > void (*cancel) (struct tpm_chip *);
> > u8 (*status) (struct tpm_chip *);
> > struct miscdevice miscdev;
> > +   struct device_attribute attr[TPM_NUM_ATTR];
> 
> So every device will have the same attrs?  If so, make that whole struct
> exported (not the individual show/store methods) and reference that in
> each driver.
> 
They are all the same except they have a different owner.  What I was
trying to do was have them initialized in the tpm specific drivers so
that the attribute ownership would be correct but then create them in
the generic driver since that was common to all the specific drivers.  I
could have one copy of the attributes like the one in the mm tree now
and change the owner when ever I want to add or remove the attribute. 
Would that be correct?

Thanks,
Kylie

> thanks,
> -chris

-
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: [tpmdd-devel] Re: [PATCH 1/1] tpm: update tpm sysfs file ownership - updated version

[Kylene Hall]

On Wed, 9 Feb 2005, Greg KH wrote:
> On Wed, Feb 09, 2005 at 12:05:42PM -0600, Kylene Hall wrote:
> > @@ -539,9 +551,8 @@ void tpm_remove_hardware(struct device *
> > dev_set_drvdata(dev, NULL);
> > misc_deregister(&chip->vendor->miscdev);
> >  
> > -   device_remove_file(dev, &dev_attr_pubek);
> > -   device_remove_file(dev, &dev_attr_pcrs);
> > -   device_remove_file(dev, &dev_attr_caps);
> > +   for ( i = 0; i < TPM_ATTRS; i++ ) 
> > +   device_remove_file(dev, &chip->attr[i]);
> >  
> > dev_mask[chip->dev_num / 32] &= !(1 << (chip->dev_num % 32));
> >  
> 
> This code works?
> 
> > @@ -608,6 +619,11 @@ int tpm_register_hardware(struct device 
> > struct tpm_chip *chip;
> > int i, j;
> >  
> > +   DEVICE_ATTR(pcrs, S_IRUGO, show_pcrs, NULL);
> > +   DEVICE_ATTR(pubek, S_IRUGO, show_pubek, NULL);
> > +   DEVICE_ATTR(caps, S_IRUGO, show_caps, NULL);
> > +   DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, store_cancel);
> > +
> > /* Driver specific per-device data */
> > chip = kmalloc(sizeof(*chip), GFP_KERNEL);
> > if (chip == NULL)
> 
> You do realize you just created those attributes on the stack?  And then
> you try to remove them from within a different scope above?
>
I didn't realize their was a string in the structure that wouldn't get 
copied when I made the copy to the kmalloc'd struct.  Here is an alternate 
implementation that pushes the initialization into the respective specific 
drivers via a MACRO which has the added benefit of getting the owner field 
right from the start.
 
> thanks,
> 
> greg k-h

Signed-off-by: Kylene Hall <[EMAIL PROTECTED]>
---

diff -uprN linux-2.6.10/drivers/char/tpm/tpm_atmel.c 
linux-2.6.10-tpm/drivers/char/tpm/tpm_atmel.c
--- linux-2.6.10/drivers/char/tpm/tpm_atmel.c   2005-02-04 15:03:03.0 
-0600
+++ linux-2.6.10-tpm/drivers/char/tpm/tpm_atmel.c   2005-02-09 
14:12:30.711621784 -0600
@@ -131,6 +131,7 @@ static struct tpm_vendor_specific tpm_at
.req_complete_mask = ATML_STATUS_BUSY | ATML_STATUS_DATA_AVAIL,
.req_complete_val = ATML_STATUS_DATA_AVAIL,
.base = TPM_ATML_BASE,
+   .attr = TPM_DEVICE_ATTRS,
.miscdev.fops = &atmel_ops,
 };
 
diff -uprN linux-2.6.10/drivers/char/tpm/tpm.c 
linux-2.6.10-tpm/drivers/char/tpm/tpm.c
--- linux-2.6.10/drivers/char/tpm/tpm.c 2005-02-04 15:03:03.0 -0600
+++ linux-2.6.10-tpm/drivers/char/tpm/tpm.c 2005-02-09 14:12:30.695624216 
-0600
@@ -213,7 +213,7 @@ static u8 pcrread[] = {
0, 0, 0, 0  /* PCR index */
 };
 
-static ssize_t show_pcrs(struct device *dev, char *buf)
+ssize_t show_pcrs(struct device *dev, char *buf)
 {
u8 data[READ_PCR_RESULT_SIZE];
ssize_t len;
@@ -245,8 +245,7 @@ static ssize_t show_pcrs(struct device *
}
return str - buf;
 }
-
-static DEVICE_ATTR(pcrs, S_IRUGO, show_pcrs, NULL);
+EXPORT_SYMBOL_GPL(show_pcrs);
 
 #define  READ_PUBEK_RESULT_SIZE 314
 static u8 readpubek[] = {
@@ -255,7 +254,7 @@ static u8 readpubek[] = {
0, 0, 0, 124,   /* TPM_ORD_ReadPubek */
 };
 
-static ssize_t show_pubek(struct device *dev, char *buf)
+ssize_t show_pubek(struct device *dev, char *buf)
 {
u8 data[READ_PUBEK_RESULT_SIZE];
ssize_t len;
@@ -308,7 +307,7 @@ static ssize_t show_pubek(struct device 
return str - buf;
 }
 
-static DEVICE_ATTR(pubek, S_IRUGO, show_pubek, NULL);
+EXPORT_SYMBOL_GPL(show_pubek);
 
 #define CAP_VER_RESULT_SIZE 18
 static u8 cap_version[] = {
@@ -329,7 +328,7 @@ static u8 cap_manufacturer[] = {
0, 0, 1, 3
 };
 
-static ssize_t show_caps(struct device *dev, char *buf)
+ssize_t show_caps(struct device *dev, char *buf)
 {
u8 data[READ_PUBEK_RESULT_SIZE];
ssize_t len;
@@ -362,7 +361,26 @@ static ssize_t show_caps(struct device *
return str - buf;
 }
 
-static DEVICE_ATTR(caps, S_IRUGO, show_caps, NULL);
+EXPORT_SYMBOL_GPL(show_caps);
+
+ssize_t store_cancel(struct device *dev, const char *buf,
+   size_t count)
+{
+   struct tpm_chip *chip = dev_get_drvdata(dev);
+   if (chip == NULL)
+   return 0;
+
+   chip->vendor->cancel(chip);
+
+   down(&chip->timer_manipulation_mutex);
+   if (timer_pending(&chip->device_timer))
+   mod_timer(&chip->device_timer, jiffies);
+   up(&chip->timer_manipulation_mutex);
+
+   return count;
+}
+
+EXPORT_SYMBOL_GPL(store_cancel);
 
 /*
  * Device file system interface to the TPM
@@ -524,6 +542,7 @@ EXPORT_SYMBOL_GPL(tpm_read);
 void tpm_remove_hardware(struct device *dev)
 {
struct tpm_chip *chip = dev_get_drvdata(dev);
+   int i;
 
if (chip == NULL) {

Re: [PATCH 1/1] tpm: update tpm sysfs file ownership

[Kylene Hall]

On Fri, 4 Feb 2005, Greg KH wrote:
> On Fri, Feb 04, 2005 at 03:37:20PM -0600, Kylene Hall wrote:
> > On Fri, 2005-02-04 at 14:52, Greg KH wrote:
> > > On Fri, Feb 04, 2005 at 02:12:50PM -0600, Kylene Hall wrote:
> > > > +static struct class tpm_class = {
> > > > +   .name = "tpm",
> > > > +   .class_dev_attrs = tpm_attrs,
> > > > +};
> > > 
> > > Where is your release function?  Did you see any warnings from the
> > > kernel when you removed any of these class devices?  Why did you ignore
> > > it?
> > > 
> > Sorry, I missed the warning message.  I have looked at some other
> > instances for what I might need to put in that function and I'm
> > stumped.  I didn't kmalloc my class_device structure so I don't need to
> > kfree it.
> 
> Anyway, why not try using the class_simple interface instead?  If you do
> that you don't have to worry (as much) in the reference counting logic.

Thanks for pointing me to the class in the miscdevice.  I was able to use 
that for my needs.  I do need this small patch however to get the sysfs 
file ownership correct on my sysfs files.  The patch also adds on more 
sysfs file we need.

Thanks,
Kylie

> 
> thanks,
> 
> greg k-h
> 
> 


Signed off by: Kylene Hall <[EMAIL PROTECTED]>
---
diff -uprN linux-2.6.10/drivers/char/tpm/tpm.c 
linux-2.6.10-tpm/drivers/char/tpm/tpm.c
--- linux-2.6.10/drivers/char/tpm/tpm.c 2005-02-04 15:03:03.0 -0600
+++ linux-2.6.10-tpm/drivers/char/tpm/tpm.c 2005-02-09 12:23:23.137004424 
-0600
@@ -246,8 +246,6 @@ static ssize_t show_pcrs(struct device *
return str - buf;
 }
 
-static DEVICE_ATTR(pcrs, S_IRUGO, show_pcrs, NULL);
-
 #define  READ_PUBEK_RESULT_SIZE 314
 static u8 readpubek[] = {
0, 193, /* TPM_TAG_RQU_COMMAND */
@@ -308,8 +306,6 @@ static ssize_t show_pubek(struct device 
return str - buf;
 }
 
-static DEVICE_ATTR(pubek, S_IRUGO, show_pubek, NULL);
-
 #define CAP_VER_RESULT_SIZE 18
 static u8 cap_version[] = {
0, 193, /* TPM_TAG_RQU_COMMAND */
@@ -362,7 +358,22 @@ static ssize_t show_caps(struct device *
return str - buf;
 }
 
-static DEVICE_ATTR(caps, S_IRUGO, show_caps, NULL);
+static ssize_t store_cancel(struct device *dev, const char *buf,
+   size_t count)
+{
+   struct tpm_chip *chip = dev_get_drvdata(dev);
+   if (chip == NULL)
+   return 0;
+
+   chip->vendor->cancel(chip);
+
+   down(&chip->timer_manipulation_mutex);
+   if (timer_pending(&chip->device_timer))
+   mod_timer(&chip->device_timer, jiffies);
+   up(&chip->timer_manipulation_mutex);
+
+   return count;
+}
 
 /*
  * Device file system interface to the TPM
@@ -524,6 +535,7 @@ EXPORT_SYMBOL_GPL(tpm_read);
 void tpm_remove_hardware(struct device *dev)
 {
struct tpm_chip *chip = dev_get_drvdata(dev);
+   int i;
 
if (chip == NULL) {
dev_err(dev, "No device data found\n");
@@ -539,9 +551,8 @@ void tpm_remove_hardware(struct device *
dev_set_drvdata(dev, NULL);
misc_deregister(&chip->vendor->miscdev);
 
-   device_remove_file(dev, &dev_attr_pubek);
-   device_remove_file(dev, &dev_attr_pcrs);
-   device_remove_file(dev, &dev_attr_caps);
+   for ( i = 0; i < TPM_ATTRS; i++ ) 
+   device_remove_file(dev, &chip->attr[i]);
 
dev_mask[chip->dev_num / 32] &= !(1 << (chip->dev_num % 32));
 
@@ -608,6 +619,11 @@ int tpm_register_hardware(struct device 
struct tpm_chip *chip;
int i, j;
 
+   DEVICE_ATTR(pcrs, S_IRUGO, show_pcrs, NULL);
+   DEVICE_ATTR(pubek, S_IRUGO, show_pubek, NULL);
+   DEVICE_ATTR(caps, S_IRUGO, show_caps, NULL);
+   DEVICE_ATTR(cancel, S_IWUSR | S_IWGRP, NULL, store_cancel);
+
/* Driver specific per-device data */
chip = kmalloc(sizeof(*chip), GFP_KERNEL);
if (chip == NULL)
@@ -663,10 +679,16 @@ dev_num_search_complete:
 
list_add(&chip->list, &tpm_chip_list);
 
-   device_create_file(dev, &dev_attr_pubek);
-   device_create_file(dev, &dev_attr_pcrs);
-   device_create_file(dev, &dev_attr_caps);
-
+   chip->attr[0] = dev_attr_pubek;
+   chip->attr[1] = dev_attr_pcrs;
+   chip->attr[2] = dev_attr_caps;
+   chip->attr[3] = dev_attr_cancel;
+
+   for ( i = 0; i < TPM_ATTRS; i++ ) {
+   chip->attr[i].attr.owner = chip->vendor->miscdev.fops->owner;
+   device_create_file(dev, &chip->attr[i]);
+   }
+   
return 0;
 }
 
diff -uprN linux-2.6.10/drivers/char/tpm/tpm.h 
linux-2.6.10-tpm/drivers/char/tpm/tpm.h
--- lin

Re: [PATCH 1/1] tpm: implement use of sysfs classes

[Kylene Hall]

On Fri, 2005-02-04 at 14:52, Greg KH wrote:
> On Fri, Feb 04, 2005 at 02:12:50PM -0600, Kylene Hall wrote:
> > +static struct class tpm_class = {
> > +   .name = "tpm",
> > +   .class_dev_attrs = tpm_attrs,
> > +};
> 
> Where is your release function?  Did you see any warnings from the
> kernel when you removed any of these class devices?  Why did you ignore
> it?
> 
Sorry, I missed the warning message.  I have looked at some other
instances for what I might need to put in that function and I'm
stumped.  I didn't kmalloc my class_device structure so I don't need to
kfree it.  I am using this mechanism so that my sysfs stuff is in a
predictable place.  It is also very convient how the driver and device
links as well as all my class specific files get created for me in the
register and likewise removed in the unregister.  I call the register
from the pci probe path and the unregister from the pci remove path. 
What might I need to put in this function?

Thanks,
Kylie

> thanks,
> 
> greg k-h
> 

-
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/

[PATCH 1/1] tpm: implement use of sysfs classes

[Kylene Hall]

With the enablement of being able to support chips on differnt buses  
(see patch sent 2/3/2005), the userspace library (TSS) needs to be able to 
find the corresponding sysfs files.  Added a new sysfs class "tpm" where 
the devices are now registered (tpm0, tpm1, ...) and links to the 
driver, device and tpm specific files (pcrs, etc) now live.  This patch 
also includes a new sysfs file for each tpm that is needed to support 
canceling an operation.

Thanks,
Kylie
 
Signed-off-by: Kylene Hall <[EMAIL PROTECTED]>
---
diff -uprN linux-2.6.10/drivers/char/tpm/tpm.c 
linux-2.6.10-tpm/drivers/char/tpm/tpm.c
--- linux-2.6.10/drivers/char/tpm/tpm.c 2005-02-04 15:03:03.0 -0600
+++ linux-2.6.10-tpm/drivers/char/tpm/tpm.c 2005-02-04 15:02:14.0 
-0600
@@ -213,14 +212,14 @@ static u8 pcrread[] = {
0, 0, 0, 0  /* PCR index */
 };
 
-static ssize_t show_pcrs(struct device *dev, char *buf)
+static ssize_t show_pcrs(struct class_device *cdev, char *buf)
 {
u8 data[READ_PCR_RESULT_SIZE];
ssize_t len;
int i, j, index, num_pcrs;
char *str = buf;
 
-   struct tpm_chip *chip = dev_get_drvdata(dev);
+   struct tpm_chip *chip = dev_get_drvdata(cdev->dev);
if (chip == NULL)
return -ENODEV;
 
@@ -246,8 +245,6 @@ static ssize_t show_pcrs(struct device *
return str - buf;
 }
 
-static DEVICE_ATTR(pcrs, S_IRUGO, show_pcrs, NULL);
-
 #define  READ_PUBEK_RESULT_SIZE 314
 static u8 readpubek[] = {
0, 193, /* TPM_TAG_RQU_COMMAND */
@@ -255,7 +252,7 @@ static u8 readpubek[] = {
0, 0, 0, 124,   /* TPM_ORD_ReadPubek */
 };
 
-static ssize_t show_pubek(struct device *dev, char *buf)
+static ssize_t show_pubek(struct class_device *cdev, char *buf)
 {
u8 data[READ_PUBEK_RESULT_SIZE];
ssize_t len;
@@ -263,7 +260,7 @@ static ssize_t show_pubek(struct device 
int i;
char *str = buf;
 
-   struct tpm_chip *chip = dev_get_drvdata(dev);
+   struct tpm_chip *chip = dev_get_drvdata(cdev->dev);
if (chip == NULL)
return -ENODEV;
 
@@ -308,8 +305,6 @@ static ssize_t show_pubek(struct device 
return str - buf;
 }
 
-static DEVICE_ATTR(pubek, S_IRUGO, show_pubek, NULL);
-
 #define CAP_VER_RESULT_SIZE 18
 static u8 cap_version[] = {
0, 193, /* TPM_TAG_RQU_COMMAND */
@@ -329,13 +324,13 @@ static u8 cap_manufacturer[] = {
0, 0, 1, 3
 };
 
-static ssize_t show_caps(struct device *dev, char *buf)
+static ssize_t show_caps(struct class_device *cdev, char *buf)
 {
u8 data[READ_PUBEK_RESULT_SIZE];
ssize_t len;
char *str = buf;
 
-   struct tpm_chip *chip = dev_get_drvdata(dev);
+   struct tpm_chip *chip = dev_get_drvdata(cdev->dev);
if (chip == NULL)
return -ENODEV;
 
@@ -362,7 +357,22 @@ static ssize_t show_caps(struct device *
return str - buf;
 }
 
-static DEVICE_ATTR(caps, S_IRUGO, show_caps, NULL);
+static ssize_t store_cancel(struct class_device *cdev, const char *buf,
+   size_t count)
+{
+   struct tpm_chip *chip = dev_get_drvdata(cdev->dev);
+   if (chip == NULL)
+   return 0;
+
+   chip->vendor->cancel(chip);
+
+   down(&chip->timer_manipulation_mutex);
+   if (timer_pending(&chip->device_timer))
+   mod_timer(&chip->device_timer, jiffies);
+   up(&chip->timer_manipulation_mutex);
+
+   return count;
+}
 
 /*
  * Device file system interface to the TPM
@@ -539,9 +549,7 @@ void tpm_remove_hardware(struct device *
dev_set_drvdata(dev, NULL);
misc_deregister(&chip->vendor->miscdev);
 
-   device_remove_file(dev, &dev_attr_pubek);
-   device_remove_file(dev, &dev_attr_pcrs);
-   device_remove_file(dev, &dev_attr_caps);
+   class_device_unregister(&chip->cdev);
 
dev_mask[chip->dev_num / 32] &= !(1 << (chip->dev_num % 32));
 
@@ -594,6 +602,19 @@ int tpm_pm_resume(struct pci_dev *pci_de
 
 EXPORT_SYMBOL_GPL(tpm_pm_resume);
 
+static struct class_device_attribute tpm_attrs[] = {
+   __ATTR(pubek, S_IRUGO, show_pubek, NULL),
+   __ATTR(pcrs, S_IRUGO, show_pcrs, NULL),
+   __ATTR(caps, S_IRUGO, show_caps, NULL),
+   __ATTR(cancel, S_IWUSR | S_IWGRP, NULL, store_cancel),
+   __ATTR_NULL
+};
+
+static struct class tpm_class = {
+   .name = "tpm",
+   .class_dev_attrs = tpm_attrs,
+};
+
 /*
  * Called from tpm_.c probe function only for devices 
  * the driver has determined it should claim.  Prior to calling
@@ -663,9 +684,10 @@ dev_num_search_complete:
 
list_add(&chip->list, &tpm_chip_list);
 
-   device_create_file(dev, &dev_attr_pubek);
-   device_create_file(dev, &dev_attr_pcrs);
-   device_create_file(dev, &dev_attr_c

[PATCH 1/1] tpm: remove pci specific stuff from the underlying generic driver

[Kylene Hall]

Since future versions of this chip might not be pci devices and the 
generic tpm driver does not need access to the pci related fields, 
I updated the structures and functions to use struct device and related 
functions 
rather than the pci equivalents.  This simplifies many things including 
dev_dbg and dev_err calls which were pulling the dev structure out of the 
pci_dev everytime.  Also pci calls to get and set driver data were merely 
pulling this structure out and passing it on as well.  Now only the vendor 
specific drivers (atmel and national at this time) will know about the pci 
device stuff and future non-pci chips can easily be connected to the 
architecture.

Thanks,
Kylie

Signed-off-by: Kylene Hall <[EMAIL PROTECTED]>
---
diff -uprN linux-2.6.10/drivers/char/tpm/tpm_atmel.c 
linux-2.6.10-tpm/drivers/char/tpm/tpm_atmel.c
--- linux-2.6.10/drivers/char/tpm/tpm_atmel.c   2005-02-01 11:08:44.0 
-0600
+++ linux-2.6.10-tpm/drivers/char/tpm/tpm_atmel.c   2005-02-01 
11:27:58.0 -0600
@@ -48,8 +48,7 @@ static int tpm_atml_recv(struct tpm_chip
for (i = 0; i < 6; i++) {
status = inb(chip->vendor->base + 1);
if ((status & ATML_STATUS_DATA_AVAIL) == 0) {
-   dev_err(&chip->pci_dev->dev,
-   "error reading header\n");
+   dev_err(chip->dev, "error reading header\n");
return -EIO;
}
*buf++ = inb(chip->vendor->base);
@@ -60,13 +59,12 @@ static int tpm_atml_recv(struct tpm_chip
size = be32_to_cpu(*native_size);
 
if (count < size) {
-   dev_err(&chip->pci_dev->dev,
+   dev_err(chip->dev,
"Recv size(%d) less than available space\n", size);
for (; i < size; i++) { /* clear the waiting data anyway */
status = inb(chip->vendor->base + 1);
if ((status & ATML_STATUS_DATA_AVAIL) == 0) {
-   dev_err(&chip->pci_dev->dev,
-   "error reading data\n");
+   dev_err(chip->dev, "error reading data\n");
return -EIO;
}
}
@@ -77,8 +75,7 @@ static int tpm_atml_recv(struct tpm_chip
for (; i < size; i++) {
status = inb(chip->vendor->base + 1);
if ((status & ATML_STATUS_DATA_AVAIL) == 0) {
-   dev_err(&chip->pci_dev->dev,
-   "error reading data\n");
+   dev_err(chip->dev, "error reading data\n");
return -EIO;
}
*buf++ = inb(chip->vendor->base);
@@ -87,7 +84,7 @@ static int tpm_atml_recv(struct tpm_chip
/* make sure data available is gone */
status = inb(chip->vendor->base + 1);
if (status & ATML_STATUS_DATA_AVAIL) {
-   dev_err(&chip->pci_dev->dev, "data available is stuck\n");
+   dev_err(chip->dev, "data available is stuck\n");
return -EIO;
}
 
@@ -98,9 +95,9 @@ static int tpm_atml_send(struct tpm_chip
 {
int i;
 
-   dev_dbg(&chip->pci_dev->dev, "tpm_atml_send: ");
+   dev_dbg(chip->dev, "tpm_atml_send: ");
for (i = 0; i < count; i++) {
-   dev_dbg(&chip->pci_dev->dev, "0x%x(%d) ", buf[i], buf[i]);
+   dev_dbg(chip->dev, "0x%x(%d) ", buf[i], buf[i]);
outb(buf[i], chip->vendor->base);
}
 
@@ -114,7 +111,7 @@ static void tpm_atml_cancel(struct tpm_c
 
 static u8 tpm_atml_status(struct tpm_chip *chip)
 {
-   return inb( chip->vendor->base + 1);
+   return inb(chip->vendor->base + 1);
 }
 
 static struct file_operations atmel_ops = {
@@ -169,7 +166,7 @@ static int __devinit tpm_atml_init(struc
goto out_err;
}
 
-   if ((rc = tpm_register_hardware(pci_dev, &tpm_atmel)) < 0)
+   if ((rc = tpm_register_hardware(&pci_dev->dev, &tpm_atmel)) < 0)
goto out_err;
 
dev_info(&pci_dev->dev,
@@ -182,6 +179,12 @@ out_err:
return rc;
 }
 
+static void __devexit tpm_atml_remove(struct pci_dev *pci_dev)
+{
+   tpm_remove_hardware(&pci_dev->dev);
+   pci_disable_device(pci_dev);
+}
+
 static struct pci_device_id tpm_pci_tbl[] __devinitdata = {
{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801BA_0)},
{PCI_DEVICE(PCI_VENDOR_ID_INTEL, PCI_DEVICE_ID_INTEL_82801CA_12)},
@@ -198,7 +201,7 @@ static struct pci_driver atmel_pci_drive
   

Re: [PATCH 1/1] tpm: insert missing up mutex in an error path, typo build fix -- updated version

[Kylene Hall]

There is also a typo in the driver version in the 2.6.11-rc2-mm2 tree that 
is not fixed by the previous patch.  Please apply this upated version to 
fix the previously acknowledged problems and this typo causing a build 
error.

Thanks,
Kylie

On Fri, 28 Jan 2005, Kylene Hall wrote:

> This patch puts in the missing up call on the tpm_mutex on an 
> error condition in the tpm_transmit function.  Bug reported by Stefan 
> Berger <[EMAIL PROTECTED]>.  This patch also implements a new status 
> function to handle future chip configurations which may generate status 
> differntly. 
> 
> Thanks,
> Kylie
>   

Signed-off-by: Kylene Hall <[EMAIL PROTECTED]>
---
diff -uprN linux-2.6.10/drivers/char/tpm/tpm_atmel.c 
linux-2.6.10-tpm/drivers/char/tpm/tpm_atmel.c
--- linux-2.6.10/drivers/char/tpm/tpm_atmel.c   2005-01-18 16:42:17.0 
-0600
+++ linux-2.6.10-tpm/drivers/char/tpm/tpm_atmel.c   2005-01-21 
13:11:11.0 -0600
@@ -112,6 +112,11 @@ static void tpm_atml_cancel(struct tpm_c
outb(ATML_STATUS_ABORT, chip->vendor->base + 1);
 }
 
+static u8 tpm_atml_status(struct tpm_chip *chip)
+{
+   return inb( chip->vendor->base + 1);
+}
+
 static struct file_operations atmel_ops = {
.owner = THIS_MODULE,
.llseek = no_llseek,
@@ -125,6 +130,7 @@ static struct tpm_vendor_specific tpm_at
.recv = tpm_atml_recv,
.send = tpm_atml_send,
.cancel = tpm_atml_cancel,
+   .status = tpm_atml_status,
.req_complete_mask = ATML_STATUS_BUSY | ATML_STATUS_DATA_AVAIL,
.req_complete_val = ATML_STATUS_DATA_AVAIL,
.base = TPM_ATML_BASE,
diff -uprN linux-2.6.10/drivers/char/tpm/tpm.c 
linux-2.6.10-tpm/drivers/char/tpm/tpm.c
--- linux-2.6.10/drivers/char/tpm/tpm.c 2005-01-31 13:59:38.0 -0600
+++ linux-2.6.10-tpm/drivers/char/tpm/tpm.c 2005-01-28 16:28:45.0 
-0600
@@ -152,6 +151,7 @@ static ssize_t tpm_transmit(struct tpm_c
if ((len = chip->vendor->send(chip, (u8 *) buf, count)) < 0) {
dev_err(&chip->pci_dev->dev,
"tpm_transmit: tpm_send: error %d\n", len);
+   up(&chip->tpm_mutex);
return len;
}
 
@@ -165,7 +165,7 @@ static ssize_t tpm_transmit(struct tpm_c
up(&chip->timer_manipulation_mutex);
 
do {
-   u8 status = inb(chip->vendor->base + 1);
+   u8 status = chip->vendor->status(chip);
if ((status & chip->vendor->req_complete_mask) ==
chip->vendor->req_complete_val) {
down(&chip->timer_manipulation_mutex);
@@ -219,7 +219,7 @@ static ssize_t show_pcrs(struct device *
int i, j, index, num_pcrs;
char *str = buf;
 
-   struct tpm_chip *chp =
+   struct tpm_chip *chip =
pci_get_drvdata(container_of(dev, struct pci_dev, dev));
if (chip == NULL)
return -ENODEV;
diff -uprN linux-2.6.10/drivers/char/tpm/tpm.h 
linux-2.6.10-tpm/drivers/char/tpm/tpm.h
--- linux-2.6.10/drivers/char/tpm/tpm.h 2005-01-18 16:42:17.0 -0600
+++ linux-2.6.10-tpm/drivers/char/tpm/tpm.h 2005-01-21 13:10:20.0 
-0600
@@ -40,6 +40,7 @@ struct tpm_vendor_specific {
int (*recv) (struct tpm_chip *, u8 *, size_t);
int (*send) (struct tpm_chip *, u8 *, size_t);
void (*cancel) (struct tpm_chip *);
+   u8 (*status) (struct tpm_chip *);
struct miscdevice miscdev;
 };
 
diff -uprN linux-2.6.10/drivers/char/tpm/tpm_nsc.c 
linux-2.6.10-tpm/drivers/char/tpm/tpm_nsc.c
--- linux-2.6.10/drivers/char/tpm/tpm_nsc.c 2005-01-18 16:42:17.0 
-0600
+++ linux-2.6.10-tpm/drivers/char/tpm/tpm_nsc.c 2005-01-21 13:12:27.0 
-0600
@@ -219,6 +219,12 @@ static void tpm_nsc_cancel(struct tpm_ch
outb(NSC_COMMAND_CANCEL, chip->vendor->base + NSC_COMMAND);
 }
 
+
+static u8 tpm_nsc_status(struct tpm_chip *chip)
+{
+   return inb(chip->vendor->base + NSC_STATUS);
+}
+
 static struct file_operations nsc_ops = {
.owner = THIS_MODULE,
.llseek = no_llseek,
@@ -232,6 +238,7 @@ static struct tpm_vendor_specific tpm_ns
.recv = tpm_nsc_recv,
.send = tpm_nsc_send,
.cancel = tpm_nsc_cancel,
+   .status = tpm_nsc_status,
.req_complete_mask = NSC_STATUS_OBF,
.req_complete_val = NSC_STATUS_OBF,
.base = TPM_NSC_BASE,
-
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/

[PATCH 1/1] tpm: insert missing up mutex in an error path

[Kylene Hall]

This patch puts in the missing up call on the tpm_mutex on an 
error condition in the tpm_transmit function.  Bug reported by Stefan 
Berger <[EMAIL PROTECTED]>.  This patch also implements a new status 
function to handle future chip configurations which may generate status 
differntly. 

Thanks,
Kylie
  
Signed-off-by: Kylene Hall <[EMAIL PROTECTED]>
---
diff -uprN linux-2.6.10/drivers/char/tpm/tpm_atmel.c 
linux-2.6.10-tpm/drivers/char/tpm/tpm_atmel.c
--- linux-2.6.10/drivers/char/tpm/tpm_atmel.c   2005-01-18 16:42:17.0 
-0600
+++ linux-2.6.10-tpm/drivers/char/tpm/tpm_atmel.c   2005-01-21 
13:11:11.0 -0600
@@ -112,6 +112,11 @@ static void tpm_atml_cancel(struct tpm_c
outb(ATML_STATUS_ABORT, chip->vendor->base + 1);
 }
 
+static u8 tpm_atml_status(struct tpm_chip *chip)
+{
+   return inb( chip->vendor->base + 1);
+}
+
 static struct file_operations atmel_ops = {
.owner = THIS_MODULE,
.llseek = no_llseek,
@@ -125,6 +130,7 @@ static struct tpm_vendor_specific tpm_at
.recv = tpm_atml_recv,
.send = tpm_atml_send,
.cancel = tpm_atml_cancel,
+   .status = tpm_atml_status,
.req_complete_mask = ATML_STATUS_BUSY | ATML_STATUS_DATA_AVAIL,
.req_complete_val = ATML_STATUS_DATA_AVAIL,
.base = TPM_ATML_BASE,
diff -uprN linux-2.6.10/drivers/char/tpm/tpm.c 
linux-2.6.10-tpm/drivers/char/tpm/tpm.c
--- linux-2.6.10/drivers/char/tpm/tpm.c 2005-01-21 12:53:26.0 -0600
+++ linux-2.6.10-tpm/drivers/char/tpm/tpm.c 2005-01-28 16:28:45.578493680 
-0600
@@ -152,6 +151,7 @@ static ssize_t tpm_transmit(struct tpm_c
if ((len = chip->vendor->send(chip, (u8 *) buf, count)) < 0) {
dev_err(&chip->pci_dev->dev,
"tpm_transmit: tpm_send: error %d\n", len);
+   up(&chip->tpm_mutex);
return len;
}
 
@@ -165,7 +165,7 @@ static ssize_t tpm_transmit(struct tpm_c
up(&chip->timer_manipulation_mutex);
 
do {
-   u8 status = inb(chip->vendor->base + 1);
+   u8 status = chip->vendor->status(chip);
if ((status & chip->vendor->req_complete_mask) ==
chip->vendor->req_complete_val) {
down(&chip->timer_manipulation_mutex);
diff -uprN linux-2.6.10/drivers/char/tpm/tpm.h 
linux-2.6.10-tpm/drivers/char/tpm/tpm.h
--- linux-2.6.10/drivers/char/tpm/tpm.h 2005-01-18 16:42:17.0 -0600
+++ linux-2.6.10-tpm/drivers/char/tpm/tpm.h 2005-01-21 13:10:20.0 
-0600
@@ -40,6 +40,7 @@ struct tpm_vendor_specific {
int (*recv) (struct tpm_chip *, u8 *, size_t);
int (*send) (struct tpm_chip *, u8 *, size_t);
void (*cancel) (struct tpm_chip *);
+   u8 (*status) (struct tpm_chip *);
struct miscdevice miscdev;
 };
 
diff -uprN linux-2.6.10/drivers/char/tpm/tpm_nsc.c 
linux-2.6.10-tpm/drivers/char/tpm/tpm_nsc.c
--- linux-2.6.10/drivers/char/tpm/tpm_nsc.c 2005-01-18 16:42:17.0 
-0600
+++ linux-2.6.10-tpm/drivers/char/tpm/tpm_nsc.c 2005-01-21 13:12:27.0 
-0600
@@ -219,6 +219,12 @@ static void tpm_nsc_cancel(struct tpm_ch
outb(NSC_COMMAND_CANCEL, chip->vendor->base + NSC_COMMAND);
 }
 
+
+static u8 tpm_nsc_status(struct tpm_chip *chip)
+{
+   return inb(chip->vendor->base + NSC_STATUS);
+}
+
 static struct file_operations nsc_ops = {
.owner = THIS_MODULE,
.llseek = no_llseek,
@@ -232,6 +238,7 @@ static struct tpm_vendor_specific tpm_ns
.recv = tpm_nsc_recv,
.send = tpm_nsc_send,
.cancel = tpm_nsc_cancel,
+   .status = tpm_nsc_status,
.req_complete_mask = NSC_STATUS_OBF,
.req_complete_val = NSC_STATUS_OBF,
.base = TPM_NSC_BASE,
-
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: [PATCH 1/1] tpm: fix cause of SMP stack traces -- updated version

[Kylene Hall]

There were misplaced spinlock acquires and releases in the probe, close and 
release 
paths which were causing might_sleep and schedule while atomic error messages 
accompanied 
by stack traces when the kernel was compiled with SMP support. Bug reported by 
Reben Jenster 
<[EMAIL PROTECTED]>

Thanks,
Kylie
 
Signed-off-by: Kylene Hall <[EMAIL PROTECTED]>
---
diff -uprN linux-2.6.10/drivers/char/tpm/tpm.c 
linux-2.6.10-tpm/drivers/char/tpm/tpm.c
--- linux-2.6.10/drivers/char/tpm/tpm.c 2005-01-18 18:10:16.0 -0600
+++ linux-2.6.10-tpm/drivers/char/tpm/tpm.c 2005-01-18 18:13:59.0 
-0600
@@ -422,21 +421,24 @@ EXPORT_SYMBOL_GPL(tpm_open);
 int tpm_release(struct inode *inode, struct file *file)
 {
struct tpm_chip *chip = file->private_data;
+   
+   file->private_data = NULL;
 
spin_lock(&driver_lock);
chip->num_opens--;
+   spin_unlock(&driver_lock);
+
down(&chip->timer_manipulation_mutex);
if (timer_pending(&chip->user_read_timer))
del_singleshot_timer_sync(&chip->user_read_timer);
else if (timer_pending(&chip->device_timer))
del_singleshot_timer_sync(&chip->device_timer);
up(&chip->timer_manipulation_mutex);
+
kfree(chip->data_buffer);
atomic_set(&chip->data_pending, 0);
 
pci_dev_put(chip->pci_dev);
-   file->private_data = NULL;
-   spin_unlock(&driver_lock);
return 0;
 }
 
@@ -534,6 +536,8 @@ void __devexit tpm_remove(struct pci_dev
 
list_del(&chip->list);
 
+   spin_unlock(&driver_lock);
+
pci_set_drvdata(pci_dev, NULL);
misc_deregister(&chip->vendor->miscdev);
 
@@ -541,8 +545,6 @@ void __devexit tpm_remove(struct pci_dev
device_remove_file(&pci_dev->dev, &dev_attr_pcrs);
device_remove_file(&pci_dev->dev, &dev_attr_caps);
 
-   spin_unlock(&driver_lock);
-
pci_disable_device(pci_dev);
 
dev_mask[chip->dev_num / 32] &= !(1 << (chip->dev_num % 32));
@@ -583,6 +585,7 @@ EXPORT_SYMBOL_GPL(tpm_pm_suspend);
 int tpm_pm_resume(struct pci_dev *pci_dev)
 {
struct tpm_chip *chip = pci_get_drvdata(pci_dev);
+
if (chip == NULL)
return -ENODEV;
 
@@ -650,15 +653,12 @@ dev_num_search_complete:
chip->vendor->miscdev.dev = &(pci_dev->dev);
chip->pci_dev = pci_dev_get(pci_dev);
 
-   spin_lock(&driver_lock);
-
if (misc_register(&chip->vendor->miscdev)) {
dev_err(&chip->pci_dev->dev,
"unable to misc_register %s, minor %d\n",
chip->vendor->miscdev.name,
chip->vendor->miscdev.minor);
pci_dev_put(pci_dev);
-   spin_unlock(&driver_lock);
kfree(chip);
dev_mask[i] &= !(1 << j);
return -ENODEV;
@@ -672,7 +672,6 @@ dev_num_search_complete:
device_create_file(&pci_dev->dev, &dev_attr_pcrs);
device_create_file(&pci_dev->dev, &dev_attr_caps);
 
-   spin_unlock(&driver_lock);
return 0;
 }
 
-
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: [PATCH 1/1] tpm: fix cause of SMP stack traces

[Kylene Hall]

On Tue, 2005-01-18 at 16:47, Greg KH wrote:
> On Tue, Jan 18, 2005 at 04:29:23PM -0600, Kylene Hall wrote:
> > There were misplaced spinlock acquires and releases in the probe, open, 
> > close and release paths which were causing might_sleep and schedule while 
> > atomic error messages accompanied by stack traces when the kernel was 
> > compiled with SMP support. Bug reported by Reben Jenster 
> > <[EMAIL PROTECTED]>
> 
> Where exactly where the trace errors coming from?  I don't see anything
> in the open path that might have caused it.
> 
True the open path was not affected.
> Anyway, Chris is right, just changing this to _irqsave will not fix the
> issue.
Fixing will reissue the patch momentarily.
> 
> thanks,
> 
> greg k-h
> 

-
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: [PATCH 1/1] tpm: fix cause of SMP stack traces

[Kylene Hall]

On Tue, 2005-01-18 at 16:37, Chris Wright wrote:
> * Kylene Hall ([EMAIL PROTECTED]) wrote:
> > There were misplaced spinlock acquires and releases in the probe, open, 
> > close and release paths which were causing might_sleep and schedule while 
> > atomic error messages accompanied by stack traces when the kernel was 
> > compiled with SMP support. Bug reported by Reben Jenster 
> > <[EMAIL PROTECTED]>
> > 
> > Signed-off-by: Kylene Hall <[EMAIL PROTECTED]>
> > ---
> > diff -uprN linux-2.6.10/drivers/char/tpm/tpm.c 
> > linux-2.6.10-tpm/drivers/char/tpm/tpm.c
> > --- linux-2.6.10/drivers/char/tpm/tpm.c 2005-01-18 16:42:17.0 
> > -0600
> > +++ linux-2.6.10-tpm/drivers/char/tpm/tpm.c 2005-01-18 12:52:53.0 
> > -0600
> > @@ -373,8 +372,9 @@ int tpm_open(struct inode *inode, struct
> >  {
> > int rc = 0, minor = iminor(inode);
> > struct tpm_chip *chip = NULL, *pos;
> > +   unsigned long flags;
> >  
> > -   spin_lock(&driver_lock);
> > +   spin_lock_irqsave(&driver_lock, flags);
> 
> Hmm, unless I'm missing something, this is only worse (for might sleep
> warnings).  Now you've disabled irq's too.

I actually had to move the location of some of the locks to remove the
might sleep warnings.  Since I didn't know much about the might sleep
warnings before, my first course of action was to try using the disable
irq mechanism and I went ahead and just left them in once it was working
with the new lock placements.  I assume you believe they shouldn't be
necessary at all?

Thanks,
Kylie   

> 
> thanks,
> -chris

-
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/

[PATCH 1/1] tpm: fix cause of SMP stack traces

[Kylene Hall]

There were misplaced spinlock acquires and releases in the probe, open, 
close and release paths which were causing might_sleep and schedule while 
atomic error messages accompanied by stack traces when the kernel was 
compiled with SMP support. Bug reported by Reben Jenster 
<[EMAIL PROTECTED]>

Signed-off-by: Kylene Hall <[EMAIL PROTECTED]>
---
diff -uprN linux-2.6.10/drivers/char/tpm/tpm.c 
linux-2.6.10-tpm/drivers/char/tpm/tpm.c
--- linux-2.6.10/drivers/char/tpm/tpm.c 2005-01-18 16:42:17.0 -0600
+++ linux-2.6.10-tpm/drivers/char/tpm/tpm.c 2005-01-18 12:52:53.0 
-0600
@@ -373,8 +372,9 @@ int tpm_open(struct inode *inode, struct
 {
int rc = 0, minor = iminor(inode);
struct tpm_chip *chip = NULL, *pos;
+   unsigned long flags;
 
-   spin_lock(&driver_lock);
+   spin_lock_irqsave(&driver_lock, flags);
 
list_for_each_entry(pos, &tpm_chip_list, list) {
if (pos->vendor->miscdev.minor == minor) {
@@ -398,7 +398,7 @@ int tpm_open(struct inode *inode, struct
chip->num_opens++;
pci_dev_get(chip->pci_dev);
 
-   spin_unlock(&driver_lock);
+   spin_unlock_irqrestore(&driver_lock, flags);
 
chip->data_buffer = kmalloc(TPM_BUFSIZE * sizeof(u8), GFP_KERNEL);
if (chip->data_buffer == NULL) {
@@ -413,7 +413,7 @@ int tpm_open(struct inode *inode, struct
return 0;
 
 err_out:
-   spin_unlock(&driver_lock);
+   spin_unlock_irqrestore(&driver_lock, flags);
return rc;
 }
 
@@ -422,21 +422,25 @@ EXPORT_SYMBOL_GPL(tpm_open);
 int tpm_release(struct inode *inode, struct file *file)
 {
struct tpm_chip *chip = file->private_data;
+   unsigned long flags;
 
-   spin_lock(&driver_lock);
+   file->private_data = NULL;
+
+   spin_lock_irqsave(&driver_lock, flags);
chip->num_opens--;
+   spin_unlock_irqrestore(&driver_lock, flags);
+
down(&chip->timer_manipulation_mutex);
if (timer_pending(&chip->user_read_timer))
del_singleshot_timer_sync(&chip->user_read_timer);
else if (timer_pending(&chip->device_timer))
del_singleshot_timer_sync(&chip->device_timer);
up(&chip->timer_manipulation_mutex);
+
kfree(chip->data_buffer);
atomic_set(&chip->data_pending, 0);
 
pci_dev_put(chip->pci_dev);
-   file->private_data = NULL;
-   spin_unlock(&driver_lock);
return 0;
 }
 
@@ -524,16 +528,19 @@ EXPORT_SYMBOL_GPL(tpm_read);
 void __devexit tpm_remove(struct pci_dev *pci_dev)
 {
struct tpm_chip *chip = pci_get_drvdata(pci_dev);
+   unsigned long flags;
 
if (chip == NULL) {
dev_err(&pci_dev->dev, "No device data found\n");
return;
}
 
-   spin_lock(&driver_lock);
+   spin_lock_irqsave(&driver_lock, flags);
 
list_del(&chip->list);
 
+   spin_unlock_irqrestore(&driver_lock, flags);
+
pci_set_drvdata(pci_dev, NULL);
misc_deregister(&chip->vendor->miscdev);
 
@@ -541,8 +548,6 @@ void __devexit tpm_remove(struct pci_dev
device_remove_file(&pci_dev->dev, &dev_attr_pcrs);
device_remove_file(&pci_dev->dev, &dev_attr_caps);
 
-   spin_unlock(&driver_lock);
-
pci_disable_device(pci_dev);
 
dev_mask[chip->dev_num / 32] &= !(1 << (chip->dev_num % 32));
@@ -583,12 +588,14 @@ EXPORT_SYMBOL_GPL(tpm_pm_suspend);
 int tpm_pm_resume(struct pci_dev *pci_dev)
 {
struct tpm_chip *chip = pci_get_drvdata(pci_dev);
+   unsigned long flags;
+
if (chip == NULL)
return -ENODEV;
 
-   spin_lock(&driver_lock);
+   spin_lock_irqsave(&driver_lock, flags);
tpm_lpc_bus_init(pci_dev, chip->vendor->base);
-   spin_unlock(&driver_lock);
+   spin_unlock_irqrestore(&driver_lock, flags);
 
return 0;
 }
@@ -650,15 +657,12 @@ dev_num_search_complete:
chip->vendor->miscdev.dev = &(pci_dev->dev);
chip->pci_dev = pci_dev_get(pci_dev);
 
-   spin_lock(&driver_lock);
-
if (misc_register(&chip->vendor->miscdev)) {
dev_err(&chip->pci_dev->dev,
"unable to misc_register %s, minor %d\n",
chip->vendor->miscdev.name,
chip->vendor->miscdev.minor);
pci_dev_put(pci_dev);
-   spin_unlock(&driver_lock);
kfree(chip);
dev_mask[i] &= !(1 << j);
return -ENODEV;
@@ -672,7 +676,6 @@ dev_num_search_complete:
device_create_file(&pci_dev->dev, &dev_attr_pcrs);
device_create_file(&pci_dev->dev, &dev_attr_caps);
 
-   spin_unlock(&driver_lock);
return 0;
 }
 
-
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/