[PATCH v3 1/2] binman: cosmetic: code formatting fixes

[Brian Ruley]

On Mon, Sep 30, 2024 at 12:52:24PM -0600, Simon Glass wrote:
> 
> WARNING: This email originated from outside of GE HealthCare. Please validate the sender's email address before clicking on links or attachments as they may not be safe.
> 
> Hi Brian,
> 
> On Mon, 30 Sept 2024 at 10:10, Brian Ruley <brian.ruley at gehealthcare.com> wrote:
> >
> > Conform to the style guide used in the project by making the following
> > changes:
> > * Use single quotes for multiline strings (except docstrings)
> > * Fix line width to 79 cols
> > * Use f-string instead of formatting a regular string
> >
> > Signed-off-by: Brian Ruley <brian.ruley at gehealthcare.com>
> > ---
> >  tools/binman/etype/nxp_imx8mcst.py | 28 +++++++++++++++++++++-------
> >  1 file changed, 21 insertions(+), 7 deletions(-)
> 
> Reviewed-by: Simon Glass <sjg at chromium.org>
> 
> Thanks for doing this.
> 
> Some of my comments on the other patch could be applied here, if you prefer.
> 
> >
> > diff --git a/tools/binman/etype/nxp_imx8mcst.py b/tools/binman/etype/nxp_imx8mcst.py
> > index 8221517b0c..0c744a00d7 100644
> > --- a/tools/binman/etype/nxp_imx8mcst.py
> > +++ b/tools/binman/etype/nxp_imx8mcst.py
> > @@ -23,7 +23,7 @@ from u_boot_pylib import tools
> >  MAGIC_NXP_IMX_IVT = 0x412000d1
> >  MAGIC_FITIMAGE    = 0xedfe0dd0
> >
> > -csf_config_template = """
> > +csf_config_template = '''
> >  [Header]
> >    Version = 4.3
> >    Hash Algorithm = sha256
> > @@ -53,7 +53,7 @@ csf_config_template = """
> >  [Authenticate Data]
> >    Verification index = 2
> >    Blocks = 0x1234 0x78 0xabcd "data.bin"
> > -"""
> > +'''
> >
> >  class Entry_nxp_imx8mcst(Entry_mkimage):
> >      """NXP i.MX8M CST .cfg file generator and cst invoker
> > @@ -69,9 +69,21 @@ class Entry_nxp_imx8mcst(Entry_mkimage):
> >      def ReadNode(self):
> >          super().ReadNode()
> >          self.loader_address = fdt_util.GetInt(self._node, 'nxp,loader-address')
> > -        self.srk_table = os.getenv('SRK_TABLE', fdt_util.GetString(self._node, 'nxp,srk-table', 'SRK_1_2_3_4_table.bin'))
> > -        self.csf_crt = os.getenv('CSF_KEY', fdt_util.GetString(self._node, 'nxp,csf-crt', 'CSF1_1_sha256_4096_65537_v3_usr_crt.pem'))
> > -        self.img_crt = os.getenv('IMG_KEY', fdt_util.GetString(self._node, 'nxp,img-crt', 'IMG1_1_sha256_4096_65537_v3_usr_crt.pem'))
> > +        self.srk_table = os.getenv(
> > +            'SRK_TABLE', fdt_util.GetString(
> > +                            self._node, 'nxp,srk-table',
> > +                            'SRK_1_2_3_4_table.bin'
> > +                         ))
> > +        self.csf_crt = os.getenv(
> > +            'CSF_KEY', fdt_util.GetString(
> > +                           self._node, 'nxp,csf-crt',
> > +                           'CSF1_1_sha256_4096_65537_v3_usr_crt.pem'
> > +                       ))
> > +        self.img_crt = os.getenv(
> > +            'IMG_KEY', fdt_util.GetString(
> > +                           self._node, 'nxp,img-crt',
> > +                           'IMG1_1_sha256_4096_65537_v3_usr_crt.pem'
> > +                       ))
> >          self.unlock = fdt_util.GetBool(self._node, 'nxp,unlock')
> >          self.ReadEntries()
> >
> > @@ -118,7 +130,7 @@ class Entry_nxp_imx8mcst(Entry_mkimage):
> >          tools.write_file(output_dname, data)
> >
> >          # Generate CST configuration file used to sign payload
> > -        cfg_fname = tools.get_output_filename('nxp.csf-config-txt.%s' % uniq)
> > +        cfg_fname = tools.get_output_filename(f'nxp.csf-config-txt.{uniq}')
> >          config = configparser.ConfigParser()
> >          # Do not make key names lowercase
> >          config.optionxform = str
> > @@ -127,7 +139,9 @@ class Entry_nxp_imx8mcst(Entry_mkimage):
> >          config['Install SRK']['File'] = '"' + self.srk_table + '"'
> >          config['Install CSFK']['File'] = '"' + self.csf_crt + '"'
> >          config['Install Key']['File'] = '"' + self.img_crt + '"'
> > -        config['Authenticate Data']['Blocks'] = hex(signbase) + ' 0 ' + hex(len(data)) + ' "' + str(output_dname) + '"'
> > +        config['Authenticate Data']['Blocks'] = (hex(signbase) + ' 0 '
> > +                                                 + hex(len(data)) + ' "'
> > +                                                 + str(output_dname) + '"')
> >          if not self.unlock:
> >              config.remove_section('Unlock')
> >          with open(cfg_fname, 'w') as cfgf:
> > --
> > 2.39.5
> >
> 
> Regards,
> Simon

Hi Simon,

Sure thing, I'm glad to contribute. I'm quite new to upstreaming so this
is a good learning experience for me, and I thank you for your patience.

I didn't have time to respond yesterday, but I sent a new revision
based on your comments.

Only one I didn't quite get was

> All three options seem to read the 'nxp,srk-crt' property, so you can
> do that once the if() to reduce the amount of duplicated code.

Each is reading a different property "img-crt", "csf-crt", and
"srk-crt", with the latter read only if "fast-auth" is set.

As for the testing part, after I got `binman test` working, I
experimented with creating a test for the `nxp-imx8mcst` etype, using
what you had done as a starting point. FYI, it doesn't depend on the
`nxp-imx8mimage` because it's for invoking `mkimage` when building
SPL, for example. So, what I did is just create a fake FIT image
inside the nxp-imx8mcst node, and run the test. I can send the patch
if you wish.

The test, however, was unsuccessful because a PKI tree is needed to
sign the assets. I thought maybe you would have a comment on that?

Best,
Brian