Thoughts on GitHub AI PR reviews

bevanweiss · March 11, 2026, 2:15am

Do people have any opinions pro/con OpenWrt possibly enabling the Github Co-Pilot driven PR reviews?

It seems that there's quite a range of 'quality' when it comes to PR submissions, and it takes up precious human reviewer resources raising some relatively trivial issues that should have been addressed long before PR submission time (i.e. Signed-off-by, PR labelling, commit messages etc).

I think having a quick turn-around AI review could be beneficial to weed out those trivial PR issues before a human has to invest time in them, and it might potentially even catch some 'gotcha' bugs that still creep into otherwise high-quality PR submissions (like not releasing memory/handles in all abnormal scenarios)

I personally also think that where a PR doesn't comply with basic requirements (i.e. commit messages, appropriate Signed-off-by), it should be automatically closed by the system after 1 day.

andrewjlamarche · March 11, 2026, 3:44am

I’m personally not interested in AI-driven reviews. I’ve found that it doesn’t really know what it’s talking about with things like adding support for new devices/platforms/targets, or anything really new in general, and tends to make of a lot of nonsense and mislead.

I am, however, in agreement that improper PRs should be closed automatically, perhaps after a week or so rather than 1 day.

bevanweiss · March 11, 2026, 4:29am

I do agree that AI can often be rubbish... my only context around OpenWrt was this comment submission from hauke, which I didn't think was bad (i.e. the wording wasn't great, but all the points made were valid, in my opinion).

github.com/openwrt/openwrt

Comment by hauke - realtek: mdio: drop realtek,smi-address property

main ← plappermaul:mach

