Re: [PATCH RFC net-next v3 21/27] net: pse-pd: Add support for getting and setting port priority

[Kory Maincent <kory.maincent@bootlin.com>]

Hello Oleksij,

Thanks for your quick reviews!

On Tue, 26 Nov 2024 09:38:27 +0100
Oleksij Rempel <o.rempel@pengutronix.de> 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.
> 
> Right now we do not have software implementation for dynamic mode,
> implementing configuration of the policies from user space can be
> implemented later. It is enough to provide information about what
> hard coded policy is currently used.

There is no PSE that support static and dynamic mode indeed but the aim was to
be able to disable the budget evaluation strategy.

In fact we could have static strategy with a disconnection policy that do not
power a newly connected PD if we become over budget. This behavior would be
something similar to no budget evaluation strategy.

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