lkml.org 
[lkml]   [2005]   [Nov]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Subject[PATCH] TPM: cleanups
From
Date
Patch to clean up a couple of issues recently pointed out with the
driver. The fix for issue (c) was also pointed out by Steve Tate on the
tpm-devel list.


On Thu, 2005-10-27 at 14:55 -0700, Andrew Morton wrote:
> 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)
> {
> f 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?
>
Fixed in the patch below.

>
> 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().
>
I'll look into this.

> 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.
>
Fixed in the patch below.

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

>

Signed-off-by: Kylene Hall <kjhall@us.ibm.com>
---
--- linux-2.6.14/drivers/char/tpm/tpm.c.orig 2005-11-11 14:08:48.000000000 -0600
+++ linux-2.6.14/drivers/char/tpm/tpm.c 2005-11-11 14:09:47.000000000
-0600
@@ -428,8 +428,7 @@ ssize_t tpm_read(struct file * file, cha
ret_size = size;

down(&chip->buffer_mutex);
- if (copy_to_user
- ((void __user *) buf, chip->data_buffer, ret_size))
+ if (copy_to_user(buf, chip->data_buffer, ret_size))
ret_size = -EFAULT;
up(&chip->buffer_mutex);
}
@@ -460,7 +459,7 @@ 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));
+ ~(1 << (chip->dev_num % TPM_NUM_MASK_ENTRIES));

kfree(chip);


-
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: 2005-11-11 21:13    [from the cache]
©2003-2011 Jasper Spaans