Re: [PATCH net 04/10] ethtool: pse-pd: fix missing ethnl_ops_complete()

[Maxime Chevallier <maxime.chevallier@bootlin.com>]

Hi,

On 5/26/26 17:35, Jakub Kicinski wrote:
> pse_prepare_data() is missing ethnl_ops_complete() if
> ethnl_req_get_phydev() returned an error. Move getting
> phydev up so that we don't have to worry about this
> (similar order to linkstate_prepare_data()).
> 
> Note that phydev may still be NULL (this is checked in
> pse_get_pse_attributes()), the goal isn't really to avoid
> the _begin() / _complete() calls, only to simplify the error
> handling.
> 
> While at it propagate the original error. Why this code
> overrides the error with -ENODEV but !phydev generates
> -EOPNOTSUPP is unclear to me...

The logic is that ethnl_req_get_phydev() may return -ENODEV,
meaning "you passed a phy index for a PHY that doesn't
exist", which is different than "there's no PHY, setting PSE-PD
params isn't supported then."

But there's no real sense overriding the phydev ERR_PTR with the same
value :) Your fix should be fine :) Thanks for this !

> 
> Fixes: 31748765bed3 ("net: ethtool: pse-pd: Target the command to the requested PHY")
> Signed-off-by: Jakub Kicinski <kuba@kernel.org>

Reviewed-by: Maxime Chevallier <maxime.chevallier@bootlin.com>

Maxime

> ---
> CC: o.rempel@pengutronix.de
> CC: kory.maincent@bootlin.com
> CC: andrew@lunn.ch
> CC: maxime.chevallier@bootlin.com
> CC: chleroy@kernel.org
> ---
>   net/ethtool/pse-pd.c | 10 +++++-----
>   1 file changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
> index 2eb9bdc2dcb9..757c9e0cc856 100644
> --- a/net/ethtool/pse-pd.c
> +++ b/net/ethtool/pse-pd.c
> @@ -62,14 +62,14 @@ static int pse_prepare_data(const struct ethnl_req_info *req_base,
>   	struct phy_device *phydev;
>   	int ret;
>   
> -	ret = ethnl_ops_begin(dev);
> -	if (ret < 0)
> -		return ret;
> -
>   	phydev = ethnl_req_get_phydev(req_base, tb, ETHTOOL_A_PSE_HEADER,
>   				      info->extack);
>   	if (IS_ERR(phydev))
> -		return -ENODEV;
> +		return PTR_ERR(phydev);
> +
> +	ret = ethnl_ops_begin(dev);
> +	if (ret < 0)
> +		return ret;
>   
>   	ret = pse_get_pse_attributes(phydev, info->extack, data);
>