Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions

2012-07-06 Thread Jean Delvare
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  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

2012-07-06 Thread Daniel Kurtz
On Thu, Jul 5, 2012 at 4:10 PM, Jean Delvare  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  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 o

Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions

2012-07-05 Thread Jean Delvare
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  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

2012-07-05 Thread Jean Delvare
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  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

2012-07-04 Thread Daniel Kurtz
Hi Jean,

On Thu, Jul 5, 2012 at 4:16 AM, Jean Delvare  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 interrup

Re: [PATCH 7/8 v3] i2c: i801: enable irq for i801 smbus transactions

2012-07-04 Thread Jean Delvare
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

2012-07-04 Thread Jean Delvare
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 
> ---
>  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 
>  #include 
>  #include 
>  #include 
> @@ -76,6 +78,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /* 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  */
>  #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