LinuxLists.cc - [PATCH] parport_pc: don't allow driver for SPARC32

[permalink] [raw]

Subject: Re: [PATCH] parport_pc: don't allow driver for SPARC32

On Thu, Apr 06, 2023 at 09:05:48AM -0700, Randy Dunlap wrote:
> arch/sparc/kernel/ebus.o is only built for SPARC64.
> ns87303_lock is only built for SPARC64.
> arch/sparc/include/asm/parport.h says that it is for sparc64.
> Various documentation on the internet says that ebus is for UltraSPARC
> systems (64-bit).
>
> Therefore don't allow PARPORT_PC to be built for SPARC32.
>
> Fixes these build errors on SPARC32:
>
> ERROR: modpost: "ebus_dma_irq_enable" [drivers/parport/parport_pc.ko] undefined!
> ERROR: modpost: "ebus_dma_unregister" [drivers/parport/parport_pc.ko] undefined!
> ERROR: modpost: "ebus_dma_register" [drivers/parport/parport_pc.ko] undefined!
> ERROR: modpost: "ns87303_lock" [drivers/parport/parport_pc.ko] undefined!
> ERROR: modpost: "ebus_dma_enable" [drivers/parport/parport_pc.ko] undefined!
> ERROR: modpost: "ebus_dma_prepare" [drivers/parport/parport_pc.ko] undefined!
> ERROR: modpost: "ebus_dma_request" [drivers/parport/parport_pc.ko] undefined!
> ERROR: modpost: "ebus_dma_residue" [drivers/parport/parport_pc.ko] undefined!
>
> Fixes: 66bcd06099bb ("parport_pc: Also enable driver for PCI systems")
> Signed-off-by: Randy Dunlap <[email protected]>
> Cc: Sudip Mukherjee <[email protected]>
> Cc: Maciej W. Rozycki <[email protected]>
> Cc: "David S. Miller" <[email protected]>
> Cc: [email protected]
> Cc: Sam Ravnborg <[email protected]>
> Cc: [email protected]
Acked-by: Sam Ravnborg <[email protected]>
> ---
> drivers/parport/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff -- a/drivers/parport/Kconfig b/drivers/parport/Kconfig
> --- a/drivers/parport/Kconfig
> +++ b/drivers/parport/Kconfig
> @@ -42,7 +42,7 @@ if PARPORT
>
> config PARPORT_PC
> tristate "PC-style hardware"
> - depends on ARCH_MIGHT_HAVE_PC_PARPORT || (PCI && !S390)
> + depends on ARCH_MIGHT_HAVE_PC_PARPORT || (PCI && !S390 && !SPARC32)
> help
> You should say Y here if you have a PC-style parallel port. All
> IBM PC compatible computers and some Alphas have PC-style

2023-04-06 19:51:42

[permalink] [raw]

Subject: Re: [PATCH] parport_pc: don't allow driver for SPARC32

On Thu, 6 Apr 2023, Randy Dunlap wrote:

> arch/sparc/kernel/ebus.o is only built for SPARC64.
> ns87303_lock is only built for SPARC64.
> arch/sparc/include/asm/parport.h says that it is for sparc64.
> Various documentation on the internet says that ebus is for UltraSPARC
> systems (64-bit).
>
> Therefore don't allow PARPORT_PC to be built for SPARC32.

