staging: vt6655: Replace macro VNSvInPortD with ioread32()

Replace macro VNSvInPortD with ioread32 and as it was the only user, it can now be removed.
The name of macro and the arguments use CamelCase which is not accepted by checkpatch.pl

Reported-by kernel test robot <[email protected]>

Signed-off-by Philipp Hortmann <[email protected]>

---
V1 -> V2: This patch was 5/7 and is now 4/6
V2 -> V3: Inserted patch that was before in a different way in
"Rename macros VNSvInPortB,W,D with CamelCase ..." This patch was part of 4/6 and is now 3/7
V3 -> V4: Removed casting of the output variable
V4 -> V5: Joint patch "Replace two VNSvInPortD with ioread64_lo_hi"
with this patch. Changed ioread64 to two ioread32 as ioread64 does not work with 32 Bit computers. Shorted and simplified patch description.
V5 -> V6: Added missing version in subject

---
 drivers/staging/vt6655/card.c        |  6 ++++--
 drivers/staging/vt6655/device_main.c |  6 +++---
 drivers/staging/vt6655/mac.h         | 18 +++++++++---------
 drivers/staging/vt6655/rf.c          |  2 +-
 drivers/staging/vt6655/upc.h         |  3 ---
 5 files changed, 17 insertions(+), 18 deletions(-)

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

index 022310af5485..0dd13e475d6b 100644
@@ -744,6 +744,7 @@ bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF)
 	void __iomem *iobase = priv->port_offset;
 	unsigned short ww;
 	unsigned char data;
+	u32 low, high;
 
 	MACvRegBitsOn(iobase, MAC_REG_TFTCTL, TFTCTL_TSFCNTRRD);
 	for (ww = 0; ww < W_MAX_TIMEOUT; ww++) {

Sent: 27 April 2022 06:55

Quoted 39 lines · Philipp Hortmann

Replace macro VNSvInPortD with ioread32 and as it was the only user, it can now be removed.
The name of macro and the arguments use CamelCase which is not accepted by checkpatch.pl

Reported-by kernel test robot <[email protected]>

Signed-off-by Philipp Hortmann <[email protected]>

---
V1 -> V2: This patch was 5/7 and is now 4/6
V2 -> V3: Inserted patch that was before in a different way in
"Rename macros VNSvInPortB,W,D with CamelCase ..." This patch was part of 4/6 and is now 3/7
V3 -> V4: Removed casting of the output variable
V4 -> V5: Joint patch "Replace two VNSvInPortD with ioread64_lo_hi"
with this patch. Changed ioread64 to two ioread32 as ioread64 does not work with 32 Bit computers. Shorted and simplified patch description.
V5 -> V6: Added missing version in subject

---
 drivers/staging/vt6655/card.c        |  6 ++++--
 drivers/staging/vt6655/device_main.c |  6 +++---
 drivers/staging/vt6655/mac.h         | 18 +++++++++---------
 drivers/staging/vt6655/rf.c          |  2 +-
 drivers/staging/vt6655/upc.h         |  3 ---
 5 files changed, 17 insertions(+), 18 deletions(-)

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

index 022310af5485..0dd13e475d6b 100644
@@ -744,6 +744,7 @@ bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF)
 	void __iomem *iobase = priv->port_offset;
 	unsigned short ww;
 	unsigned char data;
+	u32 low, high;
 
 	MACvRegBitsOn(iobase, MAC_REG_TFTCTL, TFTCTL_TSFCNTRRD);
 	for (ww = 0; ww < W_MAX_TIMEOUT; ww++) {

drivers/staging/vt6655/card.c bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF) · line 759

Quoted 8 lines

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

@@ -753,8 +754,9 @@ bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF)
 	}
 	if (ww == W_MAX_TIMEOUT)
 		return false;
