Jason Gunthorpe <[email protected]> wrote on 02/10/2016
11:28:09 AM:
>
> On Wed, Feb 10, 2016 at 12:15:44AM -0500, Stefan Berger wrote:
> > Redid that now.
> > https://github.com/stefanberger/linux/commit/
> 83019eaab2cf5eb33f2665efdf9d2a117ed703b2
>
> Yeah, I think you've got that now.
>
> Just a few other random commments..
>
> This isn't great:
>
> vtpm_dev->dev_num = find_first_zero_bit(dev_mask, VTPM_NUM_DEVICES);
>
> We shouldn't artificially limit the number of devices if
> virtualization is the target. Use an idr, or figure out how to get rid
> of it. Since it is only used here:
>
> dev_set_name(&vtpm_dev->dev, "vtpms%d", vtpm_dev->dev_num);
>
> Maybe it could be adjusted to use chip->dev_num instead.
The chip->dev_num comes back from tpmm_chip_alloc() which is called with
the device as a parameter. That's the device we're trying to create. So it
seems we need to have two independency ways to generate unique device
names. My suggestion would have been to increase the bitmap to the maximum
size of the minor numbers but that's 20 bits, 1Mb. This would need to be
done for both sides.
I don't know how idr applies here from what I read in an article.
https://lwn.net/Articles/103209/
>
> This:
> INIT_WORK(&vtpm_dev->work, vtpm_dev_work);
> should be near the kzalloc
Ok, I'll move that.
>
> Drop the 1 line functions (vtpm_dev_work_start, vtpm_delete_vtpm_dev,
We have these function pairs here:
vtpm_dev_work_start
vtpm_dev_work_stop
and
vtpm_create_vtpm_dev
vtpm_delete_vtpm_dev
Can we just keep them as pairs with the one function undoing what the
other one has done. I prefix the one-liners with a 'static inline' and let
the compiler optimize the code.
>
> Use u32's here:
>
> u8 req_buf[TPM_BUFSIZE]; /* request buffer */
>
> and related to ensure the buffer is sensibly aligned
May I ask why use u32's for a byte buffer? I am not sure what alignment we
need here. 4 byte boundary for maximum memcpy performance?
http://lxr.free-electrons.com/source/drivers/char/tpm/tpm-dev.c#L34
http://lxr.free-electrons.com/source/drivers/char/tpm/tpm_i2c_infineon.c#L67
>
> Don't log on user space inputs, use debug or something:
>
> dev_err(&vtpm_dev->dev,
> "Invalid size in recv: count=%zd, req_len=%zd\n",
> count, len);
>
> This:
>
> vtpm_dev_work_stop(vtpm_dev);
>
> Doesn't kill a running work thread - more synchronization is needed
> for that corner case
Do you mean another test for STATE_OPEN bit is needed before it enters the
wait_event_interruptible in the code below?
Otherwise I tested this. What can happen is that the worker thread is
stuck in the wait function:
static int vtpm_tpm_op_recv(struct tpm_chip *chip, u8 *buf, size_t count)
[...]
/* wait for response or responder gone */
sig = wait_event_interruptible(vtpm_dev->wq,
(vtpm_dev->resp_len != 0
|| !test_bit(STATE_OPENED, &vtpm_dev->state)));
if (sig)
return -EINTR;
/* process gone ? */
if (!test_bit(STATE_OPENED, &vtpm_dev->state))
return -EIO;
[...]
We loop while the task is still busy and kick it out of the above wait
queue with this function here. Ok, so far we may kick multiple times since
the condition may only be tested after waiting.
static void vtpm_fops_undo_open(struct vtpm_dev *vtpm_dev)
{
clear_bit(STATE_OPENED, &vtpm_dev->state);
/* no more TPM responses -- wake up anyone waiting for them */
wake_up_interruptible(&vtpm_dev->wq);
}
>
> Make sure that devm is working for your specific case when this
> happens:
>
Just tested this and it seems to work. Chip is kfree'd once user space
closes the fd.
> err:
> /* did not register chip */
> vtpm_dev->chip = NULL;
>
> Instrument the core, devm should free the tpm chip when user space
> closes the fd. If not the core needs to provide a non-devm alloc for
> this driver. (easy)
It does that. We only partly unwind in the thread by letting the file
descriptor fail upon read and write and do the full unwinding upon file
descriptor closure.
>
> This spin lock:
>
> spin_lock(&driver_lock);
> vtpm_dev->dev_num = find_first_zero_bit(dev_mask, VTPM_NUM_DEVICES);
> [..]
> err = device_register(&vtpm_dev->dev); /* does get_device */
> spin_unlock(&driver_lock);
>
> I'm not comfortable with how big a region that is. If you keep
> dev_mask, then only lock around the dev_mask manipulation not other
> stuff. Relock to undo
Ok.
>
> fops_write should fail if op_recv isn't waiting for a buffer.
So we introduce another state flag whether we are in receiving or sending
mode ? Return -EBADF in the case of an unexpected write() ? Though -EBADF
indicates "fd is not a valid file descriptor or is not open for writing."
So return -EIO?
>
> Looks pretty good really
Yes. Thanks. And let's just keep the function pairs for the humans and let
the compiler use the 'static inline' .
Stefan
>
> Jason
>
------------------------------------------------------------------------------
Site24x7 APM Insight: Get Deep Visibility into Application Performance
APM + Mobile APM + RUM: Monitor 3 App instances at just $35/Month
Monitor end-to-end web transactions and take corrective actions now
Troubleshoot faster and improve end-user experience. Signup Now!
http://pubads.g.doubleclick.net/gampad/clk?id=272487151&iu=/4140
_______________________________________________
tpmdd-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel
