Where to find the information how to write a state of the art USB driver?
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? Thanks for your support. Bye Philipp
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 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. 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? thanks, greg k-h
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);
On Tue, Jan 11, 2022 at 09:09:31PM +0100, Philipp Hortmann wrote:
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.
It all depends on the use-case for what is the "best". If you have a slow device, and only need control/bulk USB transfers, that is one type of design pattern of a USB driver, while higher throughput requirements using streams is another, and data that can be lossy using isoc endpoints is yet another. So it will depend.
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.
Doing maintenance on old drivers that currently work well is hard without having the hardware for them and finding a real problem with the driver itself. So start with drivers for devices that you have today, and look and see if you can find any throughput issues or anything else. But again, that's going to be very device/type specific as to what needs to happen there.
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.
That driver tried to be an example for an unknown device, doing multiple different things that no single driver/device would probably ever need. Also it can almost always just be replaced with a simple userspace program using libusb, as I bet your driver could be replaced with as well, right? When I wrote that original usb skeleton driver, there was still many different types of USB devices out there that we did not support, and so an example driver was one way to help others out. Given that Linux now supports all USB devices and types, it's not so useful as if you need to support a new one, it usually just requires you to modify an existing driver. New USB device types are, thankfully, quite rare these days, with most of the crazy new things being able to be controlled directly from userspace. thanks, greg k-h
On 1/12/22 10:54 AM, Greg KH wrote:
That driver tried to be an example for an unknown device, doing multiple different things that no single driver/device would probably ever need. Also it can almost always just be replaced with a simple userspace program using libusb, as I bet your driver could be replaced with as well, right? Yes it can be replaced by a userspace program but even this is not required. For the USB LCD a userspace program is available from vendor. For the USB to serial Adapter the driver exists in the kernel ch341.c. It appears as a ttyUSB0 device.
I think what happens to very custom drivers that could be userspace programs can be seen in the file /drivers/usb/misc/usblcd.c - It is written in 2005 and is only supporting one device from one vendor. - I cannot find/buy a suitable device. - I have tried two times to contact the author of the driver but: The response from the remote server was: 550 5.1.1 <g.toth@e-biz.lu>: Recipient address rejected: User unknown - The idVendor = 0x10D2 does also not help because RayComposer - R. Adams does not response and does not seam to offer such products. To me it is unused code but how to verify? When it is unused what does the kernel do with it? Putting it to the staging area? Thanks for your time. Bye Philipp
On Thu, Jan 13, 2022 at 07:39:38AM +0100, Philipp Hortmann wrote:
On 1/12/22 10:54 AM, Greg KH wrote:
That driver tried to be an example for an unknown device, doing multiple different things that no single driver/device would probably ever need. Also it can almost always just be replaced with a simple userspace program using libusb, as I bet your driver could be replaced with as well, right? Yes it can be replaced by a userspace program but even this is not required.
It is an issue if you are trying to write a new driver. We do not take new drivers into the kernel tree if they can be in the kernel instead.
For the USB LCD a userspace program is available from vendor.
Great!
For the USB to serial Adapter the driver exists in the kernel ch341.c. It appears as a ttyUSB0 device.
For devices that need to show up to userspace as a common device type (like a serial port), we need to have a kernel driver, so this is to be expected.
I think what happens to very custom drivers that could be userspace programs can be seen in the file /drivers/usb/misc/usblcd.c - It is written in 2005 and is only supporting one device from one vendor. - I cannot find/buy a suitable device. - I have tried two times to contact the author of the driver but: The response from the remote server was: 550 5.1.1 <g.toth@e-biz.lu>: Recipient address rejected: User unknown - The idVendor = 0x10D2 does also not help because RayComposer - R. Adams does not response and does not seam to offer such products.
What is the problem with this driver? If it is not hurting anyone, and still builds, it's not causing any problems.
To me it is unused code but how to verify? When it is unused what does the kernel do with it? Putting it to the staging area?
Why do you want to delete it? We can remove old drivers that no one uses, we do that all the time. But usually that is because the maintance burden on them get too high and we determine that no one actually uses the thing. This driver doesn't seem to be causing any problems that I can see, so why delete it? That being said, if you do think it should be removed, great, let's do so and if someone later shows up that uses it, we can trivially add it back. Just submit a patch to do so. But this doesn't have much to do with your original question, so I'm confused as to what usblcd.c has to do with writing a new USB driver? thanks, greg k-h
On Thu, Jan 13, 2022 at 04:33:50PM +0100, Greg KH wrote:
On Thu, Jan 13, 2022 at 07:39:38AM +0100, Philipp Hortmann wrote:
On 1/12/22 10:54 AM, Greg KH wrote:
That driver tried to be an example for an unknown device, doing multiple different things that no single driver/device would probably ever need. Also it can almost always just be replaced with a simple userspace program using libusb, as I bet your driver could be replaced with as well, right? Yes it can be replaced by a userspace program but even this is not required.
It is an issue if you are trying to write a new driver. We do not take new drivers into the kernel tree if they can be in the kernel instead.
That should be: if they can be written in userspace instead.
On 1/13/22 4:33 PM, Greg KH wrote:
On Thu, Jan 13, 2022 at 07:39:38AM +0100, Philipp Hortmann wrote:
On 1/12/22 10:54 AM, Greg KH wrote:
That driver tried to be an example for an unknown device, doing multiple different things that no single driver/device would probably ever need. Also it can almost always just be replaced with a simple userspace program using libusb, as I bet your driver could be replaced with as well, right? Yes it can be replaced by a userspace program but even this is not required. It is an issue if you are trying to write a new driver. We do not take new drivers into the kernel tree if they can be in the kernel instead.
Sorry I did not clearly express what I plan to do. I plan to do maintenance and bug fixes on the kernel. I am writing on drivers only for my personal education. No new drivers for the kernel. Thanks for your quick and long replies. Bye Philipp
On Thu, Jan 13, 2022 at 07:54:19PM +0100, Philipp Hortmann wrote:
On 1/13/22 4:33 PM, Greg KH wrote:
On Thu, Jan 13, 2022 at 07:39:38AM +0100, Philipp Hortmann wrote:
On 1/12/22 10:54 AM, Greg KH wrote:
That driver tried to be an example for an unknown device, doing multiple different things that no single driver/device would probably ever need. Also it can almost always just be replaced with a simple userspace program using libusb, as I bet your driver could be replaced with as well, right? Yes it can be replaced by a userspace program but even this is not required. It is an issue if you are trying to write a new driver. We do not take new drivers into the kernel tree if they can be in the kernel instead.
Sorry I did not clearly express what I plan to do. I plan to do maintenance and bug fixes on the kernel.
That's wonderful, try starting out in drivers/staging/*/TODO for lots of things that need work. Messing around with old USB drivers that "just work" and you don't have the hardware for, might be a tough thing to work on as a beginning task. No need to touch things that no one is complaining about if you can help it :) thanks, greg k-h
On Thu, Jan 13, 2022 at 08:01:04PM +0100, Greg KH wrote:
On Thu, Jan 13, 2022 at 07:54:19PM +0100, Philipp Hortmann wrote:
On 1/13/22 4:33 PM, Greg KH wrote:
On Thu, Jan 13, 2022 at 07:39:38AM +0100, Philipp Hortmann wrote:
On 1/12/22 10:54 AM, Greg KH wrote:
That driver tried to be an example for an unknown device, doing multiple different things that no single driver/device would probably ever need. Also it can almost always just be replaced with a simple userspace program using libusb, as I bet your driver could be replaced with as well, right? Yes it can be replaced by a userspace program but even this is not required. It is an issue if you are trying to write a new driver. We do not take new drivers into the kernel tree if they can be in the kernel instead.
Sorry I did not clearly express what I plan to do. I plan to do maintenance and bug fixes on the kernel.
That's wonderful, try starting out in drivers/staging/*/TODO for lots of things that need work.
And, there are USB drivers in there as well, if that is what you are interested in working on.
Hi Philipp, On 1/13/22 21:54, Philipp Hortmann wrote:
On 1/13/22 4:33 PM, Greg KH wrote:
On Thu, Jan 13, 2022 at 07:39:38AM +0100, Philipp Hortmann wrote:
On 1/12/22 10:54 AM, Greg KH wrote:
That driver tried to be an example for an unknown device, doing multiple different things that no single driver/device would probably ever need. Also it can almost always just be replaced with a simple userspace program using libusb, as I bet your driver could be replaced with as well, right? Yes it can be replaced by a userspace program but even this is not required. It is an issue if you are trying to write a new driver. We do not take new drivers into the kernel tree if they can be in the kernel instead.
Sorry I did not clearly express what I plan to do. I plan to do maintenance and bug fixes on the kernel.
I am writing on drivers only for my personal education. No new drivers for the kernel.
I think, syzbot dashboard can be a good place to start from [1]. There are a lot of usb related warnings in old drivers. Just grep the web page for 'usb_submit_urb'. Might be valuable work to get familiar with basic kernel APIs. Good luck :) [1] https://syzkaller.appspot.com/upstream With regards, Pavel Skripkin
participants (3)
-
Greg KH -
Pavel Skripkin -
Philipp Hortmann