Date: Thu, 9 Mar 2017 03:19:14 +0200 From: Konstantin Belousov <kostikbel@gmail.com> To: Oleksandr Tymoshenko <gonzo@FreeBSD.org> 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> In-Reply-To: <20170309011239.GA77155@bluezbox.com> References: <201703090100.v2910Rjd050904@repo.freebsd.org> <20170309010955.GV30979@kib.kiev.ua> <20170309011239.GA77155@bluezbox.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20170309011914.GW30979>