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?
>
> -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 */
> >
>
Attachment:
signature.asc
Description: PGP signature
