[PATCH v5] staging: gpib: Fix i386 build issue

[PATCH v5] staging: gpib: Fix i386 build issue

[Dave Penkler]

These drivers cast resource_type_t to void * causing the build to fail.

With CONFIG_X86_PAE enabled the resource_size_t type is a 64bit unsigned
int which cannot be cast to a 32 bit pointer.

Disable these drivers if X68_PAE is enabled

Reported-by: Guenter Roeck <linux@roeck-us.net>
Closes: https://lore.kernel.org/all/f10e976e-7a04-4454-b38d-39cd18f142da@roeck-us.net/
Fixes: e9dc69956d4d ("staging: gpib: Add Computer Boards GPIB driver")
Fixes: e1339245eba3 ("staging: gpib: Add Computer Equipment Corporation GPIB driver")
Fixes: bb1bd92fa0f2 ("staging: gpib: Add ines GPIB driver")
Fixes: 0cd5b05551e0 ("staging: gpib: Add TNT4882 chip based GPIB driver")
Signed-off-by: Dave Penkler <dpenkler@gmail.com>
---
 drivers/staging/gpib/Kconfig | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/staging/gpib/Kconfig b/drivers/staging/gpib/Kconfig
index 95308d15a555..6cf05586ca10 100644
--- a/drivers/staging/gpib/Kconfig
+++ b/drivers/staging/gpib/Kconfig
@@ -50,6 +50,7 @@ config GPIB_CEC_PCI
 	tristate "CEC PCI board"
 	depends on PCI
 	depends on HAS_IOPORT
+	depends on !X86_PAE
 	select GPIB_COMMON
 	select GPIB_NEC7210
 	help
@@ -62,6 +63,7 @@ config GPIB_CEC_PCI
 config GPIB_NI_PCI_ISA
 	tristate "NI PCI/ISA compatible boards"
 	depends on ISA_BUS || PCI || PCMCIA
+	depends on !X86_PAE
 	select GPIB_COMMON
 	select GPIB_NEC7210
 	help
@@ -85,6 +87,7 @@ config GPIB_CB7210
        tristate "Measurement Computing compatible boards"
 	depends on HAS_IOPORT
 	depends on ISA_BUS || PCI || PCMCIA
+	depends on !X86_PAE
        select GPIB_COMMON
 	select GPIB_NEC7210
        help
@@ -174,6 +177,7 @@ config GPIB_INES
        tristate "INES"
 	depends on PCI || ISA_BUS || PCMCIA
 	depends on HAS_IOPORT
+	depends on !X86_PAE
        select GPIB_COMMON
        select GPIB_NEC7210
        help
-- 
2.47.1

RE: [PATCH v5] staging: gpib: Fix i386 build issue

[David Laight]

From: Dave Penkler
> Sent: 04 December 2024 16:21
> 
> These drivers cast resource_type_t to void * causing the build to fail.
> 
> With CONFIG_X86_PAE enabled the resource_size_t type is a 64bit unsigned
> int which cannot be cast to a 32 bit pointer.
> 
> Disable these drivers if X68_PAE is enabled

You missed the obvious typo :-)

There is also a proposal to just remove PAE support.
Mostly because it is likely to have bit-rotted and isn't really
needed now 64bit code is common.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)

Re: [PATCH v5] staging: gpib: Fix i386 build issue

[Guenter Roeck]

Hi David,

On 12/6/24 10:02, David Laight wrote:
> From: Dave Penkler
>> Sent: 04 December 2024 16:21
>>
>> These drivers cast resource_type_t to void * causing the build to fail.
>>
>> With CONFIG_X86_PAE enabled the resource_size_t type is a 64bit unsigned
>> int which cannot be cast to a 32 bit pointer.
>>
>> Disable these drivers if X68_PAE is enabled
> 
> You missed the obvious typo :-)
> 

FWIW, I fail to see that typo as well, so I am not sure if it is really
that obvious.

> There is also a proposal to just remove PAE support.
> Mostly because it is likely to have bit-rotted and isn't really
> needed now 64bit code is common.
> 

Presumably that removal would not be 6.13 material, though, and I hope
that there _some_ fix will make it into 6.13.

Thanks,
Guenter

Re: [PATCH v5] staging: gpib: Fix i386 build issue

[Geert Uytterhoeven]

Hi Dave,

