From: Greg Kroah-Hartman
> Sent: 27 April 2022 06:55
>
> On Wed, Apr 27, 2022 at 07:42:23AM +0200, Philipp Hortmann wrote:
> > 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(-)
> >
> > diff --git a/drivers/staging/vt6655/card.c b/drivers/staging/vt6655/card.c
> > index 022310af5485..0dd13e475d6b 100644
> > --- a/drivers/staging/vt6655/card.c
> > +++ b/drivers/staging/vt6655/card.c
> > @@ -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++) {
> > @@ -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.
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.
David
>
> 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
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
On Wed, Apr 27, 2022 at 08:01:46AM +0000, David Laight wrote:
> From: Greg Kroah-Hartman
> > Sent: 27 April 2022 06:55
> >
> > On Wed, Apr 27, 2022 at 07:42:23AM +0200, Philipp Hortmann wrote:
> > > 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(-)
> > >
> > > diff --git a/drivers/staging/vt6655/card.c b/drivers/staging/vt6655/card.c
> > > index 022310af5485..0dd13e475d6b 100644
> > > --- a/drivers/staging/vt6655/card.c
> > > +++ b/drivers/staging/vt6655/card.c
> > > @@ -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++) {
> > > @@ -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.
>
> The new code is mostly better.
> But is different on BE systems - who knows what is actually needed.
Yeah, the endian issues here are totally ignored, my proposal would
ensure it stays the same. The change here breaks this on big endian
systems. So I'll drop this patch for now and just apply the first 2.
> 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.
Who knows, but that should be unwound eventually...
thanks,
greg k-h
On 4/27/22 10:01, David Laight wrote:
> 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:
kernel@matrix-ESPRIMO-P710:~/Documents/git/kernels/staging$ 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
Control: I/O+ Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr-
Stepping+ SERR- FastB2B- DisINTx-
Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=medium >TAbort-
<TAbort- <MAbort- >SERR- <PERR- INTx-
Latency: 32 (8000ns max), Cache Line Size: 64 bytes
Interrupt: pin A routed to IRQ 19
Region 0: Memory at f7c00000 (32-bit, non-prefetchable) [size=256]
Region 1: I/O ports at e000 [size=256]
Capabilities: [50] Power Management version 2
Flags: PMEClk- DSI- D1+ D2+ AuxCurrent=0mA PME(D0-,D1+,D2+,D3hot+,D3cold-)
Status: D0 NoSoftRst- PME-Enable- DSel=0 DScale=0 PME-
Kernel driver in use: vt6655
Kernel modules: vt6655_stage
---- 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?
This article gives more info:
https://www.kernel.org/doc/html/latest/driver-api/device-io.html
Thanks for your support.
Bye Philipp
From: Philipp Hortmann
> Sent: 27 April 2022 20:10
>
> On 4/27/22 10:01, David Laight wrote:
> > 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);
WTF is that mask?
The driver code I've got just uses pci_iomap(pci_dev, bar_number, length);
It then uses pci_iounmap(pci_dev, vaddr) to free it.
> 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.
BAR 0 is memory, BAR 1 I/O, both almost certainly refer to the
same physical device registers.
Basically PCI(e) I/O space is (mostly) deprecated.
It (sort of) exists so that PCI hardware could replace very old
(eg ISA) hardware without requiring driver changes.
> So is ioread32 wrong, right or can it be used?
(Assuming x86)
ioread32() has to contain a test to see whether the address
is an 'io address' or a 'memory address'.
For the former an 'inw' instruction is executed for the latter
a normal memory access instruction.
OTOH readl() is just a memory access (with compiler barriers)
and is inlined into the driver object.
So if you used priv->ioaddr you'd have to use ioread32().
Since you are using the memory space addresses from BAR 0
you can use ioread32() but it is more efficient to use readl().
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)