ahci: unregister acpi notify handler when a ZPODD is unbound

When a ZPODD device is unbound via sysfs, the acpi notify handler is not removed. This causes panics as observed in Bug #74601. The panic only happens when the wake happens from outside the kernel (i.e. inserting media or pressing a button). Implement a new ahci_remove_one function which causes zpodd_exit to be called for all ZPODD devices on the unbound PCI device.

Signed-off-by Levente Kurusa <[email protected]>

---

Hi,

I am not sure if the loop below is correct. Maybe there is a better solution to loop through all the devices which might use ZPODD?

Thanks, Lev.

 drivers/ata/ahci.c         | 21 +++++++++++++++++++++
 drivers/ata/ahci.h         |  4 ++++
 drivers/ata/libata-zpodd.c |  1 +
 3 files changed, 26 insertions(+)

drivers/ata/ahci.c +3 -0

index 5a0bf8e..6d92bc9 100644
@@ -475,12 +475,33 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ }	/* terminate list */
 };
 
+#ifdef CONFIG_SATA_ZPODD
+void ahci_remove_one(struct pci_dev *pdev)
+{

drivers/ata/ahci.c +7 -0

+	ata_pci_remove_one(pdev);
+}
+#endif
 
 static struct pci_driver ahci_pci_driver = {
 	.name			= DRV_NAME,
 	.id_table		= ahci_pci_tbl,
 	.probe			= ahci_init_one,
+#ifdef CONFIG_SATA_ZPODD
+	.remove			= ahci_remove_one,
+#else
 	.remove			= ata_pci_remove_one,
+#endif
 #ifdef CONFIG_PM
 	.suspend		= ahci_pci_device_suspend,
 	.resume			= ahci_pci_device_resume,

drivers/ata/ahci.h +4 -0

index 51af275..87e4e6d 100644
@@ -383,6 +383,10 @@ void ahci_print_info(struct ata_host *host, const char *scc_s);
 int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis);
 void ahci_error_handler(struct ata_port *ap);
 
