| Message ID | 20201009144047.505957-1-benjamin@sipsolutions.net |
|---|---|
| Headers | show |
| Series | UCSI race condition resulting in wrong port state | expand |
On Fri, Oct 09, 2020 at 04:40:45PM +0200, Benjamin Berg wrote: > From: Benjamin Berg <bberg@redhat.com> > > Hi all, > > so, I kept running in an issue where the UCSI port information was saying > that power was being delivered (online: 1), while no cable was attached. > > The core of the problem is that there are scenarios where UCSI change > notifications are lost. This happens because querying the changes that > happened is done using the GET_CONNECTOR_STATUS command while clearing the > bitfield happens from the separate ACK command. Any change in between will > be lost. > > Note that the problem may be almost invisible in the UI as e.g. GNOME will > still show the battery as discharging. But some policies like automatic > suspend may be applied incorrectly. > > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> Both patches: Acked-by: Heikki Krogerus <heikki.krogerus@linux.intel.com> > Benjamin Berg (2): > usb: typec: ucsi: acpi: Always decode connector change information > usb: typec: ucsi: Work around PPM losing change information > > drivers/usb/typec/ucsi/ucsi.c | 125 ++++++++++++++++++++++++----- > drivers/usb/typec/ucsi/ucsi.h | 2 + > drivers/usb/typec/ucsi/ucsi_acpi.c | 5 +- > 3 files changed, 110 insertions(+), 22 deletions(-) thanks,
On Fri, Oct 09, 2020 at 04:40:45PM +0200, Benjamin Berg wrote: > From: Benjamin Berg <bberg@redhat.com> > > Hi all, > > so, I kept running in an issue where the UCSI port information was saying > that power was being delivered (online: 1), while no cable was attached. > > The core of the problem is that there are scenarios where UCSI change > notifications are lost. This happens because querying the changes that > happened is done using the GET_CONNECTOR_STATUS command while clearing the > bitfield happens from the separate ACK command. Any change in between will > be lost. > > Note that the problem may be almost invisible in the UI as e.g. GNOME will > still show the battery as discharging. But some policies like automatic > suspend may be applied incorrectly. > > Cc: Hans de Goede <hdegoede@redhat.com> > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > Benjamin Berg (2): > usb: typec: ucsi: acpi: Always decode connector change information > usb: typec: ucsi: Work around PPM losing change information Do these need to be backported to stable kernel releases? If so, how far back? thjanks, greg k-h
On Wed, Oct 28, 2020 at 10:10:43AM +0100, Greg Kroah-Hartman wrote: > On Fri, Oct 09, 2020 at 04:40:45PM +0200, Benjamin Berg wrote: > > From: Benjamin Berg <bberg@redhat.com> > > > > Hi all, > > > > so, I kept running in an issue where the UCSI port information was saying > > that power was being delivered (online: 1), while no cable was attached. > > > > The core of the problem is that there are scenarios where UCSI change > > notifications are lost. This happens because querying the changes that > > happened is done using the GET_CONNECTOR_STATUS command while clearing the > > bitfield happens from the separate ACK command. Any change in between will > > be lost. > > > > Note that the problem may be almost invisible in the UI as e.g. GNOME will > > still show the battery as discharging. But some policies like automatic > > suspend may be applied incorrectly. > > > > Cc: Hans de Goede <hdegoede@redhat.com> > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > Benjamin Berg (2): > > usb: typec: ucsi: acpi: Always decode connector change information > > usb: typec: ucsi: Work around PPM losing change information > > Do these need to be backported to stable kernel releases? If so, how > far back? Due to the lack of response, I guess they don't need to go to any stable kernel, so will queue them up for 5.11-rc1. thanks, greg k-h
Hi, On Fri, 2020-11-06 at 11:47 +0100, Greg Kroah-Hartman wrote: > Due to the lack of response, I guess they don't need to go to any > stable kernel, so will queue them up for 5.11-rc1. Sorry, forgot to reply. Not including them in stable seems reasonable as I have not seen it cause major trouble in the wild so far (i.e. only one user report that seems related). Benjamin
Hi Greg, On Fri, Nov 06, 2020 at 11:47:25AM +0100, Greg Kroah-Hartman wrote: > On Wed, Oct 28, 2020 at 10:10:43AM +0100, Greg Kroah-Hartman wrote: > > On Fri, Oct 09, 2020 at 04:40:45PM +0200, Benjamin Berg wrote: > > > From: Benjamin Berg <bberg@redhat.com> > > > > > > Hi all, > > > > > > so, I kept running in an issue where the UCSI port information was saying > > > that power was being delivered (online: 1), while no cable was attached. > > > > > > The core of the problem is that there are scenarios where UCSI change > > > notifications are lost. This happens because querying the changes that > > > happened is done using the GET_CONNECTOR_STATUS command while clearing the > > > bitfield happens from the separate ACK command. Any change in between will > > > be lost. > > > > > > Note that the problem may be almost invisible in the UI as e.g. GNOME will > > > still show the battery as discharging. But some policies like automatic > > > suspend may be applied incorrectly. > > > > > > Cc: Hans de Goede <hdegoede@redhat.com> > > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > > > Benjamin Berg (2): > > > usb: typec: ucsi: acpi: Always decode connector change information > > > usb: typec: ucsi: Work around PPM losing change information > > > > Do these need to be backported to stable kernel releases? If so, how > > far back? > > Due to the lack of response, I guess they don't need to go to any stable > kernel, so will queue them up for 5.11-rc1. At least one user in Debian (https://bugs.debian.org/992004) would be happy to have those backported as well to the 5.10.y series (which we will pick up). So if Benjamin ack's this, this would be great to have in 5.10.y. Regards, Salvatore
Hi, On Fri, 2021-08-20 at 15:01 +0200, Salvatore Bonaccorso wrote: > Hi Greg, > > On Fri, Nov 06, 2020 at 11:47:25AM +0100, Greg Kroah-Hartman wrote: > > On Wed, Oct 28, 2020 at 10:10:43AM +0100, Greg Kroah-Hartman wrote: > > > On Fri, Oct 09, 2020 at 04:40:45PM +0200, Benjamin Berg wrote: > > > > From: Benjamin Berg <bberg@redhat.com> > > > > > > > > Hi all, > > > > > > > > so, I kept running in an issue where the UCSI port information was saying > > > > that power was being delivered (online: 1), while no cable was attached. > > > > > > > > The core of the problem is that there are scenarios where UCSI change > > > > notifications are lost. This happens because querying the changes that > > > > happened is done using the GET_CONNECTOR_STATUS command while clearing the > > > > bitfield happens from the separate ACK command. Any change in between will > > > > be lost. > > > > > > > > Note that the problem may be almost invisible in the UI as e.g. GNOME will > > > > still show the battery as discharging. But some policies like automatic > > > > suspend may be applied incorrectly. > > > > > > > > Cc: Hans de Goede <hdegoede@redhat.com> > > > > Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> > > > > > > > > Benjamin Berg (2): > > > > usb: typec: ucsi: acpi: Always decode connector change information > > > > usb: typec: ucsi: Work around PPM losing change information > > > > > > Do these need to be backported to stable kernel releases? If so, how > > > far back? > > > > Due to the lack of response, I guess they don't need to go to any stable > > kernel, so will queue them up for 5.11-rc1. > > At least one user in Debian (https://bugs.debian.org/992004) would be > happy to have those backported as well to the 5.10.y series (which we > will pick up). > > So if Benjamin ack's this, this would be great to have in 5.10.y. Sure, it is reasonable to pull it into 5.10. At the time it just seemed to me that it was enough of a corner case to not bother. Note that there was a somewhat related fix later on (for Qualcomm UCSI firmware), which probably makes sense to pull in too then. Including Bjorn into the CC list for that. commit 8c9b3caab3ac26db1da00b8117901640c55a69dd Author: Bjorn Andersson <bjorn.andersson@linaro.org> Date: Sat May 15 21:09:53 2021 -0700 usb: typec: ucsi: Clear pending after acking connector change Benjamin
On 8/20/21 9:29 AM, Benjamin Berg wrote: > On Fri, 2021-08-20 at 15:01 +0200, Salvatore Bonaccorso wrote: >> At least one user in Debian (https://bugs.debian.org/992004) would be >> happy to have those backported as well to the 5.10.y series (which we >> will pick up). >> >> So if Benjamin ack's this, this would be great to have in 5.10.y. > Sure, it is reasonable to pull it into 5.10. At the time it just seemed > to me that it was enough of a corner case to not bother. > > Note that there was a somewhat related fix later on (for Qualcomm UCSI > firmware), which probably makes sense to pull in too then. > > Including Bjorn into the CC list for that. > > commit 8c9b3caab3ac26db1da00b8117901640c55a69dd > Author: Bjorn Andersson <bjorn.andersson@linaro.org> > Date: Sat May 15 21:09:53 2021 -0700 > > usb: typec: ucsi: Clear pending after acking connector change > > Benjamin I feel that I should mention that I haven't actually tested this change, so it's just conjecture on my part that it would fix my issue (though it does seem to track pretty closely). I am happy to do that testing if it would save others time. Ian Turner
Hi Ian, On Fri, Aug 20, 2021 at 07:08:57PM -0400, Ian Turner wrote: > On 8/20/21 9:29 AM, Benjamin Berg wrote: > > On Fri, 2021-08-20 at 15:01 +0200, Salvatore Bonaccorso wrote: > > > At least one user in Debian (https://bugs.debian.org/992004) would be > > > happy to have those backported as well to the 5.10.y series (which we > > > will pick up). > > > > > > So if Benjamin ack's this, this would be great to have in 5.10.y. > > Sure, it is reasonable to pull it into 5.10. At the time it just seemed > > to me that it was enough of a corner case to not bother. > > > > Note that there was a somewhat related fix later on (for Qualcomm UCSI > > firmware), which probably makes sense to pull in too then. > > > > Including Bjorn into the CC list for that. > > > > commit 8c9b3caab3ac26db1da00b8117901640c55a69dd > > Author: Bjorn Andersson <bjorn.andersson@linaro.org> > > Date: Sat May 15 21:09:53 2021 -0700 > > > > usb: typec: ucsi: Clear pending after acking connector change > > Benjamin > > I feel that I should mention that I haven't actually tested this change, so > it's just conjecture on my part that it would fix my issue (though it does > seem to track pretty closely). I am happy to do that testing if it would > save others time. Ah apologies, I was under the impression that you confirmed that this was already the right fix. It is pretty close to what you describe, so if you can additionally confirm the fix, that would be great. Regards, Salvatore
On Fri, Aug 20, 2021 at 03:01:53PM +0200, Salvatore Bonaccorso wrote: > Hi Greg, > > On Fri, Nov 06, 2020 at 11:47:25AM +0100, Greg Kroah-Hartman wrote: Note, you are responding to an email from a very long time ago... > > Due to the lack of response, I guess they don't need to go to any stable > > kernel, so will queue them up for 5.11-rc1. > > At least one user in Debian (https://bugs.debian.org/992004) would be > happy to have those backported as well to the 5.10.y series (which we > will pick up). > > So if Benjamin ack's this, this would be great to have in 5.10.y. What are the git commit ids? Just ask for them to be applied to stable like normal... thanks, greg k-h
Hi Greg, On Sat, Aug 21, 2021 at 02:09:45PM +0200, Greg Kroah-Hartman wrote: > On Fri, Aug 20, 2021 at 03:01:53PM +0200, Salvatore Bonaccorso wrote: > > Hi Greg, > > > > On Fri, Nov 06, 2020 at 11:47:25AM +0100, Greg Kroah-Hartman wrote: > > Note, you are responding to an email from a very long time ago... Yeah, was sort of purpose :) (to try to retain the original context of the question of if the commits should be backported to stable series, which back then had no need raised) > > > > Due to the lack of response, I guess they don't need to go to any stable > > > kernel, so will queue them up for 5.11-rc1. > > > > At least one user in Debian (https://bugs.debian.org/992004) would be > > happy to have those backported as well to the 5.10.y series (which we > > will pick up). > > > > So if Benjamin ack's this, this would be great to have in 5.10.y. > > What are the git commit ids? Just ask for them to be applied to stable > like normal... Right, aplogies. The two commits were 47ea2929d58c35598e681212311d35b240c373ce and 217504a055325fe76ec1142aa15f14d3db77f94f. 47ea2929d58c ("usb: typec: ucsi: acpi: Always decode connector change information") 217504a05532 ("usb: typec: ucsi: Work around PPM losing change information") and in the followup Benjamin Berg mentioned to pick as well 8c9b3caab3ac26db1da00b8117901640c55a69dd 8c9b3caab3ac ("usb: typec: ucsi: Clear pending after acking connector change" a related fix later on. Regards, Salvatore
On 8/21/21 2:31 AM, Salvatore Bonaccorso wrote: > Ah apologies, I was under the impression that you confirmed that this > was already the right fix. It is pretty close to what you describe, so > if you can additionally confirm the fix, that would be great. > > Regards, > Salvatore Well, it would seem that I owe everyone here an apology. Not only did I not observe this issue with a patched kernel, but I'm also now unable to reproduce it booting into a stock kernel. I can only speculate about the reasons for this. I'll come back here if I learn more. Sorry about the noise. Ian Turner
On Sat, Aug 21, 2021 at 03:01:26PM +0200, Salvatore Bonaccorso wrote: > Hi Greg, > > On Sat, Aug 21, 2021 at 02:09:45PM +0200, Greg Kroah-Hartman wrote: > > On Fri, Aug 20, 2021 at 03:01:53PM +0200, Salvatore Bonaccorso wrote: > > > Hi Greg, > > > > > > On Fri, Nov 06, 2020 at 11:47:25AM +0100, Greg Kroah-Hartman wrote: > > > > Note, you are responding to an email from a very long time ago... > > Yeah, was sort of purpose :) (to try to retain the original context of > the question of if the commits should be backported to stable series, > which back then had no need raised) > > > > > > Due to the lack of response, I guess they don't need to go to any stable > > > > kernel, so will queue them up for 5.11-rc1. > > > > > > At least one user in Debian (https://bugs.debian.org/992004) would be > > > happy to have those backported as well to the 5.10.y series (which we > > > will pick up). > > > > > > So if Benjamin ack's this, this would be great to have in 5.10.y. > > > > What are the git commit ids? Just ask for them to be applied to stable > > like normal... > > Right, aplogies. The two commits were > 47ea2929d58c35598e681212311d35b240c373ce and > 217504a055325fe76ec1142aa15f14d3db77f94f. > > 47ea2929d58c ("usb: typec: ucsi: acpi: Always decode connector change information") > 217504a05532 ("usb: typec: ucsi: Work around PPM losing change information") > > and in the followup Benjamin Berg mentioned to pick as well > > 8c9b3caab3ac26db1da00b8117901640c55a69dd > > 8c9b3caab3ac ("usb: typec: ucsi: Clear pending after acking connector change" > > a related fix later on. All now queued up, thanks. greg k-h
From: Benjamin Berg <bberg@redhat.com> Hi all, so, I kept running in an issue where the UCSI port information was saying that power was being delivered (online: 1), while no cable was attached. The core of the problem is that there are scenarios where UCSI change notifications are lost. This happens because querying the changes that happened is done using the GET_CONNECTOR_STATUS command while clearing the bitfield happens from the separate ACK command. Any change in between will be lost. Note that the problem may be almost invisible in the UI as e.g. GNOME will still show the battery as discharging. But some policies like automatic suspend may be applied incorrectly. Cc: Hans de Goede <hdegoede@redhat.com> Cc: Heikki Krogerus <heikki.krogerus@linux.intel.com> Benjamin Berg (2): usb: typec: ucsi: acpi: Always decode connector change information usb: typec: ucsi: Work around PPM losing change information drivers/usb/typec/ucsi/ucsi.c | 125 ++++++++++++++++++++++++----- drivers/usb/typec/ucsi/ucsi.h | 2 + drivers/usb/typec/ucsi/ucsi_acpi.c | 5 +- 3 files changed, 110 insertions(+), 22 deletions(-)