This looks completely wrong to me, any ordinary PCI parallel port card
ought just to work as long as you have PCI (S390 is special I'm told).
What needs to be done is AFAICT just making `parport_pc_find_nonpci_ports'
in arch/sparc/include/asm/parport.h SPARC64-specific, i.e.:

static int parport_pc_find_nonpci_ports(int autoirq, int autodma)
{
return (IS_ENABLED(CONFIG_SPARC64) &&
platform_driver_register(&ecpp_driver));
}

or suchlike and let the optimiser get rid of all the unwanted unsupported
stuff.

Maciej

2023-04-06 19:52:50

[permalink] [raw]

Subject: Re: [PATCH] parport_pc: don't allow driver for SPARC32

On 4/6/23 12:49, Maciej W. Rozycki wrote:
> On Thu, 6 Apr 2023, Randy Dunlap wrote:
>
>> arch/sparc/kernel/ebus.o is only built for SPARC64.
>> ns87303_lock is only built for SPARC64.
>> arch/sparc/include/asm/parport.h says that it is for sparc64.
>> Various documentation on the internet says that ebus is for UltraSPARC
>> systems (64-bit).
>>
>> Therefore don't allow PARPORT_PC to be built for SPARC32.
>
> This looks completely wrong to me, any ordinary PCI parallel port card
> ought just to work as long as you have PCI (S390 is special I'm told).
> What needs to be done is AFAICT just making `parport_pc_find_nonpci_ports'
> in arch/sparc/include/asm/parport.h SPARC64-specific, i.e.:
>
> static int parport_pc_find_nonpci_ports(int autoirq, int autodma)
> {
> return (IS_ENABLED(CONFIG_SPARC64) &&
> platform_driver_register(&ecpp_driver));
> }
>
> or suchlike and let the optimiser get rid of all the unwanted unsupported
> stuff.

Fine, please go ahead with that solution.

--
~Randy

2023-04-06 21:07:54

[permalink] [raw]

Subject: Re: [PATCH] parport_pc: don't allow driver for SPARC32

Hi Maciej,

On Thu, Apr 06, 2023 at 08:49:50PM +0100, Maciej W. Rozycki wrote:
> On Thu, 6 Apr 2023, Randy Dunlap wrote:
>
> > arch/sparc/kernel/ebus.o is only built for SPARC64.
> > ns87303_lock is only built for SPARC64.
> > arch/sparc/include/asm/parport.h says that it is for sparc64.
> > Various documentation on the internet says that ebus is for UltraSPARC
> > systems (64-bit).
> >
> > Therefore don't allow PARPORT_PC to be built for SPARC32.
>
> This looks completely wrong to me, any ordinary PCI parallel port card
> ought just to work as long as you have PCI (S390 is special I'm told).
> What needs to be done is AFAICT just making `parport_pc_find_nonpci_ports'
> in arch/sparc/include/asm/parport.h SPARC64-specific, i.e.:
>
> static int parport_pc_find_nonpci_ports(int autoirq, int autodma)
> {
> return (IS_ENABLED(CONFIG_SPARC64) &&
> platform_driver_register(&ecpp_driver));
> }
>
> or suchlike and let the optimiser get rid of all the unwanted unsupported
> stuff.

arch/sparc/include/asm/parport.h is sparc64 specific - and it will
result in the wrong result if it is pulled in for sparc32 builds.
This is what we see today.

Randy's suggestion is fine, as we avoid building parport support
for sparc32. If someone shows up and need parport support
for sparc32 then we could look into how to enable it.
Until then, we are better helped avoiding building the driver.

Hence, my ack on the patch from Randy.

Sam

2023-04-06 21:09:47

[permalink] [raw]

Subject: Re: [PATCH] parport_pc: don't allow driver for SPARC32

Hi Sam,

> > This looks completely wrong to me, any ordinary PCI parallel port card
> > ought just to work as long as you have PCI (S390 is special I'm told).
> > What needs to be done is AFAICT just making `parport_pc_find_nonpci_ports'
> > in arch/sparc/include/asm/parport.h SPARC64-specific, i.e.:
> >
> > static int parport_pc_find_nonpci_ports(int autoirq, int autodma)
> > {
> > return (IS_ENABLED(CONFIG_SPARC64) &&
> > platform_driver_register(&ecpp_driver));
> > }
> >
> > or suchlike and let the optimiser get rid of all the unwanted unsupported
> > stuff.
>
> arch/sparc/include/asm/parport.h is sparc64 specific - and it will
> result in the wrong result if it is pulled in for sparc32 builds.
> This is what we see today.
>
> Randy's suggestion is fine, as we avoid building parport support
> for sparc32. If someone shows up and need parport support
> for sparc32 then we could look into how to enable it.
> Until then, we are better helped avoiding building the driver.

I disagree. Why artificially prevent perfectly good hardware from
working with a perfectly good driver especially as the fix is just a
trivial exercise? And I offered a solution.

I don't have a SPARC toolchain handy or I could even try and build it
(but I'm sure there are many people around who can do it without bending
backwards).

NB conversely we have plenty of useless irrelevant stuff presented in
configration even if it genuinely makes no sense and won't ever be used
for the given platform (e.g. some Intel CPU management stuff shown for
RISC-V or even DEC Alpha systems).

Maciej

2023-04-06 21:34:12

[permalink] [raw]

Subject: Re: [PATCH] parport_pc: don't allow driver for SPARC32

On 4/6/23 14:01, Maciej W. Rozycki wrote:
> Hi Sam,
>
>>> This looks completely wrong to me, any ordinary PCI parallel port card
>>> ought just to work as long as you have PCI (S390 is special I'm told).
>>> What needs to be done is AFAICT just making `parport_pc_find_nonpci_ports'
>>> in arch/sparc/include/asm/parport.h SPARC64-specific, i.e.:
>>>
>>> static int parport_pc_find_nonpci_ports(int autoirq, int autodma)
>>> {
>>> return (IS_ENABLED(CONFIG_SPARC64) &&
>>> platform_driver_register(&ecpp_driver));
>>> }
>>>
>>> or suchlike and let the optimiser get rid of all the unwanted unsupported
>>> stuff.
>>
>> arch/sparc/include/asm/parport.h is sparc64 specific - and it will
>> result in the wrong result if it is pulled in for sparc32 builds.
>> This is what we see today.
>>
>> Randy's suggestion is fine, as we avoid building parport support
>> for sparc32. If someone shows up and need parport support
>> for sparc32 then we could look into how to enable it.
>> Until then, we are better helped avoiding building the driver.
>
> I disagree. Why artificially prevent perfectly good hardware from
> working with a perfectly good driver especially as the fix is just a
> trivial exercise? And I offered a solution.
>
> I don't have a SPARC toolchain handy or I could even try and build it
> (but I'm sure there are many people around who can do it without bending
> backwards).

https://mirrors.edge.kernel.org/pub/tools/crosstool/

--
~Randy

2023-04-07 20:10:31

[permalink] [raw]

Subject: Re: [PATCH] parport_pc: don't allow driver for SPARC32

Hi Maciej,

On Thu, Apr 06, 2023 at 10:01:16PM +0100, Maciej W. Rozycki wrote:
> Hi Sam,
>
> > > This looks completely wrong to me, any ordinary PCI parallel port card
> > > ought just to work as long as you have PCI (S390 is special I'm told).
> > > What needs to be done is AFAICT just making `parport_pc_find_nonpci_ports'
> > > in arch/sparc/include/asm/parport.h SPARC64-specific, i.e.:
> > >
> > > static int parport_pc_find_nonpci_ports(int autoirq, int autodma)
> > > {
> > > return (IS_ENABLED(CONFIG_SPARC64) &&
> > > platform_driver_register(&ecpp_driver));
> > > }
> > >
> > > or suchlike and let the optimiser get rid of all the unwanted unsupported
> > > stuff.
> >
> > arch/sparc/include/asm/parport.h is sparc64 specific - and it will
> > result in the wrong result if it is pulled in for sparc32 builds.
> > This is what we see today.
> >
> > Randy's suggestion is fine, as we avoid building parport support
> > for sparc32. If someone shows up and need parport support
> > for sparc32 then we could look into how to enable it.
> > Until then, we are better helped avoiding building the driver.
>
> I disagree. Why artificially prevent perfectly good hardware from
> working with a perfectly good driver especially as the fix is just a
> trivial exercise? And I offered a solution.

There is no sparc32 with a PC style parallel port, so the parport_pc
have no value for a sparc32 machine.

Some sparc Ultra have PC style parallel ports - but this is sparc64
machines and they are covered.

The sparc32 machines have the parport_sunbpp driver for their parallel
port.

An alternative fix, and better I think, would be to audit all archs
and let the relevant ones select ARCH_MIGHT_HAVE_PC_PARPORT, so we
avoided the ugly "|| (PCI && !S390 && !SPARC32)" case for PARPORT_PC.

Sam

2023-04-07 21:03:58

[permalink] [raw]

Subject: Re: [PATCH] parport_pc: don't allow driver for SPARC32

On Fri, 7 Apr 2023, Sam Ravnborg wrote:

> > > Randy's suggestion is fine, as we avoid building parport support
> > > for sparc32. If someone shows up and need parport support
> > > for sparc32 then we could look into how to enable it.
> > > Until then, we are better helped avoiding building the driver.
> >
> > I disagree. Why artificially prevent perfectly good hardware from
> > working with a perfectly good driver especially as the fix is just a
> > trivial exercise? And I offered a solution.
>
> There is no sparc32 with a PC style parallel port, so the parport_pc
> have no value for a sparc32 machine.

There are PC-style PCI (and PCIe) parallel ports in the form of option
cards being sold; I have one in my RISC-V machine (and I had to go through
the hassle of figuring out why the heck I am not able to select the driver
in configuration; a situation analogous to what Randy's change wants to
arrange). You can plug one into any machine that has PCI slots and my
understanding from Linux Kconfig files is there are such 32-bit SPARC
machines in existence or the dependency on PCI wouldn't offer the driver.
Otherwise just don't enable CONFIG_PCI for 32-bit SPARC.

Apologies if I wasn't clear enough with my reasoning, although I think
the lone presence of the PCI dependency in Kconfig ought have to make it
clear.

> The sparc32 machines have the parport_sunbpp driver for their parallel
> port.

That's an onboard device or an SBus option card though, right?

> An alternative fix, and better I think, would be to audit all archs
> and let the relevant ones select ARCH_MIGHT_HAVE_PC_PARPORT, so we
> avoided the ugly "|| (PCI && !S390 && !SPARC32)" case for PARPORT_PC.

It's only S390 that is special in that it has a limited set of specially
crafted PCI options it can ever support (or so I am told; something about
the firmware or suchlike).

Any other platform that has PCI slots will handle PC-style PCI parallel
port option cards just fine, as long as it supports PCI I/O read/write
commands (some systems such as POWER9 machines don't; Niklas Schnelle has
been recently working on a generic way to exclude drivers for devices that
require PCI port I/O from being offered with systems that have no support
for PCI port I/O).

Let me know if you find anything here unclear or have any other questions
or comments.

Maciej

2023-04-07 21:25:38

[permalink] [raw]

Subject: Re: [PATCH] parport_pc: don't allow driver for SPARC32

On 4/7/23 14:01, Maciej W. Rozycki wrote:
> On Fri, 7 Apr 2023, Sam Ravnborg wrote:
>
>>>> Randy's suggestion is fine, as we avoid building parport support
>>>> for sparc32. If someone shows up and need parport support
>>>> for sparc32 then we could look into how to enable it.
>>>> Until then, we are better helped avoiding building the driver.
>>>
>>> I disagree. Why artificially prevent perfectly good hardware from
>>> working with a perfectly good driver especially as the fix is just a
>>> trivial exercise? And I offered a solution.
>>
>> There is no sparc32 with a PC style parallel port, so the parport_pc
>> have no value for a sparc32 machine.
>
> There are PC-style PCI (and PCIe) parallel ports in the form of option
> cards being sold; I have one in my RISC-V machine (and I had to go through
> the hassle of figuring out why the heck I am not able to select the driver
> in configuration; a situation analogous to what Randy's change wants to
> arrange). You can plug one into any machine that has PCI slots and my
> understanding from Linux Kconfig files is there are such 32-bit SPARC
> machines in existence or the dependency on PCI wouldn't offer the driver.
> Otherwise just don't enable CONFIG_PCI for 32-bit SPARC.
>

If there are 32-bit Sparc machines with PCI slots, we must not have any
users with parallel ports or we should have heard about it IMO.

> Apologies if I wasn't clear enough with my reasoning, although I think
> the lone presence of the PCI dependency in Kconfig ought have to make it
> clear.
>
>> The sparc32 machines have the parport_sunbpp driver for their parallel
>> port.
>
> That's an onboard device or an SBus option card though, right?
>
>> An alternative fix, and better I think, would be to audit all archs
>> and let the relevant ones select ARCH_MIGHT_HAVE_PC_PARPORT, so we
>> avoided the ugly "|| (PCI && !S390 && !SPARC32)" case for PARPORT_PC.
>
> It's only S390 that is special in that it has a limited set of specially
> crafted PCI options it can ever support (or so I am told; something about
> the firmware or suchlike).

From my reading, if a Sparc32 machine has a PCI port, it might be able to
have a parallel port. However, even with Maciej's suggested code change
instead of my patch, the ebus code is not being compiled for Sparc32 -- only
for Sparc64, so more changes are needed beyond Maciej's suggestion.

But the documentation that I found refers to Ebus on Sparc4 machines.

> Any other platform that has PCI slots will handle PC-style PCI parallel
> port option cards just fine, as long as it supports PCI I/O read/write
> commands (some systems such as POWER9 machines don't; Niklas Schnelle has
> been recently working on a generic way to exclude drivers for devices that
> require PCI port I/O from being offered with systems that have no support
> for PCI port I/O).
>
> Let me know if you find anything here unclear or have any other questions
> or comments.

/me wishes that we had a Sparc maintainer.

--
~Randy

2023-06-19 00:16:16

[permalink] [raw]

Subject: Re: [PATCH] parport_pc: don't allow driver for SPARC32

On Fri, 7 Apr 2023, Randy Dunlap wrote:

> > There are PC-style PCI (and PCIe) parallel ports in the form of option
> > cards being sold; I have one in my RISC-V machine (and I had to go through
> > the hassle of figuring out why the heck I am not able to select the driver
> > in configuration; a situation analogous to what Randy's change wants to
> > arrange). You can plug one into any machine that has PCI slots and my
> > understanding from Linux Kconfig files is there are such 32-bit SPARC
> > machines in existence or the dependency on PCI wouldn't offer the driver.
> > Otherwise just don't enable CONFIG_PCI for 32-bit SPARC.
> >
>
> If there are 32-bit Sparc machines with PCI slots, we must not have any
> users with parallel ports or we should have heard about it IMO.

I wouldn't be surprised, parallel ports are not that common nowadays, let
alone used. Myself I haven't used a parallel printer for ages now, though
I still own a couple of odd other parallel devices, such as a ROM emulator
or the firmware download port of an old MIPS development board (actually a
regular Super I/O parallel port of said device hijacked by an onboard FPGA
for this second purpose if enabled with an onboard rocker switch). That's
not the usual stuff people tend to use I suppose.

> >> An alternative fix, and better I think, would be to audit all archs
> >> and let the relevant ones select ARCH_MIGHT_HAVE_PC_PARPORT, so we
> >> avoided the ugly "|| (PCI && !S390 && !SPARC32)" case for PARPORT_PC.

I should have noted this before: ARCH_MIGHT_HAVE_PC_PARPORT is for true
ISA or Super I/O parallel ports only. We handle true PCI implementations
in a generic platform-agnostic way, as long as the platform implements PCI
I/O commands in the host bridge. The latter requirement only excludes a
bunch of platforms, most notably S390 and recent 64-bit POWER systems.

With Niklas's HAS_IOPORT patches the S390 special case will soon be gone.

> > It's only S390 that is special in that it has a limited set of specially
> > crafted PCI options it can ever support (or so I am told; something about
> > the firmware or suchlike).
>
> >From my reading, if a Sparc32 machine has a PCI port, it might be able to
> have a parallel port. However, even with Maciej's suggested code change
> instead of my patch, the ebus code is not being compiled for Sparc32 -- only
> for Sparc64, so more changes are needed beyond Maciej's suggestion.
>
> But the documentation that I found refers to Ebus on Sparc4 machines.

Well, even though I came across a bunch of SPARC machines in the past I'm
not familiar enough with the platform to have an idea what SPARC4 refers
to. You can enable CONFIG_PCI for a SPARC32 kernel however, which I infer
from there are 32-bit SPARC machines in existence with PCI connectivity.
That I find enough for a potential PC-style parallel port configuration
with such a system, for as many ports as the availability of slots allows.

> > Any other platform that has PCI slots will handle PC-style PCI parallel
> > port option cards just fine, as long as it supports PCI I/O read/write
> > commands (some systems such as POWER9 machines don't; Niklas Schnelle has
> > been recently working on a generic way to exclude drivers for devices that
> > require PCI port I/O from being offered with systems that have no support
> > for PCI port I/O).
> >
> > Let me know if you find anything here unclear or have any other questions
> > or comments.
>
> /me wishes that we had a Sparc maintainer.

What happened to DaveM?

In any case after a couple of iterations I have made a succesful build of
a 32-bit SPARC toolchain now, which I was able to verify a fix with I have
outlined earlier in this thread. Posted as archived at:
<https://lore.kernel.org/r/[email protected]/>.

Maciej

2023-06-19 00:42:26

[permalink] [raw]

Subject: Re: [PATCH] parport_pc: don't allow driver for SPARC32

Hi,

On 6/18/23 16:42, Maciej W. Rozycki wrote:
> On Fri, 7 Apr 2023, Randy Dunlap wrote:
>
>> /me wishes that we had a Sparc maintainer.
>
> What happened to DaveM?

I haven't seen him merge any arch/sparc/ patches lately.
I have a couple that are still pending.

> In any case after a couple of iterations I have made a succesful build of
> a 32-bit SPARC toolchain now, which I was able to verify a fix with I have

Is your newly built toolchain for riscv hosting?

> outlined earlier in this thread. Posted as archived at:
> <https://lore.kernel.org/r/[email protected]/>.

thanks.
--
~Randy

2023-06-19 02:01:00

[permalink] [raw]

Subject: Re: [PATCH] parport_pc: don't allow driver for SPARC32

Hi Randy,

> > What happened to DaveM?
>
> I haven't seen him merge any arch/sparc/ patches lately.
> I have a couple that are still pending.

Oh, I hope he's been doing good then, and it's just a change of life
priorities or suchlike. Patch reviews can take a lot of mental effort,
and I can't claim I've been as effective as I wished to with stuff that
lands on my plate either.

> > In any case after a couple of iterations I have made a succesful build of
> > a 32-bit SPARC toolchain now, which I was able to verify a fix with I have
>
> Is your newly built toolchain for riscv hosting?

Are you asking whether the SPARC toolchain has been built/installed on a
RISC-V system? If so, then no, it hasn't. It runs on POWER9.

Maciej

2023-06-19 03:12:38

[permalink] [raw]

Subject: Re: [PATCH] parport_pc: don't allow driver for SPARC32

On 6/18/23 18:29, Maciej W. Rozycki wrote:
> Hi Randy,
>
>>> What happened to DaveM?
>>
>> I haven't seen him merge any arch/sparc/ patches lately.
>> I have a couple that are still pending.
>
> Oh, I hope he's been doing good then, and it's just a change of life
> priorities or suchlike. Patch reviews can take a lot of mental effort,
> and I can't claim I've been as effective as I wished to with stuff that
> lands on my plate either.
>
>>> In any case after a couple of iterations I have made a succesful build of
>>> a 32-bit SPARC toolchain now, which I was able to verify a fix with I have
>>
>> Is your newly built toolchain for riscv hosting?
>
> Are you asking whether the SPARC toolchain has been built/installed on a
> RISC-V system? If so, then no, it hasn't. It runs on POWER9.

Yes, that's what I was asking.
So you could have used the compilers that Arnd builds:
https://mirrors.edge.kernel.org/pub/tools/crosstool/

Thanks.
--
~Randy

2023-06-23 15:50:58

by David Miller

[permalink] [raw]

Subject: Re: [PATCH] parport_pc: don't allow driver for SPARC32

From: "Maciej W. Rozycki" <[email protected]>
Date: Mon, 19 Jun 2023 02:29:57 +0100 (BST)

> Hi Randy,
>
>> > What happened to DaveM?
>>
>> I haven't seen him merge any arch/sparc/ patches lately.
>> I have a couple that are still pending.
>
> Oh, I hope he's been doing good then, and it's just a change of life
> priorities or suchlike. Patch reviews can take a lot of mental effort,
> and I can't claim I've been as effective as I wished to with stuff that
> lands on my plate either.

I'm good just too busy with networking and real life.

Thanks.

2023-06-23 18:31:36

[permalink] [raw]

Subject: Re: [PATCH] parport_pc: don't allow driver for SPARC32

Hi Davem,

> >
> >> > What happened to DaveM?
> >>
> >> I haven't seen him merge any arch/sparc/ patches lately.
> >> I have a couple that are still pending.
> >
> > Oh, I hope he's been doing good then, and it's just a change of life
> > priorities or suchlike. Patch reviews can take a lot of mental effort,
> > and I can't claim I've been as effective as I wished to with stuff that
> > lands on my plate either.
>
> I'm good just too busy with networking and real life.

Enjoy the real life!

Sam

2023-06-23 19:36:41