| Message ID | 20250109-b4-feature_poe_arrange-v2-11-55ded947b510@bootlin.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Arrange PSE core and update TPS23881 driver | expand |
On Thu, 09 Jan 2025 11:18:05 +0100 Kory Maincent wrote: > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com> > > Add support for a PSE device index to report the PSE controller index to > the user through ethtool. This will be useful for future support of power > domains and port priority management. This index is not used in the series, I see later on you'll add power evaluation strategy but that also seems to be within a domain not device? Doesn't it make sense to move patches 11-14 to the next series? The other 11 patches seem to my untrained eye to reshuffle existing stuff, so they would make sense as a cohesive series.
On Thu, 9 Jan 2025 07:59:26 -0800 Jakub Kicinski <kuba@kernel.org> wrote: > On Thu, 09 Jan 2025 11:18:05 +0100 Kory Maincent wrote: > > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com> > > > > Add support for a PSE device index to report the PSE controller index to > > the user through ethtool. This will be useful for future support of power > > domains and port priority management. > > This index is not used in the series, I see later on you'll add power > evaluation strategy but that also seems to be within a domain not > device? > > Doesn't it make sense to move patches 11-14 to the next series? > The other 11 patches seem to my untrained eye to reshuffle existing > stuff, so they would make sense as a cohesive series. Indeed PSE index is used only as user information but there is nothing correlated. You are right maybe we can add PSE index when we have something usable for it. Regards,
On Thu, Jan 09, 2025 at 05:09:57PM +0100, Kory Maincent wrote: > On Thu, 9 Jan 2025 07:59:26 -0800 > Jakub Kicinski <kuba@kernel.org> wrote: > > > On Thu, 09 Jan 2025 11:18:05 +0100 Kory Maincent wrote: > > > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com> > > > > > > Add support for a PSE device index to report the PSE controller index to > > > the user through ethtool. This will be useful for future support of power > > > domains and port priority management. > > > > This index is not used in the series, I see later on you'll add power > > evaluation strategy but that also seems to be within a domain not > > device? > > > > Doesn't it make sense to move patches 11-14 to the next series? > > The other 11 patches seem to my untrained eye to reshuffle existing > > stuff, so they would make sense as a cohesive series. > > Indeed PSE index is used only as user information but there is nothing > correlated. You are right maybe we can add PSE index when we have something > usable for it. No user, means, it is not exposed to the user space, it is not about actual user space users.
On Thu, 9 Jan 2025 17:27:03 +0100 Oleksij Rempel <o.rempel@pengutronix.de> wrote: > On Thu, Jan 09, 2025 at 05:09:57PM +0100, Kory Maincent wrote: > > On Thu, 9 Jan 2025 07:59:26 -0800 > > Jakub Kicinski <kuba@kernel.org> wrote: > > > > > On Thu, 09 Jan 2025 11:18:05 +0100 Kory Maincent wrote: > [...] > > > > > > This index is not used in the series, I see later on you'll add power > > > evaluation strategy but that also seems to be within a domain not > > > device? > > > > > > Doesn't it make sense to move patches 11-14 to the next series? > > > The other 11 patches seem to my untrained eye to reshuffle existing > > > stuff, so they would make sense as a cohesive series. I think I should only drop patch 11 and 12 from this series which add something new while the rest is reshuffle or fix code. > > Indeed PSE index is used only as user information but there is nothing > > correlated. You are right maybe we can add PSE index when we have something > > usable for it. > > No user, means, it is not exposed to the user space, it is not about > actual user space users. I may have understood incorrectly but still. Not sure the PSE device index is interesting for now even in the budget evaluation strategy series. It is related to PSE power domains therefore PSE power domain index solely should be sufficient. Regards,
On Thu, 9 Jan 2025 17:27:03 +0100 Oleksij Rempel wrote: > > > This index is not used in the series, I see later on you'll add power > > > evaluation strategy but that also seems to be within a domain not > > > device? > > > > > > Doesn't it make sense to move patches 11-14 to the next series? > > > The other 11 patches seem to my untrained eye to reshuffle existing > > > stuff, so they would make sense as a cohesive series. > > > > Indeed PSE index is used only as user information but there is nothing > > correlated. You are right maybe we can add PSE index when we have something > > usable for it. Oh, maybe you want to do the devlink-y thing then? Devlink identifies its devices by what I'd call "sysfs name" - bus name and device name. This is more meaningful to user than an artificial ID. Downside is it won't work well if you have multiple objects under a single struct device, or multiple struct device for one ASIC (e.g. multiple PCIe PFs on one PCIe card) > No user, means, it is not exposed to the user space, it is not about > actual user space users. Can't parse this :)
On Thu, Jan 09, 2025 at 08:50:02AM -0800, Jakub Kicinski wrote: > On Thu, 9 Jan 2025 17:27:03 +0100 Oleksij Rempel wrote: > > > > This index is not used in the series, I see later on you'll add power > > > > evaluation strategy but that also seems to be within a domain not > > > > device? > > > > > > > > Doesn't it make sense to move patches 11-14 to the next series? > > > > The other 11 patches seem to my untrained eye to reshuffle existing > > > > stuff, so they would make sense as a cohesive series. > > > > > > Indeed PSE index is used only as user information but there is nothing > > > correlated. You are right maybe we can add PSE index when we have something > > > usable for it. > > Oh, maybe you want to do the devlink-y thing then? > Devlink identifies its devices by what I'd call "sysfs name" - > bus name and device name. > This is more meaningful to user than an artificial ID. > Downside is it won't work well if you have multiple objects > under a single struct device, or multiple struct device for > one ASIC (e.g. multiple PCIe PFs on one PCIe card) > > > No user, means, it is not exposed to the user space, it is not about > > actual user space users. > > Can't parse this :) OK, please forget it :)
On Thu, Jan 09, 2025 at 05:49:42PM +0100, Kory Maincent wrote: > On Thu, 9 Jan 2025 17:27:03 +0100 > Oleksij Rempel <o.rempel@pengutronix.de> wrote: > > > On Thu, Jan 09, 2025 at 05:09:57PM +0100, Kory Maincent wrote: > > > On Thu, 9 Jan 2025 07:59:26 -0800 > > > Jakub Kicinski <kuba@kernel.org> wrote: > > > > > > > On Thu, 09 Jan 2025 11:18:05 +0100 Kory Maincent wrote: > > [...] > > > > > > > > This index is not used in the series, I see later on you'll add power > > > > evaluation strategy but that also seems to be within a domain not > > > > device? > > > > > > > > Doesn't it make sense to move patches 11-14 to the next series? > > > > The other 11 patches seem to my untrained eye to reshuffle existing > > > > stuff, so they would make sense as a cohesive series. > > I think I should only drop patch 11 and 12 from this series which add something > new while the rest is reshuffle or fix code. > > > > Indeed PSE index is used only as user information but there is nothing > > > correlated. You are right maybe we can add PSE index when we have something > > > usable for it. > > > > No user, means, it is not exposed to the user space, it is not about > > actual user space users. > > I may have understood incorrectly but still. Not sure the PSE device index is > interesting for now even in the budget evaluation strategy series. It is > related to PSE power domains therefore PSE power domain index solely should be > sufficient. Oh.. You can drop it for now. :)
On Thu, 9 Jan 2025 17:49:42 +0100 Kory Maincent wrote: > > > > Doesn't it make sense to move patches 11-14 to the next series? > > > > The other 11 patches seem to my untrained eye to reshuffle existing > > > > stuff, so they would make sense as a cohesive series. > > I think I should only drop patch 11 and 12 from this series which add something > new while the rest is reshuffle or fix code. I mentioned 13 & 14 because I suspected we may need to wait for the maintainers of regulator, and merge 13 in some special way. Looks like Mark merged 13 already, so
On Thu, Jan 09, 2025 at 11:51:41AM -0800, Jakub Kicinski wrote: > On Thu, 9 Jan 2025 17:49:42 +0100 Kory Maincent wrote: > > I think I should only drop patch 11 and 12 from this series which add something > > new while the rest is reshuffle or fix code. > I mentioned 13 & 14 because I suspected we may need to wait for > the maintainers of regulator, and merge 13 in some special way. > Looks like Mark merged 13 already, so
diff --git a/drivers/net/pse-pd/pse_core.c b/drivers/net/pse-pd/pse_core.c index 887a477197a6..830e8d567d4d 100644 --- a/drivers/net/pse-pd/pse_core.c +++ b/drivers/net/pse-pd/pse_core.c @@ -13,6 +13,7 @@ static DEFINE_MUTEX(pse_list_mutex); static LIST_HEAD(pse_controller_list); +static DEFINE_IDA(pse_ida); /** * struct pse_control - a PSE control @@ -448,6 +449,10 @@ int pse_controller_register(struct pse_controller_dev *pcdev) mutex_init(&pcdev->lock); INIT_LIST_HEAD(&pcdev->pse_control_head); + ret = ida_alloc_max(&pse_ida, INT_MAX, GFP_KERNEL); + if (ret < 0) + return ret; + pcdev->id = ret; if (!pcdev->nr_lines) pcdev->nr_lines = 1; @@ -461,12 +466,12 @@ int pse_controller_register(struct pse_controller_dev *pcdev) ret = of_load_pse_pis(pcdev); if (ret) - return ret; + goto free_pse_ida; if (pcdev->ops->setup_pi_matrix) { ret = pcdev->ops->setup_pi_matrix(pcdev); if (ret) - return ret; + goto free_pse_ida; } /* Each regulator name len is pcdev dev name + 7 char + @@ -483,15 +488,17 @@ int pse_controller_register(struct pse_controller_dev *pcdev) continue; reg_name = devm_kzalloc(pcdev->dev, reg_name_len, GFP_KERNEL); - if (!reg_name) - return -ENOMEM; + if (!reg_name) { + ret = -ENOMEM; + goto free_pse_ida; + } snprintf(reg_name, reg_name_len, "pse-%s_pi%d", dev_name(pcdev->dev), i); ret = devm_pse_pi_regulator_register(pcdev, reg_name, i); if (ret) - return ret; + goto free_pse_ida; } mutex_lock(&pse_list_mutex); @@ -499,6 +506,10 @@ int pse_controller_register(struct pse_controller_dev *pcdev) mutex_unlock(&pse_list_mutex); return 0; + +free_pse_ida: + ida_free(&pse_ida, pcdev->id); + return ret; } EXPORT_SYMBOL_GPL(pse_controller_register); @@ -509,6 +520,7 @@ EXPORT_SYMBOL_GPL(pse_controller_register); void pse_controller_unregister(struct pse_controller_dev *pcdev) { pse_release_pis(pcdev); + ida_free(&pse_ida, pcdev->id); mutex_lock(&pse_list_mutex); list_del(&pcdev->list); mutex_unlock(&pse_list_mutex); @@ -771,6 +783,8 @@ int pse_ethtool_get_status(struct pse_control *psec, pcdev = psec->pcdev; ops = pcdev->ops; + status->pse_id = pcdev->id; + mutex_lock(&pcdev->lock); ret = ops->pi_get_admin_state(pcdev, psec->id, &admin_state); if (ret) diff --git a/include/linux/pse-pd/pse.h b/include/linux/pse-pd/pse.h index b5ae3dcee550..be877dea6a11 100644 --- a/include/linux/pse-pd/pse.h +++ b/include/linux/pse-pd/pse.h @@ -78,6 +78,7 @@ struct pse_pw_limit_ranges { /** * struct ethtool_pse_control_status - PSE control/channel status. * + * @pse_id: index number of the PSE. * @podl_admin_state: operational state of the PoDL PSE * functions. IEEE 802.3-2018 30.15.1.1.2 aPoDLPSEAdminState * @podl_pw_status: power detection status of the PoDL PSE. @@ -99,6 +100,7 @@ struct pse_pw_limit_ranges { * ranges */ struct ethtool_pse_control_status { + u32 pse_id; enum ethtool_podl_pse_admin_state podl_admin_state; enum ethtool_podl_pse_pw_d_status podl_pw_status; enum ethtool_c33_pse_admin_state c33_admin_state; @@ -208,6 +210,7 @@ struct pse_pi { * @types: types of the PSE controller * @pi: table of PSE PIs described in this controller device * @no_of_pse_pi: flag set if the pse_pis devicetree node is not used + * @id: Index of the PSE */ struct pse_controller_dev { const struct pse_controller_ops *ops; @@ -221,6 +224,7 @@ struct pse_controller_dev { enum ethtool_pse_types types; struct pse_pi *pi; bool no_of_pse_pi; + u32 id; }; #if IS_ENABLED(CONFIG_PSE_CONTROLLER)