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

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

[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 <bzolnier@gmail.com>
---
 drivers/ide/pci/alim15x3.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

Index: b/drivers/ide/pci/alim15x3.c
===================================================================
--- a/drivers/ide/pci/alim15x3.c
+++ 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 {

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

[Sergei Shtylyov]

Bartlomiej Zolnierkiewicz wrote:
> * ->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 <bzolnier@gmail.com>

> Index: b/drivers/ide/pci/alim15x3.c
> ===================================================================
> --- a/drivers/ide/pci/alim15x3.c
> +++ 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.
> +		 */
> +

    Erm, wouldn't it have been better to mark them as unsupported while at it?

>  		if (speed < XFER_SW_DMA_0)
>  			(void) ali15x3_tune_pio(drive, speed - XFER_PIO_0);
>  	} else {

MBR, Sergei

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

[Bartlomiej Zolnierkiewicz]

On Monday 02 July 2007, Sergei Shtylyov wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > * ->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 <bzolnier@gmail.com>
> 
> > Index: b/drivers/ide/pci/alim15x3.c
> > ===================================================================
> > --- a/drivers/ide/pci/alim15x3.c
> > +++ 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.
> > +		 */
> > +
> 
>     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

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

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> 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

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

[Bartlomiej Zolnierkiewicz]

On Monday 02 July 2007, Jeff Garzik wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > 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...

> 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

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

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> On Monday 02 July 2007, Jeff Garzik wrote:
>> Bartlomiej Zolnierkiewicz wrote:
>>> 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?


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

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

[Bartlomiej Zolnierkiewicz]

On Monday 02 July 2007, Jeff Garzik wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > On Monday 02 July 2007, Jeff Garzik wrote:
> >> Bartlomiej Zolnierkiewicz wrote:
> >>> 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?

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

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

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

[Bartlomiej Zolnierkiewicz]

On Monday 02 July 2007, Bartlomiej Zolnierkiewicz wrote:
> On Monday 02 July 2007, Jeff Garzik wrote:
> > Bartlomiej Zolnierkiewicz wrote:
> > > On Monday 02 July 2007, Jeff Garzik wrote:
> > >> Bartlomiej Zolnierkiewicz wrote:
> > >>> 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?
> 
> Add the new code inside #ifdef X86, marking alim15x3 X86-only would be
> on over-kill...
> 
> > >> 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...

> 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

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

[Jeff Garzik]

Bartlomiej Zolnierkiewicz wrote:
> 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

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

[Bartlomiej Zolnierkiewicz]

On Monday 02 July 2007, Jeff Garzik wrote:
> Bartlomiej Zolnierkiewicz wrote:
> > Add the new code inside #ifdef X86, marking alim15x3 X86-only would be
> > on over-kill...

My reply is taken out of context.

> 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

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

[Alan Cox]

> 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

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

[Jeff Garzik]

Alan Cox wrote:
>> 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