lkml.org 
[lkml]   [2022]   [Aug]   [19]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH] usbip: add USBIP_URB_* URB transfer flags
From
Date
On 8/19/22 2:29 AM, Greg KH wrote:
>
> Overall, nice work, thanks for adding this, minor comments below:
>
> On Tue, Aug 16, 2022 at 03:57:11PM -0600, Shuah Khan wrote:
>> USBIP driver packs URB transfer flags in PDUs that are exchanged
>
> What is a "PDU"?

Protocol Data Unit - I will expand or reword to drop it.

[snip]

>> +-----------+--------+---------------------------------------------------+
>> | 0x14 | 4 | transfer_flags: possible values depend on the |
>> | | | URB transfer_flags (refer to URB doc in |
>
> You can drop the second "URB" here, as it's an USBIP document being
> referred to, right?

This refers to USB URB document. I can rephrase it to USBIP_URB transfer flags
as it is the API now.

>>
>> + /* Unpack the pdu to an urb (map USBIP_URB_* to URB_* flags) */
>
> No need to mention the mapping everywhere in these comments as the
> function handles that.
>

These comments were primarily added for making sure right mapping is done.
Will remove them.


>> + }
>> + pr_debug("%s: flag = %u map_flags = %u\n", __func__,
>> + flags, map_flags);
>
> When using pr_debug (or dev_dbg()), you already have access to the
> __func__ in the output automatically, so there's never a need to add it
> again, it would just show up twice.
>

Good point.

> Also, this should be dev_dbg(), as it's a driver, and you have access to
> the device being handled (well, it needs to be passed to the
> function...)
>

I am going to delete these - debug message isn't really needed.

>> +
>> + return map_flags;
>> +}
>> +
>> +static unsigned int usbip_to_urb(unsigned int flags)
>> +{
>> + unsigned int map_flags = 0;
>> + int loop;
>> +
>> + for (loop = 0; loop < NUM_USBIP_FLAGS; loop++) {
>> + if (flags & flag_map[loop].usbip_flag)
>> + map_flags |= flag_map[loop].urb_flag;
>> + }
>> + pr_debug("%s: flag = %u map_flags = %u\n", __func__,
>> + flags, map_flags);
>
> Same debug comment as above.

Same as above. I will just delete the message.

>

>> + * As a result of coying the transfer flags, they now have become part
>
> "coying"? :)

Thanks for catching this :)

>
>> + * of the API. Any changes to kernel flags now impact the USBIP user-space.
>> + * In addition, it will be compatibility problem between server and client.
>> + * These defines aren't directly referenced and it is an obscure usage
>> + * hidden away in the packets.
>
> No need to mention the history here, just say "here are the flags for
> the transfer types"
>

Will do.

>
>> + *
>> + * Add code to translate the values between the numbers used in userspace
>> + * and the numbers used in the kernel. This will help with any future
>> + * changes to kernel flags breaking the API.
>
> There's no code being added in this uapi file, so you might want to
> reword all of this. Possibly by removing most of it, as the history
> here might not be necessary to document.
>

Will do.

thanks for the review. Will send v2.

thanks,
-- Shuah

\
 
 \ /
  Last update: 2022-08-19 19:46    [W:0.045 / U:0.068 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site