Re: [PATCH 2/7] sata_promise: pdc_freeze() semantic change

[Jeff Garzik <jeff@garzik.org>]

Albert Lee wrote:
>  After checking the current implementations of freeze()/thaw(), it seems only pdc_freeze()
> does more than simple irq masking. Remove the DMA stop code from pdc_freeze().
> 
> Signed-off-by: Albert Lee <albertcc@tw.ibm.com>
> ---
> 
> diff -Nrup 01_remove_leftover_irqon/drivers/ata/sata_promise.c 02_sata_pdc_freeze/drivers/ata/sata_promise.c
> --- 01_remove_leftover_irqon/drivers/ata/sata_promise.c	2007-07-07 09:58:55.000000000 +0800
> +++ 02_sata_pdc_freeze/drivers/ata/sata_promise.c	2007-07-07 10:39:22.000000000 +0800
> @@ -578,7 +578,6 @@ static void pdc_freeze(struct ata_port *
>  
>  	tmp = readl(mmio + PDC_CTLSTAT);
>  	tmp |= PDC_IRQ_DISABLE;
> -	tmp &= ~PDC_DMA_ENABLE;
>  	writel(tmp, mmio + PDC_CTLSTAT);
>  	readl(mmio + PDC_CTLSTAT); /* flush */

Two comments:

1) I do not think it safe to simply remove this.  Have you verified that 
pdc_reset_port() without a disabled DMA engine works 100%?  At the very 
least I would say to -move- this code, rather the deleting it.

2) It was put there to disable the DMA engine at our first opportunity. 
  For some errors we really should take some preliminary 
port/DMA-disable actions in the interrupt handler, before kicking off EH 
and waiting to be scheduled in process context.

	Jeff