ata: use get/put_endian helpers
Subsystems (2)
PATCH 5/37 ata: use get/put_endian helpers
--- drivers/ata/pdc_adma.c | 12 ++++++------ drivers/ata/sata_qstor.c | 12 +++++------- 2 files changed, 11 insertions(+), 13 deletions(-)
drivers/ata/pdc_adma.c +6 -6
@@ -284,11 +284,11 @@ static int adma_fill_sg(struct ata_queued_cmd *qc)u32 len; addr = (u32 )sg_dma_address (sg); - *(__le32 *)(buf + i) =cpu_to_le32 (addr); +put_le32 (addr, (__le32 *)(buf + i)); i +=4 ; len =sg_dma_len (sg) >>3 ; - *(__le32 *)(buf + i) =cpu_to_le32 (len); +put_le32 (len, (__le32 *)(buf + i)); i +=4 ; last_buf = &buf[i];
@@ -297,8 +297,8 @@ static int adma_fill_sg(struct ata_queued_cmd *qc) buf[i++] =0 ;/* pPKLW */ buf[i++] =0 ;/* reserved */ - *(__le32 *)(buf + i) = - (pFLAGS & pEND) ?0 :cpu_to_le32 (pp->pkt_dma + i +4 ); +put_le32 ((pFLAGS & pEND) ?0 : pp->pkt_dma + i +4 , + (__le32 *)(buf + i)); i +=4 ; VPRINTK("PRD[%u] = (0x%lX, 0x%X)\n", i/4,
@@ -331,7 +331,7 @@ static void adma_qc_prep(struct ata_queued_cmd *qc) buf[i++] = cVLD | cDAT | cIEN; i++;/* cLEN, gets filled in below */ - *(__le32 *)(buf+i) =cpu_to_le32 (pkt_dma);/* cNCPB */ +put_le32 (pkt_dma, (__le32 *)(buf + i));/* cNCPB */ i +=4 ;/* cNCPB */ i +=4 ;/* cPRD, gets filled in below */
@@ -369,7 +369,7 @@ static void adma_qc_prep(struct ata_queued_cmd *qc) buf[i++] =ADMA_REGS_COMMAND | rEND; buf[3 ] = (i >>3 ) -2 ;/* cLEN */ - *(__le32 *)(buf+8 ) =cpu_to_le32 (pkt_dma + i);/* cPRD */ +put_le32 (pkt_dma + i, (__le32 *)(buf +8 ));/* cPRD */ i =adma_fill_sg (qc);wmb ();/* flush PRDs and pkt to memory */
drivers/ata/sata_qstor.c +5 -7
@@ -277,11 +277,11 @@ static unsigned int qs_fill_sg(struct ata_queued_cmd *qc)u32 len; addr =sg_dma_address (sg); - *(__le64 *)prd =cpu_to_le64 (addr); +put_le64 (addr, (__le64 *)prd); prd +=sizeof (u64); len =sg_dma_len (sg); - *(__le32 *)prd =cpu_to_le32 (len); +put_le32 (len, (__le32 *)prd); prd +=sizeof (u64); VPRINTK("PRD[%u] = (0x%llX, 0x%X)\n", si,
@@ -296,7 +296,6 @@ static void qs_qc_prep(struct ata_queued_cmd *qc)struct qs_port_priv *pp = qc->ap->private_data;u8 dflags =QS_DF_PORD , *buf = pp->pkt;u8 hflags =QS_HF_DAT |QS_HF_IEN |QS_HF_VLD ; -u64 addr;unsigned int nelem;VPRINTK ("ENTER\n" );
@@ -317,10 +316,9 @@ static void qs_qc_prep(struct ata_queued_cmd *qc)/* host control block (HCB) */ buf[0 ] =QS_HCB_HDR ; buf[1 ] = hflags; - *(__le32 *)(&buf[4 ]) =cpu_to_le32 (qc->nbytes); - *(__le32 *)(&buf[8 ]) =cpu_to_le32 (nelem); - addr = ((u64 )pp->pkt_dma) +QS_CPB_BYTES ; - *(__le64 *)(&buf[16 ]) =cpu_to_le64 (addr); +put_le32 (qc->nbytes, (__le32 *)&buf[4 ]); +put_le32 (nelem, (__le32 *)&buf[8 ]); +put_le64 ((u64 )pp->pkt_dma +QS_CPB_BYTES , (__le64 *)&buf[16 ]);/* device control block (DCB) */ buf[24 ] =QS_DCB_HDR ;
Signature · 1 line
1.5.6.rc0.277.g804cf
Quoted 11 lines · Harvey Harrison
Signed-off-by Harvey Harrison <[email protected]>--- drivers/ata/pdc_adma.c | 12 ++++++------ drivers/ata/sata_qstor.c | 12 +++++------- 2 files changed, 11 insertions(+), 13 deletions(-)drivers/ata/pdc_adma.c +0
-0
drivers/ata/pdc_adma.c static int adma_fill_sg(struct ata_queued_cmd *qc) · line 287
Quoted 5 lines
drivers/ata/pdc_adma.c +1
-1@@ -284,11 +284,11 @@ static int adma_fill_sg(struct ata_queued_cmd *qc)u32 len; addr = (u32 )sg_dma_address (sg); - *(__le32 *)(buf + i) =cpu_to_le32 (addr); +put_le32 (addr, (__le32 *)(buf + i));
..
I don't get it.
What's the point here?
Quoted 14 lines · On Thu, 29 May 2008 22:26:18 -0400 Mark Lord <[email protected]> wrote:
Quoted 13 lines · Harvey Harrison
Signed-off-by Harvey Harrison <[email protected]>--- drivers/ata/pdc_adma.c | 12 ++++++------ drivers/ata/sata_qstor.c | 12 +++++------- 2 files changed, 11 insertions(+), 13 deletions(-)drivers/ata/pdc_adma.c +0
-0@@ -284,11 +284,11 @@ static int adma_fill_sg(struct ata_queued_cmd *qc)u32 len;
Quoted 8 lines
drivers/ata/pdc_adma.c static int adma_fill_sg(struct ata_queued_cmd *qc) · line 287
Quoted 3 lines
drivers/ata/pdc_adma.c +1
-1addr = (u32 )sg_dma_address (sg); - *(__le32 *)(buf + i) =cpu_to_le32 (addr); +put_le32 (addr, (__le32 *)(buf + i));..
I don't get it.
What's the point here?
Readability and maintainability. Once one becomes familar with a particular idion like this you can read it and say "ah, I know what that's doing" rather than having to peer at every character and work it out at each site where it happens.
As usual: a PITA now, but better long-term.
otoh,
- I think the args are backwards
- I don't like the use of the put_*() namespace. It makes it look like a uaccess operation.
Quoted 7 lines · Andrew Morton
On Thu, 29 May 2008 22:26:18 -0400 Mark Lord <[email protected]> wrote:
otoh,
- I think the args are backwards
It was made to look like put_unaligned_* and put_unaligned()
I think of it as put_le16(source, dest)
Quoted 2 lines
- I don't like the use of the put_*() namespace. It makes it look like a uaccess operation.
I'm flexible...although I'd probably get nailed to a cross for changing it again so soon. (the unaligned versions, that is)
Harvey
Quoted 4 lines · On Thu, 29 May 2008 20:37:40 -0700 Harvey Harrison <[email protected]>…
Quoted 2 lines · Andrew Morton
On Thu, 29 May 2008 22:26:18 -0400 Mark Lord <[email protected]> wrote:
Quoted 8 lines
Quoted 4 lines
otoh,
- I think the args are backwards
It was made to look like put_unaligned_* and
put_unaligned()I think of it as
put_le16(source, dest)
fn(dest, src)
is a well-known C idiom which the kernel went and broke in lots of places :(
It maps nicely onto
dest = src
as a mnemonic.
But given that the compiler will reliably barf if they are backwards it's a minor thing.
Quoted 5 lines
Quoted 2 lines
- I don't like the use of the put_*() namespace. It makes it look like a uaccess operation.
I'm flexible...although I'd probably get nailed to a cross for changing it again so soon. (the unaligned versions, that is)
We'll live.
(I'm not a big fan of the patches, btw. It's more of an "oh groan, because it's there I suppose we really ought to do this" sort of thing. (strong objections would save me a lot of work (looks hopefully at inbox))).
Quoted 9 lines · Andrew Morton
Quoted 6 lines · On Thu, 29 May 2008 20:37:40 -0700 Harvey Harrison <[email protected]>…
Quoted 2 lines · Andrew Morton
On Thu, 29 May 2008 22:26:18 -0400 Mark Lord <[email protected]> wrote:
I'm flexible...although I'd probably get nailed to a cross for changing it again so soon. (the unaligned versions, that is)
Quoted 6 lines
We'll live.
(I'm not a big fan of the patches, btw. It's more of an "oh groan, because it's there I suppose we really ought to do this" sort of thing. (strong objections would save me a lot of work (looks hopefully at inbox))).
Well in the case of the le16_to_cpu(*(__le16 *)ptr), it can be profitable to use the le16_to_cpup..or get_le16 directly as some arches have optimized versions of the pointer versions.
Harvey
Quoted 30 lines · Andrew Morton
Quoted 14 lines · On Thu, 29 May 2008 22:26:18 -0400 Mark Lord <[email protected]> wrote:
Quoted 13 lines · Harvey Harrison
Signed-off-by Harvey Harrison <[email protected]>--- drivers/ata/pdc_adma.c | 12 ++++++------ drivers/ata/sata_qstor.c | 12 +++++------- 2 files changed, 11 insertions(+), 13 deletions(-)drivers/ata/pdc_adma.c +0
-0@@ -284,11 +284,11 @@ static int adma_fill_sg(struct ata_queued_cmd *qc)u32 len;Quoted 8 lines
drivers/ata/pdc_adma.c static int adma_fill_sg(struct ata_queued_cmd *qc) · line 287
Quoted 3 lines
drivers/ata/pdc_adma.c +1
-1addr = (u32 )sg_dma_address (sg); - *(__le32 *)(buf + i) =cpu_to_le32 (addr); +put_le32 (addr, (__le32 *)(buf + i));..
I don't get it.
What's the point here?
Readability and maintainability. Once one becomes familar with a particular idion like this you can read it and say "ah, I know what that's doing" rather than having to peer at every character and work it out at each site where it happens.
Quoted 5 lines
As usual: a PITA now, but better long-term.
otoh,
- I think the args are backwards
I think you just admitted that the new version is less readable. At least with an '=' operator you know which side is the thing that's being modified. With a put_XXX function, I would have to go look up the definition (particularly since outb et al. are also the wrong way around, i.e. have the destination as the second argument).
Quoted 2 lines
- I don't like the use of the put_*() namespace. It makes it look like a uaccess operation.
Plus we have a common idiom that get_*() functions increment some sort of reference count, and put_*() functions decrease a reference count and may free something...
Paul.
Quoted 7 lines · Paul Mackerras
Quoted 1 line · Andrew Morton
On Thu, 29 May 2008 22:26:18 -0400 Mark Lord <[email protected]> wrote:
Plus we have a common idiom that get_*() functions increment some sort of reference count, and put_*() functions decrease a reference count and may free something...
Except for get_unaligned(), put_unaligned()
But point taken.
Harvey
Quoted 11 lines · On Fri, 30 May 2008 14:10:47 +1000 Paul Mackerras <[email protected]> wrote:
Quoted 11 lines
Quoted 1 line
What's the point here?
Readability and maintainability. Once one becomes familar with a particular idion like this you can read it and say "ah, I know what that's doing" rather than having to peer at every character and work it out at each site where it happens.
As usual: a PITA now, but better long-term.
otoh,
Quoted 7 lines
Quoted 1 line
- I think the args are backwards
I think you just admitted that the new version is less readable. At least with an '=' operator you know which side is the thing that's being modified. With a
put_XXXfunction, I would have to go look up the definition (particularly since outb et al. are also the wrong way around, i.e. have the destination as the second argument).
Well yes, but you don't have to worry about that when reviewing because you know the compiler will catch reversals.
Still not terribly keen about it all, but geeze the code which it is trying to clarify is godawful:
*(__le32 *)(buf + i) = cpu_to_le32(addr);
wtf?
Then again the replacement
put_le32(addr, (__le32 *)(buf + i));
is still wtf.
Quoted 27 lines · Andrew Morton
Quoted 8 lines · On Fri, 30 May 2008 14:10:47 +1000 Paul Mackerras <[email protected]> wrote:
Quoted 8 lines
Quoted 1 line
What's the point here?
Readability and maintainability. Once one becomes familar with a particular idion like this you can read it and say "ah, I know what that's doing" rather than having to peer at every character and work it out at each site where it happens.
As usual: a PITA now, but better long-term.
Quoted 8 lines
Quoted 3 lines
otoh,
- I think the args are backwards
I think you just admitted that the new version is less readable. At least with an '=' operator you know which side is the thing that's being modified. With a
put_XXXfunction, I would have to go look up the definition (particularly since outb et al. are also the wrong way around, i.e. have the destination as the second argument).Well yes, but you don't have to worry about that when reviewing because you know the compiler will catch reversals.
Still not terribly keen about it all, but geeze the code which it is trying to clarify is godawful:
*(__le32 *)(buf + i) = cpu_to_le32(addr);
Quoted 7 lines
wtf?
Then again the replacement
put_le32(addr, (__le32 *)(buf + i));is still wtf.
..
Exactly my point. With the "improved" version, I now have to go hunting for yet another macro in yet another header file in order to see what the code is doing and to verify correctness. And I (and other reviewers) are just a teensy bit less likely to follow through with that every time, meaning we'll miss bugs.
This is somewhat akin to Linus's dislike of typedefs for structs. It just hides what's really going on, for no obvious gain. Sorry. :)
Cheers