Re: [PATCH v3 1/4] ieee1275: Move #defines into common ieee1275.h header

[Daniel Kiper]

On Thu, Aug 05, 2021 at 06:22:24PM -0400, Stefan Berger wrote:
> On 7/30/21 11:45 AM, Stefan Berger wrote:
> > From: Stefan Berger <stefanb@linux.ibm.com>
> >
> > Move some #defines from ieee1275.c into the common ieee1275.h
> > header file. Adjust the case used in IHANDLE_INVALID to use
> > proper ihandle_t.
> >
> > Signed-off-by: Stefan Berger <stefanb@linux.ibm.com>
> > ---
> >   grub-core/kern/ieee1275/ieee1275.c | 29 ++++++++++++-----------------
> >   include/grub/ieee1275/ieee1275.h   |  3 +++
> >   2 files changed, 15 insertions(+), 17 deletions(-)
> >
> > diff --git a/grub-core/kern/ieee1275/ieee1275.c 
> > b/grub-core/kern/ieee1275/ieee1275.c
> > index 86f81a3c4..8fe92274d 100644
> > --- a/grub-core/kern/ieee1275/ieee1275.c
> > +++ b/grub-core/kern/ieee1275/ieee1275.c
> > @@ -21,11 +21,6 @@
> >   #include <grub/types.h>
> >   #include <grub/misc.h>
> > -#define IEEE1275_PHANDLE_INVALID  ((grub_ieee1275_cell_t) -1)
> > -#define IEEE1275_IHANDLE_INVALID  ((grub_ieee1275_cell_t) 0)
> > -#define IEEE1275_CELL_INVALID     ((grub_ieee1275_cell_t) -1)
> > -
> > -
>
> I think it's safer to keep these here  because of how the sizes for the
> grub_ieee1275_cell_t are defined:
>
> # grep -r grub_ieee1275_cell include/grub/ | grep typedef
>
> include/grub/powerpc/ieee1275/ieee1275.h:typedef grub_uint32_t
> grub_ieee1275_cell_t;
> include/grub/sparc64/ieee1275/ieee1275.h:typedef grub_uint64_t
> grub_ieee1275_cell_t;
>
> # grep -r grub_ieee1275_phandle include/grub/ | grep typedef
> include/grub/ieee1275/ieee1275.h:typedef grub_uint32_t
> grub_ieee1275_phandle_t;
>
> # grep -r grub_ieee1275_ihandle include/grub/ | grep typedef
> include/grub/ieee1275/ieee1275.h:typedef grub_uint32_t
> grub_ieee1275_ihandle_t;
>
> # grep -r GRUB_IEEE1275_PH include/grub/| grep def
> include/grub/ieee1275/ieee1275.h:#define GRUB_IEEE1275_PHANDLE_INVALID 
> ((grub_ieee1275_phandle_t) -1)
>
> Using the global GRUB_IEEE1275_PHANDLE_INVALID here would probably break
> things on sparc64 if we now use 32bit '-1' rather than 64bit '-1'.

Well, it seems to me at least some code around phandle/ihandle in the
grub-core/kern/ieee1275/ieee1275.c file is buggy. Please take a look,
e.g., at grub_ieee1275_finddevice(). I think args.phandle should be
defined as grub_ieee1275_phandle_t. Otherwise *phandlep = args.phandle
assignment will work by chance only. So, I think your
GRUB_IEEE1275_PHANDLE_INVALID change to grub_ieee1275_phandle_t is
correct. However, IMO the code related to phandle/ihandle in the
grub-core/kern/ieee1275/ieee1275.c file should be fixed first. May I ask
you to do that?

Daniel