| Message ID | 20220624142122.30528-2-zhangfei.gao@linaro.org (mailing list archive) |
|---|---|
| State | Not Applicable |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | fix uacce concurrency issue of uacce_remove | expand |
On Fri, Jun 24, 2022 at 10:21:21PM +0800, Zhangfei Gao wrote: > Change cdev owner to parent driver owner, which blocks rmmod parent > driver module once fd is opened. > > Signed-off-by: Yang Shen <shenyang39@huawei.com> > Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> > --- > drivers/misc/uacce/uacce.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c > index 281c54003edc..f82f2dd30e76 100644 > --- a/drivers/misc/uacce/uacce.c > +++ b/drivers/misc/uacce/uacce.c > @@ -484,7 +484,7 @@ int uacce_register(struct uacce_device *uacce) > return -ENOMEM; > > uacce->cdev->ops = &uacce_fops; > - uacce->cdev->owner = THIS_MODULE; > + uacce->cdev->owner = uacce->parent->driver->owner; What if parent is not set? What if parent does not have a driver set to it yet? Why would a device's parent module control the lifespan of this child device's cdev? This feels wrong and like a layering violation here. If a parent's module is unloaded, then invalidate the cdev for the device when you tear it down before the module is unloaded. Yes, the interaction between the driver model and a cdev is messy, and always tricky (see the recent ksummit discussion about this again, and last year's discussion), but that does not mean you should add laying violations like this to the codebase. Please fix this properly. thanks, greg k-h
On 2022/6/27 下午9:21, Greg Kroah-Hartman wrote: > On Fri, Jun 24, 2022 at 10:21:21PM +0800, Zhangfei Gao wrote: >> Change cdev owner to parent driver owner, which blocks rmmod parent >> driver module once fd is opened. >> >> Signed-off-by: Yang Shen <shenyang39@huawei.com> >> Signed-off-by: Zhangfei Gao <zhangfei.gao@linaro.org> >> --- >> drivers/misc/uacce/uacce.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) >> >> diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c >> index 281c54003edc..f82f2dd30e76 100644 >> --- a/drivers/misc/uacce/uacce.c >> +++ b/drivers/misc/uacce/uacce.c >> @@ -484,7 +484,7 @@ int uacce_register(struct uacce_device *uacce) >> return -ENOMEM; >> >> uacce->cdev->ops = &uacce_fops; >> - uacce->cdev->owner = THIS_MODULE; >> + uacce->cdev->owner = uacce->parent->driver->owner; > What if parent is not set? What if parent does not have a driver set to > it yet? Why would a device's parent module control the lifespan of this > child device's cdev? Have used try_module_get(uacce->parent->driver->owner) in open, and module_put in release. Seems same issue. > > This feels wrong and like a layering violation here. > > If a parent's module is unloaded, then invalidate the cdev for the > device when you tear it down before the module is unloaded. Yes, make sense. > > Yes, the interaction between the driver model and a cdev is messy, and > always tricky (see the recent ksummit discussion about this again, and > last year's discussion), but that does not mean you should add laying > violations like this to the codebase. Please fix this properly. Thanks Greg Yes, I was in hesitation whether adding the patch 1, but it looks very simple. In fact, the patch 2 can cover both removing device and rmmod parent driver module. We can just keep patch 2. Thanks
diff --git a/drivers/misc/uacce/uacce.c b/drivers/misc/uacce/uacce.c index 281c54003edc..f82f2dd30e76 100644 --- a/drivers/misc/uacce/uacce.c +++ b/drivers/misc/uacce/uacce.c @@ -484,7 +484,7 @@ int uacce_register(struct uacce_device *uacce) return -ENOMEM; uacce->cdev->ops = &uacce_fops; - uacce->cdev->owner = THIS_MODULE; + uacce->cdev->owner = uacce->parent->driver->owner; return cdev_device_add(uacce->cdev, &uacce->dev); }