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

[Zhangfei Gao <zhangfei.gao@linaro.org>]

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