[PATCH] mdev: Send uevents around parent device registration

[PATCH] 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>
---
 drivers/vfio/mdev/mdev_core.c |   10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
index ae23151442cb..ecec2a3b13cb 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)
@@ -196,7 +198,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
 	list_add(&parent->next, &parent_list);
 	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 +223,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);
@@ -228,7 +233,6 @@ void mdev_unregister_device(struct device *dev)
 		mutex_unlock(&parent_list_lock);
 		return;
 	}
-	dev_info(dev, "MDEV: Unregistering\n");
 
 	list_del(&parent->next);
 	mutex_unlock(&parent_list_lock);
@@ -243,6 +247,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] mdev: Send uevents around parent device registration

[Kirti Wankhede]

On 6/26/2019 7:57 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>
> ---
>  drivers/vfio/mdev/mdev_core.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index ae23151442cb..ecec2a3b13cb 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)
> @@ -196,7 +198,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
>  	list_add(&parent->next, &parent_list);
>  	mutex_unlock(&parent_list_lock);
>  
> -	dev_info(dev, "MDEV: Registered\n");
> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> +

Its good to have udev event, but don't remove debug print from dmesg.
Same for unregister.

Thanks,
Kirti


>  	return 0;
>  
>  add_dev_err:
> @@ -220,6 +223,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);
> @@ -228,7 +233,6 @@ void mdev_unregister_device(struct device *dev)
>  		mutex_unlock(&parent_list_lock);
>  		return;
>  	}
> -	dev_info(dev, "MDEV: Unregistering\n");
>  
>  	list_del(&parent->next);
>  	mutex_unlock(&parent_list_lock);
> @@ -243,6 +247,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] mdev: Send uevents around parent device registration

[Alex Williamson]

On Wed, 26 Jun 2019 23:23:00 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 6/26/2019 7:57 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>
> > ---
> >  drivers/vfio/mdev/mdev_core.c |   10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> > index ae23151442cb..ecec2a3b13cb 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)
> > @@ -196,7 +198,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
> >  	list_add(&parent->next, &parent_list);
> >  	mutex_unlock(&parent_list_lock);
> >  
> > -	dev_info(dev, "MDEV: Registered\n");
> > +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> > +  
> 
> Its good to have udev event, but don't remove debug print from dmesg.
> Same for unregister.

Who consumes these?  They seem noisy.  Thanks,

Alex

> >  	return 0;
> >  
> >  add_dev_err:
> > @@ -220,6 +223,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);
> > @@ -228,7 +233,6 @@ void mdev_unregister_device(struct device *dev)
> >  		mutex_unlock(&parent_list_lock);
> >  		return;
> >  	}
> > -	dev_info(dev, "MDEV: Unregistering\n");
> >  
> >  	list_del(&parent->next);
> >  	mutex_unlock(&parent_list_lock);
> > @@ -243,6 +247,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] mdev: Send uevents around parent device registration

[Kirti Wankhede]

On 6/26/2019 11:35 PM, Alex Williamson wrote:
> On Wed, 26 Jun 2019 23:23:00 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 6/26/2019 7:57 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>
>>> ---
>>>  drivers/vfio/mdev/mdev_core.c |   10 ++++++++--
>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
>>> index ae23151442cb..ecec2a3b13cb 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)
>>> @@ -196,7 +198,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
>>>  	list_add(&parent->next, &parent_list);
>>>  	mutex_unlock(&parent_list_lock);
>>>  
>>> -	dev_info(dev, "MDEV: Registered\n");
>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
>>> +  
>>
>> Its good to have udev event, but don't remove debug print from dmesg.
>> Same for unregister.
> 
> Who consumes these?  They seem noisy.  Thanks,
> 

I don't think its noisy, its more of logging purpose. This is seen in
kernel log only when physical device is registered to mdev.

Thanks,
Kirti


> Alex
> 
>>>  	return 0;
>>>  
>>>  add_dev_err:
>>> @@ -220,6 +223,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);
>>> @@ -228,7 +233,6 @@ void mdev_unregister_device(struct device *dev)
>>>  		mutex_unlock(&parent_list_lock);
>>>  		return;
>>>  	}
>>> -	dev_info(dev, "MDEV: Unregistering\n");
>>>  
>>>  	list_del(&parent->next);
>>>  	mutex_unlock(&parent_list_lock);
>>> @@ -243,6 +247,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] mdev: Send uevents around parent device registration

[Cornelia Huck]

On Wed, 26 Jun 2019 08:27:58 -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>
> ---
>  drivers/vfio/mdev/mdev_core.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> index ae23151442cb..ecec2a3b13cb 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";

This string is probably reasonable enough.

> +	char *envp[] = { env_string, NULL };
>  
>  	/* check for mandatory ops */
>  	if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
> @@ -196,7 +198,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
>  	list_add(&parent->next, &parent_list);
>  	mutex_unlock(&parent_list_lock);
>  
> -	dev_info(dev, "MDEV: Registered\n");
> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);

