USB stack getting confused

[Konstantin Belousov]

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);