Messages in this thread | | | Subject | Re: [PATCH] xen: fix frontend driver disconnected from xenbus on removal | From | Oleksandr Andrushchenko <> | Date | Fri, 2 Feb 2018 09:01:25 +0200 |
| |
On 02/01/2018 11:09 PM, Boris Ostrovsky wrote: > 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(). Understood, thank you Confusion came because of the patch [1]: in .remove we wait for the backend to change its states in .otherend_changed callback and wake us, but I am not sure how those state changes may occur if during .remove the driver has already watches freed. So, this is why I tried to play around with free_otherend_watch()... > >> 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. Yes, I will implement a dedicated flow in the .remove instead of relying on .otherend_changed > -boris > >>> -boris >>> >>>> + free_otherend_watch(dev); >>>> + >>>> free_otherend_details(dev); >>>> xenbus_switch_state(dev, XenbusStateClosed); >> Thank you, >> Oleksandr Thank you, Oleksandr
[1] https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/xen-netfront.c?id=5b5971df3bc2775107ddad164018a8a8db633b81
| |