linux-kernel - RE: [PATCH v3 3/3] staging: vt6655: Replace VNSvInPortD with ioread32

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <0b6873bcd7c54d71b1f0b84a61561261@AcuMS.aculab.com>
Date:   Wed, 27 Apr 2022 21:59:17 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Philipp Hortmann' <philipp.g.hortmann@...il.com>
CC:     Forest Bond <forest@...ttletooquiet.net>,
        "linux-staging@...ts.linux.dev" <linux-staging@...ts.linux.dev>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        'Greg Kroah-Hartman' <gregkh@...uxfoundation.org>
Subject: RE: [PATCH v3 3/3] staging: vt6655: Replace VNSvInPortD with ioread32

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)