[#5647] SPI driver bus_unlock() does not schedule postponed SPI messages
Submitted By: Eduardo Tagle
Open Date
2009-10-26 10:37:23 Close Date
2009-11-06 02:33:35
Priority:
Medium Assignee:
Yi Li
Status:
Closed Fixed In Release:
N/A
Found In Release:
2010R1 Release:
2010R1
Category:
Drivers Board:
N/A
Processor:
ALL Silicon Revision:
0.5
Is this bug repeatable?:
Yes Resolution:
Fixed
Uboot version or rev.:
Toolchain version or rev.:
GCC 4.1.1
App binary format:
N/A
Summary: SPI driver bus_unlock() does not schedule postponed SPI messages
Details:
Well, just as the title says, if you bfin_spi_lock_bus(), as done by the mmc_spi driver, then spi_sync() a SPI message, but this time, done by another driver, not the one owning the SPI bus, then the SPI message is queued but not transferred (this is the expected behavior). When the driver owning the SPI bus releases it by using bfin_spi_unlock_bus(), we should expect all pending SPI messages sent by drivers not owning the bus, should be scheduled for inmediate transfer. That exctly what is not happening right now. Those messages stay on the driver queu forever, as the queue is not running, and are not scheduled to be sent, until some other driver sends a new SPI message, that, in turn, will make the queu run and luckily, all pending meesages will be sent.
The function says...
static int bfin_spi_unlock_bus(struct spi_device *spi)
{
int ret = -ENOSYS;
#ifdef CONFIG_SPI_BFIN_LOCK
struct master_data *drv_data = spi_master_get_devdata(spi->master);
struct slave_data *chip = spi_get_ctldata(spi);
unsigned long flags;
int cs = spi->chip_select ? spi->chip_select : chip->cs_gpio;
spin_lock_irqsave(&drv_data->lock, flags);
if (cs == drv_data->locked) {
drv_data->locked = 0;
ret = 0;
} else
ret = -ENOLCK;
spin_unlock_irqrestore(&drv_data->lock, flags);
#endif
return ret;
}
And i propose the following fix... As soon as the bus is unlocked, kick message transfers to complete all pending messages:
static int bfin_spi_unlock_bus(struct spi_device *spi)
{
int ret = -ENOSYS;
#ifdef CONFIG_SPI_BFIN_LOCK
struct master_data *drv_data = spi_master_get_devdata(spi->master);
struct slave_data *chip = spi_get_ctldata(spi);
unsigned long flags;
int cs = spi->chip_select ? spi->chip_select : chip->cs_gpio;
spin_lock_irqsave(&drv_data->lock, flags);
if (cs == drv_data->locked) {
drv_data->locked = 0;
ret = 0;
/* make sure to restart all pending xfers */
if (drv_data->running && !drv_data->busy)
queue_work(drv_data->workqueue, &drv_data->pump_messages);
} else
ret = -ENOLCK;
spin_unlock_irqrestore(&drv_data->lock, flags);
#endif
return ret;
}
And also, i have a question: bfin_spi_lock_bus() sematics are more like a try_lock(), rather than a lock() ... As the function does not wait until the bus is acquired ... You should either really rename that function to something more sensible, such as bfin_spi_trylock_bus(), or implement the full lock() semantics: That should mean wating until the previous owner releases the bus...
Regards,
Eduardo
Follow-ups
--- Yi Li 2009-10-29 06:06:50
Thanks Eduardo. This makes sense to me.
--- Eduardo Tagle 2009-10-31 13:10:00
I filed this bug juat because i have a custom designed board that shares the SPI
bus between an MMC card, and a keyboard/touchscreen driver chip. I have observed
that sometimes, after accessing the MMC card, the keyboard/touchscreen driver
stops working until the MMC is accessed again.
The keyboard/touchscreen driver uses an interrupt driven approach, so there is
no polling of the chip. The Blackfin does simple query status if the chip
signals something has changed from the last time status was read by means of an
interrupt signal.
That's why is was so important to make sure all SPI messages are processed,
because, on my linux driver, i am waiting for a response before sonding another
command to the controller....
And the patch i attached at least has made the SPI bus driver work realiably
for my hw for more than 2 weeks, without it, the system seems to freeze (no
keyboard and no touchscreen appears to the user as a locked system)
--- Yi Li 2009-11-02 22:46:15
Eduardo,
I will check in your patch, with a small change of the comments, adding check
for empty queue:
Index: linux-2.6.x/drivers/spi/spi_bfin5xx.c
===================================================================
--- linux-2.6.x/drivers/spi/spi_bfin5xx.c (revision 7743)
+++ linux-2.6.x/drivers/spi/spi_bfin5xx.c (working copy)
@@ -999,6 +999,11 @@
if (cs == drv_data->locked) {
drv_data->locked = 0;
ret = 0;
+
+ /* kick off transfer of all pending messeages */
+ if (drv_data->running && !drv_data->busy &&
+ !list_empty(&drv_data->queue))
+ queue_work(drv_data->workqueue, &drv_data->pump_messages);
} else
ret = -ENOLCK;
spin_unlock_irqrestore(&drv_data->lock, flags);
Could you please test on you boards?
-Yi
--- Eduardo Tagle 2009-11-03 15:04:20
Yes, the patch is working
--- Yi Li 2009-11-06 02:33:35
OK. close this bug.
Files
Changes
Commits
Dependencies
Duplicates
Associations
Tags
File Name File Type File Size Posted By
No Files Were Found