Re: [PATCH] i2c-smbus: Don't report duplicate alerts

[Benjamin Tissoires <benjamin.tissoires@redhat.com>]

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 */
> > >
> >