| Message ID | 20191121101051.71859534@endymion |
|---|---|
| State | Accepted |
| Headers | show |
| Series | i2c: i2c-smbus: Don't filter out duplicate alerts | expand |
On Thu, Nov 21, 2019 at 10:10:51AM +0100, Jean Delvare 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. > > [JD: Fixed subject] > > Signed-off-by: Corey Minyard <cminyard@mvista.com> > Signed-off-by: Jean Delvare <jdelvare@suse.de> > --- > That's a 4-year-old patch from Corey which I stumbled upon this > morning. I was supposed to test it on my ADM1032 evaluation board but > never got to it. Sorry about that. It turns out that I no longer have > any system with a parallel port to test it. Thanks. This hasn't been a huge deal, since the only system that is affected at the moment (AFAIK) is the qemu test environment that I use. But it will be nice to have this for when something real actually uses this. -corey > > I think the patch is correct and whatever the problem was on my ADM1032 > evaluation board, it should be fixed differently. Maybe it was a wrong > trigger type, or alerts must be disabled temporarily during processing, > or the hardware is actually bogus and it would be up to the device > driver to ignore alerts for some time after receiving one. Whatever, > let's apply the fix now and deal with this problem later if/when it > resurfaces. > > 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
On Fri, Nov 22, 2019 at 4:27 AM Corey Minyard <cminyard@mvista.com> wrote: > > On Thu, Nov 21, 2019 at 10:10:51AM +0100, Jean Delvare 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. > > > > [JD: Fixed subject] > > > > Signed-off-by: Corey Minyard <cminyard@mvista.com> > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > > --- > > That's a 4-year-old patch from Corey which I stumbled upon this > > morning. I was supposed to test it on my ADM1032 evaluation board but > > never got to it. Sorry about that. It turns out that I no longer have > > any system with a parallel port to test it. > > Thanks. This hasn't been a huge deal, since the only system that is > affected at the moment (AFAIK) is the qemu test environment that I use. > But it will be nice to have this for when something real actually uses > this. > > -corey > > > > > I think the patch is correct and whatever the problem was on my ADM1032 > > evaluation board, it should be fixed differently. Maybe it was a wrong > > trigger type, or alerts must be disabled temporarily during processing, > > or the hardware is actually bogus and it would be up to the device > > driver to ignore alerts for some time after receiving one. Whatever, > > let's apply the fix now and deal with this problem later if/when it > > resurfaces. > > > > 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; > > - } I assume that the SMB alert client (ara) is the one responsible for sending the data.addr, and so it would make sense to consider that as long as the client sends the address, there is something to process. With that (probably dumb) comment, the patch is: Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com> Note that I have no SMBus alert device to test this, but the patch looks correct to me. Cheers, Benjamin > > 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 >
On Thu, Nov 21, 2019 at 10:10:51AM +0100, Jean Delvare 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. > > [JD: Fixed subject] > > Signed-off-by: Corey Minyard <cminyard@mvista.com> > Signed-off-by: Jean Delvare <jdelvare@suse.de> Applied to for-next (i.e. for 5.5.), thanks!
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 */