Re: [PATCH net-next] net: phy: Add JLSemi JL21xx Gigabit Ethernet PHY driver

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential
issue(s) to consider.

Critical: 0 · High: 1 · Medium: 6 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] jlsemi_set_wol() can leave the MDIO bus mutex held permanently.
- [Medium] Wake-on-LAN cannot work, yet the driver tells userspace it is
  enabled.
- [Medium] When WoL is enabled, jlsemi_set_wol() writes JLSEMI_WOL_RMSQ
  (0x1fff) to page 0xd8a reg 17.
- [Medium] jlsemi_config_init() always calls
  jlsemi_disable_autonomous_eee() (clears PHYCR2 bit 5) on every…
- [Medium] When the LED's three link-speed bits are all set,
  jlsemi_led_hw_control_get() returns TRIGGER_NETDEV_LINK_10, LINK_100…
- [Medium] jlsemi_set_wol() ignores every hardware-programming error and
  always returns 0.
- [Medium] In the interrupt-disable branch of jlsemi_config_intr(), `err
  = phy_write_paged(phydev, JLSEMI_INER_PAGE, JLSEMI_INER, 0)` is never…
- [Low] The register map and most of the logic in
  drivers/net/phy/jlsemi.c are a near-verbatim copy of the RTL8211F…
- [Low] jlsemi_config_rgmii_delay() handles rx-/tx-internal-delay-ps
  inconsistently.
- [Low] jlsemi_handle_interrupt() reads INSR (0x1d) once.

--

net: phy: Add JLSemi JL21xx Gigabit Ethernet PHY driver

This commit adds a new phylib driver for the JLSemi JL2101, JL2121 and
generic JL21xx Gigabit PHYs. It covers paged register access, RGMII delay
configuration, vendor status register parsing, interrupts, hardware LED
control, magic packet Wake-on-LAN, autonomous EEE disable and
suspend/resume.

Link: https://www.jlsemi.com/

> diff --git a/drivers/net/phy/jlsemi.c b/drivers/net/phy/jlsemi.c
> new file mode 100644
> index 0000000000000..c3acff48d4618
> --- /dev/null
> +++ b/drivers/net/phy/jlsemi.c
> @@ -0,0 +1,551 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * Driver for JLSemi Gigabit Ethernet PHYs (JL2121 / JL2101)
> + *
> + * Copyright (C) 2026 Pufan Jin <cyan2254650260@gmail.com>
> + */

[ ... ]

> +/* Wake-on-LAN settings */
> +#define JLSEMI_WOL_PAGE			0xd8a
> +#define JLSEMI_WOL_SETTINGS_EVENTS	16
> +#define JLSEMI_WOL_EVENT_MAGIC		BIT(12)
> +#define JLSEMI_WOL_RST_RMSQ		17
> +#define JLSEMI_WOL_RG_RSTB		BIT(15)
> +#define JLSEMI_WOL_RMSQ			0x1fff

[Severity: Low]
This isn't a bug, but the register map looks identical to the RTL8211F
support in drivers/net/phy/realtek/realtek_main.c. That covers PHYCR2 0x19
bit 5, INSR 0x1d, INER page 0xa42 reg 0x12, RGMII page 0xd08 regs
0x11/0x15, WoL page 0xd8a regs 16/17 with RG_RSTB BIT(15) and RMSQ 0x1fff,
MAC page 0xd8c, and LEDCR page 0xd04 reg 0x10 with 5-bit per-LED fields.

jlsemi_led_hw_is_supported() and jlsemi_led_hw_control_get() also follow
rtl8211x_led_hw_is_supported() and rtl8211f_led_hw_control_get() closely.
The same goes for the set_wol, suspend and resume flows.

Neither the commit message nor the file header mentions this origin. The
header lists a single copyright holder, while realtek_main.c has others.

Some later fixes in the realtek code also didn't come across:

  - wakeup-source and wake IRQ setup in probe
  - disabling PME events at probe
  - the RG_RSTB pulse in suspend
  - device_can_wakeup() gating in get_wol/set_wol
  - the msleep(20) after resume

If these parts are register compatible with RTL8211F, could the IDs go into
the realtek driver, or could the code be shared, instead of keeping a copy?

[ ... ]

> +static int jlsemi_probe(struct phy_device *phydev)
> +{
> +	struct device *dev = &phydev->mdio.dev;
> +	struct jlsemi_priv *priv;
> +
> +	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +
> +	phydev->priv = priv;
> +
> +	return 0;
> +}

