i2c-smbus: Don't report duplicate alerts

From: Corey Minyard <cminyard@mvista.com>

Getting the same alert twice in a row is legal and normal,
especially on a fast device (like running in qemu).  Kind of
like interrupts.  So don't report duplicate alerts, and deliver
them normally.

Signed-off-by: Corey Minyard <cminyard@mvista.com>
---
 drivers/i2c/i2c-smbus.c | 7 -------
 1 file changed, 7 deletions(-)

On Tue, Jan 19, 2016 at 11:00:48AM -0600, minyard@acm.org wrote:
> From: Corey Minyard <cminyard@mvista.com>
> 
> Getting the same alert twice in a row is legal and normal,
> especially on a fast device (like running in qemu).  Kind of
> like interrupts.  So don't report duplicate alerts, and deliver
> them normally.
> 
> Signed-off-by: Corey Minyard <cminyard@mvista.com>

Looks plausible to me, but I never used SMBALERT myself. Any chance this
can cause a regression? Jean, what do you think?

> ---
>  drivers/i2c/i2c-smbus.c | 7 -------
>  1 file changed, 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> index 94765a8..cecd423 100644
> --- a/drivers/i2c/i2c-smbus.c
> +++ b/drivers/i2c/i2c-smbus.c
> @@ -75,7 +75,6 @@ static void smbus_alert(struct work_struct *work)
>  {
>  	struct i2c_smbus_alert *alert;
>  	struct i2c_client *ara;
> -	unsigned short prev_addr = 0;	/* Not a valid address */
>  
>  	alert = container_of(work, struct i2c_smbus_alert, alert);
>  	ara = alert->ara;
> @@ -99,18 +98,12 @@ static void smbus_alert(struct work_struct *work)
>  		data.flag = status & 1;
>  		data.addr = status >> 1;
>  
> -		if (data.addr == prev_addr) {
> -			dev_warn(&ara->dev, "Duplicate SMBALERT# from dev "
> -				"0x%02x, skipping\n", data.addr);
> -			break;
> -		}
>  		dev_dbg(&ara->dev, "SMBALERT# from dev 0x%02x, flag %d\n",
>  			data.addr, data.flag);
>  
>  		/* Notify driver for the device which issued the alert */
>  		device_for_each_child(&ara->adapter->dev, &data,
>  				      smbus_do_alert);
> -		prev_addr = data.addr;
>  	}
>  
>  	/* We handled all alerts; re-enable level-triggered IRQs */
> -- 
> 2.5.0
>

Le Thursday 03 March 2016 à 21:57 +0100, Wolfram Sang a écrit :
> On Tue, Jan 19, 2016 at 11:00:48AM -0600, minyard@acm.org wrote:
> > From: Corey Minyard <cminyard@mvista.com>
> > 
> > Getting the same alert twice in a row is legal and normal,
> > especially on a fast device (like running in qemu).  Kind of
> > like interrupts.  So don't report duplicate alerts, and deliver
> > them normally.
> > 
> > Signed-off-by: Corey Minyard <cminyard@mvista.com>
> 
> Looks plausible to me, but I never used SMBALERT myself. Any chance this
> can cause a regression? Jean, what do you think?

I'm afraid I had a good reason to add this check back then. I'll test
with my ADM1032 evaluation board when I get back home (tomorrow at the
earliest.) Maybe my hardware was misbehaving, in which case I agree any
filtering should be done at the device driver level. But I must double
check what the SMBus specification says too.

Either way the patch's subject is misleading. Should be "Don't filter
out duplicate alerts" or something like that.

> > ---
> >  drivers/i2c/i2c-smbus.c | 7 -------
> >  1 file changed, 7 deletions(-)
> > 
> > diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> > index 94765a8..cecd423 100644
> > --- a/drivers/i2c/i2c-smbus.c
> > +++ b/drivers/i2c/i2c-smbus.c
> > @@ -75,7 +75,6 @@ static void smbus_alert(struct work_struct *work)
> >  {
> >  	struct i2c_smbus_alert *alert;
> >  	struct i2c_client *ara;
> > -	unsigned short prev_addr = 0;	/* Not a valid address */
> >  
> >  	alert = container_of(work, struct i2c_smbus_alert, alert);
> >  	ara = alert->ara;
> > @@ -99,18 +98,12 @@ static void smbus_alert(struct work_struct *work)
> >  		data.flag = status & 1;
> >  		data.addr = status >> 1;
> >  
> > -		if (data.addr == prev_addr) {
> > -			dev_warn(&ara->dev, "Duplicate SMBALERT# from dev "
> > -				"0x%02x, skipping\n", data.addr);
> > -			break;
> > -		}
> >  		dev_dbg(&ara->dev, "SMBALERT# from dev 0x%02x, flag %d\n",
> >  			data.addr, data.flag);
> >  
> >  		/* Notify driver for the device which issued the alert */
> >  		device_for_each_child(&ara->adapter->dev, &data,
> >  				      smbus_do_alert);
> > -		prev_addr = data.addr;
> >  	}
> >  
> >  	/* We handled all alerts; re-enable level-triggered IRQs */

