| Message ID | 1715067.uK3nZ8ehWr@vostro.rjw.lan (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
Hello, On Mon, Jun 24, 2013 at 02:43:27PM +0200, Rafael J. Wysocki wrote: > From: Aaron Lu <aaron.lu@intel.com> > > Commit 30dcf76acc69 "libata: migrate ACPI code over to new bindings" > mistakenly dropped the code to register hotplug notificaion handler > for ATA port/devices, causing regression for people using ATA bay, > as kernel bug #59871 shows. > > Fix this by adding back the hotplug notification handler registration > code. Since this code has to be run once and notification needs to > be installed on every ATA port/devices handle no matter if there is > actual device attached, we can't do this in binding time for ATA > device ACPI handle, as the binding only occurs when a SCSI device is > created, i.e. there is device attached. So introduce the > ata_acpi_hotplug_init() function to loop scan all ATA ACPI handles > and if it is available, install the notificaion handler for it during > ATA init time. > > With the ATA ACPI handle binding to SCSI device tree, it is possible > now that when the SCSI hotplug work removes the SCSI device, the ACPI > unbind function will find that the corresponding ACPI device has > already been deleted by dock driver, causing a scaring message like: > [ 128.263966] scsi 4:0:0:0: Oops, 'acpi_handle' corrupt > Fix this by waiting for SCSI hotplug task finish in our notificaion > handler, so that the removal of ACPI device done in ACPI unbind > function triggered by the removal of SCSI device is run earlier when > ACPI device is still available. > > [rjw: Rebased] > References: https://bugzilla.kernel.org/show_bug.cgi?id=59871 > Reported-bisected-and-tested-by: Dirk Griesbach <spamthis@freenet.de> > Signed-off-by: Aaron Lu <aaron.lu@intel.com> Acked-by: Tejun Heo <tj@kernel.org> > May I add the $subject patch to that series and push along with it? Yes, please. Thanks a lot!
On 06/24/2013 08:43 PM, Rafael J. Wysocki wrote: > From: Aaron Lu <aaron.lu@intel.com> > > Commit 30dcf76acc69 "libata: migrate ACPI code over to new bindings" > mistakenly dropped the code to register hotplug notificaion handler > for ATA port/devices, causing regression for people using ATA bay, > as kernel bug #59871 shows. > > Fix this by adding back the hotplug notification handler registration > code. Since this code has to be run once and notification needs to > be installed on every ATA port/devices handle no matter if there is > actual device attached, we can't do this in binding time for ATA > device ACPI handle, as the binding only occurs when a SCSI device is > created, i.e. there is device attached. So introduce the > ata_acpi_hotplug_init() function to loop scan all ATA ACPI handles > and if it is available, install the notificaion handler for it during > ATA init time. > > With the ATA ACPI handle binding to SCSI device tree, it is possible > now that when the SCSI hotplug work removes the SCSI device, the ACPI > unbind function will find that the corresponding ACPI device has > already been deleted by dock driver, causing a scaring message like: > [ 128.263966] scsi 4:0:0:0: Oops, 'acpi_handle' corrupt > Fix this by waiting for SCSI hotplug task finish in our notificaion > handler, so that the removal of ACPI device done in ACPI unbind > function triggered by the removal of SCSI device is run earlier when > ACPI device is still available. > > [rjw: Rebased] > References: https://bugzilla.kernel.org/show_bug.cgi?id=59871 > Reported-bisected-and-tested-by: Dirk Griesbach <spamthis@freenet.de> > Signed-off-by: Aaron Lu <aaron.lu@intel.com> Sorry for replying late, I forgot to CC stable here. Hopefully it's not too late. Thanks, Aaron > --- > > Hi Tejun, > > If I'm not overlooking any hidden twists, it should be safe to add this > patch on top of the following three that I'm going to push for 3.10 > in a couple of days (unless 3.10 is released first): > > https://patchwork.kernel.org/patch/2766491/ > https://patchwork.kernel.org/patch/2766501/ > https://patchwork.kernel.org/patch/2768521/ > > May I add the $subject patch to that series and push along with it? > > Rafael > > --- > drivers/ata/libata-acpi.c | 37 ++++++++++++++++++++++++++++++++++++- > drivers/ata/libata-core.c | 2 ++ > drivers/ata/libata.h | 2 ++ > 3 files changed, 40 insertions(+), 1 deletion(-) > > Index: linux-pm/drivers/ata/libata-acpi.c > =================================================================== > --- linux-pm.orig/drivers/ata/libata-acpi.c > +++ linux-pm/drivers/ata/libata-acpi.c > @@ -156,8 +156,10 @@ static void ata_acpi_handle_hotplug(stru > > spin_unlock_irqrestore(ap->lock, flags); > > - if (wait) > + if (wait) { > ata_port_wait_eh(ap); > + flush_work(&ap->hotplug_task.work); > + } > } > > static void ata_acpi_dev_notify_dock(acpi_handle handle, u32 event, void *data) > @@ -214,6 +216,39 @@ static const struct acpi_dock_ops ata_ac > .uevent = ata_acpi_ap_uevent, > }; > > +void ata_acpi_hotplug_init(struct ata_host *host) > +{ > + int i; > + > + for (i = 0; i < host->n_ports; i++) { > + struct ata_port *ap = host->ports[i]; > + acpi_handle handle; > + struct ata_device *dev; > + > + if (!ap) > + continue; > + > + handle = ata_ap_acpi_handle(ap); > + if (handle) { > + /* we might be on a docking station */ > + register_hotplug_dock_device(handle, > + &ata_acpi_ap_dock_ops, ap, > + NULL, NULL); > + } > + > + ata_for_each_dev(dev, &ap->link, ALL) { > + handle = ata_dev_acpi_handle(dev); > + if (!handle) > + continue; > + > + /* we might be on a docking station */ > + register_hotplug_dock_device(handle, > + &ata_acpi_dev_dock_ops, > + dev, NULL, NULL); > + } > + } > +} > + > /** > * ata_acpi_dissociate - dissociate ATA host from ACPI objects > * @host: target ATA host > Index: linux-pm/drivers/ata/libata-core.c > =================================================================== > --- linux-pm.orig/drivers/ata/libata-core.c > +++ linux-pm/drivers/ata/libata-core.c > @@ -6148,6 +6148,8 @@ int ata_host_register(struct ata_host *h > if (rc) > goto err_tadd; > > + ata_acpi_hotplug_init(host); > + > /* set cable, sata_spd_limit and report */ > for (i = 0; i < host->n_ports; i++) { > struct ata_port *ap = host->ports[i]; > Index: linux-pm/drivers/ata/libata.h > =================================================================== > --- linux-pm.orig/drivers/ata/libata.h > +++ linux-pm/drivers/ata/libata.h > @@ -122,6 +122,7 @@ extern int ata_acpi_register(void); > extern void ata_acpi_unregister(void); > extern void ata_acpi_bind(struct ata_device *dev); > extern void ata_acpi_unbind(struct ata_device *dev); > +extern void ata_acpi_hotplug_init(struct ata_host *host); > #else > static inline void ata_acpi_dissociate(struct ata_host *host) { } > static inline int ata_acpi_on_suspend(struct ata_port *ap) { return 0; } > @@ -134,6 +135,7 @@ static inline int ata_acpi_register(void > static inline void ata_acpi_unregister(void) { } > static inline void ata_acpi_bind(struct ata_device *dev) { } > static inline void ata_acpi_unbind(struct ata_device *dev) { } > +static inline void ata_acpi_hotplug_init(struct ata_host *host) {} > #endif > > /* libata-scsi.c */ > > -- > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, June 26, 2013 09:43:09 AM Aaron Lu wrote: > On 06/24/2013 08:43 PM, Rafael J. Wysocki wrote: > > From: Aaron Lu <aaron.lu@intel.com> > > > > Commit 30dcf76acc69 "libata: migrate ACPI code over to new bindings" > > mistakenly dropped the code to register hotplug notificaion handler > > for ATA port/devices, causing regression for people using ATA bay, > > as kernel bug #59871 shows. > > > > Fix this by adding back the hotplug notification handler registration > > code. Since this code has to be run once and notification needs to > > be installed on every ATA port/devices handle no matter if there is > > actual device attached, we can't do this in binding time for ATA > > device ACPI handle, as the binding only occurs when a SCSI device is > > created, i.e. there is device attached. So introduce the > > ata_acpi_hotplug_init() function to loop scan all ATA ACPI handles > > and if it is available, install the notificaion handler for it during > > ATA init time. > > > > With the ATA ACPI handle binding to SCSI device tree, it is possible > > now that when the SCSI hotplug work removes the SCSI device, the ACPI > > unbind function will find that the corresponding ACPI device has > > already been deleted by dock driver, causing a scaring message like: > > [ 128.263966] scsi 4:0:0:0: Oops, 'acpi_handle' corrupt > > Fix this by waiting for SCSI hotplug task finish in our notificaion > > handler, so that the removal of ACPI device done in ACPI unbind > > function triggered by the removal of SCSI device is run earlier when > > ACPI device is still available. > > > > [rjw: Rebased] > > References: https://bugzilla.kernel.org/show_bug.cgi?id=59871 > > Reported-bisected-and-tested-by: Dirk Griesbach <spamthis@freenet.de> > > Signed-off-by: Aaron Lu <aaron.lu@intel.com> > > Sorry for replying late, I forgot to CC stable here. > Hopefully it's not too late. I've added a CC to -stable to the commit log. Thanks, Rafael > > --- > > > > Hi Tejun, > > > > If I'm not overlooking any hidden twists, it should be safe to add this > > patch on top of the following three that I'm going to push for 3.10 > > in a couple of days (unless 3.10 is released first): > > > > https://patchwork.kernel.org/patch/2766491/ > > https://patchwork.kernel.org/patch/2766501/ > > https://patchwork.kernel.org/patch/2768521/ > > > > May I add the $subject patch to that series and push along with it? > > > > Rafael > > > > --- > > drivers/ata/libata-acpi.c | 37 ++++++++++++++++++++++++++++++++++++- > > drivers/ata/libata-core.c | 2 ++ > > drivers/ata/libata.h | 2 ++ > > 3 files changed, 40 insertions(+), 1 deletion(-) > > > > Index: linux-pm/drivers/ata/libata-acpi.c > > =================================================================== > > --- linux-pm.orig/drivers/ata/libata-acpi.c > > +++ linux-pm/drivers/ata/libata-acpi.c > > @@ -156,8 +156,10 @@ static void ata_acpi_handle_hotplug(stru > > > > spin_unlock_irqrestore(ap->lock, flags); > > > > - if (wait) > > + if (wait) { > > ata_port_wait_eh(ap); > > + flush_work(&ap->hotplug_task.work); > > + } > > } > > > > static void ata_acpi_dev_notify_dock(acpi_handle handle, u32 event, void *data) > > @@ -214,6 +216,39 @@ static const struct acpi_dock_ops ata_ac > > .uevent = ata_acpi_ap_uevent, > > }; > > > > +void ata_acpi_hotplug_init(struct ata_host *host) > > +{ > > + int i; > > + > > + for (i = 0; i < host->n_ports; i++) { > > + struct ata_port *ap = host->ports[i]; > > + acpi_handle handle; > > + struct ata_device *dev; > > + > > + if (!ap) > > + continue; > > + > > + handle = ata_ap_acpi_handle(ap); > > + if (handle) { > > + /* we might be on a docking station */ > > + register_hotplug_dock_device(handle, > > + &ata_acpi_ap_dock_ops, ap, > > + NULL, NULL); > > + } > > + > > + ata_for_each_dev(dev, &ap->link, ALL) { > > + handle = ata_dev_acpi_handle(dev); > > + if (!handle) > > + continue; > > + > > + /* we might be on a docking station */ > > + register_hotplug_dock_device(handle, > > + &ata_acpi_dev_dock_ops, > > + dev, NULL, NULL); > > + } > > + } > > +} > > + > > /** > > * ata_acpi_dissociate - dissociate ATA host from ACPI objects > > * @host: target ATA host > > Index: linux-pm/drivers/ata/libata-core.c > > =================================================================== > > --- linux-pm.orig/drivers/ata/libata-core.c > > +++ linux-pm/drivers/ata/libata-core.c > > @@ -6148,6 +6148,8 @@ int ata_host_register(struct ata_host *h > > if (rc) > > goto err_tadd; > > > > + ata_acpi_hotplug_init(host); > > + > > /* set cable, sata_spd_limit and report */ > > for (i = 0; i < host->n_ports; i++) { > > struct ata_port *ap = host->ports[i]; > > Index: linux-pm/drivers/ata/libata.h > > =================================================================== > > --- linux-pm.orig/drivers/ata/libata.h > > +++ linux-pm/drivers/ata/libata.h > > @@ -122,6 +122,7 @@ extern int ata_acpi_register(void); > > extern void ata_acpi_unregister(void); > > extern void ata_acpi_bind(struct ata_device *dev); > > extern void ata_acpi_unbind(struct ata_device *dev); > > +extern void ata_acpi_hotplug_init(struct ata_host *host); > > #else > > static inline void ata_acpi_dissociate(struct ata_host *host) { } > > static inline int ata_acpi_on_suspend(struct ata_port *ap) { return 0; } > > @@ -134,6 +135,7 @@ static inline int ata_acpi_register(void > > static inline void ata_acpi_unregister(void) { } > > static inline void ata_acpi_bind(struct ata_device *dev) { } > > static inline void ata_acpi_unbind(struct ata_device *dev) { } > > +static inline void ata_acpi_hotplug_init(struct ata_host *host) {} > > #endif > > > > /* libata-scsi.c */ > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-acpi" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > >
Index: linux-pm/drivers/ata/libata-acpi.c =================================================================== --- linux-pm.orig/drivers/ata/libata-acpi.c +++ linux-pm/drivers/ata/libata-acpi.c @@ -156,8 +156,10 @@ static void ata_acpi_handle_hotplug(stru spin_unlock_irqrestore(ap->lock, flags); - if (wait) + if (wait) { ata_port_wait_eh(ap); + flush_work(&ap->hotplug_task.work); + } } static void ata_acpi_dev_notify_dock(acpi_handle handle, u32 event, void *data) @@ -214,6 +216,39 @@ static const struct acpi_dock_ops ata_ac .uevent = ata_acpi_ap_uevent, }; +void ata_acpi_hotplug_init(struct ata_host *host) +{ + int i; + + for (i = 0; i < host->n_ports; i++) { + struct ata_port *ap = host->ports[i]; + acpi_handle handle; + struct ata_device *dev; + + if (!ap) + continue; + + handle = ata_ap_acpi_handle(ap); + if (handle) { + /* we might be on a docking station */ + register_hotplug_dock_device(handle, + &ata_acpi_ap_dock_ops, ap, + NULL, NULL); + } + + ata_for_each_dev(dev, &ap->link, ALL) { + handle = ata_dev_acpi_handle(dev); + if (!handle) + continue; + + /* we might be on a docking station */ + register_hotplug_dock_device(handle, + &ata_acpi_dev_dock_ops, + dev, NULL, NULL); + } + } +} + /** * ata_acpi_dissociate - dissociate ATA host from ACPI objects * @host: target ATA host Index: linux-pm/drivers/ata/libata-core.c =================================================================== --- linux-pm.orig/drivers/ata/libata-core.c +++ linux-pm/drivers/ata/libata-core.c @@ -6148,6 +6148,8 @@ int ata_host_register(struct ata_host *h if (rc) goto err_tadd; + ata_acpi_hotplug_init(host); + /* set cable, sata_spd_limit and report */ for (i = 0; i < host->n_ports; i++) { struct ata_port *ap = host->ports[i]; Index: linux-pm/drivers/ata/libata.h =================================================================== --- linux-pm.orig/drivers/ata/libata.h +++ linux-pm/drivers/ata/libata.h @@ -122,6 +122,7 @@ extern int ata_acpi_register(void); extern void ata_acpi_unregister(void); extern void ata_acpi_bind(struct ata_device *dev); extern void ata_acpi_unbind(struct ata_device *dev); +extern void ata_acpi_hotplug_init(struct ata_host *host); #else static inline void ata_acpi_dissociate(struct ata_host *host) { } static inline int ata_acpi_on_suspend(struct ata_port *ap) { return 0; } @@ -134,6 +135,7 @@ static inline int ata_acpi_register(void static inline void ata_acpi_unregister(void) { } static inline void ata_acpi_bind(struct ata_device *dev) { } static inline void ata_acpi_unbind(struct ata_device *dev) { } +static inline void ata_acpi_hotplug_init(struct ata_host *host) {} #endif /* libata-scsi.c */