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    [W:0.029 / U:88.592 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site