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()"
v3: update patch #1 to #4 and created patch #5
- moved correction of format of function names to own patch #5
- reverted change of variable from retval to rv in patch #1
Philipp Hortmann (5):
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
Docs: usb: correct format of function names in the explanations
.../driver-api/usb/writing_usb_driver.rst | 69 +++++++++----------
1 file changed, 33 insertions(+), 36 deletions(-)
--
2.25.1
Clarification that this example is not in the driver template anymore.
Update code example so that it fits best to usb-skeleton.c
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()"
V2 -> V3: Moved corrections of the function name to an own patch in this
patch series
Took back change of variable from retval to rv
---
.../driver-api/usb/writing_usb_driver.rst | 20 +++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/Documentation/driver-api/usb/writing_usb_driver.rst b/Documentation/driver-api/usb/writing_usb_driver.rst
index b43e1ce49f0e..1fd7bf1dbdb0 100644
--- a/Documentation/driver-api/usb/writing_usb_driver.rst
+++ b/Documentation/driver-api/usb/writing_usb_driver.rst
@@ -218,7 +218,7 @@ 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
to send or receive data from a device without having to create urbs and
@@ -229,25 +229,25 @@ 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);
+ retval = 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))
+ if (copy_to_user (buffer, dev->bulk_in_buffer, len))
retval = -EFAULT;
else
- retval = count;
+ retval = len;
}
The :c:func:`usb_bulk_msg` function 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
--
2.25.1
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()
V2 -> V3: Moved correction of the function name to an own patch in this
patch series
---
Documentation/driver-api/usb/writing_usb_driver.rst | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/driver-api/usb/writing_usb_driver.rst b/Documentation/driver-api/usb/writing_usb_driver.rst
index 1fd7bf1dbdb0..c336dfd82426 100644
--- a/Documentation/driver-api/usb/writing_usb_driver.rst
+++ b/Documentation/driver-api/usb/writing_usb_driver.rst
@@ -254,8 +254,8 @@ talk to the device, the release function 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
--
2.25.1
Update code according to usb-skeleton.c
Signed-off-by: Philipp Hortmann <[email protected]>
---
V1 -> V2: Corrected format of function name to skel_delete()
V2 -> V3: Moved correction of the function name to an own patch in this
patch series
---
.../driver-api/usb/writing_usb_driver.rst | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/Documentation/driver-api/usb/writing_usb_driver.rst b/Documentation/driver-api/usb/writing_usb_driver.rst
index c336dfd82426..6c487ac5eab2 100644
--- a/Documentation/driver-api/usb/writing_usb_driver.rst
+++ b/Documentation/driver-api/usb/writing_usb_driver.rst
@@ -266,15 +266,15 @@ 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::
- 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);
}
--
2.25.1
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()
V2 -> V3: Moved correction of the function name to an own patch in this
patch series
---
.../driver-api/usb/writing_usb_driver.rst | 14 ++++++--------
1 file changed, 6 insertions(+), 8 deletions(-)
diff --git a/Documentation/driver-api/usb/writing_usb_driver.rst b/Documentation/driver-api/usb/writing_usb_driver.rst
index 6c487ac5eab2..1968cf5c55f6 100644
--- a/Documentation/driver-api/usb/writing_usb_driver.rst
+++ b/Documentation/driver-api/usb/writing_usb_driver.rst
@@ -278,15 +278,13 @@ 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, the ``skel_disconnect`` function is called. It sets
+``disconnected`` to true and cleans up.
Isochronous Data
================
--
2.25.1
Correct format of function names like the following example:
"`usb_bulk_msg` function" to "usb_bulk_msg()"
Signed-off-by: Philipp Hortmann <[email protected]>
---
.../driver-api/usb/writing_usb_driver.rst | 19 +++++++++----------
1 file changed, 9 insertions(+), 10 deletions(-)
diff --git a/Documentation/driver-api/usb/writing_usb_driver.rst b/Documentation/driver-api/usb/writing_usb_driver.rst
index 1968cf5c55f6..9f4181cafc41 100644
--- a/Documentation/driver-api/usb/writing_usb_driver.rst
+++ b/Documentation/driver-api/usb/writing_usb_driver.rst
@@ -220,10 +220,10 @@ successfully or not and then returns.
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::
@@ -244,13 +244,13 @@ error message. This can be shown with the following code::
}
-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 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::
@@ -262,9 +262,8 @@ 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 void skel_delete(struct kref *kref)
{
@@ -283,8 +282,8 @@ 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, a ``-ENODEV``
error is returned to the user-space program. When the device is
-disconnected, the ``skel_disconnect`` function is called. It sets
-``disconnected`` to true and cleans up.
+disconnected, skel_disconnect() is called. It sets ``disconnected``
+to true and cleans up.
Isochronous Data
================
--
2.25.1
On 06.12.21 21:57, Philipp Hortmann wrote:
> The :c:func:`usb_bulk_msg` function 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.
Hi,
now that I read this, shouldn't we tell the reader why exactly the use
of URBs is a good idea at that place?
Regards
Oliver
On 06.12.21 21:57, Philipp Hortmann wrote:
> Update comment: decrement our usage count ..
> and code according to usb-skeleton.c
Hi,
and that is exactly the problem, I am afraid.
Your patch would be correct if the underlying code were correct.
>
> - /* 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
I am sorry but the code in usb-skel.c is wrong. You grab a reference
in skel_open():
/* increment our usage count for the device */
kref_get(&dev->kref);
which is good, but in skel_release() we do:
/* decrement the count on our device */
kref_put(&dev->kref, skel_delete);
unconditionally.
Think this through:
- Device is plugged in -> device node and internal data is created
- open() called -> kref_get(), we get a reference
- close() -> kref_put() -> refcount goes to zero -> skel_delete() is called, struct usb_skel is freed:
static void skel_delete(struct kref *kref)
{
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);
}
with intfdata left intact.
- open() is called again -> We are following a dangling pointer into cloud cuckoo land.
Unfortunately this code is older than git, so I cannot just send a revert.
What to do?
Regards
Oliver
On 12/7/21 10:30 AM, Oliver Neukum wrote:
> On 06.12.21 21:57, Philipp Hortmann wrote:
>
>> Update comment: decrement our usage count ..
>> and code according to usb-skeleton.c
>
> Hi,
>
> and that is exactly the problem, I am afraid.
> Your patch would be correct if the underlying code were correct.
>
>>
>> - /* 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
>
> I am sorry but the code in usb-skel.c is wrong. You grab a reference
> in skel_open():
>
> /* increment our usage count for the device */
> kref_get(&dev->kref);
>
> which is good, but in skel_release() we do:
>
> /* decrement the count on our device */
> kref_put(&dev->kref, skel_delete);
>
> unconditionally.
>
> Think this through:
>
> - Device is plugged in -> device node and internal data is created
> - open() called -> kref_get(), we get a reference
> - close() -> kref_put() -> refcount goes to zero -> skel_delete() is called, struct usb_skel is freed:
>
> static void skel_delete(struct kref *kref)
> {
> 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);
> }
>
> with intfdata left intact.
>
> - open() is called again -> We are following a dangling pointer into cloud cuckoo land.
>
> Unfortunately this code is older than git, so I cannot just send a revert.
> What to do?
>
> Regards
> Oliver
>
I cannot see the issue you described.
Think this through:
- probe() is called and kref_init() sets refcount to 1
- open() is called and refcount is increased to 2
- close() is called and refcount is decreased to 1 -> delete() is not called
- disconnect() is called and refcount is decreased to 0 -> delete() is
called
Putting debug messages into the code and follow the log:
[12820.221534] skeleton 2-1.6:1.0: skel_probe called
[12820.221658] skeleton 2-1.6:1.0: USB Skeleton device now attached to
USBSkel-1
[12820.221690] usbcore: registered new interface driver skeleton
[12824.046075] skeleton 2-1.6:1.0: skel_open called
[12825.047213] skeleton 2-1.6:1.0: skel_release called
[12826.047854] skeleton 2-1.6:1.0: skel_open called
[12827.049017] skeleton 2-1.6:1.0: skel_release called
[12831.035262] usb 2-1.6: USB disconnect, device number 4
[12831.035500] skeleton 2-1.6:1.0: skel_disconnect call
[12831.035504] skeleton 2-1.6:1.0: skel_delete called
[12831.035507] skeleton 2-1.6:1.0: USB Skeleton #1 now disconnected
delete() is only called on disconnect and not earlier.
This seems to be fine to me. Please find position of debug messages below.
Thanks for your reply.
Regards,
Philipp
/* Define these values to match your devices */
-#define USB_SKEL_VENDOR_ID 0xfff0
-#define USB_SKEL_PRODUCT_ID 0xfff0
+#define USB_SKEL_VENDOR_ID 0x1a86
+#define USB_SKEL_PRODUCT_ID 0x7523
-/* table of devices that work with this driver */
static const struct usb_device_id skel_table[] = {
{ USB_DEVICE(USB_SKEL_VENDOR_ID, USB_SKEL_PRODUCT_ID) },
{ } /* Terminating entry */
@@ -73,6 +72,7 @@ static void skel_delete(struct kref *kref)
{
struct usb_skel *dev = to_skel_dev(kref);
+ dev_info(&dev->interface->dev, "skel_delete called\n");
usb_free_urb(dev->bulk_in_urb);
usb_put_intf(dev->interface);
usb_put_dev(dev->udev);
@@ -110,6 +110,7 @@ static int skel_open(struct inode *inode, struct
file *file)
/* increment our usage count for the device */
kref_get(&dev->kref);
+ dev_info(&interface->dev, "skel_open called\n");
/* save our object in the file's private structure */
file->private_data = dev;
@@ -125,6 +126,7 @@ static int skel_release(struct inode *inode, struct
file *file)
if (dev == NULL)
return -ENODEV;
+ dev_info(&dev->interface->dev, "skel_release called\n");
/* allow the device to be autosuspended */
usb_autopm_put_interface(dev->interface);
@@ -507,6 +509,7 @@ static int skel_probe(struct usb_interface *interface,
dev->udev = usb_get_dev(interface_to_usbdev(interface));
dev->interface = usb_get_intf(interface);
+ dev_info(&dev->interface->dev, "skel_probe called\n");
/* set up the endpoint information */
/* use only the first bulk-in and bulk-out endpoints */
retval = usb_find_common_endpoints(interface->cur_altsetting,
@@ -577,6 +580,7 @@ static void skel_disconnect(struct usb_interface
*interface)
usb_kill_urb(dev->bulk_in_urb);
usb_kill_anchored_urbs(&dev->submitted);
+ dev_info(&dev->interface->dev, "skel_disconnect call\n");
/* decrement our usage count */
kref_put(&dev->kref, skel_delete);
On 12/7/21 9:55 AM, Oliver Neukum wrote:
>
> On 06.12.21 21:57, Philipp Hortmann wrote:
>> The :c:func:`usb_bulk_msg` function 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.
>
> Hi,
>
> now that I read this, shouldn't we tell the reader why exactly the use
> of URBs is a good idea at that place?
>
> Regards
> Oliver
>
The Documentation/driver-api/usb/URB.rst does describe this well. We
could think about a link to it.
Regards
Philipp