[PATCH v2] mdev: Send uevents around parent device registration

[PATCH v2] mdev: Send uevents around parent device registration

[Alex Williamson]

This allows udev to trigger rules when a parent device is registered
or unregistered from mdev.

Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
---

v2: Don't remove the dev_info(), Kirti requested they stay and
    removing them is only tangential to the goal of this change.

 drivers/vfio/mdev/mdev_core.c |    8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index ae23151442cb..7fb268136c62 100644
--- a/drivers/vfio/mdev/mdev_core.c
+++ b/drivers/vfio/mdev/mdev_core.c
@@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
 {
 	int ret;
 	struct mdev_parent *parent;
+	char *env_string = "MDEV_STATE=registered";
+	char *envp[] = { env_string, NULL };
 
 	/* check for mandatory ops */
 	if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
@@ -197,6 +199,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
 	mutex_unlock(&parent_list_lock);
 
 	dev_info(dev, "MDEV: Registered\n");
+	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
+
 	return 0;
 
 add_dev_err:
@@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
 void mdev_unregister_device(struct device *dev)
 {
 	struct mdev_parent *parent;
+	char *env_string = "MDEV_STATE=unregistered";
+	char *envp[] = { env_string, NULL };
 
 	mutex_lock(&parent_list_lock);
 	parent = __find_parent_device(dev);
@@ -243,6 +249,8 @@ void mdev_unregister_device(struct device *dev)
 	up_write(&parent->unreg_sem);
 
 	mdev_put_parent(parent);
+
+	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
 }
 EXPORT_SYMBOL(mdev_unregister_device);
 

Re: [PATCH v2] mdev: Send uevents around parent device registration

[Cornelia Huck]

On Mon, 01 Jul 2019 08:54:44 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> This allows udev to trigger rules when a parent device is registered
> or unregistered from mdev.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> v2: Don't remove the dev_info(), Kirti requested they stay and
>     removing them is only tangential to the goal of this change.
> 
>  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
>  1 file changed, 8 insertions(+)

Not that fond of the dev_info(), but this still looks sane.

Reviewed-by: Cornelia Huck <cohuck@redhat.com>

Re: [PATCH v2] mdev: Send uevents around parent device registration

[Kirti Wankhede]

On 7/1/2019 8:24 PM, Alex Williamson wrote:
> This allows udev to trigger rules when a parent device is registered
> or unregistered from mdev.
> 
> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> ---
> 
> v2: Don't remove the dev_info(), Kirti requested they stay and
>     removing them is only tangential to the goal of this change.
> 

Thanks.


>  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index ae23151442cb..7fb268136c62 100644
> --- a/drivers/vfio/mdev/mdev_core.c
> +++ b/drivers/vfio/mdev/mdev_core.c
> @@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
>  {
>  	int ret;
>  	struct mdev_parent *parent;
> +	char *env_string = "MDEV_STATE=registered";
> +	char *envp[] = { env_string, NULL };
>  
>  	/* check for mandatory ops */
>  	if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
> @@ -197,6 +199,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
>  	mutex_unlock(&parent_list_lock);
>  
>  	dev_info(dev, "MDEV: Registered\n");
> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> +
>  	return 0;
>  
>  add_dev_err:
> @@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
>  void mdev_unregister_device(struct device *dev)
>  {
>  	struct mdev_parent *parent;
> +	char *env_string = "MDEV_STATE=unregistered";
> +	char *envp[] = { env_string, NULL };
>  
>  	mutex_lock(&parent_list_lock);
>  	parent = __find_parent_device(dev);
> @@ -243,6 +249,8 @@ void mdev_unregister_device(struct device *dev)
>  	up_write(&parent->unreg_sem);
>  
>  	mdev_put_parent(parent);
> +
> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);

mdev_put_parent() calls put_device(dev). If this is the last instance
holding device, then on put_device(dev) dev would get freed.

This event should be before mdev_put_parent()

Thanks,
Kirti

>  }
>  EXPORT_SYMBOL(mdev_unregister_device);
>  
> 

Re: [PATCH v2] mdev: Send uevents around parent device registration

[Alex Williamson]

On Mon, 1 Jul 2019 22:43:10 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 7/1/2019 8:24 PM, Alex Williamson wrote:
> > This allows udev to trigger rules when a parent device is registered
> > or unregistered from mdev.
> > 
> > Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > ---
> > 
> > v2: Don't remove the dev_info(), Kirti requested they stay and
> >     removing them is only tangential to the goal of this change.
> >   
> 
> Thanks.
> 
> 
> >  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> > index ae23151442cb..7fb268136c62 100644
> > --- a/drivers/vfio/mdev/mdev_core.c
> > +++ b/drivers/vfio/mdev/mdev_core.c
> > @@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
> >  {
> >  	int ret;
> >  	struct mdev_parent *parent;
> > +	char *env_string = "MDEV_STATE=registered";
> > +	char *envp[] = { env_string, NULL };
> >  
> >  	/* check for mandatory ops */
> >  	if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
> > @@ -197,6 +199,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
> >  	mutex_unlock(&parent_list_lock);
> >  
> >  	dev_info(dev, "MDEV: Registered\n");
> > +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> > +
> >  	return 0;
> >  
> >  add_dev_err:
> > @@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
> >  void mdev_unregister_device(struct device *dev)
> >  {
> >  	struct mdev_parent *parent;
> > +	char *env_string = "MDEV_STATE=unregistered";
> > +	char *envp[] = { env_string, NULL };
> >  
> >  	mutex_lock(&parent_list_lock);
> >  	parent = __find_parent_device(dev);
> > @@ -243,6 +249,8 @@ void mdev_unregister_device(struct device *dev)
> >  	up_write(&parent->unreg_sem);
> >  
> >  	mdev_put_parent(parent);
> > +
> > +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);  
> 
> mdev_put_parent() calls put_device(dev). If this is the last instance
> holding device, then on put_device(dev) dev would get freed.
> 
> This event should be before mdev_put_parent()

So you're suggesting the vendor driver is calling
mdev_unregister_device() without a reference to the struct device that
it's passing to unregister?  Sounds bogus to me.  We take a
reference to the device so that it can't disappear out from under us,
the caller cannot rely on our reference and the caller provided the
struct device.  Thanks,

Alex

Re: [PATCH v2] mdev: Send uevents around parent device registration

[Kirti Wankhede]

On 7/1/2019 10:54 PM, Alex Williamson wrote:
> On Mon, 1 Jul 2019 22:43:10 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 7/1/2019 8:24 PM, Alex Williamson wrote:
>>> This allows udev to trigger rules when a parent device is registered
>>> or unregistered from mdev.
>>>
>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>> ---
>>>
>>> v2: Don't remove the dev_info(), Kirti requested they stay and
>>>     removing them is only tangential to the goal of this change.
>>>   
>>
>> Thanks.
>>
>>
>>>  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
>>>  1 file changed, 8 insertions(+)
>>>
>>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
>>> index ae23151442cb..7fb268136c62 100644
>>> --- a/drivers/vfio/mdev/mdev_core.c
>>> +++ b/drivers/vfio/mdev/mdev_core.c
>>> @@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
>>>  {
>>>  	int ret;
>>>  	struct mdev_parent *parent;
>>> +	char *env_string = "MDEV_STATE=registered";
>>> +	char *envp[] = { env_string, NULL };
>>>  
>>>  	/* check for mandatory ops */
>>>  	if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
>>> @@ -197,6 +199,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
>>>  	mutex_unlock(&parent_list_lock);
>>>  
>>>  	dev_info(dev, "MDEV: Registered\n");
>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
>>> +
>>>  	return 0;
>>>  
>>>  add_dev_err:
>>> @@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
>>>  void mdev_unregister_device(struct device *dev)
>>>  {
>>>  	struct mdev_parent *parent;
>>> +	char *env_string = "MDEV_STATE=unregistered";
>>> +	char *envp[] = { env_string, NULL };
>>>  
>>>  	mutex_lock(&parent_list_lock);
>>>  	parent = __find_parent_device(dev);
>>> @@ -243,6 +249,8 @@ void mdev_unregister_device(struct device *dev)
>>>  	up_write(&parent->unreg_sem);
>>>  
>>>  	mdev_put_parent(parent);
>>> +
>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);  
>>
>> mdev_put_parent() calls put_device(dev). If this is the last instance
>> holding device, then on put_device(dev) dev would get freed.
>>
>> This event should be before mdev_put_parent()
> 
> So you're suggesting the vendor driver is calling
> mdev_unregister_device() without a reference to the struct device that
> it's passing to unregister?  Sounds bogus to me.  We take a
> reference to the device so that it can't disappear out from under us,
> the caller cannot rely on our reference and the caller provided the
> struct device.  Thanks,
> 

1. Register uevent is sent after mdev holding reference to device, then
ideally, unregister path should be mirror of register path, send uevent
and then release the reference to device.

2. I agree that vendor driver shouldn't call mdev_unregister_device()
without holding reference to device. But to be on safer side, if ever
such case occur, to avoid any segmentation fault in kernel, better to
send event before mdev release the reference to device.

Thanks,
Kirti

Re: [PATCH v2] mdev: Send uevents around parent device registration

[Alex Williamson]

On Mon, 1 Jul 2019 23:20:35 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 7/1/2019 10:54 PM, Alex Williamson wrote:
> > On Mon, 1 Jul 2019 22:43:10 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >   
> >> On 7/1/2019 8:24 PM, Alex Williamson wrote:  
> >>> This allows udev to trigger rules when a parent device is registered
> >>> or unregistered from mdev.
> >>>
> >>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >>> ---
> >>>
> >>> v2: Don't remove the dev_info(), Kirti requested they stay and
> >>>     removing them is only tangential to the goal of this change.
> >>>     
> >>
> >> Thanks.
> >>
> >>  
> >>>  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
> >>>  1 file changed, 8 insertions(+)
> >>>
> >>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> >>> index ae23151442cb..7fb268136c62 100644
> >>> --- a/drivers/vfio/mdev/mdev_core.c
> >>> +++ b/drivers/vfio/mdev/mdev_core.c
> >>> @@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
> >>>  {
> >>>  	int ret;
> >>>  	struct mdev_parent *parent;
> >>> +	char *env_string = "MDEV_STATE=registered";
> >>> +	char *envp[] = { env_string, NULL };
> >>>  
> >>>  	/* check for mandatory ops */
> >>>  	if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
> >>> @@ -197,6 +199,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
> >>>  	mutex_unlock(&parent_list_lock);
> >>>  
> >>>  	dev_info(dev, "MDEV: Registered\n");
> >>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> >>> +
> >>>  	return 0;
> >>>  
> >>>  add_dev_err:
> >>> @@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
> >>>  void mdev_unregister_device(struct device *dev)
> >>>  {
> >>>  	struct mdev_parent *parent;
> >>> +	char *env_string = "MDEV_STATE=unregistered";
> >>> +	char *envp[] = { env_string, NULL };
> >>>  
> >>>  	mutex_lock(&parent_list_lock);
> >>>  	parent = __find_parent_device(dev);
> >>> @@ -243,6 +249,8 @@ void mdev_unregister_device(struct device *dev)
> >>>  	up_write(&parent->unreg_sem);
> >>>  
> >>>  	mdev_put_parent(parent);
> >>> +
> >>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);    
> >>
> >> mdev_put_parent() calls put_device(dev). If this is the last instance
> >> holding device, then on put_device(dev) dev would get freed.
> >>
> >> This event should be before mdev_put_parent()  
> > 
> > So you're suggesting the vendor driver is calling
> > mdev_unregister_device() without a reference to the struct device that
> > it's passing to unregister?  Sounds bogus to me.  We take a
> > reference to the device so that it can't disappear out from under us,
> > the caller cannot rely on our reference and the caller provided the
> > struct device.  Thanks,
> >   
> 
> 1. Register uevent is sent after mdev holding reference to device, then
> ideally, unregister path should be mirror of register path, send uevent
> and then release the reference to device.

I don't see the relevance here.  We're marking an event, not unwinding
state of the device from the registration process.  Additionally, the
event we're trying to mark is the completion of each process, so the
notion that we need to mirror the ordering between the two is invalid.

> 2. I agree that vendor driver shouldn't call mdev_unregister_device()
> without holding reference to device. But to be on safer side, if ever
> such case occur, to avoid any segmentation fault in kernel, better to
> send event before mdev release the reference to device.

I know that get_device() and put_device() are GPL symbols and that's a
bit of an issue, but I don't think we should be kludging the code for a
vendor driver that might have problems with that.  A) we're using the
caller provided device  for the uevent, B) we're only releasing our own
reference to the device that was acquired during registration, the
vendor driver must have other references, C) the parent device
generally lives on a bus, with a vendor driver, there's an entire
ecosystem of references to the device below mdev.  Is this a paranoia
request or are you really concerned that your PCI device suddenly
disappears when mdev's reference to it disappears.  Let's flush those
bugs out if they exist, not mask them behind obscure ordering
dependencies.  Thanks,

Alex

Re: [PATCH v2] mdev: Send uevents around parent device registration

[Kirti Wankhede]

On 7/2/2019 1:34 AM, Alex Williamson wrote:
> On Mon, 1 Jul 2019 23:20:35 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 7/1/2019 10:54 PM, Alex Williamson wrote:
>>> On Mon, 1 Jul 2019 22:43:10 +0530
>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>   
>>>> On 7/1/2019 8:24 PM, Alex Williamson wrote:  
>>>>> This allows udev to trigger rules when a parent device is registered
>>>>> or unregistered from mdev.
>>>>>
>>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>>>> ---
>>>>>
>>>>> v2: Don't remove the dev_info(), Kirti requested they stay and
>>>>>     removing them is only tangential to the goal of this change.
>>>>>     
>>>>
>>>> Thanks.
>>>>
>>>>  
>>>>>  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
>>>>>  1 file changed, 8 insertions(+)
>>>>>
>>>>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
>>>>> index ae23151442cb..7fb268136c62 100644
>>>>> --- a/drivers/vfio/mdev/mdev_core.c
>>>>> +++ b/drivers/vfio/mdev/mdev_core.c
>>>>> @@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
>>>>>  {
>>>>>  	int ret;
>>>>>  	struct mdev_parent *parent;
>>>>> +	char *env_string = "MDEV_STATE=registered";
>>>>> +	char *envp[] = { env_string, NULL };
>>>>>  
>>>>>  	/* check for mandatory ops */
>>>>>  	if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
>>>>> @@ -197,6 +199,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
>>>>>  	mutex_unlock(&parent_list_lock);
>>>>>  
>>>>>  	dev_info(dev, "MDEV: Registered\n");
>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
>>>>> +
>>>>>  	return 0;
>>>>>  
>>>>>  add_dev_err:
>>>>> @@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
>>>>>  void mdev_unregister_device(struct device *dev)
>>>>>  {
>>>>>  	struct mdev_parent *parent;
>>>>> +	char *env_string = "MDEV_STATE=unregistered";
>>>>> +	char *envp[] = { env_string, NULL };
>>>>>  
>>>>>  	mutex_lock(&parent_list_lock);
>>>>>  	parent = __find_parent_device(dev);
>>>>> @@ -243,6 +249,8 @@ void mdev_unregister_device(struct device *dev)
>>>>>  	up_write(&parent->unreg_sem);
>>>>>  
>>>>>  	mdev_put_parent(parent);
>>>>> +
>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);    
>>>>
>>>> mdev_put_parent() calls put_device(dev). If this is the last instance
>>>> holding device, then on put_device(dev) dev would get freed.
>>>>
>>>> This event should be before mdev_put_parent()  
>>>
>>> So you're suggesting the vendor driver is calling
>>> mdev_unregister_device() without a reference to the struct device that
>>> it's passing to unregister?  Sounds bogus to me.  We take a
>>> reference to the device so that it can't disappear out from under us,
>>> the caller cannot rely on our reference and the caller provided the
>>> struct device.  Thanks,
>>>   
>>
>> 1. Register uevent is sent after mdev holding reference to device, then
>> ideally, unregister path should be mirror of register path, send uevent
>> and then release the reference to device.
> 
> I don't see the relevance here.  We're marking an event, not unwinding
> state of the device from the registration process.  Additionally, the
> event we're trying to mark is the completion of each process, so the
> notion that we need to mirror the ordering between the two is invalid.
> 
>> 2. I agree that vendor driver shouldn't call mdev_unregister_device()
>> without holding reference to device. But to be on safer side, if ever
>> such case occur, to avoid any segmentation fault in kernel, better to
>> send event before mdev release the reference to device.
> 
> I know that get_device() and put_device() are GPL symbols and that's a
> bit of an issue, but I don't think we should be kludging the code for a
> vendor driver that might have problems with that.  A) we're using the
> caller provided device  for the uevent, B) we're only releasing our own
> reference to the device that was acquired during registration, the
> vendor driver must have other references,

