From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from shelob.surriel.com (shelob.surriel.com [96.67.55.147]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id AA1D5C433EF for ; Tue, 11 Jan 2022 20:10:19 +0000 (UTC) Received: from localhost ([::1] helo=shelob.surriel.com) by shelob.surriel.com with esmtp (Exim 4.94.2) (envelope-from ) id 1n7NSc-0002MX-BJ; Tue, 11 Jan 2022 15:09:38 -0500 Received: from mail-wm1-x32e.google.com ([2a00:1450:4864:20::32e]) by shelob.surriel.com with esmtps (TLS1.2) tls TLS_ECDHE_ECDSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1n7NSa-0002MQ-5O for kernelnewbies@kernelnewbies.org; Tue, 11 Jan 2022 15:09:36 -0500 Received: by mail-wm1-x32e.google.com with SMTP id c66so103209wma.5 for ; Tue, 11 Jan 2022 12:09:35 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=subject:to:cc:references:from:message-id:date:user-agent :mime-version:in-reply-to:content-language:content-transfer-encoding; bh=i0zR35YSX3ac2Ck5nfB7h8d2qSknnfrr5qgI9sZQe5Q=; b=ajty5k1eBPhDAp6S5DbdItM/lG90FpRkqgE0np4PLdW4OinxwSgHC7bqEFjm/nX4As HpvKigK4m59jcgGZNYHP353rhx6t5I1vLdvxK7tNlO0nfo7dDrGvFbaB+qiEiScrCieF Z2dN9x517Bzy6cLslsCSrYAQjVtgezMpi9KBWpzBaow5r844uCtGGqcLOqgbSqmvDpsA xeqB4jgxzhY6bwArdZOCuQeYF/UY8z70cJy+WLjaQYRZS2nkA3VMiHaXjMzOnQPYHVp1 ZThvTThNRJOSfXTG8UTnJ1vT5A/zhO6C/K8dYSjdpDYtJDWXrL0fCtVW2iCkcSGVPJAC ezwg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=i0zR35YSX3ac2Ck5nfB7h8d2qSknnfrr5qgI9sZQe5Q=; b=F44k345IHNm0gjEPFK5CfgJuXEySaAkaBSr0d0VYLBR88RtkH3e3+okHLEKu+n974k XG3yBTQdYxvuM+PkWlWqIVbODgMXmYEgz7aCA4JzI3tyDAgTYEMbj5AQ3E+YePLCIeYF 7k9ax/enjNo21pvBm2DYXxx9x0YuGhxzlLXkd1mZZOC8XEMdaEM2oGPrvf04JTOihmyp MmW7Li0kzergo3BdgI2IE5sS1TVchkktDo5gL/nh1LPnccbxgCcJhAe4tpiVi+dv87Hp n4scXHYW0gBKGQ8431zFFQKsaZ62EiMPtg0lbrw1PfJsdjn9+bWAYw3EQkRtddUgFSIY 5/WQ== X-Gm-Message-State: AOAM532G94+w+GyvHzFXDGNlgONebQHZZJDNs7grKDuGyTseoTaa1gGe g6XdRZ6wQ8GrPHzDh8Vx3fRCLkabws8= X-Google-Smtp-Source: ABdhPJzzFhAUdfGpAAOVeessZ6zN0jscETOvRuHZUKXl/M0wDx37JQaWXILmMQxWHMWJ19aN0pwodw== X-Received: by 2002:a1c:f01a:: with SMTP id a26mr3848571wmb.175.1641931772747; Tue, 11 Jan 2022 12:09:32 -0800 (PST) Received: from ?IPv6:2003:c7:8f4e:684:a668:64b3:a390:255a? (p200300c78f4e0684a66864b3a390255a.dip0.t-ipconnect.de. [2003:c7:8f4e:684:a668:64b3:a390:255a]) by smtp.gmail.com with ESMTPSA id p62sm2456313wmp.10.2022.01.11.12.09.31 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 11 Jan 2022 12:09:32 -0800 (PST) Subject: Re: Where to find the information how to write a state of the art USB driver? To: Greg KH References: From: Philipp Hortmann Message-ID: <28e1c700-da54-8793-8c60-4ec3697fe35d@gmail.com> Date: Tue, 11 Jan 2022 21:09:31 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101 Thunderbird/78.14.0 MIME-Version: 1.0 In-Reply-To: Content-Language: en-US Cc: kernelnewbies@kernelnewbies.org X-BeenThere: kernelnewbies@kernelnewbies.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: Learn about the Linux kernel List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: kernelnewbies-bounces@kernelnewbies.org 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); > _______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org https://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies