Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

11 messages, 6 authors, 2018-06-12 · open the first message on its own page

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

From: Andreas Schwab <hidden>
Date: 2018-06-09 07:14:40

On Jun 09 2018, Michael Schmitz [off-list ref] wrote:
Hi Finn,

Am 08.06.2018 um 14:24 schrieb Finn Thain:
quoted
Don't load the via-pmu68k driver on early PowerBooks. The M50753 PMU
device found in those models was never supported by this driver.
Attempting to load the driver usually causes a boot hang.

Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Finn Thain <redacted>
---
 arch/m68k/mac/misc.c           | 6 ++----
 drivers/macintosh/via-pmu68k.c | 4 ----
 include/uapi/linux/pmu.h       | 1 -
 3 files changed, 2 insertions(+), 9 deletions(-)
...
quoted
diff --git a/include/uapi/linux/pmu.h b/include/uapi/linux/pmu.h
index 89cb1acea93a..30f64d46f5db 100644
--- a/include/uapi/linux/pmu.h
+++ b/include/uapi/linux/pmu.h
@@ -93,7 +93,6 @@ enum {
 	PMU_HEATHROW_BASED,	/* PowerBook G3 series */
 	PMU_PADDINGTON_BASED,	/* 1999 PowerBook G3 */
 	PMU_KEYLARGO_BASED,	/* Core99 motherboard (PMU99) */
-	PMU_68K_V1,		/* 68K PMU, version 1 */
 	PMU_68K_V2, 		/* 68K PMU, version 2 */
 };
Is this enum used by any user space code? If so, perhaps rather leave the
PMU_68K_V1 in there to avoid upsetting that?
It also changes the value of PMU_68K_V2, which is an ABI break.

Andreas.

-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 7578 EB47 D4E5 4D69 2510  2552 DF73 E780 A9DA AEC1
"And now for something completely different."

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

From: Michael Schmitz <schmitzmic@gmail.com>
Date: 2018-06-09 07:55:18

Hi Andreas,

Am 09.06.2018 um 19:14 schrieb Andreas Schwab:
On Jun 09 2018, Michael Schmitz [off-list ref] wrote:
quoted
Hi Finn,

Am 08.06.2018 um 14:24 schrieb Finn Thain:
quoted
Don't load the via-pmu68k driver on early PowerBooks. The M50753 PMU
device found in those models was never supported by this driver.
Attempting to load the driver usually causes a boot hang.

Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Finn Thain <redacted>
---
 arch/m68k/mac/misc.c           | 6 ++----
 drivers/macintosh/via-pmu68k.c | 4 ----
 include/uapi/linux/pmu.h       | 1 -
 3 files changed, 2 insertions(+), 9 deletions(-)
...
quoted
diff --git a/include/uapi/linux/pmu.h b/include/uapi/linux/pmu.h
index 89cb1acea93a..30f64d46f5db 100644
--- a/include/uapi/linux/pmu.h
+++ b/include/uapi/linux/pmu.h
@@ -93,7 +93,6 @@ enum {
 	PMU_HEATHROW_BASED,	/* PowerBook G3 series */
 	PMU_PADDINGTON_BASED,	/* 1999 PowerBook G3 */
 	PMU_KEYLARGO_BASED,	/* Core99 motherboard (PMU99) */
-	PMU_68K_V1,		/* 68K PMU, version 1 */
 	PMU_68K_V2, 		/* 68K PMU, version 2 */
 };
Is this enum used by any user space code? If so, perhaps rather leave the
PMU_68K_V1 in there to avoid upsetting that?
It also changes the value of PMU_68K_V2, which is an ABI break.
Yes, that's what I worry about - but do we know of any users of that 
particular interface?

Cheers,

	Michael
Andreas.

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

From: Finn Thain <hidden>
Date: 2018-06-09 12:20:46

quoted
quoted
Is this enum used by any user space code? If so, perhaps rather 
leave the PMU_68K_V1 in there to avoid upsetting that?
It also changes the value of PMU_68K_V2, which is an ABI break.
Yes, that's what I worry about - but do we know of any users of that 
particular interface?
There is no ABI issue AFAIK. The value of pmu_kind is visible to userland 
only on powerpc. /dev/pmu and /proc/pmu/* do not exist on m68k. This patch 
series will make these UAPIs available on m68k, and for that reason I've 
chosen the value PMU_UNKNOWN for pmu_kind.

New pmu_kind values can be defined as and when the need arises. But that 
would imply a useful classification scheme for pre-PCI powerbooks, and I 
don't know what that scheme will look like because at this stage there is 
neither userland nor kernel code to support backlight, buttons and battery 
for pre-PCI powerbooks.

In anycase, the "v1" and "v2" scheme is obviously inadequate when you 
consider the range of m68k powerbook models. Also, consider the 
out-of-tree adaptation of via-pmu by the Nubus-PMac project, which has 
this ABI break:
diff --git a/include/linux/pmu.h b/include/linux/pmu.h
index cafe98d9694..9882a185a52 100644
--- a/include/linux/pmu.h
+++ b/include/linux/pmu.h
@@ -90,6 +90,7 @@ enum {
         PMU_HEATHROW_BASED,     /* PowerBook G3 series */
         PMU_PADDINGTON_BASED,   /* 1999 PowerBook G3 */
         PMU_KEYLARGO_BASED,     /* Core99 motherboard (PMU99) */
+        PMU_NUBUS_BASED,        /* 1400, 2300, 5300 */
         PMU_68K_V1,             /* 68K PMU, version 1 */
         PMU_68K_V2,             /* 68K PMU, version 2 */
 };
(BTW, these powerbooks are not "nubus based", they are "pre-PCI", so I 
wouldn't want this to go upstream in this form. It could be that 
PMU_NUBUS_BASED should be PMU_UNKNOWN too.)

-- 

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: 2018-06-10 06:55:42

On Sat, 2018-06-09 at 22:21 +1000, Finn Thain wrote:
quoted hunk
In anycase, the "v1" and "v2" scheme is obviously inadequate when you 
consider the range of m68k powerbook models. Also, consider the 
out-of-tree adaptation of via-pmu by the Nubus-PMac project, which has 
this ABI break:
diff --git a/include/linux/pmu.h b/include/linux/pmu.h
index cafe98d9694..9882a185a52 100644
--- a/include/linux/pmu.h
+++ b/include/linux/pmu.h
@@ -90,6 +90,7 @@ enum {
         PMU_HEATHROW_BASED,     /* PowerBook G3 series */
         PMU_PADDINGTON_BASED,   /* 1999 PowerBook G3 */
         PMU_KEYLARGO_BASED,     /* Core99 motherboard (PMU99) */
+        PMU_NUBUS_BASED,        /* 1400, 2300, 5300 */
         PMU_68K_V1,             /* 68K PMU, version 1 */
         PMU_68K_V2,             /* 68K PMU, version 2 */
 };
(BTW, these powerbooks are not "nubus based", they are "pre-PCI", so I 
wouldn't want this to go upstream in this form. It could be that 
PMU_NUBUS_BASED should be PMU_UNKNOWN too.)
Pre-PCI is basically "NUBUS" based even in absence of an actual NuBus
slot :-) It has to do with the internal HW architecture. The only ones
that aren't are the even older designs (the 68000 based ones).

What's the situation with those NuBus things ? What do they use as a
bootloader ? The old Apple one or BootX ? We should merge that port of
it's maintained.

Cheers,
Ben.
 

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

From: Finn Thain <hidden>
Date: 2018-06-11 23:46:50

On Sun, 10 Jun 2018, Benjamin Herrenschmidt wrote:
Pre-PCI is basically "NUBUS" based even in absence of an actual NuBus 
slot :-) It has to do with the internal HW architecture. The only ones 
that aren't are the even older designs (the 68000 based ones).
There is already some disagreement in the comments in the nubus-pmac code 
about the suitability of "PMU_NUBUS_BASED" as opposed to e.g. 
"PMU_WHITNEY_BASED".

Point is, the PMU driver doesn't care about the expansion slots or 
architecture (Whitney-based PMU appears on m68k and powerpc). So NuBus vs. 
PCI is a red herring here. The pmu_kind relates to backlight, buttons and 
battery.

(Leaving aside the PMU driver, if a pre-OpenFirmware Mac has a "slot zero" 
ROM, one can argue that it is actually a NuBus machine, regardless of any 
actual expansion slots.)
What's the situation with those NuBus things ? What do they use as a 
bootloader ? The old Apple one or BootX ? We should merge that port of 
it's maintained.
I agree that this code should not languish out-of-tree. But it would need 
more work before it could reasonably be submitted to reviewers.

I do have some nubus-pmac hardware but I also have more mac/68k driver 
work to do before I can tackle another architecture.

I don't know what the bootloader situation is, but it looks messy...
http://nubus-pmac.sourceforge.net/#booters

Laurent, does Emile work on these machines?

-- 
Cheers,
Ben.
 

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

From: Laurent Vivier <hidden>
Date: 2018-06-12 06:53:12

On 12/06/2018 01:47, Finn Thain wrote:
On Sun, 10 Jun 2018, Benjamin Herrenschmidt wrote:
...
I don't know what the bootloader situation is, but it looks messy...
http://nubus-pmac.sourceforge.net/#booters

Laurent, does Emile work on these machines?
No, Emile doesn't work on pmac-nubus, I tried to implement the switch
from m68k to ppc, but it has never worked.

Laurent

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

From: Michael Schmitz <schmitzmic@gmail.com>
Date: 2018-06-12 20:12:54

Hi,

On Tue, Jun 12, 2018 at 6:53 PM, Laurent Vivier [off-list ref] wrote:
On 12/06/2018 01:47, Finn Thain wrote:
quoted
On Sun, 10 Jun 2018, Benjamin Herrenschmidt wrote:
...
quoted
I don't know what the bootloader situation is, but it looks messy...
http://nubus-pmac.sourceforge.net/#booters

Laurent, does Emile work on these machines?
No, Emile doesn't work on pmac-nubus, I tried to implement the switch
from m68k to ppc, but it has never worked.
On the PowerMac 6100 that I installed Linux on many years ago, I'm
pretty sure I used Apple's mkLinux boot loader.

Not sure how that would work with modern kernels and initrds though.
Haven't got that hardware anymore so no way try.

Cheers,

  Michael
Laurent

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: 2018-06-10 08:30:08

Hi Finn,

On Sat, Jun 9, 2018 at 2:20 PM Finn Thain [off-list ref] wrote:
quoted
quoted
quoted
Is this enum used by any user space code? If so, perhaps rather
leave the PMU_68K_V1 in there to avoid upsetting that?
It also changes the value of PMU_68K_V2, which is an ABI break.
Yes, that's what I worry about - but do we know of any users of that
particular interface?
There is no ABI issue AFAIK. The value of pmu_kind is visible to userland
only on powerpc. /dev/pmu and /proc/pmu/* do not exist on m68k. This patch
series will make these UAPIs available on m68k, and for that reason I've
chosen the value PMU_UNKNOWN for pmu_kind.
While /dev/pmu and /proc/pmu/* may not exist on m68k, definitions in
include/uapi/linux/pmu.h are part of the ABI, and cannot be changed or removed,
unless we are 100% sure there are no users.

If I would write a program interfacing with /dev/pmu and /proc/pmu/*, and
needing to check the PMU type, it would have a switch() statement with
all existing values defined in <linux/pmu.h>. So that would become broken
by your change.

Hence the enum is append-only.
New pmu_kind values can be defined as and when the need arises. But that
would imply a useful classification scheme for pre-PCI powerbooks, and I
don't know what that scheme will look like because at this stage there is
neither userland nor kernel code to support backlight, buttons and battery
for pre-PCI powerbooks.

In anycase, the "v1" and "v2" scheme is obviously inadequate when you
consider the range of m68k powerbook models. Also, consider the
New values can be added at the bottom.
quoted hunk
out-of-tree adaptation of via-pmu by the Nubus-PMac project, which has
this ABI break:
diff --git a/include/linux/pmu.h b/include/linux/pmu.h
index cafe98d9694..9882a185a52 100644
--- a/include/linux/pmu.h
+++ b/include/linux/pmu.h
@@ -90,6 +90,7 @@ enum {
         PMU_HEATHROW_BASED,     /* PowerBook G3 series */
         PMU_PADDINGTON_BASED,   /* 1999 PowerBook G3 */
         PMU_KEYLARGO_BASED,     /* Core99 motherboard (PMU99) */
+        PMU_NUBUS_BASED,        /* 1400, 2300, 5300 */
         PMU_68K_V1,             /* 68K PMU, version 1 */
         PMU_68K_V2,             /* 68K PMU, version 2 */
 };
That's bad.  But as long as the NuBus-PMac project is out-of-tree, the
enum values it uses are not part of the Linux ABI, IMHO.
During upstreaming, PMU_NUBUS_BASED should be moved to the bottom.

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 v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

From: Michael Schmitz <schmitzmic@gmail.com>
Date: 2018-06-10 09:13:09

Hi Geert,

Am 10.06.2018 um 20:29 schrieb Geert Uytterhoeven:
Hi Finn,

On Sat, Jun 9, 2018 at 2:20 PM Finn Thain [off-list ref] wrote:
quoted
quoted
quoted
quoted
Is this enum used by any user space code? If so, perhaps rather
leave the PMU_68K_V1 in there to avoid upsetting that?
It also changes the value of PMU_68K_V2, which is an ABI break.
Yes, that's what I worry about - but do we know of any users of that
particular interface?
There is no ABI issue AFAIK. The value of pmu_kind is visible to userland
only on powerpc. /dev/pmu and /proc/pmu/* do not exist on m68k. This patch
series will make these UAPIs available on m68k, and for that reason I've
chosen the value PMU_UNKNOWN for pmu_kind.
While /dev/pmu and /proc/pmu/* may not exist on m68k, definitions in
include/uapi/linux/pmu.h are part of the ABI, and cannot be changed or removed,
unless we are 100% sure there are no users.

If I would write a program interfacing with /dev/pmu and /proc/pmu/*, and
needing to check the PMU type, it would have a switch() statement with
all existing values defined in <linux/pmu.h>. So that would become broken
by your change.

Hence the enum is append-only.
The PMU type from pmu.h was never exposed to user space on m68k via 
/proc/pmu/*, and /dev/pmu is used for ioctls to the PMU driver on 
powerpc only (the 68k PMU driver doesn't have ioctl support). No way 
that I can see for user space to make use of the PMU type definition 
from pmu.h, so I suppose we can be sure there are no users.

The m68k PMU types cannot be said to be exposed on powerpc either (which 
has ioctl support to interrogate the PMU type), as these only return 
values up to PMU_KEYLARGO_BASED.

Applications like pbbuttonsd or pmud don't use the kernel PMU type at 
all, but go straight to the PMU via the ADB bus to interrogate the 
hardware type, so won't be affected either.

Is there any other way besides procfs and ioctl for user space to 
interrogate the PMU type that I'm missing here?

(I understand that breaking the ABI should not be done as a rule, but 
this may be a case where we can successfully argue the definitions were 
never in use, so the rules may be bent a little).

Cheers,

	Michael

quoted
New pmu_kind values can be defined as and when the need arises. But that
would imply a useful classification scheme for pre-PCI powerbooks, and I
don't know what that scheme will look like because at this stage there is
neither userland nor kernel code to support backlight, buttons and battery
for pre-PCI powerbooks.

In anycase, the "v1" and "v2" scheme is obviously inadequate when you
consider the range of m68k powerbook models. Also, consider the
New values can be added at the bottom.
quoted
out-of-tree adaptation of via-pmu by the Nubus-PMac project, which has
this ABI break:
diff --git a/include/linux/pmu.h b/include/linux/pmu.h
index cafe98d9694..9882a185a52 100644
--- a/include/linux/pmu.h
+++ b/include/linux/pmu.h
@@ -90,6 +90,7 @@ enum {
         PMU_HEATHROW_BASED,     /* PowerBook G3 series */
         PMU_PADDINGTON_BASED,   /* 1999 PowerBook G3 */
         PMU_KEYLARGO_BASED,     /* Core99 motherboard (PMU99) */
+        PMU_NUBUS_BASED,        /* 1400, 2300, 5300 */
         PMU_68K_V1,             /* 68K PMU, version 1 */
         PMU_68K_V2,             /* 68K PMU, version 2 */
 };
That's bad.  But as long as the NuBus-PMac project is out-of-tree, the
enum values it uses are not part of the Linux ABI, IMHO.
During upstreaming, PMU_NUBUS_BASED should be moved to the bottom.

Gr{oetje,eeting}s,

                        Geert

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

From: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Date: 2018-06-11 00:05:55

On Sun, 2018-06-10 at 21:12 +1200, Michael Schmitz wrote:
Hi Geert,
Top posting, sorry ...

We are painting that bike shed with way too many coats..

We can keep the existing definitions, stick a comment on them stating
"obsolete" and use new number if/when needed.

Ben.

Am 10.06.2018 um 20:29 schrieb Geert Uytterhoeven:
quoted
Hi Finn,

