Re: [PATCH v7 8/8] dmaengine: idxd: fix cdev setup and free device lifetime issues

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, Mar 22, 2021 at 04:44:24PM -0700, Dave Jiang wrote:
> 
> On 3/22/2021 4:38 PM, Dan Williams wrote:
> > On Mon, Mar 22, 2021 at 4:32 PM Dave Jiang <dave.jiang@intel.com> wrote:
> > > The char device setup and cleanup has device lifetime issues regarding when
> > > parts are initialized and cleaned up. The initialization of struct device is
> > > done incorrectly. device_initialize() needs to be called on the 'struct
> > > device' and then additional changes can be added. The ->release() function
> > > needs to be setup via device_type before dev_set_name() to allow proper
> > > cleanup. The change re-parents the cdev under the wq->conf_dev to get
> > > natural reference inheritance. No known dependency on the old device path exists.
> > > 
> > > Reported-by: Jason Gunthorpe <jgg@nvidia.com>
> > > Fixes: 42d279f9137a ("dmaengine: idxd: add char driver to expose submission portal to userland")
> > > Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> > > Reviewed-by: Dan Williams <dan.j.williams@intel.com>
> > [..]
> > > diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
> > > index 03079ff54889..8a08988ea9d1 100644
> > > +++ b/drivers/dma/idxd/sysfs.c
> > > @@ -1174,8 +1174,14 @@ static ssize_t wq_cdev_minor_show(struct device *dev,
> > >                                    struct device_attribute *attr, char *buf)
> > >   {
> > >          struct idxd_wq *wq = container_of(dev, struct idxd_wq, conf_dev);
> > > +       int minor = -1;
> > > 
> > > -       return sprintf(buf, "%d\n", wq->idxd_cdev.minor);
> > > +       mutex_lock(&wq->wq_lock);
> > > +       if (wq->idxd_cdev)
> > > +               minor = wq->idxd_cdev->minor;
> > > +       mutex_unlock(&wq->wq_lock);
> > > +
> > > +       return sprintf(buf, "%d\n", minor);
> > As I mentioned, let's not emit a negative value here. ...not that
> > userspace should be using this awkward recreation of the existing core
> > 'dev' attribute anyway.
> > 
> > if (minor == -1)
> >      return -ENXIO;
> > return sprintf(buf, "%d\n", minor);
> Ok I'll update. This will go away when we convert to UACCE based driver.

Also ensure all new syfs callbacks are using sysfs_emit()

Jason