Docs: usb: Code and text updates from usb-skeleton

PATCH Superseded Philipp Hortmann 2 participants 8 messages Dec 4, 2021 – Dec 6, 2021

Subsystems (2)

Documentation USB

lore

Share & tools

lore

Philipp Hortmann

author

2021-12-04 16:35 UTC #0 lore raw

v2 cover Docs: usb: Code and text updates from usb-skeleton

Explanation and example code updates from usb-skeleton

v2: update patch #1 to #4
- corrected format of function names like the following example:
"usb_bulk_msg function" to "usb_bulk_msg()"

Philipp Hortmann (4):
Docs: usb: update usb_bulk_msg receiving example
Docs: usb: update comment and code near decrement our usage count for
the device
Docs: usb: update comment and code of function skel_delete
Docs: usb: update explanation for device_present to disconnected

 .../driver-api/usb/writing_usb_driver.rst     | 73 +++++++++----------
 1 file changed, 35 insertions(+), 38 deletions(-)

Signature · 1 line

2.25.1

Philipp Hortmann

author

2021-12-04 16:35 UTC #1 lore raw

PATCH v2 1/4 Docs: usb: update usb_bulk_msg receiving example

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 <[email protected]>

---
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(-)

Documentation/driver-api/usb/writing_usb_driver.rst +16 ~~-16~~

index b43e1ce49f0e..ed11398837e5 100644
@@ -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,
+		      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;
     }
 
 
-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
 or writes to a device; however, if you need to read or write constantly to
 a device, it is recommended to set up your own urbs and submit them to
-the USB subsystem.
+the USB subsystem. The template uses urbs for read and write.
 
 When the user program releases the file handle that it has been using to
 talk to the device, the release function in the driver is called. In

Signature · 1 line

2.25.1

Greg KH

maintainer

2021-12-05 11:01 UTC #2 lore raw

Re: [PATCH v2 1/4] Docs: usb: update usb_bulk_msg receiving example

Quoted 33 lines · Philipp Hortmann

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 <[email protected]>

---
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(-)

Documentation/driver-api/usb/writing_usb_driver.rst +4 -4

index b43e1ce49f0e..ed11398837e5 100644
@@ -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

Documentation/driver-api/usb/writing_usb_driver.rst do very much processing at that time. Our implementation of · line 232

Quoted 10 lines

Documentation/driver-api/usb/writing_usb_driver.rst +1 -6

 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.

Quoted 6 lines

Documentation/driver-api/usb/writing_usb_driver.rst +5 -0

+		      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 */

Documentation/driver-api/usb/writing_usb_driver.rst do very much processing at that time. Our implementation of · line 244

Quoted 10 lines

Documentation/driver-api/usb/writing_usb_driver.rst +4 -4

-    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.

Documentation/driver-api/usb/writing_usb_driver.rst do very much processing at that time. Our implementation of · line 247

Quoted 4 lines

Documentation/driver-api/usb/writing_usb_driver.rst +1 -1

-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.

Also, any reason you aren't cc:ing the USB maintainer on these changes? :)

thanks,

greg k-h

Philipp Hortmann

author

2021-12-05 17:04 UTC #3 lore raw

Re: [PATCH v2 1/4] Docs: usb: update usb_bulk_msg receiving example

Quoted 36 lines · Greg KH

Quoted 35 lines · Philipp Hortmann

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 <[email protected]>

---
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(-)

Documentation/driver-api/usb/writing_usb_driver.rst +4 -4

index b43e1ce49f0e..ed11398837e5 100644
@@ -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::

Quoted 10 lines

