Re: [PATCH] Infineon TPM: move infineon driver off pci_dev

[Andrew Morton <akpm@osdl.org>]

Have just taken a wander through the TPM driver.  I couldn't help myself - are
you OK with the below trivial changes?

Other things:

a)

ssize_t tpm_read(struct file * file, char __user *buf,
		 size_t size, loff_t * off)
{
	struct tpm_chip *chip = file->private_data;
	int ret_size;

	del_singleshot_timer_sync(&chip->user_read_timer);
	ret_size = atomic_read(&chip->data_pending);
	atomic_set(&chip->data_pending, 0);
	if (ret_size > 0) {	/* relay data */
		if (size < ret_size)
			ret_size = size;

		down(&chip->buffer_mutex);
		if (copy_to_user
		    ((void __user *) buf, chip->data_buffer, ret_size))
			ret_size = -EFAULT;

Why is the first arg to copy_to_user() typecast in this manner?  I don't
think the cast needs to be there?


b) user_reader_timeout does down() from within a timer handler!  That's
   deadlocky and is illegal - timer handlers are run from interrupt
   context.

   This should have generated a storm of runtime warnings if tested with
   CONFIG_PREEMPT and CONFIG_DEBUG_SPINLOCK_SLEEP.  Developers really should
   enable all the kernel debug options during development - they find bugs.

   Suggest you convert this to using schedule_work() or
   schedule_delayed_work(). 

c) In tpm_remove_hardware():

	dev_mask[chip->dev_num / TPM_NUM_MASK_ENTRIES ] &= !(1 << (chip->dev_num % TPM_NUM_MASK_ENTRIES));

   Is that `!' supposed to be there?   Looks odd.

d) How did this happen?

   int tpm_pm_suspend(struct device *dev, pm_message_t pm_state, u32 level)

   device_driver.suspend() only takes two arguments nowadays.


e)

	int tpm_pm_resume(struct device *dev, u32 level)

   Ditto



diff -puN drivers/char/tpm/tpm_atmel.c~tpm-tidies drivers/char/tpm/tpm_atmel.c
--- 25/drivers/char/tpm/tpm_atmel.c~tpm-tidies	Thu Oct 27 14:52:43 2005
+++ 25-akpm/drivers/char/tpm/tpm_atmel.c	Thu Oct 27 14:52:43 2005
@@ -40,7 +40,7 @@ enum tpm_atmel_read_status {
 	ATML_STATUS_READY = 0x08
 };
 
-static int tpm_atml_recv(struct tpm_chip *chip, u8 * buf, size_t count)
+static int tpm_atml_recv(struct tpm_chip *chip, u8 *buf, size_t count)
 {
 	u8 status, *hdr = buf;
 	u32 size;
@@ -100,7 +100,7 @@ static int tpm_atml_recv(struct tpm_chip
 	return size;
 }
 
-static int tpm_atml_send(struct tpm_chip *chip, u8 * buf, size_t count)
+static int tpm_atml_send(struct tpm_chip *chip, u8  *buf, size_t count)
 {
 	int i;
 
@@ -159,12 +159,12 @@ static struct tpm_vendor_specific tpm_at
 	.miscdev = { .fops = &atmel_ops, },
 };
 
-static struct platform_device *pdev = NULL;
+static struct platform_device *pdev;
 
 static void __devexit tpm_atml_remove(struct device *dev)
 {
 	struct tpm_chip *chip = dev_get_drvdata(dev);
-	if ( chip ) {
+	if (chip) {
 		release_region(chip->vendor->base, 2);
 		tpm_remove_hardware(chip->dev);
 	}
@@ -201,19 +201,17 @@ static int __init init_atmel(void)
 		(tpm_read_index(TPM_ADDR, 0x01) != 0x01 ))
 		return -ENODEV;
 
-	pdev = kmalloc(sizeof(struct platform_device), GFP_KERNEL);
+	pdev = kzalloc(sizeof(struct platform_device), GFP_KERNEL);
 	if ( !pdev )
 		return -ENOMEM;
 
-	memset(pdev, 0, sizeof(struct platform_device));
-
 	pdev->name = "tpm_atmel0";
 	pdev->id = -1;
 	pdev->num_resources = 0;
 	pdev->dev.release = tpm_atml_remove;
 	pdev->dev.driver = &atml_drv;
 
-	if ((rc=platform_device_register(pdev)) < 0) {
+	if ((rc = platform_device_register(pdev)) < 0) {
 		kfree(pdev);
 		pdev = NULL;
 		return rc;
@@ -234,7 +232,8 @@ static int __init init_atmel(void)
 		return rc;
 	}
 
-	dev_info(&pdev->dev, "Atmel TPM 1.1, Base Address: 0x%x\n", tpm_atmel.base);
+	dev_info(&pdev->dev, "Atmel TPM 1.1, Base Address: 0x%x\n",
+			tpm_atmel.base);
 	return 0;
 }
 
diff -puN drivers/char/tpm/tpm.c~tpm-tidies drivers/char/tpm/tpm.c
--- 25/drivers/char/tpm/tpm.c~tpm-tidies	Thu Oct 27 14:52:43 2005
+++ 25-akpm/drivers/char/tpm/tpm.c	Thu Oct 27 14:52:43 2005
@@ -162,7 +162,8 @@ ssize_t tpm_show_pcrs(struct device *dev
 		    < READ_PCR_RESULT_SIZE){
 			dev_dbg(chip->dev, "A TPM error (%d) occurred"
 				" attempting to read PCR %d of %d\n",
-				be32_to_cpu(*((__be32 *) (data + 6))), i, num_pcrs);
+				be32_to_cpu(*((__be32 *) (data + 6))),
+				i, num_pcrs);
 			goto out;
 		}
 		str += sprintf(str, "PCR-%02d: ", i);
@@ -194,12 +195,11 @@ ssize_t tpm_show_pubek(struct device *de
 	if (chip == NULL)
 		return -ENODEV;
 
-	data = kmalloc(READ_PUBEK_RESULT_SIZE, GFP_KERNEL);
+	data = kxalloc(READ_PUBEK_RESULT_SIZE, GFP_KERNEL);
 	if (!data)
 		return -ENOMEM;
 
 	memcpy(data, readpubek, sizeof(readpubek));
-	memset(data + sizeof(readpubek), 0, 20);	/* zero nonce */
 
 	if ((len = tpm_transmit(chip, data, READ_PUBEK_RESULT_SIZE)) <
 	    READ_PUBEK_RESULT_SIZE) {
@@ -243,7 +243,6 @@ out:
 	kfree(data);
 	return rc;
 }
-
 EXPORT_SYMBOL_GPL(tpm_show_pubek);
 
 #define CAP_VER_RESULT_SIZE 18
@@ -312,7 +311,6 @@ ssize_t tpm_store_cancel(struct device *
 }
 EXPORT_SYMBOL_GPL(tpm_store_cancel);
 
-
 /*
  * Device file system interface to the TPM
  */
@@ -336,8 +334,7 @@ int tpm_open(struct inode *inode, struct
 	}
 
 	if (chip->num_opens) {
-		dev_dbg(chip->dev,
-			"Another process owns this TPM\n");
+		dev_dbg(chip->dev, "Another process owns this TPM\n");
 		rc = -EBUSY;
 		goto err_out;
 	}
@@ -363,7 +360,6 @@ err_out:
 	spin_unlock(&driver_lock);
 	return rc;
 }
-
 EXPORT_SYMBOL_GPL(tpm_open);
 
 int tpm_release(struct inode *inode, struct file *file)
@@ -380,10 +376,9 @@ int tpm_release(struct inode *inode, str
 	spin_unlock(&driver_lock);
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(tpm_release);
 
-ssize_t tpm_write(struct file * file, const char __user * buf,
+ssize_t tpm_write(struct file * file, const char __user *buf,
 		  size_t size, loff_t * off)
 {
 	struct tpm_chip *chip = file->private_data;
@@ -419,7 +414,7 @@ ssize_t tpm_write(struct file * file, co
 
 EXPORT_SYMBOL_GPL(tpm_write);
 
-ssize_t tpm_read(struct file * file, char __user * buf,
+ssize_t tpm_read(struct file * file, char __user *buf,
 		 size_t size, loff_t * off)
 {
 	struct tpm_chip *chip = file->private_data;
@@ -441,7 +436,6 @@ ssize_t tpm_read(struct file * file, cha
 
 	return ret_size;
 }
-
 EXPORT_SYMBOL_GPL(tpm_read);
 
 void tpm_remove_hardware(struct device *dev)
@@ -465,13 +459,13 @@ void tpm_remove_hardware(struct device *
 
 	sysfs_remove_group(&dev->kobj, chip->vendor->attr_group);
 
-	dev_mask[chip->dev_num / TPM_NUM_MASK_ENTRIES ] &= !(1 << (chip->dev_num % TPM_NUM_MASK_ENTRIES));
+	dev_mask[chip->dev_num / TPM_NUM_MASK_ENTRIES ] &=
+		!(1 << (chip->dev_num % TPM_NUM_MASK_ENTRIES));
 
 	kfree(chip);
 
 	put_device(dev);
 }
-
 EXPORT_SYMBOL_GPL(tpm_remove_hardware);
 
 static u8 savestate[] = {
@@ -493,7 +487,6 @@ int tpm_pm_suspend(struct device *dev, p
 	tpm_transmit(chip, savestate, sizeof(savestate));
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(tpm_pm_suspend);
 
 /*
@@ -509,7 +502,6 @@ int tpm_pm_resume(struct device *dev, u3
 
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(tpm_pm_resume);
 
 /*
@@ -519,8 +511,7 @@ EXPORT_SYMBOL_GPL(tpm_pm_resume);
  * upon errant exit from this function specific probe function should call
  * pci_disable_device
  */
-int tpm_register_hardware(struct device *dev,
-			  struct tpm_vendor_specific *entry)
+int tpm_register_hardware(struct device *dev, struct tpm_vendor_specific *entry)
 {
 #define DEVNAME_SIZE 7
 
@@ -529,12 +520,10 @@ int tpm_register_hardware(struct device 
 	int i, j;
 
 	/* Driver specific per-device data */
-	chip = kmalloc(sizeof(*chip), GFP_KERNEL);
+	chip = kzalloc(sizeof(*chip), GFP_KERNEL);
 	if (chip == NULL)
 		return -ENOMEM;
 
-	memset(chip, 0, sizeof(struct tpm_chip));
-
 	init_MUTEX(&chip->buffer_mutex);
 	init_MUTEX(&chip->tpm_mutex);
 	INIT_LIST_HEAD(&chip->list);
@@ -558,8 +547,7 @@ int tpm_register_hardware(struct device 
 
 dev_num_search_complete:
 	if (chip->dev_num < 0) {
-		dev_err(dev,
-			"No available tpm device numbers\n");
+		dev_err(dev, "No available tpm device numbers\n");
 		kfree(chip);
 		return -ENODEV;
 	} else if (chip->dev_num == 0)
@@ -597,7 +585,6 @@ dev_num_search_complete:
 
 	return 0;
 }
-
 EXPORT_SYMBOL_GPL(tpm_register_hardware);
 
 MODULE_AUTHOR("Leendert van Doorn (leendert@watson.ibm.com)");
diff -puN drivers/char/tpm/tpm_infineon.c~tpm-tidies drivers/char/tpm/tpm_infineon.c
--- 25/drivers/char/tpm/tpm_infineon.c~tpm-tidies	Thu Oct 27 14:52:43 2005
+++ 25-akpm/drivers/char/tpm/tpm_infineon.c	Thu Oct 27 14:52:43 2005
@@ -30,10 +30,10 @@
 #define	TPM_INFINEON_DEV_VEN_VALUE	0x15D1
 
 /* These values will be filled after PnP-call */
-static int TPM_INF_DATA = 0;
-static int TPM_INF_ADDR = 0;
-static int TPM_INF_BASE = 0;
-static int TPM_INF_PORT_LEN = 0;
+static int TPM_INF_DATA;
+static int TPM_INF_ADDR;
+static int TPM_INF_BASE;
+static int TPM_INF_PORT_LEN;
 
 /* TPM header definitions */
 enum infineon_tpm_header {
_