| Message ID | 20240215-feature_poe-v4-5-35bb4c23266c@bootlin.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | net: Add support for Power over Ethernet (PoE) | expand |
On Thu, 15 Feb 2024 17:02:46 +0100 Kory Maincent wrote: > Introduce an enumeration to define PSE types (C33 or PoDL), > utilizing a bitfield for potential future support of both types. > Include 'pse_get_types' helper for external access to PSE type info. I haven't read the series, just noticed this breaks the build: error: ../include/uapi/linux/pse.h: missing "WITH Linux-syscall-note" for SPDX-License-Identifier but why the separate header? Is it going to be used in other parts of uAPI than just in ethtool? > This patch is sponsored by Dent Project <dentproject@linuxfoundation.org> side-note: no objections to the line but for accounting purposes (i.e. when we generate development stats) we use the Author / From line exclusively. So it'd be easier to compute stats of things funded by Dent if you used: From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com> but that's entirely up to you :)
On Thu, 15 Feb 2024 10:58:46 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Thu, 15 Feb 2024 17:02:46 +0100 Kory Maincent wrote: > > Introduce an enumeration to define PSE types (C33 or PoDL), > > utilizing a bitfield for potential future support of both types. > > Include 'pse_get_types' helper for external access to PSE type info. > > I haven't read the series, just noticed this breaks the build: > > error: ../include/uapi/linux/pse.h: missing "WITH Linux-syscall-note" for > SPDX-License-Identifier By curiosity how do you get that error? Is it with C=1? I didn't faced it with W=1. C=1 is broken for several architecture like arm64, indeed I forgot to run it. > but why the separate header? Is it going to be used in other parts of > uAPI than just in ethtool? We might use it in pse core if capabilities between PoE and PoDL differ but I am not sure about it. Do you prefer to move it to ethtool header and add prefix ETHTOOL_ to the enum values? > > This patch is sponsored by Dent Project <dentproject@linuxfoundation.org> > > side-note: no objections to the line but for accounting purposes > (i.e. when we generate development stats) we use the Author / From > line exclusively. So it'd be easier to compute stats of things funded > by Dent if you used: > > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com> > > but that's entirely up to you :) Does adding the line side to the SOB in the commit message is sufficient or should I modify the git send email config? Regard,
On Fri, 16 Feb 2024 10:42:11 +0100 Köry Maincent wrote: > > On Thu, 15 Feb 2024 17:02:46 +0100 Kory Maincent wrote: > > > Introduce an enumeration to define PSE types (C33 or PoDL), > > > utilizing a bitfield for potential future support of both types. > > > Include 'pse_get_types' helper for external access to PSE type info. > > > > I haven't read the series, just noticed this breaks the build: > > > > error: ../include/uapi/linux/pse.h: missing "WITH Linux-syscall-note" for > > SPDX-License-Identifier > > By curiosity how do you get that error? > Is it with C=1? I didn't faced it with W=1. > C=1 is broken for several architecture like arm64, indeed I forgot to run it. Not 100% sure, TBH, I suspect it's somehow enabled by allmodconfig. I don't think it's a C=1 thing because our clang build doesn't do C=1 and it also hit it. > > but why the separate header? Is it going to be used in other parts of > > uAPI than just in ethtool? > > We might use it in pse core if capabilities between PoE and PoDL differ but I > am not sure about it. > Do you prefer to move it to ethtool header and add prefix ETHTOOL_ to the enum > values? I don't know enough to have an opinion :) Whatever you end up doing, it's probably worth documenting the reason for the choice in the commit message? > > > This patch is sponsored by Dent Project <dentproject@linuxfoundation.org> > > > > side-note: no objections to the line but for accounting purposes > > (i.e. when we generate development stats) we use the Author / From > > line exclusively. So it'd be easier to compute stats of things funded > > by Dent if you used: > > > > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com> > > > > but that's entirely up to you :) > > Does adding the line side to the SOB in the commit message is sufficient or > should I modify the git send email config? I think you can sed -i s/// the patches? When the From in the email file doesn't match your git config IIUC git will include the from line in the body and pick it up from them. IOW it will work. The scripts look at git author so s-o-b won't do much.
On Fri, 16 Feb 2024 17:36:38 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > > > but why the separate header? Is it going to be used in other parts of > > > uAPI than just in ethtool? > > > > We might use it in pse core if capabilities between PoE and PoDL differ but > > I am not sure about it. > > Do you prefer to move it to ethtool header and add prefix ETHTOOL_ to the > > enum values? > > I don't know enough to have an opinion :) Whatever you end up doing, > it's probably worth documenting the reason for the choice in the commit > message? Mmh, I am still not sure of the best choice on this. I think I will move it to ethtool as you suggested. > > > > This patch is sponsored by Dent Project > > > > <dentproject@linuxfoundation.org> > > > > > > side-note: no objections to the line but for accounting purposes > > > (i.e. when we generate development stats) we use the Author / From > > > line exclusively. So it'd be easier to compute stats of things funded > > > by Dent if you used: > > > > > > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com> > > > > > > but that's entirely up to you :) > > > > Does adding the line side to the SOB in the commit message is sufficient or > > should I modify the git send email config? > > I think you can sed -i s/// the patches? When the From in the email > file doesn't match your git config IIUC git will include the from line > in the body and pick it up from them. IOW it will work. The scripts look > at git author so s-o-b won't do much. Ok, I will stick to the simple sentence then. ;) Thanks for the information! Regards,
On Mon, Feb 19, 2024 at 04:04:56PM +0100, Köry Maincent wrote: > On Fri, 16 Feb 2024 17:36:38 -0800 > Jakub Kicinski <kuba@kernel.org> wrote: > > > > but why the separate header? Is it going to be used in other parts of > > > > uAPI than just in ethtool? > > > > > > We might use it in pse core if capabilities between PoE and PoDL differ but > > > I am not sure about it. > > > Do you prefer to move it to ethtool header and add prefix ETHTOOL_ to the > > > enum values? > > > > I don't know enough to have an opinion :) Whatever you end up doing, > > it's probably worth documenting the reason for the choice in the commit > > message? > > Mmh, I am still not sure of the best choice on this. I think I will move it to > ethtool as you suggested. kAPI is hard to change. So it is worth thinking about it. Can you think of any possible kAPI not using ethtool netlink? Its not going to be ioctl. We generally don't export new things in /sysfs if we have netlink, etc. So to me, it is only going to be used be the ethtool API, so i would follow the usual conventions for ethtool. Andrew
On Fri, Feb 16, 2024 at 05:36:38PM -0800, Jakub Kicinski wrote: > On Fri, 16 Feb 2024 10:42:11 +0100 Köry Maincent wrote: > > > On Thu, 15 Feb 2024 17:02:46 +0100 Kory Maincent wrote: > > > > Introduce an enumeration to define PSE types (C33 or PoDL), > > > > utilizing a bitfield for potential future support of both types. > > > > Include 'pse_get_types' helper for external access to PSE type info. > > > > > > I haven't read the series, just noticed this breaks the build: > > > > > > error: ../include/uapi/linux/pse.h: missing "WITH Linux-syscall-note" for > > > SPDX-License-Identifier > > > > By curiosity how do you get that error? > > Is it with C=1? I didn't faced it with W=1. > > C=1 is broken for several architecture like arm64, indeed I forgot to run it. > > Not 100% sure, TBH, I suspect it's somehow enabled by allmodconfig. > I don't think it's a C=1 thing because our clang build doesn't do C=1 > and it also hit it. Probably from scripts/spdxcheck.py? IIRC, it has some dependencies which have to be installed to enable it. Rob
On Mon, 19 Feb 2024 16:44:34 +0100 Andrew Lunn <andrew@lunn.ch> wrote: > On Mon, Feb 19, 2024 at 04:04:56PM +0100, Köry Maincent wrote: > > On Fri, 16 Feb 2024 17:36:38 -0800 > > Jakub Kicinski <kuba@kernel.org> wrote: > > > > > but why the separate header? Is it going to be used in other parts of > > > > > uAPI than just in ethtool? > > > > > > > > We might use it in pse core if capabilities between PoE and PoDL differ > > > > but I am not sure about it. > > > > Do you prefer to move it to ethtool header and add prefix ETHTOOL_ to > > > > the enum values? > > > > > > I don't know enough to have an opinion :) Whatever you end up doing, > > > it's probably worth documenting the reason for the choice in the commit > > > message? > > > > Mmh, I am still not sure of the best choice on this. I think I will move it > > to ethtool as you suggested. > > kAPI is hard to change. So it is worth thinking about it. > > Can you think of any possible kAPI not using ethtool netlink? Its not > going to be ioctl. We generally don't export new things in /sysfs if > we have netlink, etc. > > So to me, it is only going to be used be the ethtool API, so i would > follow the usual conventions for ethtool. Oops sorry forgot to reply to you. Indeed I reached to the same conclusion on my side. Regards,
diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c index 146b81f08a89..090e04c32f9e 100644 --- a/drivers/net/pse-pd/pse_core.c +++ b/drivers/net/pse-pd/pse_core.c @@ -312,3 +312,15 @@ int pse_ethtool_set_config(struct pse_control *psec, return err; } EXPORT_SYMBOL_GPL(pse_ethtool_set_config); + +bool pse_has_podl(struct pse_control *psec) +{ + return psec->pcdev->types & PSE_PODL; +} +EXPORT_SYMBOL_GPL(pse_has_podl); + +bool pse_has_c33(struct pse_control *psec) +{ + return psec->pcdev->types & PSE_C33; +} +EXPORT_SYMBOL_GPL(pse_has_c33); diff --git a/drivers/net/pse-pd/pse_regulator.c b/drivers/net/pse-pd/pse_regulator.c index 1dedf4de296e..e34ab8526067 100644 --- a/drivers/net/pse-pd/pse_regulator.c +++ b/drivers/net/pse-pd/pse_regulator.c @@ -116,6 +116,7 @@ pse_reg_probe(struct platform_device *pdev) priv->pcdev.owner = THIS_MODULE; priv->pcdev.ops = &pse_reg_ops; priv->pcdev.dev = dev; + priv->pcdev.types = PSE_PODL; ret = devm_pse_controller_register(dev, &priv->pcdev); if (ret) { dev_err(dev, "failed to register PSE controller (%pe)\n", diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h index be4e5754eb24..f006cbdf8b3b 100644 --- a/include/linux/pse-pd/pse.h +++ b/include/linux/pse-pd/pse.h @@ -8,6 +8,7 @@ #include <linux/ethtool.h> #include <linux/list.h> #include <uapi/linux/ethtool.h> +#include <uapi/linux/pse.h> struct phy_device; struct pse_controller_dev; @@ -77,6 +78,7 @@ struct pse_control; * device tree to id as given to the PSE control ops * @nr_lines: number of PSE controls in this controller device * @lock: Mutex for serialization access to the PSE controller + * @types: types of the PSE controller */ struct pse_controller_dev { const struct pse_controller_ops *ops; @@ -89,6 +91,7 @@ struct pse_controller_dev { const struct of_phandle_args *pse_spec); unsigned int nr_lines; struct mutex lock; + u32 types; }; #if IS_ENABLED(CONFIG_PSE_CONTROLLER) @@ -108,6 +111,9 @@ int pse_ethtool_set_config(struct pse_control *psec, struct netlink_ext_ack *extack, const struct pse_control_config *config); +bool pse_has_podl(struct pse_control *psec); +bool pse_has_c33(struct pse_control *psec); + #else static inline struct pse_control *of_pse_control_get(struct device_node *node) @@ -133,6 +139,16 @@ static inline int pse_ethtool_set_config(struct pse_control *psec, return -ENOTSUPP; } +static inline bool pse_has_podl(struct pse_control *psec) +{ + return false; +} + +static inline bool pse_has_c33(struct pse_control *psec) +{ + return false; +} + #endif #endif diff --git a/include/uapi/linux/pse.h b/include/uapi/linux/pse.h new file mode 100644 index 000000000000..ebd9b4be9d9d --- /dev/null +++ b/include/uapi/linux/pse.h @@ -0,0 +1,23 @@ +/* SPDX-License-Identifier: GPL-2.0-only */ +/* + * Userspace API for Power Sourcing Equipment + * + * Copyright (c) 2023 Bootlin, Kory Maincent <kory.maincent@bootlin.com> + */ +#ifndef _PSE_CONTROLLER_H +#define _PSE_CONTROLLER_H + +/** + * enum - Types of PSE controller. + * + * @PSE_UNKNOWN: Type of PSE controller is unknown + * @PSE_PODL: PSE controller which support PoDL + * @PSE_C33: PSE controller which support Clause 33 (PoE) + */ +enum { + PSE_UNKNOWN = 1 << 0, + PSE_PODL = 1 << 1, + PSE_C33 = 1 << 2, +}; + +#endif /* _PSE_CONTROLLER_H */