ide: ide_port_wait_ready() fix

PATCH In review Bartlomiej Zolnierkiewicz 2 participants 7 messages Oct 11, 2011 – Oct 13, 2011 libata

lore

Share & tools

lore

Bartlomiej Zolnierkiewicz

author

2011-10-11 17:13 UTC #0 lore raw

PATCH 1/1 ide: ide_port_wait_ready() fix

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: ide_port_wait_ready() fix

Fix for commit a20b2a4 ("ide: skip probe if there are no devices on the port (v2)"). We must check for slave device before failing.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
 drivers/ide/ide-probe.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

drivers/ide/ide-probe.c +4 -2

Index: b/drivers/ide/ide-probe.c
===================================================================
@@ -598,7 +598,7 @@ static int ide_port_wait_ready(ide_hwif_
 {
 	const struct ide_tp_ops *tp_ops = hwif->tp_ops;
 	ide_drive_t *drive;
-	int i, rc;
+	int i, rc, prev_rc = 0;
 
 	printk(KERN_DEBUG "Probing IDE interface %s...\n", hwif->name);

@@ -623,8 +623,10 @@ static int ide_port_wait_ready(ide_hwif_
 			tp_ops->write_devctl(hwif, ATA_DEVCTL_OBS);
 			mdelay(2);
 			rc = ide_wait_not_busy(hwif, 35000);
-			if (rc)
+			if (prev_rc && rc)
 				goto out;
+			prev_rc = rc;
+			rc = 0;
 		} else
 			printk(KERN_DEBUG "%s: ide_wait_not_busy() skipped\n",
 					  drive->name);

David Miller

2011-10-11 19:17 UTC #1 lore raw

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 11 Oct 2011 19:13:18 +0200

Quoted 7 lines

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: ide_port_wait_ready() fix
Fix for commit a20b2a4 ("ide: skip probe if there are no devices on the port (v2)"). We must check for slave device before failing.
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

This will mishandle the case where there is no slave in the device list.

Bartlomiej Zolnierkiewicz

author

2011-10-12 14:59 UTC #2 lore raw

Quoted 6 lines · David Miller

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 11 Oct 2011 19:13:18 +0200
Quoted 3 lines
From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: ide_port_wait_ready() fix

Quoted 7 lines

Quoted 4 lines
Fix for commit a20b2a4 ("ide: skip probe if there are no devices on the port (v2)"). We must check for slave device before failing.
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
This will mishandle the case where there is no slave in the device list.

I don't see it:

@ -598,7 +598,7 @@ static int ide_port_wait_ready(ide_hwif_
{
const struct ide_tp_ops *tp_ops = hwif->tp_ops; ide_drive_t *drive;
- int i, rc;
+ int i, rc, prev_rc = 0;

printk(KERN_DEBUG "Probing IDE interface %s...\n", hwif->name);

@@ -623,8 +623,10 @@ static int ide_port_wait_ready(ide_hwif_
tp_ops->write_devctl(hwif, ATA_DEVCTL_OBS); mdelay(2);
rc = ide_wait_not_busy(hwif, 35000);
- if (rc)
+ if (prev_rc && rc)
goto out;
+ prev_rc = rc;
+ rc = 0;
} else
printk(KERN_DEBUG "%s: ide_wait_not_busy() skipped\n",
drive->name);

If there is no slave device but there is a master device the code falls-through and returns a success.

The patch fixes regression introduced in commit a20b2a4 as some esoteric setups return ide_wait_not_busy() -ENODEV error on master device while there is slave device present in the system.

David Miller

2011-10-12 19:03 UTC #3 lore raw

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Wed, 12 Oct 2011 16:59:55 +0200

Quoted 40 lines

Quoted 6 lines · David Miller
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 11 Oct 2011 19:13:18 +0200
Quoted 3 lines
From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: ide_port_wait_ready() fix
Quoted 7 lines
Quoted 4 lines
Fix for commit a20b2a4 ("ide: skip probe if there are no devices on the port (v2)"). We must check for slave device before failing.
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
This will mishandle the case where there is no slave in the device list.
I don't see it:
@ -598,7 +598,7 @@ static int ide_port_wait_ready(ide_hwif_
{
drivers/ide/ide-probe.c +4 -2
 	const struct ide_tp_ops *tp_ops = hwif->tp_ops;
 	ide_drive_t *drive;
-	int i, rc;
+	int i, rc, prev_rc = 0;
 
 	printk(KERN_DEBUG "Probing IDE interface %s...\n", hwif->name);
@@ -623,8 +623,10 @@ static int ide_port_wait_ready(ide_hwif_
 			tp_ops->write_devctl(hwif, ATA_DEVCTL_OBS);
 			mdelay(2);
 			rc = ide_wait_not_busy(hwif, 35000);
-			if (rc)
+			if (prev_rc && rc)
 				goto out;
+			prev_rc = rc;
+			rc = 0;
 		} else
 			printk(KERN_DEBUG "%s: ide_wait_not_busy() skipped\n",
 					  drive->name);

Quoted 2 lines

If there is no slave device but there is a master device the code falls-through and returns a success.

That's not what we want, if there is only a master device and no slave device in the list this loop is iterating over we want to return the error code in "rc", not zero.

Bartlomiej Zolnierkiewicz

author

2011-10-13 10:41 UTC #4 lore raw

Quoted 43 lines · David Miller

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Wed, 12 Oct 2011 16:59:55 +0200
Quoted 40 lines
Quoted 6 lines · David Miller
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 11 Oct 2011 19:13:18 +0200
Quoted 3 lines
From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: ide_port_wait_ready() fix
Quoted 7 lines
Quoted 4 lines
Fix for commit a20b2a4 ("ide: skip probe if there are no devices on the port (v2)"). We must check for slave device before failing.
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
This will mishandle the case where there is no slave in the device list.
I don't see it:
@ -598,7 +598,7 @@ static int ide_port_wait_ready(ide_hwif_
{
drivers/ide/ide-probe.c +4 -2
 	const struct ide_tp_ops *tp_ops = hwif->tp_ops;
 	ide_drive_t *drive;
-	int i, rc;
+	int i, rc, prev_rc = 0;
 
 	printk(KERN_DEBUG "Probing IDE interface %s...\n", hwif->name);
@@ -623,8 +623,10 @@ static int ide_port_wait_ready(ide_hwif_
 			tp_ops->write_devctl(hwif, ATA_DEVCTL_OBS);
 			mdelay(2);
 			rc = ide_wait_not_busy(hwif, 35000);
-			if (rc)
+			if (prev_rc && rc)
 				goto out;
+			prev_rc = rc;
+			rc = 0;
 		} else
 			printk(KERN_DEBUG "%s: ide_wait_not_busy() skipped\n",
 					  drive->name);

Quoted 6 lines

Quoted 2 lines
If there is no slave device but there is a master device the code falls-through and returns a success.
That's not what we want, if there is only a master device and no slave device in the list this loop is iterating over we want to return the error code in "rc", not zero.

No, we want to return zero (success) since at least once device was found (otherwise we fail probe on some esoteric setups returning -ENODEV from ide_wait_not_busy() for master device).

This is how this function worked before commit a20b2a4 if you want something else okay but it needs to work with aforementioned setups.

David Miller

2011-10-13 17:25 UTC #5 lore raw

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Thu, 13 Oct 2011 12:41:04 +0200

Quoted 52 lines

Quoted 43 lines · David Miller
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Wed, 12 Oct 2011 16:59:55 +0200
Quoted 40 lines
Quoted 6 lines · David Miller
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 11 Oct 2011 19:13:18 +0200
Quoted 3 lines
From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: ide_port_wait_ready() fix
Quoted 7 lines
Quoted 4 lines
Fix for commit a20b2a4 ("ide: skip probe if there are no devices on the port (v2)"). We must check for slave device before failing.
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
This will mishandle the case where there is no slave in the device list.
I don't see it:
@ -598,7 +598,7 @@ static int ide_port_wait_ready(ide_hwif_
{
drivers/ide/ide-probe.c +4 -2
 	const struct ide_tp_ops *tp_ops = hwif->tp_ops;
 	ide_drive_t *drive;
-	int i, rc;
+	int i, rc, prev_rc = 0;
 
 	printk(KERN_DEBUG "Probing IDE interface %s...\n", hwif->name);
@@ -623,8 +623,10 @@ static int ide_port_wait_ready(ide_hwif_
 			tp_ops->write_devctl(hwif, ATA_DEVCTL_OBS);
 			mdelay(2);
 			rc = ide_wait_not_busy(hwif, 35000);
-			if (rc)
+			if (prev_rc && rc)
 				goto out;
+			prev_rc = rc;
+			rc = 0;
 		} else
 			printk(KERN_DEBUG "%s: ide_wait_not_busy() skipped\n",
 					  drive->name);
Quoted 6 lines
Quoted 2 lines
If there is no slave device but there is a master device the code falls-through and returns a success.
That's not what we want, if there is only a master device and no slave device in the list this loop is iterating over we want to return the error code in "rc", not zero.

Quoted 6 lines

No, we want to return zero (success) since at least once device was found (otherwise we fail probe on some esoteric setups returning -ENODEV from ide_wait_not_busy() for master device).
This is how this function worked before commit a20b2a4 if you want something else okay but it needs to work with aforementioned setups.

You unconditionally assign "prev_rc = rc" and set "rc = 0" so if we only run the loop once, we return zero.

And we do this even if that one device gave a non-zero return value.

That's not what we want.

Bartlomiej Zolnierkiewicz

author

2011-10-13 17:44 UTC #6 lore raw

Quoted 62 lines · David Miller

From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Thu, 13 Oct 2011 12:41:04 +0200
Quoted 52 lines
Quoted 43 lines · David Miller
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Wed, 12 Oct 2011 16:59:55 +0200
Quoted 40 lines
Quoted 6 lines · David Miller
From: Bartlomiej Zolnierkiewicz <[email protected]>
Date: Tue, 11 Oct 2011 19:13:18 +0200
Quoted 3 lines
From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH] ide: ide_port_wait_ready() fix
Quoted 7 lines
Quoted 4 lines
Fix for commit a20b2a4 ("ide: skip probe if there are no devices on the port (v2)"). We must check for slave device before failing.
Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>
This will mishandle the case where there is no slave in the device list.
I don't see it:
@ -598,7 +598,7 @@ static int ide_port_wait_ready(ide_hwif_
{
(quoted diff) +4 -2
 	const struct ide_tp_ops *tp_ops = hwif->tp_ops;
 	ide_drive_t *drive;
-	int i, rc;
+	int i, rc, prev_rc = 0;
 
 	printk(KERN_DEBUG "Probing IDE interface %s...\n", hwif->name);
@@ -623,8 +623,10 @@ static int ide_port_wait_ready(ide_hwif_
 			tp_ops->write_devctl(hwif, ATA_DEVCTL_OBS);
 			mdelay(2);
 			rc = ide_wait_not_busy(hwif, 35000);
-			if (rc)
+			if (prev_rc && rc)
 				goto out;
+			prev_rc = rc;
+			rc = 0;
 		} else
 			printk(KERN_DEBUG "%s: ide_wait_not_busy() skipped\n",
 					  drive->name);
Quoted 6 lines
Quoted 2 lines
If there is no slave device but there is a master device the code falls-through and returns a success.
That's not what we want, if there is only a master device and no slave device in the list this loop is iterating over we want to return the error code in "rc", not zero.
Quoted 6 lines
No, we want to return zero (success) since at least once device was found (otherwise we fail probe on some esoteric setups returning -ENODEV from ide_wait_not_busy() for master device).
This is how this function worked before commit a20b2a4 if you want something else okay but it needs to work with aforementioned setups.

Quoted 6 lines

You unconditionally assign "prev_rc = rc" and set "rc = 0" so if we only run the loop once, we return zero.
And we do this even if that one device gave a non-zero return value.
That's not what we want.

Here is revised patch:

From: Bartlomiej Zolnierkiewicz <[email protected]>
Subject: [PATCH v2] ide: ide_port_wait_ready() fix

Fix for commit a20b2a4 ("ide: skip probe if there are no devices on the port (v2)"). We must check for slave device before failing.

Signed-off-by Bartlomiej Zolnierkiewicz <[email protected]>

---
 drivers/ide/ide-probe.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

drivers/ide/ide-probe.c +3 -2

Index: b/drivers/ide/ide-probe.c
===================================================================
@@ -598,7 +598,7 @@ static int ide_port_wait_ready(ide_hwif_
 {
 	const struct ide_tp_ops *tp_ops = hwif->tp_ops;
 	ide_drive_t *drive;
-	int i, rc;
+	int i, rc, prev_rc = 0;
 
 	printk(KERN_DEBUG "Probing IDE interface %s...\n", hwif->name);

@@ -623,8 +623,9 @@ static int ide_port_wait_ready(ide_hwif_
 			tp_ops->write_devctl(hwif, ATA_DEVCTL_OBS);
 			mdelay(2);
 			rc = ide_wait_not_busy(hwif, 35000);
-			if (rc)
+			if (prev_rc && rc)
 				goto out;
+			prev_rc = rc;
 		} else
 			printk(KERN_DEBUG "%s: ide_wait_not_busy() skipped\n",
 					  drive->name);