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

Hi Ben.,

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

quoted

quoted

+ */
+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:

quoted

quoted

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

quoted

quoted

+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