Re: [PATCH-RFC] Promise TX4 implement hw-bug workaround
Alexander Sabourenkov wrote: Alan Cox wrote: I can't think of a way to avoid second pass over scatterlist without duplicating code (ata_qc_prep() and ata_fill_sg() from libata-core.c). This appears to be incomplete: [...] What guarantees you have enough PRD entries to do this without changing the limit in the structures ? Otherwise looks good PRD entries count is 256 include/linux/ata.h: ATA_MAX_PRD = 256, ATA_PRD_TBL_SZ = (ATA_MAX_PRD * ATA_PRD_SZ), drivers/ata/libata-core.c: ap-prd = dmam_alloc_coherent(dev, ATA_PRD_TBL_SZ, ap-prd_dma, sata_promise Scsi_Host declares support for half of that: include/linux/libata.h: LIBATA_MAX_PRD = ATA_MAX_PRD / 2, drivers/ata/sata_promise.c .sg_tablesize = LIBATA_MAX_PRD, Alan's point was that the existing code will give you up to LIBATA_MAX_PRD entries. After the post-virtual-merge splitting code in ata_fill_sg() executes, the worst case result is ATA_MAX_PRD entries. Thus, since your code has the potential to increase the number of s/g entries above that, it can potentially corrupt memory, lock up the machine, all the wonderful things that can happen when you run off the end of the s/g list. The fix is to decrease .sg_tablesize (LIBATA_MAX_PRD - 2 perhaps?) so that you guarantee this worst case never occurs, by guaranteeing that the system never sends you enough s/g entries to cause your code to go out of bounds. Jeff - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-RFC] Promise TX4 implement hw-bug workaround
BTW, looking at the Promise code I see cam_con.h: /* for ASIC bug, limit the last element of SG byteCount must 32 Dword */ #define SG_COUNT_ASIC_BUG 32 //#define SG_COUNT_ASIC_BUG 128 and in the code itself /* check PRD table, last element = (32 Dword), fix ASIC bug */ (though the code obviously uses SG_COUNT_ASIC_BUG==32, as the first paste indicates) so it seems like Promise first used 128 (32 dwords), but then backed down to 32 (8 dwords). Either way, we definitely have an ASIC bug to work around, it seems... Jeff - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-RFC] Promise TX4 implement hw-bug workaround
Jeff Garzik wrote: BTW, looking at the Promise code I see cam_con.h: /* for ASIC bug, limit the last element of SG byteCount must 32 Dword */ #define SG_COUNT_ASIC_BUG 32 //#define SG_COUNT_ASIC_BUG 128 and in the code itself /* check PRD table, last element = (32 Dword), fix ASIC bug */ (though the code obviously uses SG_COUNT_ASIC_BUG==32, as the first paste indicates) so it seems like Promise first used 128 (32 dwords), but then backed down to 32 (8 dwords). Which version is this define from? Both versions that are available now from their website define it at 41*4: /* for ASIC bug, limit the last element of SG byteCount must = 41 Dword */ #define SG_COUNT_ASIC_BUG 41*4 //#define SG_COUNT_ASIC_BUG 32 //#define SG_COUNT_ASIC_BUG 128 -- ./lxnt - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-RFC] Promise TX4 implement hw-bug workaround
On Sun, 28 Oct 2007 06:29:32 -0400, Jeff Garzik wrote:
BTW, looking at the Promise code I see
cam_con.h:
/* for ASIC bug, limit the last element of SG byteCount must 32 Dword */
#define SG_COUNT_ASIC_BUG 32
//#define SG_COUNT_ASIC_BUG 128
and in the code itself
/* check PRD table, last element = (32 Dword), fix ASIC bug */
(though the code obviously uses SG_COUNT_ASIC_BUG==32, as the first
paste indicates)
so it seems like Promise first used 128 (32 dwords), but then backed
down to 32 (8 dwords).
Either way, we definitely have an ASIC bug to work around, it seems...
You're looking at the old pdc-ultra2 driver. The newer unified
sataii150-300 driver (v1.01.0.23) upped the value to 41*4.
I've reviewed Alexander's patch, and I'm currently testing it
with the add-on patch below (fix sg_tablesize, code formatting
stuff, fix uninitialised 'addr' in a VPRINTK).
/Mikael
--- linux-2.6.24-rc1/drivers/ata/sata_promise.c.~1~ 2007-10-28
11:58:01.0 +0100
+++ linux-2.6.24-rc1/drivers/ata/sata_promise.c 2007-10-28 12:20:53.0
+0100
@@ -155,7 +155,7 @@ static struct scsi_host_template pdc_ata
.queuecommand = ata_scsi_queuecmd,
.can_queue = ATA_DEF_QUEUE,
.this_id= ATA_SHT_THIS_ID,
- .sg_tablesize = LIBATA_MAX_PRD,
+ .sg_tablesize = LIBATA_MAX_PRD-1,
.cmd_per_lun= ATA_SHT_CMD_PER_LUN,
.emulated = ATA_SHT_EMULATED,
.use_clustering = ATA_SHT_USE_CLUSTERING,
@@ -542,7 +542,7 @@ static void pdc_fill_sg(struct ata_queue
if (!(qc-flags ATA_QCFLAG_DMAMAP))
return;
-
+
WARN_ON(qc-__sg == NULL);
WARN_ON(qc-n_elem == 0 qc-pad_len == 0);
@@ -579,18 +579,15 @@ static void pdc_fill_sg(struct ata_queue
if (len SG_COUNT_ASIC_BUG) {
u32 addr;
- /* if len 2*SG_COUNT_ASIC_BUG then last
- segment will be larger than next-to-last.
- Somewhat ugly :(
- */
VPRINTK(Splitting last PRD.\n);
+ addr = le32_to_cpu(ap-prd[idx - 1].addr);
ap-prd[idx - 1].flags_len -=
cpu_to_le32(SG_COUNT_ASIC_BUG);
VPRINTK(PRD[%u] = (0x%X, 0x%X)\n, idx - 1, addr,
SG_COUNT_ASIC_BUG);
-
- addr = le32_to_cpu(ap-prd[idx - 1].addr) + len -
SG_COUNT_ASIC_BUG;
- len = SG_COUNT_ASIC_BUG;
+
+ addr = addr + len - SG_COUNT_ASIC_BUG;
+ len = SG_COUNT_ASIC_BUG;
ap-prd[idx].addr = cpu_to_le32(addr);
ap-prd[idx].flags_len = cpu_to_le32(len);
VPRINTK(PRD[%u] = (0x%X, 0x%X)\n, idx, addr, len);
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-RFC] Promise TX4 implement hw-bug workaround
Alexander Sabourenkov wrote: Jeff Garzik wrote: BTW, looking at the Promise code I see cam_con.h: /* for ASIC bug, limit the last element of SG byteCount must 32 Dword */ #define SG_COUNT_ASIC_BUG 32 //#define SG_COUNT_ASIC_BUG 128 and in the code itself /* check PRD table, last element = (32 Dword), fix ASIC bug */ (though the code obviously uses SG_COUNT_ASIC_BUG==32, as the first paste indicates) so it seems like Promise first used 128 (32 dwords), but then backed down to 32 (8 dwords). Which version is this define from? Both versions that are available now from their website define it at 41*4: Mikael Pettersson wrote: You're looking at the old pdc-ultra2 driver. The newer unified sataii150-300 driver (v1.01.0.23) upped the value to 41*4. I was looking at pdc-ulsata2_1.00.0.15.tgz, which was the latest driver that Promise's website gave me to when I listed SATA300 TX4 as my product. Sounds like that is outdated information, thanks for the correction! Jeff - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-RFC] Promise TX4 implement hw-bug workaround
On Sun, 28 Oct 2007 12:03:16 +0100 (MET), Mikael Pettersson wrote:
On Sun, 28 Oct 2007 06:29:32 -0400, Jeff Garzik wrote:
BTW, looking at the Promise code I see
cam_con.h:
/* for ASIC bug, limit the last element of SG byteCount must 32 Dword */
#define SG_COUNT_ASIC_BUG 32
//#define SG_COUNT_ASIC_BUG 128
and in the code itself
/* check PRD table, last element = (32 Dword), fix ASIC bug */
(though the code obviously uses SG_COUNT_ASIC_BUG==32, as the first
paste indicates)
so it seems like Promise first used 128 (32 dwords), but then backed
down to 32 (8 dwords).
Either way, we definitely have an ASIC bug to work around, it seems...
You're looking at the old pdc-ultra2 driver. The newer unified
sataii150-300 driver (v1.01.0.23) upped the value to 41*4.
I've reviewed Alexander's patch, and I'm currently testing it
with the add-on patch below (fix sg_tablesize, code formatting
stuff, fix uninitialised 'addr' in a VPRINTK).
FYI:
Several hours of testing on a SATA300 TX4 with two 3Gbps disks went well,
as did a quick test on a SATA300 TX2plus with one SATA and one PATA disk.
I'll test further on a 1st generation controller tomorrow.
/Mikael
--- linux-2.6.24-rc1/drivers/ata/sata_promise.c.~1~ 2007-10-28
11:58:01.0 +0100
+++ linux-2.6.24-rc1/drivers/ata/sata_promise.c 2007-10-28
12:20:53.0 +0100
@@ -155,7 +155,7 @@ static struct scsi_host_template pdc_ata
.queuecommand = ata_scsi_queuecmd,
.can_queue = ATA_DEF_QUEUE,
.this_id= ATA_SHT_THIS_ID,
- .sg_tablesize = LIBATA_MAX_PRD,
+ .sg_tablesize = LIBATA_MAX_PRD-1,
.cmd_per_lun= ATA_SHT_CMD_PER_LUN,
.emulated = ATA_SHT_EMULATED,
.use_clustering = ATA_SHT_USE_CLUSTERING,
@@ -542,7 +542,7 @@ static void pdc_fill_sg(struct ata_queue
if (!(qc-flags ATA_QCFLAG_DMAMAP))
return;
-
+
WARN_ON(qc-__sg == NULL);
WARN_ON(qc-n_elem == 0 qc-pad_len == 0);
@@ -579,18 +579,15 @@ static void pdc_fill_sg(struct ata_queue
if (len SG_COUNT_ASIC_BUG) {
u32 addr;
- /* if len 2*SG_COUNT_ASIC_BUG then last
-segment will be larger than next-to-last.
-Somewhat ugly :(
- */
VPRINTK(Splitting last PRD.\n);
+ addr = le32_to_cpu(ap-prd[idx - 1].addr);
ap-prd[idx - 1].flags_len -=
cpu_to_le32(SG_COUNT_ASIC_BUG);
VPRINTK(PRD[%u] = (0x%X, 0x%X)\n, idx - 1, addr,
SG_COUNT_ASIC_BUG);
-
- addr = le32_to_cpu(ap-prd[idx - 1].addr) + len -
SG_COUNT_ASIC_BUG;
- len = SG_COUNT_ASIC_BUG;
+
+ addr = addr + len - SG_COUNT_ASIC_BUG;
+ len = SG_COUNT_ASIC_BUG;
ap-prd[idx].addr = cpu_to_le32(addr);
ap-prd[idx].flags_len = cpu_to_le32(len);
VPRINTK(PRD[%u] = (0x%X, 0x%X)\n, idx, addr, len);
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH-RFC] Promise TX4 implement hw-bug workaround
Hello.
Once again,
There appears to be a hardware bug in that it chokes on scatterlist
if the last item is larger than 164 bytes. This was discovered by
reading the code of vendor-supplied driver.
The patch that follows fixes my problem on 2.6.22.
I can't think of a way to avoid second pass over scatterlist without
duplicating code (ata_qc_prep() and ata_fill_sg() from libata-core.c).
--- a/drivers/ata/sata_promise.c2007-07-09 03:32:17.0 +0400
+++ b/drivers/ata/sata_promise.c2007-10-27 19:12:46.0 +0400
@@ -531,6 +531,87 @@
memcpy(buf+31, cdb, cdb_len);
}
+/**
+ * pdc_fill_sg - Fill PCI IDE PRD table
+ * @qc: Metadata associated with taskfile to be transferred
+ *
+ * Fill PCI IDE PRD (scatter-gather) table with segments
+ * associated with the current disk command.
+ * Make sure hardware does not choke on it.
+ *
+ * LOCKING:
+ * spin_lock_irqsave(host lock)
+ *
+ */
+static void pdc_fill_sg(struct ata_queued_cmd *qc)
+{
+ struct ata_port *ap = qc-ap;
+ struct scatterlist *sg;
+ unsigned int idx;
+ const u32 SG_COUNT_ASIC_BUG = 41*4;
+
+ if (!(qc-flags ATA_QCFLAG_DMAMAP))
+ return;
+
+ WARN_ON(qc-__sg == NULL);
+ WARN_ON(qc-n_elem == 0 qc-pad_len == 0);
+
+ idx = 0;
+ ata_for_each_sg(sg, qc) {
+ u32 addr, offset;
+ u32 sg_len, len;
+
+ /* determine if physical DMA addr spans 64K boundary.
+* Note h/w doesn't support 64-bit, so we unconditionally
+* truncate dma_addr_t to u32.
+*/
+ addr = (u32) sg_dma_address(sg);
+ sg_len = sg_dma_len(sg);
+
+ while (sg_len) {
+ offset = addr 0x;
+ len = sg_len;
+ if ((offset + sg_len) 0x1)
+ len = 0x1 - offset;
+
+ ap-prd[idx].addr = cpu_to_le32(addr);
+ ap-prd[idx].flags_len = cpu_to_le32(len 0x);
+ VPRINTK(PRD[%u] = (0x%X, 0x%X)\n, idx, addr, len);
+
+ idx++;
+ sg_len -= len;
+ addr += len;
+ }
+ }
+
+ if (idx) {
+ u32 len = le32_to_cpu(ap-prd[idx - 1].flags_len);
+
+ if (len SG_COUNT_ASIC_BUG) {
+ u32 addr;
+ /* if len 2*SG_COUNT_ASIC_BUG then last
+ segment will be larger than next-to-last.
+ Somewhat ugly :(
+ */
+
+ VPRINTK(Splitting last PRD.\n);
+
+ ap-prd[idx - 1].flags_len -=
cpu_to_le32(SG_COUNT_ASIC_BUG);
+ VPRINTK(PRD[%u] = (0x%X, 0x%X)\n, idx - 1, addr,
SG_COUNT_ASIC_BUG);
+
+ addr = le32_to_cpu(ap-prd[idx - 1].addr) + len -
SG_COUNT_ASIC_BUG;
+ len = SG_COUNT_ASIC_BUG;
+ ap-prd[idx].addr = cpu_to_le32(addr);
+ ap-prd[idx].flags_len = cpu_to_le32(len);
+ VPRINTK(PRD[%u] = (0x%X, 0x%X)\n, idx, addr, len);
+
+ idx++;
+ }
+
+ ap-prd[idx - 1].flags_len |= cpu_to_le32(ATA_PRD_EOT);
+ }
+}
+
static void pdc_qc_prep(struct ata_queued_cmd *qc)
{
struct pdc_port_priv *pp = qc-ap-private_data;
@@ -540,7 +621,7 @@
switch (qc-tf.protocol) {
case ATA_PROT_DMA:
- ata_qc_prep(qc);
+ pdc_fill_sg(qc);
/* fall through */
case ATA_PROT_NODATA:
@@ -556,11 +637,11 @@
break;
case ATA_PROT_ATAPI:
- ata_qc_prep(qc);
+ pdc_fill_sg(qc);
break;
case ATA_PROT_ATAPI_DMA:
- ata_qc_prep(qc);
+ pdc_fill_sg(qc);
/*FALLTHROUGH*/
case ATA_PROT_ATAPI_NODATA:
pdc_atapi_pkt(qc);
-
To unsubscribe from this list: send the line unsubscribe linux-ide in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-RFC] Promise TX4 implement hw-bug workaround
I can't think of a way to avoid second pass over scatterlist without duplicating code (ata_qc_prep() and ata_fill_sg() from libata-core.c). This appears to be incomplete: + VPRINTK(Splitting last PRD.\n); + + ap-prd[idx - 1].flags_len -= cpu_to_le32(SG_COUNT_ASIC_BUG); + VPRINTK(PRD[%u] = (0x%X, 0x%X)\n, idx - 1, addr, SG_COUNT_ASIC_BUG); + + addr = le32_to_cpu(ap-prd[idx - 1].addr) + len - SG_COUNT_ASIC_BUG; + len = SG_COUNT_ASIC_BUG; + ap-prd[idx].addr = cpu_to_le32(addr); + ap-prd[idx].flags_len = cpu_to_le32(len); + VPRINTK(PRD[%u] = (0x%X, 0x%X)\n, idx, addr, len); + + idx++; What guarantees you have enough PRD entries to do this without changing the limit in the structures ? Otherwise looks good - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-RFC] Promise TX4 implement hw-bug workaround
Alan Cox wrote: I can't think of a way to avoid second pass over scatterlist without duplicating code (ata_qc_prep() and ata_fill_sg() from libata-core.c). This appears to be incomplete: [...] What guarantees you have enough PRD entries to do this without changing the limit in the structures ? Otherwise looks good PRD entries count is 256 include/linux/ata.h: ATA_MAX_PRD = 256, ATA_PRD_TBL_SZ = (ATA_MAX_PRD * ATA_PRD_SZ), drivers/ata/libata-core.c: ap-prd = dmam_alloc_coherent(dev, ATA_PRD_TBL_SZ, ap-prd_dma, sata_promise Scsi_Host declares support for half of that: include/linux/libata.h: LIBATA_MAX_PRD = ATA_MAX_PRD / 2, drivers/ata/sata_promise.c .sg_tablesize = LIBATA_MAX_PRD, PS: Vendor code has this limit at 32. -- ./lxnt - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH-RFC] Promise TX4 implement hw-bug workaround
Alexander Sabourenkov wrote: Alan Cox wrote: I can't think of a way to avoid second pass over scatterlist without duplicating code (ata_qc_prep() and ata_fill_sg() from libata-core.c). This appears to be incomplete: [...] What guarantees you have enough PRD entries to do this without changing the limit in the structures ? Otherwise looks good PRD entries count is 256 include/linux/ata.h: ATA_MAX_PRD = 256, ATA_PRD_TBL_SZ = (ATA_MAX_PRD * ATA_PRD_SZ), drivers/ata/libata-core.c: ap-prd = dmam_alloc_coherent(dev, ATA_PRD_TBL_SZ, ap-prd_dma, sata_promise Scsi_Host declares support for half of that: include/linux/libata.h: LIBATA_MAX_PRD= ATA_MAX_PRD / 2, drivers/ata/sata_promise.c .sg_tablesize = LIBATA_MAX_PRD, PS: Vendor code has this limit at 32. That's an interesting question of itself. I don't know what limits PRD count, but if it's hardware, then the driver should somehow make sure that it gets no more than hw can handle minus one for this errata. Right now driver declares that any hardware it supports can handle 128 PRD entries. If this is not true for any possibly existing specimen, we're welcoming trouble. -- ./lxnt - To unsubscribe from this list: send the line unsubscribe linux-ide in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
