On Fri, 2023-10-20 at 10:51 +0200, Juergen Gross wrote:
>
> > (qemu) device_del con1
> > [ 32.050919] ------------[ cut here ]------------
> > [ 32.050942] Trying to free already-free IRQ 33
> > [ 32.050990] WARNING: CPU: 0 PID: 51 at kernel/irq/manage.c:1895 __free_irq+0x1d4/0x330
> >
> > Fix that by calling notifier_del_irq() first, which only calls
> > free_irq() if the irq was requested in the first place. Then use
>
> I don't think the "if the irq was requested in the first place" is the correct
> reasoning.
>
> I think the problem is that notifier_del_irq() will be called another time
> through the .notifier_del hook. Two calls of notifier_del_irq() are fine, but
> one call of it and another call of free_irq() via unbind_from_irqhandler() is
> a problem.
Er... yes, the HVC tty device still exists, can still be open and in
use by userspace during the time that xencons_disconnect_backend() is
running.
Why does xencons_disconnect_backend() do all the evtchn and gnttab
teardown *first* and then only call hvc_remove() at the end. That seems
weird.
So if I do 'dd if=/dev/zero of=/dev/hvc1' while I remove the device
from qemu... yep, that seems to have filled the ring after the evtchn
was torn down, and it's waiting for ever in domU_write_console().
In fact, that isn't *even* because of the race within
xencons_disconnect_backend(). In xencons_backend_changed() when the
backend goes into state XenbusStateClos{ing,ed} for the disconnect, we
just set the frontend state directly to XenbusStateClosed without even
*calling* xencons_disconnect_backend().
So we were *told* of the impending unplug. We fail to actually stop
using the device, but we tell the backend that it's OK to go away.
Oops :)
The incremental patch below makes it work if I unplug a console while
writing /dev/zero to it.
But I suspect instead of calling xencons_disconnect_backend() when the
backend changes to XenbusStateClos{ing,ed}, it should actually *just*
do the hvc_close() part? The evtchn and gnttab cleanup should wait
until the backend has actually finished closing?
And what locking is there around xencons_disconnect_backend() anyway?
Do we rely on the fact that it can all only happen from the xenstore
watch thread?
diff --git a/drivers/tty/hvc/hvc_xen.c b/drivers/tty/hvc/hvc_xen.c
index f0376612b267..0806078835f6 100644
--- a/drivers/tty/hvc/hvc_xen.c
+++ b/drivers/tty/hvc/hvc_xen.c
@@ -377,8 +377,10 @@ void xen_console_resume(void)
#ifdef CONFIG_HVC_XEN_FRONTEND
static void xencons_disconnect_backend(struct xencons_info *info)
{
+ if (info->hvc != NULL)
+ hvc_remove(info->hvc);
+ info->hvc = NULL;
if (info->irq > 0) {
- notifier_del_irq(info->hvc, info->irq);
evtchn_put(info->evtchn);
info->irq = 0;
info->evtchn = 0;
@@ -390,9 +392,6 @@ static void xencons_disconnect_backend(struct xencons_info *info)
if (info->gntref > 0)
gnttab_free_grant_references(info->gntref);
info->gntref = 0;
- if (info->hvc != NULL)
- hvc_remove(info->hvc);
- info->hvc = NULL;
}
static void xencons_free(struct xencons_info *info)
@@ -558,6 +557,7 @@ static void xencons_backend_changed(struct xenbus_device *dev,
break;
fallthrough; /* Missed the backend's CLOSING state */
case XenbusStateClosing:
+ xencons_disconnect_backend(dev_get_drvdata(&dev->dev));
xenbus_frontend_closed(dev);
break;
}
Attachment:
smime.p7s
Description: S/MIME cryptographic signature
