Date: Sun, 04 Mar 2018 19:23:53 -0700 From: Ian Lepore <ian@freebsd.org> To: Thomas Skibo <thomasskibo@yahoo.com>, freebsd-arm@freebsd.org Subject: Re: Panic in spi driver Message-ID: <1520216633.38056.12.camel@freebsd.org> In-Reply-To: <838BFE5B-61CD-4EC4-BB4F-8124B5B3AF9F@yahoo.com> References: <838BFE5B-61CD-4EC4-BB4F-8124B5B3AF9F@yahoo.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?1520216633.38056.12.camel>