Are you going to assume that someone/vendor driver is always going to do
right thing?

> C) the parent device
> generally lives on a bus, with a vendor driver, there's an entire
> ecosystem of references to the device below mdev.  Is this a paranoia
> request or are you really concerned that your PCI device suddenly
> disappears when mdev's reference to it disappears. 

mdev infrastructure is not always used by PCI devices. It is designed to
be generic, so that other devices (other than PCI devices) can also use
this framework.
If there is a assumption that user of mdev framework or vendor drivers
are always going to use mdev in right way, then there is no need for
mdev core to held reference of the device?
This is not a "paranoia request". This is more of a ideal scenario, mdev
should use device by holding its reference rather than assuming (or
relying on) someone else holding the reference of device.

Thanks,
Kirti

Re: [PATCH v2] mdev: Send uevents around parent device registration

[Alex Williamson]

On Tue, 2 Jul 2019 10:25:04 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 7/2/2019 1:34 AM, Alex Williamson wrote:
> > On Mon, 1 Jul 2019 23:20:35 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >   
> >> On 7/1/2019 10:54 PM, Alex Williamson wrote:  
> >>> On Mon, 1 Jul 2019 22:43:10 +0530
> >>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>     
> >>>> On 7/1/2019 8:24 PM, Alex Williamson wrote:    
> >>>>> This allows udev to trigger rules when a parent device is registered
> >>>>> or unregistered from mdev.
> >>>>>
> >>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >>>>> ---
> >>>>>
> >>>>> v2: Don't remove the dev_info(), Kirti requested they stay and
> >>>>>     removing them is only tangential to the goal of this change.
> >>>>>       
> >>>>
> >>>> Thanks.
> >>>>
> >>>>    
> >>>>>  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
> >>>>>  1 file changed, 8 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> >>>>> index ae23151442cb..7fb268136c62 100644
> >>>>> --- a/drivers/vfio/mdev/mdev_core.c
> >>>>> +++ b/drivers/vfio/mdev/mdev_core.c
> >>>>> @@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
> >>>>>  {
> >>>>>  	int ret;
> >>>>>  	struct mdev_parent *parent;
> >>>>> +	char *env_string = "MDEV_STATE=registered";
> >>>>> +	char *envp[] = { env_string, NULL };
> >>>>>  
> >>>>>  	/* check for mandatory ops */
> >>>>>  	if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
> >>>>> @@ -197,6 +199,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
> >>>>>  	mutex_unlock(&parent_list_lock);
> >>>>>  
> >>>>>  	dev_info(dev, "MDEV: Registered\n");
> >>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> >>>>> +
> >>>>>  	return 0;
> >>>>>  
> >>>>>  add_dev_err:
> >>>>> @@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
> >>>>>  void mdev_unregister_device(struct device *dev)
> >>>>>  {
> >>>>>  	struct mdev_parent *parent;
> >>>>> +	char *env_string = "MDEV_STATE=unregistered";
> >>>>> +	char *envp[] = { env_string, NULL };
> >>>>>  
> >>>>>  	mutex_lock(&parent_list_lock);
> >>>>>  	parent = __find_parent_device(dev);
> >>>>> @@ -243,6 +249,8 @@ void mdev_unregister_device(struct device *dev)
> >>>>>  	up_write(&parent->unreg_sem);
> >>>>>  
> >>>>>  	mdev_put_parent(parent);
> >>>>> +
> >>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);      
> >>>>
> >>>> mdev_put_parent() calls put_device(dev). If this is the last instance
> >>>> holding device, then on put_device(dev) dev would get freed.
> >>>>
> >>>> This event should be before mdev_put_parent()    
> >>>
> >>> So you're suggesting the vendor driver is calling
> >>> mdev_unregister_device() without a reference to the struct device that
> >>> it's passing to unregister?  Sounds bogus to me.  We take a
> >>> reference to the device so that it can't disappear out from under us,
> >>> the caller cannot rely on our reference and the caller provided the
> >>> struct device.  Thanks,
> >>>     
> >>
> >> 1. Register uevent is sent after mdev holding reference to device, then
> >> ideally, unregister path should be mirror of register path, send uevent
> >> and then release the reference to device.  
> > 
> > I don't see the relevance here.  We're marking an event, not unwinding
> > state of the device from the registration process.  Additionally, the
> > event we're trying to mark is the completion of each process, so the
> > notion that we need to mirror the ordering between the two is invalid.
> >   
> >> 2. I agree that vendor driver shouldn't call mdev_unregister_device()
> >> without holding reference to device. But to be on safer side, if ever
> >> such case occur, to avoid any segmentation fault in kernel, better to
> >> send event before mdev release the reference to device.  
> > 
> > I know that get_device() and put_device() are GPL symbols and that's a
> > bit of an issue, but I don't think we should be kludging the code for a
> > vendor driver that might have problems with that.  A) we're using the
> > caller provided device  for the uevent, B) we're only releasing our own
> > reference to the device that was acquired during registration, the
> > vendor driver must have other references,  
> 
> Are you going to assume that someone/vendor driver is always going to do
> right thing?

mdev is a kernel driver, we make reasonable assumptions that other
drivers interact with it correctly.

> > C) the parent device
> > generally lives on a bus, with a vendor driver, there's an entire
> > ecosystem of references to the device below mdev.  Is this a paranoia
> > request or are you really concerned that your PCI device suddenly
> > disappears when mdev's reference to it disappears.   
> 
> mdev infrastructure is not always used by PCI devices. It is designed to
> be generic, so that other devices (other than PCI devices) can also use
> this framework.

Obviously mdev is not PCI specific, I only mention it because I'm
asking if you have a specific concern in mind.  If you did, I'd assume
it's related to a PCI backed vGPU.  Any physical parent device of an
mdev is likely to have some sort of bus infrastructure behind it
holding references to the device (ie. a probe and release where an
implicit reference is held between these points).  A virtual device
would be similar, it's created as part of a module init and destroyed
as part of a module exit, where mdev registration would exist between
these points.

> If there is a assumption that user of mdev framework or vendor drivers
> are always going to use mdev in right way, then there is no need for
> mdev core to held reference of the device?
> This is not a "paranoia request". This is more of a ideal scenario, mdev
> should use device by holding its reference rather than assuming (or
> relying on) someone else holding the reference of device.

In fact, at one point Parav was proposing removing these references
entirely, but Connie and I both felt uncomfortable about that.  I think
it's good practice that mdev indicates the use of the parent device by
incrementing the reference count, with each child mdev device also
taking a reference, but those references balance out within the mdev
core.  Their purpose is not to maintain the device for outside callers,
nor should outside callers assume mdev's use of references to release
their own.  I don't think it's unreasonable to assume that the caller
should have a legitimate reference to the object it's providing to this
function and therefore we should be able to use it after mdev's
internal references are balanced out.  Thanks,

Alex

Re: [PATCH v2] mdev: Send uevents around parent device registration

[Cornelia Huck]

