[#5647] SPI driver bus_unlock() does not schedule postponed SPI messages

Document created by Aaronwu Employee on Sep 5, 2013
Version 1Show Document
  • View in full screen mode

[#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

Attachments

    Outcomes