Hi Andy and Ilias, On Wed, 5 Jun 2024 at 09:31, Ilias Apalodimas <ilias.apalodi...@linaro.org> wrote:
> Hi Andy, > > [...] > > > > > > > Is this approach maintainable? > > > > > > I don't remember if we have similar in Linux kernel, for example. > > > > > > (There are few candidates like compression algorithms that are > usually > > > > > > being > > > > > > hosted elsewhere) > > > > > > > > No answer? > > > > > > subtrees is what was decided on OF_UPSRTEAM as well. If you have a > > > better idea feel free to propose it, but for the sake of conformance > > > we are better off doing the same thing on every external tree we pull > > > in > > > > How do they will (or already do) maintain this? > > > > There's a script to do that in dts/update-dts-subtree.sh > > > At least it's a good to have a few words on the choice made in the cover > > letter, so we will have no questions on it. > > > > > > > > > Moreover, due to the Windows-style files from mbedtls git repo, > > > > > > > we need to convert the CRLF endings to LF and do a commit > manually: > > > > > > > > > > > > > > $ git add --renormalize . > > > > > > > $ git commit > > > > ... > > > > > > > > > lib/mbedtls/mbedtls_def_config.h | 4262 > ++++++++++++++++++++++++++++++ > > > > > > > > > > > > This is ridiculously HUGE! This is unreviewable. Moreover, this > is even > > > > > > hard to > > > > > > configure by the user! Can you rather make it modular and maybe > create a > > > > > > separate documentation for the most important options (I do not > believe one > > > > > > needs _all_ of them to be set / tuned)? > > > > > > > > > > > > This is a file from MbedTLS and follows its own style. > > > > > And this is how MbedTLS is configured - with all features listed > in a > > > > > config file and > > > > > commenting out the unused features with "//"). > > > > > The modification here is just to control those existing options > with > > > > > Kconfigs. > > > > > > > > And why should we blindly follow this nonsense? > > > > > > It's easier to follow up future changes tbh. But I do agree the config > > > file is huge. Perhaps splitting in 2 files is going to be easier > > > mbedtls_def_config.h -> contains all the options that rarely need > > > tuning, which I assume is the majority of the header > > > mbedtls_usef_config.h -> contains the imporant options, crypto, > > > checksum algorithms etc > > > > > > Thoughts? > > > > The problem is on who decides which are "rarely need". > > Us. Sure it's going to take some time, but eventually we should be > able to isolate options that rarely need a change > > > The feasible (to me) approach is to split per domain. Like you listed at > the very end of your reply. > > We can also learn from managing MTA configurations, such as Exim4 where > user > > may decide if they want a single file or split version. > > I am not sure tbh. Let me backpaddle from my previous suggestion :). > Looking at it closer config.h is copied from mbedTLS, commenting out > features we don't want. > > What we could do instead is either trim those comments out and only > define the options we need for now. That would make the patch easily > reviewable. If in the future we decide we need a more modular > approach, we can retrofit it without breaking anything. > > Alternatively, we can undef the options we don't want in our header, > but I am not sure how often these change or how scalable this is going > to end up being. I prefer option #1 > > I can remove the disabled features and only keep the ones that should be controlled by the Kconfigs if you don't want a huge list for user's fine-tune. Currently most of them can be regarded as default setting by U-Boot. In the future if any of them needs to be exposed as a Kconfig for tuning, we can add them back. Regards, Raymond
