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

From: Jason Gunthorpe <jgg@nvidia.com>
To: Dave Jiang <dave.jiang@intel.com>
Cc: <vkoul@kernel.org>, Dan Williams <dan.j.williams@intel.com>,
	<dmaengine@vger.kernel.org>
Subject: Re: [PATCH] dmaengine: idxd: Do not use devm for 'struct device' object allocation
Date: Tue, 23 Feb 2021 13:08:51 -0400	[thread overview]
Message-ID: <20210223170851.GZ4247@nvidia.com> (raw)
In-Reply-To: <397867c3-0b0d-82aa-37c1-deccd24d8f6c@intel.com>

On Tue, Feb 23, 2021 at 08:27:46AM -0700, Dave Jiang wrote:
> 
> On 2/23/2021 5:59 AM, Jason Gunthorpe wrote:
> > On Thu, Feb 18, 2021 at 02:31:54PM -0700, Dave Jiang wrote:
> > > Remove devm_* allocation of memory of 'struct device' objects.
> > > The devm_* lifetime is incompatible with device->release() lifetime.
> > > Address issues flagged by CONFIG_DEBUG_KOBJECT_RELEASE. Add release
> > > functions for each component in order to free the allocated memory at
> > > the appropriate time. Each component such as wq, engine, and group now
> > > needs to be allocated individually in order to setup the lifetime properly.
> > I really don't understand why idxd has so many struct device objects.
> > 
> > Typically I expect a simple driver to have exactly one, usually
> > provided by its subsystem.
> > 
> > What is the purpose?
> 
> When we initially designed this, Dan suggested to tie the device and
> workqueue enabling to the Linux device model so that the enabling/disabling
> can be done via bind/unbind of the sub drivers. So that's how we ended up
> with all the 'struct device' under idxd and one for each configurable
> component of the hardware device.

IDXD created its own little bus just for that?? :\

It is really weird that something called a workqueue would show up in
the driver model at all

> > > +static int idxd_allocate_wqs(struct idxd_device *idxd)
> > > +{
> > > +	struct device *dev = &idxd->pdev->dev;
> > > +	struct idxd_wq *wq;
> > > +	int i, rc;
> > > +
> > > +	idxd->wqs = devm_kcalloc(dev, idxd->max_wqs, sizeof(struct idxd_wq *),
> > > +				 GFP_KERNEL);
> > Memory stored in the idxd_device should be freed by the release
> > function, not by devm.
> 
> These memory are not tied to 'struct device' and should be able to managed
> by devm right?

If it is a pointer inside the kref'd struct then it should be free'd
in the kref's release function.

Those should not use devm as they already have clearly defined
lifecycles based on the kref.

Either holding the kref says the struct and everything in it/pointed
to by it is valid, or you have an inscrutable mess.

Jason