re: CVS commit: src/sys/dev/scsipi

[matthew green <mrg%eterna.com.au@localhost>]

> > > If the driver's attach is called after cold (e.g. after a detach/rescan
> > > of the pci bus), the driver's attach should be called with KERNEL_LOCK
> > > held, or bad things may happen when interrupts are enabled for this 
> > > driver.
> > 
> > there should be no reliance on "cold" being set for normal driver
> > attach.  it might be a module loaded after boot.  how ever the
> > driver is loaded, it will need to work without cold being set.
> 
> If it's a module laoded after boot, and the driver is not SMP-safe,
> its attach function has to be called with KERNEL_LOCK held. Or you may
> have problems when this driver starts receiving interrupts before its
> attach function has returned (which is typically the case, interrupt enable
> occurs somewhere in _attach()).

i don't see how this matters.  drivers should never enable
interrupts if they're not ready to service them.

if this is happening, then it is a driver bug.

> > in my mind, the scsi code should try to run regardless of the value
> > of "cold" and that's why i replied above.
> > 
> > > What kind of senario do you have in mind ?
> > 
> > modules, as above.  or simply drvctl -d / -r.  IMO, only platform
> > specific code really should depend upon cold.  "scsipi", as a
> > relatively high level subsystem, should not.
> 
> But I don't think it's the job of non-SMP-safe drivers to make sure scsipi
> is called with KERNEL_LOCK held (and scsipi may be the only subsystem
> to check for KERNEL_LOCK(), but others do rely on it as well - e.g.
> sys/dev/ata). Either attach() functions should always be called with
> KERNEL_LOCK(), or for checks as above we have to accept that when
> cold, locking is not fully up yet and lock checks should be bypassed.

the lack of checks elsewhere is not the point here.  we should add
them to dev/ata (and more) as well, if they depend upon these.

currently, autoconf isn't run with the kernel lock held at any point
and i don't think that saying that we should hold it during autoconf
is a good change.  we should fix the cases that trigger problems when
they appear, and it sounds like you've hit one in your new driver.
holding the kernel lock here would be a major step backwards.

scsipi depends upon kernel lock.  thus, callers should arrange for it
to be held.  since these are drivers calling, it is upto each driver
to do this currently.  this is what we've done with other problems we
have hit as they've arrived.

it may be that we have this problem in a large number of drivers but
these KASSERT()s have been in place for a long time now and, until
your change, we've fixed it at the caller level not worked around it
at the scsipi level.

your change can't work with the drivers that are broken without the
major regression of holding kernel lock over autoconfig, or, never
ever having them be modules or detached/reattached.

please make "drvctl -d" and rescan work for your drivers.  this will
make the checks against "cold" go away.


.mrg.