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. 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
handling. Unhandled dev_set_name() followed by device_{add,register}()
is the predominant registration pattern and it isn't broken afaics.
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.
I don't think Dave needs to respin for this, but as I went to
investigate why those changes rubbed me the wrong way it led me back
here.
