Re: [PATCH net-next v5 02/12] net: pse-pd: Add support for reporting events

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

Hello Oleksij,

On Fri, 21 Feb 2025 09:50:33 +0100
Oleksij Rempel <o.rempel@pengutronix.de> wrote:

> Hi Kory,
> 
> On Tue, Feb 18, 2025 at 05:19:06PM +0100, Kory Maincent wrote:
> > From: Kory Maincent (Dent Project) <kory.maincent@bootlin.com>
> > 
> > Add support for devm_pse_irq_helper() to register PSE interrupts. This aims
> > to report events such as over-current or over-temperature conditions
> > similarly to how the regulator API handles them but using a specific PSE
> > ethtool netlink socket.  
> 
> Thank you for your work. Here some comments.
> 
> ...
> 
> > --- a/drivers/net/mdio/fwnode_mdio.c
> > +++ b/drivers/net/mdio/fwnode_mdio.c
> > @@ -18,7 +18,8 @@ MODULE_LICENSE("GPL");
> >  MODULE_DESCRIPTION("FWNODE MDIO bus (Ethernet PHY) accessors");
> >  
> >  static struct pse_control *
> > -fwnode_find_pse_control(struct fwnode_handle *fwnode)
> > +fwnode_find_pse_control(struct fwnode_handle *fwnode,
> > +			struct phy_device *phydev)
> >  {  
> 
> This change seems to be not directly related to the commit message.
> Is it the preparation for the multi-phy support?

I need to save the phy_device related to PSE control to use the right network
interface for the ethtool notification. (ethnl_pse_send_ntf())
Indeed I have not described this in the commit message.

> ...
> 
> > +/**
> > + * pse_to_regulator_notifs - Convert PSE notifications to Regulator
> > + *			     notifications
> > + * @notifs: PSE notifications
> > + *
> > + * Return: Regulator notifications
> > + */
> > +static unsigned long pse_to_regulator_notifs(unsigned long notifs)  
> 
> I prefer converting it the other way around to make it reusable for
> plain regulator-based PSEs. For example, the podl-pse-regulator driver
> won’t have its own interrupt handler but will instead use
> devm_regulator_register_notifier().

The driver PIs part send PSE notifications which will be converted to regulator
events from the core. It is posting events.
If you use devm_regulator_register_notifier() you will registers a listener for
the regulator events. It is two distinct things.

> Even full-fledged PSE controllers like the PD692x0 are just one part of
> a larger chain of regulators. An overcurrent event may originate from a
> downstream regulator that is not part of the PD692x0 itself. In this
> case, we need to process the event from the downstream regulator,
> convert it into an ethtool event, and forward it to the user.

If you want to do something in case of downstream regulator events you will deal
with regulator events not PSE events. I think you want to disable PIs in case of
event like downstream regulator over current.
What policy should we use? Should we disable all the PIs or only disabled the
low priority like the budget evaluation strategy of this series? As it is over
current event not related to budget we don't know how many PIs we should
disable.

Still as said before it is a distinct development that could be tackled later.

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