On Tue, 2 Jul 2019 10:25:04 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 7/2/2019 1:34 AM, Alex Williamson wrote:
> > On Mon, 1 Jul 2019 23:20:35 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >   
> >> On 7/1/2019 10:54 PM, Alex Williamson wrote:  
> >>> On Mon, 1 Jul 2019 22:43:10 +0530
> >>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>     
> >>>> On 7/1/2019 8:24 PM, Alex Williamson wrote:    
> >>>>> This allows udev to trigger rules when a parent device is registered
> >>>>> or unregistered from mdev.
> >>>>>
> >>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >>>>> ---
> >>>>>
> >>>>> v2: Don't remove the dev_info(), Kirti requested they stay and
> >>>>>     removing them is only tangential to the goal of this change.
> >>>>>       
> >>>>
> >>>> Thanks.
> >>>>
> >>>>    
> >>>>>  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
> >>>>>  1 file changed, 8 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> >>>>> index ae23151442cb..7fb268136c62 100644
> >>>>> --- a/drivers/vfio/mdev/mdev_core.c
> >>>>> +++ b/drivers/vfio/mdev/mdev_core.c
> >>>>> @@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
> >>>>>  {
> >>>>>  	int ret;
> >>>>>  	struct mdev_parent *parent;
> >>>>> +	char *env_string = "MDEV_STATE=registered";
> >>>>> +	char *envp[] = { env_string, NULL };
> >>>>>  
> >>>>>  	/* check for mandatory ops */
> >>>>>  	if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
> >>>>> @@ -197,6 +199,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
> >>>>>  	mutex_unlock(&parent_list_lock);
> >>>>>  
> >>>>>  	dev_info(dev, "MDEV: Registered\n");
> >>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> >>>>> +
> >>>>>  	return 0;
> >>>>>  
> >>>>>  add_dev_err:
> >>>>> @@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
> >>>>>  void mdev_unregister_device(struct device *dev)
> >>>>>  {
> >>>>>  	struct mdev_parent *parent;
> >>>>> +	char *env_string = "MDEV_STATE=unregistered";
> >>>>> +	char *envp[] = { env_string, NULL };
> >>>>>  
> >>>>>  	mutex_lock(&parent_list_lock);
> >>>>>  	parent = __find_parent_device(dev);
> >>>>> @@ -243,6 +249,8 @@ void mdev_unregister_device(struct device *dev)
> >>>>>  	up_write(&parent->unreg_sem);
> >>>>>  
> >>>>>  	mdev_put_parent(parent);
> >>>>> +
> >>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);      
> >>>>
> >>>> mdev_put_parent() calls put_device(dev). If this is the last instance
> >>>> holding device, then on put_device(dev) dev would get freed.
> >>>>
> >>>> This event should be before mdev_put_parent()    
> >>>
> >>> So you're suggesting the vendor driver is calling
> >>> mdev_unregister_device() without a reference to the struct device that
> >>> it's passing to unregister?  Sounds bogus to me.  We take a
> >>> reference to the device so that it can't disappear out from under us,
> >>> the caller cannot rely on our reference and the caller provided the
> >>> struct device.  Thanks,
> >>>     
> >>
> >> 1. Register uevent is sent after mdev holding reference to device, then
> >> ideally, unregister path should be mirror of register path, send uevent
> >> and then release the reference to device.  
> > 
> > I don't see the relevance here.  We're marking an event, not unwinding
> > state of the device from the registration process.  Additionally, the
> > event we're trying to mark is the completion of each process, so the
> > notion that we need to mirror the ordering between the two is invalid.
> >   
> >> 2. I agree that vendor driver shouldn't call mdev_unregister_device()
> >> without holding reference to device. But to be on safer side, if ever
> >> such case occur, to avoid any segmentation fault in kernel, better to
> >> send event before mdev release the reference to device.  
> > 
> > I know that get_device() and put_device() are GPL symbols and that's a
> > bit of an issue, but I don't think we should be kludging the code for a
> > vendor driver that might have problems with that.  A) we're using the
> > caller provided device  for the uevent, B) we're only releasing our own
> > reference to the device that was acquired during registration, the
> > vendor driver must have other references,  
> 
> Are you going to assume that someone/vendor driver is always going to do
> right thing?
> 
> > C) the parent device
> > generally lives on a bus, with a vendor driver, there's an entire
> > ecosystem of references to the device below mdev.  Is this a paranoia
> > request or are you really concerned that your PCI device suddenly
> > disappears when mdev's reference to it disappears.   
> 
> mdev infrastructure is not always used by PCI devices. It is designed to
> be generic, so that other devices (other than PCI devices) can also use
> this framework.

But the same argument holds there: There's a whole ecosystem of
references for other devices as well.

> If there is a assumption that user of mdev framework or vendor drivers
> are always going to use mdev in right way, then there is no need for
> mdev core to held reference of the device?

Confused. How does this follow for the general case?

> This is not a "paranoia request". This is more of a ideal scenario, mdev
> should use device by holding its reference rather than assuming (or
> relying on) someone else holding the reference of device.

I'm not really opposed to switching this around, although it's probably
not needed.

RE: [PATCH v2] mdev: Send uevents around parent device registration

[Parav Pandit]

> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org <linux-kernel-
> owner@vger.kernel.org> On Behalf Of Alex Williamson
> Sent: Tuesday, July 2, 2019 11:12 AM
> To: Kirti Wankhede <kwankhede@nvidia.com>
> Cc: cohuck@redhat.com; kvm@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2] mdev: Send uevents around parent device registration
> 
> On Tue, 2 Jul 2019 10:25:04 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
> > On 7/2/2019 1:34 AM, Alex Williamson wrote:
> > > On Mon, 1 Jul 2019 23:20:35 +0530
> > > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> > >
> > >> On 7/1/2019 10:54 PM, Alex Williamson wrote:
> > >>> On Mon, 1 Jul 2019 22:43:10 +0530
> > >>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> > >>>
> > >>>> On 7/1/2019 8:24 PM, Alex Williamson wrote:
> > >>>>> This allows udev to trigger rules when a parent device is
> > >>>>> registered or unregistered from mdev.
> > >>>>>
> > >>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> > >>>>> ---
> > >>>>>
> > >>>>> v2: Don't remove the dev_info(), Kirti requested they stay and
> > >>>>>     removing them is only tangential to the goal of this change.
> > >>>>>
> > >>>>
> > >>>> Thanks.
> > >>>>
> > >>>>
> > >>>>>  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
> > >>>>>  1 file changed, 8 insertions(+)
> > >>>>>
> > >>>>> diff --git a/drivers/vfio/mdev/mdev_core.c
> > >>>>> b/drivers/vfio/mdev/mdev_core.c index ae23151442cb..7fb268136c62
> > >>>>> 100644
> > >>>>> --- a/drivers/vfio/mdev/mdev_core.c
> > >>>>> +++ b/drivers/vfio/mdev/mdev_core.c
> > >>>>> @@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev,
> > >>>>> const struct mdev_parent_ops *ops)  {
> > >>>>>  	int ret;
> > >>>>>  	struct mdev_parent *parent;
> > >>>>> +	char *env_string = "MDEV_STATE=registered";
> > >>>>> +	char *envp[] = { env_string, NULL };
> > >>>>>
> > >>>>>  	/* check for mandatory ops */
> > >>>>>  	if (!ops || !ops->create || !ops->remove ||
> > >>>>> !ops->supported_type_groups) @@ -197,6 +199,8 @@ int
> mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
> > >>>>>  	mutex_unlock(&parent_list_lock);
> > >>>>>
> > >>>>>  	dev_info(dev, "MDEV: Registered\n");
> > >>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> > >>>>> +
> > >>>>>  	return 0;
> > >>>>>
> > >>>>>  add_dev_err:
> > >>>>> @@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
> > >>>>>  void mdev_unregister_device(struct device *dev)  {
> > >>>>>  	struct mdev_parent *parent;
> > >>>>> +	char *env_string = "MDEV_STATE=unregistered";
> > >>>>> +	char *envp[] = { env_string, NULL };
> > >>>>>
> > >>>>>  	mutex_lock(&parent_list_lock);
> > >>>>>  	parent = __find_parent_device(dev); @@ -243,6 +249,8 @@
> void
> > >>>>> mdev_unregister_device(struct device *dev)
> > >>>>>  	up_write(&parent->unreg_sem);
> > >>>>>
> > >>>>>  	mdev_put_parent(parent);
> > >>>>> +
> > >>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> > >>>>
> > >>>> mdev_put_parent() calls put_device(dev). If this is the last
> > >>>> instance holding device, then on put_device(dev) dev would get freed.
> > >>>>
> > >>>> This event should be before mdev_put_parent()
> > >>>
> > >>> So you're suggesting the vendor driver is calling
> > >>> mdev_unregister_device() without a reference to the struct device
> > >>> that it's passing to unregister?  Sounds bogus to me.  We take a
> > >>> reference to the device so that it can't disappear out from under
> > >>> us, the caller cannot rely on our reference and the caller
> > >>> provided the struct device.  Thanks,
> > >>>
> > >>
> > >> 1. Register uevent is sent after mdev holding reference to device,
> > >> then ideally, unregister path should be mirror of register path,
> > >> send uevent and then release the reference to device.
> > >
> > > I don't see the relevance here.  We're marking an event, not
> > > unwinding state of the device from the registration process.
> > > Additionally, the event we're trying to mark is the completion of
> > > each process, so the notion that we need to mirror the ordering between
> the two is invalid.
> > >
> > >> 2. I agree that vendor driver shouldn't call
> > >> mdev_unregister_device() without holding reference to device. But
> > >> to be on safer side, if ever such case occur, to avoid any
> > >> segmentation fault in kernel, better to send event before mdev release the
> reference to device.
> > >
> > > I know that get_device() and put_device() are GPL symbols and that's
> > > a bit of an issue, but I don't think we should be kludging the code
> > > for a vendor driver that might have problems with that.  A) we're
> > > using the caller provided device  for the uevent, B) we're only
> > > releasing our own reference to the device that was acquired during
> > > registration, the vendor driver must have other references,
> >
> > Are you going to assume that someone/vendor driver is always going to
> > do right thing?
> 
> mdev is a kernel driver, we make reasonable assumptions that other drivers
> interact with it correctly.
> 
That is right.
Vendor drivers must invoke mdev_register_device() and mdev_unregister_device() only once.
And it must have a valid reference to the device for which it is invoking it.
This is basic programming practice that a given driver has to follow.
mdev_register_device() has a loop to check. It needs to WARN_ON there if there are duplicate registration.
Similarly on mdev_unregister_device() to have WARN_ON if device is not found.
It was in my TODO list to submit those patches.
I was still thinking to that mdev_register_device() should return mdev_parent and mdev_unregister_device() should accept mdev_parent pointer, instead of WARN_ON on unregister().


> > > C) the parent device
> > > generally lives on a bus, with a vendor driver, there's an entire
> > > ecosystem of references to the device below mdev.  Is this a
> > > paranoia request or are you really concerned that your PCI device suddenly
> > > disappears when mdev's reference to it disappears.
> >
> > mdev infrastructure is not always used by PCI devices. It is designed
> > to be generic, so that other devices (other than PCI devices) can also
> > use this framework.
> 
> Obviously mdev is not PCI specific, I only mention it because I'm asking if you
> have a specific concern in mind.  If you did, I'd assume it's related to a PCI
> backed vGPU.  Any physical parent device of an mdev is likely to have some sort
> of bus infrastructure behind it holding references to the device (ie. a probe and
> release where an implicit reference is held between these points).  A virtual
> device would be similar, it's created as part of a module init and destroyed as
> part of a module exit, where mdev registration would exist between these
> points.
> 
> > If there is a assumption that user of mdev framework or vendor drivers
> > are always going to use mdev in right way, then there is no need for
> > mdev core to held reference of the device?
> > This is not a "paranoia request". This is more of a ideal scenario,
> > mdev should use device by holding its reference rather than assuming
> > (or relying on) someone else holding the reference of device.
> 
> In fact, at one point Parav was proposing removing these references entirely,
> but Connie and I both felt uncomfortable about that.  I think it's good practice
> that mdev indicates the use of the parent device by incrementing the reference
> count, with each child mdev device also taking a reference, but those
> references balance out within the mdev core.  Their purpose is not to maintain
> the device for outside callers, nor should outside callers assume mdev's use of
> references to release their own.  I don't think it's unreasonable to assume that
> the caller should have a legitimate reference to the object it's providing to this
> function and therefore we should be able to use it after mdev's internal
> references are balanced out.  Thanks,
> 
Yes, I also agree with Alex comment here to hold and release reference to mdev's parent device during reg/unreg routines.

> Alex

Re: [PATCH v2] mdev: Send uevents around parent device registration

[Kirti Wankhede]

