Re: [PATCH] usbip: add USBIP_URB_* URB transfer flags

[Shuah Khan]

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