IDE fixes

GIT PULL Bartlomiej Zolnierkiewicz 4 participants 9 messages Apr 22, 2009 – Apr 23, 2009

Subsystems (2)

libata Linux for power macintosh

lore

Share & tools

lore

Bartlomiej Zolnierkiewicz

author

2009-04-22 18:45 UTC #0 lore raw

[ mediabay.c changes are limited to <linux/ide.h> include removal ]

Linus, please pull from 'for-linus' branch of:

master.kernel.org:/pub/scm/linux/kernel/git/bart/ide-2.6.git for-linus

to receive the following updates:

 drivers/ide/ide-cd.c         |    2 +-
 drivers/ide/ide-gd.c         |   17 ++++++++++++++++-
 drivers/ide/palm_bk3710.c    |   26 ++++++++++++--------------
 drivers/macintosh/mediabay.c |    1 -
 4 files changed, 29 insertions(+), 17 deletions(-)

Bartlomiej Zolnierkiewicz (1):
mediabay: fix build for CONFIG_BLOCK=n

Bruno Prémont (1):
ide: Stop disks on reboot for laptop which cuts power

David Brownell (1):
palm_bk3710: UDMA performance fix

Helge Deller (1):
ide-cd: fix kernel crash on hppa regression

drivers/ide/ide-cd.c +1 -1

index 3aec19d..3d4e099 100644
@@ -609,7 +609,7 @@ static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive)
 	struct request *rq = hwif->rq;
 	ide_expiry_t *expiry = NULL;
 	int dma_error = 0, dma, thislen, uptodate = 0;
-	int write = (rq_data_dir(rq) == WRITE) ? 1 : 0, rc, nsectors;
+	int write = (rq_data_dir(rq) == WRITE) ? 1 : 0, rc = 0, nsectors;
 	int sense = blk_sense_request(rq);
 	unsigned int timeout;
 	u16 len;

drivers/ide/ide-gd.c +16 -1

index 1aebdf1..4b6b71e 100644
@@ -7,6 +7,7 @@
 #include <linux/mutex.h>
 #include <linux/ide.h>
 #include <linux/hdreg.h>
+#include <linux/dmi.h>
 
 #if !defined(CONFIG_DEBUG_BLOCK_EXT_DEVT)
 #define IDE_DISK_MINORS		(1 << PARTN_BITS)

@@ -99,6 +100,19 @@ static void ide_gd_resume(ide_drive_t *drive)
 		(void)drive->disk_ops->get_capacity(drive);
 }
 
