alim15x3: ->speedproc, filter out invalid modes passed from user-space

PATCH In review Bartlomiej Zolnierkiewicz 4 participants 12 messages Jun 30, 2007 – Jul 2, 2007 libata

lore

Share & tools

lore

Bartlomiej Zolnierkiewicz

author

2007-06-30 19:00 UTC #0 lore raw

PATCH 12/15 alim15x3: ->speedproc, filter out invalid modes passed from user-space

* ->speedproc, filter out invalid modes passed from user-space.

* Add FIXME about DMA timings never being set.

* Bump driver version.

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

---
 drivers/ide/pci/alim15x3.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

drivers/ide/pci/alim15x3.c +15 -0

Index: b/drivers/ide/pci/alim15x3.c
===================================================================
@@ -425,6 +425,17 @@ static int ali15x3_tune_chipset (ide_dri
 	u8 tmpbyte		= 0x00;
 	int m5229_udma		= (hwif->channel) ? 0x57 : 0x56;
 
+	/*
+	 * Paranoia, filter out invalid modes passed from user-space
+	 * (unsupported UDMA modes are dealt by ide_rate_filter() call).
+	 *
+	 * This will go away once ide_set_xfer() is fixed.
+	 */
+	if ((speed > XFER_PIO_5 && speed < XFER_SW_DMA_0) ||
+	    (speed > XFER_SW_DMA_2 && speed < XFER_MW_DMA_0) ||
+	    (speed > XFER_MW_DMA_2 && speed < XFER_UDMA_0))
+		return -1;
+
 	if (speed == XFER_UDMA_6)
 		speed1 = 0x47;

@@ -437,6 +448,10 @@ static int ali15x3_tune_chipset (ide_dri
 		tmpbyte &= ultra_enable;
 		pci_write_config_byte(dev, m5229_udma, tmpbyte);
 
+		/*
+		 * FIXME: Oh, my... DMA timings are never set.
+		 */
+
 		if (speed < XFER_SW_DMA_0)
 			(void) ali15x3_tune_pio(drive, speed - XFER_PIO_0);
 	} else {

Bartlomiej Zolnierkiewicz

author

2007-07-02 17:50 UTC #2 lore raw

Quoted 35 lines · Sergei Shtylyov

Quoted 34 lines · Bartlomiej Zolnierkiewicz

* ->speedproc, filter out invalid modes passed from user-space.

* Add FIXME about DMA timings never being set.

* Bump driver version.

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

Index: b/drivers/ide/pci/alim15x3.c
===================================================================

drivers/ide/pci/alim15x3.c +11 -0

@@ -425,6 +425,17 @@ static int ali15x3_tune_chipset (ide_dri
 	u8 tmpbyte		= 0x00;
 	int m5229_udma		= (hwif->channel) ? 0x57 : 0x56;
 
+	/*
+	 * Paranoia, filter out invalid modes passed from user-space
+	 * (unsupported UDMA modes are dealt by ide_rate_filter() call).
+	 *
+	 * This will go away once ide_set_xfer() is fixed.
+	 */
+	if ((speed > XFER_PIO_5 && speed < XFER_SW_DMA_0) ||
+	    (speed > XFER_SW_DMA_2 && speed < XFER_MW_DMA_0) ||
+	    (speed > XFER_MW_DMA_2 && speed < XFER_UDMA_0))
+		return -1;
+
 	if (speed == XFER_UDMA_6)
 		speed1 = 0x47;

@@ -437,6 +448,10 @@ static int ali15x3_tune_chipset (ide_dri
 		tmpbyte &= ultra_enable;
 		pci_write_config_byte(dev, m5229_udma, tmpbyte);

Quoted 6 lines

drivers/ide/pci/alim15x3.c static int ali15x3_tune_chipset (ide_dri · line 454
Quoted 4 lines
drivers/ide/pci/alim15x3.c +4 -0
+		/*
+		 * FIXME: Oh, my... DMA timings are never set.
+		 */
+
Erm, wouldn't it have been better to mark them as unsupported while at it?

This would brake setups which currently work OK, i.e. BIOS set things up (reminds me about cmd64x vs broken MWDMA)...

The RightThing(tm) to do is to fix alim15x3 driver to program DMA timings (especially given that pata_ali seems to already contain the needed code).

Thanks,
Bart

Jeff Garzik

2007-07-02 18:00 UTC #3 lore raw

Quoted 5 lines · Bartlomiej Zolnierkiewicz

This would brake setups which currently work OK, i.e. BIOS set things up (reminds me about cmd64x vs broken MWDMA)...
The RightThing(tm) to do is to fix alim15x3 driver to program DMA timings (especially given that pata_ali seems to already contain the needed code).

I am not ready to trust that pata_ali works as well as alim15x3 in all cases. Someone should test e.g. Alpha AXP systems with IDE (use alim15x3) to make sure all is well.

alim15x3 covers buggy, ugly, quirky chipsets. I consider that coverage in general highly fragile.

Jeff

Bartlomiej Zolnierkiewicz

author

2007-07-02 18:24 UTC #4 lore raw

Quoted 10 lines · Jeff Garzik

Quoted 5 lines · Bartlomiej Zolnierkiewicz
This would brake setups which currently work OK, i.e. BIOS set things up (reminds me about cmd64x vs broken MWDMA)...
The RightThing(tm) to do is to fix alim15x3 driver to program DMA timings (especially given that pata_ali seems to already contain the needed code).
I am not ready to trust that pata_ali works as well as alim15x3 in all cases. Someone should test e.g. Alpha AXP systems with IDE (use alim15x3) to make sure all is well.

Sure but there is no problem with the new code in alim15x3 being X86 until it is verified to work with Alpha AXP machines etc...

Quoted 2 lines

alim15x3 covers buggy, ugly, quirky chipsets. I consider that coverage in general highly fragile.

and in case of MWDMA modes also highly dependent on BIOS settings which very likely results in broken suspend/resume...

Thanks,
Bart

Jeff Garzik

2007-07-02 18:31 UTC #5 lore raw

Quoted 11 lines · Bartlomiej Zolnierkiewicz

Quoted 7 lines · Jeff Garzik
Quoted 5 lines · Bartlomiej Zolnierkiewicz
This would brake setups which currently work OK, i.e. BIOS set things up (reminds me about cmd64x vs broken MWDMA)...
The RightThing(tm) to do is to fix alim15x3 driver to program DMA timings (especially given that pata_ali seems to already contain the needed code).
I am not ready to trust that pata_ali works as well as alim15x3 in all cases. Someone should test e.g. Alpha AXP systems with IDE (use alim15x3) to make sure all is well.
Sure but there is no problem with the new code in alim15x3 being X86 until it is verified to work with Alpha AXP machines etc...

I'm not sure I follow. You now want to mark alim15x3 X86-only? Or add the new code inside #ifdef X86?

Quoted 4 lines

Quoted 1 line
alim15x3 covers buggy, ugly, quirky chipsets. I consider that coverage in general highly fragile.
and in case of MWDMA modes also highly dependent on BIOS settings which very likely results in broken suspend/resume...

Agreed. But I would rather verify that timing programming works on supported platforms, before expanding use of said new code?

Jeff

Bartlomiej Zolnierkiewicz

author

2007-07-02 18:38 UTC #6 lore raw

Quoted 12 lines · Jeff Garzik

Quoted 11 lines · Bartlomiej Zolnierkiewicz
Quoted 9 lines · Jeff Garzik
Quoted 5 lines · Bartlomiej Zolnierkiewicz
This would brake setups which currently work OK, i.e. BIOS set things up (reminds me about cmd64x vs broken MWDMA)...
The RightThing(tm) to do is to fix alim15x3 driver to program DMA timings (especially given that pata_ali seems to already contain the needed code).
I am not ready to trust that pata_ali works as well as alim15x3 in all cases. Someone should test e.g. Alpha AXP systems with IDE (use alim15x3) to make sure all is well.

Quoted 5 lines

Quoted 2 lines
Sure but there is no problem with the new code in alim15x3 being X86 until it is verified to work with Alpha AXP machines etc...
I'm not sure I follow. You now want to mark alim15x3 X86-only? Or add the new code inside #ifdef X86?

Add the new code inside #ifdef X86, marking alim15x3 X86-only would be on over-kill...

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
alim15x3 covers buggy, ugly, quirky chipsets. I consider that coverage in general highly fragile.
and in case of MWDMA modes also highly dependent on BIOS settings which very likely results in broken suspend/resume...
Agreed. But I would rather verify that timing programming works on supported platforms, before expanding use of said new code?

Sure given that you have Alpha and Sparc machines handy for testing...

I don't so testing all supported platforms could take a while, in the meantime X86 users could enjoy fixed suspend/resume support...

Thanks,
Bart

Bartlomiej Zolnierkiewicz

author

2007-07-02 18:58 UTC #7 lore raw

Quoted 13 lines · Bartlomiej Zolnierkiewicz

Quoted 12 lines · Jeff Garzik
Quoted 11 lines · Bartlomiej Zolnierkiewicz
Quoted 9 lines · Jeff Garzik
Quoted 5 lines · Bartlomiej Zolnierkiewicz
This would brake setups which currently work OK, i.e. BIOS set things up (reminds me about cmd64x vs broken MWDMA)...
The RightThing(tm) to do is to fix alim15x3 driver to program DMA timings (especially given that pata_ali seems to already contain the needed code).
I am not ready to trust that pata_ali works as well as alim15x3 in all cases. Someone should test e.g. Alpha AXP systems with IDE (use alim15x3) to make sure all is well.

Quoted 17 lines

Quoted 5 lines
Quoted 2 lines
Sure but there is no problem with the new code in alim15x3 being X86 until it is verified to work with Alpha AXP machines etc...
I'm not sure I follow. You now want to mark alim15x3 X86-only? Or add the new code inside #ifdef X86?
Add the new code inside #ifdef X86, marking alim15x3 X86-only would be on over-kill...
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
alim15x3 covers buggy, ugly, quirky chipsets. I consider that coverage in general highly fragile.
and in case of MWDMA modes also highly dependent on BIOS settings which very likely results in broken suspend/resume...
Agreed. But I would rather verify that timing programming works on supported platforms, before expanding use of said new code?

WRT expanding use of the new code:

The same code is used for initial tuning and re-tuning during resume...

If the new code doesn't work fine the game is already over since it is used during driver initialization to tune DMA...

Quoted 4 lines

Sure given that you have Alpha and Sparc machines handy for testing...
I don't so testing all supported platforms could take a while, in the meantime X86 users could enjoy fixed suspend/resume support...

After quick check I also seem to lack X86 ALI machine to test so I'm counting on somebody else beating me to fixing alim15x3...

Thanks,
Bart

Jeff Garzik

2007-07-02 19:17 UTC #8 lore raw

In reply to Bartlomiej Zolnierkiewicz

Quoted 2 lines · Bartlomiej Zolnierkiewicz

Add the new code inside #ifdef X86, marking alim15x3 X86-only would be on over-kill...

Ugh. That's not how we do Linux development. It is better to convert the new code and wait for alpha/sparc users to scream, than add platform ifdefs. To do so creates divergent code paths, that runs counter to Linux's goal of a portable driver.

Jeff

Bartlomiej Zolnierkiewicz

author

2007-07-02 19:37 UTC #9 lore raw

Quoted 3 lines · Jeff Garzik

Quoted 2 lines · Bartlomiej Zolnierkiewicz
Add the new code inside #ifdef X86, marking alim15x3 X86-only would be on over-kill...

My reply is taken out of context.

Quoted 4 lines

Ugh. That's not how we do Linux development. It is better to convert the new code and wait for alpha/sparc users to scream, than add platform ifdefs. To do so creates divergent code paths, that runs counter to Linux's goal of a portable driver.

I insisted on changes being done in "Linux style" since the begging but you've complained about it and requested prior testing of the changes on alpha/sparc.

Maybe I misunderstood your first mail, if this is the case then we are fully agreed and there are no discrepancies in our views on development style.

Thanks,
Bart

Alan Cox

2007-07-02 19:16 UTC #10 lore raw

In reply to Jeff Garzik

Quoted 3 lines

I am not ready to trust that pata_ali works as well as alim15x3 in all cases. Someone should test e.g. Alpha AXP systems with IDE (use alim15x3) to make sure all is well.

My pata_ali is reported to do so. I don't know if yours does because my libata tree has some changes to set the controller to PIO when doing the initial reset/probe which may be needed if you don't have BIOS init.

If it doesn't it won't be hard to tweak. The timing rules are taken from the documentation.

Alan

Jeff Garzik

2007-07-02 19:21 UTC #11 lore raw

Quoted 3 lines · Alan Cox

Quoted 1 line
I am not ready to trust that pata_ali works as well as alim15x3 in all cases. Someone should test e.g. Alpha AXP systems with IDE (use alim15x3) to make sure all is well.
My pata_ali is reported to do so. I don't know if yours does because my

Good to hear, thanks for the info. Maybe we can get Al to give my old DS10 a try with pata_ali...

Jeff