Re: [PATCH RESUBMIT net-next] net: phy: factor out legacy PHY fixup support and make it always built-in

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: phy: factor out legacy PHY fixup support and make it always built-in

This moves the legacy PHY fixup code out of the modular part of phylib
into a new always-built-in file, drivers/net/phy/phy_fixup.c, marks the
registration helpers __init and changes them to return void.  struct
phy_fixup and phy_fixup_list become private to the new file and the
fixup mutex is dropped.

> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index 41d1b20cde2c0..5b13a74e2fa92 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c

[ ... ]

> @@ -427,85 +416,6 @@ static __maybe_unused int mdio_bus_phy_resume(struct device *dev)
>  static SIMPLE_DEV_PM_OPS(mdio_bus_phy_pm_ops, mdio_bus_phy_suspend,
>  			 mdio_bus_phy_resume);
>  
> -/**
> - * phy_register_fixup - creates a new phy_fixup and adds it to the list
> - * @bus_id: A string which matches phydev->mdio.dev.bus_id (or NULL)
> - * @phy_uid: Used to match against phydev->phy_id (the UID of the PHY)
> - * @phy_uid_mask: Applied to phydev->phy_id and fixup->phy_uid before
> - *	comparison (or 0 to disable id-based matching)
> - * @run: The actual code to be run when a matching PHY is found
> - */

[ ... ]

> diff --git a/drivers/net/phy/phy_fixup.c b/drivers/net/phy/phy_fixup.c
> new file mode 100644
> index 0000000000000..f49fc4cefe817
> --- /dev/null
> +++ b/drivers/net/phy/phy_fixup.c
> @@ -0,0 +1,99 @@

[ ... ]

> +/**
> + * phy_register_fixup - creates a new phy_fixup and adds it to the list
> + * @bus_id: A string which matches phydev->mdio.dev.bus_id (or PHY_ANY_ID)
> + * @phy_uid: Used to match against phydev->phy_id (the UID of the PHY)
> + *	It can also be PHY_ANY_UID
> + * @phy_uid_mask: Applied to phydev->phy_id and fixup->phy_uid before
> + *	comparison
> + * @run: The actual code to be run when a matching PHY is found
> + */

[Severity: Low]
Was the wording change in this kernel-doc block intentional?  The version
removed from phy_device.c said "(or NULL)" for @bus_id and "(or 0 to
disable id-based matching)" for @phy_uid_mask, which matches what the code
does.  The new text mentions PHY_ANY_ID and PHY_ANY_UID instead.

Grepping the tree at this commit, PHY_ANY_ID and PHY_ANY_UID have no
definition anywhere; the only hits are these two new comment lines in
phy_fixup.c plus one sentence in Documentation/networking/phy.rst.

The matching code moved by this patch, phy_needs_fixup() in phy_fixup.c,
has no wildcard handling:

	if (!strcmp(fixup->bus_id, phydev_name(phydev)))
		return true;

	if (fixup->phy_uid_mask &&
	    phy_id_compare(phydev->phy_id, fixup->phy_uid, fixup->phy_uid_mask))
		return true;

and phy_register_fixup() only special-cases a NULL bus_id:

	if (bus_id)
		strscpy(fixup->bus_id, bus_id);
	fixup->phy_uid = phy_uid;
	fixup->phy_uid_mask = phy_uid_mask;

So platform code written against this comment would either fail to build
on the undefined identifier, or register a fixup that never matches.
Would it be better to keep the "(or NULL)" and "(or 0 to disable id-based
matching)" wording?

> +static void __init phy_register_fixup(const char *bus_id, u32 phy_uid,
> +				      u32 phy_uid_mask,
> +				      int (*run)(struct phy_device *))

[ ... ]

> +/* Registers a fixup to be run on any PHY with the UID in phy_uid */
> +void __init phy_register_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask,
> +				       int (*run)(struct phy_device *))
> +{
> +	phy_register_fixup(NULL, phy_uid, phy_uid_mask, run);
> +}
> +
> +/* Registers a fixup to be run on the PHY with id string bus_id */
> +void __init phy_register_fixup_for_id(const char *bus_id,
> +				      int (*run)(struct phy_device *))
> +{
> +	phy_register_fixup(bus_id, 0, 0, run);
> +}

[ ... ]

> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 3d8afe6b7f1c5..257634a543a14 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -2440,10 +2440,10 @@ int phy_get_mac_termination(struct phy_device *phydev, struct device *dev,
>  void phy_resolve_pause(unsigned long *local_adv, unsigned long *partner_adv,
>  		       bool *tx_pause, bool *rx_pause);
>  
> -int phy_register_fixup_for_id(const char *bus_id,
> -			      int (*run)(struct phy_device *));
> -int phy_register_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask,
> -			       int (*run)(struct phy_device *));
> +void __init phy_register_fixup_for_id(const char *bus_id,
> +				      int (*run)(struct phy_device *));
> +void __init phy_register_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask,
> +				       int (*run)(struct phy_device *));

[Severity: Low]
Should Documentation/networking/phy.rst be updated in the same patch?  The
"Board Fixups" section still documents the old prototypes:

 int phy_register_fixup_for_uid(u32 phy_uid, u32 phy_uid_mask,
		int (*run)(struct phy_device *));
 int phy_register_fixup_for_id(const char *phy_id,
		int (*run)(struct phy_device *));

Two things there are now stale.  The return type is void, so code that
assigns or tests the return value as the document suggests no longer
compiles.

The document also just says platform code registers fixups, with no
mention that registration is now init-only: the helpers are __init and are
no longer exported, and phy_register_fixup() adds to phy_fixup_list, which
is declared

	static struct list_head phy_fixup_list __ro_after_init =
		LIST_HEAD_INIT(phy_fixup_list);

so a caller outside init context gets a section mismatch from modpost and,
if it ever runs after init, writes to read-only memory.  Modules can no
longer call these at all.

Same section of phy.rst also describes PHY_ANY_ID and PHY_ANY_UID as
wildcards, which ties in with the kernel-doc comment above.
-- 
pw-bot: cr