+static const struct dmi_system_id ide_coldreboot_table[] = {
+	{
+		/* Acer TravelMate 66x cuts power during reboot */
+		.ident   = "Acer TravelMate 660",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Acer"),
+			DMI_MATCH(DMI_PRODUCT_NAME, "TravelMate 660"),
+		},
+	},
+
+	{ }	/* terminate list */
+};
+
 static void ide_gd_shutdown(ide_drive_t *drive)
 {
 #ifdef	CONFIG_ALPHA

@@ -115,7 +129,8 @@ static void ide_gd_shutdown(ide_drive_t *drive)
 	   the disk to expire its write cache. */
 	if (system_state != SYSTEM_POWER_OFF) {
 #else
-	if (system_state == SYSTEM_RESTART) {
+	if (system_state == SYSTEM_RESTART &&
+		!dmi_check_system(ide_coldreboot_table)) {
 #endif
 		drive->disk_ops->flush(drive);
 		return;

drivers/ide/palm_bk3710.c +12 ~~-14~~

index c7acca0..d1513b4 100644
@@ -39,14 +39,6 @@
 /* Primary Control Offset */
 #define IDE_PALM_ATA_PRI_CTL_OFFSET 0x3F6
 
-/*
- * PalmChip 3710 IDE Controller UDMA timing structure Definition
- */
-struct palm_bk3710_udmatiming {
-	unsigned int rptime;	/* Ready to pause time  */
-	unsigned int cycletime;	/* Cycle Time           */
-};
-
 #define BK3710_BMICP		0x00
 #define BK3710_BMISP		0x02
 #define BK3710_BMIDTP		0x04

@@ -75,13 +67,19 @@ struct palm_bk3710_udmatiming {
 
 static unsigned ideclk_period; /* in nanoseconds */
 
+struct palm_bk3710_udmatiming {
+	unsigned int rptime;	/* tRP -- Ready to pause time (nsec) */
+	unsigned int cycletime;	/* tCYCTYP2/2 -- avg Cycle Time (nsec) */
+				/* tENV is always a minimum of 20 nsec */
+};
+
 static const struct palm_bk3710_udmatiming palm_bk3710_udmatimings[6] = {
-	{160, 240},		/* UDMA Mode 0 */
-	{125, 160},		/* UDMA Mode 1 */
-	{100, 120},		/* UDMA Mode 2 */
-	{100, 90},		/* UDMA Mode 3 */
-	{100, 60},		/* UDMA Mode 4 */
-	{85,  40},		/* UDMA Mode 5 */
+	{160, 240 / 2,},	/* UDMA Mode 0 */
+	{125, 160 / 2,},	/* UDMA Mode 1 */
+	{100, 120 / 2,},	/* UDMA Mode 2 */
+	{100, 90 / 2,},		/* UDMA Mode 3 */
+	{100, 60 / 2,},		/* UDMA Mode 4 */
+	{85,  40 / 2,},		/* UDMA Mode 5 */
 };
 
 static void palm_bk3710_setudmamode(void __iomem *base, unsigned int dev,

drivers/macintosh/mediabay.c +0 -1

index d7e46d3..eca55ef 100644
@@ -18,7 +18,6 @@
 #include <linux/timer.h>
 #include <linux/stddef.h>
 #include <linux/init.h>
-#include <linux/ide.h>
 #include <linux/kthread.h>
 #include <linux/mutex.h>
 #include <asm/prom.h>

Sam Ravnborg

2009-04-22 19:20 UTC #2 lore raw

Quoted 19 lines · Joe Perches

drivers/ide/ide-cd.c static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive) · line 615
Quoted 13 lines · Bartlomiej Zolnierkiewicz
drivers/ide/ide-cd.c +1 -1
index 3aec19d..3d4e099 100644
@@ -609,7 +609,7 @@ static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive)
 	struct request *rq = hwif->rq;
 	ide_expiry_t *expiry = NULL;
 	int dma_error = 0, dma, thislen, uptodate = 0;
-	int write = (rq_data_dir(rq) == WRITE) ? 1 : 0, rc, nsectors;
+	int write = (rq_data_dir(rq) == WRITE) ? 1 : 0, rc = 0, nsectors;
 	int sense = blk_sense_request(rq);
 	unsigned int timeout;
 	u16 len;
I think ide is the only subsystem to use the
initialization style of "?:," with additional
declarations.

Quoted 5 lines

Would it be better to use a more standard style?
maybe:
	int dma_error = 0, dma, thislen, uptodate = 0, rc = 0, nsectors;
	int write = ((rq_data_dir(rq) == WRITE) ? 1 : 0;

Better to move assignment after variable definition.

Sam

Bartlomiej Zolnierkiewicz

author

2009-04-22 19:40 UTC #3 lore raw

In reply to Joe Perches

Quoted 10 lines · On Wednesday 22 April 2009 21:06:47 Joe Perches wrote:

Quoted 9 lines · Bartlomiej Zolnierkiewicz

drivers/ide/ide-cd.c +0 -1

index 3aec19d..3d4e099 100644
@@ -609,7 +609,7 @@ static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive)
 	struct request *rq = hwif->rq;
 	ide_expiry_t *expiry = NULL;
 	int dma_error = 0, dma, thislen, uptodate = 0;
-	int write = (rq_data_dir(rq) == WRITE) ? 1 : 0, rc, nsectors;

Quoted 8 lines

drivers/ide/ide-cd.c static ide_startstop_t cdrom_newpc_intr(ide_drive_t *drive) · line 615
Quoted 4 lines
drivers/ide/ide-cd.c +1 -0
+	int write = (rq_data_dir(rq) == WRITE) ? 1 : 0, rc = 0, nsectors;
 	int sense = blk_sense_request(rq);
 	unsigned int timeout;
 	u16 len;
I think ide is the only subsystem to use the
initialization style of "?:," with additional
declarations.

Well, nothing wrong with it per se.

Quoted 5 lines

Would it be better to use a more standard style?
maybe:
	int dma_error = 0, dma, thislen, uptodate = 0, rc = 0, nsectors;
	int write = ((rq_data_dir(rq) == WRITE) ? 1 : 0;

Personally I would prefer:

	int write = !!(rq_data_dir(rq) == WRITE);

However the old code is also fine with me.

Quoted 30 lines

drivers/ide/palm_bk3710.c

Quoted 4 lines

drivers/ide/palm_bk3710.c +0 -0
index c7acca0..d1513b4 100644

[]

Quoted 25 lines

drivers/ide/palm_bk3710.c +12 -6

@@ -75,13 +67,19 @@ struct palm_bk3710_udmatiming {
 
 static unsigned ideclk_period; /* in nanoseconds */
 
+struct palm_bk3710_udmatiming {
+	unsigned int rptime;	/* tRP -- Ready to pause time (nsec) */
+	unsigned int cycletime;	/* tCYCTYP2/2 -- avg Cycle Time (nsec) */
+				/* tENV is always a minimum of 20 nsec */
+};
+
 static const struct palm_bk3710_udmatiming palm_bk3710_udmatimings[6] = {
-	{160, 240},		/* UDMA Mode 0 */
-	{125, 160},		/* UDMA Mode 1 */
-	{100, 120},		/* UDMA Mode 2 */
-	{100, 90},		/* UDMA Mode 3 */
-	{100, 60},		/* UDMA Mode 4 */
-	{85,  40},		/* UDMA Mode 5 */
+	{160, 240 / 2,},	/* UDMA Mode 0 */
+	{125, 160 / 2,},	/* UDMA Mode 1 */
+	{100, 120 / 2,},	/* UDMA Mode 2 */
+	{100, 90 / 2,},		/* UDMA Mode 3 */
+	{100, 60 / 2,},		/* UDMA Mode 4 */
+	{85,  40 / 2,},		/* UDMA Mode 5 */
 };

Quoted 8 lines

drivers/ide/palm_bk3710.c struct palm_bk3710_udmatiming { · line 85
Quoted 1 line
drivers/ide/palm_bk3710.c +0 -0
 static void palm_bk3710_setudmamode(void __iomem *base, unsigned int dev,
Odd looking commas.
Maybe:
	{.rptime = 160, .cycletime = 240 / 2 },	/* UDMA Mode 0 */
etc.

I really don't consider this an improvement from readability perspective and I also don't like the added code duplication...

IMO we should just remove extra commas and add some whitespaces.

I have also more general (process oriented) comment:

All patches have been posted to linux-kernel or linux-ide for review before and it is _much_ more efficient to raise issues (including CodingStyle ones) during "review phase" instead of during "push to Linus" phase.

Thanks,
Bart

Joe Perches

2009-04-22 19:56 UTC #4 lore raw

Quoted 1 line · Bartlomiej Zolnierkiewicz

IMO we should just remove extra commas and add some whitespaces.

Your choice.
Ignore, reject, accept, improve, fine by me.

Quoted 5 lines

I have also more general (process oriented) comment:
All patches have been posted to linux-kernel or linux-ide for review before and it is _much_ more efficient to raise issues (including CodingStyle ones) during "review phase" instead of during "push to Linus" phase.

Maybe for you, but I'm an intermittent
and random reader.

If you accept an idea at all, queue it
up at your convenience.

cheers, Joe

Bartlomiej Zolnierkiewicz

author

2009-04-22 21:38 UTC #5 lore raw

Quoted 6 lines · On Wednesday 22 April 2009 21:55:03 Joe Perches wrote:

Quoted 1 line · Bartlomiej Zolnierkiewicz
IMO we should just remove extra commas and add some whitespaces.
Your choice.
Ignore, reject, accept, improve, fine by me.

Quoted 8 lines

Quoted 5 lines
I have also more general (process oriented) comment:
All patches have been posted to linux-kernel or linux-ide for review before and it is _much_ more efficient to raise issues (including CodingStyle ones) during "review phase" instead of during "push to Linus" phase.
Maybe for you, but I'm an intermittent
and random reader.

For everybody involved in the process -- this includes you.

I'm not the original author of many patches so I simply cannot be correcting every single issue because:

a) it is physically impossible

b) it doesn't really educate people

Thanks,
Bart

Ray Lee

2009-04-22 22:02 UTC #6 lore raw

Quoted 16 lines · Bartlomiej Zolnierkiewicz

Quoted 6 lines · On Wednesday 22 April 2009 21:55:03 Joe Perches wrote:
Quoted 1 line · Bartlomiej Zolnierkiewicz
IMO we should just remove extra commas and add some whitespaces.
Your choice.
Ignore, reject, accept, improve, fine by me.
Quoted 8 lines
Quoted 5 lines
I have also more general (process oriented) comment:
All patches have been posted to linux-kernel or linux-ide for review before and it is _much_ more efficient to raise issues (including CodingStyle ones) during "review phase" instead of during "push to Linus" phase.
Maybe for you, but I'm an intermittent
and random reader.

Quoted 8 lines

For everybody involved in the process -- this includes you.
I'm not the original author of many patches so I simply cannot be correcting every single issue because:
a) it is physically impossible
b) it doesn't really educate people

His point is that you can queue a follow-up patch to remove the ugliness. Just because it didn't get caught until the push stage doesn't mean the code needs to stay set in stone for the rest of eternity.

What most maintainers do at this stage is say "Great idea! Send me a patch." Perhaps you could try that tack instead.

Bartlomiej Zolnierkiewicz

author

2009-04-22 22:55 UTC #7 lore raw

Quoted 23 lines · On Thursday 23 April 2009 00:02:38 Ray Lee wrote:

Quoted 21 lines · Bartlomiej Zolnierkiewicz
Quoted 6 lines · On Wednesday 22 April 2009 21:55:03 Joe Perches wrote:
Quoted 1 line · Bartlomiej Zolnierkiewicz
IMO we should just remove extra commas and add some whitespaces.
Your choice.
Ignore, reject, accept, improve, fine by me.
Quoted 8 lines
Quoted 5 lines
I have also more general (process oriented) comment:
All patches have been posted to linux-kernel or linux-ide for review before and it is _much_ more efficient to raise issues (including CodingStyle ones) during "review phase" instead of during "push to Linus" phase.
Maybe for you, but I'm an intermittent
and random reader.
For everybody involved in the process -- this includes you.
I'm not the original author of many patches so I simply cannot be correcting every single issue because:

Quoted 8 lines

Quoted 3 lines
a) it is physically impossible
b) it doesn't really educate people
His point is that you can queue a follow-up patch to remove the ugliness. Just because it didn't get caught until the push stage doesn't mean the code needs to stay set in stone for the rest of eternity.

I get his point and of course patches are welcomed.

However my point is that it is still suboptimal way to address the issue in the long-term and from my perspective the long-term is what really matters.

Thanks,
Bart

Bartlomiej Zolnierkiewicz

author

2009-04-23 12:21 UTC #8 lore raw

In reply to Ray Lee

Quoted 23 lines · On Thursday 23 April 2009 00:02:38 Ray Lee wrote:

Quoted 21 lines · Bartlomiej Zolnierkiewicz
Quoted 6 lines · On Wednesday 22 April 2009 21:55:03 Joe Perches wrote:
Quoted 1 line · Bartlomiej Zolnierkiewicz
IMO we should just remove extra commas and add some whitespaces.
Your choice.
Ignore, reject, accept, improve, fine by me.
Quoted 8 lines
Quoted 5 lines
I have also more general (process oriented) comment:
All patches have been posted to linux-kernel or linux-ide for review before and it is _much_ more efficient to raise issues (including CodingStyle ones) during "review phase" instead of during "push to Linus" phase.
Maybe for you, but I'm an intermittent
and random reader.
For everybody involved in the process -- this includes you.
I'm not the original author of many patches so I simply cannot be correcting every single issue because:

Quoted 8 lines

Quoted 3 lines
a) it is physically impossible
b) it doesn't really educate people
His point is that you can queue a follow-up patch to remove the ugliness. Just because it didn't get caught until the push stage doesn't mean the code needs to stay set in stone for the rest of eternity.

I get his point and of course patches are welcomed.

However my point is that it is still suboptimal way to address the issue in the long-term and from my perspective the long-term is what really matters.

Thanks,
Bart