linux-kernel - Re: [PATCH 05/20] pata_efar: always program master_data before slave

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]

Date:	Sun, 20 Feb 2011 11:38:23 +0100
From:	Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
To:	Alan Cox <alan@...rguk.ukuu.org.uk>
Cc:	Sergei Shtylyov <sshtylyov@...sta.com>, linux-ide@...r.kernel.org,
	linux-kernel@...r.kernel.org, Jeff Garzik <jgarzik@...ox.com>
Subject: Re: [PATCH 05/20] pata_efar: always program master_data before slave_data

On Sat, Feb 19, 2011 at 5:48 PM, Alan Cox <alan@...rguk.ukuu.org.uk> wrote:
>> >   SLC90E66 datasheet only says that SIDETIM register has no effect without
>> > SITRE bit set.
>
> Which means the current setup is fine yes ?

That's the problem, the current setup is buggy in slave-only setups:

                slave_data |= ((timings[pio][0] << 2) |
timings[pio][1]) << shift;
                pci_write_config_byte(dev, 0x44, slave_data);
        }

        idetm_data |= 0x4000;   /* Ensure SITRE is set */
        pci_write_config_word(dev, idetm_port, idetm_data);

idetm_port needs to be programmed _before_ slave_data to "ensure SITRE is set".

IOW We currently depend on the BIOS here to set SITRE bit for us..

>> Alan, is this explanation sufficient to ACK this patch?  (Thanks
>> Sergei for digging it out.)
>>
>> Jeff, would it be possible to queue patches #01-15 for 2.6.39
>
> Have they been tested on the relevant hardware yet - I don't believe the
> changes should be made for the untested stuff and you indicated you
> agreed with that.

Unless they are obvious or risk is higher than benefit (bugfixes based
on code review).  I don't think that ATA code has became recently
special in this regard compared to the rest of the kernel.

> The tidyups for the easily tested cases (PIIX4 etc) look sensible

Please take look at the changes #01-15 then:

     ata_piix: SITRE handling fix
     ata_piix: unify code for programming PIO and MWDMA timings
     pata_efar: fix register naming used in efar_set_piomode()
     pata_efar: unify code for programming PIO and MWDMA timings
     pata_efar: always program master_data before slave_data
     pata_it8213: fix register naming used in it8213_set_piomode()
     pata_it8213: unify code for programming PIO and MWDMA timings
     pata_it8213: add UDMA100 and UDMA133 support
     pata_oldpiix: unify code for programming PIO and MWDMA timings
     pata_radisys: unify code for programming PIO and MWDMA timings
     pata_rdc: unify code for programming PIO and MWDMA timings
     pata_rdc: parallel scanning needs an extra locking
     pata_rdc: add Power Management support
     pata_oldpiix: add locking for parallel scanning
     pata_oldpiix: enable parallel scan

Most patches has been posted months ago in the past as the part of
atang tree.  They don't contain risky stuff (it was intentionally
pushed at the end of the patch queue).  We have here obvious register
naming unification, PIO/DMA folding (it was tested with ata_piix and
only later ported to other PIIX-like drivers) and:

* para_rdc: locking bug-fix and Power Management support

* pata_it8213: IDE -> libata regression fix (UDMA100/133 support based
on vendor / old IDE driver)

* pata_oldpiix: locking bug-fix (ACK-ed by you in the past) and
parallel scanning support

I see a lot of magnitude more risky stuff going elsewhere in the
kernel, including ATA itself and if there are any concerns left those
patches (#01-15) I'll be happy to address them.

Thanks,
Bartlomiej
--
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/