On Wed, Mar 24, 2021 at 5:33 AM Jason Gunthorpe <jgg@xxxxxxxxxx> wrote:
>
> 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.
I came to that conclusion after the prospect of going to fix all the
other call sites that got this wrong made me balk.
The other option I was considering was moving the name into some
initialization helper, something like:
device_setup(struct device *dev, const char *fmt, ...)
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.
