Staging: vt6655: Fix sparse warning. Restricted cast.

PATCH In review SilverPlate3 3 participants 4 messages Jan 9, 2024 – Jan 10, 2024 1 message not in the archive Staging

Share & tools

SilverPlate3

author

2024-01-09 07:27 UTC #0 lore raw

PATCH 1/1 Staging: vt6655: Fix sparse warning. Restricted cast.

Running 'make M=drivers/staging/vt6655 C=2'
causes sparse to generate few warnings.
This patch fixes the following warnings by ensuring le64_to_cpu handles only __le64 values, thus dismissing chances of bad endianness.
* drivers/staging/vt6655/card.c:302:45: warning: cast to restricted __le64
* drivers/staging/vt6655/card.c:336:23: warning: cast to restricted __le64
* drivers/staging/vt6655/card.c:804:23: warning: cast to restricted __le64
* drivers/staging/vt6655/card.c:831:18: warning: cast to restricted __le64

Signed-off-by Ariel Silver <[email protected]>

---
 drivers/staging/vt6655/card.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/staging/vt6655/card.c +12 -4

index 350ab8f3778a..5dc2200466b7 100644
@@ -292,6 +292,7 @@ bool card_update_tsf(struct vnt_private *priv, unsigned char rx_rate,
 {
 	u64 local_tsf;
 	u64 qwTSFOffset = 0;
+	__le64 le_qwTSFOffset = 0;
 
 	local_tsf = vt6655_get_current_tsf(priv);

@@ -299,7 +300,8 @@ bool card_update_tsf(struct vnt_private *priv, unsigned char rx_rate,
 		qwTSFOffset = CARDqGetTSFOffset(rx_rate, qwBSSTimestamp,
 						local_tsf);
 		/* adjust TSF, HW's TSF add TSF Offset reg */
-		qwTSFOffset =  le64_to_cpu(qwTSFOffset);
+		le_qwTSFOffset = cpu_to_le64(qwTSFOffset);
+		qwTSFOffset = le64_to_cpu(le_qwTSFOffset);
 		iowrite32((u32)qwTSFOffset, priv->port_offset + MAC_REG_TSFOFST);
 		iowrite32((u32)(qwTSFOffset >> 32), priv->port_offset + MAC_REG_TSFOFST + 4);
 		vt6655_mac_reg_bits_on(priv->port_offset, MAC_REG_TFTCTL, TFTCTL_TSFSYNCEN);

@@ -324,6 +326,7 @@ bool CARDbSetBeaconPeriod(struct vnt_private *priv,
 			  unsigned short wBeaconInterval)
 {
 	u64 qwNextTBTT;
+	__le64 le_qwNextTBTT = 0;
 
 	qwNextTBTT = vt6655_get_current_tsf(priv); /* Get Local TSF counter */

@@ -333,7 +336,8 @@ bool CARDbSetBeaconPeriod(struct vnt_private *priv,
 	iowrite16(wBeaconInterval, priv->port_offset + MAC_REG_BI);
 	priv->wBeaconInterval = wBeaconInterval;
 	/* Set NextTBTT */
-	qwNextTBTT =  le64_to_cpu(qwNextTBTT);
+	le_qwNextTBTT = cpu_to_le64(qwNextTBTT);
+	qwNextTBTT = le64_to_cpu(le_qwNextTBTT);
 	iowrite32((u32)qwNextTBTT, priv->port_offset + MAC_REG_NEXTTBTT);
 	iowrite32((u32)(qwNextTBTT >> 32), priv->port_offset + MAC_REG_NEXTTBTT + 4);
 	vt6655_mac_reg_bits_on(priv->port_offset, MAC_REG_TFTCTL, TFTCTL_TBTTSYNCEN);

@@ -796,12 +800,14 @@ void CARDvSetFirstNextTBTT(struct vnt_private *priv,
 {
 	void __iomem *iobase = priv->port_offset;
 	u64 qwNextTBTT;
+	__le64 le_qwNextTBTT = 0;
 
 	qwNextTBTT = vt6655_get_current_tsf(priv); /* Get Local TSF counter */
 
 	qwNextTBTT = CARDqGetNextTBTT(qwNextTBTT, wBeaconInterval);
 	/* Set NextTBTT */
-	qwNextTBTT =  le64_to_cpu(qwNextTBTT);
+	le_qwNextTBTT = cpu_to_le64(qwNextTBTT);
+	qwNextTBTT = le64_to_cpu(le_qwNextTBTT);
 	iowrite32((u32)qwNextTBTT, iobase + MAC_REG_NEXTTBTT);
 	iowrite32((u32)(qwNextTBTT >> 32), iobase + MAC_REG_NEXTTBTT + 4);
 	vt6655_mac_reg_bits_on(iobase, MAC_REG_TFTCTL, TFTCTL_TBTTSYNCEN);

@@ -825,10 +831,12 @@ void CARDvUpdateNextTBTT(struct vnt_private *priv, u64 qwTSF,
 			 unsigned short wBeaconInterval)
 {
 	void __iomem *iobase = priv->port_offset;
+	__le64 le_qwTSF = 0;
 
 	qwTSF = CARDqGetNextTBTT(qwTSF, wBeaconInterval);
 	/* Set NextTBTT */
-	qwTSF =  le64_to_cpu(qwTSF);
+	le_qwTSF = cpu_to_le64(qwTSF);
+	qwTSF = le64_to_cpu(le_qwTSF);
 	iowrite32((u32)qwTSF, iobase + MAC_REG_NEXTTBTT);
 	iowrite32((u32)(qwTSF >> 32), iobase + MAC_REG_NEXTTBTT + 4);
 	vt6655_mac_reg_bits_on(iobase, MAC_REG_TFTCTL, TFTCTL_TBTTSYNCEN);

Signature · 1 line

2.25.1

Greg KH

maintainer

2024-01-09 07:49 UTC #1 lore raw

Quoted 10 lines · On Tue, Jan 09, 2024 at 09:27:04AM +0200, SilverPlate3 wrote:

Running 'make M=drivers/staging/vt6655 C=2'
causes sparse to generate few warnings.
This patch fixes the following warnings by ensuring le64_to_cpu handles only __le64 values, thus dismissing chances of bad endianness.
* drivers/staging/vt6655/card.c:302:45: warning: cast to restricted __le64
* drivers/staging/vt6655/card.c:336:23: warning: cast to restricted __le64
* drivers/staging/vt6655/card.c:804:23: warning: cast to restricted __le64
* drivers/staging/vt6655/card.c:831:18: warning: cast to restricted __le64
Signed-off-by Ariel Silver <[email protected]>

From: line doesn't match the signed-off-by line :(

Quoted 17 lines

---
 drivers/staging/vt6655/card.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

drivers/staging/vt6655/card.c +1 -0

index 350ab8f3778a..5dc2200466b7 100644
@@ -292,6 +292,7 @@ bool card_update_tsf(struct vnt_private *priv, unsigned char rx_rate,
 {
 	u64 local_tsf;
 	u64 qwTSFOffset = 0;
+	__le64 le_qwTSFOffset = 0;
 
 	local_tsf = vt6655_get_current_tsf(priv);

drivers/staging/vt6655/card.c bool card_update_tsf(struct vnt_private *priv, unsigned char rx_rate, · line 304

Quoted 6 lines

drivers/staging/vt6655/card.c +2 -1

@@ -299,7 +300,8 @@ bool card_update_tsf(struct vnt_private *priv, unsigned char rx_rate,
 		qwTSFOffset = CARDqGetTSFOffset(rx_rate, qwBSSTimestamp,
 						local_tsf);
 		/* adjust TSF, HW's TSF add TSF Offset reg */
-		qwTSFOffset =  le64_to_cpu(qwTSFOffset);
+		le_qwTSFOffset = cpu_to_le64(qwTSFOffset);
+		qwTSFOffset = le64_to_cpu(le_qwTSFOffset);

Are you sure about this? Please verify just what you are doing here please...

thanks,

greg k-h

Dan Carpenter

2024-01-09 12:22 UTC #2 lore raw

In reply to SilverPlate3

Quoted 27 lines · On Tue, Jan 09, 2024 at 09:27:04AM +0200, SilverPlate3 wrote:

Running 'make M=drivers/staging/vt6655 C=2'
causes sparse to generate few warnings.
This patch fixes the following warnings by ensuring le64_to_cpu handles only __le64 values, thus dismissing chances of bad endianness.
* drivers/staging/vt6655/card.c:302:45: warning: cast to restricted __le64
* drivers/staging/vt6655/card.c:336:23: warning: cast to restricted __le64
* drivers/staging/vt6655/card.c:804:23: warning: cast to restricted __le64
* drivers/staging/vt6655/card.c:831:18: warning: cast to restricted __le64
Signed-off-by Ariel Silver <[email protected]>
---
 drivers/staging/vt6655/card.c | 16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)
drivers/staging/vt6655/card.c +1 -0
index 350ab8f3778a..5dc2200466b7 100644
@@ -292,6 +292,7 @@ bool card_update_tsf(struct vnt_private *priv, unsigned char rx_rate,
 {
 	u64 local_tsf;
 	u64 qwTSFOffset = 0;
+	__le64 le_qwTSFOffset = 0;
 
 	local_tsf = vt6655_get_current_tsf(priv);
 

drivers/staging/vt6655/card.c bool card_update_tsf(struct vnt_private *priv, unsigned char rx_rate, · line 304

Quoted 6 lines

drivers/staging/vt6655/card.c +2 -1

@@ -299,7 +300,8 @@ bool card_update_tsf(struct vnt_private *priv, unsigned char rx_rate,
 		qwTSFOffset = CARDqGetTSFOffset(rx_rate, qwBSSTimestamp,
 						local_tsf);
 		/* adjust TSF, HW's TSF add TSF Offset reg */
-		qwTSFOffset =  le64_to_cpu(qwTSFOffset);
+		le_qwTSFOffset = cpu_to_le64(qwTSFOffset);
+		qwTSFOffset = le64_to_cpu(le_qwTSFOffset);

This isn't right at all. You've just to convert it and then convert it back. (In other words do nothing but in a very complicated way).

This isn't a bug, it's just an issue of annotations. The code is re-using a single variable to hold both cpu and little endian data. So the way to write the annotations is to create two variables, one that's cpu endian and one that's little endian.

regards,
dan carpenter

A message that is not in the archive (sent off-list or not yet received): CACKMdf=Kppts=NzTVsNNuFcYge2HU+vG83b2C2QNyPEsiFHnkw@mail.gmail.com

Dan Carpenter

2024-01-10 07:25 UTC #4 lore raw

Hm... This stuff is more broken than I realized when I first looked at it. And the truth is that I don't know the hardware so I can't say exactly what the fix is. Probably best to just leave it alone for now...

Quoted 7 lines · Ariel Silver

Quoted 7 lines
Quoted 7 lines
Hello Dan and thank you for the quick response. Much appreciated!
1) I am probably wrong, but as I see it, the current code assumes that 'CARDqGetTSFOffset' returns a little endian value.
So it calls 'qwTSFOffset = le64_to_cpu(qwTSFOffset)'. However digging in the code of 'CARDqGetTSFOffset' I don't see a reason to assume that. Which in my opinion can cuase a bad endianness cast on big endianness systems.

You're actually correct.

But the thing is that le64_to_cpu() and cpu_to_le64() do the exactly the same thing. If the hardware is little endian they do nothing, but if the hardware is big endian they call bswap_64(). The only difference is how a human reader understands it and for git annotations.

What I suspect is that we should be passing little endian data to iowrite32(). In other words we should change the le64_to_cpu() to cpu_to_le64(). This wouldn't change how the code works, it would just change how it is annotated. However, on the other hand, I see plenty of examples where it's passing CPU endian data to iowrite32() so maybe we should get rid of the conversion instead. Who knows?

I suspect is that this driver has never worked on big endian CPUs...

I would say let's not change this until we're more sure what's going on. And probably if we make changes that affect runtime as opposed to just modifying endian annotations, then that needs to be tested.

regards,
dan carpenter