Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 20 Aug 2022 20:48:34 +0200
From:   Andrew Lunn <andrew@lunn.ch>
To:     Oleksij Rempel <o.rempel@pengutronix.de>
Cc:     Heiner Kallweit <hkallweit1@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Russell King <linux@armlinux.org.uk>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Jonathan Corbet <corbet@lwn.net>, kernel@pengutronix.de,
        linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-doc@vger.kernel.org,
        David Jander <david@protonic.nl>
Subject: Re: [PATCH net-next v1 7/7] ethtool: add interface to interact with
 Ethernet Power Equipment
Message-ID: <YwEsgpVPNtmvtYni@lunn.ch>
References: <20220819120109.3857571-1-o.rempel@pengutronix.de>
 <20220819120109.3857571-8-o.rempel@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220819120109.3857571-8-o.rempel@pengutronix.de>
Precedence: bulk

> +static int pse_get_pse_attributs(struct net_device *dev,
> +				 struct pse_reply_data *data)
> +{
> +	struct phy_device *phydev = dev->phydev;
> +	int ret;
> +
> +	if (!phydev)
> +		return -EOPNOTSUPP;
> +
> +	mutex_lock(&phydev->lock);
> +	if (!phydev->psec) {
> +		ret = -EOPNOTSUPP;
> +		goto error_unlock;
> +	}
> +
> +	ret = pse_podl_get_admin_sate(phydev->psec);
> +	if (ret < 0)
> +		goto error_unlock;

The locking is triggering all sorts of questions in my mind... I don't
have the answers yet, so consider this more a discussion.

You need somewhere to place the psec. Since we are talking power over
copper lines, there will be some sort of PHY, so phydev->psec seems
reasonable. However, there is a general trend of moving all DSA
Ethernet switches to phylink, which is going to make this a bit
tricker to actually get to the phydev object.

But using phydev->lock? Humm. At least in the PoE world, there seems
to be lots of I2C or SPI controllers. Why hold the phydev lock when
performing an I2C transaction? You have a generic linux regulator
driver. How would you see a generic C45.2.9 driver? If it calls in the
PHY driver, the lock is already held, and we have to be careful to not
deadlock.

I'm more thinking along the lines of psec should have a lock of its
own.  pse_podl_get_admin_state(), pse_podl_get_pw_d_status() etc
should take that mutex before calling to the actual driver.

For a PHY which actually supports C45.2.9, i hope that the phylib core
looks at the phy driver structure, sees that some pse_podl ops are
implemented, and instantiates and registers a psec object. The phylib
core provides wrappers, which take the phylib lock before calling into
the driver. And if the PHY strictly follows C45.2.9, the calls are
actually into phylib helpers. Otherwise the PHY driver can do its own
implementation.

     Andrew