On Wed, Dec 4, 2024 at 5:21 PM Dave Penkler <dpenkler@gmail.com> wrote:
> These drivers cast resource_type_t to void * causing the build to fail.
>
> With CONFIG_X86_PAE enabled the resource_size_t type is a 64bit unsigned
> int which cannot be cast to a 32 bit pointer.
>
> Disable these drivers if X68_PAE is enabled
>
> Reported-by: Guenter Roeck <linux@roeck-us.net>
> Closes: https://lore.kernel.org/all/f10e976e-7a04-4454-b38d-39cd18f142da@roeck-us.net/
> Fixes: e9dc69956d4d ("staging: gpib: Add Computer Boards GPIB driver")
> Fixes: e1339245eba3 ("staging: gpib: Add Computer Equipment Corporation GPIB driver")
> Fixes: bb1bd92fa0f2 ("staging: gpib: Add ines GPIB driver")
> Fixes: 0cd5b05551e0 ("staging: gpib: Add TNT4882 chip based GPIB driver")
> Signed-off-by: Dave Penkler <dpenkler@gmail.com>

Thanks for your patch!

> --- a/drivers/staging/gpib/Kconfig
> +++ b/drivers/staging/gpib/Kconfig
> @@ -50,6 +50,7 @@ config GPIB_CEC_PCI
>         tristate "CEC PCI board"
>         depends on PCI
>         depends on HAS_IOPORT
> +       depends on !X86_PAE

!CONFIG_PHYS_ADDR_T_64BIT, to match the definition of phys_addr_t?

Or is this only showing up on x86?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5] staging: gpib: Fix i386 build issue

[Dave Penkler]

On Mon, Dec 09, 2024 at 05:01:19PM +0100, Geert Uytterhoeven wrote:
Hi Geert,
> Hi Dave,
> 
> On Wed, Dec 4, 2024 at 5:21???PM Dave Penkler <dpenkler@gmail.com> wrote:
> > These drivers cast resource_type_t to void * causing the build to fail.
> >
> > With CONFIG_X86_PAE enabled the resource_size_t type is a 64bit unsigned
> > int which cannot be cast to a 32 bit pointer.
> >
> > Disable these drivers if X68_PAE is enabled
> >
> > Reported-by: Guenter Roeck <linux@roeck-us.net>
> > Closes: https://lore.kernel.org/all/f10e976e-7a04-4454-b38d-39cd18f142da@roeck-us.net/
> > Fixes: e9dc69956d4d ("staging: gpib: Add Computer Boards GPIB driver")
> > Fixes: e1339245eba3 ("staging: gpib: Add Computer Equipment Corporation GPIB driver")
> > Fixes: bb1bd92fa0f2 ("staging: gpib: Add ines GPIB driver")
> > Fixes: 0cd5b05551e0 ("staging: gpib: Add TNT4882 chip based GPIB driver")
> > Signed-off-by: Dave Penkler <dpenkler@gmail.com>
> 
> Thanks for your patch!
> 
> > --- a/drivers/staging/gpib/Kconfig
> > +++ b/drivers/staging/gpib/Kconfig
> > @@ -50,6 +50,7 @@ config GPIB_CEC_PCI
> >         tristate "CEC PCI board"
> >         depends on PCI
> >         depends on HAS_IOPORT
> > +       depends on !X86_PAE
> 
> !CONFIG_PHYS_ADDR_T_64BIT, to match the definition of phys_addr_t?
> 
> Or is this only showing up on x86?
> 

I think so. See this message from Guenter
https://lore.kernel.org/all/7d7e65af-b818-45de-a92c-ee59a864dbdb@roeck-us.net/

cheers,
-Dave

Re: [PATCH v5] staging: gpib: Fix i386 build issue

[Guenter Roeck]

On 12/9/24 08:01, Geert Uytterhoeven wrote:
> Hi Dave,
> 
> On Wed, Dec 4, 2024 at 5:21 PM Dave Penkler <dpenkler@gmail.com> wrote:
>> These drivers cast resource_type_t to void * causing the build to fail.
>>
>> With CONFIG_X86_PAE enabled the resource_size_t type is a 64bit unsigned
>> int which cannot be cast to a 32 bit pointer.
>>
>> Disable these drivers if X68_PAE is enabled
>>
>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>> Closes: https://lore.kernel.org/all/f10e976e-7a04-4454-b38d-39cd18f142da@roeck-us.net/
>> Fixes: e9dc69956d4d ("staging: gpib: Add Computer Boards GPIB driver")
>> Fixes: e1339245eba3 ("staging: gpib: Add Computer Equipment Corporation GPIB driver")
>> Fixes: bb1bd92fa0f2 ("staging: gpib: Add ines GPIB driver")
>> Fixes: 0cd5b05551e0 ("staging: gpib: Add TNT4882 chip based GPIB driver")
>> Signed-off-by: Dave Penkler <dpenkler@gmail.com>
> 
> Thanks for your patch!
> 
>> --- a/drivers/staging/gpib/Kconfig
>> +++ b/drivers/staging/gpib/Kconfig
>> @@ -50,6 +50,7 @@ config GPIB_CEC_PCI
>>          tristate "CEC PCI board"
>>          depends on PCI
>>          depends on HAS_IOPORT
>> +       depends on !X86_PAE
> 
> !CONFIG_PHYS_ADDR_T_64BIT, to match the definition of phys_addr_t?
>