On 03/04/2016 04:15 AM, Jean Delvare wrote:
> Le Thursday 03 March 2016 à 21:57 +0100, Wolfram Sang a écrit :
>> On Tue, Jan 19, 2016 at 11:00:48AM -0600, minyard@acm.org wrote:
>>> From: Corey Minyard <cminyard@mvista.com>
>>>
>>> Getting the same alert twice in a row is legal and normal,
>>> especially on a fast device (like running in qemu).  Kind of
>>> like interrupts.  So don't report duplicate alerts, and deliver
>>> them normally.
>>>
>>> Signed-off-by: Corey Minyard <cminyard@mvista.com>
>> Looks plausible to me, but I never used SMBALERT myself. Any chance this
>> can cause a regression? Jean, what do you think?
> I'm afraid I had a good reason to add this check back then. I'll test
> with my ADM1032 evaluation board when I get back home (tomorrow at the
> earliest.) Maybe my hardware was misbehaving, in which case I agree any
> filtering should be done at the device driver level. But I must double
> check what the SMBus specification says too.

I looked at the SMBus specification and I couldn't find anything that
would speak to this particular issue.  It says it has to stop asserting
the interrupt when the ack is received on the bus, but it doesn't say
when it can re-assert the interrupts.

I will say that without this change SMBus alerts are fairly useless
with IPMI over SMBus on both real hardware and in qemu.  It just
spews out these warning messages in qemu, and it prints them out
periodically on real hardware.

To give an idea of what's happening here, on IPMI over SMBus, the
IPMI controller (BMC) will signal that it needs the host to do something
using an alert.  The driver does an I2C write to send a request to the
BMC to find out what it needs.  The BMC performs the request then
signals with an SMBus alert that the response is ready.  If the BMC is
very fast (like in the qemu case) or the host gets delayed enough before
coming back to this loop, the BMC will have the response ready and
reassert alert before the next check in the loop.

I don't see a way to fix this that handles both scenarios.

-corey

> Either way the patch's subject is misleading. Should be "Don't filter
> out duplicate alerts" or something like that.
>
>>> ---
>>>   drivers/i2c/i2c-smbus.c | 7 -------
>>>   1 file changed, 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
>>> index 94765a8..cecd423 100644
>>> --- a/drivers/i2c/i2c-smbus.c
>>> +++ b/drivers/i2c/i2c-smbus.c
>>> @@ -75,7 +75,6 @@ static void smbus_alert(struct work_struct *work)
>>>   {
>>>   	struct i2c_smbus_alert *alert;
>>>   	struct i2c_client *ara;
>>> -	unsigned short prev_addr = 0;	/* Not a valid address */
>>>   
>>>   	alert = container_of(work, struct i2c_smbus_alert, alert);
>>>   	ara = alert->ara;
>>> @@ -99,18 +98,12 @@ static void smbus_alert(struct work_struct *work)
>>>   		data.flag = status & 1;
>>>   		data.addr = status >> 1;
>>>   
>>> -		if (data.addr == prev_addr) {
>>> -			dev_warn(&ara->dev, "Duplicate SMBALERT# from dev "
>>> -				"0x%02x, skipping\n", data.addr);
>>> -			break;
>>> -		}
>>>   		dev_dbg(&ara->dev, "SMBALERT# from dev 0x%02x, flag %d\n",
>>>   			data.addr, data.flag);
>>>   
>>>   		/* Notify driver for the device which issued the alert */
>>>   		device_for_each_child(&ara->adapter->dev, &data,
>>>   				      smbus_do_alert);
>>> -		prev_addr = data.addr;
>>>   	}
>>>   
>>>   	/* We handled all alerts; re-enable level-triggered IRQs */
>

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

