Re: [PATCH] sata_mv: stabilize for 5081 and other fixes
From: John Stoffel
Date: Wed Mar 08 2006 - 15:24:51 EST
>>>>> "Dan" == Dan Aloni <da-x@xxxxxxxxxxxxx> writes:
Dan> With the patch below I've managed to stabilize the sata_mv driver
Dan> running a Marvell 5081 SATA controller. Prior to these changes it
Dan> would cause sporadic memory corruptions right after insmod. I prefer
Dan> this driver over Marvell's own driver which have all sorts of weird
Dan> bugs.
Cool! Now I can think about SATA more seriously. Now for some
comments on the patch itself, from a non-kernel hacker, so feel free
to blow me off.
Dan> --- drivers/scsi/sata_mv.c 2006-03-08 11:30:10.000000000 +0200
Dan> +++ drivers/scsi/sata_mv.c 2006-03-08 20:59:55.000000000 +0200
Dan> @@ -72,9 +72,10 @@
Dan> */
Dan> MV_CRQB_Q_SZ = (32 * MV_MAX_Q_DEPTH),
Dan> MV_CRPB_Q_SZ = (8 * MV_MAX_Q_DEPTH),
Dan> - MV_MAX_SG_CT = 176,
Dan> + MV_MAX_SG_CT = 256,
Dan> MV_SG_TBL_SZ = (16 * MV_MAX_SG_CT),
Dan> - MV_PORT_PRIV_DMA_SZ = (MV_CRQB_Q_SZ + MV_CRPB_Q_SZ + MV_SG_TBL_SZ),
Dan> + MV_PORT_PRIV_DMA1_SZ = (MV_CRQB_Q_SZ + MV_CRPB_Q_SZ),
Dan> + MV_PORT_PRIV_DMA2_SZ = (MV_SG_TBL_SZ),
I don't like these names for the reagions. DMA1 and DMA2 mean
nothing. Should be be something a little more descriptive? And a
comment why you split them up would be good too.
Dan> +#pragma pack(1)
I don't see these used anywhere else in the kernel, they should
probably go.
Dan> +
Dan> /* Command ReQuest Block: 32B */
Dan> struct mv_crqb {
Dan> - u32 sg_addr;
Dan> - u32 sg_addr_hi;
Dan> - u16 ctrl_flags;
Dan> + volatile u32 sg_addr;
Dan> + volatile u32 sg_addr_hi;
Dan> + volatile u16 ctrl_flags;
Dan> u16 ata_cmd[11];
Dan> };
Dan> /* Command ResPonse Block: 8B */
Dan> struct mv_crpb {
Dan> - u16 id;
Dan> - u16 flags;
Dan> - u32 tmstmp;
Dan> + volatile u16 id;
Dan> + volatile u16 flags;
Dan> + volatile u32 tmstmp;
Dan> };
Dan> /* EDMA Physical Region Descriptor (ePRD); A.K.A. SG */
Dan> struct mv_sg {
Dan> - u32 addr;
Dan> - u32 flags_size;
Dan> - u32 addr_hi;
Dan> - u32 reserved;
Dan> + volatile u32 addr;
Dan> + volatile u16 size;
Dan> + volatile u16 flags;
Dan> + volatile u32 addr_hi;
Dan> + volatile u32 reserved;
Dan> };
Dan> +#pragma pack(0)
Dan> +
Dan> struct mv_port_priv {
Dan> struct mv_crqb *crqb;
Dan> dma_addr_t crqb_dma;
Dan> @@ -294,8 +300,8 @@
Dan> };
Dan> struct mv_port_signal {
Dan> - u32 amps;
Dan> - u32 pre;
Dan> + volatile u32 amps;
Dan> + volatile u32 pre;
Dan> };
Dan> struct mv_host_priv;
Dan> @@ -365,7 +371,7 @@
Dan> .eh_strategy_handler = ata_scsi_error,
Dan> .can_queue = MV_USE_Q_DEPTH,
Dan> .this_id = ATA_SHT_THIS_ID,
Dan> - .sg_tablesize = MV_MAX_SG_CT / 2,
Dan> + .sg_tablesize = MV_MAX_SG_CT,
Dan> .max_sectors = ATA_MAX_SECTORS,
Dan> .cmd_per_lun = ATA_SHT_CMD_PER_LUN,
Dan> .emulated = ATA_SHT_EMULATED,
Dan> @@ -509,10 +515,7 @@
Dan> .reset_bus = mv_reset_pci_bus,
Dan> };
Dan> -/*
Dan> - * module options
Dan> - */
Dan> -static int msi; /* Use PCI msi; either zero (off, default) or non-zero */
Dan> +static int msi; /* Use PCI msi; either zero (off, default) or non-zero */
Dan> /*
Dan> @@ -770,7 +773,8 @@
Dan> static inline void mv_priv_free(struct mv_port_priv *pp, struct device *dev)
Dan> {
Dan> - dma_free_coherent(dev, MV_PORT_PRIV_DMA_SZ, pp->crpb, pp->crpb_dma);
Dan> + dma_free_coherent(dev, MV_PORT_PRIV_DMA1_SZ, pp->crpb, pp->crpb_dma);
Dan> + dma_free_coherent(dev, MV_PORT_PRIV_DMA2_SZ, pp->sg_tbl, pp->sg_tbl_dma);
Dan> }
Could you name these something like MV_PORT_CRPB_SZ and
MV_PORT_SGTBL_SZ instead? Seems to make more sense here, and shows
exactly what they refer to.
Dan> /**
Dan> @@ -788,8 +792,8 @@
Dan> struct device *dev = ap->host_set->dev;
Dan> struct mv_port_priv *pp;
Dan> void __iomem *port_mmio = mv_ap_base(ap);
Dan> - void *mem;
Dan> - dma_addr_t mem_dma;
Dan> + void *mem, *mem2;
Dan> + dma_addr_t mem_dma, mem_dma2;
Dan> int rc = -ENOMEM;
Dan> pp = kmalloc(sizeof(*pp), GFP_KERNEL);
Dan> @@ -797,11 +801,17 @@
Dan> goto err_out;
Dan> memset(pp, 0, sizeof(*pp));
Dan> - mem = dma_alloc_coherent(dev, MV_PORT_PRIV_DMA_SZ, &mem_dma,
Dan> + mem = dma_alloc_coherent(dev, MV_PORT_PRIV_DMA1_SZ, &mem_dma,
Dan> GFP_KERNEL);
Dan> if (!mem)
Dan> goto err_out_pp;
Dan> - memset(mem, 0, MV_PORT_PRIV_DMA_SZ);
Dan> + memset(mem, 0, MV_PORT_PRIV_DMA1_SZ);
Dan> +
Dan> + mem2 = dma_alloc_coherent(dev, MV_PORT_PRIV_DMA2_SZ, &mem_dma2,
Dan> + GFP_KERNEL);
Dan> + if (!mem2)
Dan> + goto err_out_pp_2;
Dan> + memset(mem2, 0, MV_PORT_PRIV_DMA2_SZ);
Dan> rc = ata_pad_alloc(ap, dev);
Dan> if (rc)
Dan> @@ -820,14 +830,12 @@
Dan> */
pp-> crpb = mem;
pp-> crpb_dma = mem_dma;
Dan> - mem += MV_CRPB_Q_SZ;
Dan> - mem_dma += MV_CRPB_Q_SZ;
Dan> /* Third item:
Dan> * Table of scatter-gather descriptors (ePRD), 16 bytes each
Dan> */
Dan> - pp->sg_tbl = mem;
Dan> - pp->sg_tbl_dma = mem_dma;
Dan> + pp->sg_tbl = mem2;
Dan> + pp->sg_tbl_dma = mem_dma2;
Dan> writelfl(EDMA_CFG_Q_DEPTH | EDMA_CFG_RD_BRST_EXT |
Dan> EDMA_CFG_WR_BUFF_LEN, port_mmio + EDMA_CFG_OFS);
Dan> @@ -853,7 +861,9 @@
Dan> return 0;
Dan> err_out_priv:
Dan> - mv_priv_free(pp, dev);
Dan> + dma_free_coherent(dev, MV_PORT_PRIV_DMA2_SZ, pp->sg_tbl, pp->sg_tbl_dma);
Dan> +err_out_pp_2:
Dan> + dma_free_coherent(dev, MV_PORT_PRIV_DMA1_SZ, pp->crpb, pp->crpb_dma);
Dan> err_out_pp:
Dan> kfree(pp);
Dan> err_out:
Dan> @@ -915,13 +925,15 @@
pp-> sg_tbl[i].addr = cpu_to_le32(addr & 0xffffffff);
pp-> sg_tbl[i].addr_hi = cpu_to_le32((addr >> 16) >> 16);
Dan> - pp->sg_tbl[i].flags_size = cpu_to_le32(len);
Dan> + pp->sg_tbl[i].flags = cpu_to_le16(0);
Dan> + pp->sg_tbl[i].size = cpu_to_le16(len);
Dan> + pp->sg_tbl[i].reserved = 0;
Dan> sg_len -= len;
Dan> addr += len;
Dan> if (!sg_len && ata_sg_is_last(sg, qc))
Dan> - pp->sg_tbl[i].flags_size |= cpu_to_le32(EPRD_FLAG_END_OF_TBL);
Dan> + pp->sg_tbl[i].flags |= cpu_to_le16(EPRD_FLAG_END_OF_TBL);
Dan> i++;
Dan> }
John
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/