linux-kernel - Re: [PATCH v2 2/4] ata_piix: unify code for programming PIO and MWDMA timings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <4E9824AC.6020902@mvista.com>
Date:	Fri, 14 Oct 2011 16:01:48 +0400
From:	Sergei Shtylyov <sshtylyov@...sta.com>
To:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
CC:	Jeff Garzik <jgarzik@...ox.com>, linux-ide@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 2/4] ata_piix: unify code for programming PIO and MWDMA
 timings

Hello.

On 13-10-2011 17:39, Bartlomiej Zolnierkiewicz wrote:

> From: Bartlomiej Zolnierkiewicz<bzolnier@...il.com>
> Subject: [PATCH v2] ata_piix: unify code for programming PIO and MWDMA timings

> Besides making things noticably simpler it results in ~2% decrease in
> the driver LOC count and also ~2% decrease in the driver binary size
> (as measured on x86-32).

> Fix piix_set_piomode() documentation while at it.

> Signed-off-by: Bartlomiej Zolnierkiewicz<bzolnier@...il.com>

    I'm still inclined to NAK this patch.

> ---
> v2: remove use_mwdma variable and fix comment style

> earlier references:
> https://lkml.org/lkml/2011/2/8/99
>
>   drivers/ata/ata_piix.c |  111 ++++++++++++++++---------------------------------
>   1 file changed, 38 insertions(+), 73 deletions(-)

> Index: b/drivers/ata/ata_piix.c
> ===================================================================
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
[...]
> @@ -811,31 +822,20 @@ static void do_pata_set_dmamode(struct a
[...]
> -	if (ap->udma_mask)
> -		pci_read_config_byte(dev, 0x48,&udma_enable);

    Don't think you should remove this.

> -
>   	if (speed>= XFER_UDMA_0) {
> -		unsigned int udma = adev->dma_mode - XFER_UDMA_0;
> +		unsigned int udma = speed - XFER_UDMA_0;
>   		u16 udma_timing;
>   		u16 ideconf;
>   		int u_clock, u_speed;
>
> +		spin_lock_irqsave(&piix_lock, flags);
> +
> +		pci_read_config_byte(dev, 0x48, &udma_enable);

    But you should also clear the UDMA enable bit when setting an MWDMA mode, no?

> +
>   		/*
>   		 * UDMA is handled by a combination of clock switching and
>   		 * selection of dividers
> @@ -868,56 +868,21 @@ static void do_pata_set_dmamode(struct a
>   			   performance (WR_PingPong_En) */
>   			pci_write_config_word(dev, 0x54, ideconf);
>   		}
> +
> +		pci_write_config_byte(dev, 0x48, udma_enable);
> +
> +		spin_unlock_irqrestore(&piix_lock, flags);
>   	} else {
> -		/*
> -		 * MWDMA is driven by the PIO timings. We must also enable
> -		 * IORDY unconditionally along with TIME1. PPE has already
> -		 * been set when the PIO timing was set.
> -		 */
> -		unsigned int mwdma	= adev->dma_mode - XFER_MW_DMA_0;
> -		unsigned int control;
> -		u8 slave_data;
> +		/* MWDMA is driven by the PIO timings. */
> +		unsigned int mwdma = speed - XFER_MW_DMA_0;
>   		const unsigned int needed_pio[3] = {
>   			XFER_PIO_0, XFER_PIO_3, XFER_PIO_4
>   		};
>   		int pio = needed_pio[mwdma] - XFER_PIO_0;
>
[...]
> -		if (ap->udma_mask)
> -			udma_enable&= ~(1<<  devid);

    Still don't understand why you're removing this code.

> -
> -		pci_write_config_word(dev, master_port, master_data);
> +		/* XFER_PIO_0 is never used currently */
> +		piix_set_timings(ap, adev, pio);
>   	}
> -	/* Don't scribble on 0x48 if the controller does not support UDMA */
> -	if (ap->udma_mask)
> -		pci_write_config_byte(dev, 0x48, udma_enable);

    Should have kept this one too...

MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/