From owner-svn-src-head@freebsd.org Thu Mar 9 01:12:41 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 9EF0AD03C64; Thu, 9 Mar 2017 01:12:41 +0000 (UTC) (envelope-from gonzo@FreeBSD.org) Received: from id.bluezbox.com (id.bluezbox.com [45.55.20.155]) (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 744EDF78; Thu, 9 Mar 2017 01:12:41 +0000 (UTC) (envelope-from gonzo@FreeBSD.org) Received: from [127.0.0.1] (helo=id.bluezbox.com) by id.bluezbox.com with esmtps (TLSv1.2:ECDHE-RSA-AES256-GCM-SHA384:256) (Exim 4.87 (FreeBSD)) (envelope-from ) id 1clmdE-000K54-3y; Wed, 08 Mar 2017 17:12:40 -0800 Received: (from gonzo@localhost) by id.bluezbox.com (8.15.2/8.15.2/Submit) id v291CdtO077193; Wed, 8 Mar 2017 17:12:39 -0800 (PST) (envelope-from gonzo@FreeBSD.org) X-Authentication-Warning: id.bluezbox.com: gonzo set sender to gonzo@FreeBSD.org using -f Date: Wed, 8 Mar 2017 17:12:39 -0800 From: Oleksandr Tymoshenko To: Konstantin Belousov 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: <20170309011239.GA77155@bluezbox.com> References: <201703090100.v2910Rjd050904@repo.freebsd.org> <20170309010955.GV30979@kib.kiev.ua> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20170309010955.GV30979@kib.kiev.ua> X-Operating-System: FreeBSD/11.0-RELEASE-p2 (amd64) User-Agent: Mutt/1.6.1 (2016-04-27) X-Spam-Level: -- X-Spam-Report: Spam detection software, running on the system "id.bluezbox.com", has NOT identified this incoming email as spam. The original message has been attached to this so you can view it or label similar future email. If you have any questions, see The administrator of that system for details. Content preview: 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; > > > > [...] Content analysis details: (-2.9 points, 5.0 required) pts rule name description ---- ---------------------- -------------------------------------------------- -1.0 ALL_TRUSTED Passed through trusted hosts only via SMTP -1.9 BAYES_00 BODY: Bayes spam probability is 0 to 1% [score: 0.0000] 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:12:41 -0000 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. -- gonzo