Message-ID: <20241031121104.6f7d669c@kmaincent-XPS-13-7390>
Date: Thu, 31 Oct 2024 12:11:04 +0100
From: Kory Maincent <kory.maincent@...tlin.com>
To: Oleksij Rempel <o.rempel@...gutronix.de>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo
 Abeni <pabeni@...hat.com>, Jonathan Corbet <corbet@....net>, Donald Hunter
 <donald.hunter@...il.com>, Rob Herring <robh@...nel.org>, Andrew Lunn
 <andrew+netdev@...n.ch>, Simon Horman <horms@...nel.org>, Heiner Kallweit
 <hkallweit1@...il.com>, Russell King <linux@...linux.org.uk>, Liam Girdwood
 <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>, Thomas Petazzoni
 <thomas.petazzoni@...tlin.com>, linux-kernel@...r.kernel.org,
 netdev@...r.kernel.org, linux-doc@...r.kernel.org, Kyle Swenson
 <kyle.swenson@....tech>, Dent Project <dentproject@...uxfoundation.org>,
 kernel@...gutronix.de, Maxime Chevallier <maxime.chevallier@...tlin.com>
Subject: Re: [PATCH RFC net-next v2 15/18] net: pse-pd: Add support for
 getting and setting port priority

On Thu, 31 Oct 2024 07:54:08 +0100
Oleksij Rempel <o.rempel@...gutronix.de> wrote:

> > diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h
> > index a1ad257b1ec1..22664b1ea4a2 100644
> > --- a/include/uapi/linux/ethtool.h
> > +++ b/include/uapi/linux/ethtool.h
> > @@ -1002,11 +1002,35 @@ enum ethtool_c33_pse_pw_d_status {
> >   * enum ethtool_c33_pse_events - event list of the C33 PSE controller.
> >   * @ETHTOOL_C33_PSE_EVENT_OVER_CURRENT: PSE output current is too high.
> >   * @ETHTOOL_C33_PSE_EVENT_OVER_TEMP: PSE in over temperature state.
> > + * @ETHTOOL_C33_PSE_EVENT_CONNECTED: PD detected on the PSE.
> > + * @ETHTOOL_C33_PSE_EVENT_DISCONNECTED: PD has been disconnected on the
> > PSE.
> > + * @ETHTOOL_C33_PSE_EVENT_PORT_PRIO_STATIC_ERROR: PSE faced an error in
> > static
> > + *	port priority management mode.
> >   */
> >  
> >  enum ethtool_c33_pse_events {
> > -	ETHTOOL_C33_PSE_EVENT_OVER_CURRENT =	1 << 0,
> > -	ETHTOOL_C33_PSE_EVENT_OVER_TEMP =	1 << 1,
> > +	ETHTOOL_C33_PSE_EVENT_OVER_CURRENT =		1 << 0,
> > +	ETHTOOL_C33_PSE_EVENT_OVER_TEMP =		1 << 1,
> > +	ETHTOOL_C33_PSE_EVENT_CONNECTED =		1 << 2,
> > +	ETHTOOL_C33_PSE_EVENT_DISCONNECTED =		1 << 3,
> > +	ETHTOOL_C33_PSE_EVENT_PORT_PRIO_STATIC_ERROR =	1 << 4,
> > +};  
> 
> Same here, priority concept is not part of the spec, so the C33 prefix
> should be removed.

Ack. So we assume PoDL could have the same interruption events.

> > +/**
> > + * enum pse_port_prio_modes - PSE port priority modes.
> > + * @ETHTOOL_PSE_PORT_PRIO_DISABLED: Port priority disabled.
> > + * @ETHTOOL_PSE_PORT_PRIO_STATIC: PSE static port priority. Port priority
> > + *	based on the power requested during PD classification. This mode
> > + *	is managed by the PSE core.
> > + * @ETHTOOL_PSE_PORT_PRIO_DYNAMIC: PSE dynamic port priority. Port priority
> > + *	based on the current consumption per ports compared to the total
> > + *	power budget. This mode is managed by the PSE controller.
> > + */  
> 
> This part will need some clarification about behavior with mixed port
> configurations. Here is my proposal:
> 
>  * Expected behaviors in mixed port priority configurations:
>  * - When ports are configured with a mix of disabled, static, and dynamic
>  *   priority modes, the following behaviors are expected:
>  *     - Ports with priority disabled (ETHTOOL_PSE_PORT_PRIO_DISABLED) are
>  *       treated with lowest priority, receiving power only if the budget
>  *       remains after static and dynamic ports have been served.
>  *     - Static-priority ports are allocated power up to their requested
>  *       levels during PD classification, provided the budget allows.
>  *     - Dynamic-priority ports receive power based on real-time consumption,
>  *       as monitored by the PSE controller, relative to the remaining budget
>  *       after static ports.

I was not thinking of supporting mixed configuration but indeed why not.
The thing is the Microchip PSE does not support static priority. I didn't find a
way to have only detection and classification enabled without auto activation.
Mixed priority could not be tested for now.

"Requested Power: The requested power of the logical port, related to the
requested class. In case of DSPD, it is the sum of the related class power for
each pair-set. The value is in steps of 0.1 W.
Assigned Class: The assigned classification depends on the requested class and
the available power. An 0xC value means that classification was not assigned
and power was not allocated to this port."

We could set the current limit to all unconnected ports if the budget limit goes
under 100W. This will add complexity as the PD692x0 can set current limit only
inside specific ranges. Maybe it is a bit too specific to Microchip.
Microchip PSE should only support dynamic mode.

>  *
>  * Handling scenarios where power budget is exceeded:
>  * - Hot-plug behavior: If a new device is added that causes the total power
>  *   demand to exceed the PSE budget, the newly added device is de-prioritized
>  *   and shut down to maintain stability for previously connected devices.
>  *   This behavior ensures that existing connections are not disrupted, though
>  *   it may lead to inconsistent behavior if the device is disconnected and
>  *   reconnected (hot-plugged).

Do we want this behavior even if the new device has an highest priority than
other previously connected devices?

>  * - Startup behavior (boot): When the system initializes with attached
> devices,
>  *   the PSE allocates power based on a predefined order (e.g., by port index)
>  *   until the budget is exhausted. Devices connected later in this order may
>  *   not be enabled if they would exceed the power budget, resulting in
> consistent
>  *   behavior during startup but potentially differing from runtime behavior
>  *   (hot-plug).
>  *
>  * - Consistency challenge: These two scenarios—hot-plug vs. system boot—may
> lead
>  *   to different handling of devices. During system boot, power is allocated
>  *   sequentially, potentially leaving out high-priority devices added later
> due to
>  *   a first-come-first-serve approach. In contrast, hot-plug behavior favors
> the
>  *   status quo, maintaining stability for initially connected devices, which
>  *   might not align with the system's prioritization policy.

This could be solve by the future support of persistent configuration. Indeed
the Microchip controller has a non-volatile memory to save the current
configuration (3.1.3) and we could hope future PSE controller could do
the same as there is indeed a consistency challenge.
This support will be added in a later patch series.

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

Powered by blists - more mailing lists