| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <20070517131235.8f9445cc.akpm@linux-foundation.org>
Date: Thu, 17 May 2007 13:12:35 -0700
From: Andrew Morton <akpm@...ux-foundation.org>
To: "Peer Chen" <pchen@...dia.com>
Cc: <linux-kernel@...r.kernel.org>, "Jeff Garzik" <jeff@...zik.org>,
"Robert Hancock" <hancockr@...w.ca>, "Kuan Luo" <kluo@...dia.com>
Subject: Re: [PATCH] drivers/ata: Add the SW NCQ support to sata_nv for
MCP51/MCP55/MCP61
On Thu, 17 May 2007 18:15:45 +0800
"Peer Chen" <pchen@...dia.com> wrote:
> Add the Software NCQ support to sata_nv.c for MCP51/MCP55/MCP61 SATA
> controller.
>
> This patch base on sata_nv.c file from kernel 2.6.22-rc1
>
> See attachment for the patch.
>
> Signed-off-by: Kuan Luo <kluo@...dia.com>
> Signed-off-by: Peer Chen <pchen@...dia.com>
Please don't send patches as application/octet-stream attachments. Inlined
in the body is preferred, or at least text/plain attachments.
The patch generates warnings:
drivers/ata/sata_nv.c:2118: warning: cast from pointer to integer of different size
drivers/ata/sata_nv.c:2118: warning: cast to pointer from integer of different size
due to some quite suspicious-looknig code:
prd = (struct ata_prd*)((u64)pp->prd + ATA_PRD_TBL_SZ*qc->tag);
we have
struct ata_prd {
u32 addr;
u32 flags_len;
};
and the code is casting a pointer to this into a pointer to u64, then
adding stuff to it, then casting it back to the correct type.
Can't we simply do this?
--- a/drivers/ata/sata_nv.c~drivers-ata-add-sw-ncq-support-to-sata_nv-for-mcp51-mcp55-mcp61-fix
+++ a/drivers/ata/sata_nv.c
@@ -2115,7 +2115,7 @@ static void nv_fill_sg(struct ata_queued
WARN_ON(qc->__sg == NULL);
WARN_ON(qc->n_elem == 0 && qc->pad_len == 0);
- prd = (struct ata_prd*)((u64)pp->prd + ATA_PRD_TBL_SZ*qc->tag);
+ prd = pp->prd + ATA_PRD_TBL_SZ*qc->tag;
idx = 0;
ata_for_each_sg(sg, qc) {
_
Also, please please please do not do this:
static void nv_fill_sg(struct ata_queued_cmd *qc)
{
struct ata_port *ap = qc->ap;
struct scatterlist *sg;
unsigned int idx;
struct nv_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);
the random meaningless blank lines between the definitions of the local
variables makes it quite hard to read the code. I actually had to do a
text search to find the definition of "pp" because that trick hid it so
well.
Your patch added tremendous amounts of new trailing whitespace.
One function was indented an extra tabstop.
Some whitespace was broken.
diff -puN drivers/ata/sata_nv.c~drivers-ata-add-sw-ncq-support-to-sata_nv-for-mcp51-mcp55-mcp61-fix-tidy drivers/ata/sata_nv.c
--- a/drivers/ata/sata_nv.c~drivers-ata-add-sw-ncq-support-to-sata_nv-for-mcp51-mcp55-mcp61-fix-tidy
+++ a/drivers/ata/sata_nv.c
@@ -2107,9 +2107,7 @@ static void nv_fill_sg(struct ata_queued
struct ata_port *ap = qc->ap;
struct scatterlist *sg;
unsigned int idx;
-
struct nv_port_priv *pp = ap->private_data;
-
struct ata_prd *prd;
WARN_ON(qc->__sg == NULL);
@@ -2194,11 +2192,10 @@ static unsigned int nv_qc_issue_prot(str
{
struct ata_port *ap = qc->ap;
struct nv_port_priv *pp = ap->private_data;
-
struct ata_taskfile *tf = &(qc->tf);
struct ata_taskfile *ttf, rtf;
-
u32 fis, stat0, stat1;
+
ttf = &rtf;
if (qc->tf.protocol != ATA_PROT_NCQ)
@@ -2338,32 +2335,29 @@ static void fis_dump(struct ata_port *ap
void ncq_hotplug(struct ata_port *ap, u32 fis)
{
+ u32 serror;
+ struct ata_eh_info *ehi = &ap->eh_info;
- u32 serror;
- struct ata_eh_info *ehi = &ap->eh_info;
+ ata_ehi_clear_desc(ehi);
- ata_ehi_clear_desc(ehi);
-
- /* AHCI needs SError cleared; otherwise, it might lock up */
- sata_scr_read(ap, SCR_ERROR, &serror);
- sata_scr_write(ap, SCR_ERROR, serror);
+ /* AHCI needs SError cleared; otherwise, it might lock up */
+ sata_scr_read(ap, SCR_ERROR, &serror);
+ sata_scr_write(ap, SCR_ERROR, serror);
- /* analyze @irq_stat */
- ata_ehi_push_desc(ehi, "fis_stat 0x%08x", fis);
+ /* analyze @irq_stat */
+ ata_ehi_push_desc(ehi, "fis_stat 0x%08x", fis);
- ata_ehi_hotplugged(ehi);
+ ata_ehi_hotplugged(ehi);
- /* okay, let's hand over to EH */
- ehi->serror |= serror;
+ /* okay, let's hand over to EH */
+ ehi->serror |= serror;
- ata_port_freeze(ap);
+ ata_port_freeze(ap);
}
-
static int ncq_interrupt(struct ata_port *ap, u32 fis)
{
struct nv_port_priv *pp = ap->private_data;
-
u32 rc = 0;
u32 tag;
u8 ata_stat;
@@ -2371,7 +2365,6 @@ static int ncq_interrupt(struct ata_port
if (!fis)
return 0;
-
ata_stat = ap->ops->check_status(ap);
ap->ops->irq_clear(ap); /* clear bm irq */
@@ -2398,7 +2391,7 @@ static int ncq_interrupt(struct ata_port
set_back_byte(pp, pp->current_tag);
NPRINTK("BACK OUT FIS:%x \n", fis);
rc = 1;
- } /* first handle back out */
+ } /* first handle back out */
if (fis & NV_INT_SDBFIS_MCP55) {
pp->ops->clear_singlefis(ap, NV_INT_SDBFIS_MCP55 | NV_INT_DEV_MCP55);
_
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists