| 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
| ||
|
Date: Mon, 21 Sep 2009 19:36:13 -0700
From: "Jung-Ik (John) Lee" <jilee@...gle.com>
To: Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
Cc: Jeff Garzik <jeff@...zik.org>,
Andrew Morton <akpm@...ux-foundation.org>,
Linus Torvalds <torvalds@...ux-foundation.org>,
linux-ide@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
Grant Grundler <grundler@...gle.com>,
Gwendal Gringo <gwendal@...gle.com>
Subject: Re: [git patches] libata updates
On Sun, Sep 20, 2009 at 2:05 PM, Bartlomiej Zolnierkiewicz
<bzolnier@...il.com> wrote:
> On Thursday 17 September 2009 22:49:35 Jeff Garzik wrote:
>>
>> Bug fixes, and a new driver.
>>
>>
>>
>> Please pull from 'upstream-linus' branch of
>> master.kernel.org:/pub/scm/linux/kernel/git/jgarzik/libata-dev.git upstream-linus
>>
>> to receive the following updates:
>>
>> drivers/ata/Kconfig | 9 +
>> drivers/ata/Makefile | 1 +
>> drivers/ata/ahci.c | 4 +-
>> drivers/ata/libata-core.c | 4 +-
>> drivers/ata/pata_amd.c | 3 +
>> drivers/ata/pata_atp867x.c | 548 ++++++++++++++++++++++++++++++++++++++++++++
>> drivers/ata/sata_promise.c | 155 +++++++++++--
>> include/linux/pci_ids.h | 2 +
>> 8 files changed, 704 insertions(+), 22 deletions(-)
>> create mode 100644 drivers/ata/pata_atp867x.c
>>
>> John(Jung-Ik) Lee (1):
>> libata: Add pata_atp867x driver for Artop/Acard ATP867X controllers
>
> That was really fast.. Not necessarily a bad thing but this driver would
> benefit from few polishing touches..
>
>> diff --git a/drivers/ata/pata_atp867x.c b/drivers/ata/pata_atp867x.c
>> new file mode 100644
>> index 0000000..7990de9
>> --- /dev/null
>> +++ b/drivers/ata/pata_atp867x.c
>> @@ -0,0 +1,548 @@
>> +/*
>> + * pata_atp867x.c - ARTOP 867X 64bit 4-channel UDMA133 ATA controller driver
>> + *
>> + * (C) 2009 Google Inc. John(Jung-Ik) Lee <jilee@...gle.com>
>> + *
>> + * Per Atp867 data sheet rev 1.2, Acard.
>> + * Based in part on early ide code from
>> + * 2003-2004 by Eric Uhrhane, Google, Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
>> + *
>> + *
>> + * TODO:
>> + * 1. RAID features [comparison, XOR, striping, mirroring, etc.]
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/pci.h>
>> +#include <linux/init.h>
>> +#include <linux/blkdev.h>
>> +#include <linux/delay.h>
>> +#include <linux/device.h>
>> +#include <scsi/scsi_host.h>
>> +#include <linux/libata.h>
>> +
>> +#define DRV_NAME "pata_atp867x"
>> +#define DRV_VERSION "0.7.5"
>> +
>> +/*
>> + * IO Registers
>> + * Note that all runtime hot priv ports are cached in ap private_data
>> + */
>> +
>> +enum {
>> + ATP867X_IO_CHANNEL_OFFSET = 0x10,
>> +
>> + /*
>> + * IO Register Bitfields
>> + */
>> +
>> + ATP867X_IO_PIOSPD_ACTIVE_SHIFT = 4,
>> + ATP867X_IO_PIOSPD_RECOVER_SHIFT = 0,
>> +
>> + ATP867X_IO_DMAMODE_MSTR_SHIFT = 0,
>> + ATP867X_IO_DMAMODE_MSTR_MASK = 0x07,
>> + ATP867X_IO_DMAMODE_SLAVE_SHIFT = 4,
>> + ATP867X_IO_DMAMODE_SLAVE_MASK = 0x70,
>> +
>> + ATP867X_IO_DMAMODE_UDMA_6 = 0x07,
>> + ATP867X_IO_DMAMODE_UDMA_5 = 0x06,
>> + ATP867X_IO_DMAMODE_UDMA_4 = 0x05,
>> + ATP867X_IO_DMAMODE_UDMA_3 = 0x04,
>> + ATP867X_IO_DMAMODE_UDMA_2 = 0x03,
>> + ATP867X_IO_DMAMODE_UDMA_1 = 0x02,
>> + ATP867X_IO_DMAMODE_UDMA_0 = 0x01,
>> + ATP867X_IO_DMAMODE_DISABLE = 0x00,
>> +
>> + ATP867X_IO_SYS_INFO_66MHZ = 0x04,
>> + ATP867X_IO_SYS_INFO_SLOW_UDMA5 = 0x02,
>> + ATP867X_IO_SYS_MASK_RESERVED = (~0xf1),
>> +
>> + ATP867X_IO_PORTSPD_VAL = 0x1143,
>> + ATP867X_PREREAD_VAL = 0x0200,
>> +
>> + ATP867X_NUM_PORTS = 4,
>> + ATP867X_BAR_IOBASE = 0,
>> + ATP867X_BAR_ROMBASE = 6,
>> +};
>> +
>> +#define ATP867X_IOBASE(ap) ((ap)->host->iomap[0])
>> +#define ATP867X_SYS_INFO(ap) (0x3F + ATP867X_IOBASE(ap))
>> +
>> +#define ATP867X_IO_PORTBASE(ap, port) (0x00 + ATP867X_IOBASE(ap) + \
>> + (port) * ATP867X_IO_CHANNEL_OFFSET)
>> +#define ATP867X_IO_DMABASE(ap, port) (0x40 + \
>> + ATP867X_IO_PORTBASE((ap), (port)))
>> +
>> +#define ATP867X_IO_STATUS(ap, port) (0x07 + \
>> + ATP867X_IO_PORTBASE((ap), (port)))
>> +#define ATP867X_IO_ALTSTATUS(ap, port) (0x0E + \
>> + ATP867X_IO_PORTBASE((ap), (port)))
>> +
>> +/*
>> + * hot priv ports
>> + */
>> +#define ATP867X_IO_MSTRPIOSPD(ap, port) (0x08 + \
>> + ATP867X_IO_DMABASE((ap), (port)))
>> +#define ATP867X_IO_SLAVPIOSPD(ap, port) (0x09 + \
>> + ATP867X_IO_DMABASE((ap), (port)))
>> +#define ATP867X_IO_8BPIOSPD(ap, port) (0x0A + \
>> + ATP867X_IO_DMABASE((ap), (port)))
>> +#define ATP867X_IO_DMAMODE(ap, port) (0x0B + \
>> + ATP867X_IO_DMABASE((ap), (port)))
>> +
>> +#define ATP867X_IO_PORTSPD(ap, port) (0x4A + \
>> + ATP867X_IO_PORTBASE((ap), (port)))
>> +#define ATP867X_IO_PREREAD(ap, port) (0x4C + \
>> + ATP867X_IO_PORTBASE((ap), (port)))
>> +
>> +struct atp867x_priv {
>> + void __iomem *dma_mode;
>> + void __iomem *mstr_piospd;
>> + void __iomem *slave_piospd;
>> + void __iomem *eightb_piospd;
>> + int pci66mhz;
>> +};
>> +
>> +static inline u8 atp867x_speed_to_mode(u8 speed)
>> +{
>> + return speed - XFER_UDMA_0 + 1;
>> +}
>> +
>> +static void atp867x_set_dmamode(struct ata_port *ap, struct ata_device *adev)
>> +{
>> + struct pci_dev *pdev = to_pci_dev(ap->host->dev);
>> + struct atp867x_priv *dp = ap->private_data;
>> + u8 speed = adev->dma_mode;
>> + u8 b;
>> + u8 mode;
>> +
>> + mode = atp867x_speed_to_mode(speed);
>
> The driver currently doesn't support MWDMA modes but claims otherwise
> (fixed in the attached patch).
>
>> + /*
>> + * Doc 6.6.9: decrease the udma mode value by 1 for safer UDMA speed
>> + * on 66MHz bus
>> + * rev-A: UDMA_1~4 (5, 6 no change)
>> + * rev-B: all UDMA modes
>> + * UDMA_0 stays not to disable UDMA
>> + */
>> + if (dp->pci66mhz && mode > ATP867X_IO_DMAMODE_UDMA_0 &&
>> + (pdev->device == PCI_DEVICE_ID_ARTOP_ATP867B ||
>> + mode < ATP867X_IO_DMAMODE_UDMA_5))
>> + mode--;
>> +
>> + b = ioread8(dp->dma_mode);
>> + if (adev->devno & 1) {
>> + b = (b & ~ATP867X_IO_DMAMODE_SLAVE_MASK) |
>> + (mode << ATP867X_IO_DMAMODE_SLAVE_SHIFT);
>> + } else {
>> + b = (b & ~ATP867X_IO_DMAMODE_MSTR_MASK) |
>> + (mode << ATP867X_IO_DMAMODE_MSTR_SHIFT);
>> + }
>> + iowrite8(b, dp->dma_mode);
>> +}
>> +
>> +static int atp867x_get_active_clocks_shifted(unsigned int clk)
>> +{
>> + unsigned char clocks = clk;
>> +
>> + switch (clocks) {
>> + case 0:
>> + clocks = 1;
>> + break;
>> + case 1 ... 7:
>> + break;
>> + case 8 ... 12:
>> + clocks = 7;
>
> Shouldn't "clocks = 0" (the default case) be used here?
The clocks value 0 sets it to 8 clocks, while value 7 sets to 12 clocks.
I cleaned up a bit on clocks_shift. See the patch below.
>
> Otherwise it seems to result in underclocked timings for dp->pci66mhz == 0.
>
>> + break;
>> + default:
>> + printk(KERN_WARNING "ATP867X: active %dclk is invalid. "
>> + "Using default 8clk.\n", clk);
>> + clocks = 0; /* 8 clk */
>> + break;
>> + }
>> + return clocks << ATP867X_IO_PIOSPD_ACTIVE_SHIFT;
>> +}
>> +
>> +static int atp867x_get_recover_clocks_shifted(unsigned int clk)
>> +{
>> + unsigned char clocks = clk;
>> +
>> + switch (clocks) {
>> + case 0:
>> + clocks = 1;
>> + break;
>> + case 1 ... 11:
>> + break;
>> + case 12:
>> + clocks = 0;
>> + break;
>> + case 13: case 14:
>> + --clocks;
>> + break;
>
> Is "clocks == 14" a reserved setting?
12 is reserved for the default (== 0), and 13, 14 are set to value 12,
13 respectively.
>
> If so a comment documenting it would be appreciated.
Sure. see the new patch below.
>
>> + case 15:
>> + break;
>> + default:
>> + printk(KERN_WARNING "ATP867X: recover %dclk is invalid. "
>> + "Using default 15clk.\n", clk);
>> + clocks = 0; /* 12 clk */
>
> Shouldn't it use "clocks == 15" setting?
It was a typo. 12 is the right default.
>
>> + break;
>> + }
>> + return clocks << ATP867X_IO_PIOSPD_RECOVER_SHIFT;
>> +}
>> +
>> +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;
>> +
>> + 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 = ioread8(dp->dma_mode);
>> + if (adev->devno & 1)
>> + b = (b & ~ATP867X_IO_DMAMODE_SLAVE_MASK);
>> + else
>> + b = (b & ~ATP867X_IO_DMAMODE_MSTR_MASK);
>> + iowrite8(b, dp->dma_mode);
>> +
>> + b = atp867x_get_active_clocks_shifted(t.active) |
>> + atp867x_get_recover_clocks_shifted(t.recover);
>> + if (dp->pci66mhz)
>> + b += 0x10;
>
> What is the purpose of the above hack?
For safe PIO mode according to spec.
>
> AFAICS (I don't have a datasheet) it may result in invalid active
> clocks being used for t.active > 12 and 0x80 bit being set incorrectly
> for t.active values 7..12 (unless it was the purpose of the hack).
See the patch below..
>
>> + if (adev->devno & 1)
>> + iowrite8(b, dp->slave_piospd);
>> + else
>> + iowrite8(b, dp->mstr_piospd);
>> +
>> + /*
>> + * use the same value for comand timing as for PIO timimg
>> + */
>> + iowrite8(b, dp->eightb_piospd);
>
> This is incorrect if slave/master devices use different PIO modes
> or if PIO mode <= 2 is used by any device.
>
> Timing based on t.act8b and t.rec8b values should be used instead.
act8b and rec8b have the same values as active, recovery of the port.
If a:r=3:1 then they become 4:1 on 66mhz for safer transfer, and
a8:r8=3:1, which is identical but the a8 should be incremented by 1.
I can use a8:r8 with 66mhz fixup but it becomes the same as using a:r.
Take a look at the patch below.
>
> On the somehow related note:
>
> * I don't see how PIO0-2 command timings can be met with only 3 bits
> used for active clocks. Could it be that dp->eight_piospd should be
> programmed in a slightly different way than dp->{mstr,slave}_piospd?
>
See new patch on clocks_shift below.
> * The controller allows higher clocks values for recovery timings but
> ata_timing_compute() tries to fairly increase both recovery and active
> timings to meet the required cycle timing.
>
>> +}
>> +
>> +static int atp867x_cable_detect(struct ata_port *ap)
>> +{
>> + return ATA_CBL_PATA40_SHORT;
>> +}
>
> As noticed by Robert and Alan already:
>
> This should use ATA_CBL_PATA_UNK and rely on the driver-side cable detection.
I modified cable_detect() to use override; on a certain
subsystem_vendor|device, its 40short, others, unknown.
>
>
> One last thing: Power Management support is missing from this driver
> (I tried addressing this in the separately posted patch but it needs
> testing by somebody with the hardware).
>
>
> MWDMA fix:
>
> From: Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
> Subject: [PATCH] pata_atp867x: fix it to not claim MWDMA support
>
> MWDMA modes are not supported by this driver currently.
>
> Signed-off-by: Bartlomiej Zolnierkiewicz <bzolnier@...il.com>
> ---
> drivers/ata/pata_atp867x.c | 10 +---------
> 1 file changed, 1 insertion(+), 9 deletions(-)
>
> Index: b/drivers/ata/pata_atp867x.c
> ===================================================================
> --- a/drivers/ata/pata_atp867x.c
> +++ b/drivers/ata/pata_atp867x.c
> @@ -118,20 +118,13 @@ struct atp867x_priv {
> int pci66mhz;
> };
>
> -static inline u8 atp867x_speed_to_mode(u8 speed)
> -{
> - return speed - XFER_UDMA_0 + 1;
> -}
> -
> static void atp867x_set_dmamode(struct ata_port *ap, struct ata_device *adev)
> {
> struct pci_dev *pdev = to_pci_dev(ap->host->dev);
> struct atp867x_priv *dp = ap->private_data;
> u8 speed = adev->dma_mode;
> u8 b;
> - u8 mode;
> -
> - mode = atp867x_speed_to_mode(speed);
> + u8 mode = speed - XFER_UDMA_0 + 1;
>
> /*
> * Doc 6.6.9: decrease the udma mode value by 1 for safer UDMA speed
> @@ -471,7 +464,6 @@ static int atp867x_init_one(struct pci_d
> static const struct ata_port_info info_867x = {
> .flags = ATA_FLAG_SLAVE_POSS,
> .pio_mask = ATA_PIO4,
> - .mwdma_mask = ATA_MWDMA2,
This looks good to me. thx.
> .udma_mask = ATA_UDMA6,
> .port_ops = &atp867x_ops,
> };
>
From: John(Jung-Ik) Lee <jilee@...gle.com>
clarifications in timings calculations and cable detection
Signed-off-by: John(Jung-Ik) Lee <jilee@...gle.com>
---
pata_atp867x.c | 50 +++++++++++++++++++++++++++++++++++++-------------
1 files changed, 37 insertions, 13 deletions
diff --git a/drivers/ata/pata_atp867x.c b/drivers/ata/pata_atp867x.c
index e6c4706..c1c691f 100644
--- a/drivers/ata/pata_atp867x.c
+++ b/drivers/ata/pata_atp867x.c
@@ -156,8 +156,10 @@ static void atp867x_set_dmamode(struct ata_port
*ap, struct ata_device *adev)
iowrite8(b, dp->dma_mode);
}
-static int atp867x_get_active_clocks_shifted(unsigned int clk)
+static int atp867x_get_active_clocks_shifted(struct ata_port *ap,
+ unsigned int clk)
{
+ struct atp867x_priv *dp = ap->private_data;
unsigned char clocks = clk;
switch (clocks) {
@@ -166,15 +168,25 @@ static int
atp867x_get_active_clocks_shifted(unsigned int clk)
break;
case 1 ... 7:
break;
- case 8 ... 12:
+ case 9 ... 12:
clocks = 7;
break;
default:
printk(KERN_WARNING "ATP867X: active %dclk is invalid. "
"Using default 8clk.\n", clk);
- clocks = 0; /* 8 clk */
- break;
+ case 8: /* default 8 clk */
+ clocks = 0;
+ goto active_clock_shift_done;
}
+
+ /*
+ * Doc 6.6.9: increase the clock value by 1 for safer PIO speed
+ * on 66MHz bus
+ */
+ if (dp->pci66mhz && clocks < 7)
+ clocks++;
+
+active_clock_shift_done:
return clocks << ATP867X_IO_PIOSPD_ACTIVE_SHIFT;
}
@@ -188,20 +200,19 @@ static int
atp867x_get_recover_clocks_shifted(unsigned int clk)
break;
case 1 ... 11:
break;
- case 12:
- clocks = 0;
- break;
case 13: case 14:
- --clocks;
+ --clocks; /* by the spec */
break;
case 15:
break;
default:
printk(KERN_WARNING "ATP867X: recover %dclk is invalid. "
"Using default 12clk.\n", clk);
- clocks = 0; /* 12 clk */
+ case 12: /* default 12 clk */
+ clocks = 0;
break;
}
+
return clocks << ATP867X_IO_PIOSPD_RECOVER_SHIFT;
}
@@ -230,10 +241,8 @@ static void atp867x_set_piomode(struct ata_port
*ap, struct ata_device *adev)
b = (b & ~ATP867X_IO_DMAMODE_MSTR_MASK);
iowrite8(b, dp->dma_mode);
- b = atp867x_get_active_clocks_shifted(t.active) |
+ b = atp867x_get_active_clocks_shifted(ap, t.active) |
atp867x_get_recover_clocks_shifted(t.recover);
- if (dp->pci66mhz)
- b += 0x10;
if (adev->devno & 1)
iowrite8(b, dp->slave_piospd);
@@ -246,9 +255,24 @@ static void atp867x_set_piomode(struct ata_port
*ap, struct ata_device *adev)
iowrite8(b, dp->eightb_piospd);
}
+static int atp867x_cable_override(struct pci_dev *pdev)
+{
+ if (pdev->subsystem_vendor == PCI_VENDOR_ID_ARTOP &&
+ (pdev->subsystem_device == PCI_DEVICE_ID_ARTOP_ATP867A ||
+ pdev->subsystem_device == PCI_DEVICE_ID_ARTOP_ATP867B)) {
+ return 1;
+ }
+ return 0;
+}
+
static int atp867x_cable_detect(struct ata_port *ap)
{
- return ATA_CBL_PATA40_SHORT;
+ struct pci_dev *pdev = to_pci_dev(ap->host->dev);
+
+ if (atp867x_cable_override(pdev))
+ return ATA_CBL_PATA40_SHORT;
+
+ return ATA_CBL_PATA_UNK;
}
static struct scsi_host_template atp867x_sht = {
--
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/
Powered by blists - more mailing lists