I also agree with the positioning here.

> +
>  	return 0;
>  
>  add_dev_err:
> @@ -220,6 +223,8 @@ EXPORT_SYMBOL(mdev_register_device);
>  void mdev_unregister_device(struct device *dev)
>  {
>  	struct mdev_parent *parent;
> +	char *env_string = "MDEV_STATE=unregistered";

Ok.

> +	char *envp[] = { env_string, NULL };
>  
>  	mutex_lock(&parent_list_lock);
>  	parent = __find_parent_device(dev);
> @@ -228,7 +233,6 @@ void mdev_unregister_device(struct device *dev)
>  		mutex_unlock(&parent_list_lock);
>  		return;
>  	}
> -	dev_info(dev, "MDEV: Unregistering\n");
>  
>  	list_del(&parent->next);
>  	mutex_unlock(&parent_list_lock);
> @@ -243,6 +247,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);

I'm wondering whether we should indicate this uevent earlier: Once we
have detached from the parent list, we're basically done for all
practical purposes. So maybe move this right before we grab the
unreg_sem?

>  }
>  EXPORT_SYMBOL(mdev_unregister_device);
>  
> 

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

[Cornelia Huck]

On Thu, 27 Jun 2019 00:33:59 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 6/26/2019 11:35 PM, Alex Williamson wrote:
> > On Wed, 26 Jun 2019 23:23:00 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >   
> >> On 6/26/2019 7:57 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>
> >>> ---
> >>>  drivers/vfio/mdev/mdev_core.c |   10 ++++++++--
> >>>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> >>> index ae23151442cb..ecec2a3b13cb 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)
> >>> @@ -196,7 +198,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
> >>>  	list_add(&parent->next, &parent_list);
> >>>  	mutex_unlock(&parent_list_lock);
> >>>  
> >>> -	dev_info(dev, "MDEV: Registered\n");
> >>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> >>> +    
> >>
> >> Its good to have udev event, but don't remove debug print from dmesg.
> >> Same for unregister.  
> > 
> > Who consumes these?  They seem noisy.  Thanks,
> >   
> 
> I don't think its noisy, its more of logging purpose. This is seen in
> kernel log only when physical device is registered to mdev.

Yes; but why do you want to log success? If you need to log it
somewhere, wouldn't a trace event be a much better choice?

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

[Kirti Wankhede]

On 6/27/2019 1:51 PM, Cornelia Huck wrote:
> On Thu, 27 Jun 2019 00:33:59 +0530
> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> 
>> On 6/26/2019 11:35 PM, Alex Williamson wrote:
>>> On Wed, 26 Jun 2019 23:23:00 +0530
>>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
>>>   
>>>> On 6/26/2019 7:57 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>
>>>>> ---
>>>>>  drivers/vfio/mdev/mdev_core.c |   10 ++++++++--
>>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
>>>>> index ae23151442cb..ecec2a3b13cb 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)
>>>>> @@ -196,7 +198,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
>>>>>  	list_add(&parent->next, &parent_list);
>>>>>  	mutex_unlock(&parent_list_lock);
>>>>>  
>>>>> -	dev_info(dev, "MDEV: Registered\n");
>>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
>>>>> +    
>>>>
>>>> Its good to have udev event, but don't remove debug print from dmesg.
>>>> Same for unregister.  
>>>
>>> Who consumes these?  They seem noisy.  Thanks,
>>>   
>>
>> I don't think its noisy, its more of logging purpose. This is seen in
>> kernel log only when physical device is registered to mdev.
> 
> Yes; but why do you want to log success? If you need to log it
> somewhere, wouldn't a trace event be a much better choice?
> 

Trace events are not always collected in production environment, there
kernel log helps.

Thanks,
Kirti

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

[Alex Williamson]

On Thu, 27 Jun 2019 10:19:14 +0200
Cornelia Huck <cohuck@redhat.com> wrote:

> On Wed, 26 Jun 2019 08:27:58 -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>
> > ---
> >  drivers/vfio/mdev/mdev_core.c |   10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> > index ae23151442cb..ecec2a3b13cb 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";  
> 
> This string is probably reasonable enough.
> 
> > +	char *envp[] = { env_string, NULL };
> >  
> >  	/* check for mandatory ops */
> >  	if (!ops || !ops->create || !ops->remove || !ops->supported_type_groups)
> > @@ -196,7 +198,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
> >  	list_add(&parent->next, &parent_list);
> >  	mutex_unlock(&parent_list_lock);
> >  
> > -	dev_info(dev, "MDEV: Registered\n");
> > +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);  
> 
> I also agree with the positioning here.
> 
> > +
> >  	return 0;
> >  
> >  add_dev_err:
> > @@ -220,6 +223,8 @@ EXPORT_SYMBOL(mdev_register_device);
> >  void mdev_unregister_device(struct device *dev)
> >  {
> >  	struct mdev_parent *parent;
> > +	char *env_string = "MDEV_STATE=unregistered";  
> 
> Ok.
> 
> > +	char *envp[] = { env_string, NULL };
> >  
> >  	mutex_lock(&parent_list_lock);
> >  	parent = __find_parent_device(dev);
> > @@ -228,7 +233,6 @@ void mdev_unregister_device(struct device *dev)
> >  		mutex_unlock(&parent_list_lock);
> >  		return;
> >  	}
> > -	dev_info(dev, "MDEV: Unregistering\n");
> >  
> >  	list_del(&parent->next);
> >  	mutex_unlock(&parent_list_lock);
> > @@ -243,6 +247,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);  
> 
> I'm wondering whether we should indicate this uevent earlier: Once we
> have detached from the parent list, we're basically done for all
> practical purposes. So maybe move this right before we grab the
> unreg_sem?

