* [PATCH] IDE: Unregister devices if initialization fails. @ 2009-01-23 13:04 Ian Campbell 2009-01-27 16:41 ` Bartlomiej Zolnierkiewicz 0 siblings, 1 reply; 6+ messages in thread From: Ian Campbell @ 2009-01-23 13:04 UTC (permalink / raw) To: linux-ide; +Cc: Ian Campbell, Ian Campbell, Bartlomiej Zolnierkiewicz On reboot the loop in device_shutdown gets confused by these devices and goes into an infinite loop. I think this is because they don't have dev->bus or dev->driver which is because hwif_register_devices doesn't get called if !hwif->present. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> --- drivers/ide/ide-probe.c | 10 ++++++---- 1 files changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c index 312127e..120f65b 100644 --- a/drivers/ide/ide-probe.c +++ b/drivers/ide/ide-probe.c @@ -1520,6 +1520,8 @@ int ide_host_register(struct ide_host *host, const struct ide_port_info *d, if (hwif_init(hwif) == 0) { printk(KERN_INFO "%s: failed to initialize IDE " "interface\n", hwif->name); + device_unregister(&hwif->gendev); + device_destroy(ide_port_class, MKDEV(0, 0)); hwif->present = 0; continue; } @@ -1550,11 +1552,11 @@ int ide_host_register(struct ide_host *host, const struct ide_port_info *d, if (hwif == NULL) continue; - ide_sysfs_register_port(hwif); - ide_proc_register_port(hwif); - - if (hwif->present) + if (hwif->present) { + ide_sysfs_register_port(hwif); + ide_proc_register_port(hwif); ide_proc_port_register_devices(hwif); + } } return j ? 0 : -1; -- 1.5.6.5 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] IDE: Unregister devices if initialization fails. 2009-01-23 13:04 [PATCH] IDE: Unregister devices if initialization fails Ian Campbell @ 2009-01-27 16:41 ` Bartlomiej Zolnierkiewicz 2009-01-30 17:19 ` Ian Campbell 0 siblings, 1 reply; 6+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2009-01-27 16:41 UTC (permalink / raw) To: Ian Campbell; +Cc: linux-ide Hi, On Friday 23 January 2009, Ian Campbell wrote: > On reboot the loop in device_shutdown gets confused by these devices > and goes into an infinite loop. I think this is because they don't > have dev->bus or dev->driver which is because hwif_register_devices > doesn't get called if !hwif->present. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > --- > drivers/ide/ide-probe.c | 10 ++++++---- > 1 files changed, 6 insertions(+), 4 deletions(-) > > diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c > index 312127e..120f65b 100644 > --- a/drivers/ide/ide-probe.c > +++ b/drivers/ide/ide-probe.c > @@ -1520,6 +1520,8 @@ int ide_host_register(struct ide_host *host, const struct ide_port_info *d, > if (hwif_init(hwif) == 0) { > printk(KERN_INFO "%s: failed to initialize IDE " > "interface\n", hwif->name); > + device_unregister(&hwif->gendev); > + device_destroy(ide_port_class, MKDEV(0, 0)); > hwif->present = 0; I've been wondering about this a bit and I think that we should just nuke port from struct ide_host in such situation so I added ide_disable_port() helper for this (+ fixed ide_register_port() handling while at it). Could you please re-base your fix on top of attached patch? Please also remove superflous device_destroy() call -- device_unregister() should be enough and if it is not we need to fix ide_register_port() to use unique dev_t instead of MKDEV(0, 0). > continue; > } > @@ -1550,11 +1552,11 @@ int ide_host_register(struct ide_host *host, const struct ide_port_info *d, > if (hwif == NULL) > continue; > > - ide_sysfs_register_port(hwif); > - ide_proc_register_port(hwif); > - > - if (hwif->present) > + if (hwif->present) { > + ide_sysfs_register_port(hwif); > + ide_proc_register_port(hwif); > ide_proc_port_register_devices(hwif); This code have to stay unchanged for proper warm-plug support (luckily after re-basing this chunk shouldn't be needed). Here is the ide_disable_port() patch: From: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> Subject: [PATCH] ide: fix ide_register_port() failure handling * Factor out port freeing from ide_host_free() to ide_free_port(). * Add ide_disable_port() and use it on ide_register_port() failure. Cc: Ian Campbell <Ian.Campbell@citrix.com> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> --- drivers/ide/ide-probe.c | 40 ++++++++++++++++++++++++++++++++-------- 1 file changed, 32 insertions(+), 8 deletions(-) Index: b/drivers/ide/ide-probe.c =================================================================== --- a/drivers/ide/ide-probe.c +++ b/drivers/ide/ide-probe.c @@ -1466,6 +1466,30 @@ struct ide_host *ide_host_alloc(const st } EXPORT_SYMBOL_GPL(ide_host_alloc); +static void ide_port_free(ide_hwif_t *hwif) +{ + ide_port_free_devices(hwif); + ide_free_port_slot(hwif->index); + kfree(hwif); +} + +static void ide_disable_port(ide_hwif_t *hwif) +{ + struct ide_host *host = hwif->host; + int i; + + printk(KERN_INFO "%s: disabling port\n", hwif->name); + + for (i = 0; i < MAX_HOST_PORTS; i++) { + if (host->ports[i] == hwif) { + host->ports[i] = NULL; + host->n_ports--; + } + } + + ide_port_free(hwif); +} + int ide_host_register(struct ide_host *host, const struct ide_port_info *d, hw_regs_t **hws) { @@ -1506,8 +1530,12 @@ int ide_host_register(struct ide_host *h hwif->present = 1; if (hwif->chipset != ide_4drives || !hwif->mate || - !hwif->mate->present) - ide_register_port(hwif); + !hwif->mate->present) { + if (ide_register_port(hwif)) { + ide_disable_port(hwif); + continue; + } + } if (hwif->present) ide_port_tune_devices(hwif); @@ -1659,12 +1687,8 @@ void ide_host_free(struct ide_host *host int i; ide_host_for_each_port(i, hwif, host) { - if (hwif == NULL) - continue; - - ide_port_free_devices(hwif); - ide_free_port_slot(hwif->index); - kfree(hwif); + if (hwif) + ide_port_free(hwif); } kfree(host); ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] IDE: Unregister devices if initialization fails. 2009-01-27 16:41 ` Bartlomiej Zolnierkiewicz @ 2009-01-30 17:19 ` Ian Campbell 2009-02-01 16:21 ` Bartlomiej Zolnierkiewicz 0 siblings, 1 reply; 6+ messages in thread From: Ian Campbell @ 2009-01-30 17:19 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz; +Cc: linux-ide On Tue, 2009-01-27 at 17:41 +0100, Bartlomiej Zolnierkiewicz wrote: > Hi, > > On Friday 23 January 2009, Ian Campbell wrote: > > On reboot the loop in device_shutdown gets confused by these devices > > and goes into an infinite loop. I think this is because they don't > > have dev->bus or dev->driver which is because hwif_register_devices > > doesn't get called if !hwif->present. > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > > --- > > drivers/ide/ide-probe.c | 10 ++++++---- > > 1 files changed, 6 insertions(+), 4 deletions(-) > > > > diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c > > index 312127e..120f65b 100644 > > --- a/drivers/ide/ide-probe.c > > +++ b/drivers/ide/ide-probe.c > > @@ -1520,6 +1520,8 @@ int ide_host_register(struct ide_host *host, const struct ide_port_info *d, > > if (hwif_init(hwif) == 0) { > > printk(KERN_INFO "%s: failed to initialize IDE " > > "interface\n", hwif->name); > > + device_unregister(&hwif->gendev); > > + device_destroy(ide_port_class, MKDEV(0, 0)); > > hwif->present = 0; > > I've been wondering about this a bit and I think that we should just nuke > port from struct ide_host in such situation so I added ide_disable_port() > helper for this (+ fixed ide_register_port() handling while at it). > > Could you please re-base your fix on top of attached patch? Sure. Subject: IDE: Disable and unregister devices if initialization fails. On reboot the loop in device_shutdown gets confused by these partially initialized devices and goes into an infinite loop. Therefore disable and unregister these devices. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- drivers/ide/ide-probe.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff -r 12d1a7848ddf drivers/ide/ide-probe.c --- a/drivers/ide/ide-probe.c Fri Jan 30 16:37:15 2009 +0000 +++ b/drivers/ide/ide-probe.c Fri Jan 30 17:00:08 2009 +0000 @@ -1549,6 +1549,8 @@ if (hwif_init(hwif) == 0) { printk(KERN_INFO "%s: failed to initialize IDE " "interface\n", hwif->name); + ide_disable_port(hwif); + device_unregister(&hwif->gendev); hwif->present = 0; continue; } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] IDE: Unregister devices if initialization fails. 2009-01-30 17:19 ` Ian Campbell @ 2009-02-01 16:21 ` Bartlomiej Zolnierkiewicz 2009-02-02 10:09 ` Ian Campbell 0 siblings, 1 reply; 6+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2009-02-01 16:21 UTC (permalink / raw) To: Ian Campbell; +Cc: linux-ide On Friday 30 January 2009, Ian Campbell wrote: > On Tue, 2009-01-27 at 17:41 +0100, Bartlomiej Zolnierkiewicz wrote: > > Hi, > > > > On Friday 23 January 2009, Ian Campbell wrote: > > > On reboot the loop in device_shutdown gets confused by these devices > > > and goes into an infinite loop. I think this is because they don't > > > have dev->bus or dev->driver which is because hwif_register_devices > > > doesn't get called if !hwif->present. > > > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > > > --- > > > drivers/ide/ide-probe.c | 10 ++++++---- > > > 1 files changed, 6 insertions(+), 4 deletions(-) > > > > > > diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c > > > index 312127e..120f65b 100644 > > > --- a/drivers/ide/ide-probe.c > > > +++ b/drivers/ide/ide-probe.c > > > @@ -1520,6 +1520,8 @@ int ide_host_register(struct ide_host *host, const struct ide_port_info *d, > > > if (hwif_init(hwif) == 0) { > > > printk(KERN_INFO "%s: failed to initialize IDE " > > > "interface\n", hwif->name); > > > + device_unregister(&hwif->gendev); > > > + device_destroy(ide_port_class, MKDEV(0, 0)); > > > hwif->present = 0; > > > > I've been wondering about this a bit and I think that we should just nuke > > port from struct ide_host in such situation so I added ide_disable_port() > > helper for this (+ fixed ide_register_port() handling while at it). > > > > Could you please re-base your fix on top of attached patch? > > Sure. > > Subject: IDE: Disable and unregister devices if initialization fails. > > On reboot the loop in device_shutdown gets confused by these partially > initialized devices and goes into an infinite loop. Therefore disable > and unregister these devices. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > --- > drivers/ide/ide-probe.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > diff -r 12d1a7848ddf drivers/ide/ide-probe.c > --- a/drivers/ide/ide-probe.c Fri Jan 30 16:37:15 2009 +0000 > +++ b/drivers/ide/ide-probe.c Fri Jan 30 17:00:08 2009 +0000 > @@ -1549,6 +1549,8 @@ > if (hwif_init(hwif) == 0) { > printk(KERN_INFO "%s: failed to initialize IDE " > "interface\n", hwif->name); > + ide_disable_port(hwif); > + device_unregister(&hwif->gendev); > hwif->present = 0; > continue; Hmmm, ide_disable_port() may free hwif so it needs to happen last... Also did you have a chance to retest this? [ I couldn't reproduce the issue so was hoping that you could test combined fixes. ] Thanks, Bart ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] IDE: Unregister devices if initialization fails. 2009-02-01 16:21 ` Bartlomiej Zolnierkiewicz @ 2009-02-02 10:09 ` Ian Campbell 2009-02-02 17:49 ` Bartlomiej Zolnierkiewicz 0 siblings, 1 reply; 6+ messages in thread From: Ian Campbell @ 2009-02-02 10:09 UTC (permalink / raw) To: Bartlomiej Zolnierkiewicz; +Cc: linux-ide On Sun, 2009-02-01 at 17:21 +0100, Bartlomiej Zolnierkiewicz wrote: > On Friday 30 January 2009, Ian Campbell wrote: > > On Tue, 2009-01-27 at 17:41 +0100, Bartlomiej Zolnierkiewicz wrote: > > > Hi, > > > > > > On Friday 23 January 2009, Ian Campbell wrote: > > > > On reboot the loop in device_shutdown gets confused by these devices > > > > and goes into an infinite loop. I think this is because they don't > > > > have dev->bus or dev->driver which is because hwif_register_devices > > > > doesn't get called if !hwif->present. > > > > > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > > > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > > > > --- > > > > drivers/ide/ide-probe.c | 10 ++++++---- > > > > 1 files changed, 6 insertions(+), 4 deletions(-) > > > > > > > > diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c > > > > index 312127e..120f65b 100644 > > > > --- a/drivers/ide/ide-probe.c > > > > +++ b/drivers/ide/ide-probe.c > > > > @@ -1520,6 +1520,8 @@ int ide_host_register(struct ide_host *host, const struct ide_port_info *d, > > > > if (hwif_init(hwif) == 0) { > > > > printk(KERN_INFO "%s: failed to initialize IDE " > > > > "interface\n", hwif->name); > > > > + device_unregister(&hwif->gendev); > > > > + device_destroy(ide_port_class, MKDEV(0, 0)); > > > > hwif->present = 0; > > > > > > I've been wondering about this a bit and I think that we should just nuke > > > port from struct ide_host in such situation so I added ide_disable_port() > > > helper for this (+ fixed ide_register_port() handling while at it). > > > > > > Could you please re-base your fix on top of attached patch? > > > > Sure. > > > > Subject: IDE: Disable and unregister devices if initialization fails. > > > > On reboot the loop in device_shutdown gets confused by these partially > > initialized devices and goes into an infinite loop. Therefore disable > > and unregister these devices. > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > > > --- > > drivers/ide/ide-probe.c | 10 ++++++---- > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > diff -r 12d1a7848ddf drivers/ide/ide-probe.c > > --- a/drivers/ide/ide-probe.c Fri Jan 30 16:37:15 2009 +0000 > > +++ b/drivers/ide/ide-probe.c Fri Jan 30 17:00:08 2009 +0000 > > @@ -1549,6 +1549,8 @@ > > if (hwif_init(hwif) == 0) { > > printk(KERN_INFO "%s: failed to initialize IDE " > > "interface\n", hwif->name); > > + ide_disable_port(hwif); > > + device_unregister(&hwif->gendev); > > hwif->present = 0; > > continue; > > Hmmm, ide_disable_port() may free hwif so it needs to happen last... Oops, new version below. > Also did you have a chance to retest this? [ I couldn't reproduce > the issue so was hoping that you could test combined fixes. ] Yep, I can easily repro. I've actually fixed the underlying issue (a Xen IRQ problem) causing the probe to fail but can easily revert that fix to test these patches. If you wanted to repro yourself you could probably make the request_irq() in init_irq() artificially fail. Ian. IDE: Disable and unregister devices if initialization fails. On reboot the loop in device_shutdown gets confused by these partially initialized devices and goes into an infinite loop. Therefore disable and unregister these devices. Signed-off-by: Ian Campbell <ian.campbell@citrix.com> --- drivers/ide/ide-probe.c | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff -r 9d5441419978 -r f596f17b02c3 drivers/ide/ide-probe.c --- a/drivers/ide/ide-probe.c Fri Jan 30 17:39:53 2009 +0000 +++ b/drivers/ide/ide-probe.c Mon Feb 02 10:02:32 2009 +0000 @@ -1549,6 +1549,8 @@ if (hwif_init(hwif) == 0) { printk(KERN_INFO "%s: failed to initialize IDE " "interface\n", hwif->name); + device_unregister(&hwif->gendev); + ide_disable_port(hwif); hwif->present = 0; continue; } ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] IDE: Unregister devices if initialization fails. 2009-02-02 10:09 ` Ian Campbell @ 2009-02-02 17:49 ` Bartlomiej Zolnierkiewicz 0 siblings, 0 replies; 6+ messages in thread From: Bartlomiej Zolnierkiewicz @ 2009-02-02 17:49 UTC (permalink / raw) To: Ian Campbell; +Cc: linux-ide On Monday 02 February 2009, Ian Campbell wrote: > On Sun, 2009-02-01 at 17:21 +0100, Bartlomiej Zolnierkiewicz wrote: > > On Friday 30 January 2009, Ian Campbell wrote: > > > On Tue, 2009-01-27 at 17:41 +0100, Bartlomiej Zolnierkiewicz wrote: > > > > Hi, > > > > > > > > On Friday 23 January 2009, Ian Campbell wrote: > > > > > On reboot the loop in device_shutdown gets confused by these devices > > > > > and goes into an infinite loop. I think this is because they don't > > > > > have dev->bus or dev->driver which is because hwif_register_devices > > > > > doesn't get called if !hwif->present. > > > > > > > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > > > > Cc: Bartlomiej Zolnierkiewicz <bzolnier@gmail.com> > > > > > --- > > > > > drivers/ide/ide-probe.c | 10 ++++++---- > > > > > 1 files changed, 6 insertions(+), 4 deletions(-) > > > > > > > > > > diff --git a/drivers/ide/ide-probe.c b/drivers/ide/ide-probe.c > > > > > index 312127e..120f65b 100644 > > > > > --- a/drivers/ide/ide-probe.c > > > > > +++ b/drivers/ide/ide-probe.c > > > > > @@ -1520,6 +1520,8 @@ int ide_host_register(struct ide_host *host, const struct ide_port_info *d, > > > > > if (hwif_init(hwif) == 0) { > > > > > printk(KERN_INFO "%s: failed to initialize IDE " > > > > > "interface\n", hwif->name); > > > > > + device_unregister(&hwif->gendev); > > > > > + device_destroy(ide_port_class, MKDEV(0, 0)); > > > > > hwif->present = 0; > > > > > > > > I've been wondering about this a bit and I think that we should just nuke > > > > port from struct ide_host in such situation so I added ide_disable_port() > > > > helper for this (+ fixed ide_register_port() handling while at it). > > > > > > > > Could you please re-base your fix on top of attached patch? > > > > > > Sure. > > > > > > Subject: IDE: Disable and unregister devices if initialization fails. > > > > > > On reboot the loop in device_shutdown gets confused by these partially > > > initialized devices and goes into an infinite loop. Therefore disable > > > and unregister these devices. > > > > > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > > > > > > --- > > > drivers/ide/ide-probe.c | 10 ++++++---- > > > 1 file changed, 6 insertions(+), 4 deletions(-) > > > > > > diff -r 12d1a7848ddf drivers/ide/ide-probe.c > > > --- a/drivers/ide/ide-probe.c Fri Jan 30 16:37:15 2009 +0000 > > > +++ b/drivers/ide/ide-probe.c Fri Jan 30 17:00:08 2009 +0000 > > > @@ -1549,6 +1549,8 @@ > > > if (hwif_init(hwif) == 0) { > > > printk(KERN_INFO "%s: failed to initialize IDE " > > > "interface\n", hwif->name); > > > + ide_disable_port(hwif); > > > + device_unregister(&hwif->gendev); > > > hwif->present = 0; > > > continue; > > > > Hmmm, ide_disable_port() may free hwif so it needs to happen last... > > Oops, new version below. > > > Also did you have a chance to retest this? [ I couldn't reproduce > > the issue so was hoping that you could test combined fixes. ] > > Yep, I can easily repro. I've actually fixed the underlying issue (a Xen > IRQ problem) causing the probe to fail but can easily revert that fix to > test these patches. If you wanted to repro yourself you could probably > make the request_irq() in init_irq() artificially fail. > > Ian. Ok, thanks for explaining this (I get "The Big Picture" now). > IDE: Disable and unregister devices if initialization fails. > > On reboot the loop in device_shutdown gets confused by these partially > initialized devices and goes into an infinite loop. Therefore disable > and unregister these devices. > > Signed-off-by: Ian Campbell <ian.campbell@citrix.com> > --- > drivers/ide/ide-probe.c | 10 ++++++---- > 1 file changed, 6 insertions(+), 4 deletions(-) > > > diff -r 9d5441419978 -r f596f17b02c3 drivers/ide/ide-probe.c > --- a/drivers/ide/ide-probe.c Fri Jan 30 17:39:53 2009 +0000 > +++ b/drivers/ide/ide-probe.c Mon Feb 02 10:02:32 2009 +0000 > @@ -1549,6 +1549,8 @@ > if (hwif_init(hwif) == 0) { > printk(KERN_INFO "%s: failed to initialize IDE " > "interface\n", hwif->name); > + device_unregister(&hwif->gendev); > + ide_disable_port(hwif); > hwif->present = 0; I applied the patch (removing leftover hwif->present while at it) and sucessfully tested it locally (by making hwif_init() to fail). Thanks, Bart ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2009-02-02 18:36 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2009-01-23 13:04 [PATCH] IDE: Unregister devices if initialization fails Ian Campbell 2009-01-27 16:41 ` Bartlomiej Zolnierkiewicz 2009-01-30 17:19 ` Ian Campbell 2009-02-01 16:21 ` Bartlomiej Zolnierkiewicz 2009-02-02 10:09 ` Ian Campbell 2009-02-02 17:49 ` Bartlomiej Zolnierkiewicz
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).