Re: [PATCH v2 1/2] uacce: Handle parent driver module removal

[Zhangfei Gao]

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@xxxxxxxxxx>
> > Signed-off-by: Zhangfei Gao <zhangfei.gao@xxxxxxxxxx>
> > ---
> >   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