Re: [PATCH v3 46/57] perf: Simplify pmu_dev_alloc()

[Greg KH]

On Mon, Jun 12, 2023 at 11:44:00AM +0200, Peter Zijlstra wrote:
> On Mon, Jun 12, 2023 at 11:07:59AM +0200, Peter Zijlstra wrote:
> > 
> > Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> > ---
> >  kernel/events/core.c |   65 ++++++++++++++++++++++++---------------------------
> >  1 file changed, 31 insertions(+), 34 deletions(-)
> > 
> > --- a/kernel/events/core.c
> > +++ b/kernel/events/core.c
> > @@ -11285,49 +11285,46 @@ static void pmu_dev_release(struct devic
> >  
> >  static int pmu_dev_alloc(struct pmu *pmu)
> >  {
> > +	int ret;
> >  
> > +	struct device *dev __free(put_device) =
> > +		kzalloc(sizeof(struct device), GFP_KERNEL);
> > +	if (!dev)
> > +		return -ENOMEM;
> >  
> > +	dev->groups = pmu->attr_groups;
> > +	device_initialize(dev);
> >  
> > +	dev_set_drvdata(dev, pmu);
> > +	dev->bus = &pmu_bus;
> > +	dev->release = pmu_dev_release;
> >  
> > +	ret = dev_set_name(dev, "%s", pmu->name);
> >  	if (ret)
> > +		return ret;
> >  
> > +	ret = device_add(dev);
> >  	if (ret)
> > +		return ret;
> >  
> > +	struct device *del __free(device_del) = dev;
> 
> Greg, I'm not much familiar with the whole device model, but it seems
> unfortunate to me that one has to call device_del() explicitly if we
> already have a put_device() queued.
> 
> Is there a saner way to write this?

Ok, to answer my other question, yes, you are changing the free call
here in the "middle" of the function, sorry, I missed "del" vs. "dev"
and was wondering how this would work...

This should work, it's tricky, especially:

> > +	no_free_ptr(del);
> > +	pmu->dev = no_free_ptr(dev);

this.

I had to stare at it for a while to realize that yes, you are calling
the two different "cleanup" functions prior to thes lines, on the same
pointer, and that the order is correct.

Ick, this is going to be a rough audit for bus code that gets converted
to this, BUT bonus is that once it's done, any changes to the middle of
the function should "just work", and it's a good task for an intern to
do :)


Reviewed-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx>

Mind if I try this series to convert a more "normal" driver to see how
it works with that?  That's going to be the true test, see if the
changes make sense to someone who doesn't really know the internals of
the driver core like this...

thanks,

greg k-h