or51132.c: unaligned

PATCH In review Al Viro 4 participants 8 messages May 21, 2008 Media (V4L/DVB)

lore

Share & tools

lore

Al Viro

author

2008-05-21 00:37 UTC #0 lore raw

PATCH 1/1 or51132.c: unaligned

Signed-off-by Al Viro <[email protected]>

---
 drivers/media/dvb/frontends/or51132.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

drivers/media/dvb/frontends/or51132.c +3 -3

index c7b5785..5ed3254 100644
@@ -126,7 +126,7 @@ static int or51132_readreg(struct or51132_state *state, u8 reg)
 		       reg, err);
 		return -EREMOTEIO;
 	}
-	return le16_to_cpup((u16*)buf);
+	return buf[0] | (buf[1] << 8);
 }
 
 static int or51132_load_firmware (struct dvb_frontend* fe, const struct firmware *fw)

@@ -140,9 +140,9 @@ static int or51132_load_firmware (struct dvb_frontend* fe, const struct firmware
 	dprintk("Firmware is %Zd bytes\n",fw->size);
 
 	/* Get size of firmware A and B */
-	firmwareAsize = le32_to_cpu(*((u32*)fw->data));
+	firmwareAsize = le32_to_cpu(*((__le32*)fw->data));
 	dprintk("FirmwareA is %i bytes\n",firmwareAsize);
-	firmwareBsize = le32_to_cpu(*((u32*)(fw->data+4)));
+	firmwareBsize = le32_to_cpu(*((__le32*)(fw->data+4)));
 	dprintk("FirmwareB is %i bytes\n",firmwareBsize);
 
 	/* Upload firmware */

Signature · 1 line

1.5.3.GIT

Harvey Harrison

2008-05-21 00:41 UTC #1 lore raw

Quoted 10 lines · Al Viro

Signed-off-by Al Viro <[email protected]>
---
 drivers/media/dvb/frontends/or51132.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
drivers/media/dvb/frontends/or51132.c +0 -0
index c7b5785..5ed3254 100644

drivers/media/dvb/frontends/or51132.c static int or51132_readreg(struct or51132_state *state, u8 reg) · line 129

Quoted 5 lines

drivers/media/dvb/frontends/or51132.c +1 -1

@@ -126,7 +126,7 @@ static int or51132_readreg(struct or51132_state *state, u8 reg)
 		       reg, err);
 		return -EREMOTEIO;
 	}
-	return le16_to_cpup((u16*)buf);
+	return buf[0] | (buf[1] << 8);

return get_unaligned_le16(buf);

drivers/media/dvb/frontends/or51132.c static int or51132_load_firmware (struct dvb_frontend* fe, const struct firmware · line 143

Quoted 9 lines

drivers/media/dvb/frontends/or51132.c +1 -1

 }
 
 static int or51132_load_firmware (struct dvb_frontend* fe, const struct firmware *fw)

