On 1/11/22 9:51 AM, Greg KH wrote:
On Mon, Jan 10, 2022 at 10:31:28PM +0100, Philipp Hortmann wrote:
Hi all,
template usb-skeleton.c is working but outdated, documentation is helpful but years old and checkpatch.pl is giving hints to deprecated functions. This information is helpful but it does not show the way how to write a state of the art USB driver. Where to find this information? In the USB mailing list? By checking in git on which files most maintenance activities were done?
First off, what do you mean by "state of the art"? USB drivers are
Program code that is using the latest functions, macros and is up to date. Simply code that you like or consider best.
almost never just a USB driver. The USB portion of the driver is the "simple" part. The "real" part of the driver is going to be doing whatever functionality the device is (i.e. sound device, keyboard device, video device, etc.)
USB is just a dumb pipe. The USB portion of your driver just needs to set up the pipes, and get the data flowing in them. How you deal with that data is going to be the real work you have to do, and the majority of the driver size. Otherwise you really do not even need a USB driver, and you can just do everything from userspace and use libusb to read/write to the USB device directly.
If you have a new driver you need to write, look at the existing drivers for guidance, and the documentation. Then submit it to the linux-usb mailing list for review, the people there will help you out with any portions that you have problems with.
Intention of this email was to not bother the community to much as I am a newbie.
So, what have you tried so far that is not working and what type of device are you trying to control that needs a new USB driver for?
No new driver and no new device. I want to do maintenance on old drivers. I have not chosen one yet as I want to practice first to get more into details. I have adapted the usb-skeleton.c for a USB to serial adapter and for a USB LCD Display. When I am doing it right the usb-skeleton.c is working very well. The reason why I am asking is because very experienced kernel developers have said it is "out of date" and want to delete it because it is so bad and so wrong. I do not understand what is so bad and so wrong. Find referencing emails below. Thanks for your reply and the time you put into this email. Bye Philipp On 12/7/21 11:24 AM, Greg KH wrote:
On Tue, Dec 07, 2021 at 11:16:37AM +0100, Oliver Neukum wrote:
Hi,
it seems to me that the method of maintaining an example driver does not work because it will inevitably be
* untested
* out of date
Thus our documentation would be improved by replacing its examples with code from drivers for real hardware. Such code wouldn't be pretty or written for text books, but it would be tested. I could do it this week in a first proposal. But I don't want to start if somebody feels that the skeleton driver absolutely has to stay.
As the author of the skeleton driver, I have no objections to removing it as it is showing its age and all of the problems that you mention here are real.
So sure, delete away!
thanks,
greg k-h
On 12/12/21 2:25 AM, Philipp Hortmann wrote:
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);