linux-kernel - Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers

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: <4AAC3EEA.1080605@pobox.com>
Date:	Sat, 12 Sep 2009 20:38:02 -0400
From:	Jeff Garzik <jgarzik@...ox.com>
To:	"Jung-Ik (John) Lee" <jilee@...gle.com>
CC:	Robert Hancock <hancockrwd@...il.com>,
	Daniel Walker <dwalker@...o99.com>, linux-ide@...r.kernel.org,
	linux-kernel@...r.kernel.org, Grant Grundler <grundler@...gle.com>,
	Gwendal Grignou <gwendal@...gle.com>,
	Eric Uhrhane <ericu@...gle.com>, Tejun Heo <tj@...nel.org>,
	Alan Cox <alan@...ux.intel.com>
Subject: Re: [PATCH] libata: Add pata_atp867x driver for Artop/Acard ATP867X
  controllers

On 09/12/2009 08:17 PM, Jung-Ik (John) Lee wrote:
> On Sat, Sep 12, 2009 at 9:49 AM, Robert Hancock<hancockrwd@...il.com>  wrote:
>> On 09/12/2009 04:59 AM, Jung-Ik (John) Lee wrote:
>>
>> (snip)
>>
>> Looks mostly reasonable to me, other than a few issues:
>>
>>> +static void atp867x_set_piomode(struct ata_port *ap, struct ata_device
>>> *adev)
>>> +{
>>> +       struct ata_device *peer = ata_dev_pair(adev);
>>> +       struct atp867x_priv *dp = ap->private_data;
>>> +       u8 speed = adev->pio_mode;
>>> +       struct ata_timing t, p;
>>> +       int T, UT;
>>> +       u8 b;
>>> +
>>> +       T = 1000000000 / 33333;
>>> +       UT = T/4;
>>> +
>>> +       switch (speed) {
>>> +       case XFER_PIO_4:
>>> +       case XFER_PIO_3:
>>> +       case XFER_PIO_2:
>>> +       case XFER_PIO_1:
>>> +       case XFER_PIO_0:
>>> +       case XFER_PIO_SLOW:
>>> +               break;
>>> +       default:
>>> +               printk(KERN_WARNING "ATP867X: Unsupported speed %#x."
>>> +                       " Default to XFER_PIO_0.\n", (unsigned)speed);
>>> +               speed = XFER_PIO_0;
>>> +       }
>>> +
>>> +       ata_timing_compute(adev, speed,&t, T, UT);
>>> +       if (peer&&    peer->pio_mode) {
>>> +               ata_timing_compute(peer, peer->pio_mode,&p, T, UT);
>>> +               ata_timing_merge(&p,&t,&t, ATA_TIMING_8BIT);
>>> +       }
>>> +
>>> +       b = inb(dp->dma_mode);
>>> +       if (adev->devno&    1)
>>> +               b = (b&    ~ATP867X_IO_DMAMODE_SLAVE_MASK);
>>> +       else
>>> +               b = (b&    ~ATP867X_IO_DMAMODE_MSTR_MASK);
>>> +       outb(b, dp->dma_mode);
>>> +
>>> +#ifdef ATP867X_NO_HACK_PIOMODE
>>> +       b = atp867x_get_active_clocks_shifted(t.active) |
>>> +               atp867x_get_recover_clocks_shifted(t.recover);
>>> +#else
>>> +       /*
>>> +        * magic value that works (from doc 6.4, 6.6.9)
>>> +        */
>>> +       b = 0x31;
>>> +#endif
>>
>> What's the purpose of this ifdef?
>
> The magic value part must go. I'll update the patch.
>
>>
>>> +       if (dp->pci66mhz)
>>> +               b += 0x10;
>>> +
>>> +       if (adev->devno&    1)
>>> +               outb(b, dp->slave_piospd);
>>> +       else
>>> +               outb(b, dp->mstr_piospd);
>>> +
>>> +       /*
>>> +        * use the same value for comand timing as for PIO timimg
>>> +        */
>>> +       outb(b, dp->eightb_piospd);
>>> +}
>>> +
>>> +static int atp867x_cable_detect(struct ata_port *ap)
>>> +{
>>> +       return ATA_CBL_PATA40_SHORT;
>>> +}
>>
>> Doesn't the controller have a way to do proper 80-wire cable detection?
>
> No programmatic way. libata.force should be used for other configurations.

Ideally you would use ATA_CBL_PATA_UNK / ata_cable_unknown, but it seems 
that ATA_CBL_PATA_UNK has grown rather useless in the current kernel.

We should probably (a) update libata-core/libata-sff to do something 
intelligent with ATA_CBL_PATA_UNK, and (b) use ATA_CBL_PATA_UNK in 
pata_atp867x.

But yes, libata.force works as well.  It just seems like we are losing 
an opportunity to accept an existing BIOS-configured 80-wire cable 
configuration by hardcoding ATA_CBL_PATA40_SHORT.

Also, a specific note -- atp867x_cable_detect() should be moved into 
drivers/ata/libata-core.c and made generic.  We have ata_cable_xxx 
versions for everything except 40-short, it seems.

	Jeff




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