| Message ID | 20201223143644.33341-1-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | ACPI / scan: Don't create platform device for INT3515 ACPI nodes | expand |
On Wed, Dec 23, 2020 at 4:40 PM Heikki Krogerus <heikki.krogerus@linux.intel.com> wrote: > > There are several reports about the tps6598x causing > interrupt flood on boards with the INT3515 ACPI node, which > then causes instability. There appears to be several > problems with the interrupt. One problem is that the > I2CSerialBus resources do not always map to the Interrupt > resource with the same index, but that is not the only > problem. We have not been able to come up with a solution > for all the issues, and because of that disabling the device > for now. > > The PD controller on these platforms is autonomous, and the > purpose for the driver is primarily to supply status to the > userspace, so this will not affect any functionality. > > Reported-by: Moody Salem <moody@uniswap.org> > Fixes: a3dd034a1707 ("ACPI / scan: Create platform device for INT3515 ACPI nodes") > Cc: stable@vger.kernel.org > Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1883511 BugLink: ? > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > --- > drivers/platform/x86/i2c-multi-instantiate.c | 31 +++++++++++++++----- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c > index 6acc8457866e1..e1df665d3ad31 100644 > --- a/drivers/platform/x86/i2c-multi-instantiate.c > +++ b/drivers/platform/x86/i2c-multi-instantiate.c > @@ -166,13 +166,29 @@ static const struct i2c_inst_data bsg2150_data[] = { > {} > }; > > -static const struct i2c_inst_data int3515_data[] = { > - { "tps6598x", IRQ_RESOURCE_APIC, 0 }, > - { "tps6598x", IRQ_RESOURCE_APIC, 1 }, > - { "tps6598x", IRQ_RESOURCE_APIC, 2 }, > - { "tps6598x", IRQ_RESOURCE_APIC, 3 }, > - {} > -}; > +/* > + * Device with _HID INT3515 (TI PD controllers) has some unresolved interrupt > + * issues. The most common problem seen is interrupt flood. > + * > + * There are at least two known causes. Firstly, on some boards, the > + * I2CSerialBus resource index does not match the Interrupt resource, i.e. they > + * are not one-to-one mapped like in the array below. Secondly, on some boards > + * the irq line from the PD controller is not actually connected at all. But the irq -> IRQ > + * interrupt flood is also seen on some boards where those are not a problem, so > + * there are some other problems as well. > + * > + * Because of the issues with the interrupt, the device is disabled for now. If > + * you wish to debug the issues, uncomment the below, and add an entry for the > + * INT3515 device to the i2c_multi_instance__ids table. Extra _ (underscore). > + * > + * static const struct i2c_inst_data int3515_data[] = { > + * { "tps6598x", IRQ_RESOURCE_APIC, 0 }, > + * { "tps6598x", IRQ_RESOURCE_APIC, 1 }, > + * { "tps6598x", IRQ_RESOURCE_APIC, 2 }, > + * { "tps6598x", IRQ_RESOURCE_APIC, 3 }, > + * { } > + * }; > + */ > > /* > * Note new device-ids must also be added to i2c_multi_instantiate_ids in > @@ -181,7 +197,6 @@ static const struct i2c_inst_data int3515_data[] = { > static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = { > { "BSG1160", (unsigned long)bsg1160_data }, > { "BSG2150", (unsigned long)bsg2150_data }, > - { "INT3515", (unsigned long)int3515_data }, Perhaps also comment it out and refer to the above note? > { } > }; > MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids); > -- > 2.29.2 >
Hi, On 12/23/20 3:36 PM, Heikki Krogerus wrote: > There are several reports about the tps6598x causing > interrupt flood on boards with the INT3515 ACPI node, which > then causes instability. There appears to be several > problems with the interrupt. One problem is that the > I2CSerialBus resources do not always map to the Interrupt > resource with the same index, but that is not the only > problem. We have not been able to come up with a solution > for all the issues, and because of that disabling the device > for now. > > The PD controller on these platforms is autonomous, and the > purpose for the driver is primarily to supply status to the > userspace, so this will not affect any functionality. > > Reported-by: Moody Salem <moody@uniswap.org> > Fixes: a3dd034a1707 ("ACPI / scan: Create platform device for INT3515 ACPI nodes") > Cc: stable@vger.kernel.org > Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1883511 > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> Thank you for your patch, I've applied this patch to my review-hans branch: https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans Note it will show up in my review-hans branch once I've pushed my local branch there, which might take a while. Once I've run some tests on this branch the patches there will be added to the platform-drivers-x86/for-next branch and eventually will be included in the pdx86 pull-request to Linus for the next merge-window. Regards, Hans > --- > drivers/platform/x86/i2c-multi-instantiate.c | 31 +++++++++++++++----- > 1 file changed, 23 insertions(+), 8 deletions(-) > > diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c > index 6acc8457866e1..e1df665d3ad31 100644 > --- a/drivers/platform/x86/i2c-multi-instantiate.c > +++ b/drivers/platform/x86/i2c-multi-instantiate.c > @@ -166,13 +166,29 @@ static const struct i2c_inst_data bsg2150_data[] = { > {} > }; > > -static const struct i2c_inst_data int3515_data[] = { > - { "tps6598x", IRQ_RESOURCE_APIC, 0 }, > - { "tps6598x", IRQ_RESOURCE_APIC, 1 }, > - { "tps6598x", IRQ_RESOURCE_APIC, 2 }, > - { "tps6598x", IRQ_RESOURCE_APIC, 3 }, > - {} > -}; > +/* > + * Device with _HID INT3515 (TI PD controllers) has some unresolved interrupt > + * issues. The most common problem seen is interrupt flood. > + * > + * There are at least two known causes. Firstly, on some boards, the > + * I2CSerialBus resource index does not match the Interrupt resource, i.e. they > + * are not one-to-one mapped like in the array below. Secondly, on some boards > + * the irq line from the PD controller is not actually connected at all. But the > + * interrupt flood is also seen on some boards where those are not a problem, so > + * there are some other problems as well. > + * > + * Because of the issues with the interrupt, the device is disabled for now. If > + * you wish to debug the issues, uncomment the below, and add an entry for the > + * INT3515 device to the i2c_multi_instance__ids table. > + * > + * static const struct i2c_inst_data int3515_data[] = { > + * { "tps6598x", IRQ_RESOURCE_APIC, 0 }, > + * { "tps6598x", IRQ_RESOURCE_APIC, 1 }, > + * { "tps6598x", IRQ_RESOURCE_APIC, 2 }, > + * { "tps6598x", IRQ_RESOURCE_APIC, 3 }, > + * { } > + * }; > + */ > > /* > * Note new device-ids must also be added to i2c_multi_instantiate_ids in > @@ -181,7 +197,6 @@ static const struct i2c_inst_data int3515_data[] = { > static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = { > { "BSG1160", (unsigned long)bsg1160_data }, > { "BSG2150", (unsigned long)bsg2150_data }, > - { "INT3515", (unsigned long)int3515_data }, > { } > }; > MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids); >
On Mon, Jan 04, 2021 at 12:59:39PM +0100, Hans de Goede wrote: > On 12/23/20 3:36 PM, Heikki Krogerus wrote: > > There are several reports about the tps6598x causing > > interrupt flood on boards with the INT3515 ACPI node, which > > then causes instability. There appears to be several > > problems with the interrupt. One problem is that the > > I2CSerialBus resources do not always map to the Interrupt > > resource with the same index, but that is not the only > > problem. We have not been able to come up with a solution > > for all the issues, and because of that disabling the device > > for now. > > > > The PD controller on these platforms is autonomous, and the > > purpose for the driver is primarily to supply status to the > > userspace, so this will not affect any functionality. > > > > Reported-by: Moody Salem <moody@uniswap.org> > > Fixes: a3dd034a1707 ("ACPI / scan: Create platform device for INT3515 ACPI nodes") > > Cc: stable@vger.kernel.org > > Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1883511 > > Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Thank you for your patch, I've applied this patch to my review-hans > branch: > https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans > > Note it will show up in my review-hans branch once I've pushed my > local branch there, which might take a while. > > Once I've run some tests on this branch the patches there will be > added to the platform-drivers-x86/for-next branch and eventually > will be included in the pdx86 pull-request to Linus for the next > merge-window. I'm wondering if my reply has been seen... https://lore.kernel.org/platform-driver-x86/ae94a191-4273-0000-deda-4859034343b8@redhat.com/T/#m30308ca22cd0ce266aa6913ab7ef1fc56b3279de
Hi, On 1/4/21 1:23 PM, Andy Shevchenko wrote: > On Mon, Jan 04, 2021 at 12:59:39PM +0100, Hans de Goede wrote: >> On 12/23/20 3:36 PM, Heikki Krogerus wrote: >>> There are several reports about the tps6598x causing >>> interrupt flood on boards with the INT3515 ACPI node, which >>> then causes instability. There appears to be several >>> problems with the interrupt. One problem is that the >>> I2CSerialBus resources do not always map to the Interrupt >>> resource with the same index, but that is not the only >>> problem. We have not been able to come up with a solution >>> for all the issues, and because of that disabling the device >>> for now. >>> >>> The PD controller on these platforms is autonomous, and the >>> purpose for the driver is primarily to supply status to the >>> userspace, so this will not affect any functionality. >>> >>> Reported-by: Moody Salem <moody@uniswap.org> >>> Fixes: a3dd034a1707 ("ACPI / scan: Create platform device for INT3515 ACPI nodes") >>> Cc: stable@vger.kernel.org >>> Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1883511 >>> Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> >> >> Thank you for your patch, I've applied this patch to my review-hans >> branch: >> https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans >> >> Note it will show up in my review-hans branch once I've pushed my >> local branch there, which might take a while. >> >> Once I've run some tests on this branch the patches there will be >> added to the platform-drivers-x86/for-next branch and eventually >> will be included in the pdx86 pull-request to Linus for the next >> merge-window. > > I'm wondering if my reply has been seen... > > https://lore.kernel.org/platform-driver-x86/ae94a191-4273-0000-deda-4859034343b8@redhat.com/T/#m30308ca22cd0ce266aa6913ab7ef1fc56b3279de Yes I've done the s/Link/BugLink/ in the commit msg and fixed up the typo-s in the comment block locally. I should have mentioned that in my reply instead of just blindly using the template-reply which I have for this, sorry; and thank you for the review. Regards, Hans
On Mon, Jan 4, 2021 at 4:01 PM Hans de Goede <hdegoede@redhat.com> wrote: > On 1/4/21 1:23 PM, Andy Shevchenko wrote: ... > > I'm wondering if my reply has been seen... > > > > https://lore.kernel.org/platform-driver-x86/ae94a191-4273-0000-deda-4859034343b8@redhat.com/T/#m30308ca22cd0ce266aa6913ab7ef1fc56b3279de > > Yes I've done the s/Link/BugLink/ in the commit msg and fixed up the > typo-s in the comment block locally. I should have mentioned that in > my reply instead of just blindly using the template-reply which I have for > this, sorry; and thank you for the review. Ah, thanks! No problem.
diff --git a/drivers/platform/x86/i2c-multi-instantiate.c b/drivers/platform/x86/i2c-multi-instantiate.c index 6acc8457866e1..e1df665d3ad31 100644 --- a/drivers/platform/x86/i2c-multi-instantiate.c +++ b/drivers/platform/x86/i2c-multi-instantiate.c @@ -166,13 +166,29 @@ static const struct i2c_inst_data bsg2150_data[] = { {} }; -static const struct i2c_inst_data int3515_data[] = { - { "tps6598x", IRQ_RESOURCE_APIC, 0 }, - { "tps6598x", IRQ_RESOURCE_APIC, 1 }, - { "tps6598x", IRQ_RESOURCE_APIC, 2 }, - { "tps6598x", IRQ_RESOURCE_APIC, 3 }, - {} -}; +/* + * Device with _HID INT3515 (TI PD controllers) has some unresolved interrupt + * issues. The most common problem seen is interrupt flood. + * + * There are at least two known causes. Firstly, on some boards, the + * I2CSerialBus resource index does not match the Interrupt resource, i.e. they + * are not one-to-one mapped like in the array below. Secondly, on some boards + * the irq line from the PD controller is not actually connected at all. But the + * interrupt flood is also seen on some boards where those are not a problem, so + * there are some other problems as well. + * + * Because of the issues with the interrupt, the device is disabled for now. If + * you wish to debug the issues, uncomment the below, and add an entry for the + * INT3515 device to the i2c_multi_instance__ids table. + * + * static const struct i2c_inst_data int3515_data[] = { + * { "tps6598x", IRQ_RESOURCE_APIC, 0 }, + * { "tps6598x", IRQ_RESOURCE_APIC, 1 }, + * { "tps6598x", IRQ_RESOURCE_APIC, 2 }, + * { "tps6598x", IRQ_RESOURCE_APIC, 3 }, + * { } + * }; + */ /* * Note new device-ids must also be added to i2c_multi_instantiate_ids in @@ -181,7 +197,6 @@ static const struct i2c_inst_data int3515_data[] = { static const struct acpi_device_id i2c_multi_inst_acpi_ids[] = { { "BSG1160", (unsigned long)bsg1160_data }, { "BSG2150", (unsigned long)bsg2150_data }, - { "INT3515", (unsigned long)int3515_data }, { } }; MODULE_DEVICE_TABLE(acpi, i2c_multi_inst_acpi_ids);
There are several reports about the tps6598x causing interrupt flood on boards with the INT3515 ACPI node, which then causes instability. There appears to be several problems with the interrupt. One problem is that the I2CSerialBus resources do not always map to the Interrupt resource with the same index, but that is not the only problem. We have not been able to come up with a solution for all the issues, and because of that disabling the device for now. The PD controller on these platforms is autonomous, and the purpose for the driver is primarily to supply status to the userspace, so this will not affect any functionality. Reported-by: Moody Salem <moody@uniswap.org> Fixes: a3dd034a1707 ("ACPI / scan: Create platform device for INT3515 ACPI nodes") Cc: stable@vger.kernel.org Link: https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1883511 Signed-off-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> --- drivers/platform/x86/i2c-multi-instantiate.c | 31 +++++++++++++++----- 1 file changed, 23 insertions(+), 8 deletions(-)