On Wed, Apr 27, 2016 at 09:28:07AM -0500, Corey Minyard wrote:
> On 03/04/2016 04:15 AM, Jean Delvare wrote:
> >Le Thursday 03 March 2016 à 21:57 +0100, Wolfram Sang a écrit :
> >>On Tue, Jan 19, 2016 at 11:00:48AM -0600, minyard@acm.org wrote:
> >>>From: Corey Minyard <cminyard@mvista.com>
> >>>
> >>>Getting the same alert twice in a row is legal and normal,
> >>>especially on a fast device (like running in qemu).  Kind of
> >>>like interrupts.  So don't report duplicate alerts, and deliver
> >>>them normally.
> >>>
> >>>Signed-off-by: Corey Minyard <cminyard@mvista.com>
> >>Looks plausible to me, but I never used SMBALERT myself. Any chance this
> >>can cause a regression? Jean, what do you think?
> >I'm afraid I had a good reason to add this check back then. I'll test
> >with my ADM1032 evaluation board when I get back home (tomorrow at the
> >earliest.) Maybe my hardware was misbehaving, in which case I agree any
> >filtering should be done at the device driver level. But I must double
> >check what the SMBus specification says too.
> 
> I looked at the SMBus specification and I couldn't find anything that
> would speak to this particular issue.  It says it has to stop asserting
> the interrupt when the ack is received on the bus, but it doesn't say
> when it can re-assert the interrupts.
> 
> I will say that without this change SMBus alerts are fairly useless
> with IPMI over SMBus on both real hardware and in qemu.  It just
> spews out these warning messages in qemu, and it prints them out
> periodically on real hardware.
> 
> To give an idea of what's happening here, on IPMI over SMBus, the
> IPMI controller (BMC) will signal that it needs the host to do something
> using an alert.  The driver does an I2C write to send a request to the
> BMC to find out what it needs.  The BMC performs the request then
> signals with an SMBus alert that the response is ready.  If the BMC is
> very fast (like in the qemu case) or the host gets delayed enough before
> coming back to this loop, the BMC will have the response ready and
> reassert alert before the next check in the loop.
> 
> I don't see a way to fix this that handles both scenarios.

Jean: any news on this?

Adding Benjamin to CC since he dealt with alerts recently, maybe he has
something to add?

> 
> -corey
> 
> >Either way the patch's subject is misleading. Should be "Don't filter
> >out duplicate alerts" or something like that.
> >
> >>>---
> >>>  drivers/i2c/i2c-smbus.c | 7 -------
> >>>  1 file changed, 7 deletions(-)
> >>>
> >>>diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> >>>index 94765a8..cecd423 100644
> >>>--- a/drivers/i2c/i2c-smbus.c
> >>>+++ b/drivers/i2c/i2c-smbus.c
> >>>@@ -75,7 +75,6 @@ static void smbus_alert(struct work_struct *work)
> >>>  {
> >>>  	struct i2c_smbus_alert *alert;
> >>>  	struct i2c_client *ara;
> >>>-	unsigned short prev_addr = 0;	/* Not a valid address */
> >>>  	alert = container_of(work, struct i2c_smbus_alert, alert);
> >>>  	ara = alert->ara;
> >>>@@ -99,18 +98,12 @@ static void smbus_alert(struct work_struct *work)
> >>>  		data.flag = status & 1;
> >>>  		data.addr = status >> 1;
> >>>-		if (data.addr == prev_addr) {
> >>>-			dev_warn(&ara->dev, "Duplicate SMBALERT# from dev "
> >>>-				"0x%02x, skipping\n", data.addr);
> >>>-			break;
> >>>-		}
> >>>  		dev_dbg(&ara->dev, "SMBALERT# from dev 0x%02x, flag %d\n",
> >>>  			data.addr, data.flag);
> >>>  		/* Notify driver for the device which issued the alert */
> >>>  		device_for_each_child(&ara->adapter->dev, &data,
> >>>  				      smbus_do_alert);
> >>>-		prev_addr = data.addr;
> >>>  	}
> >>>  	/* We handled all alerts; re-enable level-triggered IRQs */
> >
>

