libata: remove dead code from libata-acpi.c

PATCH In review Liu Jiang 6 participants 14 messages Jun 15, 2013 – Jun 26, 2013 libata

lore

Share & tools

lore

Liu Jiang

author

2013-06-15 03:05 UTC #0 lore raw

PATCH 1/1 libata: remove dead code from libata-acpi.c

From: Liu Jiang <[email protected]>

Commit 30dcf76acc69 "libata: migrate ACPI code over to new bindings" removed ACPI dock notification related code, but there's some dead code left, so clean up it.

Cc Tejun Heo <[email protected]>

Cc Matthew Garrett <[email protected]>

Cc Aaron Lu <[email protected]>

Cc [email protected]

Signed-off-by Liu Jiang <[email protected]>

---
 drivers/ata/libata-acpi.c | 123 ----------------------------------------------
 1 file changed, 123 deletions(-)

drivers/ata/libata-acpi.c +0 ~~-123~~

index 87f2f39..e50c987 100644
@@ -91,129 +91,6 @@ acpi_handle ata_dev_acpi_handle(struct ata_device *dev)
 }
 EXPORT_SYMBOL(ata_dev_acpi_handle);
 
-/* @ap and @dev are the same as ata_acpi_handle_hotplug() */
-static void ata_acpi_detach_device(struct ata_port *ap, struct ata_device *dev)
-{
-	if (dev)
-		dev->flags |= ATA_DFLAG_DETACH;
-	else {
-		struct ata_link *tlink;
-		struct ata_device *tdev;
-
-		ata_for_each_link(tlink, ap, EDGE)
-			ata_for_each_dev(tdev, tlink, ALL)
-				tdev->flags |= ATA_DFLAG_DETACH;
-	}
-
-	ata_port_schedule_eh(ap);
-}
-
-/**
- * ata_acpi_handle_hotplug - ACPI event handler backend
- * @ap: ATA port ACPI event occurred
- * @dev: ATA device ACPI event occurred (can be NULL)
- * @event: ACPI event which occurred
- *
- * All ACPI bay / device realted events end up in this function.  If
- * the event is port-wide @dev is NULL.  If the event is specific to a
- * device, @dev points to it.
- *
- * Hotplug (as opposed to unplug) notification is always handled as
- * port-wide while unplug only kills the target device on device-wide
- * event.
- *
- * LOCKING:
- * ACPI notify handler context.  May sleep.
- */
-static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device *dev,
-				    u32 event)
-{
-	struct ata_eh_info *ehi = &ap->link.eh_info;
-	int wait = 0;
-	unsigned long flags;
-
-	spin_lock_irqsave(ap->lock, flags);
-	/*
-	 * When dock driver calls into the routine, it will always use
-	 * ACPI_NOTIFY_BUS_CHECK/ACPI_NOTIFY_DEVICE_CHECK for add and
-	 * ACPI_NOTIFY_EJECT_REQUEST for remove
-	 */
-	switch (event) {
-	case ACPI_NOTIFY_BUS_CHECK:
-	case ACPI_NOTIFY_DEVICE_CHECK:
-		ata_ehi_push_desc(ehi, "ACPI event");
-
-		ata_ehi_hotplugged(ehi);
-		ata_port_freeze(ap);
-		break;
-	case ACPI_NOTIFY_EJECT_REQUEST:
-		ata_ehi_push_desc(ehi, "ACPI event");
-
-		ata_acpi_detach_device(ap, dev);
-		wait = 1;
-		break;
-	}
-
-	spin_unlock_irqrestore(ap->lock, flags);
-
-	if (wait)
-		ata_port_wait_eh(ap);
-}
-
-static void ata_acpi_dev_notify_dock(acpi_handle handle, u32 event, void *data)
-{
-	struct ata_device *dev = data;
-
-	ata_acpi_handle_hotplug(dev->link->ap, dev, event);
-}
-
-static void ata_acpi_ap_notify_dock(acpi_handle handle, u32 event, void *data)
-{
-	struct ata_port *ap = data;
-
-	ata_acpi_handle_hotplug(ap, NULL, event);
-}
-
-static void ata_acpi_uevent(struct ata_port *ap, struct ata_device *dev,
-	u32 event)
-{
-	struct kobject *kobj = NULL;
-	char event_string[20];
-	char *envp[] = { event_string, NULL };
-
-	if (dev) {
-		if (dev->sdev)
-			kobj = &dev->sdev->sdev_gendev.kobj;
-	} else
-		kobj = &ap->dev->kobj;
-
-	if (kobj) {
-		snprintf(event_string, 20, "BAY_EVENT=%d", event);
-		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
-	}
-}
-
-static void ata_acpi_ap_uevent(acpi_handle handle, u32 event, void *data)
-{
-	ata_acpi_uevent(data, NULL, event);
-}
-
-static void ata_acpi_dev_uevent(acpi_handle handle, u32 event, void *data)
-{
-	struct ata_device *dev = data;
-	ata_acpi_uevent(dev->link->ap, dev, event);
-}
-
-static const struct acpi_dock_ops ata_acpi_dev_dock_ops = {
-	.handler = ata_acpi_dev_notify_dock,
-	.uevent = ata_acpi_dev_uevent,
-};
-
-static const struct acpi_dock_ops ata_acpi_ap_dock_ops = {
-	.handler = ata_acpi_ap_notify_dock,
-	.uevent = ata_acpi_ap_uevent,
-};
-
 /**
  * ata_acpi_dissociate - dissociate ATA host from ACPI objects
  * @host: target ATA host

Signature · 1 line

1.8.1.2

Aaron Lu

2013-06-17 01:49 UTC #1 lore raw

Quoted 5 lines · Liu Jiang

From: Liu Jiang <[email protected]>
Commit 30dcf76acc69 "libata: migrate ACPI code over to new bindings" removed ACPI dock notification related code, but there's some dead code left, so clean up it.

I never noticed this, but it looks to be the case...

I'm not sure the dock notification code is removed intentionally or mistakenly though, if it is a mistake, then instead of removing the left code here, we probably should add the dock notification code back.

But I have no test system or any knowledge about how ata dock works, so I may be wrong :-)

Thanks,
Aaron

Quoted 146 lines

Cc Tejun Heo <[email protected]>

Cc Matthew Garrett <[email protected]>

Cc Aaron Lu <[email protected]>

Cc [email protected]

Signed-off-by Liu Jiang <[email protected]>

---
 drivers/ata/libata-acpi.c | 123 ----------------------------------------------
 1 file changed, 123 deletions(-)

drivers/ata/libata-acpi.c +0 ~~-123~~

index 87f2f39..e50c987 100644
@@ -91,129 +91,6 @@ acpi_handle ata_dev_acpi_handle(struct ata_device *dev)
 }
 EXPORT_SYMBOL(ata_dev_acpi_handle);
 
-/* @ap and @dev are the same as ata_acpi_handle_hotplug() */
-static void ata_acpi_detach_device(struct ata_port *ap, struct ata_device *dev)
-{
-	if (dev)
-		dev->flags |= ATA_DFLAG_DETACH;
-	else {
-		struct ata_link *tlink;
-		struct ata_device *tdev;
-
-		ata_for_each_link(tlink, ap, EDGE)
-			ata_for_each_dev(tdev, tlink, ALL)
-				tdev->flags |= ATA_DFLAG_DETACH;
-	}
-
-	ata_port_schedule_eh(ap);
-}
-
-/**
- * ata_acpi_handle_hotplug - ACPI event handler backend
- * @ap: ATA port ACPI event occurred
- * @dev: ATA device ACPI event occurred (can be NULL)
- * @event: ACPI event which occurred
- *
- * All ACPI bay / device realted events end up in this function.  If
- * the event is port-wide @dev is NULL.  If the event is specific to a
- * device, @dev points to it.
- *
- * Hotplug (as opposed to unplug) notification is always handled as
- * port-wide while unplug only kills the target device on device-wide
- * event.
- *
- * LOCKING:
- * ACPI notify handler context.  May sleep.
- */
-static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device *dev,
-				    u32 event)
-{
-	struct ata_eh_info *ehi = &ap->link.eh_info;
-	int wait = 0;
-	unsigned long flags;
-
-	spin_lock_irqsave(ap->lock, flags);
-	/*
-	 * When dock driver calls into the routine, it will always use
-	 * ACPI_NOTIFY_BUS_CHECK/ACPI_NOTIFY_DEVICE_CHECK for add and
-	 * ACPI_NOTIFY_EJECT_REQUEST for remove
-	 */
-	switch (event) {
-	case ACPI_NOTIFY_BUS_CHECK:
-	case ACPI_NOTIFY_DEVICE_CHECK:
-		ata_ehi_push_desc(ehi, "ACPI event");
-
-		ata_ehi_hotplugged(ehi);
-		ata_port_freeze(ap);
-		break;
-	case ACPI_NOTIFY_EJECT_REQUEST:
-		ata_ehi_push_desc(ehi, "ACPI event");
-
-		ata_acpi_detach_device(ap, dev);
-		wait = 1;
-		break;
-	}
-
-	spin_unlock_irqrestore(ap->lock, flags);
-
-	if (wait)
-		ata_port_wait_eh(ap);
-}
-
-static void ata_acpi_dev_notify_dock(acpi_handle handle, u32 event, void *data)
-{
-	struct ata_device *dev = data;
-
-	ata_acpi_handle_hotplug(dev->link->ap, dev, event);
-}
-
-static void ata_acpi_ap_notify_dock(acpi_handle handle, u32 event, void *data)
-{
-	struct ata_port *ap = data;
-
-	ata_acpi_handle_hotplug(ap, NULL, event);
-}
-
-static void ata_acpi_uevent(struct ata_port *ap, struct ata_device *dev,
-	u32 event)
-{
-	struct kobject *kobj = NULL;
-	char event_string[20];
-	char *envp[] = { event_string, NULL };
-
-	if (dev) {
-		if (dev->sdev)
-			kobj = &dev->sdev->sdev_gendev.kobj;
-	} else
-		kobj = &ap->dev->kobj;
-
-	if (kobj) {
-		snprintf(event_string, 20, "BAY_EVENT=%d", event);
-		kobject_uevent_env(kobj, KOBJ_CHANGE, envp);
-	}
-}
-
-static void ata_acpi_ap_uevent(acpi_handle handle, u32 event, void *data)
-{
-	ata_acpi_uevent(data, NULL, event);
-}
-
-static void ata_acpi_dev_uevent(acpi_handle handle, u32 event, void *data)
-{
-	struct ata_device *dev = data;
-	ata_acpi_uevent(dev->link->ap, dev, event);
-}
-
-static const struct acpi_dock_ops ata_acpi_dev_dock_ops = {
-	.handler = ata_acpi_dev_notify_dock,
-	.uevent = ata_acpi_dev_uevent,
-};
-
-static const struct acpi_dock_ops ata_acpi_ap_dock_ops = {
-	.handler = ata_acpi_ap_notify_dock,
-	.uevent = ata_acpi_ap_uevent,
-};
-
 /**
  * ata_acpi_dissociate - dissociate ATA host from ACPI objects
  * @host: target ATA host

Tejun Heo

2013-06-17 18:01 UTC #2 lore raw

Quoted 7 lines · Aaron Lu

Quoted 5 lines · Liu Jiang
From: Liu Jiang <[email protected]>
Commit 30dcf76acc69 "libata: migrate ACPI code over to new bindings" removed ACPI dock notification related code, but there's some dead code left, so clean up it.

Quoted 8 lines

I never noticed this, but it looks to be the case...
I'm not sure the dock notification code is removed intentionally or mistakenly though, if it is a mistake, then instead of removing the left code here, we probably should add the dock notification code back.
But I have no test system or any knowledge about how ata dock works, so I may be wrong :-)

Looks like a regression to me. We're probably locking up on some older laptops which connects optical drives with hotpluggable PATA. Matthew, can you please fix it?

Thanks.

Signature · 1 line

tejun

Jiang Liu (Gerry)

2013-06-18 01:15 UTC #3 lore raw

Quoted 14 lines · Tejun Heo

Quoted 13 lines · Aaron Lu
Quoted 5 lines · Liu Jiang
From: Liu Jiang <[email protected]>
Commit 30dcf76acc69 "libata: migrate ACPI code over to new bindings" removed ACPI dock notification related code, but there's some dead code left, so clean up it.
I never noticed this, but it looks to be the case...
I'm not sure the dock notification code is removed intentionally or mistakenly though, if it is a mistake, then instead of removing the left code here, we probably should add the dock notification code back.

Quoted 9 lines

Quoted 2 lines
But I have no test system or any knowledge about how ata dock works, so I may be wrong :-)
Looks like a regression to me. We're probably locking up on some older laptops which connects optical drives with hotpluggable PATA. Matthew, can you please fix it?
Thanks.

Yeah, Aaron is right. We should restore the dock notification handler to support ATA hotplug on some IBM laptops. Regards!
Gerry

Aaron Lu

2013-06-18 09:16 UTC #4 lore raw

In reply to Tejun Heo

Quoted 14 lines · Tejun Heo

Quoted 13 lines · Aaron Lu
Quoted 5 lines · Liu Jiang
From: Liu Jiang <[email protected]>
Commit 30dcf76acc69 "libata: migrate ACPI code over to new bindings" removed ACPI dock notification related code, but there's some dead code left, so clean up it.
I never noticed this, but it looks to be the case...
I'm not sure the dock notification code is removed intentionally or mistakenly though, if it is a mistake, then instead of removing the left code here, we probably should add the dock notification code back.

Quoted 6 lines

Quoted 2 lines
But I have no test system or any knowledge about how ata dock works, so I may be wrong :-)
Looks like a regression to me. We're probably locking up on some older laptops which connects optical drives with hotpluggable PATA. Matthew, can you please fix it?

A user just reported a bug for this:
https://bugzilla.kernel.org/show_bug.cgi?id=59871

And bisected to commit 30dcf76acc69. I proposed a fix patch there, let's see what the user's test result.

Thanks,
Aaron

Aaron Lu

2013-06-20 02:26 UTC #5 lore raw

Quoted 22 lines · Aaron Lu

Quoted 14 lines · Tejun Heo
Quoted 13 lines · Aaron Lu
Quoted 5 lines · Liu Jiang
From: Liu Jiang <[email protected]>
Commit 30dcf76acc69 "libata: migrate ACPI code over to new bindings" removed ACPI dock notification related code, but there's some dead code left, so clean up it.
I never noticed this, but it looks to be the case...
I'm not sure the dock notification code is removed intentionally or mistakenly though, if it is a mistake, then instead of removing the left code here, we probably should add the dock notification code back.
Quoted 6 lines
Quoted 2 lines
But I have no test system or any knowledge about how ata dock works, so I may be wrong :-)
Looks like a regression to me. We're probably locking up on some older laptops which connects optical drives with hotpluggable PATA. Matthew, can you please fix it?

Quoted 5 lines

A user just reported a bug for this:
https://bugzilla.kernel.org/show_bug.cgi?id=59871
And bisected to commit 30dcf76acc69. I proposed a fix patch there, let's see what the user's test result.

Patch below.

If it is true that the ATA dock is only for PATA devices, then I can simply the code a little bit in that I can do the notification registration in ATA port's binding time for both ATA port ACPI handle and ACPI handle of ATA devices belonging to this port, since we will always create SCSI host devices along with its corresponding ATA port, the binding for ATA port will always occur no matter if there is devices attached or not.

Hi Dirk,
I made a small change in the following patch than the last one I posted in bugzilla that you tested OK to mimic the original behavior in pre-3.6 kernels to install notification handler for ATA devices even when the ATA port's handle is NULL, this can only happen in SATA case, so if the above question is true, then this change will not be necessary.

Subject: [PATCH] libata-acpi: add back ACPI based hotplug functionality

Commit 30dcf76acc mistakenly dropped the code to register hotplug notificaion handler for ATA port/devices, causing regression for people using ATA bay, as 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 introduced the ata_acpi_hotplug_init function to loop scan all ATA ACPI handles and if it is available, install the notification 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.

References: https://bugzilla.kernel.org/show_bug.cgi?id=59871

Reported-and-bisected-by Dirk Griesbach <[email protected]>

Cc <[email protected]>

Signed-off-by Aaron Lu <[email protected]>

---
 drivers/ata/libata-acpi.c | 34 +++++++++++++++++++++++++++++++++-
 drivers/ata/libata-core.c |  2 ++
 drivers/ata/libata.h      |  2 ++
 3 files changed, 37 insertions(+), 1 deletion(-)

drivers/ata/libata-acpi.c +33 -1

index 87f2f39..0ad54bb 100644
@@ -156,8 +156,10 @@ static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device *dev,
 
 	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,36 @@ static const struct acpi_dock_ops ata_acpi_ap_dock_ops = {
 	.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);
+		}
+
+		ata_for_each_dev(dev, &ap->link, ALL) {
+			handle = ata_dev_acpi_handle(dev);
+			if (handle) {
+				/* we might be on a docking station */
+				register_hotplug_dock_device(handle,
+						&ata_acpi_dev_dock_ops, dev);
+			}
+		}
+	}
+}
+
 /**
  * ata_acpi_dissociate - dissociate ATA host from ACPI objects
  * @host: target ATA host

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

index f218427..adf002a 100644
@@ -6148,6 +6148,8 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht)
 	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];

drivers/ata/libata.h +2 -0

index c949dd3..577d902b 100644
@@ -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) { return 0; }
 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 */

