Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26178d5b48
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
e2a80ee to
85c7ca9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85c7ca98ce
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
85c7ca9 to
a49ca02
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a49ca0256f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
a49ca02 to
b6bd11c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6bd11cf55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
iav
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6bd11cf55
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
2d07037 to
570ac8a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 570ac8a543
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
570ac8a to
4f904d8
Compare
|
Codex Review: Didn't find any major issues. Can't wait for the next one! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
4f904d8 to
6996897
Compare
iav
changed the title
6996897 to
c6ec4b7
Compare
|
Codex Review: Didn't find any major issues. Another round soon, please! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
c6ec4b7 to
60b6a6c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f722427ce1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
f722427 to
4d965fe
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d965fecb4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
4d965fe to
a0e3560
Compare
|
Rebuilt the branch. Patch 4 is folded into patch 3: the watchdog and stopping it are one change now, not a fix in the next patch. Both Codex findings applied: the retry budget no longer stops the poll, only the reset, so a link that comes back on its own is noticed and the bus scanned again; and phylink now remembers the modes handed to the PHY and gives them back to the MAC when that PHY detaches. On hardware, six shutdowns in a row: without stopping the watchdog, four end in a synchronous external abort in |
a0e3560 to
dfbbf19
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfbbf19492
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
dfbbf19 to
ad55a5a
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad55a5ad4a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ad55a5a to
5da6cd4
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5da6cd45b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
5da6cd4 to
3b5af5d
Compare
|
@codex review Since the last round: the PHY hand-off now offers only the modes the PHY reports as supported; |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b5af5d8e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
3b5af5d to
eb0b47f
Compare
|
@codex review Since the last round: the hand-off now carries along the modes the PHY already wakes on that the MAC cannot hold, so |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb0b47f1af
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
eb0b47f to
cf61431
Compare
488cd99 to
713a45a
Compare
|
@codex review Changed since your last pass: the MAC interrupt unwind in |
Three patches for pci-meson.c, added to meson64-6.12, -6.18, -7.2 and -7.3. 1. PM callbacks. On SM1 the controller sits in a power domain that genpd switches off during the noirq suspend phase, and the driver has no PM callbacks at all. The controller comes back unprogrammed, and the first config access to the root port after resume hangs the CPU. On ODROID-HC4, where the SATA controller lives behind that port, s2idle never returns. Suspend asserts PERST#, stops the LTSSM and powers the PHY off; resume repeats the probe sequence and hands over to dw_pcie_resume_noirq(), which reprograms the root complex, MSI and iATU and restarts the link. A stop_link callback, which the driver was missing entirely, goes in as well. dw_pcie_suspend_noirq() is not used: it broadcasts PME_Turn_Off through a MSG TLP window this controller does not have and fails with -ENOSPC, which aborts the noirq suspend phase for the whole system; the board then hangs in ahci's resume. Measured on HC4. A glue driver can ask the core for that window by setting pp.use_atu_msg before dw_pcie_host_init(), as pci-imx6.c and pcie-nxp-s32g.c do - worth doing, not done here. 2. Retrain the link on resume. meson_pcie_link_up() reports the controller's SMLH and RDLH bits, and after a resume that does not train the link both read as up while the data link layer is down: LNKSTA comes back as 0x1011 with DLL Active clear and the endpoint answers 0xffffffff. Ask the link status register once the root complex is back and, while the data link layer is down, rebuild it and train again, holding the endpoint in reset for T_PVPERL first. A dw_pcie_resume_noirq() that fails counts as an attempt too; after three of them resume gives up rather than reporting a link that carries nothing. A slot that had no device before the suspend gets a single attempt and no error - an empty slot never trains, and host init leaves it alone as well. 3. Rebuild the root complex after a link down reset. The same signature has also shown up with no sleep at all, after swapping the two disks on a running board: the root complex registers read as their defaults (bus numbers, COMMAND and LNKCTL zero), every config access below the port returns all ones, and the devices stay gone. Nothing puts those registers back: the only interrupt this controller has goes to MSI, so there is no link down handler, and AER reports nothing either. The endpoint survives it - unbinding and binding the driver brings everything back - so poll the link once a second and, when a link that carried a device goes down, do that automatically the way pciehp handles a surprise removal: mark the devices disconnected, remove them, and run the same path resume uses to program the root complex and train the link, with the MSI interrupt masked while the controller is in reset. On success the bus is scanned again. An empty slot is left alone. Once the devices are removed the port looks empty as well, so a flag keeps the poll going, and after three failed rebuilds the port is no longer reset, rather than reset once a second forever. The poll carries on, so a link that comes back on its own is still noticed and the bus scanned again; a system resume gives such a port three more tries. A link seen down is read again 100 ms later before anything is torn down, and the warning carries COMMAND and LNKSTA of the root port and the controller's STATUS12, to tell a real link down from a misread. The poll stops in .prepare, before the devices behind the port suspend, and restarts from .complete, once they have resumed. It is disabled rather than cancelled: the work re-arms itself at the end of every run, so a cancel that caught it running would be undone by that same run. Removal and shutdown disable it for good - otherwise the poll outlives the controller and reads registers whose power and clocks are gone. Without that, four shutdowns out of six on an ODROID-HC4 end in a synchronous external abort in meson_pcie_link_watch and the board keeps running past reboot: Power down; with it, six out of six power down cleanly. Builds on all four branches. On ODROID-HC4 with two SATA disks (7.2): s2idle woken by the RTC alarm, the link back at Gen2 x1 in resume, both disks read afterwards. In two of ten cold boots the poll rebuilt the port while the disks were spinning up; the one warning that carried the new fields read COMMAND 0x0000 and LNKSTA 0x1011, a real link down. The disks came back after about 4 s once and after about 30 s once, the latter while SCSI commands to the lost disk ran out their timeout. Assisted-by: Claude:claude-opus-5
A board configured for Wake-on-Lan the usual way - nmcli, which netplan
turns into a .link file - never wakes on a magic packet. udev applies
WakeOnLan= as soon as the interface appears, and stmmac attaches the PHY
only when the interface is opened, so phylink has no PHY to offer the
request to and leaves it at the MAC. The MAC on SM1 has no wake line of
its own ("IRQ eth_wake_irq not found"), and a later ethtool call is a
no-op because the state already reads as "g".
Offer the stored MAC options to the PHY as it attaches, keep at the MAC
only what the PHY does not take, as phylink_ethtool_set_wol() does. The
series that put WoL under phylink (b79fbd86c849, v6.19) left the initial
state unhandled; this covers it.
A PHY on its way out - an SFP pulled, the interface taken down - takes its
Wake-on-Lan with it, so ask it what it wakes on while it is still there and
arm the MAC for as much of that as the MAC supports; the next PHY is then
offered everything the MAC holds.
Assisted-by: Claude:claude-opus-5
Resume resets the MAC, and every core interrupt stays unmasked until the mask is written again: a link change in that window storms the CPU with "unexpected status 04000000" until the board is power cycled. Shut the line for as long as that takes, from the start of resume until the hardware is set up again. The line is requested with IRQF_SHARED, so disabling it stops every handler on it, not only this one; a line closed for the whole sleep would silence a co-owner that wakes the system. Closing it in resume alone also covers a MAC that owns a wake source itself: such a MAC needs the line alive while asleep, and the same reset waits for it on the way back. Resume gives the line back on every path out, not just the successful one: the PM core clears the suspended flag whatever the callback returns, so a resume that failed early would otherwise leave the interrupt disabled one level deeper each cycle. Assisted-by: Claude:claude-opus-5
The PCF8563 node on ODROID-HC4 is marked as a wakeup source but has no interrupt, so its alarm reaches nothing and the board sleeps through it. The INT line sits on GPIOX_5; point the node at it and the alarm wakes the board from s2idle. Added to all four branches. Assisted-by: Claude:claude-opus-5
Since v6.18 the kernel enables L0s and L1 by default on devicetree platforms. On ODROID-HC4 the link to the ASM1061 then died in L1 while the disk spun up after s2idle resume, in 6 of 21 cycles; the watchdog rebuilt the root complex each time. With this quirk: 0 of 15. Disable L0s and L1 on 1b21:0611 as mainline does for the ASM1083. Assisted-by: Claude:claude-opus-5
713a45a to
45d42e7
Compare
|
@codex review Same head as the earlier call: |
|
Codex Review: Didn't find any major issues. Already looking forward to the next diff. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
1 participant
Three patches for
pci-meson.c, added to meson64-6.12, -6.18, -7.2 and -7.3.1. PM callbacks
On SM1 the controller sits in a power domain that genpd switches off during
the noirq suspend phase, and the driver has no PM callbacks at all. The
controller comes back unprogrammed, and the first config access to the root
port after resume hangs the CPU. On ODROID-HC4, where the SATA controller
lives behind that port, s2idle never returns.
Suspend asserts
PERST#and stops the LTSSM; resume repeats the probesequence and hands over to
dw_pcie_resume_noirq(), which reprograms the rootcomplex, MSI and iATU and restarts the link. A
stop_linkcallback, which thedriver was missing entirely, goes in as well.
dw_pcie_suspend_noirq()is not used: it broadcasts PME_Turn_Off through aMSG TLP window this controller does not have and fails with
-ENOSPC, whichaborts the noirq suspend phase for the whole system; the board then hangs in
ahci's resume. Measured on HC4. A glue driver can ask the core for that window
by setting
pp.use_atu_msgbeforedw_pcie_host_init(), aspci-imx6.candpcie-nxp-s32g.cdo — worth doing, not done here.2. Retrain the link on resume
meson_pcie_link_up()reports the controller's SMLH and RDLH bits, and aftera resume that does not train the link both read as up while the data link
layer is down:
LNKSTAcomes back as0x1011with DLL Active clear and theendpoint answers
0xffffffff. Ask the link status register once the rootcomplex is back and, while the data link layer is down, rebuild it and train
again, holding the endpoint in reset for
T_PVPERLfirst; three attempts,then resume fails with
-ETIMEDOUTrather than reporting a link that carriesnothing.
3. Rebuild the root complex after a link down reset
The same signature has also shown up with no sleep at all, after swapping the
two disks on a running board: the root complex registers read as their
defaults (bus numbers,
COMMANDandLNKCTLzero), every config access belowthe port returns all ones, and the devices stay gone.
Nothing puts those registers back: the only interrupt this controller has goes
to MSI, so there is no link down handler, and AER reports nothing either. The
endpoint survives it — unbinding and binding the driver brings everything back
— so poll the link once a second and, when a link that carried a device goes
down, do that automatically the way pciehp handles a surprise removal: mark the
devices disconnected, remove them, power the controller off and on, let
dw_pcie_resume_noirq()program the root complex and rescan the bus. An emptyslot is left alone. The poll stops for system sleep and restarts from
.complete, once the devices behind the port have resumed.Tested on ODROID-HC4 with two SATA disks
is up at Gen2 x1, the SATA controller answers and both disks read. Without
patch 1 s2idle never comes back;
None of them dropped the link this time, so the rebuild path of this version
is not exercised on hardware. An earlier version of patch 3 caught a real
link down and brought the disks back in about a second, no power cycle.
Related work — how this fits with the other suspend patches
IRQCHIP_SKIP_SET_WAKEfor the Amlogic GPIO interruptcontroller (merged). It gives the board something to wake it; this one lets
the board survive the sleep.
wakeup-sourcefor the ethernet PHY, Wake-on-LAN onHC4 (merged). Needs this patch for the woken board to find its SATA
controller afterwards.
it gives the root port a driver and enables AER. AER turns out not to help
here: when the link drops, no AER event arrives at all.
Added 22 Sep, later.
Patch 4 (phylink), one more review fix: a SecureOn mode the PHY holds and the MAC cannot carries the PHY's own password along, from the same
phy_ethtool_get_wol()query.Patch 7 (ASM1061 and ASPM,
quirks.c, 6.18, 7.2 and 7.3). Since v6.18 the kernel enables L0s and L1 on devicetree platforms by default (f3ac2ff14834, narrowed in df5192d9bb0e); before that nothing turned ASPM on for the HC4's SATA bridge. After s2idle resume its link then died in L1 while the disk spun up - 6 of 21 cycles, each one rebuilt by the watchdog. The patch disables L0s and L1 on 1b21:0611 the way mainline does for the ASM1083 (b361663c5a40) and the 6.18 regressions (921b3f59b7b0), and the way storage drivers do for their own controllers: hpsa (e5a44df85e8d), aacraid (cf16123c9c8e), mpt3sas (ffdadd68af5a). 6.12 does not enable ASPM on devicetree platforms and needs no change.Tested on ODROID-HC4, 7.2.7, ASPM policy
default:lspcireadsASPM Disabledon both ends; 15 s2idle cycles woken by a magic packet - no link down, both disks present after each.