On 2021-11-25 5:22 a.m., Ted Bullock wrote:
> On 2021-11-25 5:05 a.m., Mark Kettenis wrote:
>> From: Ted Bullock <[email protected]>
>>> On 2021-11-25 3:55 a.m., Otto Moerbeek wrote:
>>>>> + parent = OF_parent(handle);
>>>>
>>>> I think the OF_parent call can go inside the !strcmp(buf, "block")
>>>> block.
>>>
>>>
>>> I worried that the following re-assignment of the handle would cause
>>> problems, so I chose an order of operations and placed the parent call
>>> before this re-assignment. The reason for my concern is based on not
>>> knowing the proper distinction between OF_finddevice and OF_open in the
>>> boot prom itself (they are both kind of opaque to me and just send bits
>>> for interpretation into the boot prom).
>>>
>>> if ((handle = OF_open(fname)) == -1) {
>>> DNPRINTF(BOOT_D_OFDEV, "devopen: open of %s failed\n", fname);
>>> return ENXIO;
>>> }
>>>
>>> It's possible that there is no meaningful distinction between when
>>> handle is re-assigned midway through the function, in which case you are
>>> correct it could absolutely be moved later in the function call.
>>> Notably there is quite a bit of variable re-use and re-purposing in the
>>> devopen function so I chose the place I thought would be safest.
>>
>> I think you have a point. Might make sense to have a separate
>> variables here. Probably best to keep using handle for the OF_open()
>> result, and use "node" or "dhandle" for the OF_finddevice() result.
>>
>
> More to the point I just went to test since I was now awake and thinking
> about the problem some more and moving the call to OF_parent past the
> reassignment with OF_open does indeed break booting. I presume that the
> handle return value is contextually entirely different between
> OF_finddevice and OF_open.
I deliberately left the call to OF_parent earlier in the function. This
can be moved closer to the IDE comparison logic only if the variable
handle isn't re-assigned. Maybe do this in a future patch.
Also I left the query to name rather than device_type because it seems
more correct from my reading of the ieee1275 spec. The device tree in OF
uses the name as the identifier in the tree, not the device_type.
I do agree that checking if the parent exists is more correct and I have
done that.
This patch currently works for me.
Index: arch/sparc64/stand/ofwboot/ofdev.c
===================================================================
RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/ofdev.c,v
retrieving revision 1.31
diff -u -p -u -p -r1.31 ofdev.c
--- arch/sparc64/stand/ofwboot/ofdev.c 9 Dec 2020 18:10:19 -0000 1.31
+++ arch/sparc64/stand/ofwboot/ofdev.c 25 Nov 2021 22:04:01 -0000
@@ -520,7 +520,7 @@ devopen(struct open_file *of, const char
char fname[256];
char buf[DEV_BSIZE];
struct disklabel label;
- int handle, part;
+ int handle, part, parent;
int error = 0;
#ifdef SOFTRAID
char volno;
@@ -649,6 +649,9 @@ devopen(struct open_file *of, const char
#endif
if ((handle = OF_finddevice(fname)) == -1)
return ENOENT;
+
+ parent = OF_parent(handle);
+
DNPRINTF(BOOT_D_OFDEV, "devopen: found %s\n", fname);
if (OF_getprop(handle, "name", buf, sizeof buf) < 0)
return ENXIO;
@@ -685,6 +688,17 @@ devopen(struct open_file *of, const char
of->f_dev = devsw;
of->f_devdata = &ofdev;
+
+ /* Some PROMS have bugged writing code for ide block devices */
+ if (parent &&
+ OF_getprop(parent, "name", buf, sizeof buf) > 0 &&
+ !strcmp(buf, "ide"))
+ {
+ DNPRINTF(BOOT_D_OFDEV,
+ "devopen: Disable writing for IDE block device\n");
+ of->f_flags |= F_NOWRITE;
+ }
+
#ifdef SPARC_BOOT_UFS
bcopy(&file_system_ufs, &file_system[nfsys++], sizeof
file_system[0]);
bcopy(&file_system_ufs2, &file_system[nfsys++], sizeof
file_system[0]);
Index: arch/sparc64/stand/ofwboot/vers.c
===================================================================
RCS file: /cvs/src/sys/arch/sparc64/stand/ofwboot/vers.c,v
retrieving revision 1.22
diff -u -p -u -p -r1.22 vers.c
--- arch/sparc64/stand/ofwboot/vers.c 9 Dec 2020 18:10:19 -0000 1.22
+++ arch/sparc64/stand/ofwboot/vers.c 25 Nov 2021 22:04:01 -0000
@@ -1 +1 @@
-const char version[] = "1.21";
+const char version[] = "1.22";
Index: lib/libsa/fchmod.c
===================================================================
RCS file: /cvs/src/sys/lib/libsa/fchmod.c,v
retrieving revision 1.1
diff -u -p -u -p -r1.1 fchmod.c
--- lib/libsa/fchmod.c 3 Aug 2019 15:22:17 -0000 1.1
+++ lib/libsa/fchmod.c 25 Nov 2021 22:04:03 -0000
@@ -53,6 +53,11 @@ fchmod(int fd, mode_t m)
errno = EOPNOTSUPP;
return (-1);
}
+ /* writing is broken or unsupported */
+ if (f->f_flags & F_NOWRITE) {
+ errno = EOPNOTSUPP;
+ return (-1);
+ }
errno = (f->f_ops->fchmod)(f, m);
return (0);
Index: lib/libsa/stand.h
===================================================================
RCS file: /cvs/src/sys/lib/libsa/stand.h,v
retrieving revision 1.71
diff -u -p -u -p -r1.71 stand.h
--- lib/libsa/stand.h 24 Oct 2021 17:49:19 -0000 1.71
+++ lib/libsa/stand.h 25 Nov 2021 22:04:03 -0000
@@ -107,10 +107,11 @@ struct open_file {
extern struct open_file files[];
/* f_flags values */
-#define F_READ 0x0001 /* file opened for reading */
-#define F_WRITE 0x0002 /* file opened for writing */
-#define F_RAW 0x0004 /* raw device open - no file system */
-#define F_NODEV 0x0008 /* network open - no device */
+#define F_READ 0x0001 /* file opened for reading */
+#define F_WRITE 0x0002 /* file opened for writing */
+#define F_RAW 0x0004 /* raw device open - no file system */
+#define F_NODEV 0x0008 /* network open - no device */
+#define F_NOWRITE 0x0010 /* bootblock writing broken or unsupported */
#define isupper(c) ((c) >= 'A' && (c) <= 'Z')
#define islower(c) ((c) >= 'a' && (c) <= 'z')
--
Ted Bullock <[email protected]>
