hvc_console: Fix race between hvc_close and hvc_remove
in [Kernel]
|
Prev: [PATCH -v2 0/2] ACPI, APEI, use general HEST table parsing in AER firmware_first setup
Next: [PATCH] mm: Fix some coding styles on mm/ tree
From: Amit Shah on 25 Mar 2010 22:10 On (Fri) Mar 26 2010 [10:30:31], Anton Blanchard wrote: > > Hi, > > > And this suggests that hvc_kick() is called before hvc_task is > > initialised, ie, before hvc_init() is called. > > > > Does this help? > > Looks good, tests OK on my POWER5 box. Thanks! > > Tested-by: Anton Blanchard <anton(a)samba.org> Thanks. Before I submit this patch, though, I'd like to see why hvc_kick() gets called before hvc_init. If I can get to it real quick, I'll do it, else I'll just queue this one up. > > diff --git a/drivers/char/hvc_console.c b/drivers/char/hvc_console.c > > index ba55bba..50ac983 100644 > > --- a/drivers/char/hvc_console.c > > +++ b/drivers/char/hvc_console.c > > @@ -285,6 +285,9 @@ EXPORT_SYMBOL_GPL(hvc_instantiate); > > /* Wake the sleeping khvcd */ > > void hvc_kick(void) > > { > > + if (!hvc_task) > > + return; > > + > > hvc_kicked = 1; > > wake_up_process(hvc_task); > > } Amit -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Amit Shah on 26 Mar 2010 06:10 On (Fri) Mar 26 2010 [14:43:56], Sachin Sant wrote: >> And this suggests that hvc_kick() is called before hvc_task is >> initialised, ie, before hvc_init() is called. >> >> Does this help? >> > Hi Amit, > > I too ran into this issue as reported by Anton. Unfortunately in my > case the following suggested patch does not help boot the powerpc > box successfully. The boot still hangs with following messages Was the hang you saw before applying this patch at the same place / for the same reason? > /dev/sda5: clean, 229739/2097152 files, 2485598/8377888 blocks (check in 4 mounts) > fsck succeeded. Mounting root device read-write. > Mounting root /dev/sda5 > mount -o rw,acl,user_xattr -t ext3 /dev/sda5 /root > kjournald starting. Commit interval 5 seconds > EXT3-fs (sda5): using internal journal > EXT3-fs (sda5): mounted filesystem with writeback data mode > mount: can't find /root/proc in /etc/fstab or /etc/mtab > /sbin/smart_agetty: can not determine 'console' speed OK, this gets even murkier. I don't know why taking a few krefs modifies hvc_console behaviour to such an extent. > Just to confirm i too am facing same issue as reported by Anton, > reverted commit e74d098c66543d0731de62eb747ccd5b636a6f4c and that allowed > by box to boot successfully. Which module uses hvc_console in your case? Amit -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Amit Shah on 26 Mar 2010 08:50 On (Fri) Mar 26 2010 [17:12:22], Sachin Sant wrote: > Amit Shah wrote: >> On (Fri) Mar 26 2010 [14:43:56], Sachin Sant wrote: >> >>>> And this suggests that hvc_kick() is called before hvc_task is >>>> initialised, ie, before hvc_init() is called. >>>> >>>> Does this help? >>>> >>> Hi Amit, >>> >>> I too ran into this issue as reported by Anton. Unfortunately in my >>> case the following suggested patch does not help boot the powerpc >>> box successfully. The boot still hangs with following messages >>> >> >> Was the hang you saw before applying this patch at the same place / for >> the same reason? >> > Yes the machine hangs at the same place with or without the patch. OK. >>> mount: can't find /root/proc in /etc/fstab or /etc/mtab >>> /sbin/smart_agetty: can not determine 'console' speed >>> >> >> OK, this gets even murkier. I don't know why taking a few krefs modifies >> hvc_console behaviour to such an extent. >> > Looking at the commit e74d098c66543d0731de62eb747ccd5b636a6f4c, > i see that for every tty_kref_get() there is a corresponding > tty_kref_put() except maybe for the one in the following patch snippet > > spin_lock_irqsave(&hp->lock, flags); > /* Check and then increment for fast path open. */ > if (hp->count++ > 0) { > + tty_kref_get(tty); > spin_unlock_irqrestore(&hp->lock, flags); > hvc_kick(); > return 0; > > I don't know this code very well but we might be missing a > corresponding tty_kref_put() some place ? See hvc_hangup: temp_open_count = hp->count; ... while(temp_open_count) { --temp_open_count; tty_kref_put(tty); kref_put(&hp->kref, destroy_hvc_struct); } > If i comment out the above tty_kref_get() call, the machine > boots fine with or without the suggested patch. Hm, so as Alan suggests, it could be a timing issue in getting the ref and returning back from hvc_open(). I don't know how the tty layer works, but delays in getting the open() succeed results in badness? Guess we'll have to poke at the other drivers to see how they handle open() calls. Amit -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Amit Shah on 26 Mar 2010 09:00 On (Fri) Mar 26 2010 [11:52:33], Alan Cox wrote: > > >> mount: can't find /root/proc in /etc/fstab or /etc/mtab > > >> /sbin/smart_agetty: can not determine 'console' speed > > >> > > > > > > OK, this gets even murkier. I don't know why taking a few krefs modifies > > > hvc_console behaviour to such an extent. > > Can you see if the agetty is getting a hangup or open failing. I wonder > if its just a timing change ? Can someone please try this? Looks like it only reproduces on ppc, I can't get it to reproduce in my kvm vm. I'm trying to get hold of a ppc box, though. (btw ppc, ppc64, both should trigger this, right?) Amit -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
From: Amit Shah on 6 Apr 2010 08:20
On (Tue) Apr 06 2010 [21:42:38], Anton Blanchard wrote: > > Hi, > > > > Looking at the commit e74d098c66543d0731de62eb747ccd5b636a6f4c, > > > i see that for every tty_kref_get() there is a corresponding > > > tty_kref_put() except maybe for the one in the following patch snippet > > > > > > spin_lock_irqsave(&hp->lock, flags); > > > /* Check and then increment for fast path open. */ > > > if (hp->count++ > 0) { > > > + tty_kref_get(tty); > > > spin_unlock_irqrestore(&hp->lock, flags); > > > hvc_kick(); > > > return 0; > > > > > > I don't know this code very well but we might be missing a > > > corresponding tty_kref_put() some place ? > > > > See hvc_hangup: > > > > temp_open_count = hp->count; > > ... > > while(temp_open_count) { > > --temp_open_count; > > tty_kref_put(tty); > > kref_put(&hp->kref, destroy_hvc_struct); > > } > > I don't claim to understand the tty layer, but it seems like hvc_open and > hvc_close should be balanced in their kref reference counting. > > Right now we get a kref every call to hvc_open: > > if (hp->count++ > 0) { > tty_kref_get(tty); <----- here > spin_unlock_irqrestore(&hp->lock, flags); > hvc_kick(); > return 0; > } /* else count == 0 */ > > tty->driver_data = hp; > > hp->tty = tty_kref_get(tty); <------ or here if hp->count was 0 > > But hvc_close has: > > tty_kref_get(tty); > > if (--hp->count == 0) { > ... > /* Put the ref obtained in hvc_open() */ > tty_kref_put(tty); > ... > } > > tty_kref_put(tty); > > Since the outside kref get/put balance we only do a single kref_put when > count reaches 0. OK, makes sense and since it works for you, Acked-by: Amit Shah <amit.shah(a)redhat.com> > The patch below changes things to call tty_kref_put once for every > hvc_close call, and with that my machine boots fine. > > Signed-off-by: Anton Blanchard <anton(a)samba.org> -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo(a)vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/ |