Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions
On Thu, Jul 5, 2012 at 4:10 PM, Jean Delvare kh...@linux-fr.org wrote: Hi Daniel, On Thu, 5 Jul 2012 12:31:11 +0800, Daniel Kurtz wrote: On Thu, Jul 5, 2012 at 4:16 AM, Jean Delvare kh...@linux-fr.org wrote: You should be able to reproduce this bug by loading i2c-i801 with option disable_features=0x0002, assuming your SMBus IRQ is shared. This leads me to several thoughts (feel free to correct me if I'm wrong, I am getting very tired): 1* This must be the reason why a bit was added to the PCI status register starting with the ICH5, telling you if an interrupt has been delivered to you. Checking for it as you originally did was a good idea after all. Reducing scope to get accepted patches in sooner sounds like a good plan to me. Agreed. 2* Is there any reason why you decided to clear the status bits in i801_isr(), rather than after wait_event()? I am no expert in interrupt handling, I admit, but letting the caller clear the status bits would ensure we don't clear status bits when nobody is actually waiting to catch them. BYTE_DONE is cleared to kick off the next byte, and it doesn't generate a wait_event, thus it is cleared right in the irq. No problem with BYTE_DONE. The logic for clearing the STATUS_FLAGS was simply that they all indicate the end of a transaction, so there won't be any further interrupts. Thus, it is safe to clear it in the irq, since the irq will be followed by exactly one wait_event, which can read and process saved status. It is safe if and only if someone is actually listening to the event. This was not the case for me yesterday. Even if we don't mix interrupts and polling, i801_isr() might still get called when we aren't listening. Not only because the IRQ may be shared, but also for events we don't yet handle, such as an SMBus Alert. Not sure about slave mode. The real reason for clearing the flag in the hard irq is that otherwise, we end up with an infinite irq loop. The interrupt is apparently level triggered, and must be cleared before exiting the ISR. Unfortunately, I only had time today to discover this, but not time to fix it. Next week I will be away, so I won't have a chance to provide more patches for the next 10 days. I think the 6 patches you have already make a complete set, however, and shouldn't be waiting for the interrupt patch(es) which can follow at some future date. -Daniel 3* Applying this patch (7/8) without the one adding interrupt support to i801_block_transaction_byte_by_byte (8/8) is not OK: mixing interrupts and polling isn't safe. So unless we implement 1* or 2*, both patches would have to be merged before being pushed upstream. 4* Even then, we have to keep in mind that i801_isr() may be called before the transaction is finished (if IRQ is shared.) My testing has shown that error flags may be raised before the BUSY flag is cleared, so we should use in i801_isr() the same strict conditions we are now using in the polling code. In other words, if we get an interrupt but the BUSY flag is still set, then it's not our interrupt and we should ignore it. This applies even if 2* or 3* above are implemented, but no longer if 1* is implemented. 5* Your claim that no locking is needed might have to be revisited when interrupts are shared. Implementing 1* has the drawback of limiting interrupt support to ICH5 and later chips, but I suspect it is the easiest and safest way, so I have no objection if you want to do that. Let's do this first, and then refactor later to add support for pre-ICH5 parts, if needed. OK, fine with me. The only downside is that it excludes my heavily-shared IRQ test machine, so testing that the shared IRQ case is properly covered will be a little harder. It sounds like supporting the pre-ICH5 parts w/ shared IRQ is not very pretty, since we are no longer guaranteed that we get a single transaction terminating interrupt. Indeed. In that case, using interrupts will be much like using polling, except that status check happens whenever we receive an interrupt, rather than at fixed time interval. Do you still maintain a public git repository? I never did. Have you updated it with the accepted version of the previous cleanup patches? I see this one, but it doesn't look updated: http://git.kernel.org/?p=linux/kernel/git/jdelvare/staging.git;a=summary This tree is only for sending patches to Linus. It doesn't reflect the current status of my working tree. I am maintaining quilt series for that, which you can find at: http://khali.linux-fr.org/devel/linux-3/ I'd like to fix up the interrupt patches per your feedback, but I'm not quite sure what the current status is of all the cleanup patches. In other words, if you push up the complete set of cleanup patches, I can then rebase the new consolidated irq patch onto it, and send them here for review. I
Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions
Hi Daniel, On Fri, 6 Jul 2012 18:28:28 +0800, Daniel Kurtz wrote: On Thu, Jul 5, 2012 at 4:10 PM, Jean Delvare kh...@linux-fr.org wrote: On Thu, 5 Jul 2012 12:31:11 +0800, Daniel Kurtz wrote: The logic for clearing the STATUS_FLAGS was simply that they all indicate the end of a transaction, so there won't be any further interrupts. Thus, it is safe to clear it in the irq, since the irq will be followed by exactly one wait_event, which can read and process saved status. It is safe if and only if someone is actually listening to the event. This was not the case for me yesterday. Even if we don't mix interrupts and polling, i801_isr() might still get called when we aren't listening. Not only because the IRQ may be shared, but also for events we don't yet handle, such as an SMBus Alert. Not sure about slave mode. The real reason for clearing the flag in the hard irq is that otherwise, we end up with an infinite irq loop. The interrupt is apparently level triggered, and must be cleared before exiting the ISR. Oh, OK. If we have to do it that way, then we do. Unfortunately, I only had time today to discover this, but not time to fix it. Next week I will be away, so I won't have a chance to provide more patches for the next 10 days. I think the 6 patches you have already make a complete set, however, and shouldn't be waiting for the interrupt patch(es) which can follow at some future date. Agreed, that's why they are already in linux-next waiting for the next merge window to open. That being said, I would hate to miss that merge window for interrupt support patches, as this is really a great performance improvement. So, if you have no objection, I propose to fix your patches myself today of tomorrow, test them, and if I find no problem, post them and push them to linux-next. You can review and test them when you return. -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions
Hi Daniel, On Thu, 5 Jul 2012 12:31:11 +0800, Daniel Kurtz wrote: On Thu, Jul 5, 2012 at 4:16 AM, Jean Delvare kh...@linux-fr.org wrote: You should be able to reproduce this bug by loading i2c-i801 with option disable_features=0x0002, assuming your SMBus IRQ is shared. This leads me to several thoughts (feel free to correct me if I'm wrong, I am getting very tired): 1* This must be the reason why a bit was added to the PCI status register starting with the ICH5, telling you if an interrupt has been delivered to you. Checking for it as you originally did was a good idea after all. Reducing scope to get accepted patches in sooner sounds like a good plan to me. Agreed. 2* Is there any reason why you decided to clear the status bits in i801_isr(), rather than after wait_event()? I am no expert in interrupt handling, I admit, but letting the caller clear the status bits would ensure we don't clear status bits when nobody is actually waiting to catch them. BYTE_DONE is cleared to kick off the next byte, and it doesn't generate a wait_event, thus it is cleared right in the irq. No problem with BYTE_DONE. The logic for clearing the STATUS_FLAGS was simply that they all indicate the end of a transaction, so there won't be any further interrupts. Thus, it is safe to clear it in the irq, since the irq will be followed by exactly one wait_event, which can read and process saved status. It is safe if and only if someone is actually listening to the event. This was not the case for me yesterday. Even if we don't mix interrupts and polling, i801_isr() might still get called when we aren't listening. Not only because the IRQ may be shared, but also for events we don't yet handle, such as an SMBus Alert. Not sure about slave mode. 3* Applying this patch (7/8) without the one adding interrupt support to i801_block_transaction_byte_by_byte (8/8) is not OK: mixing interrupts and polling isn't safe. So unless we implement 1* or 2*, both patches would have to be merged before being pushed upstream. 4* Even then, we have to keep in mind that i801_isr() may be called before the transaction is finished (if IRQ is shared.) My testing has shown that error flags may be raised before the BUSY flag is cleared, so we should use in i801_isr() the same strict conditions we are now using in the polling code. In other words, if we get an interrupt but the BUSY flag is still set, then it's not our interrupt and we should ignore it. This applies even if 2* or 3* above are implemented, but no longer if 1* is implemented. 5* Your claim that no locking is needed might have to be revisited when interrupts are shared. Implementing 1* has the drawback of limiting interrupt support to ICH5 and later chips, but I suspect it is the easiest and safest way, so I have no objection if you want to do that. Let's do this first, and then refactor later to add support for pre-ICH5 parts, if needed. OK, fine with me. The only downside is that it excludes my heavily-shared IRQ test machine, so testing that the shared IRQ case is properly covered will be a little harder. It sounds like supporting the pre-ICH5 parts w/ shared IRQ is not very pretty, since we are no longer guaranteed that we get a single transaction terminating interrupt. Indeed. In that case, using interrupts will be much like using polling, except that status check happens whenever we receive an interrupt, rather than at fixed time interval. Do you still maintain a public git repository? I never did. Have you updated it with the accepted version of the previous cleanup patches? I see this one, but it doesn't look updated: http://git.kernel.org/?p=linux/kernel/git/jdelvare/staging.git;a=summary This tree is only for sending patches to Linus. It doesn't reflect the current status of my working tree. I am maintaining quilt series for that, which you can find at: http://khali.linux-fr.org/devel/linux-3/ I'd like to fix up the interrupt patches per your feedback, but I'm not quite sure what the current status is of all the cleanup patches. In other words, if you push up the complete set of cleanup patches, I can then rebase the new consolidated irq patch onto it, and send them here for review. I updated the i2c series this morning, so it's up-to-date. -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions
On Thu, 5 Jul 2012 10:10:15 +0200, Jean Delvare wrote: On Thu, 5 Jul 2012 12:31:11 +0800, Daniel Kurtz wrote: On Thu, Jul 5, 2012 at 4:16 AM, Jean Delvare kh...@linux-fr.org wrote: Implementing 1* has the drawback of limiting interrupt support to ICH5 and later chips, but I suspect it is the easiest and safest way, so I have no objection if you want to do that. Let's do this first, and then refactor later to add support for pre-ICH5 parts, if needed. OK, fine with me. The only downside is that it excludes my heavily-shared IRQ test machine, so testing that the shared IRQ case is properly covered will be a little harder. Actually, no problem there: I can reproduce the issue just fine on my ICH5 system, which shares an IRQ between the sound chip and the SMBus controller. So I can use that system to test the updated code too. -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions
Hi Daniel,
On Wed, 27 Jun 2012 21:54:14 +0800, Daniel Kurtz wrote:
Add a new 'feature' to i2c-i801 to enable using i801 interrupts.
When the feature is enabled, then an isr is installed for the device's
pci irq.
An I2C/SMBus transaction is always terminated by one of the following
interrupt sources: FAILED, BUS_ERR, DEV_ERR, or on success: INTR.
When the isr fires for one of these cases, it sets the -status variable
and wakes up the waitq. The waitq then saves off the status code, and
clears -status (in preparation for some future transaction).
The SMBus controller generates an INTR irq at the end of each
transaction where INTREN was set in the HST_CNT register.
No locking is needed around accesses to priv-status since all writes to
it are serialized: it is only ever set once in the isr at the end of a
transaction, and cleared while no irqs can occur. In addition, the I2C
adapter lock guarantees that entire I2C transactions for a single
adapter are always serialized.
For this patch, the INTREN bit is set only for SMBus block, byte and word
transactions, but not for I2C reads or writes. The use of the DS
(BYTE_DONE) interrupt with byte-by-byte I2C transactions is implemented in
a subsequent patch.
The interrupt feature has only been enabled for COUGARPOINT hardware.
In addition, it is disabled if SMBus is using the SMI# interrupt.
Signed-off-by: Daniel Kurtz djku...@chromium.org
---
drivers/i2c/busses/i2c-i801.c | 93
++---
1 files changed, 87 insertions(+), 6 deletions(-)
diff --git a/drivers/i2c/busses/i2c-i801.c b/drivers/i2c/busses/i2c-i801.c
index 6fa2a0b..6bfedc0 100644
--- a/drivers/i2c/busses/i2c-i801.c
+++ b/drivers/i2c/busses/i2c-i801.c
@@ -60,10 +60,12 @@
Block process call transaction no
I2C block read transaction yes (doesn't use the block buffer)
Slave mode no
+ Interrupt processing yes
See the file Documentation/i2c/busses/i2c-i801 for details.
*/
+#include linux/interrupt.h
#include linux/module.h
#include linux/pci.h
#include linux/kernel.h
@@ -76,6 +78,7 @@
#include linux/io.h
#include linux/dmi.h
#include linux/slab.h
+#include linux/wait.h
/* I801 SMBus address offsets */
#define SMBHSTSTS(p) (0 + (p)-smba)
@@ -134,8 +137,9 @@
#define STATUS_ERROR_FLAGS (SMBHSTSTS_FAILED | SMBHSTSTS_BUS_ERR | \
SMBHSTSTS_DEV_ERR)
-#define STATUS_FLAGS (SMBHSTSTS_BYTE_DONE | SMBHSTSTS_INTR | \
- STATUS_ERROR_FLAGS)
+#define STATUS_RESULT_FLAGS (SMBHSTSTS_INTR | STATUS_ERROR_FLAGS)
I see no good reason to introduce this, you use it only once and
STATUS_FLAGS would work as well there.
+
+#define STATUS_FLAGS (SMBHSTSTS_BYTE_DONE | STATUS_RESULT_FLAGS)
/* Older devices have their ID defined in linux/pci_ids.h */
#define PCI_DEVICE_ID_INTEL_COUGARPOINT_SMBUS0x1c22
@@ -155,6 +159,10 @@ struct i801_priv {
unsigned char original_hstcfg;
struct pci_dev *pci_dev;
unsigned int features;
+
+ /* isr processing */
+ wait_queue_head_t waitq;
+ u8 status;
};
static struct pci_driver i801_driver;
@@ -163,6 +171,7 @@ static struct pci_driver i801_driver;
#define FEATURE_BLOCK_BUFFER (1 1)
#define FEATURE_BLOCK_PROC (1 2)
#define FEATURE_I2C_BLOCK_READ (1 3)
+#define FEATURE_IRQ (1 4)
/* Not really a feature, but it's convenient to handle it as such */
#define FEATURE_IDF (1 15)
@@ -171,6 +180,7 @@ static const char *i801_feature_names[] = {
Block buffer,
Block process call,
I2C block read,
+ Interrupt,
};
static unsigned int disable_features;
@@ -211,7 +221,12 @@ static int i801_check_post(struct i801_priv *priv, int
status, int timeout)
{
int result = 0;
- /* If the SMBus is still busy, we give up */
+ /*
+ * If the SMBus is still busy, we give up
+ * Note: This timeout condition only happens when using polling
+ * transactions. For interrupt operation, NAK/timeout is indicated by
+ * DEV_ERR.
+ */
if (timeout) {
dev_err(priv-pci_dev-dev, Transaction timeout\n);
/* try to stop the current command */
@@ -287,6 +302,14 @@ static int i801_transaction(struct i801_priv *priv, int
xact)
if (result 0)
return result;
+ if (priv-features FEATURE_IRQ) {
+ outb_p(xact | SMBHSTCNT_INTREN | SMBHSTCNT_START,
+SMBHSTCNT(priv));
+ wait_event(priv-waitq, (status = priv-status));
+ priv-status = 0;
+ return i801_check_post(priv, status, 0);
+ }
+
/* the current contents of SMBHSTCNT can be overwritten, since PEC,
* SMBSCMD are passed in xact */
outb_p(xact | SMBHSTCNT_START, SMBHSTCNT(priv));
@@ -341,6
Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions
Hi again Daniel, On Wed, 27 Jun 2012 21:54:14 +0800, Daniel Kurtz wrote: Add a new 'feature' to i2c-i801 to enable using i801 interrupts. When the feature is enabled, then an isr is installed for the device's pci irq. An I2C/SMBus transaction is always terminated by one of the following interrupt sources: FAILED, BUS_ERR, DEV_ERR, or on success: INTR. When the isr fires for one of these cases, it sets the -status variable and wakes up the waitq. The waitq then saves off the status code, and clears -status (in preparation for some future transaction). The SMBus controller generates an INTR irq at the end of each transaction where INTREN was set in the HST_CNT register. No locking is needed around accesses to priv-status since all writes to it are serialized: it is only ever set once in the isr at the end of a transaction, and cleared while no irqs can occur. In addition, the I2C adapter lock guarantees that entire I2C transactions for a single adapter are always serialized. For this patch, the INTREN bit is set only for SMBus block, byte and word transactions, but not for I2C reads or writes. The use of the DS (BYTE_DONE) interrupt with byte-by-byte I2C transactions is implemented in a subsequent patch. Hmm, I have hit a bug with this patch. I was testing it on my ICH3-M laptop, and while SMBus byte and word reads worked fine, SMBus block reads did fail once in a while. The ICH3-M lacks the block buffer, so SMBus block reads use the i801_block_transaction_byte_by_byte function, which at this point does not use interrupts. However, the IRQ is heavily shared on this laptop: additionally to SMBus, it is used for USB, sound, network and even the graphics chip. So function i801_isr() is called all the time. If i801_isr() is being called while i801_block_transaction_byte_by_byte() is running, there is a chance that the status register will have some flags set, in particular INTR. If so, the code in i801_isr() will clear the flags and wake up the wait queue while nobody was actually waiting for. And i801_block_transaction_byte_by_byte() will wait for an event which was already processed, leading to a timeout. You should be able to reproduce this bug by loading i2c-i801 with option disable_features=0x0002, assuming your SMBus IRQ is shared. This leads me to several thoughts (feel free to correct me if I'm wrong, I am getting very tired): 1* This must be the reason why a bit was added to the PCI status register starting with the ICH5, telling you if an interrupt has been delivered to you. Checking for it as you originally did was a good idea after all. 2* Is there any reason why you decided to clear the status bits in i801_isr(), rather than after wait_event()? I am no expert in interrupt handling, I admit, but letting the caller clear the status bits would ensure we don't clear status bits when nobody is actually waiting to catch them. 3* Applying this patch (7/8) without the one adding interrupt support to i801_block_transaction_byte_by_byte (8/8) is not OK: mixing interrupts and polling isn't safe. So unless we implement 1* or 2*, both patches would have to be merged before being pushed upstream. 4* Even then, we have to keep in mind that i801_isr() may be called before the transaction is finished (if IRQ is shared.) My testing has shown that error flags may be raised before the BUSY flag is cleared, so we should use in i801_isr() the same strict conditions we are now using in the polling code. In other words, if we get an interrupt but the BUSY flag is still set, then it's not our interrupt and we should ignore it. This applies even if 2* or 3* above are implemented, but no longer if 1* is implemented. 5* Your claim that no locking is needed might have to be revisited when interrupts are shared. Implementing 1* has the drawback of limiting interrupt support to ICH5 and later chips, but I suspect it is the easiest and safest way, so I have no objection if you want to do that. -- Jean Delvare -- To unsubscribe from this list: send the line unsubscribe linux-i2c in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions
Hi Jean, On Thu, Jul 5, 2012 at 4:16 AM, Jean Delvare kh...@linux-fr.org wrote: Hi again Daniel, On Wed, 27 Jun 2012 21:54:14 +0800, Daniel Kurtz wrote: Add a new 'feature' to i2c-i801 to enable using i801 interrupts. When the feature is enabled, then an isr is installed for the device's pci irq. An I2C/SMBus transaction is always terminated by one of the following interrupt sources: FAILED, BUS_ERR, DEV_ERR, or on success: INTR. When the isr fires for one of these cases, it sets the -status variable and wakes up the waitq. The waitq then saves off the status code, and clears -status (in preparation for some future transaction). The SMBus controller generates an INTR irq at the end of each transaction where INTREN was set in the HST_CNT register. No locking is needed around accesses to priv-status since all writes to it are serialized: it is only ever set once in the isr at the end of a transaction, and cleared while no irqs can occur. In addition, the I2C adapter lock guarantees that entire I2C transactions for a single adapter are always serialized. For this patch, the INTREN bit is set only for SMBus block, byte and word transactions, but not for I2C reads or writes. The use of the DS (BYTE_DONE) interrupt with byte-by-byte I2C transactions is implemented in a subsequent patch. Hmm, I have hit a bug with this patch. I was testing it on my ICH3-M laptop, and while SMBus byte and word reads worked fine, SMBus block reads did fail once in a while. The ICH3-M lacks the block buffer, so SMBus block reads use the i801_block_transaction_byte_by_byte function, which at this point does not use interrupts. However, the IRQ is heavily shared on this laptop: additionally to SMBus, it is used for USB, sound, network and even the graphics chip. So function i801_isr() is called all the time. If i801_isr() is being called while i801_block_transaction_byte_by_byte() is running, there is a chance that the status register will have some flags set, in particular INTR. If so, the code in i801_isr() will clear the flags and wake up the wait queue while nobody was actually waiting for. And i801_block_transaction_byte_by_byte() will wait for an event which was already processed, leading to a timeout. You should be able to reproduce this bug by loading i2c-i801 with option disable_features=0x0002, assuming your SMBus IRQ is shared. This leads me to several thoughts (feel free to correct me if I'm wrong, I am getting very tired): 1* This must be the reason why a bit was added to the PCI status register starting with the ICH5, telling you if an interrupt has been delivered to you. Checking for it as you originally did was a good idea after all. Reducing scope to get accepted patches in sooner sounds like a good plan to me. 2* Is there any reason why you decided to clear the status bits in i801_isr(), rather than after wait_event()? I am no expert in interrupt handling, I admit, but letting the caller clear the status bits would ensure we don't clear status bits when nobody is actually waiting to catch them. BYTE_DONE is cleared to kick off the next byte, and it doesn't generate a wait_event, thus it is cleared right in the irq. The logic for clearing the STATUS_FLAGS was simply that they all indicate the end of a transaction, so there won't be any further interrupts. Thus, it is safe to clear it in the irq, since the irq will be followed by exactly one wait_event, which can read and process saved status. 3* Applying this patch (7/8) without the one adding interrupt support to i801_block_transaction_byte_by_byte (8/8) is not OK: mixing interrupts and polling isn't safe. So unless we implement 1* or 2*, both patches would have to be merged before being pushed upstream. 4* Even then, we have to keep in mind that i801_isr() may be called before the transaction is finished (if IRQ is shared.) My testing has shown that error flags may be raised before the BUSY flag is cleared, so we should use in i801_isr() the same strict conditions we are now using in the polling code. In other words, if we get an interrupt but the BUSY flag is still set, then it's not our interrupt and we should ignore it. This applies even if 2* or 3* above are implemented, but no longer if 1* is implemented. 5* Your claim that no locking is needed might have to be revisited when interrupts are shared. Implementing 1* has the drawback of limiting interrupt support to ICH5 and later chips, but I suspect it is the easiest and safest way, so I have no objection if you want to do that. Let's do this first, and then refactor later to add support for pre-ICH5 parts, if needed. It sounds like supporting the pre-ICH5 parts w/ shared IRQ is not very pretty, since we are no longer guaranteed that we get a single transaction terminating interrupt. -- Jean Delvare Do you still maintain a public git repository? Have you updated
