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 <[email protected]>
Reported-by: H. Peter Anvin <[email protected]>
---
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 <[email protected]>
Date: Tue Jan 11 23:54:21 2011 +0000
s3fb: Compute VGA base iomem pointer explicitly.
commit 892c24ca40ffebf6d0ca4cc1454e58db131a4f5a
Author: David Miller <[email protected]>
Date: Tue Jan 11 23:54:35 2011 +0000
arkfb: Compute VGA base iomem pointer explicitly.
commit 6a2f6d5e970afbc1b8b08bafae9d9138a3206960
Author: David Miller <[email protected]>
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(-)
diff --git a/drivers/video/arkfb.c b/drivers/video/arkfb.c
index 94a51f1..a4e487c 100644
--- a/drivers/video/arkfb.c
+++ b/drivers/video/arkfb.c
@@ -1016,7 +1016,7 @@ static int ark_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 *)(unsigned long) vga_res.start;
/* FIXME get memsize */
regval = vga_rseq(par->state.vgabase, 0x10);
diff --git a/drivers/video/s3fb.c b/drivers/video/s3fb.c
index 47ca86c..fc2208b 100644
--- a/drivers/video/s3fb.c
+++ b/drivers/video/s3fb.c
@@ -1183,7 +1183,7 @@ static int s3_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 *)(unsigned long) vga_res.start;
/* Unlock regs */
cr38 = vga_rcrt(par->state.vgabase, 0x38);
diff --git a/drivers/video/vt8623fb.c b/drivers/video/vt8623fb.c
index e9557fa..3039d82 100644
--- a/drivers/video/vt8623fb.c
+++ b/drivers/video/vt8623fb.c
@@ -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 *)(unsigned long) vga_res.start;
/* Find how many physical memory there is on card */
memsize1 = (vga_rseq(par->state.vgabase, 0x34) + 1) >> 1;
--
1.8.3.4
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 <[email protected]>
> Reported-by: H. Peter Anvin <[email protected]>
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: "H. Peter Anvin" <[email protected]>
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.
On Sun, Aug 04, 2013 at 06:51:46PM -0700, David Miller wrote:
> From: "H. Peter Anvin" <[email protected]>
> 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.
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: [email protected])
OpenPGP encrypted e-mails preferred (KeyID 0x11DEADC3, wwwkeys.pgp.net)
"To err is human -- to blame it on a computer is even more so."
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
On 08/05/2013 02:02 PM, H. Peter Anvin wrote:
> 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.
>
So the confusion really is in the iowrite interface, which can take
either a physical port address or a virtual memory address...
-hpa
From: Ondrej Zajicek <[email protected]>
Date: Mon, 5 Aug 2013 22:29:55 +0200
> On Sun, Aug 04, 2013 at 06:51:46PM -0700, David Miller wrote:
>> From: "H. Peter Anvin" <[email protected]>
>> 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.
>
> 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.
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 <[email protected]>
> Reported-by: H. Peter Anvin <[email protected]>
> ---
> 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
Hi Tomi
On Mon, Feb 10, 2014 at 12:23 PM, Tomi Valkeinen <[email protected]> wrote:
> 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 <[email protected]>
>> Reported-by: H. Peter Anvin <[email protected]>
>> ---
>> 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