On Friday, July 30, 2010 10:33 AM, Vasiliy Kulikov wrote:
> On Fri, Jul 30, 2010 at 11:47 -0500, H Hartley Sweeten wrote:
>> On Friday, July 30, 2010 4:07 AM, Kulikov Vasiliy wrote:
>>> put_user() may fail, if so return -EFAULT.
>>> Also compare count with copied data size, not size of struct with these
>>> fields.
>>>
>>> Signed-off-by: Kulikov Vasiliy <segooon@xxxxxxxxx>
>>> ---
>>> drivers/staging/dt3155/dt3155_drv.c | 121 ++++++++++++++++------------------
>>> 1 files changed, 57 insertions(+), 64 deletions(-)
>>
>> As Jiri mentioned, whitespace cleanups should be a separate patch.
>>
>> Also, please base your patch on the current linux-next tree. This patch will
>> not apply to the current source.
>
> No problem, I'll be able to do it on Monday or a bit later.
OK.
>>
>> I do have a couple more comments below.
>>
>
>>> + struct frame_info *frame_info;
>>> + u32 *buffer = (u32 *)buf;
>>
>> Why are you recasting the __user buffer? This will cause sparse warnings like:
>>
>> warning: cast removes address space of expression
>> warning: incorrect type in argument 1 (different address spaces)
>> expected void const volatile [noderef] <asn:1>*<noident>
>> got unsigned int [usertype] *buffer
>>
>> For each put_user and copy_to_user call.
>
> Right, I've thought that it's just a zero macro, but now I see that
> it is compiler __attribute__().
Yup.
>>
>>> +
>>> + /* TODO: this should check the error flag and */
>>> + /* return an error on hardware failures */
>>> + if (count != 4*3 + sizeof(struct frame_info)) {
>>> + pr_err("DT3155 ERROR (NJC): count is not right\n");
>>> + return -EINVAL;
>>> + }
>>
>> This is prone to breakage. It's safer to check against the struct size. Also,
>> the 4*3 is a bit ambiguous.
>>
>
> I think current variant is ambiguous too as it copies concrete data
> chunks, but measures struct size. In generic case sizeof(struct) != sum
> of sizeof(fields) (current case is ok).
Not really. The count is checked against what will actually be returned a
'struct dt3155_read'. It's the copy below that is confusing.
>>>
>>> - put_user(offset, (unsigned int __user *)buf);
>>> - buf += sizeof(u32);
>>> - put_user(fb->frame_count, (unsigned int __user *)buf);
>>> - buf += sizeof(u32);
>>> - put_user(dts->state, (unsigned int __user *)buf);
>>> - buf += sizeof(u32);
>>> - if (copy_to_user(buf, frame_info, sizeof(*frame_info)))
>>> - return -EFAULT;
>>> + if (put_user(offset, buffer) ||
>>> + put_user(fb->frame_count, buffer + 1) ||
>>> + put_user(dts->state, buffer + 2) ||
>>> + copy_to_user(buffer + 3, frame_info, sizeof(*frame_info)))
>>> + return -EFAULT;
>>
>> It would be better to just create a temporary 'struct dt3155_read' variable, fill
>> in the fields, and then just do one copy_to_user(). That's basically what the
>> three put_user calls followed by the copy_to_user() are doing. By using a local
>> variable you also prevent any problems with the data order getting changed.
>
> Yes, using struct for copying is the best variant IMO, but its second field
> is named frame_seq, not frame_count:
>
> struct dt3155_read {
> u32 offset;
> u32 frame_seq;
> u32 state;
>
> struct frame_info frame_info;
> };
>
> maybe it should be renamed to frame_count?
No. For two reasons.
1) The dt3155.h header is used by user space. All user space code would need to be
updated.
2) From the user space view 'frame_seq' makes sense. When the user 'starts' the driver
in dt3155_ioctl, fb->frame_count is set to 0. As each frame is processed in dt3155_isr,
fb->frame_count is incremented. So, when the user 'reads' from the driver to get the
current ready frame they also get back the sequence number of the frame.
Really it's just semantics. The first reason above is why we can't just rename the
variable.
Regards,
Hartley
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxxxxxxxxxxx
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
