svn commit: r314933 - head/sys/dev/spibus

[Oleksandr Tymoshenko]

Konstantin Belousov (kostikbel at 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