-	VNSvInPortD(iobase + MAC_REG_TSFCNTR, (u32 *)pqwCurrTSF);
-	VNSvInPortD(iobase + MAC_REG_TSFCNTR + 4, (u32 *)pqwCurrTSF + 1);
+	low = ioread32(iobase + MAC_REG_TSFCNTR);
+	high = ioread32(iobase + MAC_REG_TSFCNTR + 4);
+	*pqwCurrTSF = low + ((u64)high << 32);

Are you _sure_ this is doing the same thing?

Adding 1 to a u64 pointer increments it by a full u64. So I guess the cast to u32 * moves it only by a u32? Hopefully? That's messy.

Why not keep the current mess and do:

	pqwCurrTSF = ioread32(iobase + MAC_REG_TSFCNTR);
	((u32 *)pqwCurTSF + 1) = ioread32(iobase + MAC_REG_TSFCNTR + 4);

Or does that not compile? Ick, how about:

	u32 *temp = (u32 *)pqwCurTSF;

	temp = ioread32(iobase + MAC_REG_TSFCNTR);
	temp++;
	temp = ioread32(iobase + MAC_REG_TSFCNTR + 4);

As that duplicates the current code a bit better.

I don't know, it's like polishing dirt, in the end, it's still dirt...

How about looking at the caller of this to see what it expects to do with this information? Unwind the mess from there?

thanks,

greg k-h

From: Greg Kroah-Hartman

Quoted 53 lines

Sent: 27 April 2022 06:55

Quoted 39 lines · Philipp Hortmann

Replace macro VNSvInPortD with ioread32 and as it was the only user, it can now be removed.
The name of macro and the arguments use CamelCase which is not accepted by checkpatch.pl

Reported-by kernel test robot <[email protected]>

Signed-off-by Philipp Hortmann <[email protected]>

---
V1 -> V2: This patch was 5/7 and is now 4/6
V2 -> V3: Inserted patch that was before in a different way in
"Rename macros VNSvInPortB,W,D with CamelCase ..." This patch was part of 4/6 and is now 3/7
V3 -> V4: Removed casting of the output variable
V4 -> V5: Joint patch "Replace two VNSvInPortD with ioread64_lo_hi"
with this patch. Changed ioread64 to two ioread32 as ioread64 does not work with 32 Bit computers. Shorted and simplified patch description.
V5 -> V6: Added missing version in subject

---
 drivers/staging/vt6655/card.c        |  6 ++++--
 drivers/staging/vt6655/device_main.c |  6 +++---
 drivers/staging/vt6655/mac.h         | 18 +++++++++---------
 drivers/staging/vt6655/rf.c          |  2 +-
 drivers/staging/vt6655/upc.h         |  3 ---
 5 files changed, 17 insertions(+), 18 deletions(-)

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

index 022310af5485..0dd13e475d6b 100644
@@ -744,6 +744,7 @@ bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF)
 	void __iomem *iobase = priv->port_offset;
 	unsigned short ww;
 	unsigned char data;
+	u32 low, high;
 
 	MACvRegBitsOn(iobase, MAC_REG_TFTCTL, TFTCTL_TSFCNTRRD);
 	for (ww = 0; ww < W_MAX_TIMEOUT; ww++) {

drivers/staging/vt6655/card.c bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF) · line 759

Quoted 8 lines

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

@@ -753,8 +754,9 @@ bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF)
 	}
 	if (ww == W_MAX_TIMEOUT)
 		return false;
-	VNSvInPortD(iobase + MAC_REG_TSFCNTR, (u32 *)pqwCurrTSF);
-	VNSvInPortD(iobase + MAC_REG_TSFCNTR + 4, (u32 *)pqwCurrTSF + 1);
+	low = ioread32(iobase + MAC_REG_TSFCNTR);
+	high = ioread32(iobase + MAC_REG_TSFCNTR + 4);
+	*pqwCurrTSF = low + ((u64)high << 32);

Are you _sure_ this is doing the same thing?

Quoted 2 lines

Adding 1 to a u64 pointer increments it by a full u64. So I guess the cast to u32 * moves it only by a u32? Hopefully? That's messy.

The new code is mostly better.
But is different on BE systems - who knows what is actually needed.

