From: David Herrmann <hidden> Date: 2013-08-04 17:26:38
If drivers use "struct resource" objects to retrieve the vga-base, they
must correctly cast the integer to pointer. With x86+PAE we have 32bit
pointers but 64bit resource_size_t. Hence, cast it to "unsigned long"
before casting to "void*" to suppress warnings due to size differences.
As IO addresses are always low addresses, we can safely drop the higher
part of the address. This is what these drivers did before, anyway.
Signed-off-by: David Herrmann <redacted>
Reported-by: H. Peter Anvin <hpa@zytor.com>
---
Hi
hpa reported build-warnings on i386+PAE:
/home/hpa/kernel/distwork/drivers/video/arkfb.c:1019:23: warning: cast to
pointer from integer of different size [-Wint-to-pointer-cast]
par->state.vgabase = (void __iomem *) vga_res.start;
^
/home/hpa/kernel/distwork/drivers/video/s3fb.c: In function ‘s3_pci_probe’:
/home/hpa/kernel/distwork/drivers/video/s3fb.c:1186:23: warning: cast to pointer
from integer of different size [-Wint-to-pointer-cast]
par->state.vgabase = (void __iomem *) vga_res.start;
^
This is due to resource_size_t being 64bit but "void*" 32bit. This patch tries
to suppress these warnings but I am not really comfortable fixing this. I have
no idea whether my assumption (IO address are 32bit) is right. Please verify.
@David: The following 3 commits of yours presumably introduced the warnings. I
would be glad if you could review this:
commit 94c322c30bd14ae6cdd369cb4a1f94c5c3809ac9
Author: David Miller [off-list ref]
Date: Tue Jan 11 23:54:21 2011 +0000
s3fb: Compute VGA base iomem pointer explicitly.
commit 892c24ca40ffebf6d0ca4cc1454e58db131a4f5a
Author: David Miller [off-list ref]
Date: Tue Jan 11 23:54:35 2011 +0000
arkfb: Compute VGA base iomem pointer explicitly.
commit 6a2f6d5e970afbc1b8b08bafae9d9138a3206960
Author: David Miller [off-list ref]
Date: Tue Jan 11 23:55:17 2011 +0000
vt8623fb: Compute VGA base iomem pointer explicitly.
Cheers
David
drivers/video/arkfb.c | 2 +-
drivers/video/s3fb.c | 2 +-
drivers/video/vt8623fb.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
@@ -729,7 +729,7 @@ static int vt8623_pci_probe(struct pci_dev *dev, const struct pci_device_id *id)pcibios_bus_to_resource(dev,&vga_res,&bus_reg);-par->state.vgabase=(void__iomem*)vga_res.start;+par->state.vgabase=(void__iomem*)(unsignedlong)vga_res.start;/* Find how many physical memory there is on card */memsize1=(vga_rseq(par->state.vgabase,0x34)+1)>>1;
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2013-08-04 17:34:40
On 08/04/2013 10:25 AM, David Herrmann wrote:
If drivers use "struct resource" objects to retrieve the vga-base, they
must correctly cast the integer to pointer. With x86+PAE we have 32bit
pointers but 64bit resource_size_t. Hence, cast it to "unsigned long"
before casting to "void*" to suppress warnings due to size differences.
As IO addresses are always low addresses, we can safely drop the higher
part of the address. This is what these drivers did before, anyway.
Signed-off-by: David Herrmann <redacted>
Reported-by: H. Peter Anvin <hpa@zytor.com>
I'm still bothered here. Casting between resource_size_t and void *
implies a confusion between physical and virtual addresses. That may be
unavoidable for some reason, but I'd like to know what exactly is confused.
Anyone who can dig backwards and summarize? In other words:
Where in the current code do we stuff a physical address in a pointer,
or a virtual address into a non-pointer?
-hpa
From: David Miller <davem@davemloft.net> Date: 2013-08-05 01:51:49
From: "H. Peter Anvin" <hpa@zytor.com>
Date: Sun, 04 Aug 2013 10:33:46 -0700
Anyone who can dig backwards and summarize? In other words:
Where in the current code do we stuff a physical address in a pointer,
or a virtual address into a non-pointer?
The VGA register accessors try to accomodate iomem and ioport
accesses.
If they are given a non-NULL iomem pointer 'regbase' they use
iomem accesses, otherwise they do direct ISA port poking.
And yes the drivers in question are making some brash assumptions.
I suspect they should be using ioremap() or similar.
From: Ondrej Zajicek <hidden> Date: 2013-08-05 20:23:46
On Sun, Aug 04, 2013 at 06:51:46PM -0700, David Miller wrote:
From: "H. Peter Anvin" <hpa@zytor.com>
Date: Sun, 04 Aug 2013 10:33:46 -0700
quoted
Anyone who can dig backwards and summarize? In other words:
Where in the current code do we stuff a physical address in a pointer,
or a virtual address into a non-pointer?
The VGA register accessors try to accomodate iomem and ioport
accesses.
If they are given a non-NULL iomem pointer 'regbase' they use
iomem accesses, otherwise they do direct ISA port poking.
And yes the drivers in question are making some brash assumptions.
I suspect they should be using ioremap() or similar.
Well, these drivers were written without MMIO (port IO only with NULL
instead of 'vgabase' for VGA register accessors). They were converted in
patches 94c322c30bd14ae6cdd369cb4a1f94c5c3809ac9,
f8645933513c65ac55f23c63b2649097289795c6 and a few others (from David
Miller) to potentially use MMIO by using 'vgabase' instead of NULL:
pcibios_bus_to_resource(dev, &vga_res, &bus_reg);
par->state.vgabase = (void __iomem *) vga_res.start;
How this could even work? AFAIK these cards have to be explicitly programmed
to enable MMIO (which was not done in the patches). These patches claim that
it is for multi-domain PCI. I would guess that vgabase is NULL in common
configurations but if it is non-NULL, it probably wouldn't work, unless
there is some hardware magic that transparently converts MMIO (from CPU PoV)
to port IO (from card/PCI PoV).
Note that there are some later patches (86c0f043a737dadf034a4e6f29aefb074f4a1146)
from Ondrej Zary that independently enable MMIO, but they use par->mmio
field instead of par->state.vgabase .
--
Elen sila lumenn' omentielvo
Ondrej 'SanTiago' Zajicek (email: santiago@crfreenet.org)
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2013-08-05 21:02:52
On 08/05/2013 01:29 PM, Ondrej Zajicek wrote:
How this could even work? AFAIK these cards have to be explicitly programmed
to enable MMIO (which was not done in the patches). These patches claim that
it is for multi-domain PCI. I would guess that vgabase is NULL in common
configurations but if it is non-NULL, it probably wouldn't work, unless
there is some hardware magic that transparently converts MMIO (from CPU PoV)
to port IO (from card/PCI PoV).
They presumably use iowrite/ioread, which use either I/O instructions or
memory instructions depending on the address.
-hpa
From: "H. Peter Anvin" <hpa@zytor.com> Date: 2013-08-05 21:14:26
On 08/05/2013 02:02 PM, H. Peter Anvin wrote:
On 08/05/2013 01:29 PM, Ondrej Zajicek wrote:
quoted
How this could even work? AFAIK these cards have to be explicitly programmed
to enable MMIO (which was not done in the patches). These patches claim that
it is for multi-domain PCI. I would guess that vgabase is NULL in common
configurations but if it is non-NULL, it probably wouldn't work, unless
there is some hardware magic that transparently converts MMIO (from CPU PoV)
to port IO (from card/PCI PoV).
They presumably use iowrite/ioread, which use either I/O instructions or
memory instructions depending on the address.
So the confusion really is in the iowrite interface, which can take
either a physical port address or a virtual memory address...
-hpa
From: David Miller <davem@davemloft.net> Date: 2013-08-05 22:29:27
From: Ondrej Zajicek <redacted>
Date: Mon, 5 Aug 2013 22:29:55 +0200
On Sun, Aug 04, 2013 at 06:51:46PM -0700, David Miller wrote:
quoted
From: "H. Peter Anvin" <hpa@zytor.com>
Date: Sun, 04 Aug 2013 10:33:46 -0700
quoted
Anyone who can dig backwards and summarize? In other words:
Where in the current code do we stuff a physical address in a pointer,
or a virtual address into a non-pointer?
The VGA register accessors try to accomodate iomem and ioport
accesses.
If they are given a non-NULL iomem pointer 'regbase' they use
iomem accesses, otherwise they do direct ISA port poking.
And yes the drivers in question are making some brash assumptions.
I suspect they should be using ioremap() or similar.
Well, these drivers were written without MMIO (port IO only with NULL
instead of 'vgabase' for VGA register accessors). They were converted in
patches 94c322c30bd14ae6cdd369cb4a1f94c5c3809ac9,
f8645933513c65ac55f23c63b2649097289795c6 and a few others (from David
Miller) to potentially use MMIO by using 'vgabase' instead of NULL:
pcibios_bus_to_resource(dev, &vga_res, &bus_reg);
par->state.vgabase = (void __iomem *) vga_res.start;
How this could even work? AFAIK these cards have to be explicitly programmed
to enable MMIO (which was not done in the patches). These patches claim that
it is for multi-domain PCI. I would guess that vgabase is NULL in common
configurations but if it is non-NULL, it probably wouldn't work, unless
there is some hardware magic that transparently converts MMIO (from CPU PoV)
to port IO (from card/PCI PoV).
Note that there are some later patches (86c0f043a737dadf034a4e6f29aefb074f4a1146)
from Ondrej Zary that independently enable MMIO, but they use par->mmio
field instead of par->state.vgabase .
Ok, so the correct fix would be to make them pass NULL again and to simply
ignore the resources.
From: Tomi Valkeinen <hidden> Date: 2014-02-10 11:23:58
Hi David,
On 04/08/13 20:25, David Herrmann wrote:
If drivers use "struct resource" objects to retrieve the vga-base, they
must correctly cast the integer to pointer. With x86+PAE we have 32bit
pointers but 64bit resource_size_t. Hence, cast it to "unsigned long"
before casting to "void*" to suppress warnings due to size differences.
As IO addresses are always low addresses, we can safely drop the higher
part of the address. This is what these drivers did before, anyway.
Signed-off-by: David Herrmann <redacted>
Reported-by: H. Peter Anvin <hpa@zytor.com>
---
Hi
hpa reported build-warnings on i386+PAE:
/home/hpa/kernel/distwork/drivers/video/arkfb.c:1019:23: warning: cast to
pointer from integer of different size [-Wint-to-pointer-cast]
par->state.vgabase = (void __iomem *) vga_res.start;
^
/home/hpa/kernel/distwork/drivers/video/s3fb.c: In function ‘s3_pci_probe’:
/home/hpa/kernel/distwork/drivers/video/s3fb.c:1186:23: warning: cast to pointer
from integer of different size [-Wint-to-pointer-cast]
par->state.vgabase = (void __iomem *) vga_res.start;
^
This is due to resource_size_t being 64bit but "void*" 32bit. This patch tries
to suppress these warnings but I am not really comfortable fixing this. I have
no idea whether my assumption (IO address are 32bit) is right. Please verify.
@David: The following 3 commits of yours presumably introduced the warnings. I
would be glad if you could review this:
What was the conclusion on this patch? Should I apply for 3.14 fixes?
Tomi
From: David Herrmann <hidden> Date: 2014-02-10 11:32:31
Hi Tomi
On Mon, Feb 10, 2014 at 12:23 PM, Tomi Valkeinen [off-list ref] wrote:
Hi David,
On 04/08/13 20:25, David Herrmann wrote:
quoted
If drivers use "struct resource" objects to retrieve the vga-base, they
must correctly cast the integer to pointer. With x86+PAE we have 32bit
pointers but 64bit resource_size_t. Hence, cast it to "unsigned long"
before casting to "void*" to suppress warnings due to size differences.
As IO addresses are always low addresses, we can safely drop the higher
part of the address. This is what these drivers did before, anyway.
Signed-off-by: David Herrmann <redacted>
Reported-by: H. Peter Anvin <hpa@zytor.com>
---
Hi
hpa reported build-warnings on i386+PAE:
/home/hpa/kernel/distwork/drivers/video/arkfb.c:1019:23: warning: cast to
pointer from integer of different size [-Wint-to-pointer-cast]
par->state.vgabase = (void __iomem *) vga_res.start;
^
/home/hpa/kernel/distwork/drivers/video/s3fb.c: In function 's3_pci_probe':
/home/hpa/kernel/distwork/drivers/video/s3fb.c:1186:23: warning: cast to pointer
from integer of different size [-Wint-to-pointer-cast]
par->state.vgabase = (void __iomem *) vga_res.start;
^
This is due to resource_size_t being 64bit but "void*" 32bit. This patch tries
to suppress these warnings but I am not really comfortable fixing this. I have
no idea whether my assumption (IO address are 32bit) is right. Please verify.
@David: The following 3 commits of yours presumably introduced the warnings. I
would be glad if you could review this:
What was the conclusion on this patch? Should I apply for 3.14 fixes?
No, please drop it. If I understand David correctly, we should just
revert the patches in question. But someone who actually understands
what it is doing should propose that (which is not me..).
Thanks
David