[PATCH 05/37] ata: use get/put_endian helpers

[PATCH 05/37] ata: use get/put_endian helpers

[Harvey Harrison]

Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
---
 drivers/ata/pdc_adma.c   |   12 ++++++------
 drivers/ata/sata_qstor.c |   12 +++++-------
 2 files changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/ata/pdc_adma.c b/drivers/ata/pdc_adma.c
index be53545..162eecb 100644
--- a/drivers/ata/pdc_adma.c
+++ b/drivers/ata/pdc_adma.c
@@ -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 */
diff --git a/drivers/ata/sata_qstor.c b/drivers/ata/sata_qstor.c
index 1600107..7e3c4d5 100644
--- a/drivers/ata/sata_qstor.c
+++ b/drivers/ata/sata_qstor.c
@@ -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;
-- 
1.5.6.rc0.277.g804cf

Re: [PATCH 05/37] ata: use get/put_endian helpers

[Mark Lord]

Harvey Harrison wrote:
> Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> ---
>  drivers/ata/pdc_adma.c   |   12 ++++++------
>  drivers/ata/sata_qstor.c |   12 +++++-------
>  2 files changed, 11 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/ata/pdc_adma.c b/drivers/ata/pdc_adma.c
> index be53545..162eecb 100644
> --- a/drivers/ata/pdc_adma.c
> +++ b/drivers/ata/pdc_adma.c
> @@ -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?

Re: [PATCH 05/37] ata: use get/put_endian helpers

[Andrew Morton]

On Thu, 29 May 2008 22:26:18 -0400 Mark Lord <liml@rtr.ca> wrote:

> Harvey Harrison wrote:
> > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > ---
> >  drivers/ata/pdc_adma.c   |   12 ++++++------
> >  drivers/ata/sata_qstor.c |   12 +++++-------
> >  2 files changed, 11 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/ata/pdc_adma.c b/drivers/ata/pdc_adma.c
> > index be53545..162eecb 100644
> > --- a/drivers/ata/pdc_adma.c
> > +++ b/drivers/ata/pdc_adma.c
> > @@ -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?

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.

Re: [PATCH 05/37] ata: use get/put_endian helpers

[Harvey Harrison]

On Thu, 2008-05-29 at 19:52 -0700, Andrew Morton wrote:
> On Thu, 29 May 2008 22:26:18 -0400 Mark Lord <liml@rtr.ca> 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)

> - 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

Re: [PATCH 05/37] ata: use get/put_endian helpers

[Andrew Morton]

On Thu, 29 May 2008 20:37:40 -0700 Harvey Harrison <harvey.harrison@gmail.com> wrote:

> On Thu, 2008-05-29 at 19:52 -0700, Andrew Morton wrote:
> > On Thu, 29 May 2008 22:26:18 -0400 Mark Lord <liml@rtr.ca> 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)

	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.

> > - 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))).

Re: [PATCH 05/37] ata: use get/put_endian helpers

[Paul Mackerras]

Andrew Morton writes:

> On Thu, 29 May 2008 22:26:18 -0400 Mark Lord <liml@rtr.ca> wrote:
> 
> > Harvey Harrison wrote:
> > > Signed-off-by: Harvey Harrison <harvey.harrison@gmail.com>
> > > ---
> > >  drivers/ata/pdc_adma.c   |   12 ++++++------
> > >  drivers/ata/sata_qstor.c |   12 +++++-------
> > >  2 files changed, 11 insertions(+), 13 deletions(-)
> > > 
> > > diff --git a/drivers/ata/pdc_adma.c b/drivers/ata/pdc_adma.c
> > > index be53545..162eecb 100644
> > > --- a/drivers/ata/pdc_adma.c
> > > +++ b/drivers/ata/pdc_adma.c
> > > @@ -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?
> 
> 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 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).

> - 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.

Re: [PATCH 05/37] ata: use get/put_endian helpers

[Harvey Harrison]

On Thu, 2008-05-29 at 21:08 -0700, Andrew Morton wrote:
> On Thu, 29 May 2008 20:37:40 -0700 Harvey Harrison <harvey.harrison@gmail.com> wrote:
> 
> > On Thu, 2008-05-29 at 19:52 -0700, Andrew Morton wrote:
> > > On Thu, 29 May 2008 22:26:18 -0400 Mark Lord <liml@rtr.ca> wrote:
> > > 

> > 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))).

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

Re: [PATCH 05/37] ata: use get/put_endian helpers

[Harvey Harrison]

On Fri, 2008-05-30 at 14:10 +1000, Paul Mackerras wrote:
> Andrew Morton writes:
> 
> > On Thu, 29 May 2008 22:26:18 -0400 Mark Lord <liml@rtr.ca> 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

Re: [PATCH 05/37] ata: use get/put_endian helpers

[Andrew Morton]

On Fri, 30 May 2008 14:10:47 +1000 Paul Mackerras <paulus@samba.org> wrote:

> > > 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 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).

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.

Re: [PATCH 05/37] ata: use get/put_endian helpers

[Mark Lord]

Andrew Morton wrote:
> On Fri, 30 May 2008 14:10:47 +1000 Paul Mackerras <paulus@samba.org> wrote:
> 
>>>> 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 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).
> 
> 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.
..

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