On 7/2/2019 12:43 PM, Parav Pandit wrote:
> 
> 
>> -----Original Message-----
>> From: linux-kernel-owner@vger.kernel.org <linux-kernel-
>> owner@vger.kernel.org> On Behalf Of Alex Williamson
>> Sent: Tuesday, July 2, 2019 11:12 AM
>> To: Kirti Wankhede <kwankhede@nvidia.com>
>> Cc: cohuck@redhat.com; kvm@vger.kernel.org; linux-kernel@vger.kernel.org
>> Subject: Re: [PATCH v2] mdev: Send uevents around parent device registration
>>
>> On Tue, 2 Jul 2019 10:25:04 +0530
>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>
>>> On 7/2/2019 1:34 AM, Alex Williamson wrote:
>>>> On Mon, 1 Jul 2019 23:20:35 +0530
>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>>
>>>>> On 7/1/2019 10:54 PM, Alex Williamson wrote:
>>>>>> On Mon, 1 Jul 2019 22:43:10 +0530
>>>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>>>>
>>>>>>> On 7/1/2019 8:24 PM, Alex Williamson wrote:
>>>>>>>> This allows udev to trigger rules when a parent device is
>>>>>>>> registered or unregistered from mdev.
>>>>>>>>
>>>>>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>>>>>>> ---
>>>>>>>>
>>>>>>>> v2: Don't remove the dev_info(), Kirti requested they stay and
>>>>>>>>     removing them is only tangential to the goal of this change.
>>>>>>>>
>>>>>>>
>>>>>>> Thanks.
>>>>>>>
>>>>>>>
>>>>>>>>  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
>>>>>>>>  1 file changed, 8 insertions(+)
>>>>>>>>
>>>>>>>> diff --git a/drivers/vfio/mdev/mdev_core.c
>>>>>>>> b/drivers/vfio/mdev/mdev_core.c index ae23151442cb..7fb268136c62
>>>>>>>> 100644
>>>>>>>> --- a/drivers/vfio/mdev/mdev_core.c
>>>>>>>> +++ b/drivers/vfio/mdev/mdev_core.c
>>>>>>>> @@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev,
>>>>>>>> const struct mdev_parent_ops *ops)  {
>>>>>>>>  	int ret;
>>>>>>>>  	struct mdev_parent *parent;
>>>>>>>> +	char *env_string = "MDEV_STATE=registered";
>>>>>>>> +	char *envp[] = { env_string, NULL };
>>>>>>>>
>>>>>>>>  	/* check for mandatory ops */
>>>>>>>>  	if (!ops || !ops->create || !ops->remove ||
>>>>>>>> !ops->supported_type_groups) @@ -197,6 +199,8 @@ int
>> mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
>>>>>>>>  	mutex_unlock(&parent_list_lock);
>>>>>>>>
>>>>>>>>  	dev_info(dev, "MDEV: Registered\n");
>>>>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
>>>>>>>> +
>>>>>>>>  	return 0;
>>>>>>>>
>>>>>>>>  add_dev_err:
>>>>>>>> @@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
>>>>>>>>  void mdev_unregister_device(struct device *dev)  {
>>>>>>>>  	struct mdev_parent *parent;
>>>>>>>> +	char *env_string = "MDEV_STATE=unregistered";
>>>>>>>> +	char *envp[] = { env_string, NULL };
>>>>>>>>
>>>>>>>>  	mutex_lock(&parent_list_lock);
>>>>>>>>  	parent = __find_parent_device(dev); @@ -243,6 +249,8 @@
>> void
>>>>>>>> mdev_unregister_device(struct device *dev)
>>>>>>>>  	up_write(&parent->unreg_sem);
>>>>>>>>
>>>>>>>>  	mdev_put_parent(parent);
>>>>>>>> +
>>>>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
>>>>>>>
>>>>>>> mdev_put_parent() calls put_device(dev). If this is the last
>>>>>>> instance holding device, then on put_device(dev) dev would get freed.
>>>>>>>
>>>>>>> This event should be before mdev_put_parent()
>>>>>>
>>>>>> So you're suggesting the vendor driver is calling
>>>>>> mdev_unregister_device() without a reference to the struct device
>>>>>> that it's passing to unregister?  Sounds bogus to me.  We take a
>>>>>> reference to the device so that it can't disappear out from under
>>>>>> us, the caller cannot rely on our reference and the caller
>>>>>> provided the struct device.  Thanks,
>>>>>>
>>>>>
>>>>> 1. Register uevent is sent after mdev holding reference to device,
>>>>> then ideally, unregister path should be mirror of register path,
>>>>> send uevent and then release the reference to device.
>>>>
>>>> I don't see the relevance here.  We're marking an event, not
>>>> unwinding state of the device from the registration process.
>>>> Additionally, the event we're trying to mark is the completion of
>>>> each process, so the notion that we need to mirror the ordering between
>> the two is invalid.
>>>>
>>>>> 2. I agree that vendor driver shouldn't call
>>>>> mdev_unregister_device() without holding reference to device. But
>>>>> to be on safer side, if ever such case occur, to avoid any
>>>>> segmentation fault in kernel, better to send event before mdev release the
>> reference to device.
>>>>
>>>> I know that get_device() and put_device() are GPL symbols and that's
>>>> a bit of an issue, but I don't think we should be kludging the code
>>>> for a vendor driver that might have problems with that.  A) we're
>>>> using the caller provided device  for the uevent, B) we're only
>>>> releasing our own reference to the device that was acquired during
>>>> registration, the vendor driver must have other references,
>>>
>>> Are you going to assume that someone/vendor driver is always going to
>>> do right thing?
>>
>> mdev is a kernel driver, we make reasonable assumptions that other drivers
>> interact with it correctly.
>>
> That is right.
> Vendor drivers must invoke mdev_register_device() and mdev_unregister_device() only once.
> And it must have a valid reference to the device for which it is invoking it.
> This is basic programming practice that a given driver has to follow.
> mdev_register_device() has a loop to check. It needs to WARN_ON there if there are duplicate registration.
> Similarly on mdev_unregister_device() to have WARN_ON if device is not found.

If assumption is vendor driver is always going to do right way, then why
need check for duplicate registration? vendor driver is always going to
do it right way, right?


> It was in my TODO list to submit those patches.
> I was still thinking to that mdev_register_device() should return mdev_parent and mdev_unregister_device() should accept mdev_parent pointer, instead of WARN_ON on unregister().
> 
> 
>>>> C) the parent device
>>>> generally lives on a bus, with a vendor driver, there's an entire
>>>> ecosystem of references to the device below mdev.  Is this a
>>>> paranoia request or are you really concerned that your PCI device suddenly
>>>> disappears when mdev's reference to it disappears.
>>>
>>> mdev infrastructure is not always used by PCI devices. It is designed
>>> to be generic, so that other devices (other than PCI devices) can also
>>> use this framework.
>>
>> Obviously mdev is not PCI specific, I only mention it because I'm asking if you
>> have a specific concern in mind.  If you did, I'd assume it's related to a PCI
>> backed vGPU.

Its not always good to assume certain things.

>> Any physical parent device of an mdev is likely to have some sort
>> of bus infrastructure behind it holding references to the device (ie. a probe and
>> release where an implicit reference is held between these points).  A virtual
>> device would be similar, it's created as part of a module init and destroyed as
>> part of a module exit, where mdev registration would exist between these
>> points.
>>
>>> If there is a assumption that user of mdev framework or vendor drivers
>>> are always going to use mdev in right way, then there is no need for
>>> mdev core to held reference of the device?
>>> This is not a "paranoia request". This is more of a ideal scenario,
>>> mdev should use device by holding its reference rather than assuming
>>> (or relying on) someone else holding the reference of device.
>>
>> In fact, at one point Parav was proposing removing these references entirely,
>> but Connie and I both felt uncomfortable about that.  I think it's good practice
>> that mdev indicates the use of the parent device by incrementing the reference
>> count, with each child mdev device also taking a reference, but those
>> references balance out within the mdev core.  Their purpose is not to maintain
>> the device for outside callers, nor should outside callers assume mdev's use of
>> references to release their own.  I don't think it's unreasonable to assume that
>> the caller should have a legitimate reference to the object it's providing to this
>> function and therefore we should be able to use it after mdev's internal
>> references are balanced out.  Thanks,
>>

I'm not fully convinced with what is the advantage of sending uevent
after releasing reference to device or disadvantage of sending uevent
before releasing reference to device.

Still if you want to go ahead with this change, please add a check or
assert if (dev != NULL) and add an comment highlighting the assumption.

Thanks,
Kirti

Re: [PATCH v2] mdev: Send uevents around parent device registration

[Alex Williamson]

On Tue, 2 Jul 2019 18:17:41 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 7/2/2019 12:43 PM, Parav Pandit wrote:
> > 
> >   
> >> -----Original Message-----
> >> From: linux-kernel-owner@vger.kernel.org <linux-kernel-  
> >> owner@vger.kernel.org> On Behalf Of Alex Williamson  
> >> Sent: Tuesday, July 2, 2019 11:12 AM
> >> To: Kirti Wankhede <kwankhede@nvidia.com>
> >> Cc: cohuck@redhat.com; kvm@vger.kernel.org; linux-kernel@vger.kernel.org
> >> Subject: Re: [PATCH v2] mdev: Send uevents around parent device registration
> >>
> >> On Tue, 2 Jul 2019 10:25:04 +0530
> >> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>  
> >>> On 7/2/2019 1:34 AM, Alex Williamson wrote:  
> >>>> On Mon, 1 Jul 2019 23:20:35 +0530
> >>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>>  
> >>>>> On 7/1/2019 10:54 PM, Alex Williamson wrote:  
> >>>>>> On Mon, 1 Jul 2019 22:43:10 +0530
> >>>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>>>>  
> >>>>>>> On 7/1/2019 8:24 PM, Alex Williamson wrote:  
> >>>>>>>> This allows udev to trigger rules when a parent device is
> >>>>>>>> registered or unregistered from mdev.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >>>>>>>> ---
> >>>>>>>>
> >>>>>>>> v2: Don't remove the dev_info(), Kirti requested they stay and
> >>>>>>>>     removing them is only tangential to the goal of this change.
> >>>>>>>>  
> >>>>>>>
> >>>>>>> Thanks.
> >>>>>>>
> >>>>>>>  
> >>>>>>>>  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
> >>>>>>>>  1 file changed, 8 insertions(+)
> >>>>>>>>
> >>>>>>>> diff --git a/drivers/vfio/mdev/mdev_core.c
> >>>>>>>> b/drivers/vfio/mdev/mdev_core.c index ae23151442cb..7fb268136c62
> >>>>>>>> 100644
> >>>>>>>> --- a/drivers/vfio/mdev/mdev_core.c
> >>>>>>>> +++ b/drivers/vfio/mdev/mdev_core.c
> >>>>>>>> @@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev,
> >>>>>>>> const struct mdev_parent_ops *ops)  {
> >>>>>>>>  	int ret;
> >>>>>>>>  	struct mdev_parent *parent;
> >>>>>>>> +	char *env_string = "MDEV_STATE=registered";
> >>>>>>>> +	char *envp[] = { env_string, NULL };
> >>>>>>>>
> >>>>>>>>  	/* check for mandatory ops */
> >>>>>>>>  	if (!ops || !ops->create || !ops->remove ||
> >>>>>>>> !ops->supported_type_groups) @@ -197,6 +199,8 @@ int  
> >> mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)  
> >>>>>>>>  	mutex_unlock(&parent_list_lock);
> >>>>>>>>
> >>>>>>>>  	dev_info(dev, "MDEV: Registered\n");
> >>>>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> >>>>>>>> +
> >>>>>>>>  	return 0;
> >>>>>>>>
> >>>>>>>>  add_dev_err:
> >>>>>>>> @@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
> >>>>>>>>  void mdev_unregister_device(struct device *dev)  {
> >>>>>>>>  	struct mdev_parent *parent;
> >>>>>>>> +	char *env_string = "MDEV_STATE=unregistered";
> >>>>>>>> +	char *envp[] = { env_string, NULL };
> >>>>>>>>
> >>>>>>>>  	mutex_lock(&parent_list_lock);
> >>>>>>>>  	parent = __find_parent_device(dev); @@ -243,6 +249,8 @@  
> >> void  
> >>>>>>>> mdev_unregister_device(struct device *dev)
> >>>>>>>>  	up_write(&parent->unreg_sem);
> >>>>>>>>
> >>>>>>>>  	mdev_put_parent(parent);
> >>>>>>>> +
> >>>>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);  
> >>>>>>>
> >>>>>>> mdev_put_parent() calls put_device(dev). If this is the last
> >>>>>>> instance holding device, then on put_device(dev) dev would get freed.
> >>>>>>>
> >>>>>>> This event should be before mdev_put_parent()  
> >>>>>>
> >>>>>> So you're suggesting the vendor driver is calling
> >>>>>> mdev_unregister_device() without a reference to the struct device
> >>>>>> that it's passing to unregister?  Sounds bogus to me.  We take a
> >>>>>> reference to the device so that it can't disappear out from under
> >>>>>> us, the caller cannot rely on our reference and the caller
> >>>>>> provided the struct device.  Thanks,
> >>>>>>  
> >>>>>
> >>>>> 1. Register uevent is sent after mdev holding reference to device,
> >>>>> then ideally, unregister path should be mirror of register path,
> >>>>> send uevent and then release the reference to device.  
> >>>>
> >>>> I don't see the relevance here.  We're marking an event, not
> >>>> unwinding state of the device from the registration process.
> >>>> Additionally, the event we're trying to mark is the completion of
> >>>> each process, so the notion that we need to mirror the ordering between  
> >> the two is invalid.  
> >>>>  
> >>>>> 2. I agree that vendor driver shouldn't call
> >>>>> mdev_unregister_device() without holding reference to device. But
> >>>>> to be on safer side, if ever such case occur, to avoid any
> >>>>> segmentation fault in kernel, better to send event before mdev release the  
> >> reference to device.  
> >>>>
> >>>> I know that get_device() and put_device() are GPL symbols and that's
> >>>> a bit of an issue, but I don't think we should be kludging the code
> >>>> for a vendor driver that might have problems with that.  A) we're
> >>>> using the caller provided device  for the uevent, B) we're only
> >>>> releasing our own reference to the device that was acquired during
> >>>> registration, the vendor driver must have other references,  
> >>>
> >>> Are you going to assume that someone/vendor driver is always going to
> >>> do right thing?  
> >>
> >> mdev is a kernel driver, we make reasonable assumptions that other drivers
> >> interact with it correctly.
> >>  
> > That is right.
> > Vendor drivers must invoke mdev_register_device() and mdev_unregister_device() only once.
> > And it must have a valid reference to the device for which it is invoking it.
> > This is basic programming practice that a given driver has to follow.
> > mdev_register_device() has a loop to check. It needs to WARN_ON there if there are duplicate registration.
> > Similarly on mdev_unregister_device() to have WARN_ON if device is not found.  
> 
> If assumption is vendor driver is always going to do right way, then why
> need check for duplicate registration? vendor driver is always going to
> do it right way, right?