Depends what is being copied.

Actually I suspect that 'iobase' should be an __iomem structure pointer, pqwCurrTSF a point of the same type and MAC_REG_xxxx structure members.

Then the code should be using readl() not ioread32(). I very much doubt that 'iobase' is in PCI IO space.

Actually I suspect that 'iobase' should be an __iomem structure pointer, pqwCurrTSF a point of the same type and MAC_REG_xxxx structure members.

Then the code should be using readl() not ioread32(). I very much doubt that 'iobase' is in PCI IO space.

Sent: 27 April 2022 20:10

Quoted 6 lines · David Laight

Actually I suspect that 'iobase' should be an __iomem structure pointer, pqwCurrTSF a point of the same type and MAC_REG_xxxx structure members.

Then the code should be using readl() not ioread32(). I very much doubt that 'iobase' is in PCI IO space.

Hi David,

here some infos and questions:

$ sudo lspci -s 01:05.0 -vvv
01:05.0 Network controller: VIA Technologies, Inc. VT6655 WiFi Adapter, 802.11a/b/g
	Subsystem: VIA Technologies, Inc. VT6655 WiFi Adapter, 802.11a/b/g

	Region 0: Memory at f7c00000 (32-bit, non-prefetchable) [size=256]
	Region 1: I/O ports at e000 [size=256]

---- In file device_main.c line 1699

	priv->memaddr = pci_resource_start(pcid, 0);
	priv->ioaddr = pci_resource_start(pcid, 1);
	priv->port_offset = ioremap(priv->memaddr & PCI_BASE_ADDRESS_MEM_MASK, 256);

	dev_info(&pcid->dev, "vt6655_probe priv->memaddr: %x priv->ioaddr: %x",

priv->memaddr, priv->ioaddr);

----- Output:
[ +0.000018] vt6655 0000:01:05.0: vt6655_probe priv->memaddr: f7c00000 priv->ioaddr: e000

So port_offset is derived from memaddr.

----- In file card.c line 742
bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF)

{
	void __iomem *iobase = priv->port_offset;
...
	VNSvInPortD(iobase + MAC_REG_TSFCNTR, (u32 *)pqwCurrTSF);

Please tell me if you need further infos to see if it is PCI IO space. I think it is memory-mapped.

So is ioread32 wrong, right or can it be used?

Quoted 4 lines

MACvRegBitsOn(iobase, MAC_REG_TFTCTL, TFTCTL_TSFCNTRRD);
for (ww = 0; ww < W_MAX_TIMEOUT; ww++) {

(quoted diff) +0 -0

@@ -753,8 +754,9 @@ bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF)
  	}

drivers/staging/vt6655/card.c bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF) · line 759

Quoted 7 lines

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

  	if (ww == W_MAX_TIMEOUT)
  		return false;
-	VNSvInPortD(iobase + MAC_REG_TSFCNTR, (u32 *)pqwCurrTSF);
-	VNSvInPortD(iobase + MAC_REG_TSFCNTR + 4, (u32 *)pqwCurrTSF + 1);
+	low = ioread32(iobase + MAC_REG_TSFCNTR);
+	high = ioread32(iobase + MAC_REG_TSFCNTR + 4);
+	*pqwCurrTSF = low + ((u64)high << 32);

Are you_sure_ this is doing the same thing?

Adding 1 to a u64 pointer increments it by a full u64. So I guess the cast to u32 * moves it only by a u32? Hopefully? That's messy.

Why not keep the current mess and do:

	pqwCurrTSF = ioread32(iobase + MAC_REG_TSFCNTR);
	((u32 *)pqwCurTSF + 1) = ioread32(iobase + MAC_REG_TSFCNTR + 4);

Or does that not compile?

	u32 *temp = (u32 *)pqwCurTSF;

	temp = ioread32(iobase + MAC_REG_TSFCNTR);
	temp++;
	temp = ioread32(iobase + MAC_REG_TSFCNTR + 4);

As that duplicates the current code a bit better.

