On Tue, Mar 23, 2021 at 10:07:46PM -0700, Dan Williams wrote:
> On Thu, Mar 4, 2021 at 10:04 AM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
> >
> > On Wed, Mar 03, 2021 at 07:56:30AM -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.
> > > In the process also fix up issues from the fallout of the changes.
> > >
> > > Reported-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > Fixes: bfe1d56091c1 ("dmaengine: idxd: Init and probe for Intel data accelerators")
> > > Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx>
> > > Reviewed-by: Dan Williams <dan.j.williams@xxxxxxxxx>
> > > v5:
> > > - Rebased against 5.12-rc dmaengine/fixes
> > > v4:
> > > - fix up the life time of cdev creation/destruction (Jason)
> > > - Tested with KASAN and other memory allocation leak detections. (Jason)
> > >
> > > v3:
> > > - Remove devm_* for irq request and cleanup related bits (Jason)
> > > v2:
> > > - Remove all devm_* alloc for idxd_device (Jason)
> > > - Add kref dep for dma_dev (Jason)
> > >
> > > drivers/dma/idxd/cdev.c | 32 +++---
> > > drivers/dma/idxd/device.c | 20 ++-
> > > drivers/dma/idxd/dma.c | 13 ++
> > > drivers/dma/idxd/idxd.h | 8 +
> > > drivers/dma/idxd/init.c | 261 +++++++++++++++++++++++++++++++++------------
> > > drivers/dma/idxd/irq.c | 6 +
> > > drivers/dma/idxd/sysfs.c | 77 +++++++++----
> > > 7 files changed, 290 insertions(+), 127 deletions(-)
> > >
> > > diff --git a/drivers/dma/idxd/cdev.c b/drivers/dma/idxd/cdev.c
> > > index 0db9b82ed8cf..1b98e06fa228 100644
> > > +++ b/drivers/dma/idxd/cdev.c
> > > @@ -259,6 +259,7 @@ static int idxd_wq_cdev_dev_setup(struct idxd_wq *wq)
> > > return -ENOMEM;
> > >
> > > dev = idxd_cdev->dev;
> > > + device_initialize(dev);
> > > dev->parent = &idxd->pdev->dev;
> > > dev_set_name(dev, "%s/wq%u.%u", idxd_get_dev_name(idxd),
> > > idxd->id, wq->id);
> >
> > dev_set_name() can fail
>
> Something bubbled up in my mind several hours after the fact looking
> at Dave's lifetime reworks...
>
> As long as there are no error returns between dev_set_name() and
> device_{add,register}() then those will abort with a "name_error:"
> exit and require put_device() to clean up the name.
IMHO, that is gross. We should not rely on implicit error handling like
this, it is too easy to make later change and not realize that
dev_set_name() can't be moved.
> I'd much rather drivers depend on proper dev_set_name() ordering
> relative to device_add() than pollute drivers with pedantic
> dev_set_name() error
I want to go the other direction and add a WARN_ON to dev_set_name()
if device_initialize() hasn't been called yet.
IMHO the initialize and add pattern is a bad idea just to save 1 line
of code. Look at how many mistakes are in IDXD alone. Lots of places
get the very tricky switch to put not kfree after register fails wrong
as well.
> handling. Unhandled dev_set_name() followed by device_{add,register}()
> is the predominant registration pattern and it isn't broken afaics.
It is very fragile, and IMHO, wrong. As a general pattern drivers
should be setting the name very early so they can start using things
like dev_dbg().
> Only buses that expressly want to avoid fallback to a bus provided
> dev_name() would need to make sure that dev_set_name() is successful.
Yuk, now it is bus dependent if the shortcut works?
People need to code this stuff robustly! Call device_initialize()
early, check the error from dev_set_name(), do not call
device_register()
I *constantly* see drivers using these APIs wrong. Look at how many
bugs are in IDXD around this area alone.
Making it more subtle to save LOC is the wrong direction.
Jason
