> +static void nv_swncq_qc_to_dq(struct ata_port *ap, struct
ata_queued_cmd *qc)
> +{
> + struct nv_swncq_port_priv *pp = ap->private_data;
> + defer_queue_t *dq = &pp->defer_queue;
> +
> + /* queue is full */
> + WARN_ON((dq->rear + 1) % (ATA_MAX_QUEUE + 1) == dq->front);
>This is peculiar. The array is sized ATA_MAX_QUEUE+1 (ie: 33) and the
code
>uses ATA_MAX_QUEUE+1 everywhere.
>It looks to me like the ata code was designed to queue up to 32
elements
>and all this code has taken that to 33. What exactly is going on here?
The code is designed to contain 32 elements. But the position of rear
doesn't point to
a valid element to check whether the queue is full or null. If front ==
rear , queue is null.
if (rear + 1) % (ATA_MAX_QUEUE + 1) == front, queue is full.
So the value of the array is 32 + 1.
> +static void nv_swncq_fill_sg(struct ata_queued_cmd *qc)
> +{
> + struct ata_port *ap = qc->ap;
> + struct scatterlist *sg;
> + unsigned int idx;
> +
> + struct nv_swncq_port_priv *pp = ap->private_data;
> +
> + struct ata_prd *prd;
> +
> + WARN_ON(qc->__sg == NULL);
> + WARN_ON(qc->n_elem == 0 && qc->pad_len == 0);
> +
> + prd = (void*)pp->prd + (ATA_PRD_TBL_SZ * qc->tag);
>Are you sure that should have been ATA_PRD_TBL_SZ and not ATA_PRD_SZ?
>Can this expression be done in a more type-friendly way?
Yes, i am sure.
I allocated prdt memory of 32 commands.
pp->prd = dmam_alloc_coherent(dev, ATA_PRD_TBL_SZ * ATA_MAX_QUEUE,
&pp->prd_dma, GFP_KERNEL);
Maybe below way is more type-friendly.
prd = pp->prd + ATA_MAX_PRD * qc->tag;
Best Regards,
Kuan Luo
-----Original Message-----
From: Andrew Morton [mailto:[email protected]]
Sent: Wednesday, June 27, 2007 1:27 PM
To: kuan luo
Cc: [email protected]; [email protected]; Peer Chen; Kuan Luo
Subject: Re: [PATCH] ata: Add the SW NCQ support to sata_nv for
MCP51/MCP55/MCP61
On Wed, 27 Jun 2007 11:04:44 +0800 "kuan luo" <[email protected]> wrote:
> Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA
controller.
> NCQ function is disable by default, you can enable it with 'swncq=1'
>
This patch adds a large amount of new trailing whitespace.
> ---
> diff -Nurp a/sata_nv.c b/sata_nv.c
> --- a/sata_nv.c 2007-06-13 10:15:07.000000000 -0400
> +++ b/sata_nv.c 2007-06-26 12:52:27.000000000 -0400
Please prepare patches in `pathc -p1' form.
> +typedef struct {
> + u32 defer_bits;
> + u8 front;
> + u8 rear;
> + unsigned int tag[ATA_MAX_QUEUE + 1];
> +}defer_queue_t;
Avoid adding typedefs.
> +static int swncq_enabled = 0;
Don't initialise static storage to zero: it needlessly increases the
vmlinux size.
> nv_hardreset, ata_std_postreset);
> }
>
> +static void nv_swncq_qc_to_dq(struct ata_port *ap, struct
ata_queued_cmd *qc)
> +{
> + struct nv_swncq_port_priv *pp = ap->private_data;
> + defer_queue_t *dq = &pp->defer_queue;
> +
> + /* queue is full */
> + WARN_ON((dq->rear + 1) % (ATA_MAX_QUEUE + 1) == dq->front);
This is peculiar. The array is sized ATA_MAX_QUEUE+1 (ie: 33) and the
code
uses ATA_MAX_QUEUE+1 everywhere.
It looks to me like the ata code was designed to queue up to 32 elements
and all this code has taken that to 33. What exactly is going on here?
> +
> + dq->defer_bits |= (1 << qc->tag);
> +
> + dq->tag[dq->rear] = qc->tag;
> + dq->rear = (dq->rear + 1) % (ATA_MAX_QUEUE + 1);
> +
> +}
> +
> +static struct ata_queued_cmd *nv_swncq_qc_from_dq(struct ata_port
*ap)
> +{
> + struct nv_swncq_port_priv *pp = ap->private_data;
> + defer_queue_t *dq = &pp->defer_queue;
> + unsigned int tag;
> +
> + if (dq->front == dq->rear) /* null queue */
> + return NULL;
> +
> + tag = dq->tag[dq->front];
> + dq->tag[dq->front] = ATA_TAG_POISON;
> + dq->front = (dq->front + 1) % (ATA_MAX_QUEUE + 1);
etc.
> + WARN_ON(!(dq->defer_bits & (1 << tag)));
> + dq->defer_bits &= ~(1 << tag);
> +
> + return ata_qc_from_tag(ap, tag);
> +}
> +
> + dq->front = dq->rear = 0;
> + dq->defer_bits = 0;
> + pp->qc_active = 0;
> + pp->last_issue_tag = ATA_TAG_POISON;
> + nv_swncq_fis_reinit(ap);
> +}
> +
> +static void nv_swncq_irq_clear(struct ata_port *ap, u32 val)
> +{
> + void __iomem *mmio = ap->host->iomap[NV_MMIO_BAR];
> + u32 flags = (val << (ap->port_no * NV_INT_PORT_SHIFT_MCP55));
I hope we'll never need to support more than two ports...
> + writel(flags, mmio + NV_INT_STATUS_MCP55);
> +}
> +
> +static void nv_swncq_ncq_stop(struct ata_port *ap)
> +{
> + struct nv_swncq_port_priv *pp = ap->private_data;
> + unsigned int i;
> + u32 sactive;
> + u32 done_mask;
> +
> + ata_port_printk(ap, KERN_ERR,
> + "EH in SWNCQ mode,QC:qc_active 0x%X sactive 0x%X\n",
> + ap->qc_active, ap->sactive);
> + ata_port_printk(ap, KERN_ERR,
> + "SWNCQ:qc_active 0x%X defer_bits 0x%X last_issue_tag
0x%x\n "
> + "dhfis 0x%X dmafis 0x%X sdbfis 0x%X\n",
> + pp->qc_active, pp->defer_queue.defer_bits,
pp->last_issue_tag,
> + pp->dhfis_bits, pp->dmafis_bits,
> + pp->sdbfis_bits);
> +
> + ata_port_printk(ap, KERN_ERR, "ATA_REG 0x%X ERR_REG 0x%X\n",
> + ap->ops->check_status(ap),
ioread8(ap->ioaddr.error_addr));
> +
> + sactive = readl(pp->sactive_block);
> + done_mask = pp->qc_active ^ sactive;
> +
> + ata_port_printk(ap, KERN_ERR, "tag : dhfis dmafis sdbfis
sacitve\n");
> + for (i=0; i < ATA_MAX_QUEUE; i++) {
Missing spaces around the "=".
We have a script in scripts/checkpatch.pl which will inform you about
many
of these little things. Please familiarise yourself with it.
> + u8 err = 0;
> + if (pp->qc_active & (1 << i))
> + err = 0;
> + else if (done_mask & (1 << i))
> + err = 1;
> + else
> + continue;
> +
> + ata_port_printk(ap, KERN_ERR,
> + "tag 0x%x: %01x %01x %01x %01x %s\n", i,
> + (pp->dhfis_bits >> i) & 0x1,
> + (pp->dmafis_bits >> i) & 0x1 , (pp->sdbfis_bits >> i) &
0x1,
> + (sactive >> i) & 0x1,
> + (err ? "error!tag doesn't exit, but sactive bit is set"
: " "));
> + }
> +
> + nv_swncq_pp_reinit(ap);
> + ap->ops->irq_clear(ap);
> + nv_swncq_bmdma_stop(ap);
> + nv_swncq_irq_clear(ap, 0xffff);
> +}
>
> ...
>
> +
> +static void nv_swncq_fill_sg(struct ata_queued_cmd *qc)
> +{
> + struct ata_port *ap = qc->ap;
> + struct scatterlist *sg;
> + unsigned int idx;
> +
> + struct nv_swncq_port_priv *pp = ap->private_data;
> +
> + struct ata_prd *prd;
> +
> + WARN_ON(qc->__sg == NULL);
> + WARN_ON(qc->n_elem == 0 && qc->pad_len == 0);
> +
> + prd = (void*)pp->prd + (ATA_PRD_TBL_SZ * qc->tag);
Are you sure that should have been ATA_PRD_TBL_SZ and not ATA_PRD_SZ?
Can this expression be done in a more type-friendly way?
> + idx = 0;
> + ata_for_each_sg(sg, qc) {
> + u32 addr, offset;
> + u32 sg_len, len;
> +
> + addr = (u32) sg_dma_address(sg);
> + sg_len = sg_dma_len(sg);
> +
> + while (sg_len) {
> + offset = addr & 0xffff;
> + len = sg_len;
> + if ((offset + sg_len) > 0x10000)
> + len = 0x10000 - offset;
> +
> + prd[idx].addr = cpu_to_le32(addr);
> + prd[idx].flags_len = cpu_to_le32(len & 0xffff);
> +
> + idx++;
> + sg_len -= len;
> + addr += len;
> + }
> + }
> +
> + if (idx)
> + prd[idx - 1].flags_len |= cpu_to_le32(ATA_PRD_EOT);
> +}
> +
Various fixlets:
From: Andrew Morton <[email protected]>
- remove new typedef
- use bss: it's already initialised to zero
- cleanups
Cc: Kuan Luo <[email protected]>
Cc: Peer Chen <[email protected]>
Signed-off-by: Andrew Morton <[email protected]>
---
drivers/ata/sata_nv.c | 30 ++++++++++++++----------------
1 file changed, 14 insertions(+), 16 deletions(-)
diff -puN
drivers/ata/sata_nv.c~ata-add-the-sw-ncq-support-to-sata_nv-for-mcp51-mc
p55-mcp61-fix drivers/ata/sata_nv.c
---
a/drivers/ata/sata_nv.c~ata-add-the-sw-ncq-support-to-sata_nv-for-mcp51-
mcp55-mcp61-fix
+++ a/drivers/ata/sata_nv.c
@@ -255,12 +255,12 @@ struct nv_host_priv {
unsigned long type;
};
-typedef struct {
+struct defer_queue {
u32 defer_bits;
u8 front;
u8 rear;
unsigned int tag[ATA_MAX_QUEUE + 1];
-}defer_queue_t;
+};
struct nv_swncq_port_priv {
struct ata_prd *prd; /* our SG list */
@@ -270,7 +270,7 @@ struct nv_swncq_port_priv {
unsigned int last_issue_tag;
spinlock_t lock;
/* fifo loop queue to store deferral command */
- defer_queue_t defer_queue;
+ struct defer_queue defer_queue;
/* for NCQ interrupt analysis */
u32 dhfis_bits;
@@ -637,7 +637,7 @@ MODULE_DEVICE_TABLE(pci, nv_pci_tbl);
MODULE_VERSION(DRV_VERSION);
static int adma_enabled = 1;
-static int swncq_enabled = 0;
+static int swncq_enabled;
static void nv_adma_register_mode(struct ata_port *ap)
{
@@ -1705,7 +1705,7 @@ static void nv_adma_error_handler(struct
static void nv_swncq_qc_to_dq(struct ata_port *ap, struct
ata_queued_cmd *qc)
{
struct nv_swncq_port_priv *pp = ap->private_data;
- defer_queue_t *dq = &pp->defer_queue;
+ struct defer_queue *dq = &pp->defer_queue;
/* queue is full */
WARN_ON((dq->rear + 1) % (ATA_MAX_QUEUE + 1) == dq->front);
@@ -1720,7 +1720,7 @@ static void nv_swncq_qc_to_dq(struct ata
static struct ata_queued_cmd *nv_swncq_qc_from_dq(struct ata_port *ap)
{
struct nv_swncq_port_priv *pp = ap->private_data;
- defer_queue_t *dq = &pp->defer_queue;
+ struct defer_queue *dq = &pp->defer_queue;
unsigned int tag;
if (dq->front == dq->rear) /* null queue */
@@ -1760,7 +1760,7 @@ static void nv_swncq_fis_reinit(struct a
static void nv_swncq_pp_reinit(struct ata_port *ap)
{
struct nv_swncq_port_priv *pp = ap->private_data;
- defer_queue_t *dq = &pp->defer_queue;
+ struct defer_queue *dq = &pp->defer_queue;
dq->front = dq->rear = 0;
dq->defer_bits = 0;
@@ -1801,7 +1801,7 @@ static void nv_swncq_ncq_stop(struct ata
done_mask = pp->qc_active ^ sactive;
ata_port_printk(ap, KERN_ERR, "tag : dhfis dmafis sdbfis
sacitve\n");
- for (i=0; i < ATA_MAX_QUEUE; i++) {
+ for (i = 0; i < ATA_MAX_QUEUE; i++) {
u8 err = 0;
if (pp->qc_active & (1 << i))
err = 0;
@@ -1912,7 +1912,7 @@ static void nv_swncq_host_init(struct at
writel(~0x0, mmio + NV_INT_STATUS_MCP55);
}
-static int nv_swncq_port_start(struct ata_port *ap)
+static int nv_swncq_port_start(struct ata_port *ap)
{
struct device *dev = ap->host->dev;
void __iomem *mmio = ap->host->iomap[NV_MMIO_BAR];
@@ -1957,9 +1957,7 @@ static void nv_swncq_fill_sg(struct ata_
struct ata_port *ap = qc->ap;
struct scatterlist *sg;
unsigned int idx;
-
struct nv_swncq_port_priv *pp = ap->private_data;
-
struct ata_prd *prd;
WARN_ON(qc->__sg == NULL);
@@ -1972,7 +1970,7 @@ static void nv_swncq_fill_sg(struct ata_
u32 addr, offset;
u32 sg_len, len;
- addr = (u32) sg_dma_address(sg);
+ addr = (u32)sg_dma_address(sg);
sg_len = sg_dma_len(sg);
while (sg_len) {
@@ -2051,7 +2049,6 @@ static void nv_swncq_hotplug(struct ata_
/* analyze @irq_stat */
if (fis & NV_SWNCQ_IRQ_ADDED)
ata_ehi_push_desc(ehi, "hot plug");
-
else if (fis & NV_SWNCQ_IRQ_REMOVED)
ata_ehi_push_desc(ehi, "hot unplug");
@@ -2122,7 +2119,7 @@ static int nv_swncq_sdbfis(struct ata_po
}
if (pp->qc_active & pp->dhfis_bits)
- return nr_done;
+ return nr_done;
if (pp->ncq_saw_backout || (pp->qc_active ^pp->dhfis_bits))
/* if the controller cann't get a device to host
register FIS,
@@ -2181,7 +2178,7 @@ static int nv_swncq_dmafis(struct ata_po
if (unlikely(!qc))
return 0;
- rw = ((qc->tf.flags) & ATA_TFLAG_WRITE);
+ rw = qc->tf.flags & ATA_TFLAG_WRITE;
/* load PRD table addr. */
iowrite32(pp->prd_dma + ATA_PRD_TBL_SZ * qc->tag,
@@ -2189,7 +2186,7 @@ static int nv_swncq_dmafis(struct ata_po
/* specify data direction, triple-check start bit is clear */
dmactl = ioread8(ap->ioaddr.bmdma_addr + ATA_DMA_CMD);
- dmactl &= ~(ATA_DMA_WR);
+ dmactl &= ~ATA_DMA_WR;
if (!rw)
dmactl |= ATA_DMA_WR;
@@ -2305,6 +2302,7 @@ static irqreturn_t nv_swncq_interrupt(in
unsigned int handled = 0;
unsigned long flags;
u32 irq_stat;
+
spin_lock_irqsave(&host->lock, flags);
irq_stat = readl(host->iomap[NV_MMIO_BAR] +
NV_INT_STATUS_MCP55);
_
-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information. Any unauthorized review, use, disclosure or distribution
is prohibited. If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
