[PATCH i-g-t v2 3/6] tools/intel-gfx-fw-info: Embed fw decoding

[Gustavo Sousa]

Quoting Lucas De Marchi (2024-08-20 20:29:25-03:00)
>Use a factory class method to decide what class to use depending on the
>magic and embed the calls to cstruct in each of them. Main motivation is
>that in future the decode method will need to seek the binary as it
>can't really know the version just checking the beginning of the file.
>
>For now, keep the open/close of the file in the main function, but
>eventually can be migrated as well.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>---
> tools/intel-gfx-fw-info | 73 ++++++++++++++++++++++++++++-------------
> 1 file changed, 51 insertions(+), 22 deletions(-)
>
>diff --git a/tools/intel-gfx-fw-info b/tools/intel-gfx-fw-info
>index 3105144b7..0f1da8bff 100755
>--- a/tools/intel-gfx-fw-info
>+++ b/tools/intel-gfx-fw-info
>@@ -105,8 +105,38 @@ def FIELD_GET(mask: int, value: int) -> int:
> 
> 
> class Fw(abc.ABC):
>-    def __init__(self, fw):
>-        self.fw = fw
>+    def __init__(self, cparser, f):
>+        self.f = f
>+        self.cparser = cparser
>+        self.fw = None
>+
>+    def checksum(self):
>+        self.f.seek(0, 0)
>+        return hashlib.sha256(self.f.read()).hexdigest()
>+
>+    def dump(self, **kw):
>+        cstruct.dumpstruct(self.fw, kw)

s/kw/**kw/

>+
>+    @classmethod
>+    def create(cls, f):
>+        cparser = cstruct.cstruct()
>+        cparser.load(CDEF)
>+
>+        magic = cparser.magic(f).data
>+        default = None
>+
>+        for s in cls.__subclasses__():
>+            if s.MAGIC is None:
>+                default = s
>+            elif s.MAGIC == magic:
>+                f.seek(0, 0)
>+                return s(cparser, f)
>+
>+        if default:
>+            f.seek(0, 0)

Is this seek() necessary? Looks like functions that use f already rewind
before using f.

>+            return default(cparser, f)

I believe a cleaner way of doing this is to have a single "constructor"
variable so that we have a single point of instantiation. Something
like:

    constructor = None

    for s in cls.__subclasses__():
        if s.MAGIC is None:
            constructor = s
        elif s.MAGIC == magic:
            constructor = s
            break

    if constructor:
        f.seek(0, 0)
        return constructor(cparser, f)

>+
>+        return None
> 
>     @abc.abstractmethod
>     def decode(self):
>@@ -114,8 +144,13 @@ class Fw(abc.ABC):
> 
> 
> class FwCss(Fw):
>+    MAGIC = None
>+
>     def decode(self):
>-        data = []
>+        self.f.seek(0, 0)
>+        self.fw = self.cparser.uc_css_header(self.f)
>+
>+        data = ["header-type: CSS"]
> 
>         CSS_SW_VERSION_UC_MAJOR = 0xFF << 16
>         CSS_SW_VERSION_UC_MINOR = 0xFF << 8
>@@ -137,8 +172,13 @@ class FwCss(Fw):
> 
> 
> class FwGsc(Fw):
>+    MAGIC = b"$CPD"
>+
>     def decode(self):
>-        data = []
>+        self.f.seek(0, 0)
>+        self.fw = self.cparser.uc_huc_gsc_header(self.f)
>+
>+        data = ["header-type: GSC"]
> 
>         HUC_GSC_MINOR_VER_HI_MASK = 0xFF << 16
>         HUC_GSC_MAJOR_VER_HI_MASK = 0xFF
>@@ -167,40 +207,29 @@ def parse_args(argv: typing.List[str]) -> argparse.Namespace:
>     return parser.parse_args(argv)
> 
> 
>-def calculate_checksum(f: typing.BinaryIO) -> str:
>-    return hashlib.sha256(f.read()).hexdigest()
>-
>-
> def main(argv: typing.List[str]) -> int:
>     args = parse_args(argv)
> 
>-    cparser = cstruct.cstruct()
>-    cparser.load(CDEF)
>-
>     checksum = None
> 
>     try:
>         with open(args.filename, mode="rb") as f:
>-            magic = cparser.magic(f)
>-            f.seek(0, 0)
>-            if magic.data == b"$CPD":
>-                fw = FwGsc(cparser.uc_huc_gsc_header(f))
>-            else:
>-                fw = FwCss(cparser.uc_css_header(f))
>+            fw = Fw.create(f)
>+            if not fw:
>+                logging.fatal("Unknown firmware type in '{args.filename}'")

Note that this doesn't cause the script to abort...

> 
>+            decoded_data = fw.decode()

...meaning that we crash here if fw is None.

--
Gustavo Sousa

>             if args.checksum:
>-                f.seek(0, 0)
>-                checksum = calculate_checksum(f)
>-
>+                checksum = fw.checksum()
>     except FileNotFoundError as e:
>         logging.fatal(e)
>         return 1
> 
>-    print(*fw.decode(), sep="\n")
>+    print(*decoded_data, sep="\n")
> 
>     if args.raw:
>         print("raw dump:", end="")
>-        cstruct.dumpstruct(fw.fw, color=sys.stdout.isatty())
>+        fw.dump(color=sys.stdout.isatty())
> 
>     if checksum:
>         print(f"checksum: {checksum}")
>-- 
>2.43.0
>