| Message ID | 20210715194125.898305-6-ben.widawsky@intel.com |
|---|---|
| State | New, archived |
| Headers | show |
| Series | CXL core reorganization | expand |
On Thu, Jul 15, 2021 at 12:41 PM Ben Widawsky <ben.widawsky@intel.com> wrote: > > Drivers that use cxl_core for registering a cxl_memdev will likely > require synchronization with the core for shutdown so as to not race > against the device going away. This isn't quite right. There are three independent lifetimes to worry about: the liveness of the file_operations (bounded by userspace last close), the driver attach lifetime (bounded by ->remove()), and the device lifetime (bounded by last put_device()). The devm vs file_operations lifetime collision is a common problem [1]. I'd go so far as to define something like: struct devres_file_operations { struct file_operations *fops; (int)(*shutdown)(struct device *); }; ...where the definition of @shutdown is: "invoked in the devres release path to disconnect any driver instance data from @dev. It assumes synchronization with any fops operation that requires driver data. After @shutdown an operation may only reference @device data." I'd define it locally for now, unless / until other subsystems want to share the CXL device allocation scheme. [1]: https://lore.kernel.org/r/YOagA4bgdGYos5aa@kroah.com > The main example currently is with the > ioctl interface. Through the duration of the ioctl it's expected that > the underlying memdev will not disappear. It's not cxlmd that's the concern, it's cxlm. > Additionally, it may be desirable to have the fops be passed along as > well for drivers which do not want the standard handler for the > character device's ioctl. Let's give a concrete example. "The unit test code for CXL will also use a mock driver that provides its own devres_file_operations instance." I'll take a shot at reworking this. > > As memdev is being migrated to core, this separation must be made. > > Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> > --- > drivers/cxl/mem.h | 2 ++ > drivers/cxl/pci.c | 23 +++++++++++++++-------- > 2 files changed, 17 insertions(+), 8 deletions(-) > > diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h > index 2092f86beeb8..2b7481376621 100644 > --- a/drivers/cxl/mem.h > +++ b/drivers/cxl/mem.h > @@ -42,12 +42,14 @@ extern int cxl_mem_major; > * @cdev: char dev core object for ioctl operations > * @cxlm: pointer to the parent device driver data > * @id: id number of this memdev instance. > + * @shutdown: Optional function to call on memory device shutdown. > */ > struct cxl_memdev { > struct device dev; > struct cdev cdev; > struct cxl_mem *cxlm; > int id; > + void (*shutdown)(struct cxl_memdev *cxlmd); > }; > > /** > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c > index 6f10b19c9c83..418ae0eac188 100644 > --- a/drivers/cxl/pci.c > +++ b/drivers/cxl/pci.c > @@ -1293,11 +1293,13 @@ static void cxl_memdev_unregister(void *_cxlmd) > struct device *dev = &cxlmd->dev; > > cdev_device_del(&cxlmd->cdev, dev); > - cxl_memdev_shutdown(cxlmd); > + if (cxlmd->shutdown) > + cxlmd->shutdown(cxlmd); > put_device(dev); > } > > -static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm) > +static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm, > + const struct file_operations *fops) > { > struct pci_dev *pdev = cxlm->pdev; > struct cxl_memdev *cxlmd; > @@ -1323,7 +1325,7 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm) > device_set_pm_not_required(dev); > > cdev = &cxlmd->cdev; > - cdev_init(cdev, &cxl_memdev_fops); > + cdev_init(cdev, fops); > return cxlmd; > > err: > @@ -1331,15 +1333,17 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm) > return ERR_PTR(rc); > } > > -static struct cxl_memdev *devm_cxl_add_memdev(struct device *host, > - struct cxl_mem *cxlm) > +static struct cxl_memdev * > +devm_cxl_add_memdev(struct device *host, struct cxl_mem *cxlm, > + const struct file_operations *fops, > + void (*shutdown)(struct cxl_memdev *cxlmd)) > { > struct cxl_memdev *cxlmd; > struct device *dev; > struct cdev *cdev; > int rc; > > - cxlmd = cxl_memdev_alloc(cxlm); > + cxlmd = cxl_memdev_alloc(cxlm, fops); > if (IS_ERR(cxlmd)) > return cxlmd; > > @@ -1362,6 +1366,7 @@ static struct cxl_memdev *devm_cxl_add_memdev(struct device *host, > rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd); > if (rc) > return ERR_PTR(rc); > + cxlmd->shutdown = shutdown; > return cxlmd; > > err: > @@ -1369,7 +1374,8 @@ static struct cxl_memdev *devm_cxl_add_memdev(struct device *host, > * The cdev was briefly live, shutdown any ioctl operations that > * saw that state. > */ > - cxl_memdev_shutdown(cxlmd); > + if (shutdown) > + shutdown(cxlmd); > put_device(dev); > return ERR_PTR(rc); > } > @@ -1610,7 +1616,8 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) > if (rc) > return rc; > > - cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlm); > + cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlm, &cxl_memdev_fops, > + cxl_memdev_shutdown); > if (IS_ERR(cxlmd)) > return PTR_ERR(cxlmd); > > -- > 2.32.0 >
diff --git a/drivers/cxl/mem.h b/drivers/cxl/mem.h index 2092f86beeb8..2b7481376621 100644 --- a/drivers/cxl/mem.h +++ b/drivers/cxl/mem.h @@ -42,12 +42,14 @@ extern int cxl_mem_major; * @cdev: char dev core object for ioctl operations * @cxlm: pointer to the parent device driver data * @id: id number of this memdev instance. + * @shutdown: Optional function to call on memory device shutdown. */ struct cxl_memdev { struct device dev; struct cdev cdev; struct cxl_mem *cxlm; int id; + void (*shutdown)(struct cxl_memdev *cxlmd); }; /** diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c index 6f10b19c9c83..418ae0eac188 100644 --- a/drivers/cxl/pci.c +++ b/drivers/cxl/pci.c @@ -1293,11 +1293,13 @@ static void cxl_memdev_unregister(void *_cxlmd) struct device *dev = &cxlmd->dev; cdev_device_del(&cxlmd->cdev, dev); - cxl_memdev_shutdown(cxlmd); + if (cxlmd->shutdown) + cxlmd->shutdown(cxlmd); put_device(dev); } -static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm) +static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm, + const struct file_operations *fops) { struct pci_dev *pdev = cxlm->pdev; struct cxl_memdev *cxlmd; @@ -1323,7 +1325,7 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm) device_set_pm_not_required(dev); cdev = &cxlmd->cdev; - cdev_init(cdev, &cxl_memdev_fops); + cdev_init(cdev, fops); return cxlmd; err: @@ -1331,15 +1333,17 @@ static struct cxl_memdev *cxl_memdev_alloc(struct cxl_mem *cxlm) return ERR_PTR(rc); } -static struct cxl_memdev *devm_cxl_add_memdev(struct device *host, - struct cxl_mem *cxlm) +static struct cxl_memdev * +devm_cxl_add_memdev(struct device *host, struct cxl_mem *cxlm, + const struct file_operations *fops, + void (*shutdown)(struct cxl_memdev *cxlmd)) { struct cxl_memdev *cxlmd; struct device *dev; struct cdev *cdev; int rc; - cxlmd = cxl_memdev_alloc(cxlm); + cxlmd = cxl_memdev_alloc(cxlm, fops); if (IS_ERR(cxlmd)) return cxlmd; @@ -1362,6 +1366,7 @@ static struct cxl_memdev *devm_cxl_add_memdev(struct device *host, rc = devm_add_action_or_reset(host, cxl_memdev_unregister, cxlmd); if (rc) return ERR_PTR(rc); + cxlmd->shutdown = shutdown; return cxlmd; err: @@ -1369,7 +1374,8 @@ static struct cxl_memdev *devm_cxl_add_memdev(struct device *host, * The cdev was briefly live, shutdown any ioctl operations that * saw that state. */ - cxl_memdev_shutdown(cxlmd); + if (shutdown) + shutdown(cxlmd); put_device(dev); return ERR_PTR(rc); } @@ -1610,7 +1616,8 @@ static int cxl_mem_probe(struct pci_dev *pdev, const struct pci_device_id *id) if (rc) return rc; - cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlm); + cxlmd = devm_cxl_add_memdev(&pdev->dev, cxlm, &cxl_memdev_fops, + cxl_memdev_shutdown); if (IS_ERR(cxlmd)) return PTR_ERR(cxlmd);
Drivers that use cxl_core for registering a cxl_memdev will likely require synchronization with the core for shutdown so as to not race against the device going away. The main example currently is with the ioctl interface. Through the duration of the ioctl it's expected that the underlying memdev will not disappear. Additionally, it may be desirable to have the fops be passed along as well for drivers which do not want the standard handler for the character device's ioctl. As memdev is being migrated to core, this separation must be made. Signed-off-by: Ben Widawsky <ben.widawsky@intel.com> --- drivers/cxl/mem.h | 2 ++ drivers/cxl/pci.c | 23 +++++++++++++++-------- 2 files changed, 17 insertions(+), 8 deletions(-)