[Severity: Medium]
Can Wake-on-LAN work with this driver? Nothing here marks the mdio device
as wakeup capable. There is no device_set_wakeup_capable() or
device_init_wakeup(), no wakeup-source handling and no
devm_pm_set_wake_irq(). Compare rtl8211f_probe():

drivers/net/phy/realtek/realtek_main.c:rtl8211f_probe() {
    ...
	if (device_property_read_bool(dev, "wakeup-source") &&
	    phy_interrupt_is_valid(phydev)) {
		device_set_wakeup_capable(dev, true);
		devm_pm_set_wake_irq(dev, phydev->irq);
	}
    ...
}

Because dev->power.can_wakeup stays false, device_may_wakeup() is always
false. device_set_wakeup_enable() in jlsemi_set_wol() returns -EINVAL. That
error is ignored and set_wol returns 0, and jlsemi_get_wol() still reports
WAKE_MAGIC as supported and enabled.

On suspend, phy_may_wakeup() falls back to the driver's WoL state, so
phydev->wol_enabled becomes true. PHY_ALWAYS_CALL_SUSPEND then skips the
core's -EBUSY guard:

drivers/net/phy/phy_device.c:phy_suspend() {
    ...
	phydev->wol_enabled = phy_may_wakeup(phydev) ||
			      (netdev && netdev->ethtool->wol_enabled);
	/* If the device has WOL enabled, we cannot suspend the PHY */
	if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
		return -EBUSY;
    ...
}

jlsemi_suspend() checks only device_may_wakeup(). It skips the INER=PME
setup and calls genphy_suspend(), which sets BMCR_PDOWN.

Doesn't this power down the PHY while magic packet matching is armed? And
since phydev->wol_enabled is never checked, doesn't this also take the
copper link down when the MAC provides WoL?

For comparison, rtl821x_suspend() checks !phydev->wol_enabled before
calling genphy_suspend(). rtl8211f_get_wol() reports no support and
rtl8211f_set_wol() returns -EOPNOTSUPP when !device_can_wakeup().

