Re: [PATCH v5] dmaengine: idxd: Do not use devm for 'struct device' object allocation

[Dan Carpenter <dan.carpenter@oracle.com>]

On Wed, Mar 24, 2021 at 08:35:25PM -0300, Jason Gunthorpe wrote:
> On Wed, Mar 24, 2021 at 10:52:52PM +0300, Dan Carpenter wrote:
> > On Wed, Mar 24, 2021 at 01:52:46PM -0300, Jason Gunthorpe wrote:
> > > On Wed, Mar 24, 2021 at 09:13:35AM -0700, Dan Williams wrote:
> > > 
> > > > Which is just:
> > > > 
> > > > device_initialize()
> > > > dev_set_name()
> > > > 
> > > > ...then the name is set as early as the device is ready to filled in
> > > > with other details. Just checking for dev_set_name() failures does not
> > > > move the api forward in my opinion.
> > > 
> > > This doesn't work either as the release function must be set after
> > > initialize but before dev_set_name(), otherwise we both can't and must
> > > call put_device() after something like this fails.
> > > 
> > > I can't see an option other than bite the bullet and fix things.
> > > 
> > > A static tool to look for these special lifetime rules around the
> > > driver core would be nice.
> > 
> > If y'all are specific enough about what you want, then I can write the
> > check for you.  What I really want is some buggy sample code and the
> > warning you want me to print.  I kind of vaguely know that devm_ life
> > time rules are tricky but I don't know the details.
> 
> This is driver core rules.
> 
> The setup is:
> 
> struct foo_device
> {
>     struct device dev;
> }
> 
> struct foo_device *fdev = kzalloc(sizeo(*fdev), GFP_KERNEL);
> 
> Then in each of these situations:
> 
>   device_initialize(&fdev->dev);
>   // WARNING initialized struct device's must be destroyed with put_device()
>   kfree(fdev); 
> 

This email is perfect!  Exactly what I want.  My one question would be
what happens if we don't call put_device() in this first example?

The laziest thing would be to just add them to check_unwind.c:

	{ "device_initialize", ALLOC,   0, "$" },
	{ "dev_set_name",      ALLOC,   0, "$" },
	{ "device_register",   ALLOC,   0, "$" },
	{ "put_device", RELEASE, 0, "$" },

The check_unwind.c file assumes that every function cleans up after
itself on error.  It doesn't look for the kfree(fdev).  I could make
kfree() the rule if you want.  I tested it on one file to see if it
generated a warning and it does.

net/atm/atm_sysfs.c:167 atm_register_sysfs() warn: '&adev->class_dev' not released on lines: 153,167.

The line 153 is a real bug, but line 167 calls device_del().  The
comments device_del() say "NOTE: this should be called manually _iff_
device_add() was also called manually." which suggests that this is a
different sort of bug...  Should I add device_del() optional release
function?  I have device_unregister() there already.

	{ "device_unregister", RELEASE, 0, "$" },
	{ "device_del", RELEASE, 0, "$" },

Do we care about nesting?

Anyway, I'm going to pull these out of check_unwind.c into their own
file so that I can make the warning messages a bit better.

regards,
dan carpenter