[PATCH 3/4] ide: make ide_rate_filter() also respect PIO and SW/MW DMA mode masks (take 2)

[PATCH 3/4] ide: make ide_rate_filter() also respect PIO and SW/MW DMA mode masks (take 2)

[Sergei Shtylyov]

Make ide_rate_filter() also respect PIO/SWDMA/MWDMA mode masks.  While at it,
make the udma_filter() method calls take precedence over using the mode masks.

Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

---
Alas, the previous version of the patch wasn't even worth signing it off --
the code had to be much trickier. The patch against the current Linus' tree and
unfortunately I was able to only compile test it since that tree gives MODPOST
warning and dies early on bootup. I'm hoping to give it more tesring when/if
I have time... :-)

 drivers/ide/ide-dma.c |    7 +++---
 drivers/ide/ide-lib.c |   57 ++++++++++++++++++++++++++++++--------------------
 2 files changed, 39 insertions(+), 25 deletions(-)

Index: linux-2.6/drivers/ide/ide-dma.c
===================================================================
--- linux-2.6.orig/drivers/ide/ide-dma.c
+++ linux-2.6/drivers/ide/ide-dma.c
@@ -664,10 +664,11 @@ static unsigned int ide_get_mode_mask(id
 		if ((id->field_valid & 4) == 0)
 			break;
 
-		mask = id->dma_ultra & hwif->ultra_mask;
-
 		if (hwif->udma_filter)
-			mask &= hwif->udma_filter(drive);
+			mask = hwif->udma_filter(drive);
+		else
+			mask = hwif->ultra_mask;
+		mask &= id->dma_ultra;
 
 		if ((mask & 0x78) && (eighty_ninty_three(drive) == 0))
 			mask &= 0x07;
Index: linux-2.6/drivers/ide/ide-lib.c
===================================================================
--- linux-2.6.orig/drivers/ide/ide-lib.c
+++ linux-2.6/drivers/ide/ide-lib.c
@@ -75,38 +75,51 @@ EXPORT_SYMBOL(ide_xfer_verbose);
  *
  *	Given the available transfer modes this function returns
  *	the best available speed at or below the speed requested.
- *
- *	FIXME: filter also PIO/SWDMA/MWDMA modes
  */
 
 u8 ide_rate_filter(ide_drive_t *drive, u8 speed)
 {
-#ifdef CONFIG_BLK_DEV_IDEDMA
 	ide_hwif_t *hwif = drive->hwif;
-	u8 mask = hwif->ultra_mask, mode = XFER_MW_DMA_2;
+	u8 mask, base = speed & 0xf8, bit = 1 << (speed & 0x07);
 
-	if (hwif->udma_filter)
-		mask = hwif->udma_filter(drive);
+	switch (base) {
+#ifdef CONFIG_BLK_DEV_IDEDMA
+	case XFER_UDMA_0:
+		mask = hwif->udma_filter ? hwif->udma_filter(drive) :
+					   hwif->ultra_mask;
 
-	/*
-	 * TODO: speed > XFER_UDMA_2 extra check is needed to avoid false
-	 * cable warning from eighty_ninty_three(), moving ide_rate_filter()
-	 * calls from ->speedproc to core code will make this hack go away
-	 */
-	if (speed > XFER_UDMA_2) {
-		if ((mask & 0x78) && (eighty_ninty_three(drive) == 0))
+		/*
+		 * TODO: if speed > XFER_UDMA_2, an extra check is needed to
+		 * avoid a false cable warning from eighty_ninty_three() --
+		 * moving ide_rate_filter() calls from ->speedproc to core
+		 * code will make this hack go away...
+		 */
+		if (speed > XFER_UDMA_2 && (mask & 0x78) &&
+		    eighty_ninty_three(drive) == 0)
 			mask &= 0x07;
-	}
-
-	if (mask)
-		mode = fls(mask) - 1 + XFER_UDMA_0;
 
-//	printk("%s: mode 0x%02x, speed 0x%02x\n", __FUNCTION__, mode, speed);
-
-	return min(speed, mode);
-#else /* !CONFIG_BLK_DEV_IDEDMA */
-	return min(speed, (u8)XFER_PIO_4);
+		if (mask & bit)
+			break;
+		base = XFER_MW_DMA_0;
+		/* Fall thru */
+	case XFER_MW_DMA_0:
+		if (hwif->mwdma_mask & bit)
+			break;
+		base = XFER_SW_DMA_0;
+		/* Fall thru */
+	case XFER_SW_DMA_0:
+		if (hwif->swdma_mask & bit)
+			break;
+		base = XFER_PIO_0;
+		/* Fall thru */
 #endif /* CONFIG_BLK_DEV_IDEDMA */
+	case XFER_PIO_0:
+		if (hwif->pio_mask & bit)
+			break;
+	default:
+		BUG();
+	}
+	return base | bit;
 }
 
 EXPORT_SYMBOL(ide_rate_filter);

Re: [PATCH 3/4] ide: make ide_rate_filter() also respect PIO and SW/MW DMA mode masks (take 2)

[Bartlomiej Zolnierkiewicz]

Hi,

On Sunday 02 September 2007, Sergei Shtylyov wrote:
> Make ide_rate_filter() also respect PIO/SWDMA/MWDMA mode masks.  While at it,

Hm, this seems to be already dealt with by:

http://kernel.org/pub/linux/kernel/people/bart/pata-2.6/patches/ide-mode-limiting-fixes-for-user-requested-speed-changes.patch

after applying the above patch ide_rate_filter() looks like this:

/**
 *      ide_rate_filter         -       filter transfer mode
 *      @drive: IDE device
 *      @speed: desired speed
 *
 *      Given the available transfer modes this function returns
 *      the best available speed at or below the speed requested.
 *
 *      TODO: check device PIO capabilities
 */

u8 ide_rate_filter(ide_drive_t *drive, u8 speed)
{
        ide_hwif_t *hwif = drive->hwif;
        u8 mode = ide_find_dma_mode(drive, speed);

        if (mode == 0) {
                if (hwif->pio_mask)
                        mode = fls(hwif->pio_mask) - 1 + XFER_PIO_0;
                else
                        mode = XFER_PIO_4;
        }

//      printk("%s: mode 0x%02x, speed 0x%02x\n", __FUNCTION__, mode, speed);

        return min(speed, mode);
}

What is still missing from IDE tree is "respecting device PIO limits" (sorry
I was quoting from memory it is TODO not FIXME [1]) which probably comes down
to adding ide_get_best_pio_mode() call to ide_rate_filter() (IMO this should
be done before ide-mode-limiting-fixes-for-user-requested-speed-changes.patch
changes).

[1] http://www.mail-archive.com/linux-ide@vger.kernel.org/msg09750.html

Ah, I see the source of the confusion now, I misunderstood your remark about
->mwdma_mask being ignored (for some reason I though that you were referring
to MWDMA mode not being tried if device refuses SET_XFER command w/ UDMA).

ide_find_dma_mode() is a bit tricky because it used also by ide_max_dma_mode()
but it should take care of ->mwdma_mask if ->udma_mask is zero just fine.

> make the udma_filter() method calls take precedence over using the mode masks.

This change is OK - could you make a new patch?

> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>
> 
> ---
> Alas, the previous version of the patch wasn't even worth signing it off --
> the code had to be much trickier. The patch against the current Linus' tree and

take #1 was correct - i.e. if UDMA0 is not available we want to go down
to MWDMA2 and not MWDMA0 (this is what the old code was doing BTW)

> unfortunately I was able to only compile test it since that tree gives MODPOST
> warning and dies early on bootup. I'm hoping to give it more tesring when/if
> I have time... :-)
> 
>  drivers/ide/ide-dma.c |    7 +++---
>  drivers/ide/ide-lib.c |   57 ++++++++++++++++++++++++++++++--------------------
>  2 files changed, 39 insertions(+), 25 deletions(-)
> 
> Index: linux-2.6/drivers/ide/ide-dma.c
> ===================================================================
> --- linux-2.6.orig/drivers/ide/ide-dma.c
> +++ linux-2.6/drivers/ide/ide-dma.c
> @@ -664,10 +664,11 @@ static unsigned int ide_get_mode_mask(id
>  		if ((id->field_valid & 4) == 0)
>  			break;
>  
> -		mask = id->dma_ultra & hwif->ultra_mask;
> -
>  		if (hwif->udma_filter)
> -			mask &= hwif->udma_filter(drive);
> +			mask = hwif->udma_filter(drive);
> +		else
> +			mask = hwif->ultra_mask;
> +		mask &= id->dma_ultra;
>  
>  		if ((mask & 0x78) && (eighty_ninty_three(drive) == 0))
>  			mask &= 0x07;
> Index: linux-2.6/drivers/ide/ide-lib.c
> ===================================================================
> --- linux-2.6.orig/drivers/ide/ide-lib.c
> +++ linux-2.6/drivers/ide/ide-lib.c
> @@ -75,38 +75,51 @@ EXPORT_SYMBOL(ide_xfer_verbose);
>   *
>   *	Given the available transfer modes this function returns
>   *	the best available speed at or below the speed requested.
> - *
> - *	FIXME: filter also PIO/SWDMA/MWDMA modes
>   */
>  
>  u8 ide_rate_filter(ide_drive_t *drive, u8 speed)
>  {
> -#ifdef CONFIG_BLK_DEV_IDEDMA
>  	ide_hwif_t *hwif = drive->hwif;
> -	u8 mask = hwif->ultra_mask, mode = XFER_MW_DMA_2;
> +	u8 mask, base = speed & 0xf8, bit = 1 << (speed & 0x07);
>  
> -	if (hwif->udma_filter)
> -		mask = hwif->udma_filter(drive);
> +	switch (base) {
> +#ifdef CONFIG_BLK_DEV_IDEDMA
> +	case XFER_UDMA_0:
> +		mask = hwif->udma_filter ? hwif->udma_filter(drive) :
> +					   hwif->ultra_mask;
>  
> -	/*
> -	 * TODO: speed > XFER_UDMA_2 extra check is needed to avoid false
> -	 * cable warning from eighty_ninty_three(), moving ide_rate_filter()
> -	 * calls from ->speedproc to core code will make this hack go away
> -	 */
> -	if (speed > XFER_UDMA_2) {
> -		if ((mask & 0x78) && (eighty_ninty_three(drive) == 0))
> +		/*
> +		 * TODO: if speed > XFER_UDMA_2, an extra check is needed to
> +		 * avoid a false cable warning from eighty_ninty_three() --
> +		 * moving ide_rate_filter() calls from ->speedproc to core
> +		 * code will make this hack go away...
> +		 */
> +		if (speed > XFER_UDMA_2 && (mask & 0x78) &&
> +		    eighty_ninty_three(drive) == 0)
>  			mask &= 0x07;
> -	}
> -
> -	if (mask)
> -		mode = fls(mask) - 1 + XFER_UDMA_0;
>  
> -//	printk("%s: mode 0x%02x, speed 0x%02x\n", __FUNCTION__, mode, speed);
> -
> -	return min(speed, mode);
> -#else /* !CONFIG_BLK_DEV_IDEDMA */
> -	return min(speed, (u8)XFER_PIO_4);
> +		if (mask & bit)
> +			break;
> +		base = XFER_MW_DMA_0;
> +		/* Fall thru */
> +	case XFER_MW_DMA_0:
> +		if (hwif->mwdma_mask & bit)
> +			break;
> +		base = XFER_SW_DMA_0;
> +		/* Fall thru */
> +	case XFER_SW_DMA_0:
> +		if (hwif->swdma_mask & bit)
> +			break;
> +		base = XFER_PIO_0;
> +		/* Fall thru */
>  #endif /* CONFIG_BLK_DEV_IDEDMA */
> +	case XFER_PIO_0:
> +		if (hwif->pio_mask & bit)
> +			break;
> +	default:
> +		BUG();
> +	}
> +	return base | bit;
>  }
>  
>  EXPORT_SYMBOL(ide_rate_filter);

Re: [PATCH 3/4] ide: make ide_rate_filter() also respect PIO and SW/MW DMA mode masks (take 2)

[Sergei Shtylyov]

Once I quothed:

> Make ide_rate_filter() also respect PIO/SWDMA/MWDMA mode masks.  While at it,
> make the udma_filter() method calls take precedence over using the mode masks.

    This one not looking to pretty -- I've geve some thought on how to 
beautify all these switch fallthoughs but haven't got something much better 
then uour original approach (AKA Occam's razor :-).

> Signed-off-by: Sergei Shtylyov <sshtylyov@ru.mvista.com>

Farewell!

Re: [PATCH 3/4] ide: make ide_rate_filter() also respect PIO and SW/MW DMA mode masks (take 2)

[Bartlomiej Zolnierkiewicz]

Hi,

On Saturday 08 September 2007, Sergei Shtylyov wrote:
> Once I quothed:
> 
> > Make ide_rate_filter() also respect PIO/SWDMA/MWDMA mode masks.  While at it,
> > make the udma_filter() method calls take precedence over using the mode masks.
> 
>     This one not looking to pretty -- I've geve some thought on how to 
> beautify all these switch fallthoughs but haven't got something much better 
> then uour original approach (AKA Occam's razor :-).

Care to recast the patch to contain only udma_filter() part?

BTW ide_rate_filter() still lacks respecting PIO limits of the device
(Occam's razor doesn't apply to "PIO side" of ide_rate_filter() ;-)

Thanks,
Bart

Re: [PATCH 3/4] ide: make ide_rate_filter() also respect PIO and SW/MW DMA mode masks (take 2)

[Sergei Shtylyov]

Once I quothed:

>> Make ide_rate_filter() also respect PIO/SWDMA/MWDMA mode masks.  While 
>> at it,
>> make the udma_filter() method calls take precedence over using the 
>> mode masks.

>    This one not looking to pretty -- I've geve some thought on how to 
> beautify all these switch fallthoughs but haven't got something much 
> better then uour original approach (AKA Occam's razor :-).

    So, it's probably better forget about this patch. But please do take patch 
4/4 to merge either before or after your "Occam's razor" -- it makes its best 
at masking out the MWDMA modes anyways...

Farewell!

Re: [PATCH 3/4] ide: make ide_rate_filter() also respect PIO and SW/MW DMA mode masks (take 2)

[Bartlomiej Zolnierkiewicz]

On Monday 24 September 2007, Sergei Shtylyov wrote:
> Once I quothed:
> 
> >> Make ide_rate_filter() also respect PIO/SWDMA/MWDMA mode masks.  While 
> >> at it,
> >> make the udma_filter() method calls take precedence over using the 
> >> mode masks.
> 
> >    This one not looking to pretty -- I've geve some thought on how to 
> > beautify all these switch fallthoughs but haven't got something much 
> > better then uour original approach (AKA Occam's razor :-).
> 
>     So, it's probably better forget about this patch. But please do take patch 
> 4/4 to merge either before or after your "Occam's razor" -- it makes its best 
> at masking out the MWDMA modes anyways...

Sure, could you re-send patch 4/4 synced against current Linus' tree?

Thanks,
Bart