@@ -140,9 +140,9 @@ static int or51132_load_firmware (struct dvb_frontend* fe, const struct firmware
 	dprintk("Firmware is %Zd bytes\n",fw->size);
 
 	/* Get size of firmware A and B */
-	firmwareAsize = le32_to_cpu(*((u32*)fw->data));
+	firmwareAsize = le32_to_cpu(*((__le32*)fw->data));

firmwareAsize = le32_to_cpup((__le32 *)fw->data);

drivers/media/dvb/frontends/or51132.c static int or51132_load_firmware (struct dvb_frontend* fe, const struct firmware · line 145

Quoted 3 lines

drivers/media/dvb/frontends/or51132.c +1 -1

 	dprintk("FirmwareA is %i bytes\n",firmwareAsize);
-	firmwareBsize = le32_to_cpu(*((u32*)(fw->data+4)));
+	firmwareBsize = le32_to_cpu(*((__le32*)(fw->data+4)));

firmwareBsize = le32_to_cpup((__le32 *)(fw->data + 4));

Cheers,

Harvey

Al Viro

2008-05-21 00:45 UTC #2 lore raw

Quoted 3 lines · Harvey Harrison

drivers/media/dvb/frontends/or51132.c static int or51132_readreg(struct or51132_state *state, u8 reg) · line 129
Quoted 1 line
drivers/media/dvb/frontends/or51132.c +1 -0
+	return buf[0] | (buf[1] << 8);
return get_unaligned_le16(buf);

And the point of that would be?

Quoted 3 lines

drivers/media/dvb/frontends/or51132.c static int or51132_load_firmware (struct dvb_frontend* fe, const struct firmware · line 143
Quoted 1 line
drivers/media/dvb/frontends/or51132.c +1 -0
+	firmwareAsize = le32_to_cpu(*((__le32*)fw->data));
???firmwareAsize = le32_to_cpup((__le32 *)fw->data);

... and the point of that would be? FWIW, I really don't like the ...p() forms - they are hard to distinguish from normal ones visually and any possible performance benefit is too small for most of the uses. IOW, it's mostly redundant API.

Harvey Harrison

2008-05-21 00:51 UTC #3 lore raw

Quoted 7 lines · Al Viro

Quoted 3 lines · Harvey Harrison
drivers/media/dvb/frontends/or51132.c static int or51132_readreg(struct or51132_state *state, u8 reg) · line 129
Quoted 1 line
drivers/media/dvb/frontends/or51132.c +1 -0
+	return buf[0] | (buf[1] << 8);
return get_unaligned_le16(buf);
And the point of that would be?

Other than using the available helper, none.

Quoted 9 lines

Quoted 4 lines
drivers/media/dvb/frontends/or51132.c static int or51132_load_firmware (struct dvb_frontend* fe, const struct firmware · line 143
Quoted 2 lines
drivers/media/dvb/frontends/or51132.c +1 -0
+	firmwareAsize = le32_to_cpu(*((__le32*)fw->data));
???firmwareAsize = le32_to_cpup((__le32 *)fw->data);
... and the point of that would be? FWIW, I really don't like the ...p() forms - they are hard to distinguish from normal ones visually and any possible performance benefit is too small for most of the uses. IOW, it's mostly redundant API.

I sent a patchset getting rid of the p variants earlier today, Dave Miller made a good point that some arches have well optimized versions of these as they have specific machine instructions they can use.

Agreed that I don't much like them either, was thinking of adding a u32 get_le32(__le32 *ptr) type api and get rid of the le32_to_cpup api.

Cheers,

Harvey

Al Viro

2008-05-21 00:57 UTC #4 lore raw

Quoted 8 lines · Harvey Harrison

Quoted 4 lines
... and the point of that would be? FWIW, I really don't like the ...p() forms - they are hard to distinguish from normal ones visually and any possible performance benefit is too small for most of the uses. IOW, it's mostly redundant API.
I sent a patchset getting rid of the p variants earlier today, Dave Miller made a good point that some arches have well optimized versions of these as they have specific machine instructions they can use.

Thus the "mostly" part - on hot paths the microoptimization is worth the trouble. None of the places in question is on such paths...

Linus Torvalds

2008-05-21 00:56 UTC #5 lore raw

In reply to Al Viro

Quoted 7 lines · Al Viro

Quoted 3 lines · Harvey Harrison
drivers/media/dvb/frontends/or51132.c static int or51132_readreg(struct or51132_state *state, u8 reg) · line 129
Quoted 1 line
drivers/media/dvb/frontends/or51132.c +1 -0
+	return buf[0] | (buf[1] << 8);
return get_unaligned_le16(buf);
And the point of that would be?

Perhaps better code generation?

I didn't look what

	buf[0] | (buf[1] << 8)

generates, but on x86, get_unaligned_le16() should just boil down to

*(unsigned short *)buf

which is certainly going to mostly generate better code (smaller, faster) than at least the most likely and straigthforward of the former.

(Just checked. Gcc is not smart enough to make those two loads plus a shift be one word load. At least not my version)

Linus

Al Viro

2008-05-21 01:18 UTC #6 lore raw

Quoted 7 lines · Linus Torvalds

Quoted 5 lines
Quoted 3 lines
drivers/media/dvb/frontends/or51132.c static int or51132_readreg(struct or51132_state *state, u8 reg) · line 129
Quoted 1 line
drivers/media/dvb/frontends/or51132.c +1 -0
+	return buf[0] | (buf[1] << 8);
return get_unaligned_le16(buf);
And the point of that would be?
Perhaps better code generation?

FWIW, I wonder how they really compare on misaligned and whether it would make sense for gcc to try and generate a single load on targets that are known to allow that...

Hell knows; I still find explicit variant more readable in this case and AFAICS it's not a critical path...

Linus Torvalds

2008-05-21 02:02 UTC #7 lore raw

Quoted 4 lines · Al Viro

FWIW, I wonder how they really compare on misaligned and whether it would make sense for gcc to try and generate a single load on targets that are known to allow that...

It would almost certainly help on x86. The cost of an unaligned integer access that doesn't cross a cache-fetch boundary (8 bytes on older CPU's, 16 or 32 bytes on newer ones) is zero, last I saw. IOW, there are misaligned cases that have a higher cost, but they are pretty rare, and especially so with small data and modern CPU's.

So no disadvantage for 95% of all cases, and the advantage of doing just a single instruction, rather than four (2 zero-extending loads, a shift and an add/or, with data dependencies on most of them).

Linus