From owner-freebsd-hackers@freebsd.org Sun Mar 10 15:52:58 2019 Return-Path: Delivered-To: freebsd-hackers@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 7EDF01537C38 for ; Sun, 10 Mar 2019 15:52:58 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from kib.kiev.ua (kib.kiev.ua [IPv6:2001:470:d5e7:1::1]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id CD4C082966 for ; Sun, 10 Mar 2019 15:52:57 +0000 (UTC) (envelope-from kostikbel@gmail.com) Received: from tom.home (kib@localhost [127.0.0.1]) by kib.kiev.ua (8.15.2/8.15.2) with ESMTPS id x2AFqTwZ027727 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Sun, 10 Mar 2019 17:52:32 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua x2AFqTwZ027727 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id x2AFqSM9027726; Sun, 10 Mar 2019 17:52:28 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Sun, 10 Mar 2019 17:52:28 +0200 From: Konstantin Belousov To: Hans Petter Selasky Cc: Warner Losh , FreeBSD Hackers , "O'Connor, Daniel" Subject: Re: USB stack getting confused Message-ID: <20190310155228.GR2492@kib.kiev.ua> References: <20190309152613.GM2492@kib.kiev.ua> <20190309162640.GN2492@kib.kiev.ua> <20190309192330.GO2492@kib.kiev.ua> <20190310094758.GP2492@kib.kiev.ua> <35f69493-4bbb-4142-b61a-3e90adc8777b@selasky.org> <20190310102629.GQ2492@kib.kiev.ua> <40bf77e0-47a5-6edc-b5d0-58e3c44988ac@selasky.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <40bf77e0-47a5-6edc-b5d0-58e3c44988ac@selasky.org> User-Agent: Mutt/1.11.3 (2019-02-01) X-Spam-Status: No, score=-1.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FORGED_GMAIL_RCVD,FREEMAIL_FROM, NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on tom.home X-BeenThere: freebsd-hackers@freebsd.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Technical Discussions relating to FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 10 Mar 2019 15:52:58 -0000 On Sun, Mar 10, 2019 at 02:10:23PM +0100, Hans Petter Selasky wrote: > On 3/10/19 11:26 AM, Konstantin Belousov wrote: > > On Sun, Mar 10, 2019 at 11:18:36AM +0100, Hans Petter Selasky wrote: > >> On 3/10/19 10:47 AM, Konstantin Belousov wrote: > >>>> Yes, I can do that if destroy_dev() ensures that d_close is called for > >>>> all open file handles once and only once before it returns. I think this > >>>> is where the problem comes from. > >>> See above. For d_close it is impossible, for cdevpriv dtr it is already > >>> there by design. > >>> > >> > >> Yes, cdevpriv_dtr will wait for the final close() from user-space > >> unfortunately. Or am I mistaken? > > > > You are mistaken. Cdevpriv destructors are called either on the file close > > (not the last close in d_close sense, just file close) OR during destroy_dev(). > > Each destructor/file pair is called exactly once, regardless of the cause. > > > > Can you try the attached patch? If you are asking me, then my testing would take too much time and I am not even sure when to declare success. As I noted, apcupsd closes the descriptor after access, so it is very hard to reproduce the problem. OTOH Daniel' setup sounds good for testing. > > --HPS > Index: sys/dev/usb/controller/usb_controller.c > =================================================================== > --- sys/dev/usb/controller/usb_controller.c (revision 344575) > +++ sys/dev/usb/controller/usb_controller.c (working copy) > @@ -664,6 +664,33 @@ > USB_BUS_LOCK(bus); > } > } > + > +void > +usb_device_cleanup(struct usb_device *udev) > +{ > + struct usb_fs_privdata *pd; > + struct usb_bus *bus; > + int bus_index; > + int dev_index; > + > + bus = udev->bus; > + bus_index = device_get_unit(bus->bdev); > + dev_index = udev->device_index; > + > +retry: > + USB_BUS_LOCK(bus); > + LIST_FOREACH(pd, &bus->pd_cleanup_list, pd_next) { > + if (pd->bus_index != bus_index || > + pd->dev_index != dev_index) > + continue; > + LIST_REMOVE(pd, pd_next); > + USB_BUS_UNLOCK(bus); > + > + usb_destroy_dev_sync(pd); > + goto retry; > + } > + USB_BUS_UNLOCK(bus); > +} > #endif > > static void > Index: sys/dev/usb/usb_device.c > =================================================================== > --- sys/dev/usb/usb_device.c (revision 344575) > +++ sys/dev/usb/usb_device.c (working copy) > @@ -2322,6 +2322,13 @@ > &udev->cs_msg[0], &udev->cs_msg[1]); > USB_BUS_UNLOCK(udev->bus); > > +#if USB_HAVE_UGEN > + /* > + * Destroy character devices belonging to this > + * device synchronously: > + */ > + usb_device_cleanup(udev); > +#endif > /* wait for all references to go away */ > usb_wait_pending_refs(udev); > > Index: sys/dev/usb/usb_device.h > =================================================================== > --- sys/dev/usb/usb_device.h (revision 344575) > +++ sys/dev/usb/usb_device.h (working copy) > @@ -309,6 +309,7 @@ > usb_error_t usb_suspend_resume(struct usb_device *udev, > uint8_t do_suspend); > void usb_devinfo(struct usb_device *udev, char *dst_ptr, uint16_t dst_len); > +void usb_device_cleanup(struct usb_device *); > void usb_free_device(struct usb_device *, uint8_t); > void usb_linux_free_device(struct usb_device *dev); > uint8_t usb_peer_can_wakeup(struct usb_device *udev);