From owner-svn-src-head@freebsd.org Thu Mar 9 01:19:20 2017 Return-Path: Delivered-To: svn-src-head@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 7F668D0321B; Thu, 9 Mar 2017 01:19:20 +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 2503C1348; Thu, 9 Mar 2017 01:19:19 +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 v291JEop051487 (version=TLSv1.2 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=NO); Thu, 9 Mar 2017 03:19:14 +0200 (EET) (envelope-from kostikbel@gmail.com) DKIM-Filter: OpenDKIM Filter v2.10.3 kib.kiev.ua v291JEop051487 Received: (from kostik@localhost) by tom.home (8.15.2/8.15.2/Submit) id v291JEuG051486; Thu, 9 Mar 2017 03:19:14 +0200 (EET) (envelope-from kostikbel@gmail.com) X-Authentication-Warning: tom.home: kostik set sender to kostikbel@gmail.com using -f Date: Thu, 9 Mar 2017 03:19:14 +0200 From: Konstantin Belousov To: Oleksandr Tymoshenko Cc: src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r314933 - head/sys/dev/spibus Message-ID: <20170309011914.GW30979@kib.kiev.ua> References: <201703090100.v2910Rjd050904@repo.freebsd.org> <20170309010955.GV30979@kib.kiev.ua> <20170309011239.GA77155@bluezbox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170309011239.GA77155@bluezbox.com> User-Agent: Mutt/1.8.0 (2017-02-23) X-Spam-Status: No, score=-2.0 required=5.0 tests=ALL_TRUSTED,BAYES_00, DKIM_ADSP_CUSTOM_MED,FREEMAIL_FROM,NML_ADSP_CUSTOM_MED autolearn=no autolearn_force=no version=3.4.1 X-Spam-Checker-Version: SpamAssassin 3.4.1 (2015-04-28) on tom.home X-BeenThere: svn-src-head@freebsd.org X-Mailman-Version: 2.1.23 Precedence: list List-Id: SVN commit messages for the src tree for head/-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Mar 2017 01:19:20 -0000 On Wed, Mar 08, 2017 at 05:12:39PM -0800, Oleksandr Tymoshenko wrote: > Konstantin Belousov (kostikbel@gmail.com) wrote: > > On Thu, Mar 09, 2017 at 01:00:27AM +0000, Oleksandr Tymoshenko wrote: > > > Author: gonzo > > > Date: Thu Mar 9 01:00:27 2017 > > > New Revision: 314933 > > > URL: https://svnweb.freebsd.org/changeset/base/314933 > > > > > > Log: > > > [spigen] make spigen device ready to be compiled as a module > > > > > > - Add flag to indicate that device is opened by userland > > > - Replace "always fail" detach method with proper detach implementation > > > > > > MFC after: 1 week > > > > > > Modified: > > > head/sys/dev/spibus/spigen.c > > > > > > Modified: head/sys/dev/spibus/spigen.c > > > ============================================================================== > > > --- head/sys/dev/spibus/spigen.c Thu Mar 9 00:58:21 2017 (r314932) > > > +++ head/sys/dev/spibus/spigen.c Thu Mar 9 01:00:27 2017 (r314933) > > > @@ -53,6 +53,9 @@ __FBSDID("$FreeBSD$"); > > > > > > #include "spibus_if.h" > > > > > > +#define SPIGEN_OPEN (1 << 0) > > > +#define SPIGEN_MMAP_BUSY (1 << 1) > > > + > > > struct spigen_softc { > > > device_t sc_dev; > > > struct cdev *sc_cdev; > > > @@ -63,8 +66,8 @@ struct spigen_softc { > > > vm_object_t sc_mmap_buffer; /* command, then data */ > > > vm_offset_t sc_mmap_kvaddr; > > > size_t sc_mmap_buffer_size; > > > - int sc_mmap_busy; > > > int sc_debug; > > > + int sc_flags; > > > }; > > > > > > #ifdef FDT > > > @@ -191,10 +194,24 @@ spigen_attach(device_t dev) > > > } > > > > > > static int > > > -spigen_open(struct cdev *dev, int oflags, int devtype, struct thread *td) > > > +spigen_open(struct cdev *cdev, int oflags, int devtype, struct thread *td) > > > { > > > + int error; > > > + device_t dev; > > > + struct spigen_softc *sc; > > > > > > - return (0); > > > + error = 0; > > > + dev = cdev->si_drv1; > > > + sc = device_get_softc(dev); > > > + > > > + mtx_lock(&sc->sc_mtx); > > > + if (sc->sc_flags & SPIGEN_OPEN) > > > + error = EBUSY; > > > + else > > > + sc->sc_flags |= SPIGEN_OPEN; > > > + mtx_unlock(&sc->sc_mtx); > > > + > > > + return (error); > > > } > > > > > > static int > > > @@ -264,7 +281,7 @@ spigen_transfer_mmapped(struct cdev *cde > > > int error = 0; > > > > > > mtx_lock(&sc->sc_mtx); > > > - if (sc->sc_mmap_busy) > > > + if (sc->sc_flags & SPIGEN_MMAP_BUSY) > > > error = EBUSY; > > > else if (stm->stm_command_length > sc->sc_command_length_max || > > > stm->stm_data_length > sc->sc_data_length_max) > > > @@ -275,7 +292,7 @@ spigen_transfer_mmapped(struct cdev *cde > > > stm->stm_command_length + stm->stm_data_length) > > > error = ENOMEM; > > > if (error == 0) > > > - sc->sc_mmap_busy = 1; > > > + sc->sc_flags |= SPIGEN_MMAP_BUSY; > > > mtx_unlock(&sc->sc_mtx); > > > if (error) > > > return (error); > > > @@ -288,8 +305,8 @@ spigen_transfer_mmapped(struct cdev *cde > > > error = SPIBUS_TRANSFER(device_get_parent(dev), dev, &transfer); > > > > > > mtx_lock(&sc->sc_mtx); > > > - KASSERT(sc->sc_mmap_busy, ("mmap no longer marked busy")); > > > - sc->sc_mmap_busy = 0; > > > + KASSERT((sc->sc_flags & SPIGEN_MMAP_BUSY), ("mmap no longer marked busy")); > > > + sc->sc_flags &= ~(SPIGEN_MMAP_BUSY); > > > mtx_unlock(&sc->sc_mtx); > > > return (error); > > > } > > > @@ -391,6 +408,7 @@ spigen_close(struct cdev *cdev, int ffla > > > sc->sc_mmap_buffer = NULL; > > > sc->sc_mmap_buffer_size = 0; > > > } > > > + sc->sc_flags &= ~(SPIGEN_OPEN); > > > mtx_unlock(&sc->sc_mtx); > > > return (0); > > > } > > > @@ -398,8 +416,23 @@ spigen_close(struct cdev *cdev, int ffla > > > static int > > > spigen_detach(device_t dev) > > > { > > > + struct spigen_softc *sc; > > > + > > > + sc = device_get_softc(dev); > > > + > > > + mtx_lock(&sc->sc_mtx); > > > + if (sc->sc_flags & SPIGEN_OPEN) { > > > + mtx_unlock(&sc->sc_mtx); > > > + return (EBUSY); > > > + } > > > + mtx_unlock(&sc->sc_mtx); > > This does not really work, since another thread might have already > > started executing open, but did not executed enough to set the flag. The > > destroy_dev(9) KPI guarantees that there is no other threads calling > > into devsw methods when the device is teared down. If you need to do an > > additional cleanup, do it after the call to destroy_dev(). > > > > BTW, since si_drv1 + open are used, you might close the usual race with > > the make_dev_s(9) KPI. > > Thanks for the pointers, I'll clean up this code in next iteration. I also did a quick look at the spigen_mmap_single(). The function locks a mutex, then it: - allocates a new vm object - mallocs array for vm_page's - does vm_page_grab() under the mutex. Note that all listed calls might sleep. You would need to allocate and configure complete set of resources, then lock the mutex and assign sc members if not raced.