Documentation/driver-api/usb/writing_usb_driver.rst do very much processing at that time. Our implementation of · line 232
Quoted 8 lines
Documentation/driver-api/usb/writing_usb_driver.rst +1 -6
      /* 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().

Quoted 11 lines

Documentation/driver-api/usb/writing_usb_driver.rst +6 -3

+		      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) {

Quoted 8 lines

Documentation/driver-api/usb/writing_usb_driver.rst do very much processing at that time. Our implementation of · line 244
Quoted 6 lines
Documentation/driver-api/usb/writing_usb_driver.rst +3 -1
+	    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.

Quoted 10 lines

Documentation/driver-api/usb/writing_usb_driver.rst do very much processing at that time. Our implementation of · line 247
Quoted 5 lines
Documentation/driver-api/usb/writing_usb_driver.rst +1 -1
-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.

Quoted 2 lines

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 <[email protected]>,[email protected],[email protected]
you are not in charge.
thanks,
Philipp Hortmann

Quoted 4 lines

thanks,
greg k-h

Greg KH

maintainer

2021-12-06 07:47 UTC #4 lore raw

Re: [PATCH v2 1/4] Docs: usb: update usb_bulk_msg receiving example

Quoted 45 lines · Philipp Hortmann

Quoted 36 lines · Greg KH
Quoted 35 lines · Philipp Hortmann
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 <[email protected]>
---
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(-)
Documentation/driver-api/usb/writing_usb_driver.rst +4 -5
index b43e1ce49f0e..ed11398837e5 100644
@@ -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,
Quoted 8 lines
Documentation/driver-api/usb/writing_usb_driver.rst do very much processing at that time. Our implementation of · line 232
Quoted 6 lines
Documentation/driver-api/usb/writing_usb_driver.rst +1 -5
-			   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.

Quoted 1 line

Reason is that retval does not exist in skel_read().

Neither does any call to usb_bulk_msg(). So this is not code that is from that file at all. "retval" is easier to understand than "rv".

Quoted 8 lines

Quoted 1 line
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 <[email protected]>,[email protected],[email protected] you are not in charge.

Ah, documentation isn't added to the maintainers entry for USB, I'll go fix that up...

But note, my name is at the top of that file still, right?

thanks,

greg k-h

Philipp Hortmann

author

2021-12-04 16:35 UTC #5 lore raw

PATCH v2 2/4 Docs: usb: update comment and code near decrement our usage count for the device

In reply to Philipp Hortmann

Put release function in the document typical form
Update comment: decrement our usage count ..
and code according to usb-skeleton.c

Signed-off-by Philipp Hortmann <[email protected]>

---
V1 -> V2: Corrected format of function name to skel_release()

---
 Documentation/driver-api/usb/writing_usb_driver.rst | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Documentation/driver-api/usb/writing_usb_driver.rst +3 -3

index ed11398837e5..297b622f5438 100644
@@ -250,12 +250,12 @@ a device, it is recommended to set up your own urbs and submit them to
 the USB subsystem. The template uses urbs for read and write.
 
 When the user program releases the file handle that it has been using to
-talk to the device, the release function in the driver is called. In
+talk to the device, skel_release() in the driver is called. In
 this function we decrement our private usage count and wait for possible
 pending writes::
 
-    /* decrement our usage count for the device */
-    --skel->open_count;
+    /* decrement the count on our device */
+    kref_put(&dev->kref, skel_delete);
 
 
 One of the more difficult problems that USB drivers must be able to

Signature · 1 line

2.25.1

Philipp Hortmann

author

2021-12-04 16:35 UTC #6 lore raw

PATCH v2 3/4 Docs: usb: update comment and code of function skel_delete

In reply to Philipp Hortmann

Put skel_delete function in the document typical form Update code according to usb-skeleton.c

Signed-off-by Philipp Hortmann <[email protected]>

---
V1 -> V2: Corrected format of function name to skel_delete()

---
 .../driver-api/usb/writing_usb_driver.rst     | 21 +++++++++----------
 1 file changed, 10 insertions(+), 11 deletions(-)

Documentation/driver-api/usb/writing_usb_driver.rst +10 ~~-11~~

index 297b622f5438..b459f9e089e0 100644
@@ -262,19 +262,18 @@ One of the more difficult problems that USB drivers must be able to
 handle smoothly is the fact that the USB device may be removed from the
 system at any point in time, even if a program is currently talking to
 it. It needs to be able to shut down any current reads and writes and
-notify the user-space programs that the device is no longer there. The
-following code (function ``skel_delete``) is an example of how to do
-this::
+notify the user-space programs that the device is no longer there.
+skel_delete() is an example of how to do this::
 
-    static inline void skel_delete (struct usb_skel *dev)
+    static void skel_delete(struct kref *kref)
     {
-	kfree (dev->bulk_in_buffer);
-	if (dev->bulk_out_buffer != NULL)
-	    usb_free_coherent (dev->udev, dev->bulk_out_size,
-		dev->bulk_out_buffer,
-		dev->write_urb->transfer_dma);
-	usb_free_urb (dev->write_urb);
-	kfree (dev);
+	struct usb_skel *dev = to_skel_dev(kref);
+
+	usb_free_urb(dev->bulk_in_urb);
+	usb_put_intf(dev->interface);
+	usb_put_dev(dev->udev);
+	kfree(dev->bulk_in_buffer);
+	kfree(dev);
     }

Signature · 1 line

2.25.1

Philipp Hortmann

author

2021-12-04 16:35 UTC #7 lore raw

PATCH v2 4/4 Docs: usb: update explanation for device_present to disconnected

In reply to Philipp Hortmann

Update text for device_present flag to disconnected flag

Signed-off-by Philipp Hortmann <[email protected]>

---
V1 -> V2: Corrected format of function name to skel_disconnect()

---
 .../driver-api/usb/writing_usb_driver.rst          | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

Documentation/driver-api/usb/writing_usb_driver.rst +6 -8

index b459f9e089e0..fa795b8d7eac 100644
@@ -277,15 +277,13 @@ skel_delete() is an example of how to do this::
     }
 
 
-If a program currently has an open handle to the device, we reset the
-flag ``device_present``. For every read, write, release and other
+If the driver probed the device successfully, the flag ``disconnected``
+is initialized and set to false. For every read, write and other
 functions that expect a device to be present, the driver first checks
-this flag to see if the device is still present. If not, it releases
-that the device has disappeared, and a ``-ENODEV`` error is returned to the
-user-space program. When the release function is eventually called, it
-determines if there is no device and if not, it does the cleanup that
-the ``skel_disconnect`` function normally does if there are no open files
-on the device (see Listing 5).
+this flag to see if the device is still present. If not, a ``-ENODEV``
+error is returned to the user-space program. When the device is
+disconnected, skel_disconnect() is called. It sets ``disconnected``
+to true and cleans up.
 
 Isochronous Data
 ================

Signature · 1 line

2.25.1