+#ifdef CONFIG_SATA_ZPODD
+extern void zpodd_exit(struct ata_device *dev);
+#endif /* CONFIG_SATA_ZPODD */
+
 static inline void __iomem *__ahci_port_base(struct ata_host *host,
 					     unsigned int port_no)
 {

drivers/ata/libata-zpodd.c +0 -0

index f3a65a3..fe66949 100644

When a ZPODD device is unbound via sysfs, the acpi notify handler is not removed. This causes panics as observed in Bug #74601. The

panic only happens when the wake happens from outside the kernel (i.e. inserting media or pressing a button). Implement a new ahci_remove_one function which causes zpodd_exit to be called for all ZPODD devices on the unbound PCI device.

Signed-off-by Levente Kurusa <[email protected]>

---

Hi,

I am not sure if the loop below is correct. Maybe there is a better solution to loop through all the devices which might use ZPODD?

Thanks, Lev.

 drivers/ata/ahci.c         | 21 +++++++++++++++++++++
 drivers/ata/ahci.h         |  4 ++++
 drivers/ata/libata-zpodd.c |  1 +
 3 files changed, 26 insertions(+)

drivers/ata/ahci.c +21 -0

index 5a0bf8e..6d92bc9 100644
@@ -475,12 +475,33 @@ static const struct pci_device_id ahci_pci_tbl[] = {
 	{ }	/* terminate list */
 };
 
+#ifdef CONFIG_SATA_ZPODD
+void ahci_remove_one(struct pci_dev *pdev)
+{
+	struct ata_host *host = pci_get_drvdata(pdev);
+	struct ata_link *link;
+	struct ata_device *dev;
+	int i;
+
+	for (i = 0; i < host->n_ports; i++)
+		ata_for_each_link(link, host->ports[i], HOST_FIRST)
+			ata_for_each_dev(dev, link, ALL)
+				if (dev->zpodd)
+					zpodd_exit(dev);
+
+	ata_pci_remove_one(pdev);
+}
+#endif
 
 static struct pci_driver ahci_pci_driver = {
 	.name			= DRV_NAME,
 	.id_table		= ahci_pci_tbl,
 	.probe			= ahci_init_one,
+#ifdef CONFIG_SATA_ZPODD
+	.remove			= ahci_remove_one,
+#else
 	.remove			= ata_pci_remove_one,
+#endif
 #ifdef CONFIG_PM
 	.suspend		= ahci_pci_device_suspend,
 	.resume			= ahci_pci_device_resume,

drivers/ata/ahci.h +4 -0

index 51af275..87e4e6d 100644
@@ -383,6 +383,10 @@ void ahci_print_info(struct ata_host *host, const char *scc_s);
 int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis);
 void ahci_error_handler(struct ata_port *ap);
 
+#ifdef CONFIG_SATA_ZPODD
+extern void zpodd_exit(struct ata_device *dev);
+#endif /* CONFIG_SATA_ZPODD */
+
 static inline void __iomem *__ahci_port_base(struct ata_host *host,
 					     unsigned int port_no)
 {

drivers/ata/libata-zpodd.c +1 -0

index f3a65a3..fe66949 100644
@@ -281,3 +281,4 @@ void zpodd_exit(struct ata_device *dev)
 	kfree(dev->zpodd);
 	dev->zpodd = NULL;
 }
+EXPORT_SYMBOL_GPL(zpodd_exit);

Quoted 2 lines · Levente Kurusa

When a ZPODD device is unbound via sysfs, the acpi notify handler is not removed. This causes panics as observed in Bug #74601. The

Ah...too bad, I forgot to consider this situation, thanks for tracking this.

Quoted 5 lines

panic only happens when the wake happens from outside the kernel (i.e. inserting media or pressing a button). Implement a new ahci_remove_one function which causes zpodd_exit to be called for all ZPODD devices on the unbound PCI device.

Quoted 7 lines

Signed-off-by Levente Kurusa <[email protected]>

---

Hi,

I am not sure if the loop below is correct. Maybe there is a better solution to loop through all the devices which might use ZPODD?

I didn't find a proper place either. For hotplug, we did the zpodd_exit at ata_scsi_handle_link_detach. But for host controller pci device removal, we used scsi_remove_host in ata_port_detach and there is no place to add the zpodd_exit for a to-be-removed scsi device...

Looks like we can only iterate the ata devices and call zpodd_exit explicitly for them if they are zpodd devices. Instead of adding a new remove callback, what about just embed that into the ata_port_detach like the following example?

drivers/ata/libata-core.c +9 -0

index 943cc8b83e59..43652da6fea6 100644
@@ -6314,6 +6314,8 @@ int ata_host_activate(struct ata_host *host, int irq,
 static void ata_port_detach(struct ata_port *ap)
 {
 	unsigned long flags;
+	struct ata_link *link;
+	struct ata_device *dev;
 
 	if (!ap->ops->error_handler)
 		goto skip_eh;

@@ -6333,6 +6335,13 @@ static void ata_port_detach(struct ata_port *ap)
 	cancel_delayed_work_sync(&ap->hotplug_task);
 
  skip_eh:
+	/* clean up zpodd related stuffs on port removal */
+	ata_for_each_link(link, ap, HOST_FIRST) {
+		ata_for_each_dev(dev, link, ALL) {
+			if (zpodd_dev_enabled(dev))
+				zpodd_exit(dev);
+		}
+	}
 	if (ap->pmp_link) {
 		int i;
 		for (i = 0; i < SATA_PMP_MAX_PORTS; i++)

Hi,

Quoted 25 lines · Aaron Lu

Quoted 2 lines · Levente Kurusa

When a ZPODD device is unbound via sysfs, the acpi notify handler is not removed. This causes panics as observed in Bug #74601. The

Ah...too bad, I forgot to consider this situation, thanks for tracking this.

Quoted 5 lines

panic only happens when the wake happens from outside the kernel (i.e. inserting media or pressing a button). Implement a new ahci_remove_one function which causes zpodd_exit to be called for all ZPODD devices on the unbound PCI device.

Quoted 7 lines

Signed-off-by Levente Kurusa <[email protected]>

---

Hi,

I am not sure if the loop below is correct. Maybe there is a better solution to loop through all the devices which might use ZPODD?

I didn't find a proper place either. For hotplug, we did the zpodd_exit at ata_scsi_handle_link_detach. But for host controller pci device removal, we used scsi_remove_host in ata_port_detach and there is no place to add the zpodd_exit for a to-be-removed scsi device...

Quoted 4 lines

Looks like we can only iterate the ata devices and call zpodd_exit explicitly for them if they are zpodd devices. Instead of adding a new remove callback, what about just embed that into the ata_port_detach like the following example?

Yes, this makes more sense as this doesn't tinker with exports and such... However this will throw unused variable compiler warnings if we add the required #ifdefs... Maybe a new function? ata_zpodd_detach_port?

Quoted 30 lines

drivers/ata/libata-core.c +9 -0

index 943cc8b83e59..43652da6fea6 100644
@@ -6314,6 +6314,8 @@ int ata_host_activate(struct ata_host *host, int irq,
 static void ata_port_detach(struct ata_port *ap)
 {
 	unsigned long flags;
+	struct ata_link *link;
+	struct ata_device *dev;
 
 	if (!ap->ops->error_handler)
 		goto skip_eh;

@@ -6333,6 +6335,13 @@ static void ata_port_detach(struct ata_port *ap)
 	cancel_delayed_work_sync(&ap->hotplug_task);
 
  skip_eh:
+	/* clean up zpodd related stuffs on port removal */
+	ata_for_each_link(link, ap, HOST_FIRST) {
+		ata_for_each_dev(dev, link, ALL) {
+			if (zpodd_dev_enabled(dev))
+				zpodd_exit(dev);
+		}
+	}
 	if (ap->pmp_link) {
 		int i;
 		for (i = 0; i < SATA_PMP_MAX_PORTS; i++)

Quoted 33 lines · Levente Kurusa

Hi,

Quoted 29 lines · Aaron Lu

Quoted 2 lines · Levente Kurusa

When a ZPODD device is unbound via sysfs, the acpi notify handler is not removed. This causes panics as observed in Bug #74601. The

Ah...too bad, I forgot to consider this situation, thanks for tracking this.

Quoted 5 lines

panic only happens when the wake happens from outside the kernel (i.e. inserting media or pressing a button). Implement a new ahci_remove_one function which causes zpodd_exit to be called for all ZPODD devices on the unbound PCI device.

Quoted 7 lines

Signed-off-by Levente Kurusa <[email protected]>

---

Hi,

I am not sure if the loop below is correct. Maybe there is a better solution to loop through all the devices which might use ZPODD?

I didn't find a proper place either. For hotplug, we did the zpodd_exit at ata_scsi_handle_link_detach. But for host controller pci device removal, we used scsi_remove_host in ata_port_detach and there is no place to add the zpodd_exit for a to-be-removed scsi device...

Looks like we can only iterate the ata devices and call zpodd_exit explicitly for them if they are zpodd devices. Instead of adding a new remove callback, what about just embed that into the ata_port_detach like the following example?

Quoted 3 lines

Yes, this makes more sense as this doesn't tinker with exports and such... However this will throw unused variable compiler warnings if we add the required #ifdefs... Maybe a new function? ata_zpodd_detach_port?

I think we can omit the #ifdefs as the loop is not called frequently and thus doesn't cost much. We already have stubs for zpodd_dev_enabled and zpodd_exit.

Hi,

Quoted 38 lines · Aaron Lu

Quoted 36 lines · Levente Kurusa

Hi,

Quoted 25 lines · Aaron Lu

Quoted 2 lines · Levente Kurusa

When a ZPODD device is unbound via sysfs, the acpi notify handler is not removed. This causes panics as observed in Bug #74601. The

Ah...too bad, I forgot to consider this situation, thanks for tracking this.

Quoted 5 lines

panic only happens when the wake happens from outside the kernel (i.e. inserting media or pressing a button). Implement a new ahci_remove_one function which causes zpodd_exit to be called for all ZPODD devices on the unbound PCI device.

Quoted 7 lines

Signed-off-by Levente Kurusa <[email protected]>

---

Hi,

I am not sure if the loop below is correct. Maybe there is a better solution to loop through all the devices which might use ZPODD?

I didn't find a proper place either. For hotplug, we did the zpodd_exit at ata_scsi_handle_link_detach. But for host controller pci device removal, we used scsi_remove_host in ata_port_detach and there is no place to add the zpodd_exit for a to-be-removed scsi device...

Quoted 4 lines

Looks like we can only iterate the ata devices and call zpodd_exit explicitly for them if they are zpodd devices. Instead of adding a new remove callback, what about just embed that into the ata_port_detach like the following example?

Yes, this makes more sense as this doesn't tinker with exports and such... However this will throw unused variable compiler warnings if we add the required #ifdefs... Maybe a new function? ata_zpodd_detach_port?

Quoted 4 lines

I think we can omit the #ifdefs as the loop is not called frequently and thus doesn't cost much. We already have stubs for zpodd_dev_enabled and zpodd_exit.

Ah, I see. Shall I send V2? Any tags I should add for you?