lkml.org 
[lkml]   [2007]   [Jun]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    SubjectRE: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
    Date
    From
    > +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:akpm@linux-foundation.org]
    Sent: Wednesday, June 27, 2007 1:27 PM
    To: kuan luo
    Cc: linux-kernel@vger.kernel.org; jeff@garzik.org; 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" <kuanluo@gmail.com> 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 <akpm@linux-foundation.org>

    - remove new typedef

    - use bss: it's already initialised to zero

    - cleanups

    Cc: Kuan Luo <kluo@nvidia.com>
    Cc: Peer Chen <pchen@nvidia.com>
    Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
    ---

    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 majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/
    \
     
     \ /
      Last update: 2007-06-27 11:19    [W:0.049 / U:59.924 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site