Thanks for the helpful comments I am working on a patch to fix your
concerns but I have a couple of questions.
On Wed, 2005-03-09 at 22:51 -0500, Jeff Garzik wrote:
<snip>
> > + 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.
>
> > + up(&chip->timer_manipulation_mutex);
When calling mod_timer and an occasional del_singleshot_timer_sync is it
necessary to protect this with a mutex like I was doing or not?
> > + pci_dev_get(chip->pci_dev);
> > +
> > + spin_unlock(&driver_lock);
> > +
> > + 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 prevent
> hotplug or something?
We were doing reference counting since their is a pci_dev in the chip
structure which set to the file->private_data pointer. Not correct?
>
> > + }
> > +
> > + down(&chip->buffer_mutex);
> > +
> > + if (in_size > TPM_BUFSIZE)
> > + in_size = TPM_BUFSIZE;
> > +
> > + if (copy_from_user
> > + (chip->data_buffer, (void __user *) buf, in_size)) {
> > + up(&chip->buffer_mutex);
> > + return -EFAULT;
> > + }
> > +
> > + /* atomic tpm command send and result receive */
> > + out_size = tpm_transmit(chip, chip->data_buffer, TPM_BUFSIZE);
>
> major bug? in_size may be smaller than TPM_BUFSIZE
>
Yes in_size might be but the chip->data_buffer will always be this size since
it is malloc'd in open. The operation needs to be atomic so the tpm_transmit
function is sending the command to the device and receiving the result which
might be bigger than in_size. The result is reported back to userspace from
this buffer on a read.
> > +
> > +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 = -ENODATA;
> > +
> > + if (atomic_read(&chip->data_pending) != 0) { /* Result available */
> > + down(&chip->timer_manipulation_mutex);
> > + del_singleshot_timer_sync(&chip->user_read_timer);
> > + up(&chip->timer_manipulation_mutex);
> > +
> > + down(&chip->buffer_mutex);
> > +
> > + ret_size = atomic_read(&chip->data_pending);
> > + atomic_set(&chip->data_pending, 0);
> > +
> > + if (ret_size == 0) /* timeout just occurred */
> > + ret_size = -ETIME;
> > + else if (ret_size > 0) { /* relay data */
> > + if (size < ret_size)
> > + ret_size = size;
> > +
> > + if (copy_to_user((void __user *) buf,
> > + chip->data_buffer, ret_size)) {
> > + ret_size = -EFAULT;
> > + }
> > + }
> > + up(&chip->buffer_mutex);
> > + }
> > +
> > + return ret_size;
>
> POSIX violation -- when there is no data available, returning a
> non-standard error is silly
>
So read should just return 0 if no data available?
Thanks,
Kylie
-
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/
