Re: Do any drivers access the cx25840 module in an atomic context?
On Monday 18 January 2010 06:19:28 Andy Walls wrote: Hi, I am going to add locking to the cx25840 module register reads and writes because I now have a case where a workqueue, a userpace, and/or the cx25840 firmware loading workqueue could contend for access to the CX2584x or equivalent device. I have identiifed the following drivers that actually use this module: cx231xx, pvrusb2, ivtv, cx23885 Now here's where I need help, since I don't understand the USB stuff too well and there's a lot of code to audit: Do any of these modules call the cx25840 routines with either: a. call the cx25840 module subdev functions with a spinlock held ? or b. call the cx25840 module subdev functions from an interrupt context: i.e. a hard irq or tasklet ? or c. bypass the normal cx25840_read/write calls with direct I2C access to the client address of 0x44 (0x88 1) ? Any definitive confirmation anyone can give on any of these drivers would be helpful and would save me some time. For ivtv: a. no b. no c. no BTW: b should never happen. i2c reads/writes are very slow and so it is a very bad idea to do that in interrupt context. Since there is some low-level locking in the i2c stack as well I think it will probably not even work correctly if called from interrupt context. Note that AFAIK the i2c stack will already serialize i2c commands for you. So are you sure you need to serialize access on the higher levels as well? Firmware load should be already serialized in the bridge driver. That leaves a possible call to cx25840_reset() which can trigger a fw load at any time, but I believe that can be removed as well since it is only used by the IR reset code which is obsolete. At least in ivtv I have never seen any issues with cx25840 and atomic contexts. Regards, Hans For the cx23885 driver I think these are the answers: a. probably not b. probably not c. yes but I have to double check. I can probably audit the ivtv driver on my own. I understand it's structure, but it's a big driver and will take time to check, if no one knows off hand. The pvrusb2 and cx231xx will be a little harder for me. They aren't terribly large, but I don't understand USB device interrupt contexts. TIA. Regards, Andy -- Hans Verkuil - video4linux developer - sponsored by TANDBERG -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do any drivers access the cx25840 module in an atomic context?
On Mon, 2010-01-18 at 11:16 +0100, Hans Verkuil wrote: On Monday 18 January 2010 06:19:28 Andy Walls wrote: Hi, I am going to add locking to the cx25840 module register reads and writes because I now have a case where a workqueue, a userpace, and/or the cx25840 firmware loading workqueue could contend for access to the CX2584x or equivalent device. I have identiifed the following drivers that actually use this module: cx231xx, pvrusb2, ivtv, cx23885 Now here's where I need help, since I don't understand the USB stuff too well and there's a lot of code to audit: Do any of these modules call the cx25840 routines with either: a. call the cx25840 module subdev functions with a spinlock held ? or b. call the cx25840 module subdev functions from an interrupt context: i.e. a hard irq or tasklet ? or c. bypass the normal cx25840_read/write calls with direct I2C access to the client address of 0x44 (0x88 1) ? Any definitive confirmation anyone can give on any of these drivers would be helpful and would save me some time. For ivtv: a. no b. no c. no Thanks. I thought so, but wasn't sure. BTW: b should never happen. i2c reads/writes are very slow and so it is a very bad idea to do that in interrupt context. Yeah. I just found that out. :P Since there is some low-level locking in the i2c stack as well I think it will probably not even work correctly if called from interrupt context. Well it can work, but I don't think what the i2c stack does in such cases is reliable. The i2c subsystem will call in_atomic() and in_interrupt() to find out if it can sleep or not (it turns out this is unreliable with PREEMPT disabled) and then calls mutex_trylock(). Obtaining a mutex with mutex_trylock() in an interrupt context isn't allowed AFAICT as it at breaks lockdep's ownership tracking of mutexes. If the i2c stack doesn't get the *adapter* mutex lock in the perceived atomic context, the i2c transfer fails with -EAGAIN. I'm hoping nothing accesses the cx25840 routines from an atomic context, but hope is usually not a good basis for a plan. ;) Note that AFAIK the i2c stack will already serialize i2c commands for you. So are you sure you need to serialize access on the higher levels as well? The i2c stack serializes the adapter accesses. The problem is that if there is contention for a particular client, and an i2c read transaction is broken up into 2 distinct transfers, like it is in the cx25840 module (i2c_master_send() of register address followed by a separate i2c_master_recv()), then the individiual i2c transfers in these transactions can get interleaved. I don't know why the cx25840 module did things this way, but the ability to do a cx25840 device register read in a single i2c_trasnfer() transaction with a repeated start is contingent on the underlying master implementation. I'll have to audit the underlying drivers to check if their master implementations support that. If they can, then I can combine the i2c_master_send() and i2c_master_recv() into a single i2c_transfer(). Otherwise a lock is actually needed in the cx25840_read() routines, even without the question of atomic context. Firmware load should be already serialized in the bridge driver. That leaves a possible call to cx25840_reset() which can trigger a fw load at any time, but I believe that can be removed as well since it is only used by the IR reset code which is obsolete. Yes, you're right. So, with the CX2388[578] devices, the only real contention I then need to deal with would be between a file operation (e.g. ioctl) indcued I2C transaction in a process context, contending with an IR interrupt induced I2C transaction running in a workqueue context. I really just don't want to break any existing drivers, while ensuring reliable operation of the device handled by the cx25840 module, and allowing maximum concurrency to the device. Right now the cx25840 module cannot ensure reliability. Ensuring no register manipulation requests are coming in from an atomic context makes solving that reliability problem easier, hence my question. I can always just reduce the problem scope by doing locking in the cx25840 module for only CX2388[578] devices. Then I would only need to audit the cx23885 driver. At least in ivtv I have never seen any issues with cx25840 and atomic contexts. Well, if one ever does call the cx25840_read() in an atomic context, if there is no contention for the I2C adapter at that time, the I2C transfers should succeed. If there is contention in this atomic context, the transaction silently fails, returning 0 as the value read from the register. I have learned this from recent experience. :P ivtv serializes all ioctl calls, and open/read/write/close calls from user space are unlikely to be concurrent with ioctls. I didn't check if ivtv manipulates the cx25840 from a workqueue context; I suspect it doesn't. Also if ivtv never
Re: Do any drivers access the cx25840 module in an atomic context?
On Monday 18 January 2010 17:52:43 Andy Walls wrote: On Mon, 2010-01-18 at 11:16 +0100, Hans Verkuil wrote: On Monday 18 January 2010 06:19:28 Andy Walls wrote: Hi, I am going to add locking to the cx25840 module register reads and writes because I now have a case where a workqueue, a userpace, and/or the cx25840 firmware loading workqueue could contend for access to the CX2584x or equivalent device. I have identiifed the following drivers that actually use this module: cx231xx, pvrusb2, ivtv, cx23885 Now here's where I need help, since I don't understand the USB stuff too well and there's a lot of code to audit: Do any of these modules call the cx25840 routines with either: a. call the cx25840 module subdev functions with a spinlock held ? or b. call the cx25840 module subdev functions from an interrupt context: i.e. a hard irq or tasklet ? or c. bypass the normal cx25840_read/write calls with direct I2C access to the client address of 0x44 (0x88 1) ? Any definitive confirmation anyone can give on any of these drivers would be helpful and would save me some time. For ivtv: a. no b. no c. no Thanks. I thought so, but wasn't sure. BTW: b should never happen. i2c reads/writes are very slow and so it is a very bad idea to do that in interrupt context. Yeah. I just found that out. :P Since there is some low-level locking in the i2c stack as well I think it will probably not even work correctly if called from interrupt context. Well it can work, but I don't think what the i2c stack does in such cases is reliable. The i2c subsystem will call in_atomic() and in_interrupt() to find out if it can sleep or not (it turns out this is unreliable with PREEMPT disabled) and then calls mutex_trylock(). Obtaining a mutex with mutex_trylock() in an interrupt context isn't allowed AFAICT as it at breaks lockdep's ownership tracking of mutexes. If the i2c stack doesn't get the *adapter* mutex lock in the perceived atomic context, the i2c transfer fails with -EAGAIN. I'm hoping nothing accesses the cx25840 routines from an atomic context, but hope is usually not a good basis for a plan. ;) Note that AFAIK the i2c stack will already serialize i2c commands for you. So are you sure you need to serialize access on the higher levels as well? The i2c stack serializes the adapter accesses. The problem is that if there is contention for a particular client, and an i2c read transaction is broken up into 2 distinct transfers, like it is in the cx25840 module (i2c_master_send() of register address followed by a separate i2c_master_recv()), then the individiual i2c transfers in these transactions can get interleaved. I don't know why the cx25840 module did things this way, but the ability to do a cx25840 device register read in a single i2c_trasnfer() transaction with a repeated start is contingent on the underlying master implementation. I'll have to audit the underlying drivers to check if their master implementations support that. If they can, then I can combine the i2c_master_send() and i2c_master_recv() into a single i2c_transfer(). Otherwise a lock is actually needed in the cx25840_read() routines, even without the question of atomic context. Ah, I hadn't realized that it was split up. If it can be merged into a single i2c_transfer, then that would certainly help things. Firmware load should be already serialized in the bridge driver. That leaves a possible call to cx25840_reset() which can trigger a fw load at any time, but I believe that can be removed as well since it is only used by the IR reset code which is obsolete. Yes, you're right. So, with the CX2388[578] devices, the only real contention I then need to deal with would be between a file operation (e.g. ioctl) indcued I2C transaction in a process context, contending with an IR interrupt induced I2C transaction running in a workqueue context. I really wonder if any locking is needed provided the read and write commands can be done atomically in the i2c layer. The only reason why that might be necessary is if there are multiple reads or writes that must be done in order without other reads/writes being mixed in. From memory the only place where that is needed is during the firmware load. Of course, this is from memory only, so I might well be wrong. I really just don't want to break any existing drivers, while ensuring reliable operation of the device handled by the cx25840 module, and allowing maximum concurrency to the device. Right now the cx25840 module cannot ensure reliability. Ensuring no register manipulation requests are coming in from an atomic context makes solving that reliability problem easier, hence my question. I can always just reduce the problem scope by doing locking in the cx25840 module for only CX2388[578] devices. Then I would only need to
Re: Do any drivers access the cx25840 module in an atomic context?
On Mon, 2010-01-18 at 14:18 -0600, Mike Isely wrote: On Mon, 18 Jan 2010, Andy Walls wrote: Any definitive confirmation anyone can give on any of these drivers would be helpful and would save me some time. Mike, Great! Thank you for the answers. If you would indulge one more (compound) question: Looking at the I2C master implementation in pvrusb2, it looks like it would be OK for me to combine the i2c_master_write() and i2c_master_read() in cx25840_read() into a single 2 msg i2c_transfer() without the pvrusb2 driver having a problem. a. Is that correct? b. Is there a limit on the combined payload, such that a the cx25840_read4() would not work as a combined i2c_transfer() ? Regards, Andy -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Do any drivers access the cx25840 module in an atomic context?
On Mon, 18 Jan 2010, Andy Walls wrote: On Mon, 2010-01-18 at 14:18 -0600, Mike Isely wrote: On Mon, 18 Jan 2010, Andy Walls wrote: Any definitive confirmation anyone can give on any of these drivers would be helpful and would save me some time. Mike, Great! Thank you for the answers. You're welcome. If you would indulge one more (compound) question: Looking at the I2C master implementation in pvrusb2, it looks like it would be OK for me to combine the i2c_master_write() and i2c_master_read() in cx25840_read() into a single 2 msg i2c_transfer() without the pvrusb2 driver having a problem. a. Is that correct? Yes, that is correct. b. Is there a limit on the combined payload, such that a the cx25840_read4() would not work as a combined i2c_transfer() ? There is an overall limit on the size of the I2C transfer. This is due to the underlying firmware on pvrusb2-support devices. Essentially the entire outgoing transfer plus a few bytes of overhead has to fit inside a single 64 byte URB. This also limits the atomic read size to roughly 64 bytes as well (the URB size on the returned data). You should be able to reliably write up to 48 bytes at a time, perhaps even a little more. This issue caused a problem for the cx25840 module a few years back when it used to do firmware downloads with large (e.g. 1024 byte or larger) single I2C transfers. Hans told me then it was that large because it allowed the ivtv driver to run more efficiently, but we cut it back to 48 bytes since it triggered problems with I2C adapters (e.g. pvrusb2) which could not handle such larger transfers at all. The pvrusb2 driver's I2C adapter is really just a proxy for the I2C implementation in the device at the far end of the USB cable. So it works at a higher level than one might normally expect - it operates at the transfer level, no bit-banging. The communications protocol required by the hardware limits the I2C transfers to be either a write of some size, or an atomic write followed by a read of various sizes. The pvrusb2 implementation looks at the incoming transfers and tries to map them as best it can over what the device protocol allows. Generally this means that if it is possible it will do the right thing. In the specific case you mentioned above, the result should in fact be exactly what you need. (I'm saying that without having looked at that area of code for quite a while, but it's what I remember being in my head when I did that part..) -Mike -- Mike Isely isely @ isely (dot) net PGP: 03 54 43 4D 75 E5 CC 92 71 16 01 E2 B5 F5 C1 E8 -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
