fbdev: suppress warning when assigning vga-save/restore base

PATCH In review David Herrmann 5 participants 9 messages Aug 4, 2013 – Feb 10, 2014 Framebuffer layer

Share & tools

David Herrmann

author

2013-08-04 17:25 UTC #0 lore raw

PATCH 1/1 fbdev: suppress warning when assigning vga-save/restore base

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(-)

drivers/video/arkfb.c +1 -1

index 94a51f1..a4e487c 100644
@@ -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);

drivers/video/s3fb.c +1 -1

index 47ca86c..fc2208b 100644
@@ -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);

drivers/video/vt8623fb.c +1 -1

index e9557fa..3039d82 100644
@@ -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;

Signature · 1 line

1.8.3.4

H. Peter Anvin

2013-08-04 17:33 UTC #1 lore raw

Quoted 10 lines · David Herrmann

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

David Miller

2013-08-05 01:51 UTC #2 lore raw

From: "H. Peter Anvin" <[email protected]>
Date: Sun, 04 Aug 2013 10:33:46 -0700

Quoted 4 lines

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.

Ondrej Zajicek

2013-08-05 20:23 UTC #3 lore raw

Quoted 8 lines · David Miller

From: "H. Peter Anvin" <[email protected]>
Date: Sun, 04 Aug 2013 10:33:46 -0700
Quoted 4 lines
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?

Quoted 8 lines

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 .

Signature · 5 lines

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

H. Peter Anvin

2013-08-05 21:02 UTC #4 lore raw

Quoted 8 lines · Ondrej Zajicek

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

H. Peter Anvin

2013-08-05 21:14 UTC #5 lore raw

Quoted 10 lines · H. Peter Anvin

Quoted 8 lines · Ondrej Zajicek
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).

Quoted 3 lines

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

David Miller

2013-08-05 22:29 UTC #6 lore raw

In reply to Ondrej Zajicek

From: Ondrej Zajicek <[email protected]>
Date: Mon, 5 Aug 2013 22:29:55 +0200

Quoted 34 lines · David Miller

Quoted 8 lines
From: "H. Peter Anvin" <[email protected]>
Date: Sun, 04 Aug 2013 10:33:46 -0700
Quoted 4 lines
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?
Quoted 8 lines
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).

Quoted 3 lines

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.

Tomi Valkeinen

2014-02-10 11:23 UTC #7 lore raw

In reply to David Herrmann

Hi David,

Quoted 24 lines · David Herrmann

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;
                         ^

Quoted 6 lines

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

David Herrmann

author

2014-02-10 11:32 UTC #8 lore raw

Hi Tomi

Quoted 27 lines · Tomi Valkeinen

Hi David,
Quoted 24 lines · David Herrmann
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;
                         ^

Quoted 8 lines

Quoted 6 lines
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