Re: [PATCH 2/8] vfio/mdev: embedd struct mdev_parent in the parent data structure

[Christoph Hellwig <hch@lst.de>]

On Tue, Jun 07, 2022 at 12:52:30AM +0530, Kirti Wankhede wrote:
> By removing this list, there is no way to know if parent device is 
> registered repeatedly? What will happen if same parent device is registered 
> twice? will it fail somewhere else?

The driver core will warn if you double register a device.

>>   probe'd to then it should call::
>>   -	extern int  mdev_register_device(struct device *dev,
>> -	                                 struct mdev_driver *mdev_driver);
>> +	int mdev_register_parent(struct mdev_parent *parent, struct device *dev,
>> +			struct mdev_driver *mdev_driver);
>>
>
> No need to change API name as it still registers 'struct device *dev', it 
> could be 'mdev_register_device' but with new argument list.

I think the name name is a lot more clear, as device is really overused.
Especially as this is not a mdev_device, which are registered
elsewhere..

>>   -	mdev_unregister_device(i915->drm.dev);
>> +	mdev_unregister_parent(&i915->vgpu.parent);
>
> Ideally, parent should be member of gvt. There could be multiple vgpus 
> created on one physical device. Intel folks would be better reviewer 
> though.

i915->vgpu is not for a single VGPU, but all VGPU related core
support.

>> -	struct mdev_parent *parent;
>>   	char *env_string = "MDEV_STATE=registered";
>>   	char *envp[] = { env_string, NULL };
>> +	int ret;
>>     	/* check for mandatory ops */
>>   	if (!mdev_driver->supported_type_groups)
>>   		return -EINVAL;
>>   -	dev = get_device(dev);
>> -	if (!dev)
>> -		return -EINVAL;
>> -
>
> Why not held device here? What if some driver miss behave where it 
> registers device to mdev, but unloads without unregistering from mdev?

Then that driver is buggy.  We don't add extra reference to slightly
paper over buggy code elsewhere in the kernel either.