[PATCH v2 1/2] net: pse-pd: Do not return EOPNOSUPP if config is null

[PATCH v2 1/2] net: pse-pd: Do not return EOPNOSUPP if config is null

[Kory Maincent]

For a PSE supporting both c33 and PoDL, setting config for one type of PoE
leaves the other type's config null. Currently, this case returns
EOPNOTSUPP, which is incorrect. Instead, we should do nothing if the
configuration is empty.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Fixes: d83e13761d5b ("net: pse-pd: Use regulator framework within PSE framework")
---

Changes in v2:
- New patch to fix dealing with a null config.
---
 drivers/net/pse-pd/pse_core.c | 4 ++--
 net/ethtool/pse-pd.c          | 4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c
index 795ab264eaf2..513cd7f85933 100644
--- a/drivers/net/pse-pd/pse_core.c
+++ b/drivers/net/pse-pd/pse_core.c
@@ -719,13 +719,13 @@ int pse_ethtool_set_config(struct pse_control *psec,
 {
 	int err = 0;
 
-	if (pse_has_c33(psec)) {
+	if (pse_has_c33(psec) && config->c33_admin_control) {
 		err = pse_ethtool_c33_set_config(psec, config);
 		if (err)
 			return err;
 	}
 
-	if (pse_has_podl(psec))
+	if (pse_has_podl(psec) && config->podl_admin_control)
 		err = pse_ethtool_podl_set_config(psec, config);
 
 	return err;
diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index 2c981d443f27..982995ff1628 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -183,7 +183,9 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 	if (pse_has_c33(phydev->psec))
 		config.c33_admin_control = nla_get_u32(tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL]);
 
-	/* Return errno directly - PSE has no notification */
+	/* Return errno directly - PSE has no notification
+	 * pse_ethtool_set_config() will do nothing if the config is null
+	 */
 	return pse_ethtool_set_config(phydev->psec, info->extack, &config);
 }
 
-- 
2.34.1

[PATCH v2 2/2] net: ethtool: pse-pd: Fix possible null-deref

[Kory Maincent]

Fix a possible null dereference when a PSE supports both c33 and PoDL, but
only one of the netlink attributes is specified. The c33 or PoDL PSE
capabilities are already validated in the ethnl_set_pse_validate() call.

Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
Reported-by: Jakub Kicinski <kuba@kernel.org>
Closes: https://lore.kernel.org/netdev/20240705184116.13d8235a@kernel.org/
Fixes: 4d18e3ddf427 ("net: ethtool: pse-pd: Expand pse commands with the PSE PoE interface")
---
 net/ethtool/pse-pd.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/ethtool/pse-pd.c b/net/ethtool/pse-pd.c
index 982995ff1628..776ac96cdadc 100644
--- a/net/ethtool/pse-pd.c
+++ b/net/ethtool/pse-pd.c
@@ -178,9 +178,9 @@ ethnl_set_pse(struct ethnl_req_info *req_info, struct genl_info *info)
 
 	phydev = dev->phydev;
 	/* These values are already validated by the ethnl_pse_set_policy */
-	if (pse_has_podl(phydev->psec))
+	if (tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL])
 		config.podl_admin_control = nla_get_u32(tb[ETHTOOL_A_PODL_PSE_ADMIN_CONTROL]);
-	if (pse_has_c33(phydev->psec))
+	if (tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL])
 		config.c33_admin_control = nla_get_u32(tb[ETHTOOL_A_C33_PSE_ADMIN_CONTROL]);
 
 	/* Return errno directly - PSE has no notification
-- 
2.34.1

Re: [PATCH v2 1/2] net: pse-pd: Do not return EOPNOSUPP if config is null

[Simon Horman]

On Wed, Jul 10, 2024 at 01:42:30PM +0200, Kory Maincent wrote:
> For a PSE supporting both c33 and PoDL, setting config for one type of PoE
> leaves the other type's config null. Currently, this case returns
> EOPNOTSUPP, which is incorrect. Instead, we should do nothing if the
> configuration is empty.
> 
> Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> Fixes: d83e13761d5b ("net: pse-pd: Use regulator framework within PSE framework")
> ---
> 
> Changes in v2:
> - New patch to fix dealing with a null config.

Hi Kory,

A few thing from a process perspective:

1. As fixes, with fixes tags (good!), this patchset seems like it is
   appropriate for net rather than net-next. And indeed it applies
   to net but not net-next. However, the absence of a target tree
   confuses our CI which failed to apply the patchset to net-next.

   Probably this means it should be reposted, targeted at net.

   Subject: [Patch v3 net x/y] ...

   See: https://docs.kernel.org/process/maintainer-netdev.html

2. Please provide a cover letter for patch sets with more than one
   (but not just one) patch. That provides an overview of how
   the patches relate to each other. And a convenient anchor for
   feedback such as point 1 above.

...

Re: [PATCH v2 1/2] net: pse-pd: Do not return EOPNOSUPP if config is null

[Kory Maincent]

On Thu, 11 Jul 2024 09:43:00 +0100
Simon Horman <horms@kernel.org> wrote:

> On Wed, Jul 10, 2024 at 01:42:30PM +0200, Kory Maincent wrote:
> > For a PSE supporting both c33 and PoDL, setting config for one type of PoE
> > leaves the other type's config null. Currently, this case returns
> > EOPNOTSUPP, which is incorrect. Instead, we should do nothing if the
> > configuration is empty.
> > 
> > Signed-off-by: Kory Maincent <kory.maincent@bootlin.com>
> > Fixes: d83e13761d5b ("net: pse-pd: Use regulator framework within PSE
> > framework") ---
> > 
> > Changes in v2:
> > - New patch to fix dealing with a null config.  
> 
> Hi Kory,
> 
> A few thing from a process perspective:
> 
> 1. As fixes, with fixes tags (good!), this patchset seems like it is
>    appropriate for net rather than net-next. And indeed it applies
>    to net but not net-next. However, the absence of a target tree
>    confuses our CI which failed to apply the patchset to net-next.
> 
>    Probably this means it should be reposted, targeted at net.
> 
>    Subject: [Patch v3 net x/y] ...
> 
>    See: https://docs.kernel.org/process/maintainer-netdev.html

Oops indeed sorry, forgot to add the net prefix.

> 2. Please provide a cover letter for patch sets with more than one
>    (but not just one) patch. That provides an overview of how
>    the patches relate to each other. And a convenient anchor for
>    feedback such as point 1 above.
> 
> ...

As my first version contained only one patch I did not thought of adding a
cover letter for the 2nd version but indeed I should. Sorry.

Thanks for the reminder I will try to not have my head in the cloud next time.

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

Re: [PATCH v2 1/2] net: pse-pd: Do not return EOPNOSUPP if config is null

[Andrew Lunn]

> > 1. As fixes, with fixes tags (good!), this patchset seems like it is
> >    appropriate for net rather than net-next. And indeed it applies
> >    to net but not net-next. However, the absence of a target tree
> >    confuses our CI which failed to apply the patchset to net-next.
> > 
> >    Probably this means it should be reposted, targeted at net.
> > 
> >    Subject: [Patch v3 net x/y] ...
> > 
> >    See: https://docs.kernel.org/process/maintainer-netdev.html
> 
> Oops indeed sorry, forgot to add the net prefix.

B4 is nice with this:

b4 prep --set-prefixes net

And it should then remember for all versions of the patchset.

    Andrew