USB stack getting confused

[Hans Petter Selasky]

On 3/10/19 10:47 AM, Konstantin Belousov wrote:
> On Sat, Mar 09, 2019 at 10:35:28PM +0100, Hans Petter Selasky wrote:
>> On 3/9/19 8:23 PM, Konstantin Belousov wrote:
>>> On Sat, Mar 09, 2019 at 11:41:31AM -0700, Warner Losh wrote:
>>>>
>>>> Is there a form of destroy_dev() that does a revoke on all open instances?
>>>> Eg, this is gone, you can't use it anymore, and all further attempts to use
>>>> the device will generate an error, but in the mean time we destroy the
>>>> device and let the detach routine get on with life. waiting may make sense
>>>> when you are merely unloading the driver (and getting to the detach routine
>>>> that way), but when the device is gone, I've come around to the point of
>>>> view that we should just destroy it w/o waiting for closes and anybody that
>>>> touches it afterwards gets an error and has to cope with the error. But
>>>> even in the unload case, we maybe we shouldn't get to the detach routine
>>>> unless we're forcing and/or the detach routine just returns EBUSY since the
>>>> only one that knows what dev_t's are associated with the device_t is the
>>>> driver itself.
>>> You are asking very basic questions about devfs there.
>>>
>>> destroy_dev(9) waits for two things:
>>> - that all threads left the cdevsw methods for the given device;
>>> - that all cdevpriv destructors finished running.
>>
>> Hi,
>>
>>> To facilitate waking up threads potentially sleeping inside the cdevsw
>>> methods, drivers might implement d_purge method which must weed out sleeping
>>> threads from inside the code in the bound time.
>>
>> USB will purge all callers before calling destroy_dev(). This is not the
>> problem.
>>
>>> After that we return from destroy_dev(9) and guarantee that no new calls
>>> into cdevsw is done for this device.  devfs magic consumes  the fo_ and
>>> VOP_ calls and does not allow them to reach into the driver.
>>
>> When I designed the current USB devfs it was important to me to keep
>> open() and close() calls balanced to avoid situations where an open call
>> may setup some resource and then close(), which free this resource
>> again, never gets called. destroy_dev(9) makes no such guarantee, and I
>> think that is a failure of destroy_dev(9). That's when I started using
>> the cdev's destructor callback function.
> Lets correct the terminology first.
> Are you referring to the d_open/d_close pairing ?
> 
> Without D_TRACKCLOSE, d_close() is only called on the last close of
> the device.  With D_TRACKCLOSE, devfs _tries_ to call d_close each time
> it sees the VOP_CLOSE() operation from VFS, but due to way VFS works
> VOP_CLOSE() could be missed.  Also, d_open vs d_close are not synchronized,
> so a driver might get call to d_open in parallel to last d_close.

Hi,

I'm using D_TRACKCLOSE.

> 
> What do you mean by cdev destructor callback function ?  Do you mean
> callback from destroy_dev_cb(), or do you actually reference the
> destructors from devfs_set_cdevpriv(9) ?

Yes, I mean the use of devfs_set_cdevpriv(9).

> If the later, then destroy_dev() guarantees that all cdevpriv destructors
> for all file descriptors opened against the destroyed cdev are finished
> before destroy_dev() returns.  In other words, if you use cdevpriv, you
> can remove the drain for your refcount and everything should just work.

Using devfs_set_cdevpriv(9), means destroy_dev() will be called after 
the last close() on the file descriptor in question. Is this strictly 
needed? You see this in the code that devfs_close_f() calls devfs_fpdrop().

> 
>>
>>> So what usb does there is actively defeating existing mechanism by
>>> keeping internal refcount on opens and refusing to call destroy_dev()
>>> until the count goes to zero
>>
>> The FreeBSD USB stack also is used in environments w/o DEVFS and need
>> own refcounts.

 >
> I completely disagree with use of code sharing as excuse for FreeBSD bugs.
 >

Like said, using devfs_set_cdevpriv(9), which the USB stack needs, 
basically means we are waiting for the final user-space close() or 
"struct file" refcount drop. This behaviour is not like announced. I 
would like to have the cdevpriv's destructor executed before 
destroy_dev() returns.

Again, the USB stack needs paired operation. An open() call must always 
be followed by a close() call on the same FD. Else memory resources will 
leak simply.

--HPS