Re: [PATCH v2 15/18] ACPI: platform_profile: Remove platform_profile_handler from exported symbols

["Ilpo Järvinen" <ilpo.jarvinen@linux.intel.com>]

[-- Attachment #1: Type: text/plain, Size: 5174 bytes --]

On Tue, 14 Jan 2025, Kurt Borja wrote:
> On Tue, Jan 14, 2025 at 06:55:34PM +0200, Ilpo Järvinen wrote:
> > On Tue, 14 Jan 2025, Kurt Borja wrote:
> > 
> > > In order to protect the platform_profile_handler from API consumers,
> > > allocate it in platform_profile_register() and modify it's signature
> > > accordingly.
> > > 
> > > Remove the platform_profile_handler from all consumer drivers and
> > > replace them with a pointer to the class device, which is
> > > now returned from platform_profile_register().
> > > 
> > > Replace *pprof with a pointer to the class device in the rest of
> > > exported symbols.
> > > 
> > > Signed-off-by: Kurt Borja <kuurtb@gmail.com>
> > > ---

> > > -int platform_profile_register(struct platform_profile_handler *pprof, void *drvdata)
> > > +struct device *platform_profile_register(struct device *dev, const char *name,
> > > +					 void *drvdata,
> > > +					 const struct platform_profile_ops *ops)
> > >  {
> > > +	int minor;
> > >  	int err;
> > >  
> > > -	/* Sanity check the profile handler */
> > > -	if (!pprof || !pprof->ops->profile_set || !pprof->ops->profile_get ||
> > > -	    !pprof->ops->probe) {
> > > +	/* Sanity check */
> > > +	if (!dev || !name || !ops || !ops->profile_get ||
> > > +	    !ops->profile_set || !ops->probe) {
> > >  		pr_err("platform_profile: handler is invalid\n");
> > > -		return -EINVAL;
> > > +		return ERR_PTR(-EINVAL);
> > >  	}
> > >  
> > > -	err = pprof->ops->probe(drvdata, pprof->choices);
> > > +	struct platform_profile_handler *pprof __free(kfree) = kzalloc(
> > > +		sizeof(*pprof), GFP_KERNEL);
> > > +	if (!pprof)
> > > +		return ERR_PTR(-ENOMEM);
> > > +
> > > +	err = ops->probe(drvdata, pprof->choices);
> > >  	if (err < 0)
> > > -		return err;
> > > +		return ERR_PTR(err);
> > >  
> > >  	if (bitmap_empty(pprof->choices, PLATFORM_PROFILE_LAST)) {
> > >  		pr_err("platform_profile: no available profiles\n");
> > > -		return -EINVAL;
> > > +		return ERR_PTR(-EINVAL);
> > >  	}
> > >  
> > >  	guard(mutex)(&profile_lock);
> > >  
> > >  	/* create class interface for individual handler */
> > > -	pprof->minor = ida_alloc(&platform_profile_ida, GFP_KERNEL);
> > > -	if (pprof->minor < 0)
> > > -		return pprof->minor;
> > > +	minor = ida_alloc(&platform_profile_ida, GFP_KERNEL);
> > > +	if (minor < 0)
> > > +		return ERR_PTR(minor);
> > >  
> > > +	pprof->name = name;
> > > +	pprof->ops = ops;
> > > +	pprof->minor = minor;
> > >  	pprof->class_dev.class = &platform_profile_class;
> > > -	pprof->class_dev.parent = pprof->dev;
> > > +	pprof->class_dev.parent = dev;
> > >  	dev_set_drvdata(&pprof->class_dev, drvdata);
> > >  	dev_set_name(&pprof->class_dev, "platform-profile-%d", pprof->minor);
> > >  	err = device_register(&pprof->class_dev);
> > >  	if (err) {
> > > -		put_device(&pprof->class_dev);
> > > +		put_device(&no_free_ptr(pprof)->class_dev);
> > >  		goto cleanup_ida;
> > >  	}
> > >  
> > > @@ -504,20 +524,21 @@ int platform_profile_register(struct platform_profile_handler *pprof, void *drvd
> > >  	if (err)
> > >  		goto cleanup_cur;
> > >  
> > > -	return 0;
> > > +	return &no_free_ptr(pprof)->class_dev;
> > >  
> > >  cleanup_cur:
> > > -	device_unregister(&pprof->class_dev);
> > > +	device_unregister(&no_free_ptr(pprof)->class_dev);
> > 
> > I don't like how this is architected.
> > 
> > IMO, no_free_ptr() should not appear on error/rollback paths. The pointer 
> > is going to be freed despite the code just told it's not going to be 
> > freed, which sends conflicting signals. Obviously, it is because this 
> > function has relinquished its ownership of the pointer but as is it seems 
> > a dangerous/confusing pattern.
> 
> Makes sense.
> 
> Quick fix would be to replace `goto cleanup_cur` with
> 
> 	device_unregister(&no_free_ptr(pprof)->class_dev);
> 	goto cleanup_ida;
> 
> and add a comment about ownership. Similar to the put_device() call
> above. Is this ok? If not I will think of a better way of writing this.

I think it would still be on the error path which is undesirable. While a 
comment would make it understandable, it would be more logical to call 
no_free_ptr() near device_register() which is when the ownership 
gets transferred.

The trouble with that approach then is that no_free_ptr(pprof) will set 
the pprof to NULL because of how the internal cleanup magic prevents 
automatic freeing of pprof (Don't ask me how I know about that trap :-D).

I suppose you could take pointer of the pprof->class_dev into a local 
variable before making the device_register() call since that is all you 
need after that point?

So my suggestion is along the lines of:

	/* device_register() takes the ownership of the pointer */
	class_dev = &no_free_ptr(pprof)->class_dev;
	err = device_register(class_dev);
	...
	

> > >  cleanup_ida:
> > > -	ida_free(&platform_profile_ida, pprof->minor);
> > > +	ida_free(&platform_profile_ida, minor);
> > >  
> > > -	return err;
> > > +	return ERR_PTR(err);
> > >  }
> > >  EXPORT_SYMBOL_GPL(platform_profile_register);

--
 i.