That would be wrong. It would disable the code for all 64-bit builds.

The underlying problem is that the code uses a pointer to store the physical
address. That doesn't work if sizeof(pointer) < sizeof(physical address),
which affects systems with X86_PAE enabled. I have not seen the problem
anywhere else.

Guenter

Re: [PATCH v5] staging: gpib: Fix i386 build issue

[Geert Uytterhoeven]

Hi Günter,

On Mon, Dec 9, 2024 at 5:18 PM Guenter Roeck <linux@roeck-us.net> wrote:
> On 12/9/24 08:01, Geert Uytterhoeven wrote:
> > On Wed, Dec 4, 2024 at 5:21 PM Dave Penkler <dpenkler@gmail.com> wrote:
> >> These drivers cast resource_type_t to void * causing the build to fail.
> >>
> >> With CONFIG_X86_PAE enabled the resource_size_t type is a 64bit unsigned
> >> int which cannot be cast to a 32 bit pointer.
> >>
> >> Disable these drivers if X68_PAE is enabled
> >>
> >> Reported-by: Guenter Roeck <linux@roeck-us.net>
> >> Closes: https://lore.kernel.org/all/f10e976e-7a04-4454-b38d-39cd18f142da@roeck-us.net/
> >> Fixes: e9dc69956d4d ("staging: gpib: Add Computer Boards GPIB driver")
> >> Fixes: e1339245eba3 ("staging: gpib: Add Computer Equipment Corporation GPIB driver")
> >> Fixes: bb1bd92fa0f2 ("staging: gpib: Add ines GPIB driver")
> >> Fixes: 0cd5b05551e0 ("staging: gpib: Add TNT4882 chip based GPIB driver")
> >> Signed-off-by: Dave Penkler <dpenkler@gmail.com>
> >
> > Thanks for your patch!
> >
> >> --- a/drivers/staging/gpib/Kconfig
> >> +++ b/drivers/staging/gpib/Kconfig
> >> @@ -50,6 +50,7 @@ config GPIB_CEC_PCI
> >>          tristate "CEC PCI board"
> >>          depends on PCI
> >>          depends on HAS_IOPORT
> >> +       depends on !X86_PAE
> >
> > !CONFIG_PHYS_ADDR_T_64BIT, to match the definition of phys_addr_t?
>
> That would be wrong. It would disable the code for all 64-bit builds.

Oops...

    depends on 64BIT || !PHYS_ADDR_T_64BIT

Assuming the driver actually works on 64-bit?
Perhaps people keep an old i386 to control their GPIB gear?

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5] staging: gpib: Fix i386 build issue

[Guenter Roeck]

On 12/9/24 08:27, Geert Uytterhoeven wrote:
> Hi Günter,
> 
> On Mon, Dec 9, 2024 at 5:18 PM Guenter Roeck <linux@roeck-us.net> wrote:
>> On 12/9/24 08:01, Geert Uytterhoeven wrote:
>>> On Wed, Dec 4, 2024 at 5:21 PM Dave Penkler <dpenkler@gmail.com> wrote:
>>>> These drivers cast resource_type_t to void * causing the build to fail.
>>>>
>>>> With CONFIG_X86_PAE enabled the resource_size_t type is a 64bit unsigned
>>>> int which cannot be cast to a 32 bit pointer.
>>>>
>>>> Disable these drivers if X68_PAE is enabled
>>>>
>>>> Reported-by: Guenter Roeck <linux@roeck-us.net>
>>>> Closes: https://lore.kernel.org/all/f10e976e-7a04-4454-b38d-39cd18f142da@roeck-us.net/
>>>> Fixes: e9dc69956d4d ("staging: gpib: Add Computer Boards GPIB driver")
>>>> Fixes: e1339245eba3 ("staging: gpib: Add Computer Equipment Corporation GPIB driver")
>>>> Fixes: bb1bd92fa0f2 ("staging: gpib: Add ines GPIB driver")
>>>> Fixes: 0cd5b05551e0 ("staging: gpib: Add TNT4882 chip based GPIB driver")
>>>> Signed-off-by: Dave Penkler <dpenkler@gmail.com>
>>>
>>> Thanks for your patch!
>>>
>>>> --- a/drivers/staging/gpib/Kconfig
>>>> +++ b/drivers/staging/gpib/Kconfig
>>>> @@ -50,6 +50,7 @@ config GPIB_CEC_PCI
>>>>           tristate "CEC PCI board"
>>>>           depends on PCI
>>>>           depends on HAS_IOPORT
>>>> +       depends on !X86_PAE
>>>
>>> !CONFIG_PHYS_ADDR_T_64BIT, to match the definition of phys_addr_t?
>>
>> That would be wrong. It would disable the code for all 64-bit builds.
> 
> Oops...
> 
>      depends on 64BIT || !PHYS_ADDR_T_64BIT
> 

