From owner-freebsd-hackers@freebsd.org Sun Mar 10 10:15:54 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 BBCAA152E83B for ; Sun, 10 Mar 2019 10:15:54 +0000 (UTC) (envelope-from hps@selasky.org) Received: from mail.turbocat.net (turbocat.net [88.99.82.50]) (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 4DCCB6F1B0 for ; Sun, 10 Mar 2019 10:15:54 +0000 (UTC) (envelope-from hps@selasky.org) Received: from hps2016.home.selasky.org (unknown [176.74.212.121]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.turbocat.net (Postfix) with ESMTPSA id 087D7260209; Sun, 10 Mar 2019 11:15:51 +0100 (CET) Subject: Re: USB stack getting confused To: Konstantin Belousov Cc: Warner Losh , FreeBSD Hackers , "O'Connor, Daniel" References: <3B29D870-41F9-46AF-B9F3-03106DEC417D@dons.net.au> <20190309152613.GM2492@kib.kiev.ua> <20190309162640.GN2492@kib.kiev.ua> <20190309192330.GO2492@kib.kiev.ua> <20190310094758.GP2492@kib.kiev.ua> From: Hans Petter Selasky Message-ID: <3be2ea6e-f819-90e1-6e8b-56a5ed62d274@selasky.org> Date: Sun, 10 Mar 2019 11:15:28 +0100 User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:60.0) Gecko/20100101 Thunderbird/60.4.0 MIME-Version: 1.0 In-Reply-To: <20190310094758.GP2492@kib.kiev.ua> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Rspamd-Queue-Id: 4DCCB6F1B0 X-Spamd-Bar: ------ Authentication-Results: mx1.freebsd.org X-Spamd-Result: default: False [-6.97 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000,0]; NEURAL_HAM_SHORT(-0.97)[-0.974,0]; REPLY(-4.00)[]; NEURAL_HAM_LONG(-1.00)[-1.000,0] 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 10:15:55 -0000 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