I don't know, it's like polishing dirt, in the end, it's still dirt...

How about looking at the caller of this to see what it expects to do with this information? Unwind the mess from there?

thanks,

greg k-h

Quoted 4 lines · Greg Kroah-Hartman

Quoted 4 lines

MACvRegBitsOn(iobase, MAC_REG_TFTCTL, TFTCTL_TSFCNTRRD);
for (ww = 0; ww < W_MAX_TIMEOUT; ww++) {

(quoted diff) +0 -0

@@ -753,8 +754,9 @@ bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF)
  	}

Quoted 9 lines

drivers/staging/vt6655/card.c bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF) · line 759

Quoted 7 lines

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

  	if (ww == W_MAX_TIMEOUT)
  		return false;
-	VNSvInPortD(iobase + MAC_REG_TSFCNTR, (u32 *)pqwCurrTSF);
-	VNSvInPortD(iobase + MAC_REG_TSFCNTR + 4, (u32 *)pqwCurrTSF + 1);
+	low = ioread32(iobase + MAC_REG_TSFCNTR);
+	high = ioread32(iobase + MAC_REG_TSFCNTR + 4);
+	*pqwCurrTSF = low + ((u64)high << 32);

Are you_sure_ this is doing the same thing?

To compare I used the following code:
VNSvInPortD(iobase + MAC_REG_TSFCNTR, (u32 *)pqwCurrTSF); VNSvInPortD(iobase + MAC_REG_TSFCNTR + 4, (u32 *)pqwCurrTSF + 1); dev_info(&priv->pcid->dev, "CARDbGetCurrentTSF *pqwCurrTSF: %llx", *pqwCurrTSF);
low = ioread32(iobase + MAC_REG_TSFCNTR);
high = ioread32(iobase + MAC_REG_TSFCNTR + 4); dev_info(&priv->pcid->dev, "CARDbGetCurrentTSF low/high: %llx", low + ((u64)high << 32));

Output:
vt6655 0000:01:05.0: CARDbGetCurrentTSF *pqwCurrTSF: 1155ba
vt6655 0000:01:05.0: CARDbGetCurrentTSF low/high: 1155ba
vt6655 0000:01:05.0: CARDbGetCurrentTSF *pqwCurrTSF: 35d7cbd7c
vt6655 0000:01:05.0: CARDbGetCurrentTSF low/high: 35d7cbd7c
vt6655 0000:01:05.0: CARDbGetCurrentTSF *pqwCurrTSF: 35d7cbd8a
vt6655 0000:01:05.0: CARDbGetCurrentTSF low/high: 35d7cbd8a

So no different results for numbers larger than 32 Bit.

The pqwCurrTSF is a microsecond counter running in the WLAN Router:
At a later Measurement I got the following values:
269 seconds later: 0x3 6d89 fd91 -> 269.30 seconds
15 minutes later: 0x3 6d89 fd91 -> 15.54 minutes
8:38 hours later: 0xa 9787 ad91 -> 8.62 hours

So both methods work on a AMD64 processor.

Quoted 2 lines

Adding 1 to a u64 pointer increments it by a full u64. So I guess the cast to u32 * moves it only by a u32? Hopefully? That's messy.

That is the reason why I wanted to change this.

Quoted 4 lines

Why not keep the current mess and do:

	pqwCurrTSF = ioread32(iobase + MAC_REG_TSFCNTR);
	((u32 *)pqwCurTSF + 1) = ioread32(iobase + MAC_REG_TSFCNTR + 4);

Or does that not compile?

drivers/staging/vt6655/card.c:760:13: warning: assignment to ‘u64 *’ {aka ‘long long unsigned int *’} from ‘unsigned int’ makes pointer from integer without a cast [-Wint-conversion]
760 | pqwCurrTSF = ioread32(iobase + MAC_REG_TSFCNTR);
| ^
drivers/staging/vt6655/card.c:761:26: error: lvalue required as left operand of assignment
761 | ((u32 *)pqwCurrTSF + 1) = ioread32(iobase + MAC_REG_TSFCNTR + 4);
| ^

