Re: [PATCH v5 4/5] Initialize TPM and get durations and timeouts

["Stefan Berger" <stefanb-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 5276 bytes --]

Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> 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
> 


[-- Attachment #1.2: Type: text/html, Size: 10283 bytes --]

[-- Attachment #2: Type: text/plain, Size: 413 bytes --]

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

[-- Attachment #3: Type: text/plain, Size: 192 bytes --]

_______________________________________________
tpmdd-devel mailing list
tpmdd-devel-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
https://lists.sourceforge.net/lists/listinfo/tpmdd-devel