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@xxxxxxx wrote:
> > >>>From: Corey Minyard <cminyard@xxxxxxxxxx>
> > >>>
> > >>>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@xxxxxxxxxx>
> > >>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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
