Date: Sun, 10 Mar 2019 17:52:28 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Hans Petter Selasky <hps@selasky.org> Cc: Warner Losh <imp@bsdimp.com>, FreeBSD Hackers <freebsd-hackers@freebsd.org>, "O'Connor, Daniel" <darius@dons.net.au> Subject: Re: USB stack getting confused Message-ID: <20190310155228.GR2492@kib.kiev.ua> In-Reply-To: <40bf77e0-47a5-6edc-b5d0-58e3c44988ac@selasky.org> References: <20190309152613.GM2492@kib.kiev.ua> <ea6e2690-1ad7-6c06-49e5-c528013f26c0@selasky.org> <20190309162640.GN2492@kib.kiev.ua> <CANCZdfr9jRcXQeZWMPKSMvUB5u7kE0eDvbuKrtGvuUDYOr=n4A@mail.gmail.com> <20190309192330.GO2492@kib.kiev.ua> <fd5038a4-406b-6e4b-bb52-b567b1954ad1@selasky.org> <20190310094758.GP2492@kib.kiev.ua> <35f69493-4bbb-4142-b61a-3e90adc8777b@selasky.org> <20190310102629.GQ2492@kib.kiev.ua> <40bf77e0-47a5-6edc-b5d0-58e3c44988ac@selasky.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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);
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20190310155228.GR2492>