On Tue, 26 Nov 2024 16:31:55 +0100
Kory Maincent <kory.maincent@xxxxxxxxxxx> wrote:
> Hello Oleksij,
>
> Thanks for your quick reviews!
>
> On Tue, 26 Nov 2024 09:38:27 +0100
> Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:
>
> > > +int pse_ethtool_set_prio_mode(struct pse_control *psec,
> > > + struct netlink_ext_ack *extack,
> > > + u32 prio_mode)
> > > +{
> > > + struct pse_controller_dev *pcdev = psec->pcdev;
> > > + const struct pse_controller_ops *ops;
> > > + int ret = 0, i;
> > > +
> > > + if (!(prio_mode & pcdev->port_prio_supp_modes)) {
> > > + NL_SET_ERR_MSG(extack, "priority mode not supported");
> > > + return -EOPNOTSUPP;
> > > + }
> > > +
> > > + if (!pcdev->pi[psec->id].pw_d) {
> > > + NL_SET_ERR_MSG(extack, "no power domain attached");
> > > + return -EOPNOTSUPP;
> > > + }
> > > +
> > > + /* ETHTOOL_PSE_PORT_PRIO_DISABLED can't be ORed with another mode
> > > */
> > > + if (prio_mode & ETHTOOL_PSE_PORT_PRIO_DISABLED &&
> > > + prio_mode & ~ETHTOOL_PSE_PORT_PRIO_DISABLED) {
> > > + NL_SET_ERR_MSG(extack,
> > > + "port priority can't be enabled and
> > > disabled simultaneously");
> > > + return -EINVAL;
> > > + }
> > > +
> > > + ops = psec->pcdev->ops;
> > > +
> > > + /* We don't want priority mode change in the middle of an
> > > + * enable/disable call
> > > + */
> > > + mutex_lock(&pcdev->lock);
> > > + pcdev->pi[psec->id].pw_d->port_prio_mode = prio_mode;
> >
> > In proposed implementation we have can set policies per port, but it
> > will affect complete domain. This is not good. It feels like a separate
> > challenge with extra discussion and work. I would recommend not to
> > implement policy setting right now.
> >
> > If you will decide to implement setting of policies anyway, then we need
> > to discuss the interface.
> > - If the policy should be done per domain, then we will need a separate
> > interface to interact with domains.
> > Pro: seems to be easier to implement.
> > - If we will go with policy per port, wich would make sense too, then
> > some rework of this patch is needed.
> > Pro: can combine best of both strategies: set ports with wide load
> > range to static strategy and use dynamic strategy on other ports.
We already talked about it but a policies per port seems irrelevant to me.
https://lore.kernel.org/netdev/ZySR75i3BEzNbjnv@xxxxxxxxxxxxxx/
How do we compare the priority value of ports that use different budget
strategy? How do we manage in the same power domain two ports with
different budget strategies or disconnection policies?
We indeed may need a separate interface to configure the PSE power domain
budget strategies and disconnection policies.
I think not being able to set the budget evaluation strategy is not relevant
for now as we don't have PSE which could support both, but being able to
set the disconnection policies may be relevant.
If we don't add this support to this series how do we decide which is the
default disconnection policy supported?
Regards,
--
Köry Maincent, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