Signature · 1 line

1.8.3.3.gfada522

Sergei Shtylyov

2013-06-20 11:02 UTC #6 lore raw

Hello.

Quoted 5 lines · Aaron Lu

Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
Quoted 5 lines
From: Liu Jiang <[email protected]>
Commit 30dcf76acc69 "libata: migrate ACPI code over to new bindings" removed ACPI dock notification related code, but there's some dead code left, so clean up it.

[...]

Quoted 3 lines

Subject: [PATCH] libata-acpi: add back ACPI based hotplug functionality
Commit 30dcf76acc mistakenly dropped the code to register hotplug

Please specify the summary line for this commit, the same as was
done by Liu above (may use parens instead of quotes).

Quoted 36 lines

notificaion handler for ATA port/devices, causing regression for people using ATA bay, as 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 introduced the ata_acpi_hotplug_init function to loop scan all ATA ACPI handles and if it is available, install the notification 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.
References: https://bugzilla.kernel.org/show_bug.cgi?id=59871
Reported-and-bisected-by Dirk Griesbach <[email protected]>
Cc <[email protected]>
Signed-off-by Aaron Lu <[email protected]>
---
  drivers/ata/libata-acpi.c | 34 +++++++++++++++++++++++++++++++++-
  drivers/ata/libata-core.c |  2 ++
  drivers/ata/libata.h      |  2 ++
  3 files changed, 37 insertions(+), 1 deletion(-)
