linux-kernel - Re: [PATCH v3] Resurrect IT8172 IDE controller driver

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] [day] [month] [year] [list]

Message-ID: <b2b2f2320812301920y551371b7u2292fbccc73f4fec@mail.gmail.com>
Date:	Tue, 30 Dec 2008 21:20:07 -0600
From:	"Shane McDonald" <mcdonald.shane@...il.com>
To:	"Sergei Shtylyov" <sshtylyov@...mvista.com>
Cc:	alan@...rguk.ukuu.org.uk, bzolnier@...il.com,
	linux-ide@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] Resurrect IT8172 IDE controller driver

I have made the changes you have suggested.  See resolution below.
A v4 patch follows shortly.

On Sun, Dec 28, 2008 at 4:44 PM, Sergei Shtylyov
<sshtylyov@...mvista.com> wrote:
> Shane McDonald wrote:
>
>  Well, you're close. :-)

Just 3 more iterations... :-)

>> diff -uprN a/drivers/ide/it8172.c b/drivers/ide/it8172.c
>> --- a/drivers/ide/it8172.c      1969-12-31 18:00:00.000000000 -0600
>> +++ b/drivers/ide/it8172.c      2008-12-22 01:23:15.000000000 -0600
>> @@ -0,0 +1,188 @@
>> +                                    /* RTx  PWx */
>> +       static const u8 timings[][2] = {
>> +                                       { 7, 7 },
>> +                                       { 3, 4 },
>
>  This is not enough for PIO mode 1, as this only gives 150 ns active + 120
> ns recovery = 270 ns where minimum is 383 ns

OK, I think I understand the required timings now.  I will update PIO mode 1
to set RTx to 7 and PWx to 4, giving 150 ns active + 240 ns recovery
= 390 ns (> minimum 383 ns).

>> +       if (drive->dn) {
>
>  Would have been good to collapse both branches...

Yes, the code is quite repetitive.  I've collapsed it into a single
chunk of code.

>> +               if (pio > 2 || ata_id_has_iordy(drive->id))
>
>  You can actually skip "pio > 2" test.

Agreed.

>> +       if (speed >= XFER_UDMA_0) {
>> +               u8 udma = speed - XFER_UDMA_0;
>> +               u_speed = min_t(u8, 2 - (udma & 1), udma) << (drive->dn *
>> 4);
>
>  There's no need to use such complex math when the chip only supports
> UltraDMA modes 0 thru 2. Be simple:
>
>                u_speed = udma << (drive->dn * 4);

Suggested change has been implemented.

>> +               if (speed >= XFER_MW_DMA_0)
>
>  We dropped support for SWDMA, so the check  should not be needed...

Yes.  Check will be removed.

>> +                       pio = mwdma_to_pio[speed - XFER_MW_DMA_0];
>> +               else
>> +                       pio = 2;
>>
>
>  ... and *else* branch is pointless.

and is now removed.

>> +               it8172_set_pio_mode(drive, pio);
>
>  Well, not the best place in the PIIX driver -- DMA modes shouldn't affect
> prefetch and IORDY settings (regardless of what the Intel's programming
> manuals say :-).
> Àlthough, it's probably OK to continue abusing the set_pio_mode() method.

I will continue to abuse set_pio_mode().

>
>> +static const struct ide_port_info it8172_port_info __devinitdata = {
>> +       .name           = DRV_NAME,
>> +       .port_ops       = &it8172_port_ops,
>> +       .enablebits     = {{0x41, 0x80, 0x80}, {0x00, 0x00, 0x00} },
>>
>
>  Inconsistent spacing, add space after first {...

I will correct the spacing.

> MBR, Sergei

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