This compiles:

	*(u32 *)pqwCurrTSF = ioread32(iobase + MAC_REG_TSFCNTR);
	*((u32 *)pqwCurrTSF + 1) = ioread32(iobase + MAC_REG_TSFCNTR + 4);

Log:
vt6655 0000:01:05.0: CARDbGetCurrentTSF *pqwCurrTSF: 178f41d90 vt6655 0000:01:05.0: CARDbGetCurrentTSF with ioread: 178f41d90

Ick, how about:

Quoted 5 lines

	u32 *temp = (u32 *)pqwCurTSF;

	temp = ioread32(iobase + MAC_REG_TSFCNTR);
	temp++;
	temp = ioread32(iobase + MAC_REG_TSFCNTR + 4);

This is working:

	u32 *temp = (u32 *)pqwCurrTSF;

	*temp = ioread32(iobase + MAC_REG_TSFCNTR);
	temp++;
	*temp = ioread32(iobase + MAC_REG_TSFCNTR + 4);

And for a big endian system? Do you get the same result?

Quoted 1 line · Greg Kroah-Hartman

And for a big endian system? Do you get the same result?

Just looking if I can buy one.

Can you propose a low cost big endian system with PCI bus?

Quoted 32 lines · Philipp Hortmann

Quoted 4 lines · Greg Kroah-Hartman

Quoted 4 lines

MACvRegBitsOn(iobase, MAC_REG_TFTCTL, TFTCTL_TSFCNTRRD);
   	for (ww = 0; ww < W_MAX_TIMEOUT; ww++) {

(quoted diff) +0 -0

@@ -753,8 +754,9 @@ bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF)
   	}

Quoted 9 lines

drivers/staging/vt6655/card.c bool CARDbGetCurrentTSF(struct vnt_private *priv, u64 *pqwCurrTSF) · line 759

Quoted 7 lines

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

   	if (ww == W_MAX_TIMEOUT)
   		return false;
-	VNSvInPortD(iobase + MAC_REG_TSFCNTR, (u32 *)pqwCurrTSF);
-	VNSvInPortD(iobase + MAC_REG_TSFCNTR + 4, (u32 *)pqwCurrTSF + 1);
+	low = ioread32(iobase + MAC_REG_TSFCNTR);
+	high = ioread32(iobase + MAC_REG_TSFCNTR + 4);
+	*pqwCurrTSF = low + ((u64)high << 32);

Are you_sure_ this is doing the same thing?

To compare I used the following code:
VNSvInPortD(iobase + MAC_REG_TSFCNTR, (u32 *)pqwCurrTSF); VNSvInPortD(iobase + MAC_REG_TSFCNTR + 4, (u32 *)pqwCurrTSF + 1); dev_info(&priv->pcid->dev, "CARDbGetCurrentTSF *pqwCurrTSF: %llx", *pqwCurrTSF);
low = ioread32(iobase + MAC_REG_TSFCNTR);
high = ioread32(iobase + MAC_REG_TSFCNTR + 4); dev_info(&priv->pcid->dev, "CARDbGetCurrentTSF low/high: %llx", low + ((u64)high << 32));

Output:
vt6655 0000:01:05.0: CARDbGetCurrentTSF *pqwCurrTSF: 1155ba
vt6655 0000:01:05.0: CARDbGetCurrentTSF low/high: 1155ba
vt6655 0000:01:05.0: CARDbGetCurrentTSF *pqwCurrTSF: 35d7cbd7c
vt6655 0000:01:05.0: CARDbGetCurrentTSF low/high: 35d7cbd7c
vt6655 0000:01:05.0: CARDbGetCurrentTSF *pqwCurrTSF: 35d7cbd8a
vt6655 0000:01:05.0: CARDbGetCurrentTSF low/high: 35d7cbd8a

Quoted 1 line

So no different results for numbers larger than 32 Bit.

And for a big endian system? Do you get the same result?