drivers/ata/libata-acpi.c +0 -0
index 87f2f39..0ad54bb 100644

[...]

Quoted 11 lines

drivers/ata/libata-acpi.c +7 -0

@@ -214,6 +216,36 @@ static const struct acpi_dock_ops ata_acpi_ap_dock_ops = {
  	.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;

drivers/ata/libata-acpi.c static const struct acpi_dock_ops ata_acpi_ap_dock_ops = { · line 235

Quoted 10 lines

drivers/ata/libata-acpi.c +10 -0

+		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);

Please indent this line under the next character after ( above.

drivers/ata/libata-acpi.c static const struct acpi_dock_ops ata_acpi_ap_dock_ops = { · line 243

Quoted 8 lines

drivers/ata/libata-acpi.c +8 -0

+		}
+
+		ata_for_each_dev(dev, &ap->link, ALL) {
+			handle = ata_dev_acpi_handle(dev);
+			if (handle) {
+				/* we might be on a docking station */
+				register_hotplug_dock_device(handle,
+						&ata_acpi_dev_dock_ops, dev);

Same comment.

WBR, Sergei

Aaron Lu

2013-06-21 00:55 UTC #7 lore raw

Quoted 1 line · Sergei Shtylyov

Hello.

Hi,

Thanks for your comments.

Quoted 14 lines · Aaron Lu

Quoted 12 lines

drivers/ata/libata-acpi.c +11 -0

+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;

Quoted 8 lines

drivers/ata/libata-acpi.c static const struct acpi_dock_ops ata_acpi_ap_dock_ops = { · line 235
Quoted 6 lines
drivers/ata/libata-acpi.c +6 -0
+
+		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);
Please indent this line under the next character after ( above.

Is there a link about this rule? I might have missed something about coding style.

Thanks,
Aaron

Quoted 14 lines

drivers/ata/libata-acpi.c static const struct acpi_dock_ops ata_acpi_ap_dock_ops = { · line 243
Quoted 9 lines
drivers/ata/libata-acpi.c +8 -0
+		}
+
+		ata_for_each_dev(dev, &ap->link, ALL) {
+			handle = ata_dev_acpi_handle(dev);
+			if (handle) {
+				/* we might be on a docking station */
+				register_hotplug_dock_device(handle,
+						&ata_acpi_dev_dock_ops, dev);
Same comment.
WBR, Sergei

Tejun Heo

2013-06-21 06:29 UTC #8 lore raw

Hello,

Quoted 4 lines · Aaron Lu

Quoted 1 line
Please indent this line under the next character after ( above.
Is there a link about this rule? I might have missed something about coding style.

I don't think there's an explicit rule about that and I don't follow it religiously but I think it does make things easier on the eyes and emacs does that by default so I just got used to it. It's a very minor thing but FWIW I prefer things that way.

Thanks.

Signature · 1 line

tejun

Aaron Lu

2013-06-21 06:47 UTC #9 lore raw

Quoted 5 lines · Tejun Heo

Hello,
Quoted 2 lines · Aaron Lu
Quoted 1 line
Please indent this line under the next character after ( above.

Quoted 7 lines

Quoted 2 lines
Is there a link about this rule? I might have missed something about coding style.
I don't think there's an explicit rule about that and I don't follow it religiously but I think it does make things easier on the eyes and emacs does that by default so I just got used to it. It's a very minor thing but FWIW I prefer things that way.

OK, will refresh the patch and send out again.

Thanks,
Aaron

Sergei Shtylyov

2013-06-21 11:35 UTC #10 lore raw

In reply to Aaron Lu

Hello.

Quoted 20 lines · Aaron Lu

Quoted 11 lines
Quoted 11 lines
drivers/ata/libata-acpi.c +11 -0
+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;
Quoted 8 lines
drivers/ata/libata-acpi.c static const struct acpi_dock_ops ata_acpi_ap_dock_ops = { · line 235
Quoted 6 lines
drivers/ata/libata-acpi.c +6 -0
+
+		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);
Please indent this line under the next character after ( above.

Quoted 2 lines

Is there a link about this rule? I might have missed something about coding style.

Don't think so. This is a rule in some subsystems like networking,
and it's also the way Emacs does such things. So, in principle, you can ignore my comment (although libata seems to also use this style).

WBR, Sergei

Aaron Lu

2013-06-26 06:27 UTC #11 lore raw

Quoted 24 lines · Sergei Shtylyov

Hello.
Quoted 19 lines · Aaron Lu
Quoted 19 lines
Quoted 7 lines
drivers/ata/libata-acpi.c +7 -0
+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;
drivers/ata/libata-acpi.c static const struct acpi_dock_ops ata_acpi_ap_dock_ops = { · line 235
Quoted 10 lines
drivers/ata/libata-acpi.c +10 -0
+		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);
Please indent this line under the next character after ( above.

Quoted 6 lines

Quoted 2 lines
Is there a link about this rule? I might have missed something about coding style.
Don't think so. This is a rule in some subsystems like networking,
and it's also the way Emacs does such things. So, in principle, you can ignore my comment (although libata seems to also use this style).

FYI, the updated patch has been sent by Rafael for me:
http://marc.info/?l=linux-acpi&m=137207724507019&w=2
with your suggestion taken.

Thanks,
Aaron

James Bottomley

2013-06-21 15:25 UTC #12 lore raw

In reply to Aaron Lu

Quoted 30 lines · Aaron Lu

Quoted 1 line · Sergei Shtylyov
Hello.
Hi,
Thanks for your comments.
Quoted 14 lines · Aaron Lu
Quoted 12 lines
drivers/ata/libata-acpi.c +11 -0
+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;
Quoted 8 lines
drivers/ata/libata-acpi.c static const struct acpi_dock_ops ata_acpi_ap_dock_ops = { · line 235
Quoted 6 lines
drivers/ata/libata-acpi.c +6 -0
+
+		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);
Please indent this line under the next character after ( above.

Quoted 2 lines

Is there a link about this rule? I might have missed something about coding style.

The rule is follow the coding style in the file, unless there's something really wrong with it (which there might be in the case of really old drivers). The reason is that a mixture of coding styles makes the file much harder to read than a single consistent style.

In this case, if you look at libata-acpi.c you see all continuation lines of function calls are aligned with open braces.

James

Aaron Lu

2013-06-26 02:01 UTC #13 lore raw

Quoted 31 lines · James Bottomley

Quoted 30 lines · Aaron Lu
Quoted 1 line · Sergei Shtylyov
Hello.
Hi,
Thanks for your comments.
Quoted 22 lines · Aaron Lu
Quoted 8 lines
drivers/ata/libata-acpi.c +7 -0
+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;
drivers/ata/libata-acpi.c static const struct acpi_dock_ops ata_acpi_ap_dock_ops = { · line 235
Quoted 10 lines
drivers/ata/libata-acpi.c +10 -0
+		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);
Please indent this line under the next character after ( above.

Quoted 7 lines

Quoted 2 lines
Is there a link about this rule? I might have missed something about coding style.
The rule is follow the coding style in the file, unless there's something really wrong with it (which there might be in the case of really old drivers). The reason is that a mixture of coding styles makes the file much harder to read than a single consistent style.

Oh right, that's the rule I missed. Thanks for letting me know.

Quoted 3 lines

In this case, if you look at libata-acpi.c you see all continuation lines of function calls are aligned with open braces.

Indeed.

-Aaron