* [PATCH] i2c-smbus: Don't report duplicate alerts @ 2016-01-19 17:00 minyard 2016-03-03 20:57 ` Wolfram Sang 0 siblings, 1 reply; 6+ messages in thread From: minyard @ 2016-01-19 17:00 UTC (permalink / raw) To: Wolfram Sang, linux-i2c; +Cc: Corey Minyard 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(-) 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 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c-smbus: Don't report duplicate alerts 2016-01-19 17:00 [PATCH] i2c-smbus: Don't report duplicate alerts minyard @ 2016-03-03 20:57 ` Wolfram Sang 2016-03-04 10:15 ` Jean Delvare 0 siblings, 1 reply; 6+ messages in thread From: Wolfram Sang @ 2016-03-03 20:57 UTC (permalink / raw) To: minyard, Jean Delvare; +Cc: linux-i2c, Corey Minyard [-- Attachment #1: Type: text/plain, Size: 1732 bytes --] 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 > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c-smbus: Don't report duplicate alerts 2016-03-03 20:57 ` Wolfram Sang @ 2016-03-04 10:15 ` Jean Delvare 2016-04-27 14:28 ` Corey Minyard 0 siblings, 1 reply; 6+ messages in thread From: Jean Delvare @ 2016-03-04 10:15 UTC (permalink / raw) To: Wolfram Sang; +Cc: minyard, linux-i2c, Corey Minyard 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 */ -- Jean Delvare SUSE L3 Support ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c-smbus: Don't report duplicate alerts 2016-03-04 10:15 ` Jean Delvare @ 2016-04-27 14:28 ` Corey Minyard 2016-06-19 12:06 ` Wolfram Sang 0 siblings, 1 reply; 6+ messages in thread From: Corey Minyard @ 2016-04-27 14:28 UTC (permalink / raw) To: Jean Delvare, Wolfram Sang; +Cc: linux-i2c, Corey Minyard 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 */ > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c-smbus: Don't report duplicate alerts 2016-04-27 14:28 ` Corey Minyard @ 2016-06-19 12:06 ` Wolfram Sang 2016-06-20 9:31 ` Benjamin Tissoires 0 siblings, 1 reply; 6+ messages in thread From: Wolfram Sang @ 2016-06-19 12:06 UTC (permalink / raw) To: Corey Minyard; +Cc: Jean Delvare, linux-i2c, Corey Minyard, Benjamin Tissoires [-- Attachment #1: Type: text/plain, Size: 3784 bytes --] 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 */ > > > [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 819 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] i2c-smbus: Don't report duplicate alerts 2016-06-19 12:06 ` Wolfram Sang @ 2016-06-20 9:31 ` Benjamin Tissoires 0 siblings, 0 replies; 6+ messages in thread From: Benjamin Tissoires @ 2016-06-20 9:31 UTC (permalink / raw) To: Wolfram Sang; +Cc: Corey Minyard, Jean Delvare, linux-i2c, Corey Minyard 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 */ > > > > > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2016-06-20 9:31 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-01-19 17:00 [PATCH] i2c-smbus: Don't report duplicate alerts minyard 2016-03-03 20:57 ` Wolfram Sang 2016-03-04 10:15 ` Jean Delvare 2016-04-27 14:28 ` Corey Minyard 2016-06-19 12:06 ` Wolfram Sang 2016-06-20 9:31 ` Benjamin Tissoires
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).