Re: [PATCH v3 5/6] cxl/pci: Introduce cdevm_file_operations

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Mon, 2 Aug 2021 09:15:10 -0700
Dan Williams <dan.j.williams@intel.com> wrote:

> On Mon, Aug 2, 2021 at 8:05 AM Jonathan Cameron
> <Jonathan.Cameron@huawei.com> wrote:
> >
> > On Fri, 30 Jul 2021 13:07:22 -0700
> > Dan Williams <dan.j.williams@intel.com> wrote:
> >  
> > > In preparation for moving cxl_memdev allocation to the core, introduce
> > > cdevm_file_operations to coordinate file operations shutdown relative to
> > > driver data release.
> > >
> > > The motivation for moving cxl_memdev allocation to the core (beyond
> > > better file organization of sysfs attributes in core/ and drivers in
> > > cxl/), is that device lifetime is longer than module lifetime. The cxl_pci
> > > module should be free to come and go without needing to coordinate with
> > > devices that need the text associated with cxl_memdev_release() to stay
> > > resident. The move will fix a use after free bug when looping driver
> > > load / unload with CONFIG_DEBUG_KOBJECT_RELEASE=y.
> > >
> > > Another motivation for passing in file_operations to the core cxl_memdev
> > > creation flow is to allow for alternate drivers, like unit test code, to
> > > define their own ioctl backends.
> > >
> > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com>
> > > Signed-off-by: Dan Williams <dan.j.williams@intel.com>  
> >
> > Hi Dan / Ben,
> >
> > One utterly trivial question inline, otherwise looks good.
> >  
> > > ---
> > >  drivers/cxl/mem.h |   15 ++++++++++++
> > >  drivers/cxl/pci.c |   65 +++++++++++++++++++++++++++++++----------------------
> > >  2 files changed, 53 insertions(+), 27 deletions(-)
> > >
> > > diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h
> > > index 8f02d02b26b4..77f4411c7c6a 100644
> > > --- a/drivers/cxl/mem.h
> > > +++ b/drivers/cxl/mem.h
> > > @@ -34,6 +34,21 @@
> > >   */
> > >  #define CXL_MEM_MAX_DEVS 65536
> > >
> > > +/**
> > > + * struct cdevm_file_operations - devm coordinated cdev file operations
> > > + * @fops: typical file operations  
> >
> > Why typical? Seems that simply saying "file operations" conveys the same
> > information.  What is atypical here?  
> 
> Hmm, I was thinking the need for an extra shutdown operation is
> "atypical", but you're right this description does not add anything.
> I'll change it too:
> 
> @fops: file operations that are synchronized against @shutdown

Indeed, much better.

Jonathan