That would make it a "this thing is about to go away" (ie.
"unregistering") rather than "this thing is gone" ("unregistered").  I
was aiming for the latter as the former just seems like it might make
userspace race to remove devices.  Note that I don't actually make use
of this event in mdevctl currently, so we could maybe save it for
later, but the symmetry seemed preferable.  Thanks,

Alex

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

[Cornelia Huck]

On Fri, 28 Jun 2019 09:56:08 -0600
Alex Williamson <alex.williamson@redhat.com> wrote:

> On Thu, 27 Jun 2019 10:19:14 +0200
> Cornelia Huck <cohuck@redhat.com> wrote:
> 
> > On Wed, 26 Jun 2019 08:27:58 -0600
> > Alex Williamson <alex.williamson@redhat.com> wrote:

> > > @@ -243,6 +247,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);    
> > 
> > I'm wondering whether we should indicate this uevent earlier: Once we
> > have detached from the parent list, we're basically done for all
> > practical purposes. So maybe move this right before we grab the
> > unreg_sem?  
> 
> That would make it a "this thing is about to go away" (ie.
> "unregistering") rather than "this thing is gone" ("unregistered").  I
> was aiming for the latter as the former just seems like it might make
> userspace race to remove devices.  Note that I don't actually make use
> of this event in mdevctl currently, so we could maybe save it for
> later, but the symmetry seemed preferable.  Thanks,
> 
> Alex

Fair enough. I was thinking about signaling that it does not make much
sense to register new devices after that point, but if that might
trigger userspace to actually try and remove devices, not much is
gained.

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

[Cornelia Huck]

On Thu, 27 Jun 2019 19:42:32 +0530
Kirti Wankhede <kwankhede@nvidia.com> wrote:

> On 6/27/2019 1:51 PM, Cornelia Huck wrote:
> > On Thu, 27 Jun 2019 00:33:59 +0530
> > Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >   
> >> On 6/26/2019 11:35 PM, Alex Williamson wrote:  
> >>> On Wed, 26 Jun 2019 23:23:00 +0530
> >>> Kirti Wankhede <kwankhede@nvidia.com> wrote:
> >>>     
> >>>> On 6/26/2019 7:57 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>
> >>>>> ---
> >>>>>  drivers/vfio/mdev/mdev_core.c |   10 ++++++++--
> >>>>>  1 file changed, 8 insertions(+), 2 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/vfio/mdev/mdev_core.c b/drivers/vfio/mdev/mdev_core.c
> >>>>> index ae23151442cb..ecec2a3b13cb 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)
> >>>>> @@ -196,7 +198,8 @@ int mdev_register_device(struct device *dev, const struct mdev_parent_ops *ops)
> >>>>>  	list_add(&parent->next, &parent_list);
> >>>>>  	mutex_unlock(&parent_list_lock);
> >>>>>  
> >>>>> -	dev_info(dev, "MDEV: Registered\n");
> >>>>> +	kobject_uevent_env(&dev->kobj, KOBJ_CHANGE, envp);
> >>>>> +      
> >>>>
> >>>> Its good to have udev event, but don't remove debug print from dmesg.
> >>>> Same for unregister.    
> >>>
> >>> Who consumes these?  They seem noisy.  Thanks,
> >>>     
> >>
> >> I don't think its noisy, its more of logging purpose. This is seen in
> >> kernel log only when physical device is registered to mdev.  
> > 
> > Yes; but why do you want to log success? If you need to log it
> > somewhere, wouldn't a trace event be a much better choice?
> >   
> 
> Trace events are not always collected in production environment, there
> kernel log helps.

I'm with you for *errors*, but I'm not sure you should rely on
*success* messages, though. If you want to be able to figure out the
sequence of registering etc. in all cases, I think it makes more sense
to invest in an infrastructure like tracing and make sure that is it
turned on for any system that matters.

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

[Cornelia Huck]

On Wed, 26 Jun 2019 08:27:58 -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>
> ---
>  drivers/vfio/mdev/mdev_core.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)

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