Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Am Montag 09 Juli 2007 schrieb Peer Chen: > It's a rule for all our drivers not just for linux, also if we put the > Maxtor drive to the blacklist so that its NCQ function won't be enable for > all controllers of other vendors,but we don't have the strong evidence that > those Maxtor HDDs are also broken for other controllers. The Maxtor drive I mentioned earlier worked fine on a ULI AHCI controller with NCQ enabled for the few hours I used Linux on that machine for recovering purposes So I wouldn't want a global balcklist. BTW, I now have 4 Seagate ST3320620AS on the MCP51, mostly in raid5 and they work fine with NCQ enabled and the second patch you/your co-worker posted. Cheers, -- (°= =°) //\ Prakash Punnoor /\\ V_/ \_V signature.asc Description: This is a digitally signed message part.
Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Am Montag 09 Juli 2007 schrieb Peer Chen: It's a rule for all our drivers not just for linux, also if we put the Maxtor drive to the blacklist so that its NCQ function won't be enable for all controllers of other vendors,but we don't have the strong evidence that those Maxtor HDDs are also broken for other controllers. The Maxtor drive I mentioned earlier worked fine on a ULI AHCI controller with NCQ enabled for the few hours I used Linux on that machine for recovering purposes So I wouldn't want a global balcklist. BTW, I now have 4 Seagate ST3320620AS on the MCP51, mostly in raid5 and they work fine with NCQ enabled and the second patch you/your co-worker posted. Cheers, -- (°= =°) //\ Prakash Punnoor /\\ V_/ \_V signature.asc Description: This is a digitally signed message part.
RE: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
It's a rule for all our drivers not just for linux, also if we put the Maxtor drive to the blacklist so that its NCQ function won't be enable for all controllers of other vendors,but we don't have the strong evidence that those Maxtor HDDs are also broken for other controllers. BRs Peer Chen -Original Message- From: Zoltan Boszormenyi [mailto:[EMAIL PROTECTED] Sent: Saturday, July 07, 2007 3:33 PM To: kuan luo Cc: linux-kernel@vger.kernel.org; [EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED]; Peer Chen; Kuan Luo Subject: Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61 kuan luo írta: > From: Kuan Luo <[EMAIL PROTECTED]> > > 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'. NCQ will be turned off if the drive is Maxtor on MCP51 > or > MCP55 rev 0xa2 platform. > > Signed-off-by: Kuan Luo <[EMAIL PROTECTED]> > Signed-off-by: Peer Chen <[EMAIL PROTECTED]> > --- Thanks, I am using it on 2.6.22-rc7-git5, have run a stress test yesterday night. It seems to be as stable as the previous version. I guess it's safe to turn it on by default when it gets into Linus' kernels. I have a question though. Why is the blanket needed for Maxtor drives? Can't it be narrowed down to certain models? Maybe those models should be put into libata blacklist... Not that my current machine is affected, I am just curious. Best regards, Zoltán Böszörményi --- 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/
RE: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
It's a rule for all our drivers not just for linux, also if we put the Maxtor drive to the blacklist so that its NCQ function won't be enable for all controllers of other vendors,but we don't have the strong evidence that those Maxtor HDDs are also broken for other controllers. BRs Peer Chen -Original Message- From: Zoltan Boszormenyi [mailto:[EMAIL PROTECTED] Sent: Saturday, July 07, 2007 3:33 PM To: kuan luo Cc: linux-kernel@vger.kernel.org; [EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED]; [EMAIL PROTECTED]; Peer Chen; Kuan Luo Subject: Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61 kuan luo írta: From: Kuan Luo [EMAIL PROTECTED] 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'. NCQ will be turned off if the drive is Maxtor on MCP51 or MCP55 rev 0xa2 platform. Signed-off-by: Kuan Luo [EMAIL PROTECTED] Signed-off-by: Peer Chen [EMAIL PROTECTED] --- Thanks, I am using it on 2.6.22-rc7-git5, have run a stress test yesterday night. It seems to be as stable as the previous version. I guess it's safe to turn it on by default when it gets into Linus' kernels. I have a question though. Why is the blanket needed for Maxtor drives? Can't it be narrowed down to certain models? Maybe those models should be put into libata blacklist... Not that my current machine is affected, I am just curious. Best regards, Zoltán Böszörményi --- 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/
Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
kuan luo írta: From: Kuan Luo <[EMAIL PROTECTED]> 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'. NCQ will be turned off if the drive is Maxtor on MCP51 or MCP55 rev 0xa2 platform. Signed-off-by: Kuan Luo <[EMAIL PROTECTED]> Signed-off-by: Peer Chen <[EMAIL PROTECTED]> --- Thanks, I am using it on 2.6.22-rc7-git5, have run a stress test yesterday night. It seems to be as stable as the previous version. I guess it's safe to turn it on by default when it gets into Linus' kernels. I have a question though. Why is the blanket needed for Maxtor drives? Can't it be narrowed down to certain models? Maybe those models should be put into libata blacklist... Not that my current machine is affected, I am just curious. Best regards, Zoltán Böszörményi - 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/
Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
kuan luo írta: From: Kuan Luo [EMAIL PROTECTED] 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'. NCQ will be turned off if the drive is Maxtor on MCP51 or MCP55 rev 0xa2 platform. Signed-off-by: Kuan Luo [EMAIL PROTECTED] Signed-off-by: Peer Chen [EMAIL PROTECTED] --- Thanks, I am using it on 2.6.22-rc7-git5, have run a stress test yesterday night. It seems to be as stable as the previous version. I guess it's safe to turn it on by default when it gets into Linus' kernels. I have a question though. Why is the blanket needed for Maxtor drives? Can't it be narrowed down to certain models? Maybe those models should be put into libata blacklist... Not that my current machine is affected, I am just curious. Best regards, Zoltán Böszörményi - 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/
Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Zoltan Boszormenyi wrote: Hi, Zoltan Boszormenyi írta: Hi, I am testing your current code with akpm's beautifying patches for about an hour now. I have seen no problems with it so far. Still using the patch on 2.6.22-rc6 and no problems so far. It's really stable. I am looking forward to the next version and the inclusion into mainstream kernels. Thanks! I am going to hold off any more -rc testing, but if there's a patch against 2.6.22 when it releases, I would certainly try it on a system which is about to be redeploted. I'm also scheduling testing of several RAID queueing patches there as well. -- Bill Davidsen <[EMAIL PROTECTED]> "We have more to fear from the bungling of the incompetent than from the machinations of the wicked." - from Slashdot - 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/
Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Zoltan Boszormenyi wrote: Hi, Zoltan Boszormenyi írta: Hi, I am testing your current code with akpm's beautifying patches for about an hour now. I have seen no problems with it so far. Still using the patch on 2.6.22-rc6 and no problems so far. It's really stable. I am looking forward to the next version and the inclusion into mainstream kernels. Thanks! I am going to hold off any more -rc testing, but if there's a patch against 2.6.22 when it releases, I would certainly try it on a system which is about to be redeploted. I'm also scheduling testing of several RAID queueing patches there as well. -- Bill Davidsen [EMAIL PROTECTED] We have more to fear from the bungling of the incompetent than from the machinations of the wicked. - from Slashdot - 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/
Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Hi, Zoltan Boszormenyi írta: Hi, I am testing your current code with akpm's beautifying patches for about an hour now. I have seen no problems with it so far. Still using the patch on 2.6.22-rc6 and no problems so far. It's really stable. I am looking forward to the next version and the inclusion into mainstream kernels. Thanks! Best regards, Zoltán Böszörményi - 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/
Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Hi, Zoltan Boszormenyi írta: Hi, I am testing your current code with akpm's beautifying patches for about an hour now. I have seen no problems with it so far. Still using the patch on 2.6.22-rc6 and no problems so far. It's really stable. I am looking forward to the next version and the inclusion into mainstream kernels. Thanks! Best regards, Zoltán Böszörményi - 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/
Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Am Mittwoch 27 Juni 2007 schrieb Peer Chen: > We did the many test with the new version driver and didn't encounter > that problem, but in certain cases, DMASETUP command packets from drive > to the controller are corrupted, and the controller issues an R_ERR to > the drive. Drives that comply with SATA spec will re-transmit the > corrupted packet and normal operation continues. However, some Maxtor > NCQ drives do not re-transmit the DMASETUP command packet, resulting in > software timeout for this command. So if you are using the Maxtor HD and > meet this problem,please don't enable the NCQ function. Out of interest I tested the patch again with my Maxtor drive I mentioned in the other thread. Ths time it got worse. I couldn't boot anymore, ie init starts some services and then dies at random places as if corrupt data was transfered. With the previous patch the system worked, except the port resets. Anyway, I'll ask Maxtor or rather Seagate to fix their firmware pointing at your diagnosis. bye, -- (°= =°) //\ Prakash Punnoor /\\ V_/ \_V signature.asc Description: This is a digitally signed message part.
RE: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Hi, I am testing your current code with akpm's beautifying patches for about an hour now. I have seen no problems with it so far. **A pretty good way. I will modify my code. Please, cc me when sending your next patch to LKML, thanks. Best regards, Zoltán Böszörményi - 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/
RE: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
A pretty good way. I will modify my code.
-Original Message-
From: Andrew Morton [mailto:[EMAIL PROTECTED]
Sent: Wednesday, June 27, 2007 5:21 PM
To: Kuan Luo
Cc: linux-kernel@vger.kernel.org; [EMAIL PROTECTED]; Peer Chen
Subject: Re: [PATCH] ata: Add the SW NCQ support to sata_nv for
MCP51/MCP55/MCP61
On Wed, 27 Jun 2007 17:09:29 +0800 "Kuan Luo" <[EMAIL PROTECTED]> wrote:
> > +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 = >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.
that's the wrong way of doing a circular buffer..
It's better to allow the head and tail indices to wrap all the way
through
0x and only mask them when they are actually used for
subscripting.
That way:
add:
array[head++ & (ATA_MAX_QUEUE-1)] = item;
remove:
item = array[tail++ & (ATA_MAX_QUEUE-1)];
full:
head - tail == ATA_MAX_QUEUE
empty:
head == tail
number of items:
head - tail
This requires that the array size be a power of two.
---
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/
Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
On Wed, 27 Jun 2007 17:09:29 +0800 "Kuan Luo" <[EMAIL PROTECTED]> wrote:
> > +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 = >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.
that's the wrong way of doing a circular buffer..
It's better to allow the head and tail indices to wrap all the way through
0x and only mask them when they are actually used for subscripting.
That way:
add:
array[head++ & (ATA_MAX_QUEUE-1)] = item;
remove:
item = array[tail++ & (ATA_MAX_QUEUE-1)];
full:
head - tail == ATA_MAX_QUEUE
empty:
head == tail
number of items:
head - tail
This requires that the array size be a power of two.
-
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/
RE: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
> +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 = >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,
>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: linux-kernel@vger.kernel.org; [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.0 -0400
> +++ b/sata_nv.c 2007-06-26 12:52:27.0 -0400
Please prepare patches in `pathc -p1' form.
> +typedef struct {
> + u32 defer_bits;
> + u8 front;
> + u8 rear;
> + unsigned inttag[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 = >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 = >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",
Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Peer Chen wrote: We did the many test with the new version driver and didn't encounter that problem, but in certain cases, DMASETUP command packets from drive to the controller are corrupted, and the controller issues an R_ERR to the drive. Drives that comply with SATA spec will re-transmit the corrupted packet and normal operation continues. However, some Maxtor NCQ drives do not re-transmit the DMASETUP command packet, resulting in software timeout for this command. So if you are using the Maxtor HD and meet this problem,please don't enable the NCQ function. Users should not be expected to know those details, nor be forced to suffer discovering this in the field (over and over again for each customer). We have a list of problem devices ata_device_blacklist[] in drivers/ata/libata-core.c that should be modified to note each drive where NCQ should never be used. Jeff - 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/
RE: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
We did the many test with the new version driver and didn't encounter that problem, but in certain cases, DMASETUP command packets from drive to the controller are corrupted, and the controller issues an R_ERR to the drive. Drives that comply with SATA spec will re-transmit the corrupted packet and normal operation continues. However, some Maxtor NCQ drives do not re-transmit the DMASETUP command packet, resulting in software timeout for this command. So if you are using the Maxtor HD and meet this problem,please don't enable the NCQ function. BRs Peer Chen -Original Message- From: Robert Hancock [mailto:[EMAIL PROTECTED] Sent: Wednesday, June 27, 2007 1:07 PM To: kuan luo Cc: linux-kernel@vger.kernel.org; [EMAIL PROTECTED]; [EMAIL PROTECTED]; Peer Chen; Kuan Luo Subject: Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61 kuan luo 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' > > Signed-off-by: Kuan Luo <[EMAIL PROTECTED]> > Signed-off-by: Peer Chen <[EMAIL PROTECTED]> > Haven't reviewed in detail, but does look cleaner than the previous version. Some people reported seeing some unrecognized FIS, etc. errors with the previous version, have those been looked into/fixed? -- Robert Hancock Saskatoon, SK, Canada To email, remove "nospam" from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ --- 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/
RE: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
We did the many test with the new version driver and didn't encounter that problem, but in certain cases, DMASETUP command packets from drive to the controller are corrupted, and the controller issues an R_ERR to the drive. Drives that comply with SATA spec will re-transmit the corrupted packet and normal operation continues. However, some Maxtor NCQ drives do not re-transmit the DMASETUP command packet, resulting in software timeout for this command. So if you are using the Maxtor HD and meet this problem,please don't enable the NCQ function. BRs Peer Chen -Original Message- From: Robert Hancock [mailto:[EMAIL PROTECTED] Sent: Wednesday, June 27, 2007 1:07 PM To: kuan luo Cc: linux-kernel@vger.kernel.org; [EMAIL PROTECTED]; [EMAIL PROTECTED]; Peer Chen; Kuan Luo Subject: Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61 kuan luo 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' Signed-off-by: Kuan Luo [EMAIL PROTECTED] Signed-off-by: Peer Chen [EMAIL PROTECTED] Haven't reviewed in detail, but does look cleaner than the previous version. Some people reported seeing some unrecognized FIS, etc. errors with the previous version, have those been looked into/fixed? -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ --- 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/
Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Peer Chen wrote: We did the many test with the new version driver and didn't encounter that problem, but in certain cases, DMASETUP command packets from drive to the controller are corrupted, and the controller issues an R_ERR to the drive. Drives that comply with SATA spec will re-transmit the corrupted packet and normal operation continues. However, some Maxtor NCQ drives do not re-transmit the DMASETUP command packet, resulting in software timeout for this command. So if you are using the Maxtor HD and meet this problem,please don't enable the NCQ function. Users should not be expected to know those details, nor be forced to suffer discovering this in the field (over and over again for each customer). We have a list of problem devices ata_device_blacklist[] in drivers/ata/libata-core.c that should be modified to note each drive where NCQ should never be used. Jeff - 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/
RE: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
+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: linux-kernel@vger.kernel.org; [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.0 -0400
+++ b/sata_nv.c 2007-06-26 12:52:27.0 -0400
Please prepare patches in `pathc -p1' form.
+typedef struct {
+ u32 defer_bits;
+ u8 front;
+ u8 rear;
+ unsigned inttag[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
Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
On Wed, 27 Jun 2007 17:09:29 +0800 Kuan Luo [EMAIL PROTECTED] wrote:
+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.
that's the wrong way of doing a circular buffer..
It's better to allow the head and tail indices to wrap all the way through
0x and only mask them when they are actually used for subscripting.
That way:
add:
array[head++ (ATA_MAX_QUEUE-1)] = item;
remove:
item = array[tail++ (ATA_MAX_QUEUE-1)];
full:
head - tail == ATA_MAX_QUEUE
empty:
head == tail
number of items:
head - tail
This requires that the array size be a power of two.
-
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/
RE: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
A pretty good way. I will modify my code.
-Original Message-
From: Andrew Morton [mailto:[EMAIL PROTECTED]
Sent: Wednesday, June 27, 2007 5:21 PM
To: Kuan Luo
Cc: linux-kernel@vger.kernel.org; [EMAIL PROTECTED]; Peer Chen
Subject: Re: [PATCH] ata: Add the SW NCQ support to sata_nv for
MCP51/MCP55/MCP61
On Wed, 27 Jun 2007 17:09:29 +0800 Kuan Luo [EMAIL PROTECTED] wrote:
+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.
that's the wrong way of doing a circular buffer..
It's better to allow the head and tail indices to wrap all the way
through
0x and only mask them when they are actually used for
subscripting.
That way:
add:
array[head++ (ATA_MAX_QUEUE-1)] = item;
remove:
item = array[tail++ (ATA_MAX_QUEUE-1)];
full:
head - tail == ATA_MAX_QUEUE
empty:
head == tail
number of items:
head - tail
This requires that the array size be a power of two.
---
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/
RE: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Hi, I am testing your current code with akpm's beautifying patches for about an hour now. I have seen no problems with it so far. **A pretty good way. I will modify my code. Please, cc me when sending your next patch to LKML, thanks. Best regards, Zoltán Böszörményi - 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/
Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
Am Mittwoch 27 Juni 2007 schrieb Peer Chen: We did the many test with the new version driver and didn't encounter that problem, but in certain cases, DMASETUP command packets from drive to the controller are corrupted, and the controller issues an R_ERR to the drive. Drives that comply with SATA spec will re-transmit the corrupted packet and normal operation continues. However, some Maxtor NCQ drives do not re-transmit the DMASETUP command packet, resulting in software timeout for this command. So if you are using the Maxtor HD and meet this problem,please don't enable the NCQ function. Out of interest I tested the patch again with my Maxtor drive I mentioned in the other thread. Ths time it got worse. I couldn't boot anymore, ie init starts some services and then dies at random places as if corrupt data was transfered. With the previous patch the system worked, except the port resets. Anyway, I'll ask Maxtor or rather Seagate to fix their firmware pointing at your diagnosis. bye, -- (°= =°) //\ Prakash Punnoor /\\ V_/ \_V signature.asc Description: This is a digitally signed message part.
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.0 -0400
> +++ b/sata_nv.c 2007-06-26 12:52:27.0 -0400
Please prepare patches in `pathc -p1' form.
> +typedef struct {
> + u32 defer_bits;
> + u8 front;
> + u8 rear;
> + unsigned inttag[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 = >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 = >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, 0x);
> +}
>
> ...
>
> +
> +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;
> +
>
Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
kuan luo 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' Signed-off-by: Kuan Luo <[EMAIL PROTECTED]> Signed-off-by: Peer Chen <[EMAIL PROTECTED]> Haven't reviewed in detail, but does look cleaner than the previous version. Some people reported seeing some unrecognized FIS, etc. errors with the previous version, have those been looked into/fixed? -- Robert Hancock Saskatoon, SK, Canada To email, remove "nospam" from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - 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/
Re: [PATCH] ata: Add the SW NCQ support to sata_nv for MCP51/MCP55/MCP61
kuan luo 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' Signed-off-by: Kuan Luo [EMAIL PROTECTED] Signed-off-by: Peer Chen [EMAIL PROTECTED] Haven't reviewed in detail, but does look cleaner than the previous version. Some people reported seeing some unrecognized FIS, etc. errors with the previous version, have those been looked into/fixed? -- Robert Hancock Saskatoon, SK, Canada To email, remove nospam from [EMAIL PROTECTED] Home Page: http://www.roberthancock.com/ - 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/
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.0 -0400
+++ b/sata_nv.c 2007-06-26 12:52:27.0 -0400
Please prepare patches in `pathc -p1' form.
+typedef struct {
+ u32 defer_bits;
+ u8 front;
+ u8 rear;
+ unsigned inttag[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, 0x);
+}
...
+
+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
