Re: [PATCH v3 3/3] staging: vt6655: Replace VNSvInPortD with ioread32

[Philipp Hortmann]

On 4/27/22 07:55, Greg Kroah-Hartman wrote:
> > MACvRegBitsOn(iobase, MAC_REG_TFTCTL, TFTCTL_TSFCNTRRD);
> >   	for (ww = 0; ww < W_MAX_TIMEOUT; ww++) {
> > @@ -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?

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.

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

> 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:
> 	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);

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

I will propose something for that.

> thanks,
>
> greg k-h

Thanks

Bye Philipp