On Jun 19 2016 or thereabouts, Wolfram Sang wrote:
> On Wed, Apr 27, 2016 at 09:28:07AM -0500, Corey Minyard wrote:
> > On 03/04/2016 04:15 AM, Jean Delvare wrote:
> > >Le Thursday 03 March 2016 à 21:57 +0100, Wolfram Sang a écrit :
> > >>On Tue, Jan 19, 2016 at 11:00:48AM -0600, minyard@acm.org wrote:
> > >>>From: Corey Minyard <cminyard@mvista.com>
> > >>>
> > >>>Getting the same alert twice in a row is legal and normal,
> > >>>especially on a fast device (like running in qemu).  Kind of
> > >>>like interrupts.  So don't report duplicate alerts, and deliver
> > >>>them normally.
> > >>>
> > >>>Signed-off-by: Corey Minyard <cminyard@mvista.com>
> > >>Looks plausible to me, but I never used SMBALERT myself. Any chance this
> > >>can cause a regression? Jean, what do you think?
> > >I'm afraid I had a good reason to add this check back then. I'll test
> > >with my ADM1032 evaluation board when I get back home (tomorrow at the
> > >earliest.) Maybe my hardware was misbehaving, in which case I agree any
> > >filtering should be done at the device driver level. But I must double
> > >check what the SMBus specification says too.
> > 
> > I looked at the SMBus specification and I couldn't find anything that
> > would speak to this particular issue.  It says it has to stop asserting
> > the interrupt when the ack is received on the bus, but it doesn't say
> > when it can re-assert the interrupts.
> > 
> > I will say that without this change SMBus alerts are fairly useless
> > with IPMI over SMBus on both real hardware and in qemu.  It just
> > spews out these warning messages in qemu, and it prints them out
> > periodically on real hardware.
> > 
> > To give an idea of what's happening here, on IPMI over SMBus, the
> > IPMI controller (BMC) will signal that it needs the host to do something
> > using an alert.  The driver does an I2C write to send a request to the
> > BMC to find out what it needs.  The BMC performs the request then
> > signals with an SMBus alert that the response is ready.  If the BMC is
> > very fast (like in the qemu case) or the host gets delayed enough before
> > coming back to this loop, the BMC will have the response ready and
> > reassert alert before the next check in the loop.
> > 
> > I don't see a way to fix this that handles both scenarios.
> 
> Jean: any news on this?
> 
> Adding Benjamin to CC since he dealt with alerts recently, maybe he has
> something to add?

Not much. I can see why Corey has such a patch but I miss why this check
was in in the first place. Given the scheduling and the irq and the
worker, I'd say having a duplicate alert is valid in case the device
reassert the line as soon as it gets contacted by the host.

I think we need Jean to check whether the invalid duplicate alert was a
misbehavior, just an extra check, or actually required in some cases.

Cheers,
Benjamin

> 
> > 
> > -corey
> > 
> > >Either way the patch's subject is misleading. Should be "Don't filter
> > >out duplicate alerts" or something like that.
> > >
> > >>>---
> > >>>  drivers/i2c/i2c-smbus.c | 7 -------
> > >>>  1 file changed, 7 deletions(-)
> > >>>
> > >>>diff --git a/drivers/i2c/i2c-smbus.c b/drivers/i2c/i2c-smbus.c
> > >>>index 94765a8..cecd423 100644
> > >>>--- a/drivers/i2c/i2c-smbus.c
> > >>>+++ b/drivers/i2c/i2c-smbus.c
> > >>>@@ -75,7 +75,6 @@ static void smbus_alert(struct work_struct *work)
> > >>>  {
> > >>>  	struct i2c_smbus_alert *alert;
> > >>>  	struct i2c_client *ara;
> > >>>-	unsigned short prev_addr = 0;	/* Not a valid address */
> > >>>  	alert = container_of(work, struct i2c_smbus_alert, alert);
> > >>>  	ara = alert->ara;
> > >>>@@ -99,18 +98,12 @@ static void smbus_alert(struct work_struct *work)
> > >>>  		data.flag = status & 1;
> > >>>  		data.addr = status >> 1;
> > >>>-		if (data.addr == prev_addr) {
> > >>>-			dev_warn(&ara->dev, "Duplicate SMBALERT# from dev "
> > >>>-				"0x%02x, skipping\n", data.addr);
> > >>>-			break;
> > >>>-		}
> > >>>  		dev_dbg(&ara->dev, "SMBALERT# from dev 0x%02x, flag %d\n",
> > >>>  			data.addr, data.flag);
> > >>>  		/* Notify driver for the device which issued the alert */
> > >>>  		device_for_each_child(&ara->adapter->dev, &data,
> > >>>  				      smbus_do_alert);
> > >>>-		prev_addr = data.addr;
> > >>>  	}
> > >>>  	/* We handled all alerts; re-enable level-triggered IRQs */
> > >
> > 


--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html