| Message ID | 20241002-feature_poe_port_prio-v1-11-787054f74ed5@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add support for PSE port priority | expand |
> +int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq, > + int irq_flags, int supported_errs, > + const struct pse_irq_desc *d) > +{ > + struct regulator_dev **rdevs; > + void *irq_helper; > + int i; > + > + rdevs = devm_kcalloc(pcdev->dev, pcdev->nr_lines, > + sizeof(struct regulator_dev *), GFP_KERNEL); > + if (!rdevs) > + return -ENOMEM; > + > + for (i = 0; i < pcdev->nr_lines; i++) > + rdevs[i] = pcdev->pi[i].rdev; > + > + /* Register notifiers - can fail if IRQ is not given */ > + irq_helper = devm_regulator_irq_helper(pcdev->dev, d, irq, > + 0, supported_errs, NULL, > + &rdevs[0], pcdev->nr_lines); Should irq_flags be passed through? I'm guessing one usage of it will be IRQF_SHARED when there is one interrupt shared by a number of controllers. Andrew
On Wed, Oct 02, 2024 at 06:28:07PM +0200, Kory Maincent wrote: > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com> > > Add support for devm_pse_irq_helper(), a wrapper for > devm_regulator_irq_helper(). This aims to report events such as > over-current or over-temperature conditions similarly to how the regulator > API handles them. Additionally, this patch introduces several define > wrappers to keep regulator naming conventions out of PSE drivers. I'm missing something here, i think. https://docs.kernel.org/power/regulator/consumer.html#regulator-events Suggests these are internal events, using a notification chain. How does user space get to know about such events? Andrew
On Thu, 3 Oct 2024 01:52:20 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > > +int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq, > > + int irq_flags, int supported_errs, > > + const struct pse_irq_desc *d) > > +{ > > + struct regulator_dev **rdevs; > > + void *irq_helper; > > + int i; > > + > > + rdevs = devm_kcalloc(pcdev->dev, pcdev->nr_lines, > > + sizeof(struct regulator_dev *), GFP_KERNEL); > > + if (!rdevs) > > + return -ENOMEM; > > + > > + for (i = 0; i < pcdev->nr_lines; i++) > > + rdevs[i] = pcdev->pi[i].rdev; > > + > > + /* Register notifiers - can fail if IRQ is not given */ > > + irq_helper = devm_regulator_irq_helper(pcdev->dev, d, irq, > > + 0, supported_errs, NULL, > > + &rdevs[0], > > pcdev->nr_lines); > > Should irq_flags be passed through? I'm guessing one usage of it will > be IRQF_SHARED when there is one interrupt shared by a number of > controllers. Oh yes, you are right! Thanks for spotting it! > > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com> > > > > Add support for devm_pse_irq_helper(), a wrapper for > > devm_regulator_irq_helper(). This aims to report events such as > > over-current or over-temperature conditions similarly to how the regulator > > API handles them. Additionally, this patch introduces several define > > wrappers to keep regulator naming conventions out of PSE drivers. > > I'm missing something here, i think. > > https://docs.kernel.org/power/regulator/consumer.html#regulator-events > > Suggests these are internal events, using a notification chain. How > does user space get to know about such events? When events appears, _notifier_call_chain() is called which can generate netlink messages alongside the internal events: https://elixir.bootlin.com/linux/v6.11.1/source/drivers/regulator/core.c#L4898 Regards,
> > https://docs.kernel.org/power/regulator/consumer.html#regulator-events > > > > Suggests these are internal events, using a notification chain. How > > does user space get to know about such events? > > When events appears, _notifier_call_chain() is called which can generate netlink > messages alongside the internal events: > https://elixir.bootlin.com/linux/v6.11.1/source/drivers/regulator/core.c#L4898 Ah, O.K. But is this in the correct 'address space' for the want of a better term. Everything else to do with PSE is in the networking domain of netlink. ethtool is used to configure PSE. Shouldn't the notification also close by to ethtool? When an interface changes state, there is a notification sent. Maybe we want to piggyback on that? Also, how do regulator events work in combination with network namespaces? If you move the interface into a different network namespace, do the regulator events get delivered to the root namespace or the namespace the interface is in? Andrew
On Thu, 3 Oct 2024 14:56:21 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > > > https://docs.kernel.org/power/regulator/consumer.html#regulator-events > > > > > > Suggests these are internal events, using a notification chain. How > > > does user space get to know about such events? > > > > When events appears, _notifier_call_chain() is called which can generate > > netlink messages alongside the internal events: > > https://elixir.bootlin.com/linux/v6.11.1/source/drivers/regulator/core.c#L4898 > > > > Ah, O.K. > > But is this in the correct 'address space' for the want of a better > term. Everything else to do with PSE is in the networking domain of > netlink. ethtool is used to configure PSE. Shouldn't the notification > also close by to ethtool? When an interface changes state, there is a > notification sent. Maybe we want to piggyback on that? Indeed, but regulator API already provide such events, which will even be sent when we enable or disable the PSE. Should we write a second event management. Using regulator event API allows to report over current internal events to the parents regulator the power supply of the PSE which could also do something to avoid smoke. Or maybe we should add another wrapper which will send PSE ethtool netlink notification alongside the regulator notifications supported by this patch. > Also, how do regulator events work in combination with network > namespaces? If you move the interface into a different network > namespace, do the regulator events get delivered to the root namespace > or the namespace the interface is in? regulator events are sent in root namespace. Regards,
> Indeed, but regulator API already provide such events, which will even be sent > when we enable or disable the PSE. Should we write a second event management. > Using regulator event API allows to report over current internal events to the > parents regulator the power supply of the PSE which could also do something to > avoid smoke. > > Or maybe we should add another wrapper which will send PSE ethtool netlink > notification alongside the regulator notifications supported by this patch. > > > Also, how do regulator events work in combination with network > > namespaces? If you move the interface into a different network > > namespace, do the regulator events get delivered to the root namespace > > or the namespace the interface is in? > > regulator events are sent in root namespace. I think we will need two event, the base regulator event, and a networking event. Since it is a regulator, sending a normal regulator event makes a lot of sense. But mapping that regulator event to a netns:ifnam is going to be hard. Anything wanting to take an action is probably going to want to use ethtool, and so needs to be in the correct netns, etc. But it does get messy if there is some sort of software driven prioritisation going on, some daemon needs to pick a victim to reduce power to, and the interfaces are spread over multiple network namespaces. What i don't know is if we can use an existing event, or we should add a new one. Often rtnetlink_event() is used: https://elixir.bootlin.com/linux/v6.12-rc1/source/net/core/rtnetlink.c#L6679 but without some PSE information in it, it would be hard to know why it was sent. So we probably either want a generic ethtool event, or a PSE event. Andrew
On Thu, Oct 03, 2024 at 05:22:58PM +0200, Andrew Lunn wrote: > > Indeed, but regulator API already provide such events, which will even be sent > > when we enable or disable the PSE. Should we write a second event management. > > Using regulator event API allows to report over current internal events to the > > parents regulator the power supply of the PSE which could also do something to > > avoid smoke. > > > > Or maybe we should add another wrapper which will send PSE ethtool netlink > > notification alongside the regulator notifications supported by this patch. > > > > > Also, how do regulator events work in combination with network > > > namespaces? If you move the interface into a different network > > > namespace, do the regulator events get delivered to the root namespace > > > or the namespace the interface is in? > > > > regulator events are sent in root namespace. > > I think we will need two event, the base regulator event, and a > networking event. Since it is a regulator, sending a normal regulator > event makes a lot of sense. But mapping that regulator event to a > netns:ifnam is going to be hard. Anything wanting to take an action is > probably going to want to use ethtool, and so needs to be in the > correct netns, etc. But it does get messy if there is some sort of > software driven prioritisation going on, some daemon needs to pick a > victim to reduce power to, and the interfaces are spread over multiple > network namespaces. > > What i don't know is if we can use an existing event, or we should add > a new one. Often rtnetlink_event() is used: > > https://elixir.bootlin.com/linux/v6.12-rc1/source/net/core/rtnetlink.c#L6679 > > but without some PSE information in it, it would be hard to know why > it was sent. So we probably either want a generic ethtool event, or a > PSE event. Hm... assuming we have following scenario: .--------- PI 1 / .--------- PI 2 .========= PSE /----------( PI 3 ) NNS red // \----------( PI 4 ) NNS blue Main supply // `---------( PI 5 ) NNS blue o================´--- System, CPU In this case we seems to have a new challenge: On one side, a system wide power manager should see and mange all ports. On other side, withing a name space, we should be able to play in a isolated sand box. There is a reason why it is isolated. So, we should be able to sandbox power delivery and port prios too. Means, by creating network names space, we will need a power names space. I can even imagine a use case: an admin limited access to a switch for developer. A developer name space is created with PSE budget and max prios available for this name space. This will prevent users from DoSing system critical ports. At this point, creating a power name space will an overkill for this patch set, so it should be enough to allow controlling prios over ethtool per port and isolation support if needed. Regards, Oleksij
On Fri, Oct 04, 2024 at 03:56:33PM +0200, Oleksij Rempel wrote: > On Thu, Oct 03, 2024 at 05:22:58PM +0200, Andrew Lunn wrote: > > > Indeed, but regulator API already provide such events, which will even be sent > > > when we enable or disable the PSE. Should we write a second event management. > > > Using regulator event API allows to report over current internal events to the > > > parents regulator the power supply of the PSE which could also do something to > > > avoid smoke. > > > > > > Or maybe we should add another wrapper which will send PSE ethtool netlink > > > notification alongside the regulator notifications supported by this patch. > > > > > > > Also, how do regulator events work in combination with network > > > > namespaces? If you move the interface into a different network > > > > namespace, do the regulator events get delivered to the root namespace > > > > or the namespace the interface is in? > > > > > > regulator events are sent in root namespace. > > > > I think we will need two event, the base regulator event, and a > > networking event. Since it is a regulator, sending a normal regulator > > event makes a lot of sense. But mapping that regulator event to a > > netns:ifnam is going to be hard. Anything wanting to take an action is > > probably going to want to use ethtool, and so needs to be in the > > correct netns, etc. But it does get messy if there is some sort of > > software driven prioritisation going on, some daemon needs to pick a > > victim to reduce power to, and the interfaces are spread over multiple > > network namespaces. > > > > What i don't know is if we can use an existing event, or we should add > > a new one. Often rtnetlink_event() is used: > > > > https://elixir.bootlin.com/linux/v6.12-rc1/source/net/core/rtnetlink.c#L6679 > > > > but without some PSE information in it, it would be hard to know why > > it was sent. So we probably either want a generic ethtool event, or a > > PSE event. > > Hm... assuming we have following scenario: > > .--------- PI 1 > / .--------- PI 2 > .========= PSE /----------( PI 3 ) NNS red > // \----------( PI 4 ) NNS blue > Main supply // `---------( PI 5 ) NNS blue > o================´--- System, CPU > > In this case we seems to have a new challenge: > > On one side, a system wide power manager should see and mange all ports. > On other side, withing a name space, we should be able to play in a > isolated sand box. There is a reason why it is isolated. So, we should > be able to sandbox power delivery and port prios too. Means, by creating > network names space, we will need a power names space. > > I can even imagine a use case: an admin limited access to a switch for > developer. A developer name space is created with PSE budget and max > prios available for this name space. This will prevent users from DoSing > system critical ports. > > At this point, creating a power name space will an overkill for this > patch set, so it should be enough to allow controlling prios over > ethtool per port and isolation support if needed. Oh, sorry, i'm too tired. Too many words are missing in my answer ...
On Fri, 4 Oct 2024 16:02:15 +0200 Oleksij Rempel <o.rempel@pengutronix.de> wrote: > On Fri, Oct 04, 2024 at 03:56:33PM +0200, Oleksij Rempel wrote: > > On Thu, Oct 03, 2024 at 05:22:58PM +0200, Andrew Lunn wrote: > [...] > [...] > [...] > > > > > > I think we will need two event, the base regulator event, and a > > > networking event. Since it is a regulator, sending a normal regulator > > > event makes a lot of sense. But mapping that regulator event to a > > > netns:ifnam is going to be hard. Anything wanting to take an action is > > > probably going to want to use ethtool, and so needs to be in the > > > correct netns, etc. But it does get messy if there is some sort of > > > software driven prioritisation going on, some daemon needs to pick a > > > victim to reduce power to, and the interfaces are spread over multiple > > > network namespaces. > > > > > > What i don't know is if we can use an existing event, or we should add > > > a new one. Often rtnetlink_event() is used: > > > > > > https://elixir.bootlin.com/linux/v6.12-rc1/source/net/core/rtnetlink.c#L6679 > > > > > > but without some PSE information in it, it would be hard to know why > > > it was sent. So we probably either want a generic ethtool event, or a > > > PSE event. > > > > Hm... assuming we have following scenario: > > > > .--------- PI 1 > > / .--------- PI 2 > > .========= PSE /----------( PI 3 ) NNS red > > // \----------( PI 4 ) NNS blue > > Main supply // `---------( PI 5 ) NNS blue > > o================´--- System, CPU > > > > In this case we seems to have a new challenge: > > > > On one side, a system wide power manager should see and mange all ports. > > On other side, withing a name space, we should be able to play in a > > isolated sand box. There is a reason why it is isolated. So, we should > > be able to sandbox power delivery and port prios too. Means, by creating > > network names space, we will need a power names space. > > > > I can even imagine a use case: an admin limited access to a switch for > > developer. A developer name space is created with PSE budget and max > > prios available for this name space. This will prevent users from DoSing > > system critical ports. > > > > At this point, creating a power name space will an overkill for this > > patch set, so it should be enough to allow controlling prios over > > ethtool per port and isolation support if needed. Yes, I will add simple ethtool notification for now to report events on each interfaces. > Oh, sorry, i'm too tired. Too many words are missing in my answer ... Nearly the weekend!! Rest well! Regards,
diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c index d365fb7c8a98..a9f102507f5e 100644 --- a/drivers/net/pse-pd/pse_core.c +++ b/drivers/net/pse-pd/pse_core.c @@ -8,7 +8,6 @@ #include <linux/device.h> #include <linux/of.h> #include <linux/pse-pd/pse.h> -#include <linux/regulator/driver.h> #include <linux/regulator/machine.h> static DEFINE_MUTEX(pse_list_mutex); @@ -536,6 +535,37 @@ int devm_pse_controller_register(struct device *dev, } EXPORT_SYMBOL_GPL(devm_pse_controller_register); +int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq, + int irq_flags, int supported_errs, + const struct pse_irq_desc *d) +{ + struct regulator_dev **rdevs; + void *irq_helper; + int i; + + rdevs = devm_kcalloc(pcdev->dev, pcdev->nr_lines, + sizeof(struct regulator_dev *), GFP_KERNEL); + if (!rdevs) + return -ENOMEM; + + for (i = 0; i < pcdev->nr_lines; i++) + rdevs[i] = pcdev->pi[i].rdev; + + /* Register notifiers - can fail if IRQ is not given */ + irq_helper = devm_regulator_irq_helper(pcdev->dev, d, irq, + 0, supported_errs, NULL, + &rdevs[0], pcdev->nr_lines); + if (IS_ERR(irq_helper)) { + if (PTR_ERR(irq_helper) == -EPROBE_DEFER) + return -EPROBE_DEFER; + + dev_warn(pcdev->dev, "IRQ disabled %pe\n", irq_helper); + } + + return 0; +} +EXPORT_SYMBOL_GPL(devm_pse_irq_helper); + /* PSE control section */ static void __pse_control_release(struct kref *kref) diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h index b60fc56923bd..ba3d6630d768 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 <linux/regulator/driver.h> /* Maximum current in uA according to IEEE 802.3-2022 Table 145-1 */ #define MAX_PI_CURRENT 1920000 @@ -15,6 +16,26 @@ struct phy_device; struct pse_controller_dev; +/* structure and define wrappers from PSE to regulator */ +#define pse_irq_desc regulator_irq_desc +#define pse_irq_data regulator_irq_data +#define pse_err_data regulator_err_data +#define pse_err_state regulator_err_state + +#define PSE_EVENT_TABLE(event) REGULATOR_EVENT_##event +#define PSE_ERROR_TABLE(error) REGULATOR_ERROR_##error + +#define PSE_EVENT_OVER_CURRENT PSE_EVENT_TABLE(OVER_CURRENT) +#define PSE_EVENT_OVER_TEMP PSE_EVENT_TABLE(OVER_TEMP) + +#define PSE_ERROR_OVER_CURRENT PSE_ERROR_TABLE(OVER_CURRENT) +#define PSE_ERROR_OVER_TEMP PSE_ERROR_TABLE(OVER_TEMP) + +/* Return values for PSE IRQ helpers */ +#define PSE_ERROR_CLEARED PSE_ERROR_TABLE(CLEARED) +#define PSE_FAILED_RETRY REGULATOR_FAILED_RETRY +#define PSE_ERROR_ON PSE_ERROR_TABLE(ON) + /** * struct pse_control_config - PSE control/channel configuration. * @@ -180,6 +201,9 @@ void pse_controller_unregister(struct pse_controller_dev *pcdev); struct device; int devm_pse_controller_register(struct device *dev, struct pse_controller_dev *pcdev); +int devm_pse_irq_helper(struct pse_controller_dev *pcdev, int irq, + int irq_flags, int supported_errs, + const struct pse_irq_desc *d); struct pse_control *of_pse_control_get(struct device_node *node); void pse_control_put(struct pse_control *psec);