| Message ID | 20210920142419.54493-1-heikki.krogerus@linux.intel.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | usb: typec: ucsi: Driver improvements | expand |
Hi, On Mon, 2021-09-20 at 17:24 +0300, Heikki Krogerus wrote: > The goal of this series was to improve the alt mode handling in the > driver, but now it seems that we can use the "poll worker" that was > introduced for that to handle other tasks better as well. > > Ulrich reported some problems that are caused by the second > GET_CONNECTOR_STATUS right after the first one that was introduced in > 217504a05532 ("usb: typec: ucsi: Work around PPM losing change > information"). In the last patch I try to improve that workaround by > extracting it out of the generic event handler into its own task and > executing it only when it's really needed. That seems to improve the > situation. > > These patches definitely improve the quality of the driver by making > it a bit more readable, but they also appear to make the behaviour a > bit more predictably and uniform on different platforms. > > Benjamin, can you test these? I just gave this a spin on a X1 Carbon Gen 8 with a Lenovo TB 3 Dock. Unfortunately, I can still reproduce the issue occasionally. My take is that the rate is much lower than it was before my patch was introduced. However, unfortunately the patchset does appear to cause a regression on the machine I tested. As before. The "online" status of the UCSI power supply is reported as "1" occasionally even after the cable was unplugged. And the issue seems to only happens with a dock, not if I use a USB-C charger. Benjamin > Heikki Krogerus (7): > usb: typec: ucsi: Always cancel the command if PPM reports BUSY > condition > usb: typec: ucsi: Don't stop alt mode registration on busy condition > usb: typec: ucsi: Add polling mechanism for partner tasks like alt > mode checking > usb: typec: ucsi: acpi: Reduce the command completion timeout > usb: typec: ucsi: Check the partner alt modes always if there is PD > contract > usb: typec: ucsi: Read the PDOs in separate work > usb: typec: ucsi: Better fix for missing unplug events issue > > drivers/usb/typec/ucsi/ucsi.c | 337 ++++++++++++++--------------- > drivers/usb/typec/ucsi/ucsi.h | 3 +- > drivers/usb/typec/ucsi/ucsi_acpi.c | 2 +- > 3 files changed, 167 insertions(+), 175 deletions(-) >
Hi, Am 23.09.21 um 16:38 schrieb Benjamin Berg: > Hi, > > On Mon, 2021-09-20 at 17:24 +0300, Heikki Krogerus wrote: >> The goal of this series was to improve the alt mode handling in the >> driver, but now it seems that we can use the "poll worker" that was >> introduced for that to handle other tasks better as well. >> >> Ulrich reported some problems that are caused by the second >> GET_CONNECTOR_STATUS right after the first one that was introduced in >> 217504a05532 ("usb: typec: ucsi: Work around PPM losing change >> information"). In the last patch I try to improve that workaround by >> extracting it out of the generic event handler into its own task and >> executing it only when it's really needed. That seems to improve the >> situation. >> >> These patches definitely improve the quality of the driver by making >> it a bit more readable, but they also appear to make the behaviour a >> bit more predictably and uniform on different platforms. >> >> Benjamin, can you test these? > I just gave this a spin on a X1 Carbon Gen 8 with a Lenovo TB 3 Dock. > Unfortunately, I can still reproduce the issue occasionally. My take is > that the rate is much lower than it was before my patch was introduced. > However, unfortunately the patchset does appear to cause a regression > on the machine I tested. > > As before. The "online" status of the UCSI power supply is reported as > "1" occasionally even after the cable was unplugged. And the issue > seems to only happens with a dock, not if I use a USB-C charger. > > Benjamin From my point of view the patch set is still a huge improvement to the current state of the driver. Before it, the status of the UCSI power supply was unpredictable when using an USB-C charger with my Lenovo Yoga 9i. I do still get error messages in the kernel log right after waking from suspend occasionally, but I have not yet found reproducible steps. Most likely it has something to do with the controller being in an invalid state after waking from suspension. Though even then the status of the UCSI power supply is correct when this happens. Ulrich > >> Heikki Krogerus (7): >> usb: typec: ucsi: Always cancel the command if PPM reports BUSY >> condition >> usb: typec: ucsi: Don't stop alt mode registration on busy condition >> usb: typec: ucsi: Add polling mechanism for partner tasks like alt >> mode checking >> usb: typec: ucsi: acpi: Reduce the command completion timeout >> usb: typec: ucsi: Check the partner alt modes always if there is PD >> contract >> usb: typec: ucsi: Read the PDOs in separate work >> usb: typec: ucsi: Better fix for missing unplug events issue >> >> drivers/usb/typec/ucsi/ucsi.c | 337 ++++++++++++++--------------- >> drivers/usb/typec/ucsi/ucsi.h | 3 +- >> drivers/usb/typec/ucsi/ucsi_acpi.c | 2 +- >> 3 files changed, 167 insertions(+), 175 deletions(-) >>
On Thu, Sep 23, 2021 at 06:06:21PM +0200, Ulrich Huber wrote: > Hi, > > Am 23.09.21 um 16:38 schrieb Benjamin Berg: > > Hi, > > > > On Mon, 2021-09-20 at 17:24 +0300, Heikki Krogerus wrote: > > > The goal of this series was to improve the alt mode handling in the > > > driver, but now it seems that we can use the "poll worker" that was > > > introduced for that to handle other tasks better as well. > > > > > > Ulrich reported some problems that are caused by the second > > > GET_CONNECTOR_STATUS right after the first one that was introduced in > > > 217504a05532 ("usb: typec: ucsi: Work around PPM losing change > > > information"). In the last patch I try to improve that workaround by > > > extracting it out of the generic event handler into its own task and > > > executing it only when it's really needed. That seems to improve the > > > situation. > > > > > > These patches definitely improve the quality of the driver by making > > > it a bit more readable, but they also appear to make the behaviour a > > > bit more predictably and uniform on different platforms. > > > > > > Benjamin, can you test these? > > I just gave this a spin on a X1 Carbon Gen 8 with a Lenovo TB 3 Dock. > > Unfortunately, I can still reproduce the issue occasionally. My take is > > that the rate is much lower than it was before my patch was introduced. > > However, unfortunately the patchset does appear to cause a regression > > on the machine I tested. > > > > As before. The "online" status of the UCSI power supply is reported as > > "1" occasionally even after the cable was unplugged. And the issue > > seems to only happens with a dock, not if I use a USB-C charger. > > > > Benjamin > > From my point of view the patch set is still a huge improvement to the > current state of the driver. Before it, the status of the UCSI power supply > was unpredictable when using an USB-C charger with my Lenovo Yoga 9i. This is the problem with these workarounds that attempt to fix firmware issues. It's difficult to find a solution that works on every board. That's why it's important to attempt to isolate them, and use them only when needed. Right now the driver does behave quite unpredictable on several boards because of commit 217504a05532. The way that it solves a single issue is not isolated enough like it should be, which means every single connector change event is affected by it even when there is no need for that, but the solution itself - duplicated command execution - is also simply too heavy for many EC firmwares. I do admit that my series still leaves problems, it does not solve everything, and I'm not claiming that it's actually fixing anything (it's not tagged as a fix), but it does improve the behaviour of the driver so much that I still think that we should use it as the new "baseline" for future improvements. > I do still get error messages in the kernel log right after waking from > suspend occasionally, but I have not yet found reproducible steps. Most > likely it has something to do with the controller being in an invalid state > after waking from suspension. Though even then the status of the UCSI power > supply is correct when this happens. This is most likely separate issue that needs its own fix. thanks,
Hi, The goal of this series was to improve the alt mode handling in the driver, but now it seems that we can use the "poll worker" that was introduced for that to handle other tasks better as well. Ulrich reported some problems that are caused by the second GET_CONNECTOR_STATUS right after the first one that was introduced in 217504a05532 ("usb: typec: ucsi: Work around PPM losing change information"). In the last patch I try to improve that workaround by extracting it out of the generic event handler into its own task and executing it only when it's really needed. That seems to improve the situation. These patches definitely improve the quality of the driver by making it a bit more readable, but they also appear to make the behaviour a bit more predictably and uniform on different platforms. Benjamin, can you test these? thanks, Heikki Krogerus (7): usb: typec: ucsi: Always cancel the command if PPM reports BUSY condition usb: typec: ucsi: Don't stop alt mode registration on busy condition usb: typec: ucsi: Add polling mechanism for partner tasks like alt mode checking usb: typec: ucsi: acpi: Reduce the command completion timeout usb: typec: ucsi: Check the partner alt modes always if there is PD contract usb: typec: ucsi: Read the PDOs in separate work usb: typec: ucsi: Better fix for missing unplug events issue drivers/usb/typec/ucsi/ucsi.c | 337 ++++++++++++++--------------- drivers/usb/typec/ucsi/ucsi.h | 3 +- drivers/usb/typec/ucsi/ucsi_acpi.c | 2 +- 3 files changed, 167 insertions(+), 175 deletions(-)