Messages in this thread | | | Subject | Re: [PATCH] usbip: add USBIP_URB_* URB transfer flags | From | Shuah Khan <> | Date | Fri, 19 Aug 2022 11:09:50 -0600 |
| |
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
| |