linux-kernel - Re: [PATCH v2 1/4] Docs: usb: update usb_bulk

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <f3a1b2ba-85fe-e829-0d11-07a4bc4d8756@gmail.com>
Date:   Sun, 5 Dec 2021 18:04:48 +0100
From:   Philipp Hortmann <philipp.g.hortmann@...il.com>
To:     Greg KH <gregkh@...uxfoundation.org>
Cc:     corbet@....net, linux-doc@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-usb@...r.kernel.org
Subject: Re: [PATCH v2 1/4] Docs: usb: update usb_bulk_msg receiving example

On 12/5/21 12:00 PM, Greg KH wrote:
> On Sat, Dec 04, 2021 at 05:35:11PM +0100, Philipp Hortmann wrote:
>> Clarification that this example is not in the driver template anymore.
>> Update code example so that it fits best to usb-skeleton.c
>> Update format of function names
>>
>> Signed-off-by: Philipp Hortmann <philipp.g.hortmann@...il.com>
>> ---
>> V1 -> V2: Added "Update format of function names" to patch description
>>            Corrected format of function names like the following example:
>>            "`usb_bulk_msg` function" to "usb_bulk_msg()"
>> ---
>>   .../driver-api/usb/writing_usb_driver.rst     | 32 +++++++++----------
>>   1 file changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/Documentation/driver-api/usb/writing_usb_driver.rst b/Documentation/driver-api/usb/writing_usb_driver.rst
>> index b43e1ce49f0e..ed11398837e5 100644
>> --- a/Documentation/driver-api/usb/writing_usb_driver.rst
>> +++ b/Documentation/driver-api/usb/writing_usb_driver.rst
>> @@ -218,36 +218,36 @@ do very much processing at that time. Our implementation of
>>   ``skel_write_bulk_callback`` merely reports if the urb was completed
>>   successfully or not and then returns.
>>   
>> -The read function works a bit differently from the write function in
>> +This read function works a bit differently from the write function in
>>   that we do not use an urb to transfer data from the device to the
>> -driver. Instead we call the :c:func:`usb_bulk_msg` function, which can be used
>> +driver. Instead we call usb_bulk_msg(), which can be used
>>   to send or receive data from a device without having to create urbs and
>> -handle urb completion callback functions. We call the :c:func:`usb_bulk_msg`
>> -function, giving it a buffer into which to place any data received from
>> +handle urb completion callback functions. We call usb_bulk_msg(),
>> +giving it a buffer into which to place any data received from
>>   the device and a timeout value. If the timeout period expires without
>>   receiving any data from the device, the function will fail and return an
>>   error message. This can be shown with the following code::
>>   
>>       /* do an immediate bulk read to get data from the device */
>> -    retval = usb_bulk_msg (skel->dev,
>> -			   usb_rcvbulkpipe (skel->dev,
>> -			   skel->bulk_in_endpointAddr),
>> -			   skel->bulk_in_buffer,
>> -			   skel->bulk_in_size,
>> -			   &count, 5000);
>> +    rv = usb_bulk_msg(dev->udev,
> 
> Why are you changing the varible name?  That seems unnecessary.
Reason is that retval does not exist in skel_read().
> 
>> +		      usb_rcvbulkpipe (dev->udev,
>> +		      dev->bulk_in_endpointAddr),
>> +		      dev->bulk_in_buffer,
>> +	              dev->bulk_in_size,
>> +		      &len, 5000);
>>       /* if the read was successful, copy the data to user space */
>> -    if (!retval) {
>> -	    if (copy_to_user (buffer, skel->bulk_in_buffer, count))
>> -		    retval = -EFAULT;
>> +    if (!rv) {
>> +	    if (copy_to_user (buffer, dev->bulk_in_buffer, len))
>> +		    rv = -EFAULT;
>>   	    else
>> -		    retval = count;
>> +		    rv = len;
>>       }
> 
> Leaving the variable name alone keeps this patch much smaller.
Will leave the variable name in the next patch.
> 
>>   
>>   
>> -The :c:func:`usb_bulk_msg` function can be very useful for doing single reads
>> +usb_bulk_msg() can be very useful for doing single reads
> 
> You are doing different things in this patch, one is converting the
> function style and one is updating the text.  How about doing just the
> function name stuff first, all in one patch, and then the updates, as
> that would make it much easier to read.
Will be changed.
> 
> Also, any reason you aren't cc:ing the USB maintainer on these changes?  :)
According to:
perl scripts/get_maintainer.pl --separator , --nokeywords --nogit 
--nogit-fallback --norolestats -f 
Documentation/driver-api/usb/writing_usb_driver.rst
Jonathan Corbet 
<corbet@....net>,linux-doc@...r.kernel.org,linux-kernel@...r.kernel.org
you are not in charge.
thanks,
Philipp Hortmann
> thanks,
> 
> greg k-h
>