Skip to content

Fix/atcphy pipehandler #503

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
milijan wants to merge 2 commits into
base: asahi
Choose a base branch
from
+36 −13
Open

Fix/atcphy pipehandler #503

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5f22d75
phy: apple: atc: retry pipehandler lock with backoff on timeout
milijan May 25, 2026
cfca9fe
phy: apple: atc: fix pipehandler teardown race and false-positive WAR…
milijan May 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 36 additions & 13 deletions drivers/phy/apple/atc.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -922,23 +922,25 @@ static void atcphy_apply_tunables(struct apple_atcphy *atcphy, enum atcphy_mode

static int atcphy_pipehandler_lock(struct apple_atcphy *atcphy)
{
int ret;
int ret, retries = 10;
u32 reg;

if (readl(atcphy->regs.pipehandler + PIPEHANDLER_LOCK_REQ) & PIPEHANDLER_LOCK_EN) {
dev_warn(atcphy->dev, "Pipehandler already locked\n");
return 0;
}

set32(atcphy->regs.pipehandler + PIPEHANDLER_LOCK_REQ, PIPEHANDLER_LOCK_EN);

ret = readl_poll_timeout(atcphy->regs.pipehandler + PIPEHANDLER_LOCK_ACK, reg,
reg & PIPEHANDLER_LOCK_EN, 10, PIPEHANDLER_LOCK_ACK_TIMEOUT_US);
if (ret) {
clear32(atcphy->regs.pipehandler + PIPEHANDLER_LOCK_REQ, 1);
dev_warn(atcphy->dev, "Pipehandler lock not acked.\n");
}
do {
set32(atcphy->regs.pipehandler + PIPEHANDLER_LOCK_REQ, PIPEHANDLER_LOCK_EN);
ret = readl_poll_timeout(atcphy->regs.pipehandler + PIPEHANDLER_LOCK_ACK, reg,
Copy link
Copy Markdown
Member

@svenpeter42 svenpeter42 May 31, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've never seen macos do this in traces and I highly suspect we're doing something else that's wrong here. have you seen this in traces or is this something you worked out by just playing with the hardware?

Copy link
Copy Markdown
Author

@milijan milijan Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly try and error. My use case is using an M1 air in clamshell mode with fedora remix server (low idle power compute node for my homelab). So I need an Ethernet adapter. While it works fine when Hot Plugging the adapter, it fails on reboot (no phy detected/initialised). This was a defensive retry code attempt that seemed to work. But I agree with you something else is going on. Further testing shows that it is not reliable and if I removed some debug traces it would change the timing and fail again. Will probably move this issue to the forum before making further changes. Need to also understand some iboot behaviour differences between m1 and newer chips and assumptions made for how long it takes to load necessary firmwares. Thanks for reviewing.

Copy link
Copy Markdown
Member

@svenpeter42 svenpeter42 Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, that's what I was afraid of :( atcphy shouldn't have any firmware fwiw

Copy link
Copy Markdown
Author

@milijan milijan Jun 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but some usb hubs also have an external display (dcpext) which requires some lane allocation, initialisation and firmware (video modes). Not sure how all this is orchestrated to work in a generic way for typeC hubs in non-macOS and macOS systems. Any idea?

reg & PIPEHANDLER_LOCK_EN, 10, PIPEHANDLER_LOCK_ACK_TIMEOUT_US);
if (!ret)
return 0;
clear32(atcphy->regs.pipehandler + PIPEHANDLER_LOCK_REQ, PIPEHANDLER_LOCK_EN);
usleep_range(5000, 10000);
} while (--retries);

dev_warn(atcphy->dev, "Pipehandler lock not acked.\n");
return ret;
}

Expand Down Expand Up @@ -2081,6 +2083,7 @@ static int atcphy_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *sta
{
struct apple_atcphy *atcphy = typec_mux_get_drvdata(mux);
enum atcphy_mode target_mode;
int ret;

guard(mutex)(&atcphy->lock);

Expand Down Expand Up @@ -2138,11 +2141,31 @@ static int atcphy_mux_set(struct typec_mux_dev *mux, struct typec_mux_state *sta
return 0;

/*
* If the pipehandler is still/already up here there's a bug somewhere so make sure to
* complain loudly. We can still try to switch modes and hope for the best though,
* in the worst case the hardware will fall back to USB2-only.
* If the pipehandler is still up and the target mode uses the dummy PIPE
* state (DP, USB2, TBT, OFF), tear it down before reconfiguring the lanes.
* Normally atcphy_usb3_power_off() does this, but a TypeC mux switch to
* DP alt mode can race the DWC3 PHY teardown on hotplug. By the time the
Copy link
Copy Markdown
Member

@svenpeter42 svenpeter42 May 31, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how does this race actually happen? can you describe this in more detail?

the fix here may be correct but I want to be sure we can't fix this at another level (e.g. the type c controller)

* kernel TypeC stack reaches here the USB PD Enter_Mode handshake is
* complete and the SS lanes are already being repurposed, so switching the
* pipehandler to dummy is safe even if DWC3 has not yet called phy_power_off.
*/
if (atcphy->pipehandler_up &&
atcphy_modes[target_mode].pipehandler_state == ATCPHY_PIPEHANDLER_STATE_DUMMY) {
ret = atcphy_configure_pipehandler_dummy(atcphy);
if (ret)
dev_warn(atcphy->dev,
"Failed to clear pipehandler before mode switch: %d\n", ret);
else
atcphy->pipehandler_up = false;
}

/*
* For modes that keep the pipehandler in USB3 state (e.g. USB3_DP),
* pipehandler_up being true here is expected and correct. Only warn
* if we tried to tear down the pipehandler above but failed.
*/
WARN_ON_ONCE(atcphy->pipehandler_up);
WARN_ON_ONCE(atcphy->pipehandler_up &&
atcphy_modes[target_mode].pipehandler_state == ATCPHY_PIPEHANDLER_STATE_DUMMY);
return atcphy_configure(atcphy, target_mode);
}

Expand Down