Are we intentionally misinterpreting "reasonable assumptions" here?

> > It was in my TODO list to submit those patches.
> > I was still thinking to that mdev_register_device() should return mdev_parent and mdev_unregister_device() should accept mdev_parent pointer, instead of WARN_ON on unregister().
> > 
> >   
> >>>> C) the parent device
> >>>> generally lives on a bus, with a vendor driver, there's an entire
> >>>> ecosystem of references to the device below mdev.  Is this a
> >>>> paranoia request or are you really concerned that your PCI device suddenly
> >>>> disappears when mdev's reference to it disappears.  
> >>>
> >>> mdev infrastructure is not always used by PCI devices. It is designed
> >>> to be generic, so that other devices (other than PCI devices) can also
> >>> use this framework.  
> >>
> >> Obviously mdev is not PCI specific, I only mention it because I'm asking if you
> >> have a specific concern in mind.  If you did, I'd assume it's related to a PCI
> >> backed vGPU.  
> 
> Its not always good to assume certain things.

It was only an attempt to relate to a specific issue that might concern
you.

> >> Any physical parent device of an mdev is likely to have some sort
> >> of bus infrastructure behind it holding references to the device (ie. a probe and
> >> release where an implicit reference is held between these points).  A virtual
> >> device would be similar, it's created as part of a module init and destroyed as
> >> part of a module exit, where mdev registration would exist between these
> >> points.
> >>  
> >>> If there is a assumption that user of mdev framework or vendor drivers
> >>> are always going to use mdev in right way, then there is no need for
> >>> mdev core to held reference of the device?
> >>> This is not a "paranoia request". This is more of a ideal scenario,
> >>> mdev should use device by holding its reference rather than assuming
> >>> (or relying on) someone else holding the reference of device.  
> >>
> >> In fact, at one point Parav was proposing removing these references entirely,
> >> but Connie and I both felt uncomfortable about that.  I think it's good practice
> >> that mdev indicates the use of the parent device by incrementing the reference
> >> count, with each child mdev device also taking a reference, but those
> >> references balance out within the mdev core.  Their purpose is not to maintain
> >> the device for outside callers, nor should outside callers assume mdev's use of
> >> references to release their own.  I don't think it's unreasonable to assume that
> >> the caller should have a legitimate reference to the object it's providing to this
> >> function and therefore we should be able to use it after mdev's internal
> >> references are balanced out.  Thanks,
> >>  
> 
> I'm not fully convinced with what is the advantage of sending uevent
> after releasing reference to device or disadvantage of sending uevent
> before releasing reference to device.

If mdev-core still holds a reference to the device, is it fully
unregistered?  Why not send the uevent at the point where the
notification is actually true?

> Still if you want to go ahead with this change, please add a check or
> assert if (dev != NULL) and add an comment highlighting the assumption.

If CONFIG_DEBUG_KOBJECT_RELEASE is enabled then the deletion of the
kobject can occur at some random delay after the last reference is
removed via a workqueue, so such a test would only introduce a false
sense of security for an issue that should not exist anyway.  Thanks,

Alex

Re: [PATCH v2] mdev: Send uevents around parent device registration

[Kirti Wankhede]

On 7/2/2019 6:38 PM, Alex Williamson wrote:
> On Tue, 2 Jul 2019 18:17:41 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 7/2/2019 12:43 PM, Parav Pandit wrote:
>>>
>>>   
>>>> -----Original Message-----
>>>> From: linux-kernel-owner@vger.kernel.org <linux-kernel-  
>>>> owner@vger.kernel.org> On Behalf Of Alex Williamson  
>>>> Sent: Tuesday, July 2, 2019 11:12 AM
>>>> To: Kirti Wankhede <kwankhede@nvidia.com>
>>>> Cc: cohuck@redhat.com; kvm@vger.kernel.org; linux-kernel@vger.kernel.org
>>>> Subject: Re: [PATCH v2] mdev: Send uevents around parent device registration
>>>>
>>>> On Tue, 2 Jul 2019 10:25:04 +0530
>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>>  
>>>>> On 7/2/2019 1:34 AM, Alex Williamson wrote:  
>>>>>> On Mon, 1 Jul 2019 23:20:35 +0530
>>>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>>>>  
>>>>>>> On 7/1/2019 10:54 PM, Alex Williamson wrote:  
>>>>>>>> On Mon, 1 Jul 2019 22:43:10 +0530
>>>>>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>>>>>>  
>>>>>>>>> On 7/1/2019 8:24 PM, Alex Williamson wrote:  
>>>>>>>>>> This allows udev to trigger rules when a parent device is
>>>>>>>>>> registered or unregistered from mdev.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>>>>>>>>> ---
>>>>>>>>>>
>>>>>>>>>> v2: Don't remove the dev_info(), Kirti requested they stay and
>>>>>>>>>>     removing them is only tangential to the goal of this change.
>>>>>>>>>>  
>>>>>>>>>
>>>>>>>>> Thanks.
>>>>>>>>>
>>>>>>>>>  
>>>>>>>>>>  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
>>>>>>>>>>  1 file changed, 8 insertions(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/vfio/mdev/mdev_core.c
>>>>>>>>>> b/drivers/vfio/mdev/mdev_core.c index ae23151442cb..7fb268136c62
>>>>>>>>>> 100644
>>>>>>>>>> --- a/drivers/vfio/mdev/mdev_core.c
>>>>>>>>>> +++ b/drivers/vfio/mdev/mdev_core.c
>>>>>>>>>> @@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev,
>>>>>>>>>> const struct mdev_parent_ops *ops)  {
>>>>>>>>>>  	int ret;
>>>>>>>>>>  	struct mdev_parent *parent;
>>>>>>>>>> +	char *env_string = "MDEV_STATE=registered";
>>>>>>>>>> +	char *envp[] = { env_string, NULL };
>>>>>>>>>>
>>>>>>>>>>  	/* check for mandatory ops */
>>>>>>>>>>  	if (!ops || !ops->create || !ops->remove ||
>>>>>>>>>> !ops->supported_type_groups) @@ -197,6 +199,8 @@ int  
>>>> mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)  
>>>>>>>>>>  	mutex_unlock(&parent_list_lock);
>>>>>>>>>>
>>>>>>>>>>  	dev_info(dev, "MDEV: Registered\n");
>>>>>>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
>>>>>>>>>> +
>>>>>>>>>>  	return 0;
>>>>>>>>>>
>>>>>>>>>>  add_dev_err:
>>>>>>>>>> @@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
>>>>>>>>>>  void mdev_unregister_device(struct device *dev)  {
>>>>>>>>>>  	struct mdev_parent *parent;
>>>>>>>>>> +	char *env_string = "MDEV_STATE=unregistered";
>>>>>>>>>> +	char *envp[] = { env_string, NULL };
>>>>>>>>>>
>>>>>>>>>>  	mutex_lock(&parent_list_lock);
>>>>>>>>>>  	parent = __find_parent_device(dev); @@ -243,6 +249,8 @@  
>>>> void  
>>>>>>>>>> mdev_unregister_device(struct device *dev)
>>>>>>>>>>  	up_write(&parent->unreg_sem);
>>>>>>>>>>
>>>>>>>>>>  	mdev_put_parent(parent);
>>>>>>>>>> +
>>>>>>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);  
>>>>>>>>>
>>>>>>>>> mdev_put_parent() calls put_device(dev). If this is the last
>>>>>>>>> instance holding device, then on put_device(dev) dev would get freed.
>>>>>>>>>
>>>>>>>>> This event should be before mdev_put_parent()  
>>>>>>>>
>>>>>>>> So you're suggesting the vendor driver is calling
>>>>>>>> mdev_unregister_device() without a reference to the struct device
>>>>>>>> that it's passing to unregister?  Sounds bogus to me.  We take a
>>>>>>>> reference to the device so that it can't disappear out from under
>>>>>>>> us, the caller cannot rely on our reference and the caller
>>>>>>>> provided the struct device.  Thanks,
>>>>>>>>  
>>>>>>>
>>>>>>> 1. Register uevent is sent after mdev holding reference to device,
>>>>>>> then ideally, unregister path should be mirror of register path,
>>>>>>> send uevent and then release the reference to device.  
>>>>>>
>>>>>> I don't see the relevance here.  We're marking an event, not
>>>>>> unwinding state of the device from the registration process.
>>>>>> Additionally, the event we're trying to mark is the completion of
>>>>>> each process, so the notion that we need to mirror the ordering between  
>>>> the two is invalid.  
>>>>>>  
>>>>>>> 2. I agree that vendor driver shouldn't call
>>>>>>> mdev_unregister_device() without holding reference to device. But
>>>>>>> to be on safer side, if ever such case occur, to avoid any
>>>>>>> segmentation fault in kernel, better to send event before mdev release the  
>>>> reference to device.  
>>>>>>
>>>>>> I know that get_device() and put_device() are GPL symbols and that's
>>>>>> a bit of an issue, but I don't think we should be kludging the code
>>>>>> for a vendor driver that might have problems with that.  A) we're
>>>>>> using the caller provided device  for the uevent, B) we're only
>>>>>> releasing our own reference to the device that was acquired during
>>>>>> registration, the vendor driver must have other references,  
>>>>>
>>>>> Are you going to assume that someone/vendor driver is always going to
>>>>> do right thing?  
>>>>
>>>> mdev is a kernel driver, we make reasonable assumptions that other drivers
>>>> interact with it correctly.
>>>>  
>>> That is right.
>>> Vendor drivers must invoke mdev_register_device() and mdev_unregister_device() only once.
>>> And it must have a valid reference to the device for which it is invoking it.
>>> This is basic programming practice that a given driver has to follow.
>>> mdev_register_device() has a loop to check. It needs to WARN_ON there if there are duplicate registration.
>>> Similarly on mdev_unregister_device() to have WARN_ON if device is not found.  
>>
>> If assumption is vendor driver is always going to do right way, then why
>> need check for duplicate registration? vendor driver is always going to
>> do it right way, right?
> 
> Are we intentionally misinterpreting "reasonable assumptions" here?
> 
>>> It was in my TODO list to submit those patches.
>>> I was still thinking to that mdev_register_device() should return mdev_parent and mdev_unregister_device() should accept mdev_parent pointer, instead of WARN_ON on unregister().
>>>
>>>   
>>>>>> C) the parent device
>>>>>> generally lives on a bus, with a vendor driver, there's an entire
>>>>>> ecosystem of references to the device below mdev.  Is this a
>>>>>> paranoia request or are you really concerned that your PCI device suddenly
>>>>>> disappears when mdev's reference to it disappears.  
>>>>>
>>>>> mdev infrastructure is not always used by PCI devices. It is designed
>>>>> to be generic, so that other devices (other than PCI devices) can also
>>>>> use this framework.  
>>>>
>>>> Obviously mdev is not PCI specific, I only mention it because I'm asking if you
>>>> have a specific concern in mind.  If you did, I'd assume it's related to a PCI
>>>> backed vGPU.  
>>
>> Its not always good to assume certain things.
> 
> It was only an attempt to relate to a specific issue that might concern
> you.
> 
>>>> Any physical parent device of an mdev is likely to have some sort
>>>> of bus infrastructure behind it holding references to the device (ie. a probe and
>>>> release where an implicit reference is held between these points).  A virtual
>>>> device would be similar, it's created as part of a module init and destroyed as
>>>> part of a module exit, where mdev registration would exist between these
>>>> points.
>>>>  
>>>>> If there is a assumption that user of mdev framework or vendor drivers
>>>>> are always going to use mdev in right way, then there is no need for
>>>>> mdev core to held reference of the device?
>>>>> This is not a "paranoia request". This is more of a ideal scenario,
>>>>> mdev should use device by holding its reference rather than assuming
>>>>> (or relying on) someone else holding the reference of device.  
>>>>
>>>> In fact, at one point Parav was proposing removing these references entirely,
>>>> but Connie and I both felt uncomfortable about that.  I think it's good practice
>>>> that mdev indicates the use of the parent device by incrementing the reference
>>>> count, with each child mdev device also taking a reference, but those
>>>> references balance out within the mdev core.  Their purpose is not to maintain
>>>> the device for outside callers, nor should outside callers assume mdev's use of
>>>> references to release their own.  I don't think it's unreasonable to assume that
>>>> the caller should have a legitimate reference to the object it's providing to this
>>>> function and therefore we should be able to use it after mdev's internal
>>>> references are balanced out.  Thanks,
>>>>  
>>
>> I'm not fully convinced with what is the advantage of sending uevent
>> after releasing reference to device or disadvantage of sending uevent
>> before releasing reference to device.
> 
> If mdev-core still holds a reference to the device, is it fully
> unregistered?  Why not send the uevent at the point where the
> notification is actually true?
>

