| Message ID | 20250217134812.1925345-1-kory.maincent@bootlin.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f6093c5ec74d5cc495f89bd359253d9c738d04d9 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] net: pse-pd: pd692x0: Fix power limit retrieval | expand |
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
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,
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..
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!
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,
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(-)