Spinlock vs mutexes for spi network driver
I have modified 'drivers/net/ethoc.c' for spi communication and it is able to transmit/receive packets. However, I am confused about using spinlocks vs mutexes for locking access to the spi device.
e.g. For the transmit function I use a work_queue to schedule the transmits. In the handler function, I use a mutex to lock the device. Could I be using a spinlock here instead? Should I use a spinlock to disable the irq while I'm in the middle of transmitting data?
The ethoc device sends out an interrupt everytime a packet is successfully transmitted and received. So it has to be acked otherwise the interrupt line stays high and no transmit or receive can happen (which why I'm not use if I should use the spin_lock_irq variant in this case).
Some other spi net drivers such as ks8851.c have mutexes around all spi device accesses, which I guess is to prevent simultaneous uses of the device. Is this a good idea? Doesn't the spi_transfer calls schedule device accesses already?
Thanks for any comment on this.
Amit
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
A message that is not in the archive (sent off-list or not yet received): 20100317204915.GB6358-QCuvCd35e3/[email protected]
Quoted 16 lines · Amit Uttamchandani
I have modified '
drivers/net/ethoc.c' for spi communication and it is able to transmit/receive packets. However, I am confused about using spinlocks vs mutexes for locking access to the spi device.e.g. For the transmit function I use a
work_queueto schedule the transmits. In the handler function, I use a mutex to lock the device. Could I be using a spinlock here instead? Should I use a spinlock to disable the irq while I'm in the middle of transmitting data?The ethoc device sends out an interrupt everytime a packet is successfully transmitted and received. So it has to be acked otherwise the interrupt line stays high and no transmit or receive can happen (which why I'm not use if I should use the
spin_lock_irqvariant in this case).
Quoted 4 lines
Some other spi net drivers such as
ks8851.chave mutexes around all spi device accesses, which I guess is to prevent simultaneous uses of the device. Is this a good idea? Doesn't thespi_transfercalls schedule device accesses already?
Mutexes allow the process to sleep if the locked resource is held by another process. For that reason, they are not permitted in interrupt context. You can read all about both types of locking in "Linux Device Drivers":
That link is to the third edition. I see the fourth edition may be out.
I expect that the fundamentals have not changed.
If I recall correctly, the work queue does NOT run in interrupt context (are allowed to sleep), and therefore mutexs are permitted (for locking with other non-interrupt activity). The interrupt handler definitely runs in interrupt context. If the locking protects data that is shared between interrupt context and non-interrupt context, then it will have to be done with a spinlock. pxa2xx_spi.c (the driver I am familiar with) does not use any mutexes, because the protected data structure is used in the interrupt handlers.
It has been a while since I have dealt with this stuff, so I have said something wrong, above, I'm sure I will be quickly corrected.
Signature · 16 lines
Ned Forrester nforrester-/[email protected] Oceanographic Systems Lab 508-289-2226 Applied Ocean Physics and Engineering Dept. Woods Hole Oceanographic Institution Woods Hole, MA 02543, USA http://www.whoi.edu/ http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212 http://www.whoi.edu/hpb/Site.do?id=1532 http://www.whoi.edu/page.do?pid=10079 ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev
A message that is not in the archive (sent off-list or not yet received): 4BA14970.3050603-/[email protected]
On Wed, Mar 17, 2010 at 05:28:16PM -0400, Ned Forrester wrote:
[...]
Quoted 9 lines
If I recall correctly, the work queue does NOT run in interrupt context (are allowed to sleep), and therefore mutexs are permitted (for locking with other non-interrupt activity). The interrupt handler definitely runs in interrupt context. If the locking protects data that is shared between interrupt context and non-interrupt context, then it will have to be done with a spinlock.
pxa2xx_spi.c(the driver I am familiar with) does not use any mutexes, because the protected data structure is used in the interrupt handlers.
Thanks for the explanation. It fixed a few issues I was having.
Regarding mutexes and spi reads and transfers. Is it possible for multiple SPI reads to be 'overwriting' each other thus resulting in wrong reads? Which is why mutexes should be used to lock SPI bus? I understand there is a patch waiting to be included to the mainline that has functions to lock the bus before doing a transfer.
e.g. You issue a read and while that read is happening, a second spi read call from another function uses the bus and thus results in wrong data being returned to the first read.
Quoted 3 lines
It has been a while since I have dealt with this stuff, so I have said something wrong, above, I'm sure I will be quickly corrected.
Thanks again.
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
A message that is not in the archive (sent off-list or not yet received): 20100318164641.GA22298-QCuvCd35e3/[email protected]
Quoted 22 lines · Amit Uttamchandani
On Wed, Mar 17, 2010 at 05:28:16PM -0400, Ned Forrester wrote:
[...]
Quoted 9 lines
If I recall correctly, the work queue does NOT run in interrupt context (are allowed to sleep), and therefore mutexs are permitted (for locking with other non-interrupt activity). The interrupt handler definitely runs in interrupt context. If the locking protects data that is shared between interrupt context and non-interrupt context, then it will have to be done with a spinlock.
pxa2xx_spi.c(the driver I am familiar with) does not use any mutexes, because the protected data structure is used in the interrupt handlers.Thanks for the explanation. It fixed a few issues I was having.
Regarding mutexes and spi reads and transfers. Is it possible for multiple SPI reads to be 'overwriting' each other thus resulting in wrong reads? Which is why mutexes should be used to lock SPI bus? I understand there is a patch waiting to be included to the mainline that has functions to lock the bus before doing a transfer.
Quoted 3 lines
e.g. You issue a read and while that read is happening, a second spi read call from another function uses the bus and thus results in wrong data being returned to the first read.
The model of SPI transactions this is embodied in the SPI core:
drivers/spi/spi.c,
include/linux/spi/spi.h
defines SPI "messages", which contain any number of "transfers". Messages are queued by a protocol driver (one for each type of chip or device attached to a physical bus), and are executed by the controller driver (one for each physical bus). The controller driver (also sometimes referred to as the master driver) manipulates the physical bus to do all the reading and writing of all the devices/chips attached to the bus.
The message is atomic in the sense that the controller driver guarantees to complete all the transfers defined in the message (assuming no errors) before it handles the next message. Each transfer may change the clock or bits/word, and may also ask for the chip select to manipulated (or not) at the end of the transfer. There is no guarantee that the next message processed by the controller driver will be for the same device as the previous message; the controller driver just takes the next message in the queue, which could have come from any of the protocol drivers that is operating devices on the bus. Only when there is a single device on the bus is the next device certain to be the same as the last device.
Normally, there is no locking used with SPI bus messages (except for spinlocks within the controller driver and its interrupt routines). Each protocol driver sends messages that accomplish complete tasks, after which the chip select to the device can be de-asserted.
Apparently, there is an issue with certain types of memory cards (SD, MMC?), that prevents these cards from being used on a multi-device bus within the present transaction model of the SPI core. I am not familiar with the details, but I gather that the chip select must stay asserted between messages, and the needed operations cannot be combined into a single message (probably because the next action depends on the results of previous actions). These devices are traditionally accommodated by making them the only device on the bus, which is somewhat inconvenient.
To get around this problem there have been recent proposals to
introduce some form of locking so that the protocol driver for a memory device can be assured of exclusive access to the bus for a short period of time. I suspect that this is patch set you are referring to.
In general, the design of the SPI core and controller drivers prevents the type of collision that you describe above. The controller driver can only respond to one message at a time, and will complete that message before taking the next in the queue. Only if you need to guarantee an un-interrupted sequence of messages, such as seems to be necessary for those memories, would locking be necessary. The bus can only be locked in a cooperative or centralized way; there is no way to prevent another protocol driver from inserting a message in the controller driver's queue, unless all drivers have been programmed to respect the locks, or unless the queue mechanism is altered to pass messages to the controller driver only from a protocol driver that is holding the lock, and to put all messages from other, non lock-aware, drivers in a separate holding queue. I think the latter is the method implemented by the proposed locking patches, but I'm not sure (I haven't been paying a lot of attention to that).
I'm assuming that you are using the SPI-core/protocol driver/controller driver model in the system that you have having trouble with. If you are somehow attaching two controller drivers to the same bus, there there is only trouble to be had. I hope the kernel prevents assignment of the same hardware resources to two drivers.
Signature · 16 lines
Ned Forrester nforrester-/[email protected] Oceanographic Systems Lab 508-289-2226 Applied Ocean Physics and Engineering Dept. Woods Hole Oceanographic Institution Woods Hole, MA 02543, USA http://www.whoi.edu/ http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212 http://www.whoi.edu/hpb/Site.do?id=1532 http://www.whoi.edu/page.do?pid=10079 ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev
A message that is not in the archive (sent off-list or not yet received): 4BA262B1.5050001-/[email protected]
On Thu, Mar 18, 2010 at 01:28:17PM -0400, Ned Forrester wrote:
[...]
Quoted 12 lines
The message is atomic in the sense that the controller driver guarantees to complete all the transfers defined in the message (assuming no errors) before it handles the next message. Each transfer may change the clock or bits/word, and may also ask for the chip select to manipulated (or not) at the end of the transfer. There is no guarantee that the next message processed by the controller driver will be for the same device as the previous message; the controller driver just takes the next message in the queue, which could have come from any of the protocol drivers that is operating devices on the bus. Only when there is a single device on the bus is the next device certain to be the same as the last device.
That's good to know that it is queuing the messages. I am assuming that this goes for all messages going into the controller right? Not just for one specific spi message? (e.g. different spi messages from different function calls in the same driver).
Quoted 6 lines
Normally, there is no locking used with SPI bus messages (except for spinlocks within the controller driver and its interrupt routines). Each protocol driver sends messages that accomplish complete tasks, after which the chip select to the device can be de-asserted.
Quoted 13 lines
Apparently, there is an issue with certain types of memory cards (SD, MMC?), that prevents these cards from being used on a multi-device bus within the present transaction model of the SPI core. I am not familiar with the details, but I gather that the chip select must stay asserted between messages, and the needed operations cannot be combined into a single message (probably because the next action depends on the results of previous actions). These devices are traditionally accommodated by making them the only device on the bus, which is somewhat inconvenient.
To get around this problem there have been recent proposals to
introduce some form of locking so that the protocol driver for a memory device can be assured of exclusive access to the bus for a short period of time. I suspect that this is patch set you are referring to.
Yes, I think those are the ones I was talking about.
Quoted 7 lines
In general, the design of the SPI core and controller drivers prevents the type of collision that you describe above. The controller driver can only respond to one message at a time, and will complete that message before taking the next in the queue. Only if you need to guarantee an un-interrupted sequence of messages, such as seems to be necessary for those memories, would locking be necessary. The bus can only be locked in a cooperative or centralized way; there is no way to
Quoted 9 lines
prevent another protocol driver from inserting a message in the controller driver's queue, unless all drivers have been programmed to respect the locks, or unless the queue mechanism is altered to pass messages to the controller driver only from a protocol driver that is holding the lock, and to put all messages from other, non lock-aware, drivers in a separate holding queue. I think the latter is the method implemented by the proposed locking patches, but I'm not sure (I haven't been paying a lot of attention to that).
So using spi_read and spi_write functions (which in turn calls spi_sync) guarantees a blocking transfer. Which in my understanding means in the code, the spi transfer will complete before moving onto the next line.
e.g.:
-> private data manipulation code
-> use spi transfer to read large amounts of data
will it wait here until transfer is complete before moving to next line?
-> use data read from spi transfer
Quoted 6 lines
I'm assuming that you are using the SPI-core/protocol driver/controller driver model in the system that you have having trouble with. If you are somehow attaching two controller drivers to the same bus, there there is only trouble to be had. I hope the kernel prevents assignment of the same hardware resources to two drivers.
Yes, I am only using one controller. In this case it is the omap2_mcspi.c. I only have one SPI device connected.
I guess my problem seems to lie elsewhere. It could be a flaky hardware from my end. I am transferring data to and fro an FPGA.
Could I be using SPI with dma to improve speed and maybe have access to more resource in system memory?
Thanks again for the explanation,
Amit
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
A message that is not in the archive (sent off-list or not yet received): 20100318200940.GC16834-QCuvCd35e3/[email protected]
Quoted 17 lines · Amit Uttamchandani
On Thu, Mar 18, 2010 at 01:28:17PM -0400, Ned Forrester wrote:
[...]
Quoted 12 lines
The message is atomic in the sense that the controller driver guarantees to complete all the transfers defined in the message (assuming no errors) before it handles the next message. Each transfer may change the clock or bits/word, and may also ask for the chip select to manipulated (or not) at the end of the transfer. There is no guarantee that the next message processed by the controller driver will be for the same device as the previous message; the controller driver just takes the next message in the queue, which could have come from any of the protocol drivers that is operating devices on the bus. Only when there is a single device on the bus is the next device certain to be the same as the last device.
Quoted 4 lines
That's good to know that it is queuing the messages. I am assuming that this goes for all messages going into the controller right? Not just for one specific spi message? (e.g. different spi messages from different function calls in the same driver).
A message is a message. If it is sent through spi_async or spi_sync, then it goes in the queue (there is no other way to queue a message, so far as I know). Of course, it is up to the controller drivers to be well behaved, and to read the queue in order, but I don't think any of them would have passed review if they did not.
Quoted 17 lines
Quoted 7 lines
In general, the design of the SPI core and controller drivers prevents the type of collision that you describe above. The controller driver can only respond to one message at a time, and will complete that message before taking the next in the queue. Only if you need to guarantee an un-interrupted sequence of messages, such as seems to be necessary for those memories, would locking be necessary. The bus can only be locked in a cooperative or centralized way; there is no way to
Quoted 9 lines
prevent another protocol driver from inserting a message in the controller driver's queue, unless all drivers have been programmed to respect the locks, or unless the queue mechanism is altered to pass messages to the controller driver only from a protocol driver that is holding the lock, and to put all messages from other, non lock-aware, drivers in a separate holding queue. I think the latter is the method implemented by the proposed locking patches, but I'm not sure (I haven't been paying a lot of attention to that).
Quoted 3 lines
So using
spi_readandspi_writefunctions (which in turn callsspi_sync) guarantees a blocking transfer. Which in my understanding means in the code, the spi transfer will complete before moving onto the next line.
I'm not familiar with spi_read and spi_write. I guess those may be part of the user space driver: spidev? (I wrote a dedicated protocol driver for my device.)
That is the way that spi_sync is supposed to work: accept a message, and wait until that message is returned by the controller driver, prior to spi_sync returning to from the call to it. That does not mean that messages from other protocol drivers were not handled before or after, except when, as in your case, there is only one device on the bus.
Quoted 9 lines
e.g.:
-> private data manipulation code
-> use spi transfer to read large amounts of data
will it wait here until transfer is complete before moving to next line?-> use data read from spi transfer
I'm not sure what you are suggesting here. I can't tell if your use of the word "transfer" is meant in the strictly defined sense of the SPI
core: it is a single exchange of n-bytes that may or may not be the only one in a message. If the code ultimately calls spi_sync, it should wait at the call until the message is complete.
Quoted 7 lines
Quoted 6 lines
I'm assuming that you are using the SPI-core/protocol driver/controller driver model in the system that you have having trouble with. If you are somehow attaching two controller drivers to the same bus, there there is only trouble to be had. I hope the kernel prevents assignment of the same hardware resources to two drivers.
Quoted 5 lines
Yes, I am only using one controller. In this case it is the
omap2_mcspi.c. I only have one SPI device connected.I guess my problem seems to lie elsewhere. It could be a flaky hardware from my end. I am transferring data to and fro an FPGA.
It could be a lot of places, from bug in your code to bug in the controller driver. I'm not sure how much testing omap2_mcspi.c has received. When I first started using pxa2xx_spi.c, less than a year after it appeared in the kernel, I found a variety of bugs. I then spent about 6 months re-writing the driver to handle a high data-rate read-only master. It was much harder for me than I expected.
Quoted 2 lines
Could I be using SPI with dma to improve speed and maybe have access to more resource in system memory?
Way too short a question, with way too many unspecified variables. I really can't help you with that, because I know nothing about omap. DMA is useful if you are transmitting/receiving lots of data. If your device requires short exchanges (bytes, not Kbytes or Mbytes) with decision making in between, then DMA might not help. Many of the controller drivers have DMA built-in, and it is not hard to enable it, but I don't know about omap2_mcspi.c.
Signature · 16 lines
Ned Forrester nforrester-/[email protected] Oceanographic Systems Lab 508-289-2226 Applied Ocean Physics and Engineering Dept. Woods Hole Oceanographic Institution Woods Hole, MA 02543, USA http://www.whoi.edu/ http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212 http://www.whoi.edu/hpb/Site.do?id=1532 http://www.whoi.edu/page.do?pid=10079 ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev
A message that is not in the archive (sent off-list or not yet received): 4BA2A4F4.60207-/[email protected]
Quoted 7 lines · Ned Forrester
It could be a lot of places, from bug in your code to bug in the controller driver. I'm not sure how much testing
omap2_mcspi.chas received. When I first started usingpxa2xx_spi.c, less than a year after it appeared in the kernel, I found a variety of bugs. I then spent about 6 months re-writing the driver to handle a high data-rate read-only master. It was much harder for me than I expected.
Oops! On re-reading what I wrote, I see that left some loose ends.
For the bugs that I found, I worked with the original author (Stephen Street) to develop a patch, which he then submitted and which has long since been incorporated in the mainline kernel. I and many others have fixed other bugs since then.
The major re-write for a high-speed master device (streaming data, the only device on the bus) have never been submitted as patches. This is partly because, while it now works, it is ugly and has never been cleaned up to be suitable for submission. The other reason is that, while it might be useful as a model to others, the major changes greatly expand the size of the driver, and they would only be useful for single devices that stream large amounts of data in or out. This work was done several years ago, and the driver would now need major renovation to move it from 2.6.20 to 2.6.35.
Signature · 16 lines
Ned Forrester nforrester-/[email protected] Oceanographic Systems Lab 508-289-2226 Applied Ocean Physics and Engineering Dept. Woods Hole Oceanographic Institution Woods Hole, MA 02543, USA http://www.whoi.edu/ http://www.whoi.edu/sbl/liteSite.do?litesiteid=7212 http://www.whoi.edu/hpb/Site.do?id=1532 http://www.whoi.edu/page.do?pid=10079 ------------------------------------------------------------------------------ Download Intel® Parallel Studio Eval Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta. http://p.sf.net/sfu/intel-sw-dev
On Thu, Mar 18, 2010 at 06:11:00PM -0400, Ned Forrester wrote:
[...]
Quoted 4 lines
I'm not familiar with
spi_readandspi_write. I guess those may be part of the user space driver: spidev? (I wrote a dedicated protocol driver for my device.)
Looking at spi.c, spi_read and spi_write both use spi_sync, so both are blocking.
Quoted 7 lines
It could be a lot of places, from bug in your code to bug in the controller driver. I'm not sure how much testing
omap2_mcspi.chas received. When I first started usingpxa2xx_spi.c, less than a year after it appeared in the kernel, I found a variety of bugs. I then spent about 6 months re-writing the driver to handle a high data-rate read-only master. It was much harder for me than I expected.
You're right. In our case, a lot of the problems where from the FPGA side of things. We implemented the open cores ethmac controller in our FPGA. Then wrote a SPI to wishbone protocol communication controller. I am sure there are bugs in there too.
Quoted 4 lines
Quoted 3 lines
Could I be using SPI with dma to improve speed and maybe have access to more resource in system memory?
Quoted 8 lines
Way too short a question, with way too many unspecified variables. I really can't help you with that, because I know nothing about omap. DMA is useful if you are transmitting/receiving lots of data. If your device requires short exchanges (bytes, not Kbytes or Mbytes) with decision making in between, then DMA might not help. Many of the controller drivers have DMA built-in, and it is not hard to enable it, but I don't know about
omap2_mcspi.c.
I guess I won't be needing DMA as our transfers and quite small. Thanks for the explanation.
Thank you again for your help.
Amit
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs proactively, and fine-tune applications for parallel performance. See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev