Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH] usbip: add USBIP_URB_* URB transfer flags
To:     Greg KH <gregkh@linuxfoundation.org>
Cc:     valentina.manea.m@gmail.com, shuah@kernel.org,
        mailhol.vincent@wanadoo.fr, linux-usb@vger.kernel.org,
        linux-kernel@vger.kernel.org,
        Hongren Zenithal Zheng <i@zenithal.me>,
        Alan Stern <stern@rowland.harvard.edu>,
        Shuah Khan <skhan@linuxfoundation.org>
References: <20220816215711.55025-1-skhan@linuxfoundation.org>
 <Yv9J2d1bVe7Xntxu@kroah.com>
From:   Shuah Khan <skhan@linuxfoundation.org>
Message-ID: <f2cf1fad-143f-f09b-0874-7e68c675697c@linuxfoundation.org>
Date:   Fri, 19 Aug 2022 11:09:50 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.8.1
MIME-Version: 1.0
In-Reply-To: <Yv9J2d1bVe7Xntxu@kroah.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk

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