Re: [PATCH V3 1/3] drivers/char/tpm: Add new device driver tosupport IBM vTPM

[Ashley Lai]

Hi Ben.,

Thank you so much for the comments.  Please see my response below.

> > > + */
> > > +static int tpm_ibmvtpm_recv(struct tpm_chip *chip, u8 *buf, size_t count)
> > > +{
> > > +	struct ibmvtpm_dev *ibmvtpm;
> > > +	u16 len;
> > > +
> > > +	ibmvtpm = (struct ibmvtpm_dev *)chip->vendor.data;
> > > +
> > > +	if (!ibmvtpm->rtce_buf) {
> > > +		dev_err(ibmvtpm->dev, "ibmvtpm device is not ready\n");
> > > +		return 0;
> > > +	}
> > > +
> > > +	wait_event_interruptible(wq, ibmvtpm->crq_res.len != 0);
> > > +
> > > +	if (count < ibmvtpm->crq_res.len) {
> 
> That doesn't look right. The "other side" as far as I can tell is:
> 
> > > +		case VTPM_TPM_COMMAND_RES:
> > > +			ibmvtpm->crq_res.valid = crq->valid;
> > > +			ibmvtpm->crq_res.msg = crq->msg;
> > > +			ibmvtpm->crq_res.len = crq->len;
> > > +			ibmvtpm->crq_res.data = crq->data;
> > > +			wake_up_interruptible(&wq);
> 
> That looks racy to me. At the very least it should be doing:
> 
> 			ibmvtpm->crq_res.data = crq->data;
> 			smp_wmb();
> 			ibmvtpm->crq_res.len = crq->len;
> 
> IE. Ensure that "len" is written last, and possibly have an
> corresponding smp_rmb() after wait_event_interruptible() in the receive
> case.

Good catch. I agreed len should be written last and adding memory
barrier is a good idea.

> 
> Also, I dislike having a global symbol called "wq". You also don't seem
> to have any synchronization on access to that wq, can't you end up with
> several clients trying to send messages & wait for responses getting all
> mixed up ? You might need to make sure that at least you do a
> wake_up_interruptible_all() to ensure you wake them all up (inefficient
> but works). Unless you can track per-client ?

The TPM layer above allows only one request at any given point in time.
Therefore we don't need to wake_up_interruptible_all().  I can move wq
to the private structure.

> 
> Or is the above TPM layer making sure only one command/response happens
> at a given point in time ?

You are right.  See above.
> 
> You also do an interruptible wait but don't seem to be checking for
> signals (and not testing the result from wait_event_interruptible which
> might be returning -ERESTARTSYS in this case).
> 
Good point.  I will check for signal in the next version.

> That all sound a bit wrong to me ...
> > > +static irqreturn_t ibmvtpm_interrupt(int irq, void *vtpm_instance)
> > > +{
> > > +	struct ibmvtpm_dev *ibmvtpm = (struct ibmvtpm_dev *) vtpm_instance;
> > > +	unsigned long flags;
> > > +
> > > +	spin_lock_irqsave(&ibmvtpm->lock, flags);
> > > +	vio_disable_interrupts(ibmvtpm->vdev);
> > > +	tasklet_schedule(&ibmvtpm->tasklet);
> > > +	spin_unlock_irqrestore(&ibmvtpm->lock, flags);
> > > +
> > > +	return IRQ_HANDLED;
> > > +}
> 
> Tasklets ? We still use those things ? That's softirq iirc, do you
> really need to offload ? I mean, it allows to limit the amount of time
> an interrupt is disabled, but that's only useful if you assume you'll
> have quite a lot of traffic here, is that the case ?
> 
> You might be actually increasing latency here in favor of throughput, is
> that what you are aiming for ? Also keep in mind that
> vio_disable/enable_interrupt() being an hcall, it can have significant
> overhead. So again, only worth it if you're going to process a bulk of
> data at a time.

My original thought was to have the bottom half process the crq data
while the top half can service another request.  There is some work in
processing crq data but not that bad. You are right the
vio_disable/enable_interrupt() can have significant overhead.  I will
remove tasklet in the next version.

> 
> Cheers,
> Ben.
> 
> 

Thanks,
--Ashley Lai


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/