The ISRs of the tps25750 and tps6598x do not handle generated events
properly under all circumstances.
The tps6598x ISR does not read all bits of the INT_EVENTX registers,
leaving events signaled with bits above 64 unattended. Moreover, these
events are not cleared, leaving the interrupt enabled.
The tps25750 reads all bits of the INT_EVENT1 register, but the event
checking is not right because the same event is checked in two different
regions of the same register by means of an OR operation.
This series aims to fix both issues by reading all bits of the
INT_EVENTX registers, and limiting the event checking to the region
where the supported events are defined (currently they are limited to
the first 64 bits of the registers, as the are defined as BIT_ULL()).
If the need for events above the first 64 bits of the INT_EVENTX
registers arises, a different mechanism might be required. But for the
current needs, all definitions can be left as they are.
Note: resend to add 'stable' mailing list (fixes in the series).
Signed-off-by: Javier Carrasco <[email protected]>
---
Javier Carrasco (2):
usb: typec: tipd: fix event checking for tps25750
usb: typec: tipd: fix event checking for tps6598x
drivers/usb/typec/tipd/core.c | 37 +++++++++++++++++++++----------------
1 file changed, 21 insertions(+), 16 deletions(-)
---
base-commit: 4cece764965020c22cff7665b18a012006359095
change-id: 20240328-tps6598x_fix_event_handling-8fff3a6018d9
Best regards,
--
Javier Carrasco <[email protected]>
In its current form, the interrupt service routine of the tps25750
checks the event flags in the lowest 64 bits of the interrupt event
register (event[0]), but also in the upper part (event[1]).
Given that all flags are defined as BIT() or BIT_ULL(), they are
restricted to the first 64 bits of the INT_EVENT1 register. Including
the upper part of the register can lead to false positives e.g. if the
event 64 bits above the one being checked is set, but the one being
checked is not.
Restrict the flag checking to the first 64 bits of the INT_EVENT1
register.
Fixes: 7e7a3c815d22 ("USB: typec: tps6598x: Add TPS25750 support")
Signed-off-by: Javier Carrasco <[email protected]>
---
drivers/usb/typec/tipd/core.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
index 0717cfcd9f8c..7c2f01344860 100644
--- a/drivers/usb/typec/tipd/core.c
+++ b/drivers/usb/typec/tipd/core.c
@@ -604,11 +604,11 @@ static irqreturn_t tps25750_interrupt(int irq, void *data)
if (!tps6598x_read_status(tps, &status))
goto err_clear_ints;
- if ((event[0] | event[1]) & TPS_REG_INT_POWER_STATUS_UPDATE)
+ if (event[0] & TPS_REG_INT_POWER_STATUS_UPDATE)
if (!tps6598x_read_power_status(tps))
goto err_clear_ints;
- if ((event[0] | event[1]) & TPS_REG_INT_DATA_STATUS_UPDATE)
+ if (event[0] & TPS_REG_INT_DATA_STATUS_UPDATE)
if (!tps6598x_read_data_status(tps))
goto err_clear_ints;
@@ -617,7 +617,7 @@ static irqreturn_t tps25750_interrupt(int irq, void *data)
* a plug event. Therefore, we need to check
* for pr/dr status change to set TypeC dr/pr accordingly.
*/
- if ((event[0] | event[1]) & TPS_REG_INT_PLUG_EVENT ||
+ if (event[0] & TPS_REG_INT_PLUG_EVENT ||
tps6598x_has_role_changed(tps, status))
tps6598x_handle_plug_event(tps, status);
--
2.40.1
On 3/28/24 17:44, Greg Kroah-Hartman wrote:
> On Thu, Mar 28, 2024 at 05:25:20PM +0100, Javier Carrasco wrote:
>> The ISRs of the tps25750 and tps6598x do not handle generated events
>> properly under all circumstances.
>>
>> The tps6598x ISR does not read all bits of the INT_EVENTX registers,
>> leaving events signaled with bits above 64 unattended. Moreover, these
>> events are not cleared, leaving the interrupt enabled.
>>
>> The tps25750 reads all bits of the INT_EVENT1 register, but the event
>> checking is not right because the same event is checked in two different
>> regions of the same register by means of an OR operation.
>>
>> This series aims to fix both issues by reading all bits of the
>> INT_EVENTX registers, and limiting the event checking to the region
>> where the supported events are defined (currently they are limited to
>> the first 64 bits of the registers, as the are defined as BIT_ULL()).
>>
>> If the need for events above the first 64 bits of the INT_EVENTX
>> registers arises, a different mechanism might be required. But for the
>> current needs, all definitions can be left as they are.
>>
>> Note: resend to add 'stable' mailing list (fixes in the series).
>
>
> <formletter>
>
> This is not the correct way to submit patches for inclusion in the
> stable kernel tree. Please read:
> https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
> for how to do this properly.
>
> </formletter>
Sorry, I will add [email protected] right above the Signed-off-by:
instead of adding to the Cc list and resend again.
On Thu, Mar 28, 2024 at 05:25:20PM +0100, Javier Carrasco wrote:
> The ISRs of the tps25750 and tps6598x do not handle generated events
> properly under all circumstances.
>
> The tps6598x ISR does not read all bits of the INT_EVENTX registers,
> leaving events signaled with bits above 64 unattended. Moreover, these
> events are not cleared, leaving the interrupt enabled.
>
> The tps25750 reads all bits of the INT_EVENT1 register, but the event
> checking is not right because the same event is checked in two different
> regions of the same register by means of an OR operation.
>
> This series aims to fix both issues by reading all bits of the
> INT_EVENTX registers, and limiting the event checking to the region
> where the supported events are defined (currently they are limited to
> the first 64 bits of the registers, as the are defined as BIT_ULL()).
>
> If the need for events above the first 64 bits of the INT_EVENTX
> registers arises, a different mechanism might be required. But for the
> current needs, all definitions can be left as they are.
>
> Note: resend to add 'stable' mailing list (fixes in the series).
<formletter>
This is not the correct way to submit patches for inclusion in the
stable kernel tree. Please read:
https://www.kernel.org/doc/html/latest/process/stable-kernel-rules.html
for how to do this properly.
</formletter>