On Tue, 2 Jul 2019 10:25:04 +0530
Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
> On 7/2/2019 1:34 AM, Alex Williamson wrote:
> > On Mon, 1 Jul 2019 23:20:35 +0530
> > Kirti Wankhede <kwankhede@xxxxxxxxxx> wrote:
> >
> >> On 7/1/2019 10:54 PM, Alex Williamson wrote:
> >>> On Mon, 1 Jul 2019 22:43:10 +0530
> >>> Kirti Wankhede <kwankhede@xxxxxxxxxx> 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@xxxxxxxxxx>
> >>>>> ---
> >>>>>
> >>>>> 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.
