> Date: Thu, 25 Nov 2021 15:14:27 -0700
> From: Ted Bullock <tbull...@comlore.com>
Hi Ted,
I made some small changes to the code and committed it. I chose to
use device_type in the end since that better reflects the intention of
disabling devices that use the Open Firmware driver for IDE devices.
Thanks for getting to the bottom of this!
> 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 <tbull...@comlore.com>
> >>> 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 <tbull...@comlore.com>
>
