Hi Köry,
sorry for late review.
On Tue, Mar 04, 2025 at 11:18:51AM +0100, Kory Maincent wrote:
> From: Kory Maincent (Dent Project) <kory.maincent@xxxxxxxxxxx>
...
> diff --git a/drivers/net/mdio/fwnode_mdio.c b/drivers/net/mdio/fwnode_mdio.c
> index aea0f03575689..9b41d4697a405 100644
> --- a/drivers/net/mdio/fwnode_mdio.c
> +++ b/drivers/net/mdio/fwnode_mdio.c
> @@ -18,7 +18,8 @@ MODULE_LICENSE("GPL");
> MODULE_DESCRIPTION("FWNODE MDIO bus (Ethernet PHY) accessors");
>
> static struct pse_control *
> -fwnode_find_pse_control(struct fwnode_handle *fwnode)
> +fwnode_find_pse_control(struct fwnode_handle *fwnode,
> + struct phy_device *phydev)
> {
> struct pse_control *psec;
> struct device_node *np;
> @@ -30,7 +31,7 @@ fwnode_find_pse_control(struct fwnode_handle *fwnode)
> if (!np)
> return NULL;
>
> - psec = of_pse_control_get(np);
> + psec = of_pse_control_get(np, phydev);
> if (PTR_ERR(psec) == -ENOENT)
> return NULL;
>
> @@ -128,15 +129,9 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> u32 phy_id;
> int rc;
>
> - psec = fwnode_find_pse_control(child);
> - if (IS_ERR(psec))
> - return PTR_ERR(psec);
> -
> mii_ts = fwnode_find_mii_timestamper(child);
> - if (IS_ERR(mii_ts)) {
> - rc = PTR_ERR(mii_ts);
> - goto clean_pse;
> - }
> + if (IS_ERR(mii_ts))
> + return PTR_ERR(mii_ts);
>
> is_c45 = fwnode_device_is_compatible(child, "ethernet-phy-ieee802.3-c45");
> if (is_c45 || fwnode_get_phy_id(child, &phy_id))
> @@ -169,6 +164,12 @@ int fwnode_mdiobus_register_phy(struct mii_bus *bus,
> goto clean_phy;
> }
>
> + psec = fwnode_find_pse_control(child, phy);
> + if (IS_ERR(psec)) {
> + rc = PTR_ERR(psec);
> + goto unregister_phy;
> + }
Hm... we are starting to dereference the phydev pointer to a different
framework without managing the ref-counting.
We will need to have some form of get_device(&phydev->mdio.dev).
Normally it is done by phy_attach_direct().
And the counterpart: put_device() or phy_device_free()
> static int of_load_single_pse_pi_pairset(struct device_node *node,
> @@ -557,6 +560,151 @@ int devm_pse_controller_register(struct device *dev,
> }
> EXPORT_SYMBOL_GPL(devm_pse_controller_register);
>
> +struct pse_irq {
> + struct pse_controller_dev *pcdev;
> + struct pse_irq_desc desc;
> + unsigned long *notifs;
> +};
> +
> +/**
> + * pse_to_regulator_notifs - Convert PSE notifications to Regulator
> + * notifications
> + * @notifs: PSE notifications
> + *
> + * Return: Regulator notifications
> + */
> +static unsigned long pse_to_regulator_notifs(unsigned long notifs)
> +{
> + unsigned long rnotifs = 0;
> +
> + if (notifs & ETHTOOL_PSE_EVENT_OVER_CURRENT)
> + rnotifs |= REGULATOR_EVENT_OVER_CURRENT;
> + if (notifs & ETHTOOL_PSE_EVENT_OVER_TEMP)
> + rnotifs |= REGULATOR_EVENT_OVER_TEMP;
> +
> + return rnotifs;
> +}
> +
> +/**
> + * pse_control_find_phy_by_id - Find PHY attached to the pse control id
> + * @pcdev: a pointer to the PSE
> + * @id: index of the PSE control
> + *
> + * Return: PHY device pointer or NULL
> + */
> +static struct phy_device *
> +pse_control_find_phy_by_id(struct pse_controller_dev *pcdev, int id)
> +{
> + struct pse_control *psec;
> +
> + mutex_lock(&pse_list_mutex);
> + list_for_each_entry(psec, &pcdev->pse_control_head, list) {
> + if (psec->id == id) {
> + mutex_unlock(&pse_list_mutex);
> + return psec->attached_phydev;
> + }
> + }
> + mutex_unlock(&pse_list_mutex);
> +
Here we should increase ref-counting withing the mutex protection:
mutex_lock(&pse_list_mutex);
list_for_each_entry(psec, &pcdev->pse_control_head, list) {
if (psec->id == id) {
phydev = psec->attached_phydev;
if (phydev)
get_device(&phydev->mdio.dev); // Increase reference count
break;
}
}
mutex_unlock(&pse_list_mutex);
May be we will need a generic function for the PHYlib: phy_device_get() ?
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
