Oliver Neukum <oneukum@xxxxxxx> writes:
> Please see the comments in the quoted code.
Thanks so much for your review; it's my first USB driver, so I was
pretty sure I'd make plenty of mistakes.
> Do we really need yet another version of home grown debug macros?
No, and now that the driver is working, it's just noise. I've removed a
bunch of the debug statements and changed the remaining ones to use
dev_dbg. That will use dynamic_dev_dbg when CONFIG_DYNAMIC_DEBUG is
enabled, otherwise it will follow any setting of the DEBUG define.
I did wrap dev_dbg with a 'usb_dbg' which takes the interface instead of
the embedded device.
> That is a violation of the DMA rules on non-coherent architectures.
> The buffer must be allocated separately.
Ok, I'm not sure I understand the reasoning given that both come from
kmalloc, but that's easy to do.
> You do the test for the plausibility of size after you've used it.
Thanks. fixed.
> This can be easily deduced from the interface.
Extra struct member removed.
>> + mutex_init(&dev->lock);
>> + mutex_init(&dev->rng_lock);
>
> Shift this.
Moved above the call to usb_register_dev.
>> + while (count > 0) {
>> +
>> + /* Grab the rng_lock briefly to ensure that the hwrng interface
>> + * gets priority over other user access
>> + */
>> + intr = mutex_lock_interruptible(&dev->rng_lock);
>> + if (intr) {
>> + CK_LEAVE_INT(DEBUG_DEV, "read (rng_lock)", -EINTR);
>> + return -EINTR;
>
> At this point you may have already copied data to user space. A signal
> should cause a short read in that case, not -EINTR.
> Also the mutex is still held.
Right. I've rewritten this to provide a common path at the bottom of the
function which returns the number of bytes read if non-zero, otherwise
returns any error value. That eliminates differences in behaviour
depending on where an error occurred during the operation.
In this case, if the mutex_lock fails, we *don't* have the mutex though,
so it should not be unlocked. There are other cases which do need to
unlock the mutex below.
> The same issue as above.
>
>> + return -EINTR;
>> + }
>> + if (dev->valid == dev->used) {
>> + result = _chaoskey_fill(dev);
>> + if (result) {
>> + if (read_count) {
>> + CK_LEAVE_SIZE(DEBUG_DEV, "read (count)",
>> + read_count);
>> + return read_count;
>
> The mutex is still held.
>
>> + }
>> + CK_LEAVE_INT(DEBUG_DEV, "read (result)", result);
>> + return result;
>
> The mutex is still held.
These are now handled by the same common return path, with the addition
of a call to mutex_unlock before jumping there.
Again, thanks very much for your thoughtful review. I'll reply to this
message with an updated version of the driver patch; I discovered that
my local emacs configuration used spaces instead of tabs in the kernel
tree I'm working in, so the diff from the version you've seen changes
every single line...
--
-keith
Attachment:
signature.asc
Description: PGP signature
