Date: Wed, 10 Apr 2019 20:05:02 -0700 From: Mark Millard <marklmi@yahoo.com> To: FreeBSD PowerPC ML <freebsd-ppc@freebsd.org>, Justin Hibbits <chmeeedalf@gmail.com> Subject: Re: head -r345758: usefdt=1 style boot fails on PowerMac7,2 G5 (1 core per socket): Error -2 adding node /cpus/PowerpC,970 [G4 failures too, investigatory patch] Message-ID: <7383CCAA-FEB4-4CE6-ABEB-74781A18CCF4@yahoo.com> In-Reply-To: <DF388686-6D89-4F1A-B12E-1F5F5B4DF3B2@yahoo.com> References: <BD01C826-89A2-46A0-9157-022481E0DC88@yahoo.com> <98A19824-3C07-4ED6-A848-5A634F95E1CF@yahoo.com> <D473FC85-E895-48C0-A587-F52A4FB403AF@yahoo.com> <9A2F72B7-A25C-478C-B1AD-0661278F0B46@yahoo.com> <88C954A4-6D5A-4AB9-AA26-0ACD6D298605@yahoo.com> <5A26E6F0-13FB-4177-B284-45DA6FBED78E@yahoo.com> <DF388686-6D89-4F1A-B12E-1F5F5B4DF3B2@yahoo.com>
next in thread | previous in thread | raw e-mail | index | archive | help
[I show an investigatory patch and indicate its consequences. This
ends up indicating what is not a "extra '\0's in paths problem".]
On 2019-Apr-10, at 02:30, Mark Millard <marklmi at yahoo.com> wrote:
> On 2019-Apr-9, at 19:44, Mark Millard <marklmi at yahoo.com> wrote:
>=20
>> . . .
>=20
> I discovered a specific PowerMac11,2 vs. PowerMac7,2/PowerMac3,6
> difference that is involved:
>=20
> The difference is where nulls are vs. are not . . .
>=20
> I found a linux comment (after the later path evidence was
> observed):
>=20
> /* Fixup an Apple bug where they have bogus \0 chars in =
the
> * middle of the path in some properties, and extract
> * the unit name (everything after the last '/').
> */
>=20
> This was in the context of package-to-path use.
>=20
> I have evidence of this (though not from OF_package_to_path use but
> ofw_getprop_alloc use) . . .
>=20
> The PowerMac11,2 has, for example, in the notation of my
> dumping tool for looking at diff's of the (sorted) dumps:
>=20
> /device-tree/cpus/PowerPC,G5/cpu-version: hex_bytes_line# 0: 00 44 01 =
01
> /device-tree/cpus/PowerPC,G5/cpu-version: txt_bytes_line# 0: =
\000D\^A\^A
> /device-tree/cpus/PowerPC,G5/cpu-version: hex_bytes_line# 0: 00 44 01 =
01
> /device-tree/cpus/PowerPC,G5/cpu-version: txt_bytes_line# 0: =
\000D\^A\^A
> /device-tree/cpus/PowerPC,G5/cpu-version: hex_bytes_line# 0: 00 44 01 =
01
> /device-tree/cpus/PowerPC,G5/cpu-version: txt_bytes_line# 0: =
\000D\^A\^A
> /device-tree/cpus/PowerPC,G5/cpu-version: hex_bytes_line# 0: 00 44 01 =
01
> /device-tree/cpus/PowerPC,G5/cpu-version: txt_bytes_line# 0: =
\000D\^A\^A
>=20
> But the PowerMac7,2 and PowerMac3,6 have a null character in the =
analogous
> prefix (produced by the same criteria), here shown with ^@:
>=20
> /device-tree/cpus/PowerPC,970^@/cpu-version: hex_bytes_line# 0: 00 39 =
02 02
> /device-tree/cpus/PowerPC,970^@/cpu-version: txt_bytes_line# 0: =
\0009\^B\^B
> /device-tree/cpus/PowerPC,970^@/cpu-version: hex_bytes_line# 0: 00 39 =
02 02
> /device-tree/cpus/PowerPC,970^@/cpu-version: txt_bytes_line# 0: =
\0009\^B\^B
>=20
> /device-tree/cpus/PowerPC,G4^@/cpu-version: hex_bytes_line# 0: 80 01 =
03 03
> /device-tree/cpus/PowerPC,G4^@/cpu-version: txt_bytes_line# 0: =
\M^@\^A\^C\^C
> /device-tree/cpus/PowerPC,G4^@/cpu-version: hex_bytes_line# 0: 80 01 =
03 03
> /device-tree/cpus/PowerPC,G4^@/cpu-version: txt_bytes_line# 0: =
\M^@\^A\^C\^C
>=20
> The code that produces a name for a ofw node, as shown after a / =
above,
> is (C++17 notation):
>=20
> auto name_for_ofw_node=3D [&ofw_fd](auto ofw_nd) -> auto
> {
> std::string nd_id_text{};
>=20
> void* name_buf=3D nullptr;
> int name_buf_len=3D 0;
> auto const name_len=3D =
ofw_getprop_alloc(ofw_fd,ofw_nd,"name",&name_buf,&name_buf_len,1);
> if (0<name_len)
> nd_id_text+=3D std::string{static_cast<char const =
*>(name_buf), static_cast<std::string::size_type>(name_len-1)};
> else
> {
> . . . (does not happen) . . .
> }
>=20
> return nd_id_text;
> };
>=20
> [With name_len instead of name_len-1 there would be one more '\0'
> character in each such name after a / (before the first ":"):
> the terminating null character would be included.]
>=20
> If such a before-the-end-of-path ^@ shows up in the likes of
> add_node_to_fdt via its use of OF_package_to_path:
>=20
> char name[255], *lastprop, *subname;
> . . .
> for (node =3D OF_child(node); node > 0; node =3D OF_peer(node)) =
{
> OF_package_to_path(node, name, sizeof(name));
> subname =3D strrchr(name, '/');
> subname++;
> child_offset =3D fdt_add_subnode(buffer, fdt_offset, =
subname);
> if (child_offset < 0) {
> printf("Error %d adding node %s (%s), =
skipping\n",
> child_offset, name, subname);
> continue;
> }
>=20
> then the strrchr will not work as intended and the error message
> will result (as it does for PowerMac7,2/PowerMac3,6).
>=20
> I'll note that the return value of OF_package_to_path is ignored
> above but the description I find is:
>=20
> QUOTE
> package-to-path
> IN: phandle, [address] buf, buflen OUT: length
>=20
> Returns the fully qualified pathname corresponding to the node =
identifier phandle, storing, at most, buflen bytes as a null-terminated =
string in the memory buffer starting at the address buf. If the length =
of the null- terminated pathname is greater than buflen, the trailing =
characters and the null terminator are not stored. Length is the length =
of the fully qualified pathname excluding any null terminator, or =E2=80=93=
1 if phandle is invalid.
> END QUOTE
>=20
> The linux code does use the return value in order to not be fooled by
> null characters in the middle of the path.
The following investigatory patch prevents some of the
"Error -2 adding node . . ." notices. It:
A) makes no change for the PowerMac11,2 G5 (2 sockets, 2 cores each)
B) eliminates the message for PowerMac7,2 G5 (2 sockets, 1 core each),
eliminated is: Error -2 adding node /cpus/PowerPC,970 (PowerPC,970), =
skipping
C) eliminates one message for PowerMac3,6 G4 (2 sockets, 1 core each), =
the
one is: Error -2 adding node /cpus/PowerPC,G4 (PowerPC,G4), skipping
For (A): Apparently extra '\0's is not the reason the PowerMac11,2
gets one such message. I do not know why it does. Both powerpc64
and 32-bit powerpc FreeBSD still report:
Error -2 adding node /ht@0,f2000000/pci@8/mac-io@7/i2c@18000/i2c-bus@0 =
(i2c-bus@0), skipping
For (B): Eliminating the message is still followed by the boot
hanging up after "Kernel entry at . . .". It does not get as far
as clearing the console screen. Something else is going on for
this. The powerpc64 and 32-bit powerpc FreeBSD results are the
same. But I've no clue how to isolate it. (Booting without
usefdt mode works fine.)
For (C): Both CPUs are now used in usefdt mode but ethernet is
still not present. The messages still generated are:
Error -2 adding node /pci@f2000000//mac-io@17/gpio@50/gpio5@6f =
(gpio5@6f), skipping
Error -2 adding node /pci@f2000000//mac-io@17/gpio@50/gpio6@70 =
(gpio6@70), skipping
Error -2 adding node /pci@f2000000//mac-io@17/gpio@50/gpio11@75 =
(gpio11@75), skipping
Error -2 adding node /pci@f2000000//mac-io@17/extint-gpio@15@67 =
(extint-gpio@15@67), skipping
While the patch is justified by Macintosh problems, it should
be valid for openfirmware that has no "extra '\0' character"
problems in paths. My only testing environment is the old
PowerMacs, however.
The patch has add_node_to_fdt eliminate the extra/internal '\0'
characters in paths/names it creates, instead of preserving them
carefully. Why? If preserved, lots of other code seems to need to
be modified to deal with them. Another type of alternative would
have been to replace the '\0' characters with some other, say '_'.
(I'm not aware of a reason that name and path lengths would need
to be preserved.)
The patch is:
(I do not claim to have coded for direct acceptance into FreeBSD's
code base: investigatory material.)
Index: /usr/src/stand/powerpc/ofw/ofwfdt.c
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
--- /usr/src/stand/powerpc/ofw/ofwfdt.c (revision 345758)
+++ /usr/src/stand/powerpc/ofw/ofwfdt.c (working copy)
@@ -45,7 +45,7 @@
add_node_to_fdt(void *buffer, phandle_t node, int fdt_offset)
{
int i, child_offset, error;
- char name[255], *lastprop, *subname;
+ char name[255+1], *lastprop, *subname; // +1 added for always =
having a trailing '\0' position.
void *propbuf;
ssize_t proplen;
=20
@@ -77,9 +77,54 @@
&& !OF_hasprop(node, "ibm,phandle"))
fdt_setprop(buffer, fdt_offset, "phandle", &node, =
sizeof(node));
=20
+ // WARNING: openfirmware's package-to-path(nd,nm,len) does not =
place a trailing '\0'
+ // character in nm when it returns a full_str_len with =
len<=3Dfull_str_len .
+ // For full_str_len<len, a trailing '\0' is placed at =
nm[full_str_len].
+
+ name[255]=3D '\0'; // Avoid OF_package_to_path leaving no '\0' =
at end of long path.
for (node =3D OF_child(node); node > 0; node =3D OF_peer(node)) =
{
- OF_package_to_path(node, name, sizeof(name));
- subname =3D strrchr(name, '/');
+ int full_str_len=3D OF_package_to_path(node, name, =
sizeof(name)-1); // Avoids having trailing '\0' missing.
+ if (-1=3D=3Dfull_str_len) { // Highly unlikely.
+ printf("add_node_to_fdt got -1 return from =
OF_packakge_to_path\n");
+ continue;
+ }
+
+ // WARNING: For some Macintoshes, name can sometimes =
contain '\0' characters
+ // in the middle (before what will be subname)!
+
+ // full_str_len omits the offical trailing '\0' =
position.
+ // full_str_len is *not* limited by the sizeof(name)-1 =
value above: it reports
+ // the space needed to get all the text (ignoring the =
official trailing '\0').
+ if (0=3D=3Dfull_str_len) { // Highly unlikely.
+ printf("Error: Node name has no bytes before =
trailing null byte\n");
+ continue;
+ }
+ if (255<full_str_len) {
+ printf("Error: Node path %s (truncated), needs =
more than 255+1 bytes\n", name);
+ continue;
+ }
+
+ // Eliminate internal '\0' path characters: make saved =
results more standard.
+ char *from_pos=3D name;
+ char *to_pos=3D name;
+ char *original_end=3D name+full_str_len;
+ while (from_pos<original_end) {
+ if ('\0'!=3D*from_pos) {
+ *to_pos++=3D *from_pos;
+ }
+ from_pos++;
+ }
+ *to_pos=3D '\0'; // Make sure '\0' terminated.
+ if (to_pos=3D=3Dname) { // Highly unlikely.
+ printf("Error: Adjusted node name has no bytes =
before trailing null byte\n");
+ continue;
+ }
+
+ subname=3D strrchr(name,'/');
+ if (NULL=3D=3Dsubname) { // Highly unlikely.
+ printf("Error: Node name %s has no '/'\n", =
name);
+ continue;
+ }
subname++;
child_offset =3D fdt_add_subnode(buffer, fdt_offset, =
subname);
if (child_offset < 0) {
=3D=3D=3D
Mark Millard
marklmi at yahoo.com
( dsl-only.net went
away in early 2018-Mar)
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?7383CCAA-FEB4-4CE6-ABEB-74781A18CCF4>