By that time, device is removed from parent list, each child is removed
and sysfs files related to that parent are removed so that no new child
can be created, which means device is unregistered, only mdev_parent
structure is not yet freed which gets freed from mdev_put_parent().

Thanks,
Kirti

Re: [PATCH v2] mdev: Send uevents around parent device registration

[Alex Williamson]

On Tue, 2 Jul 2019 19:10:17 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 7/2/2019 6:38 PM, Alex Williamson wrote:
> > On Tue, 2 Jul 2019 18:17:41 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >   
> >> On 7/2/2019 12:43 PM, Parav Pandit wrote:  
> >>>
> >>>     
> >>>> -----Original Message-----
> >>>> From: linux-kernel-owner@vger.kernel.org <linux-kernel-    
> >>>> owner@vger.kernel.org> On Behalf Of Alex Williamson    
> >>>> Sent: Tuesday, July 2, 2019 11:12 AM
> >>>> To: Kirti Wankhede <kwankhede@nvidia.com>
> >>>> Cc: cohuck@redhat.com; kvm@vger.kernel.org; linux-kernel@vger.kernel.org
> >>>> Subject: Re: [PATCH v2] mdev: Send uevents around parent device registration
> >>>>
> >>>> On Tue, 2 Jul 2019 10:25:04 +0530
> >>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>>    
> >>>>> On 7/2/2019 1:34 AM, Alex Williamson wrote:    
> >>>>>> On Mon, 1 Jul 2019 23:20:35 +0530
> >>>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>>>>    
> >>>>>>> On 7/1/2019 10:54 PM, Alex Williamson wrote:    
> >>>>>>>> On Mon, 1 Jul 2019 22:43:10 +0530
> >>>>>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>>>>>>    
> >>>>>>>>> On 7/1/2019 8:24 PM, Alex Williamson wrote:    
> >>>>>>>>>> This allows udev to trigger rules when a parent device is
> >>>>>>>>>> registered or unregistered from mdev.
> >>>>>>>>>>
> >>>>>>>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >>>>>>>>>> ---
> >>>>>>>>>>
> >>>>>>>>>> v2: Don't remove the dev_info(), Kirti requested they stay and
> >>>>>>>>>>     removing them is only tangential to the goal of this change.
> >>>>>>>>>>    
> >>>>>>>>>
> >>>>>>>>> Thanks.
> >>>>>>>>>
> >>>>>>>>>    
> >>>>>>>>>>  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
> >>>>>>>>>>  1 file changed, 8 insertions(+)
> >>>>>>>>>>
> >>>>>>>>>> diff --git a/drivers/vfio/mdev/mdev_core.c
> >>>>>>>>>> b/drivers/vfio/mdev/mdev_core.c index ae23151442cb..7fb268136c62
> >>>>>>>>>> 100644
> >>>>>>>>>> --- a/drivers/vfio/mdev/mdev_core.c
> >>>>>>>>>> +++ b/drivers/vfio/mdev/mdev_core.c
> >>>>>>>>>> @@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev,
> >>>>>>>>>> const struct mdev_parent_ops *ops)  {
> >>>>>>>>>>  	int ret;
> >>>>>>>>>>  	struct mdev_parent *parent;
> >>>>>>>>>> +	char *env_string = "MDEV_STATE=registered";
> >>>>>>>>>> +	char *envp[] = { env_string, NULL };
> >>>>>>>>>>
> >>>>>>>>>>  	/* check for mandatory ops */
> >>>>>>>>>>  	if (!ops || !ops->create || !ops->remove ||
> >>>>>>>>>> !ops->supported_type_groups) @@ -197,6 +199,8 @@ int    
> >>>> mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)    
> >>>>>>>>>>  	mutex_unlock(&parent_list_lock);
> >>>>>>>>>>
> >>>>>>>>>>  	dev_info(dev, "MDEV: Registered\n");
> >>>>>>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> >>>>>>>>>> +
> >>>>>>>>>>  	return 0;
> >>>>>>>>>>
> >>>>>>>>>>  add_dev_err:
> >>>>>>>>>> @@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
> >>>>>>>>>>  void mdev_unregister_device(struct device *dev)  {
> >>>>>>>>>>  	struct mdev_parent *parent;
> >>>>>>>>>> +	char *env_string = "MDEV_STATE=unregistered";
> >>>>>>>>>> +	char *envp[] = { env_string, NULL };
> >>>>>>>>>>
> >>>>>>>>>>  	mutex_lock(&parent_list_lock);
> >>>>>>>>>>  	parent = __find_parent_device(dev); @@ -243,6 +249,8 @@    
> >>>> void    
> >>>>>>>>>> mdev_unregister_device(struct device *dev)
> >>>>>>>>>>  	up_write(&parent->unreg_sem);
> >>>>>>>>>>
> >>>>>>>>>>  	mdev_put_parent(parent);
> >>>>>>>>>> +
> >>>>>>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);    
> >>>>>>>>>
> >>>>>>>>> mdev_put_parent() calls put_device(dev). If this is the last
> >>>>>>>>> instance holding device, then on put_device(dev) dev would get freed.
> >>>>>>>>>
> >>>>>>>>> This event should be before mdev_put_parent()    
> >>>>>>>>
> >>>>>>>> So you're suggesting the vendor driver is calling
> >>>>>>>> mdev_unregister_device() without a reference to the struct device
> >>>>>>>> that it's passing to unregister?  Sounds bogus to me.  We take a
> >>>>>>>> reference to the device so that it can't disappear out from under
> >>>>>>>> us, the caller cannot rely on our reference and the caller
> >>>>>>>> provided the struct device.  Thanks,
> >>>>>>>>    
> >>>>>>>
> >>>>>>> 1. Register uevent is sent after mdev holding reference to device,
> >>>>>>> then ideally, unregister path should be mirror of register path,
> >>>>>>> send uevent and then release the reference to device.    
> >>>>>>
> >>>>>> I don't see the relevance here.  We're marking an event, not
> >>>>>> unwinding state of the device from the registration process.
> >>>>>> Additionally, the event we're trying to mark is the completion of
> >>>>>> each process, so the notion that we need to mirror the ordering between    
> >>>> the two is invalid.    
> >>>>>>    
> >>>>>>> 2. I agree that vendor driver shouldn't call
> >>>>>>> mdev_unregister_device() without holding reference to device. But
> >>>>>>> to be on safer side, if ever such case occur, to avoid any
> >>>>>>> segmentation fault in kernel, better to send event before mdev release the    
> >>>> reference to device.    
> >>>>>>
> >>>>>> I know that get_device() and put_device() are GPL symbols and that's
> >>>>>> a bit of an issue, but I don't think we should be kludging the code
> >>>>>> for a vendor driver that might have problems with that.  A) we're
> >>>>>> using the caller provided device  for the uevent, B) we're only
> >>>>>> releasing our own reference to the device that was acquired during
> >>>>>> registration, the vendor driver must have other references,    
> >>>>>
> >>>>> Are you going to assume that someone/vendor driver is always going to
> >>>>> do right thing?    
> >>>>
> >>>> mdev is a kernel driver, we make reasonable assumptions that other drivers
> >>>> interact with it correctly.
> >>>>    
> >>> That is right.
> >>> Vendor drivers must invoke mdev_register_device() and mdev_unregister_device() only once.
> >>> And it must have a valid reference to the device for which it is invoking it.
> >>> This is basic programming practice that a given driver has to follow.
> >>> mdev_register_device() has a loop to check. It needs to WARN_ON there if there are duplicate registration.
> >>> Similarly on mdev_unregister_device() to have WARN_ON if device is not found.    
> >>
> >> If assumption is vendor driver is always going to do right way, then why
> >> need check for duplicate registration? vendor driver is always going to
> >> do it right way, right?  
> > 
> > Are we intentionally misinterpreting "reasonable assumptions" here?
> >   
> >>> It was in my TODO list to submit those patches.
> >>> I was still thinking to that mdev_register_device() should return mdev_parent and mdev_unregister_device() should accept mdev_parent pointer, instead of WARN_ON on unregister().
> >>>
> >>>     
> >>>>>> C) the parent device
> >>>>>> generally lives on a bus, with a vendor driver, there's an entire
> >>>>>> ecosystem of references to the device below mdev.  Is this a
> >>>>>> paranoia request or are you really concerned that your PCI device suddenly
> >>>>>> disappears when mdev's reference to it disappears.    
> >>>>>
> >>>>> mdev infrastructure is not always used by PCI devices. It is designed
> >>>>> to be generic, so that other devices (other than PCI devices) can also
> >>>>> use this framework.    
> >>>>
> >>>> Obviously mdev is not PCI specific, I only mention it because I'm asking if you
> >>>> have a specific concern in mind.  If you did, I'd assume it's related to a PCI
> >>>> backed vGPU.    
> >>
> >> Its not always good to assume certain things.  
> > 
> > It was only an attempt to relate to a specific issue that might concern
> > you.
> >   
> >>>> Any physical parent device of an mdev is likely to have some sort
> >>>> of bus infrastructure behind it holding references to the device (ie. a probe and
> >>>> release where an implicit reference is held between these points).  A virtual
> >>>> device would be similar, it's created as part of a module init and destroyed as
> >>>> part of a module exit, where mdev registration would exist between these
> >>>> points.
> >>>>    
> >>>>> If there is a assumption that user of mdev framework or vendor drivers
> >>>>> are always going to use mdev in right way, then there is no need for
> >>>>> mdev core to held reference of the device?
> >>>>> This is not a "paranoia request". This is more of a ideal scenario,
> >>>>> mdev should use device by holding its reference rather than assuming
> >>>>> (or relying on) someone else holding the reference of device.    
> >>>>
> >>>> In fact, at one point Parav was proposing removing these references entirely,
> >>>> but Connie and I both felt uncomfortable about that.  I think it's good practice
> >>>> that mdev indicates the use of the parent device by incrementing the reference
> >>>> count, with each child mdev device also taking a reference, but those
> >>>> references balance out within the mdev core.  Their purpose is not to maintain
> >>>> the device for outside callers, nor should outside callers assume mdev's use of
> >>>> references to release their own.  I don't think it's unreasonable to assume that
> >>>> the caller should have a legitimate reference to the object it's providing to this
> >>>> function and therefore we should be able to use it after mdev's internal
> >>>> references are balanced out.  Thanks,
> >>>>    
> >>
> >> I'm not fully convinced with what is the advantage of sending uevent
> >> after releasing reference to device or disadvantage of sending uevent
> >> before releasing reference to device.  
> > 
> > If mdev-core still holds a reference to the device, is it fully
> > unregistered?  Why not send the uevent at the point where the
> > notification is actually true?
> >  
> 
> By that time, device is removed from parent list, each child is removed
> and sysfs files related to that parent are removed so that no new child
> can be created, which means device is unregistered, only mdev_parent
> structure is not yet freed which gets freed from mdev_put_parent().

So you're saying it's 95% unregistered, but there's still a tracking
structure yet to free, so go ahead an send a uevent just in case the
caller didn't have a valid reference to the device they passed and it
might get freed.  Isn't this the original request which we've decided
is unreasonable paranoia?  Please cite an instance where this makes any
sense.  Someone called us with a reference to the device to register
with mdev.  The references we've acquired are entirely balanced within
the mdev-core and you're suggesting that the unregistration caller has
released their own reference to the device and now relies on ours,
which we're under no obligation to hold in the first place.  We're
using a caller provided object after tearing down our own internal
tracking, which should have no bearing on external tracking of this
object.  How can that be an issue?  Thanks,

Alex

Re: [PATCH v2] mdev: Send uevents around parent device registration

[Kirti Wankhede]