## Summary This PR refactors the Realtek MDIO DTS bindings to align with upst…ream conventions by making `reg` represent the bus address and deriving the overall port address from the DTS port nodes, eliminating the vendor-specific `realtek,smi-address` property. The DTS changes are straightforward renames/removals, but the driver logic has some correctness concerns. ## Issues **[critical]** `target/linux/realtek/files-6.12/drivers/net/mdio/mdio-realtek-otto.c` (probe function) `of_find_node_by_name(NULL, "ethernet-ports")` performs a global tree search and will find the *first* node named `ethernet-ports` in the entire DTS. If the system has multiple MDIO controllers (multiple `mdio_busX` instances calling probe), each probe call will find the same `ethernet-ports` node, which may or may not be the one associated with the controller being probed. This could cause cross-contamination between controllers. The original code iterated all `ethernet-phy` nodes globally but correlated them through the `reg` address — problematic in its own way but at least scoped by address. The new code should navigate from the platform device's own device node upward/sideways to find the associated `ethernet-ports` node, not search the whole tree. **[major]** `target/linux/realtek/files-6.12/drivers/net/mdio/mdio-realtek-otto.c` (probe function) `of_node_put(phy)` is called right after `dn[addr] = of_node_get(phy)`, which is correct. However when the loop hits the error path for an illegal `smi_bus` value, `of_node_put(ports)` is called but none of the `dn[0..addr-1]` nodes that were already `of_node_get()`'d are released, leaking node references. The original code had the same issue, but this PR doesn't fix it and arguably makes it slightly harder to track. **[major]** `target/linux/realtek/files-6.12/drivers/net/mdio/mdio-realtek-otto.c` The `addr` variable (port address from `port->reg`) is used directly as an index into `ctrl->smi_bus[]` and `ctrl->smi_addr[]` (both sized `RTMDIO_MAX_PHY`). The port `reg` value and the PHY address space are now treated as the same domain, but the check `addr >= ctrl->cfg->num_phys` only validates against the configured PHY count. If ports have `reg` values ≥ `RTMDIO_MAX_PHY` (which the constant may or may not cover), a buffer overrun occurs. The previous code had the same index from the PHY's own `reg`, so the domain hasn't changed, but the semantic shift deserves explicit validation. **[major]** `target/linux/realtek/dts/rtl9302_zyxel_xgs1x10-12-common.dtsi` The `phy24` and `phy25` node unit-addresses (`@24`, `@25`) in the dtsi no longer match the `reg` property (which is removed, leaving them without a `reg` at all). This is a DTS validity issue — a node's unit-address must match its `reg` property. The zyxel xgs1010 and xgs1210 device files override `reg` to `<8>` and `<9>` (or `<1>`,`<2>`) respectively, but the base dtsi leaves the nodes with no `reg` property and a stale unit-address. This will produce dtc warnings and may confuse the driver. **[minor]** `target/linux/realtek/files-6.12/drivers/net/mdio/mdio-realtek-otto.c` The comment in the old code made explicit that `smi_addr[addr] = addr` was the fallback when `realtek,smi-address` was absent. The new code has no fallback: if the phy `reg` read fails (which triggers `continue`), `ctrl->smi_addr[addr]` is left at its uninitialized value (or whatever `ctrl` was `devm_kzalloc`'d to — likely 0). This silent zero-fallback may mask misconfigured DTS entries rather than reporting an error. **[minor]** `target/linux/realtek/dts/rtl9312_plasmacloud_common.dtsi` A stray blank line was introduced inside the `phy37` node block (after the closing `};`), which is harmless but inconsistent with the style of other entries. ## Suggestions - Consider using `of_get_parent()` on the platform device's own node and walking to find the associated `ethernet-ports` sibling or child, rather than a global `of_find_node_by_name(NULL, ...)`. For example, navigate from `pdev->dev.of_node` upward to the SoC node, then find `ethernet-ports` as a child of the same parent — or pass the association via a phandle in the MDIO bus DTS node itself. - Add a `dev_err` log for the case where phy `reg` or parent `reg` read fails in the loop, rather than silently `continue`-ing, to aid debugging of malformed DTS. - The `for_each_child_of_node_scoped` macro (if it's the kernel's scoped iterator) handles ref-counting automatically for the loop variable; verify this is the correct macro for the kernel version targeted and that it handles early `return` paths correctly (it should, since it's scoped). - Consider adding a comment explaining *why* the port's `reg` value equals the overall PHY address (the hardware design assumption described in the PR description), since this is non-obvious to future readers. ## Verdict **Request Changes** — The global `of_find_node_by_name` lookup is architecturally incorrect for multi-controller systems, and the incomplete dtsi node (missing `reg` with stale unit-address) will cause DTS validation issues.

It wasn't stated that it was AI generated. But it does have that vibe to it (apologies to hauke if it was not AI).
I'd personally prefer if rather than lots of text, it just had a line comment (like humans tend to do) which indicated the particular concern with that line/nearby lines.

anon63541380 · March 11, 2026, 6:12am

Sounds fun in the arrogant world of yours.
and then .. 2 months later CM formatting regex gets changed and all contributors efforts dumped?

bevanweiss · March 11, 2026, 8:15am

Sounds fun in the arrogant world of yours.

You may not be aware, but this sounds very much like an offensive personal attack, which is not appreciated. Please be a bit more mindful of your tone.

The current basic requirement is that there is a commit message. So I'm unsure which aspect of a 'CM formatting regex' you're talking of.
I would very much not expect 'all contributors' to be submitting every PR with commits entirely devoid of a commit message.

Also, PRs that are closed are not 'dumped'. They can be re-opened again. They just don't consume reviewer's time/mental burden.

anon63541380 · March 11, 2026, 10:53pm

Throw in more clankers, best way to gatekeep outside contributors, as if 50 mails for 1 line change was not offputting enough.

systemcrash · March 12, 2026, 8:21pm

They're OK - if it's bots responding with pre-written human messages to fix common problems, great. AI can be a bit of a battle if it starts nit-picking. Some of the repos have automated formality checks which catch the SoB and subject line stuff, and that's fine. AI doesn't cut it with newer languages like ucode, yet, however. If AI can be manually triggered, fine, but not as an automated check.