Re: [PATCH] xen: fix frontend driver disconnected from xenbus on removal

[Boris Ostrovsky <boris.ostrovsky@oracle.com>]

On 02/01/2018 03:24 PM, Oleksandr Andrushchenko wrote:
>
>
> On 02/01/2018 10:08 PM, Boris Ostrovsky wrote:
>> On 02/01/2018 03:57 AM, Oleksandr Andrushchenko wrote:
>>> From: Oleksandr Andrushchenko <oleksandr_andrushchenko@epam.com>
>>>
>>> Current xenbus frontend driver removal flow first disconnects
>>> the driver from xenbus and then calls driver's remove callback.
>>> This makes it impossible for the driver to listen to backend's
>>> state changes and synchronize the removal procedure.
>>>
>>> Fix this by removing other end XenBus watches after the
>>> driver's remove callback is called.
>>>
>>> Signed-off-by: Oleksandr Andrushchenko
>>> <oleksandr_andrushchenko@epam.com>
>>> ---
>>>   drivers/xen/xenbus/xenbus_probe.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/xen/xenbus/xenbus_probe.c
>>> b/drivers/xen/xenbus/xenbus_probe.c
>>> index 74888cacd0b0..9c63cd3f416b 100644
>>> --- a/drivers/xen/xenbus/xenbus_probe.c
>>> +++ b/drivers/xen/xenbus/xenbus_probe.c
>>> @@ -258,11 +258,11 @@ int xenbus_dev_remove(struct device *_dev)
>>>         DPRINTK("%s", dev->nodename);
>>>   -    free_otherend_watch(dev);
>>> -
>>>       if (drv->remove)
>>>           drv->remove(dev);
>>
>> Is it possible for the watch to fire here?
> Indeed. Yes, It is possible, so we have to somehow protect the removed
> driver from being called, e.g. the driver cleans up in its .remove,
> but watch may still trigger .otherend_changed callback.
> Is this what you mean?

(-David who is not at Citrix anymore)

Exactly.

That's why otherend cleanup is split into free_otherend_watch() and
free_otherend_details().


> If so, do you have something neat on your mind how to solve this?

Not necessarily "neat" but perhaps you can use
xenbus_read_otherend_details() in both front and back ends. After all,
IIUIC you are doing something synchronously so you don't really need a
watch.

-boris

>
>> -boris
>>
>>>   +    free_otherend_watch(dev);
>>> +
>>>       free_otherend_details(dev);
>>>         xenbus_switch_state(dev, XenbusStateClosed);
> Thank you,
> Oleksandr