> +
> +static int jlsemi_config_rgmii_delay(struct phy_device *phydev)
> +{

[ ... ]

> +	if (phydev->interface != PHY_INTERFACE_MODE_RGMII_TXID)
> +		rx_delay = phy_get_internal_delay(phydev, NULL, 0, true);
> +
> +	if (phydev->interface != PHY_INTERFACE_MODE_RGMII_RXID)
> +		tx_delay = phy_get_internal_delay(phydev, NULL, 0, false);

[ ... ]

> +	if (rx_delay >= 0)
> +		val_rxdly = rx_delay > 0 ? JLSEMI_RX_DELAY : 0;
> +	if (tx_delay >= 0)
> +		val_txdly = tx_delay > 0 ? JLSEMI_TX_DELAY : 0;

[Severity: Low]
With a NULL table and size 0, phy_get_internal_delay() returns the raw
picosecond value whenever rx-internal-delay-ps or tx-internal-delay-ps is
present. Any positive value is then mapped to the single fixed delay bit.

So a DT value like <100> or <500> silently enables the full fixed delay,
and unsupported values are never rejected. Would passing a delay table let
phy_get_internal_delay() validate the value or pick the nearest supported
one?

Also, the RX property is never read for rgmii-txid, and the TX property is
never read for rgmii-rxid. ethernet-phy.yaml says "If this property is
present then the PHY applies the RX delay". Is it intended that the
property is ignored in those modes?

[ ... ]

> +static int jlsemi_config_init(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	ret = jlsemi_disable_autonomous_eee(phydev);
> +	if (ret < 0)
> +		return ret;

[Severity: Medium]
Should this be called unconditionally from config_init? The same function
is also registered as .disable_autonomous_eee. phylib calls that op only
from phy_support_eee(), when the MAC says it handles LPI, and then sets
phydev->autonomous_eee_disabled. phy_init_hw() reapplies the disable only
when that flag is set.

Calling it here turns off autonomous EEE on every phy_init_hw(). That
includes MACs that never call phy_support_eee().

phy_probe() leaves EEE advertised by default. Couldn't such a MAC
negotiate EEE and then get LPI signalling it doesn't handle, or at least
lose the power savings of autonomous mode?

For comparison, rtl8211f_disable_autonomous_eee() is wired only through
the ops table and isn't called from config_init.

The commit message says:

  - Autonomous EEE disable to prevent interoperability packet drops

Is there more detail on which packet drops this refers to?

[ ... ]

> +static int jlsemi_config_intr(struct phy_device *phydev)
> +{

[ ... ]

> +	} else {
> +		val = 0;
> +		err = phy_write_paged(phydev, JLSEMI_INER_PAGE, JLSEMI_INER, val);
> +		if (!err)
> +			priv->iner = val;
> +
> +		err = jlsemi_ack_interrupt(phydev);
> +	}

[Severity: Medium]
Is the result of the INER write lost here? If phy_write_paged() fails and
the INSR read succeeds, err is overwritten with 0 and jlsemi_config_intr()
reports success.

phy_disable_interrupts() and phy_init_hw() would then assume the interrupt
sources are masked, while INER may still have LINK_STATUS enabled. Could
the PHY keep asserting its IRQ line after phylib has stopped handling it?

[ ... ]

> +static irqreturn_t jlsemi_handle_interrupt(struct phy_device *phydev)
> +{
> +	int irq_status;
> +
> +	irq_status = phy_read(phydev, JLSEMI_INSR);
> +	if (irq_status < 0) {
> +		phy_error(phydev);
> +		return IRQ_NONE;
> +	}
> +
> +	if (irq_status & JLSEMI_INER_LINK_STATUS) {
> +		phy_trigger_machine(phydev);
> +		return IRQ_HANDLED;
> +	}

[Severity: Low]
The INSR read is also what acknowledges the interrupt, since that read is
all jlsemi_ack_interrupt() does. What happens if LINK_STATUS and PME are
both latched in the same read?

The early return means pm_wakeup_event() is never called for a PME that
the hardware has already cleared. One example is a PME latched during
suspend plus a link change around resume, after jlsemi_resume() restores
INER.

Could both bits be handled from the single read?

> +
> +	if (irq_status & JLSEMI_INER_PME) {
> +		pm_wakeup_event(&phydev->mdio.dev, 0);
> +		return IRQ_HANDLED;
> +	}

[ ... ]

> +static int jlsemi_set_wol(struct phy_device *phydev, struct ethtool_wolinfo *wol)
> +{

[ ... ]

> +		oldpage = phy_select_page(phydev, JLSEMI_PHYSICAL_ADDR_PAGE);
> +		if (oldpage < 0)
> +			return oldpage;

[Severity: High]
Does this return leave the MDIO bus lock held? phy_select_page() takes the
lock in phy_save_page() before it reads the page register, and it returns
errors with the lock still held:

drivers/net/phy/phy-core.c:phy_select_page() {
	oldpage = ret = phy_save_page(phydev);
	if (ret < 0)
		return ret;

	if (oldpage != page) {
		ret = __phy_write_page(phydev, page);
		if (ret < 0)
			return ret;
	}
    ...
}

Its kernel-doc says phy_restore_page() must always be called afterwards,
whether or not it succeeded. phy_restore_page() is also what unlocks the
bus.

If the page read or write fails with an MDIO error, this early return
skips the unlock. Every later access on that bus would then block forever,
including the state machine, the threaded PHY IRQ and other PHYs on the
same bus. The disable branch below has the same early return.

> +
> +		__phy_write(phydev, JLSEMI_PHYSICAL_ADDR_WORD0,
> +			    mac_addr[1] << 8 | mac_addr[0]);
> +		__phy_write(phydev, JLSEMI_PHYSICAL_ADDR_WORD1,
> +			    mac_addr[3] << 8 | mac_addr[2]);
> +		__phy_write(phydev, JLSEMI_PHYSICAL_ADDR_WORD2,
> +			    mac_addr[5] << 8 | mac_addr[4]);
> +
> +		ret = __phy_write(phydev, JLSEMI_PAGE_SELECT, JLSEMI_WOL_PAGE);
> +		if (ret < 0)
> +			goto restore;
> +
> +		__phy_write(phydev, JLSEMI_WOL_SETTINGS_EVENTS,
> +			    JLSEMI_WOL_EVENT_MAGIC);
> +		__phy_write(phydev, JLSEMI_WOL_RST_RMSQ,
> +			    JLSEMI_WOL_RMSQ);
> +restore:
> +		phy_restore_page(phydev, oldpage, ret);
> +	} else {
> +		oldpage = phy_select_page(phydev, JLSEMI_WOL_PAGE);
> +		if (oldpage < 0)
> +			return oldpage;
> +
> +		__phy_write(phydev, JLSEMI_WOL_SETTINGS_EVENTS, 0);
> +		__phy_clear_bits(phydev, JLSEMI_WOL_RST_RMSQ,
> +				 JLSEMI_WOL_RG_RSTB);
> +
> +		phy_restore_page(phydev, oldpage, 0);
> +	}
> +
> +	device_set_wakeup_enable(&phydev->mdio.dev, !!(wol->wolopts & WAKE_MAGIC));
> +
> +	return 0;
> +}

[Severity: Medium]
Can jlsemi_set_wol() return anything other than 0 once the page has been
selected? The three MAC address writes and the SETTINGS_EVENTS and RST_RMSQ
writes are all unchecked.

If the switch to JLSEMI_WOL_PAGE fails, the code jumps to restore with
ret < 0. The return value of phy_restore_page() is discarded, though, and
the function still calls device_set_wakeup_enable() and returns 0.

In the disable branch, the __phy_write() and __phy_clear_bits() results
are ignored. phy_restore_page() gets a hard-coded 0 and its result is
discarded too.

phy_ethtool_set_wol() passes that 0 back to userspace. If the page restore
fails, register 0x1f is left on page 0xd8a or 0xd8c. Wouldn't later
unpaged accesses, such as INSR 0x1d in the IRQ handler or PHYCR2 0x19, hit
the wrong registers?

The realtek version at least returns the result of phy_restore_page().

> +
> +static int jlsemi_suspend(struct phy_device *phydev)
> +{
> +	int ret;
> +
> +	if (device_may_wakeup(&phydev->mdio.dev)) {
> +		ret = phy_write_paged(phydev, JLSEMI_INER_PAGE, JLSEMI_INER,
> +				      JLSEMI_INER_PME);
> +		if (ret < 0)
> +			return ret;
> +
> +		jlsemi_ack_interrupt(phydev);
> +		return 0;
> +	}

[Severity: Medium]
Is the WoL block ever taken out of reset? jlsemi_set_wol() writes
JLSEMI_WOL_RMSQ (0x1fff) to page 0xd8a reg 17, and that clears RG_RSTB.
For the same RTL8211F register, the realtek driver's comment says "Set the
maximum packet size, and assert WoL reset". Nothing in this driver sets
RG_RSTB again.

rtl8211f_suspend() pulses RG_RSTB low and then high after enabling the PME
interrupt:

drivers/net/phy/realtek/realtek_main.c:rtl8211f_suspend() {
    ...
		/* Reset the WoL to ensure that an event is picked up.
		 * Unless we do this, even if we receive another packet,
		 * we may not have a PME interrupt raised.
		 */
		ret = phy_read_paged(phydev, RTL8211F_WOL_PAGE,
				     RTL8211F_WOL_RST_RMSQ);
    ...
		wol_rst = ret & ~RTL8211F_WOL_RG_RSTB;
		ret = phy_write_paged(phydev, RTL8211F_WOL_PAGE,
				      RTL8211F_WOL_RST_RMSQ, wol_rst);
    ...
		wol_rst |= RTL8211F_WOL_RG_RSTB;
		ret = phy_write_paged(phydev, RTL8211F_WOL_PAGE,
				      RTL8211F_WOL_RST_RMSQ, wol_rst);
    ...
}

jlsemi_suspend() has the INER=PME write and the INSR read, but not the
pulse.

If the JLSemi part has the same RG_RSTB semantics, wouldn't the magic
packet matcher stay in reset? PME would then never be raised, even after
the wakeup capability problem is fixed.

[ ... ]

> +static int jlsemi_led_hw_control_get(struct phy_device *phydev, u8 index,
> +				     unsigned long *rules)
> +{

[ ... ]

> +	if ((val & JLSEMI_LEDCR_LINK_10) &&
> +	    (val & JLSEMI_LEDCR_LINK_100) &&
> +	    (val & JLSEMI_LEDCR_LINK_1000)) {
> +		__set_bit(TRIGGER_NETDEV_LINK, rules);
> +	}

[Severity: Medium]
When all three speed bits are set, this returns LINK_10, LINK_100 and
LINK_1000 plus TRIGGER_NETDEV_LINK. That is also the hardware state that
jlsemi_led_hw_control_set() writes for a plain link rule.

netdev_trig_activate() copies the bitmap straight into trigger_data->mode.
netdev_led_attr_store() then returns -EINVAL for any write that leaves
LINK set together with a LINK_xx bit:

drivers/leds/trigger/ledtrig-netdev.c:netdev_led_attr_store() {
    ...
	if (test_bit(TRIGGER_NETDEV_LINK, &mode) &&
	    (test_bit(TRIGGER_NETDEV_LINK_10, &mode) ||
	     test_bit(TRIGGER_NETDEV_LINK_100, &mode) ||
	     test_bit(TRIGGER_NETDEV_LINK_1000, &mode) ||
    ...
}

Wouldn't toggling rx, tx, link_10 or link_1000 from sysfs then fail with
-EINVAL until link is cleared first? Should this return either the
aggregate LINK rule or the individual speed rules, but not both?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261016161543.444356-1-cyan2254650260%40gmail.com