linux-kernel - Re: [PATCH net-next v6 02/12] net: pse-pd: Add support for reporting events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z9frWsKESEaB9GcZ@pengutronix.de>
Date: Mon, 17 Mar 2025 10:28:58 +0100
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Kory Maincent <kory.maincent@...tlin.com>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Jonathan Corbet <corbet@....net>,
	Donald Hunter <donald.hunter@...il.com>,
	Rob Herring <robh@...nel.org>, Andrew Lunn <andrew+netdev@...n.ch>,
	Simon Horman <horms@...nel.org>,
	Heiner Kallweit <hkallweit1@...il.com>,
	Russell King <linux@...linux.org.uk>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>,
	Thomas Petazzoni <thomas.petazzoni@...tlin.com>,
	netdev@...r.kernel.org, linux-doc@...r.kernel.org,
	Kyle Swenson <kyle.swenson@....tech>,
	Dent Project <dentproject@...uxfoundation.org>,
	kernel@...gutronix.de,
	Maxime Chevallier <maxime.chevallier@...tlin.com>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v6 02/12] net: pse-pd: Add support for reporting
 events

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@...tlin.com>
...
> 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 |