Re: [PATCH net-next v7 04/13] net: pse-pd: Add support for PSE power domains

[Kory Maincent]

Hello Oleksij,

On Tue, 8 Apr 2025 20:09:36 +0200
Oleksij Rempel <o.rempel@xxxxxxxxxxxxxx> wrote:

> Hi Kory,
> 
> here are some points
> 
> On Tue, Apr 08, 2025 at 04:32:13PM +0200, Kory Maincent wrote:
> > From: Kory Maincent (Dent Project) <kory.maincent@xxxxxxxxxxx>
> > +static struct pse_power_domain *devm_pse_alloc_pw_d(struct device *dev)
> > +{
> > +	struct pse_power_domain *pw_d;
> > +	int index, ret;
> > +
> > +	pw_d = devm_kzalloc(dev, sizeof(*pw_d), GFP_KERNEL);
> > +	if (!pw_d)
> > +		return ERR_PTR(-ENOMEM);
> > +
> > +	ret = xa_alloc(&pse_pw_d_map, &index, pw_d, XA_LIMIT(1,
> > PSE_PW_D_LIMIT),
> > +		       GFP_KERNEL);
> > +	if (ret)
> > +		return ERR_PTR(ret);  
> 
> Missing "kref_init(&pw_d->refcnt);" ?

Oh yes, indeed thanks.

> > +
> > +		pw_d = devm_pse_alloc_pw_d(pcdev->dev);
> > +		if (IS_ERR_OR_NULL(pw_d)) {  
> 
> s/IS_ERR_OR_NULL/IS_ERR
> 
> devm_pse_alloc_pw_d() is not returning NULL.
 
Yes, that's right!

> > +			ret = PTR_ERR(pw_d);
> > +			goto out;
> > +		}
> > +
> > +		supply = regulator_get(&rdev->dev, rdev->supply_name);
> > +		if (IS_ERR(supply)) {
> > +			xa_erase(&pse_pw_d_map, pw_d->id);
> > +			ret = PTR_ERR(supply);  
> 
> Here:
> Either we need to ensure pse_flush_pw_ds() handles incomplete setups
> or immediately clean up earlier entries in the loop when an error
> occurs.

The pw_d has not yet been saved to pcdev->pi[i].pw_d so we need to remove the
pw_d from the xarray here. pse_flush_pw_ds will deal with all the pw_d entries
in pcdev->pi[x].

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