Panic in spi driver

[Ian Lepore]

On Sun, 2018-03-04 at 11:38 -0800, Thomas Skibo via freebsd-arm wrote:
> I’m developing a qspi driver for Zynq/Zedboard and running into a
> problem that might affect other spi drivers.  When my driver does its
> attach, it triggers the attach of the flash driver,
> dev/flash/mx25l.c, through spibus.  In turn, the flash driver
> attempts to read the flash device ident by calling back to my
> driver’s transfer function.  My driver initiates the transfer and
> then sleeps on its lock with a timeout (mtx_sleep(…, 2 * hz)).  That
> panics the system: “panic: timed sleep before timers are
> working”.  I’ve attached the stack backtrace.
> 
> I loosely based my driver on bcm2835_spi.c which also sleeps with a
> timeout in its transfer function.  I tried greping through a few
> other spi drivers and noticed dev/intel/spi.c does it too.  
> 
> Okay, before I hit send, I noticed that the other flash driver,
> at45d.c, uses some hook to do a delayed attach with the comment
> “We’ll see what kind of flash we have later…”.  Maybe mx25l.c needs
> something like this.
> 
> My other idea is to create a “fast transfer” function that doesn’t
> use interrupts to be used for trivial transfers.  That would take
> care of the IDENT and READ_STATUS commands that happen in the flash
> driver’s attach routine.  It might be a nice optimization too.
> 
> —Thomas

Ooops, I just realized I had the same problem in the new imx6 spi
driver I committed a couple days ago.  I did all my testing using
loadable modules, forgot to ever test compiling everything in.  As soon
as I did, panic.

So in the i2c world this problem was historically fixed by every
individual slave device driver using a config_intrhook_establish() in
its attach routine to do the real attach work later, after interrupts
are working.  We should avoid the same mistake in the spi world.

The way I see it, if a driver that performs IO for its children
requires interrupts to function, it should not attach the child devices
until interrupts are working.  In other words, put the config_intrhook
in the single driver that really needs it, not in every child driver.
If there's a situation where the spi driver has to do IO before
interrupts can be enabled (maybe to make a PMIC or other hardware-
control device work early in boot), then it has to have a transfer
implementation that uses some kind of polling loop until interrupts are
available (basically, have two implementations of transfer()).

I just committed the imx_spi fix in r330438.  The basic fix is instead
of ending attach() with 

  return (bus_generic_attach(sc->dev));

do 

  config_intrhook_oneshot((ich_func_t)bus_generic_attach, dev);
  return (0);

-- Ian