Date: Sat, 02 Jun 2007 20:53:18 +0200 From: Attilio Rao <attilio@FreeBSD.org> To: Rui Paulo <rpaulo@FreeBSD.org> Cc: Perforce Change Reviews <perforce@FreeBSD.org> Subject: Re: PERFORCE change 120788 for review Message-ID: <4661BC9E.9070603@FreeBSD.org> In-Reply-To: <200706021756.l52Huq9A049371@repoman.freebsd.org> References: <200706021756.l52Huq9A049371@repoman.freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
Rui Paulo wrote: > http://perforce.freebsd.org/chv.cgi?CH=120788 > > Change 120788 by rpaulo@rpaulo_epsilon on 2007/06/02 17:55:58 > > Add locking. > > Affected files ... > > .. //depot/projects/soc2007/rpaulo-macbook/dev/asmc/asmc.c#8 edit > .. //depot/projects/soc2007/rpaulo-macbook/dev/asmc/asmcvar.h#4 edit > > Differences ... > > ==== //depot/projects/soc2007/rpaulo-macbook/dev/asmc/asmc.c#8 (text+ko) ==== > > @@ -23,7 +23,7 @@ > * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE > * POSSIBILITY OF SUCH DAMAGE. > * > - * $P4: //depot/projects/soc2007/rpaulo-macbook/dev/asmc/asmc.c#7 $ > + * $P4: //depot/projects/soc2007/rpaulo-macbook/dev/asmc/asmc.c#8 $ > * > */ > > @@ -46,7 +46,8 @@ > #include <sys/conf.h> > #include <sys/kernel.h> > #include <sys/sysctl.h> > -#include <sys/sbuf.h> > +#include <sys/lock.h> > +#include <sys/mutex.h> > > #include <isa/isavar.h> You should mantain includes sorted by name (where possible). > @@ -239,6 +240,8 @@ > > model = asmc_match(dev); > > + mtx_init(&sc->sc_mtx, "asmc_mtx", NULL, MTX_SPIN); > + > asmc_init(dev); You don't need to write again 'mtx' in the description. Just add something descriptive for the mutexes (since you alredy know it is a spin mutexes). > @@ -740,6 +756,9 @@ > { > uint8_t type; > device_t dev = (device_t) arg; > + struct asmc_softc *sc = device_get_softc(dev); > + > + mtx_lock_spin(&sc->sc_mtx); > > type = inb(ASMC_INTPORT); > > @@ -757,6 +776,8 @@ > device_printf(dev, "%s unknown interrupt\n", __func__); > } > > + mtx_unlock_spin(&sc->sc_mtx); I'm not sure you really need to maintain the lock for so long. Probabilly, you just need for the duration of inb(). > ==== //depot/projects/soc2007/rpaulo-macbook/dev/asmc/asmcvar.h#4 (text+ko) ==== > > @@ -23,7 +23,7 @@ > * ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE > * POSSIBILITY OF SUCH DAMAGE. > * > - * $P4: //depot/projects/soc2007/rpaulo-macbook/dev/asmc/asmcvar.h#3 $ > + * $P4: //depot/projects/soc2007/rpaulo-macbook/dev/asmc/asmcvar.h#4 $ > * > */ > > @@ -49,6 +49,8 @@ > int sc_rid; > struct resource *sc_res; > void *sc_cookie; > + > + struct mtx sc_mtx; > }; The extra white-line is not needed and since mutexes are not so light, it would be better to put it on the top of the structure. Attilio
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?4661BC9E.9070603>