Jason Gunthorpe <[email protected]> wrote on 02/10/2016
05:23:13 PM:
>
>
> > > 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
>
> Hm, that is only needed for devm, could also do the tpm/tpmm split and
> avoid needing a registered dev.
>
How about another patch on top of the ones I have so far only solving this
problem ?
>
>
> > > 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?
>
> Yes, it is nice to see the alignment guarenteed if memcpy is the
> primary use of this memory. It almost always happens naturally..
We now have only one buffer and there's a size_t in front of it. So
alignment will be at 4 bytes.
>
> > > 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?
>
> No, just use a typical kernel idiom:
>
> clear_bit(STATE_OPENED, &vtpm_dev->state);
> wake_up_interruptible(&vtpm_dev->wq);
> flush_workqueue(&vtpm_dev->work);
>
> And it would be typical/desirable to see that open coded in the
> one and only cleanup routine. That is what I usually look for when
> looking at work queues.
>
> I can't rember off hand if you need locking around dev->state to make
> the above work, IIRC the bit ops are special.
These ops are atomic:
http://lxr.free-electrons.com/source/include/asm-generic/bitops/atomic.h#L86
Stefan
>
> Don't use work_busy in production code.
>
> > >
> > > 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?
>
> Probably yeah, pick an error code makes sense. EIO is probably OK, but
> maybe shift the 'STATE_OPEN' code to EPIPE or something.
>
> 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