On 7/2/2019 8:13 PM, Alex Williamson wrote:
> On Tue, 2 Jul 2019 19:10:17 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 7/2/2019 6:38 PM, Alex Williamson wrote:
>>> On Tue, 2 Jul 2019 18:17:41 +0530
>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>   
>>>> On 7/2/2019 12:43 PM, Parav Pandit wrote:  
>>>>>
>>>>>     
>>>>>> -----Original Message-----
>>>>>> From: linux-kernel-owner@vger.kernel.org <linux-kernel-    
>>>>>> owner@vger.kernel.org> On Behalf Of Alex Williamson    
>>>>>> Sent: Tuesday, July 2, 2019 11:12 AM
>>>>>> To: Kirti Wankhede <kwankhede@nvidia.com>
>>>>>> Cc: cohuck@redhat.com; kvm@vger.kernel.org; linux-kernel@vger.kernel.org
>>>>>> Subject: Re: [PATCH v2] mdev: Send uevents around parent device registration
>>>>>>
>>>>>> On Tue, 2 Jul 2019 10:25:04 +0530
>>>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>>>>    
>>>>>>> On 7/2/2019 1:34 AM, Alex Williamson wrote:    
>>>>>>>> On Mon, 1 Jul 2019 23:20:35 +0530
>>>>>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>>>>>>    
>>>>>>>>> On 7/1/2019 10:54 PM, Alex Williamson wrote:    
>>>>>>>>>> On Mon, 1 Jul 2019 22:43:10 +0530
>>>>>>>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>>>>>>>>    
>>>>>>>>>>> On 7/1/2019 8:24 PM, Alex Williamson wrote:    
>>>>>>>>>>>> This allows udev to trigger rules when a parent device is
>>>>>>>>>>>> registered or unregistered from mdev.
>>>>>>>>>>>>
>>>>>>>>>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
>>>>>>>>>>>> ---
>>>>>>>>>>>>
>>>>>>>>>>>> v2: Don't remove the dev_info(), Kirti requested they stay and
>>>>>>>>>>>>     removing them is only tangential to the goal of this change.
>>>>>>>>>>>>    
>>>>>>>>>>>
>>>>>>>>>>> Thanks.
>>>>>>>>>>>
>>>>>>>>>>>    
>>>>>>>>>>>>  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
>>>>>>>>>>>>  1 file changed, 8 insertions(+)
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/drivers/vfio/mdev/mdev_core.c
>>>>>>>>>>>> b/drivers/vfio/mdev/mdev_core.c index ae23151442cb..7fb268136c62
>>>>>>>>>>>> 100644
>>>>>>>>>>>> --- a/drivers/vfio/mdev/mdev_core.c
>>>>>>>>>>>> +++ b/drivers/vfio/mdev/mdev_core.c
>>>>>>>>>>>> @@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev,
>>>>>>>>>>>> const struct mdev_parent_ops *ops)  {
>>>>>>>>>>>>  	int ret;
>>>>>>>>>>>>  	struct mdev_parent *parent;
>>>>>>>>>>>> +	char *env_string = "MDEV_STATE=registered";
>>>>>>>>>>>> +	char *envp[] = { env_string, NULL };
>>>>>>>>>>>>
>>>>>>>>>>>>  	/* check for mandatory ops */
>>>>>>>>>>>>  	if (!ops || !ops->create || !ops->remove ||
>>>>>>>>>>>> !ops->supported_type_groups) @@ -197,6 +199,8 @@ int    
>>>>>> mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)    
>>>>>>>>>>>>  	mutex_unlock(&parent_list_lock);
>>>>>>>>>>>>
>>>>>>>>>>>>  	dev_info(dev, "MDEV: Registered\n");
>>>>>>>>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
>>>>>>>>>>>> +
>>>>>>>>>>>>  	return 0;
>>>>>>>>>>>>
>>>>>>>>>>>>  add_dev_err:
>>>>>>>>>>>> @@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
>>>>>>>>>>>>  void mdev_unregister_device(struct device *dev)  {
>>>>>>>>>>>>  	struct mdev_parent *parent;
>>>>>>>>>>>> +	char *env_string = "MDEV_STATE=unregistered";
>>>>>>>>>>>> +	char *envp[] = { env_string, NULL };
>>>>>>>>>>>>
>>>>>>>>>>>>  	mutex_lock(&parent_list_lock);
>>>>>>>>>>>>  	parent = __find_parent_device(dev); @@ -243,6 +249,8 @@    
>>>>>> void    
>>>>>>>>>>>> mdev_unregister_device(struct device *dev)
>>>>>>>>>>>>  	up_write(&parent->unreg_sem);
>>>>>>>>>>>>
>>>>>>>>>>>>  	mdev_put_parent(parent);
>>>>>>>>>>>> +
>>>>>>>>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);    
>>>>>>>>>>>
>>>>>>>>>>> mdev_put_parent() calls put_device(dev). If this is the last
>>>>>>>>>>> instance holding device, then on put_device(dev) dev would get freed.
>>>>>>>>>>>
>>>>>>>>>>> This event should be before mdev_put_parent()    
>>>>>>>>>>
>>>>>>>>>> So you're suggesting the vendor driver is calling
>>>>>>>>>> mdev_unregister_device() without a reference to the struct device
>>>>>>>>>> that it's passing to unregister?  Sounds bogus to me.  We take a
>>>>>>>>>> reference to the device so that it can't disappear out from under
>>>>>>>>>> us, the caller cannot rely on our reference and the caller
>>>>>>>>>> provided the struct device.  Thanks,
>>>>>>>>>>    
>>>>>>>>>
>>>>>>>>> 1. Register uevent is sent after mdev holding reference to device,
>>>>>>>>> then ideally, unregister path should be mirror of register path,
>>>>>>>>> send uevent and then release the reference to device.    
>>>>>>>>
>>>>>>>> I don't see the relevance here.  We're marking an event, not
>>>>>>>> unwinding state of the device from the registration process.
>>>>>>>> Additionally, the event we're trying to mark is the completion of
>>>>>>>> each process, so the notion that we need to mirror the ordering between    
>>>>>> the two is invalid.    
>>>>>>>>    
>>>>>>>>> 2. I agree that vendor driver shouldn't call
>>>>>>>>> mdev_unregister_device() without holding reference to device. But
>>>>>>>>> to be on safer side, if ever such case occur, to avoid any
>>>>>>>>> segmentation fault in kernel, better to send event before mdev release the    
>>>>>> reference to device.    
>>>>>>>>
>>>>>>>> I know that get_device() and put_device() are GPL symbols and that's
>>>>>>>> a bit of an issue, but I don't think we should be kludging the code
>>>>>>>> for a vendor driver that might have problems with that.  A) we're
>>>>>>>> using the caller provided device  for the uevent, B) we're only
>>>>>>>> releasing our own reference to the device that was acquired during
>>>>>>>> registration, the vendor driver must have other references,    
>>>>>>>
>>>>>>> Are you going to assume that someone/vendor driver is always going to
>>>>>>> do right thing?    
>>>>>>
>>>>>> mdev is a kernel driver, we make reasonable assumptions that other drivers
>>>>>> interact with it correctly.
>>>>>>    
>>>>> That is right.
>>>>> Vendor drivers must invoke mdev_register_device() and mdev_unregister_device() only once.
>>>>> And it must have a valid reference to the device for which it is invoking it.
>>>>> This is basic programming practice that a given driver has to follow.
>>>>> mdev_register_device() has a loop to check. It needs to WARN_ON there if there are duplicate registration.
>>>>> Similarly on mdev_unregister_device() to have WARN_ON if device is not found.    
>>>>
>>>> If assumption is vendor driver is always going to do right way, then why
>>>> need check for duplicate registration? vendor driver is always going to
>>>> do it right way, right?  
>>>
>>> Are we intentionally misinterpreting "reasonable assumptions" here?
>>>   
>>>>> It was in my TODO list to submit those patches.
>>>>> I was still thinking to that mdev_register_device() should return mdev_parent and mdev_unregister_device() should accept mdev_parent pointer, instead of WARN_ON on unregister().
>>>>>
>>>>>     
>>>>>>>> C) the parent device
>>>>>>>> generally lives on a bus, with a vendor driver, there's an entire
>>>>>>>> ecosystem of references to the device below mdev.  Is this a
>>>>>>>> paranoia request or are you really concerned that your PCI device suddenly
>>>>>>>> disappears when mdev's reference to it disappears.    
>>>>>>>
>>>>>>> mdev infrastructure is not always used by PCI devices. It is designed
>>>>>>> to be generic, so that other devices (other than PCI devices) can also
>>>>>>> use this framework.    
>>>>>>
>>>>>> Obviously mdev is not PCI specific, I only mention it because I'm asking if you
>>>>>> have a specific concern in mind.  If you did, I'd assume it's related to a PCI
>>>>>> backed vGPU.    
>>>>
>>>> Its not always good to assume certain things.  
>>>
>>> It was only an attempt to relate to a specific issue that might concern
>>> you.
>>>   
>>>>>> Any physical parent device of an mdev is likely to have some sort
>>>>>> of bus infrastructure behind it holding references to the device (ie. a probe and
>>>>>> release where an implicit reference is held between these points).  A virtual
>>>>>> device would be similar, it's created as part of a module init and destroyed as
>>>>>> part of a module exit, where mdev registration would exist between these
>>>>>> points.
>>>>>>    
>>>>>>> If there is a assumption that user of mdev framework or vendor drivers
>>>>>>> are always going to use mdev in right way, then there is no need for
>>>>>>> mdev core to held reference of the device?
>>>>>>> This is not a "paranoia request". This is more of a ideal scenario,
>>>>>>> mdev should use device by holding its reference rather than assuming
>>>>>>> (or relying on) someone else holding the reference of device.    
>>>>>>
>>>>>> In fact, at one point Parav was proposing removing these references entirely,
>>>>>> but Connie and I both felt uncomfortable about that.  I think it's good practice
>>>>>> that mdev indicates the use of the parent device by incrementing the reference
>>>>>> count, with each child mdev device also taking a reference, but those
>>>>>> references balance out within the mdev core.  Their purpose is not to maintain
>>>>>> the device for outside callers, nor should outside callers assume mdev's use of
>>>>>> references to release their own.  I don't think it's unreasonable to assume that
>>>>>> the caller should have a legitimate reference to the object it's providing to this
>>>>>> function and therefore we should be able to use it after mdev's internal
>>>>>> references are balanced out.  Thanks,
>>>>>>    
>>>>
>>>> I'm not fully convinced with what is the advantage of sending uevent
>>>> after releasing reference to device or disadvantage of sending uevent
>>>> before releasing reference to device.  
>>>
>>> If mdev-core still holds a reference to the device, is it fully
>>> unregistered?  Why not send the uevent at the point where the
>>> notification is actually true?
>>>  
>>
>> By that time, device is removed from parent list, each child is removed
>> and sysfs files related to that parent are removed so that no new child
>> can be created, which means device is unregistered, only mdev_parent
>> structure is not yet freed which gets freed from mdev_put_parent().
> 
> So you're saying it's 95% unregistered, but there's still a tracking
> structure yet to free,

Its almost unregistered, that tracking structure is unusable since all
other interfaces are already removed and it is also removed from parent
list.

> so go ahead an send a uevent just in case the
> caller didn't have a valid reference to the device they passed and it
> might get freed.  Isn't this the original request which we've decided
> is unreasonable paranoia?  Please cite an instance where this makes any
> sense.  Someone called us with a reference to the device to register
> with mdev.  The references we've acquired are entirely balanced within
> the mdev-core and you're suggesting that the unregistration caller has
> released their own reference to the device and now relies on ours,
> which we're under no obligation to hold in the first place.  We're
> using a caller provided object after tearing down our own internal
> tracking, which should have no bearing on external tracking of this
> object.  How can that be an issue?  Thanks,
> 

You gave a reference in previous mail, release is deferred via workqueue

> If CONFIG_DEBUG_KOBJECT_RELEASE is enabled then the deletion of the
> kobject can occur at some random delay after the last reference is
> removed via a workqueue,

similarly if someone defers unregistration, this situation might occur.
Again this is a very acute corner case.
I still think right way is to use device by holding its reference rather
than assuming someone else holding the reference of device.

Thanks,
Kirti

Re: [PATCH v2] mdev: Send uevents around parent device registration

[Alex Williamson]

