[PATCH net] net: pse-pd: pd692x0: Fix power limit retrieval

[PATCH net] net: pse-pd: pd692x0: Fix power limit retrieval

[Kory Maincent]

Fix incorrect data offset read in the pd692x0_pi_get_pw_limit callback.
The issue was previously unnoticed as it was only used by the regulator
API and not thoroughly tested, since the PSE is mainly controlled via
ethtool.

The function became actively used by ethtool after commit 3e9dbfec4998
("net: pse-pd: Split ethtool_get_status into multiple callbacks"),
which led to the discovery of this issue.

Fix it by using the correct data offset.

Fixes: a87e699c9d33 ("net: pse-pd: pd692x0: Enhance with new current limit and voltage read callbacks")
Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
---
 drivers/net/pse-pd/pd692x0.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c
index fc9e23927b3b..7d60a714ca53 100644
--- a/drivers/net/pse-pd/pd692x0.c
+++ b/drivers/net/pse-pd/pd692x0.c
@@ -1047,7 +1047,7 @@ static int pd692x0_pi_get_pw_limit(struct pse_controller_dev *pcdev,
 	if (ret < 0)
 		return ret;
 
-	return pd692x0_pi_get_pw_from_table(buf.data[2], buf.data[3]);
+	return pd692x0_pi_get_pw_from_table(buf.data[0], buf.data[1]);
 }
 
 static int pd692x0_pi_set_pw_limit(struct pse_controller_dev *pcdev,
-- 
2.34.1

Re: [PATCH net] net: pse-pd: pd692x0: Fix power limit retrieval

[Andrew Lunn]

On Mon, Feb 17, 2025 at 02:48:11PM +0100, Kory Maincent wrote:
> Fix incorrect data offset read in the pd692x0_pi_get_pw_limit callback.
> The issue was previously unnoticed as it was only used by the regulator
> API and not thoroughly tested, since the PSE is mainly controlled via
> ethtool.
> 
> The function became actively used by ethtool after commit 3e9dbfec4998
> ("net: pse-pd: Split ethtool_get_status into multiple callbacks"),
> which led to the discovery of this issue.
> 
> Fix it by using the correct data offset.
> 
> Fixes: a87e699c9d33 ("net: pse-pd: pd692x0: Enhance with new current limit and voltage read callbacks")
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> ---
>  drivers/net/pse-pd/pd692x0.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c
> index fc9e23927b3b..7d60a714ca53 100644
> --- a/drivers/net/pse-pd/pd692x0.c
> +++ b/drivers/net/pse-pd/pd692x0.c
> @@ -1047,7 +1047,7 @@ static int pd692x0_pi_get_pw_limit(struct pse_controller_dev *pcdev,
>  	if (ret < 0)
>  		return ret;
>  
> -	return pd692x0_pi_get_pw_from_table(buf.data[2], buf.data[3]);
> +	return pd692x0_pi_get_pw_from_table(buf.data[0], buf.data[1]);

Would the issue of been more obvious if some #defines were used,
rather than magic numbers?

	Andrew

Re: [PATCH net] net: pse-pd: pd692x0: Fix power limit retrieval

[Kory Maincent]

On Mon, 17 Feb 2025 16:24:44 +0100
Andrew Lunn <andrew@lunn.ch> wrote:

> On Mon, Feb 17, 2025 at 02:48:11PM +0100, Kory Maincent wrote:
> > Fix incorrect data offset read in the pd692x0_pi_get_pw_limit callback.
> > The issue was previously unnoticed as it was only used by the regulator
> > API and not thoroughly tested, since the PSE is mainly controlled via
> > ethtool.
> > 
> > The function became actively used by ethtool after commit 3e9dbfec4998
> > ("net: pse-pd: Split ethtool_get_status into multiple callbacks"),
> > which led to the discovery of this issue.
> > 
> > Fix it by using the correct data offset.
> > 
> > Fixes: a87e699c9d33 ("net: pse-pd: pd692x0: Enhance with new current limit
> > and voltage read callbacks") Signed-off-by: Kory Maincent
> > <kory.maincent@bootlin.com> ---
> >  drivers/net/pse-pd/pd692x0.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/net/pse-pd/pd692x0.c b/drivers/net/pse-pd/pd692x0.c
> > index fc9e23927b3b..7d60a714ca53 100644
> > --- a/drivers/net/pse-pd/pd692x0.c
> > +++ b/drivers/net/pse-pd/pd692x0.c
> > @@ -1047,7 +1047,7 @@ static int pd692x0_pi_get_pw_limit(struct
> > pse_controller_dev *pcdev, if (ret < 0)
> >  		return ret;
> >  
> > -	return pd692x0_pi_get_pw_from_table(buf.data[2], buf.data[3]);
> > +	return pd692x0_pi_get_pw_from_table(buf.data[0], buf.data[1]);  
> 
> Would the issue of been more obvious if some #defines were used,
> rather than magic numbers?

We would need lots of defines as the offset of the useful data can
change between each command. Don't know if it would have been better. 

On my current patch priority series.
git grep "buf\." drivers/net/pse-pd/pd692x0.c | wc -l
29

Regards,
-- 
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

Re: [PATCH net] net: pse-pd: pd692x0: Fix power limit retrieval

[Jakub Kicinski]

On Mon, 17 Feb 2025 17:15:00 +0100 Kory Maincent wrote:
> > > -	return pd692x0_pi_get_pw_from_table(buf.data[2], buf.data[3]);
> > > +	return pd692x0_pi_get_pw_from_table(buf.data[0], buf.data[1]);    
> > 
> > Would the issue of been more obvious if some #defines were used,
> > rather than magic numbers?  
> 
> We would need lots of defines as the offset of the useful data can
> change between each command. Don't know if it would have been better. 
> 
> On my current patch priority series.
> git grep "buf\." drivers/net/pse-pd/pd692x0.c | wc -l
> 29

I guess it'd take a bigger rewrite, so I'll apply.
But I fully agree with Andrew that the current coding is very error
prone :( If you ever need to add more message you should start from
refactoring this code..

Re: [PATCH net] net: pse-pd: pd692x0: Fix power limit retrieval

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 17 Feb 2025 14:48:11 +0100 you wrote:
> Fix incorrect data offset read in the pd692x0_pi_get_pw_limit callback.
> The issue was previously unnoticed as it was only used by the regulator
> API and not thoroughly tested, since the PSE is mainly controlled via
> ethtool.
> 
> The function became actively used by ethtool after commit 3e9dbfec4998
> ("net: pse-pd: Split ethtool_get_status into multiple callbacks"),
> which led to the discovery of this issue.
> 
> [...]

Here is the summary with links:
  - [net] net: pse-pd: pd692x0: Fix power limit retrieval
    https://git.kernel.org/netdev/net/c/f6093c5ec74d

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html