On Sat, Jun 9, 2018 at 2:20 PM Finn Thain [off-list ref] wrote:
quoted
quoted
quoted
quoted
Is this enum used by any user space code? If so, perhaps rather
leave the PMU_68K_V1 in there to avoid upsetting that?
It also changes the value of PMU_68K_V2, which is an ABI break.
Yes, that's what I worry about - but do we know of any users of that
particular interface?
There is no ABI issue AFAIK. The value of pmu_kind is visible to userland
only on powerpc. /dev/pmu and /proc/pmu/* do not exist on m68k. This patch
series will make these UAPIs available on m68k, and for that reason I've
chosen the value PMU_UNKNOWN for pmu_kind.
While /dev/pmu and /proc/pmu/* may not exist on m68k, definitions in
include/uapi/linux/pmu.h are part of the ABI, and cannot be changed or removed,
unless we are 100% sure there are no users.

If I would write a program interfacing with /dev/pmu and /proc/pmu/*, and
needing to check the PMU type, it would have a switch() statement with
all existing values defined in <linux/pmu.h>. So that would become broken
by your change.

Hence the enum is append-only.
The PMU type from pmu.h was never exposed to user space on m68k via 
/proc/pmu/*, and /dev/pmu is used for ioctls to the PMU driver on 
powerpc only (the 68k PMU driver doesn't have ioctl support). No way 
that I can see for user space to make use of the PMU type definition 
from pmu.h, so I suppose we can be sure there are no users.

The m68k PMU types cannot be said to be exposed on powerpc either (which 
has ioctl support to interrogate the PMU type), as these only return 
values up to PMU_KEYLARGO_BASED.

Applications like pbbuttonsd or pmud don't use the kernel PMU type at 
all, but go straight to the PMU via the ADB bus to interrogate the 
hardware type, so won't be affected either.

Is there any other way besides procfs and ioctl for user space to 
interrogate the PMU type that I'm missing here?

(I understand that breaking the ABI should not be done as a rule, but 
this may be a case where we can successfully argue the definitions were 
never in use, so the rules may be bent a little).

Re: [PATCH v2 08/12] macintosh/via-pmu68k: Don't load driver on unsupported hardware

From: Michael Schmitz <schmitzmic@gmail.com>
Date: 2018-06-11 01:45:48

Hi Ben,

I'm glad Finn is caring enough to keep this 20 year old bike shed in
good repair, but this may be overdoing it a little indeed. My bad.

A comment on the V1 PMU entry everyone should be OK with, I hope.

Cheers,

  Michael



On Mon, Jun 11, 2018 at 12:05 PM, Benjamin Herrenschmidt
[off-list ref] wrote:
On Sun, 2018-06-10 at 21:12 +1200, Michael Schmitz wrote:
quoted
Hi Geert,
Top posting, sorry ...

We are painting that bike shed with way too many coats..

We can keep the existing definitions, stick a comment on them stating
"obsolete" and use new number if/when needed.

Ben.

quoted
Am 10.06.2018 um 20:29 schrieb Geert Uytterhoeven:
quoted
Hi Finn,

On Sat, Jun 9, 2018 at 2:20 PM Finn Thain [off-list ref] wrote:
quoted
quoted
quoted
quoted
Is this enum used by any user space code? If so, perhaps rather
leave the PMU_68K_V1 in there to avoid upsetting that?
It also changes the value of PMU_68K_V2, which is an ABI break.
Yes, that's what I worry about - but do we know of any users of that
particular interface?
There is no ABI issue AFAIK. The value of pmu_kind is visible to userland
only on powerpc. /dev/pmu and /proc/pmu/* do not exist on m68k. This patch
series will make these UAPIs available on m68k, and for that reason I've
chosen the value PMU_UNKNOWN for pmu_kind.
While /dev/pmu and /proc/pmu/* may not exist on m68k, definitions in
include/uapi/linux/pmu.h are part of the ABI, and cannot be changed or removed,
unless we are 100% sure there are no users.

If I would write a program interfacing with /dev/pmu and /proc/pmu/*, and
needing to check the PMU type, it would have a switch() statement with
all existing values defined in <linux/pmu.h>. So that would become broken
by your change.

Hence the enum is append-only.
The PMU type from pmu.h was never exposed to user space on m68k via
/proc/pmu/*, and /dev/pmu is used for ioctls to the PMU driver on
powerpc only (the 68k PMU driver doesn't have ioctl support). No way
that I can see for user space to make use of the PMU type definition
from pmu.h, so I suppose we can be sure there are no users.

The m68k PMU types cannot be said to be exposed on powerpc either (which
has ioctl support to interrogate the PMU type), as these only return
values up to PMU_KEYLARGO_BASED.

Applications like pbbuttonsd or pmud don't use the kernel PMU type at
all, but go straight to the PMU via the ADB bus to interrogate the
hardware type, so won't be affected either.

Is there any other way besides procfs and ioctl for user space to
interrogate the PMU type that I'm missing here?

(I understand that breaking the ABI should not be done as a rule, but
this may be a case where we can successfully argue the definitions were
never in use, so the rules may be bent a little).
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help