On Tue, 2 Jul 2019 23:34:30 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 7/2/2019 8:13 PM, Alex Williamson wrote:
> > On Tue, 2 Jul 2019 19:10:17 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >   
> >> On 7/2/2019 6:38 PM, Alex Williamson wrote:  
> >>> On Tue, 2 Jul 2019 18:17:41 +0530
> >>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>     
> >>>> On 7/2/2019 12:43 PM, Parav Pandit wrote:    
> >>>>>
> >>>>>       
> >>>>>> -----Original Message-----
> >>>>>> From: linux-kernel-owner@vger.kernel.org <linux-kernel-      
> >>>>>> owner@vger.kernel.org> On Behalf Of Alex Williamson      
> >>>>>> Sent: Tuesday, July 2, 2019 11:12 AM
> >>>>>> To: Kirti Wankhede <kwankhede@nvidia.com>
> >>>>>> Cc: cohuck@redhat.com; kvm@vger.kernel.org; linux-kernel@vger.kernel.org
> >>>>>> Subject: Re: [PATCH v2] mdev: Send uevents around parent device registration
> >>>>>>
> >>>>>> On Tue, 2 Jul 2019 10:25:04 +0530
> >>>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>>>>      
> >>>>>>> On 7/2/2019 1:34 AM, Alex Williamson wrote:      
> >>>>>>>> On Mon, 1 Jul 2019 23:20:35 +0530
> >>>>>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>>>>>>      
> >>>>>>>>> On 7/1/2019 10:54 PM, Alex Williamson wrote:      
> >>>>>>>>>> On Mon, 1 Jul 2019 22:43:10 +0530
> >>>>>>>>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>>>>>>>>      
> >>>>>>>>>>> On 7/1/2019 8:24 PM, Alex Williamson wrote:      
> >>>>>>>>>>>> This allows udev to trigger rules when a parent device is
> >>>>>>>>>>>> registered or unregistered from mdev.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Signed-off-by: Alex Williamson <alex.williamson@redhat.com>
> >>>>>>>>>>>> ---
> >>>>>>>>>>>>
> >>>>>>>>>>>> v2: Don't remove the dev_info(), Kirti requested they stay and
> >>>>>>>>>>>>     removing them is only tangential to the goal of this change.
> >>>>>>>>>>>>      
> >>>>>>>>>>>
> >>>>>>>>>>> Thanks.
> >>>>>>>>>>>
> >>>>>>>>>>>      
> >>>>>>>>>>>>  drivers/vfio/mdev/mdev_core.c |    8 ++++++++
> >>>>>>>>>>>>  1 file changed, 8 insertions(+)
> >>>>>>>>>>>>
> >>>>>>>>>>>> diff --git a/drivers/vfio/mdev/mdev_core.c
> >>>>>>>>>>>> b/drivers/vfio/mdev/mdev_core.c index ae23151442cb..7fb268136c62
> >>>>>>>>>>>> 100644
> >>>>>>>>>>>> --- a/drivers/vfio/mdev/mdev_core.c
> >>>>>>>>>>>> +++ b/drivers/vfio/mdev/mdev_core.c
> >>>>>>>>>>>> @@ -146,6 +146,8 @@ int mdev_register_device(struct device *dev,
> >>>>>>>>>>>> const struct mdev_parent_ops *ops)  {
> >>>>>>>>>>>>  	int ret;
> >>>>>>>>>>>>  	struct mdev_parent *parent;
> >>>>>>>>>>>> +	char *env_string = "MDEV_STATE=registered";
> >>>>>>>>>>>> +	char *envp[] = { env_string, NULL };
> >>>>>>>>>>>>
> >>>>>>>>>>>>  	/* check for mandatory ops */
> >>>>>>>>>>>>  	if (!ops || !ops->create || !ops->remove ||
> >>>>>>>>>>>> !ops->supported_type_groups) @@ -197,6 +199,8 @@ int      
> >>>>>> mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)      
> >>>>>>>>>>>>  	mutex_unlock(&parent_list_lock);
> >>>>>>>>>>>>
> >>>>>>>>>>>>  	dev_info(dev, "MDEV: Registered\n");
> >>>>>>>>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> >>>>>>>>>>>> +
> >>>>>>>>>>>>  	return 0;
> >>>>>>>>>>>>
> >>>>>>>>>>>>  add_dev_err:
> >>>>>>>>>>>> @@ -220,6 +224,8 @@ EXPORT_SYMBOL(mdev_register_device);
> >>>>>>>>>>>>  void mdev_unregister_device(struct device *dev)  {
> >>>>>>>>>>>>  	struct mdev_parent *parent;
> >>>>>>>>>>>> +	char *env_string = "MDEV_STATE=unregistered";
> >>>>>>>>>>>> +	char *envp[] = { env_string, NULL };
> >>>>>>>>>>>>
> >>>>>>>>>>>>  	mutex_lock(&parent_list_lock);
> >>>>>>>>>>>>  	parent = __find_parent_device(dev); @@ -243,6 +249,8 @@      
> >>>>>> void      
> >>>>>>>>>>>> mdev_unregister_device(struct device *dev)
> >>>>>>>>>>>>  	up_write(&parent->unreg_sem);
> >>>>>>>>>>>>
> >>>>>>>>>>>>  	mdev_put_parent(parent);
> >>>>>>>>>>>> +
> >>>>>>>>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);      
> >>>>>>>>>>>
> >>>>>>>>>>> mdev_put_parent() calls put_device(dev). If this is the last
> >>>>>>>>>>> instance holding device, then on put_device(dev) dev would get freed.
> >>>>>>>>>>>
> >>>>>>>>>>> This event should be before mdev_put_parent()      
> >>>>>>>>>>
> >>>>>>>>>> So you're suggesting the vendor driver is calling
> >>>>>>>>>> mdev_unregister_device() without a reference to the struct device
> >>>>>>>>>> that it's passing to unregister?  Sounds bogus to me.  We take a
> >>>>>>>>>> reference to the device so that it can't disappear out from under
> >>>>>>>>>> us, the caller cannot rely on our reference and the caller
> >>>>>>>>>> provided the struct device.  Thanks,
> >>>>>>>>>>      
> >>>>>>>>>
> >>>>>>>>> 1. Register uevent is sent after mdev holding reference to device,
> >>>>>>>>> then ideally, unregister path should be mirror of register path,
> >>>>>>>>> send uevent and then release the reference to device.      
> >>>>>>>>
> >>>>>>>> I don't see the relevance here.  We're marking an event, not
> >>>>>>>> unwinding state of the device from the registration process.
> >>>>>>>> Additionally, the event we're trying to mark is the completion of
> >>>>>>>> each process, so the notion that we need to mirror the ordering between      
> >>>>>> the two is invalid.      
> >>>>>>>>      
> >>>>>>>>> 2. I agree that vendor driver shouldn't call
> >>>>>>>>> mdev_unregister_device() without holding reference to device. But
> >>>>>>>>> to be on safer side, if ever such case occur, to avoid any
> >>>>>>>>> segmentation fault in kernel, better to send event before mdev release the      
> >>>>>> reference to device.      
> >>>>>>>>
> >>>>>>>> I know that get_device() and put_device() are GPL symbols and that's
> >>>>>>>> a bit of an issue, but I don't think we should be kludging the code
> >>>>>>>> for a vendor driver that might have problems with that.  A) we're
> >>>>>>>> using the caller provided device  for the uevent, B) we're only
> >>>>>>>> releasing our own reference to the device that was acquired during
> >>>>>>>> registration, the vendor driver must have other references,      
> >>>>>>>
> >>>>>>> Are you going to assume that someone/vendor driver is always going to
> >>>>>>> do right thing?      
> >>>>>>
> >>>>>> mdev is a kernel driver, we make reasonable assumptions that other drivers
> >>>>>> interact with it correctly.
> >>>>>>      
> >>>>> That is right.
> >>>>> Vendor drivers must invoke mdev_register_device() and mdev_unregister_device() only once.
> >>>>> And it must have a valid reference to the device for which it is invoking it.
> >>>>> This is basic programming practice that a given driver has to follow.
> >>>>> mdev_register_device() has a loop to check. It needs to WARN_ON there if there are duplicate registration.
> >>>>> Similarly on mdev_unregister_device() to have WARN_ON if device is not found.      
> >>>>
> >>>> If assumption is vendor driver is always going to do right way, then why
> >>>> need check for duplicate registration? vendor driver is always going to
> >>>> do it right way, right?    
> >>>
> >>> Are we intentionally misinterpreting "reasonable assumptions" here?
> >>>     
> >>>>> It was in my TODO list to submit those patches.
> >>>>> I was still thinking to that mdev_register_device() should return mdev_parent and mdev_unregister_device() should accept mdev_parent pointer, instead of WARN_ON on unregister().
> >>>>>
> >>>>>       
> >>>>>>>> C) the parent device
> >>>>>>>> generally lives on a bus, with a vendor driver, there's an entire
> >>>>>>>> ecosystem of references to the device below mdev.  Is this a
> >>>>>>>> paranoia request or are you really concerned that your PCI device suddenly
> >>>>>>>> disappears when mdev's reference to it disappears.      
> >>>>>>>
> >>>>>>> mdev infrastructure is not always used by PCI devices. It is designed
> >>>>>>> to be generic, so that other devices (other than PCI devices) can also
> >>>>>>> use this framework.      
> >>>>>>
> >>>>>> Obviously mdev is not PCI specific, I only mention it because I'm asking if you
> >>>>>> have a specific concern in mind.  If you did, I'd assume it's related to a PCI
> >>>>>> backed vGPU.      
> >>>>
> >>>> Its not always good to assume certain things.    
> >>>
> >>> It was only an attempt to relate to a specific issue that might concern
> >>> you.
> >>>     
> >>>>>> Any physical parent device of an mdev is likely to have some sort
> >>>>>> of bus infrastructure behind it holding references to the device (ie. a probe and
> >>>>>> release where an implicit reference is held between these points).  A virtual
> >>>>>> device would be similar, it's created as part of a module init and destroyed as
> >>>>>> part of a module exit, where mdev registration would exist between these
> >>>>>> points.
> >>>>>>      
> >>>>>>> If there is a assumption that user of mdev framework or vendor drivers
> >>>>>>> are always going to use mdev in right way, then there is no need for
> >>>>>>> mdev core to held reference of the device?
> >>>>>>> This is not a "paranoia request". This is more of a ideal scenario,
> >>>>>>> mdev should use device by holding its reference rather than assuming
> >>>>>>> (or relying on) someone else holding the reference of device.      
> >>>>>>
> >>>>>> In fact, at one point Parav was proposing removing these references entirely,
> >>>>>> but Connie and I both felt uncomfortable about that.  I think it's good practice
> >>>>>> that mdev indicates the use of the parent device by incrementing the reference
> >>>>>> count, with each child mdev device also taking a reference, but those
> >>>>>> references balance out within the mdev core.  Their purpose is not to maintain
> >>>>>> the device for outside callers, nor should outside callers assume mdev's use of
> >>>>>> references to release their own.  I don't think it's unreasonable to assume that
> >>>>>> the caller should have a legitimate reference to the object it's providing to this
> >>>>>> function and therefore we should be able to use it after mdev's internal
> >>>>>> references are balanced out.  Thanks,
> >>>>>>      
> >>>>
> >>>> I'm not fully convinced with what is the advantage of sending uevent
> >>>> after releasing reference to device or disadvantage of sending uevent
> >>>> before releasing reference to device.    
> >>>
> >>> If mdev-core still holds a reference to the device, is it fully
> >>> unregistered?  Why not send the uevent at the point where the
> >>> notification is actually true?
> >>>    
> >>
> >> By that time, device is removed from parent list, each child is removed
> >> and sysfs files related to that parent are removed so that no new child
> >> can be created, which means device is unregistered, only mdev_parent
> >> structure is not yet freed which gets freed from mdev_put_parent().  
> > 
> > So you're saying it's 95% unregistered, but there's still a tracking
> > structure yet to free,  
> 
> Its almost unregistered, that tracking structure is unusable since all
> other interfaces are already removed and it is also removed from parent
> list.

So we agree that it's not fully unregistered at that point.

> > so go ahead an send a uevent just in case the
> > caller didn't have a valid reference to the device they passed and it
> > might get freed.  Isn't this the original request which we've decided
> > is unreasonable paranoia?  Please cite an instance where this makes any
> > sense.  Someone called us with a reference to the device to register
> > with mdev.  The references we've acquired are entirely balanced within
> > the mdev-core and you're suggesting that the unregistration caller has
> > released their own reference to the device and now relies on ours,
> > which we're under no obligation to hold in the first place.  We're
> > using a caller provided object after tearing down our own internal
> > tracking, which should have no bearing on external tracking of this
> > object.  How can that be an issue?  Thanks,
> >   
> 
> You gave a reference in previous mail, release is deferred via workqueue
> 
> > If CONFIG_DEBUG_KOBJECT_RELEASE is enabled then the deletion of the
> > kobject can occur at some random delay after the last reference is
> > removed via a workqueue,  
> 
> similarly if someone defers unregistration, this situation might occur.
> Again this is a very acute corner case.
> I still think right way is to use device by holding its reference rather
> than assuming someone else holding the reference of device.

Again, we're under no obligation from the caller to hold this
reference at all.  If they don't have a device reference, they're
already broken.  We hold that reference for our own benefit.  We
trigger a uevent not on our reference to the device, but the one
provided by the caller that we found to match our reference.

Here's an analogy just for fun (no analogy is perfect), a parent (vendor
driver) presents their child (parent device) to register them for
summer camp (mdev).  The camp director (mdev-core) registers the child
in the roster and later additional references are made to reserve the
child for various activities.  The parent later comes in and presents
the child again to be unregistered.  The camp director locks the
database, removes the child from the activity references, but before
pressing the button to remove the child from the roster says "I need to
tell you that the child is unregistered before I click this button
because they might not exist after I do..."  I think that's essentially
what your concern is asking us to do.  Thanks,

Alex