Yes, that should work.

> Assuming the driver actually works on 64-bit?

No idea.

> Perhaps people keep an old i386 to control their GPIB gear?
> 

The code does not depend on x86, so presumably it could be any kind of CPU
as long as it supports PCI.

Guenter

Re: [PATCH v5] staging: gpib: Fix i386 build issue

[Geert Uytterhoeven]

Hi Günter,

On Mon, Dec 9, 2024 at 5:18 PM Guenter Roeck <linux@roeck-us.net> wrote:
> The underlying problem is that the code uses a pointer to store the physical
> address. That doesn't work if sizeof(pointer) < sizeof(physical address),
> which affects systems with X86_PAE enabled. I have not seen the problem
> anywhere else.

I could reproduce the build issue on ARM, with CONFIG_ARM_LPAE=y,
which is not enabled by allmodconfig.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5] staging: gpib: Fix i386 build issue

[Greg KH]

On Tue, Dec 10, 2024 at 08:52:08AM +0100, Geert Uytterhoeven wrote:
> Hi Günter,
> 
> On Mon, Dec 9, 2024 at 5:18 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > The underlying problem is that the code uses a pointer to store the physical
> > address. That doesn't work if sizeof(pointer) < sizeof(physical address),
> > which affects systems with X86_PAE enabled. I have not seen the problem
> > anywhere else.
> 
> I could reproduce the build issue on ARM, with CONFIG_ARM_LPAE=y,
> which is not enabled by allmodconfig.

So does that mean this patch is incorrect?

confused,

greg k-h

Re: [PATCH v5] staging: gpib: Fix i386 build issue

[Geert Uytterhoeven]

Hi Greg,

On Tue, Dec 10, 2024 at 9:39 AM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Tue, Dec 10, 2024 at 08:52:08AM +0100, Geert Uytterhoeven wrote:
> > On Mon, Dec 9, 2024 at 5:18 PM Guenter Roeck <linux@roeck-us.net> wrote:
> > > The underlying problem is that the code uses a pointer to store the physical
> > > address. That doesn't work if sizeof(pointer) < sizeof(physical address),
> > > which affects systems with X86_PAE enabled. I have not seen the problem
> > > anywhere else.
> >
> > I could reproduce the build issue on ARM, with CONFIG_ARM_LPAE=y,
> > which is not enabled by allmodconfig.
>
> So does that mean this patch is incorrect?

Purely from an arch-agnostic LPAE PoV, it should be:

    depends on 64BIT || !PHYS_ADDR_T_64BIT

However, that assumes the driver actually works on 64-bit or non-x86.
Perhaps people keep an old i386 to control their GPIB gear?

The drivers do not use ioremap(), but just cast the PCI resource
addresses to void * pointers. No idea if that works on x86_64.
It will probably crash spectacularly on non-x86...

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH v5] staging: gpib: Fix i386 build issue

[Guenter Roeck]

On 12/10/24 01:02, Geert Uytterhoeven wrote:
> Hi Greg,
> 
> On Tue, Dec 10, 2024 at 9:39 AM Greg KH <gregkh@linuxfoundation.org> wrote:
>> On Tue, Dec 10, 2024 at 08:52:08AM +0100, Geert Uytterhoeven wrote:
>>> On Mon, Dec 9, 2024 at 5:18 PM Guenter Roeck <linux@roeck-us.net> wrote:
>>>> The underlying problem is that the code uses a pointer to store the physical
>>>> address. That doesn't work if sizeof(pointer) < sizeof(physical address),
>>>> which affects systems with X86_PAE enabled. I have not seen the problem
>>>> anywhere else.
>>>
>>> I could reproduce the build issue on ARM, with CONFIG_ARM_LPAE=y,
>>> which is not enabled by allmodconfig.

I think it may actually also affect mips (CPU_MIPS32_R5_XPA) and arc
(ARC_HAS_PAE40).

>>
>> So does that mean this patch is incorrect?
> 
> Purely from an arch-agnostic LPAE PoV, it should be:
> 
>      depends on 64BIT || !PHYS_ADDR_T_64BIT
> 

Dave, would you mind submitting another version of your patch with the above
dependencies ?

> However, that assumes the driver actually works on 64-bit or non-x86.
> Perhaps people keep an old i386 to control their GPIB gear?
> 
> The drivers do not use ioremap(), but just cast the PCI resource
> addresses to void * pointers. No idea if that works on x86_64.

I did wonder about that as well, but unfortunately it isn't easy
to change, and would most definitely require testing on real hardware.

> It will probably crash spectacularly on non-x86...
> 

I'd guess so. However, that isn't something we can address here.
All we can make sure is that the code compiles.

Thanks,
Guenter