| Message ID | 20200219094726.26798-1-rasmus.villemoes@prevas.dk |
|---|---|
| Headers | show |
| Series | CMD_SAVEENV ifdef cleanup | expand |
Dear Rasmus, In message <20200219094726.26798-1-rasmus.villemoes at prevas.dk> you wrote: > > [1] Here's the current conditions for which these three drivers > provide .save: > > SPL U-Boot > ext4.c CONFIG_CMD_SAVEENV=y CONFIG_CMD_SAVEENV=y > fat.c never CONFIG_CMD_SAVEENV=y > sf.c never CONFIG_CMD_SAVEENV=y [2] Some questions: 1) I'm not sure if your changes cover the situation that you want to have "saveenv" available in U-Boot proper, but do NOT want to have it in SPL. This may makie sense in situations where you need to be able to read the saved environment in SPL (say, to set up the configures console baud rate), but cannot affort the fule size resulting from adding "saveenv" etc. It is mandatory that this possibility is kept. 2) It seems wrong to me to make such cleanup in any way dependent on file system type or a mix of arbitrary storage driver types. this should be handled in two independent, orthogonal steps: a) clean up any drivers or file system accessors that do not fit into the general model b) adapt the general model to such changes Maybe it makes sense to change the order of these steps, if this results in less intrusive patches - I have no ides. In any case testing must _also_ include all the other many ways of storing the environment, including parallel or SPI NOR flash, NAND flash, UBI, UNIFS, etc. Best regards, Wolfgang Denk
On 19/02/2020 14.25, Wolfgang Denk wrote: > Dear Rasmus, > > In message <20200219094726.26798-1-rasmus.villemoes at prevas.dk> you wrote: >> >> [1] Here's the current conditions for which these three drivers >> provide .save: >> >> SPL U-Boot >> ext4.c CONFIG_CMD_SAVEENV=y CONFIG_CMD_SAVEENV=y >> fat.c never CONFIG_CMD_SAVEENV=y >> sf.c never CONFIG_CMD_SAVEENV=y [2] > > Some questions: > > > 1) I'm not sure if your changes cover the situation that you want to > have "saveenv" available in U-Boot proper, but do NOT want to > have it in SPL. They do, that's the whole point of introducing the simple CONFIG_IS_ENABLED(SAVEENV) - for answering the question "do we want to enable saving the environment in this context". Then the .save method gets built and linked in precisely if that's the case, so one gets a consistent matrix that says SPL U-Boot ext4.c CONFIG_SPL_SAVEENV=y CONFIG_CMD_SAVEENV=y fat.c CONFIG_SPL_SAVEENV=y CONFIG_CMD_SAVEENV=y sf.c CONFIG_SPL_SAVEENV=y CONFIG_CMD_SAVEENV=y But I can't fix the whole world in one go, so only the above three get fixed to that state for now. > It is mandatory that this possibility is kept. Of course, and _nothing_ changes in that regard. [It is of course possible that I messed up with the implementation, but it is certainly the intention to keep it that way.] > > 2) It seems wrong to me to make such cleanup in any way dependent on > file system type or a mix of arbitrary storage driver types. > this should be handled in two independent, orthogonal steps: > a) clean up any drivers or file system accessors that do not fit > into the general model > b) adapt the general model to such changes > > Maybe it makes sense to change the order of these steps, if this > results in less intrusive patches - I have no ides. > > In any case testing must _also_ include all the other many ways > of storing the environment, including parallel or SPI NOR flash, > NAND flash, UBI, UNIFS, etc. See above, I can't fix, much less test, all those backend drivers. Nothing changes for those, they provide a .save method under exactly the same (probably mutually inconsistent...) conditions as they used to. So I expect that when someone else runs into wanting CONFIG_SPL_SAVEENV honoured with, say, mmc backend, they probably quickly discover that doesn't work at all, and then they can fix mmc.c to make it work. But it's not in all cases as simply as just removing the custom/arbitrary ifdef logic, sometimes those ifdefs cover code that depends on certain macros or whatnot that are not available for an SPL_BUILD. Rasmus
On 19/02/2020 10.47, Rasmus Villemoes wrote: > The various env storage drivers almost all have their own logic [1] > for deciding whether to compile and provide the .save method, many of > which fail to honour CONFIG_SPL_SAVEENV. For example, fat.c and sf.c > define a CMD_SAVEENV macro only for !CONFIG_SPL_BUILD, while ext4.c > "only" depends on CONFIG_CMD_SAVEENV - but CONFIG_SPL_SAVEENV=y, > CONFIG_CMD_SAVEENV=n is a valid combination. > > A lot of that ifdeffery can be removed while at the same time > providing the .save method if either CONFIG_SPL_SAVEENV (for an SPL > build) or CONFIG_CMD_SAVEENV (for U-Boot proper) is set. The first two > patches introduce infrastructure for that, while the last three are > example conversions for the above-mentioned three storage drivers. The > sf.c is the one I need to use in the SPL and have actually tested, > ext4.c and fat.c are included mostly as low-hanging fruit. Dear Wolfgang Can I ask whether you request changes to this patch series or if my answers to your various comments have been satisfactory? Thanks, Rasmus
Dear Rasmus Villemoes, In message <9c03710e-5eec-da6e-6c15-2f8a14cfcc36 at prevas.dk> you wrote: > > Can I ask whether you request changes to this patch series or if my > answers to your various comments have been satisfactory? I think you did no really answer to some of my concerns. In Message <20200219132715.1F81A240036 at gemini.denx.de> I asked: | Have you tested that this works? How do the sizes of the | images differe before and after applying your changes? You replied: ... Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my patches we get | $ size u-boot spl/u-boot-spl | text data bss dec hex filename | 407173 45308 98352 550833 867b1 u-boot | 58298 3360 65860 127518 1f21e spl/u-boot-spl | .... | but without, | | $ size u-boot spl/u-boot-spl | text data bss dec hex filename | 407173 45308 98352 550833 867b1 u-boot | 52659 3360 280 56299 dbeb spl/u-boot-spl We can observe that - the text size of the SPL grows from 52659 to 58298, i. e. by about 5.5 kB or more than 10% - the BSS size explodes from 280 to 65860 bytes, i. e. it grows from a few hndet bytes to more than 64 kB I can see where the increase in text size is coming from - your removal of #ifdef's now unconditionally includes some code that was omitted before, for example functions env_fat_save(), env_ext4_save(), env_sf_save(), plus a few variables. It is not obvious to me but scary to see such an explosion of BSS size. It's difficult to comment here as it is not clear to me which exact configuration you reported about, and it's also not clear if this is a typical result, of if it's the only configuration you ever tested. Your patch description sounds as if it was just a #ifdef cleanup without actual impact on the generated code, but the SPL size differences above make it clear that it is not - or that your testing has issues. You also failed to comment on impact on other environment storage configurations (NOR flash, NAND flash, UBI volume, ...). If it's only #ifdef changes without impact on function, then we should get exactly the same images. You did not comment if you have verified that. Best regards, Wolfgang Denk
On 25/03/2020 08.50, Wolfgang Denk wrote: > Dear Rasmus Villemoes, > > In message <9c03710e-5eec-da6e-6c15-2f8a14cfcc36 at prevas.dk> you wrote: >> >> Can I ask whether you request changes to this patch series or if my >> answers to your various comments have been satisfactory? > > I think you did no really answer to some of my concerns. > > In Message <20200219132715.1F81A240036 at gemini.denx.de> I asked: > > | Have you tested that this works? How do the sizes of the > | images differe before and after applying your changes? > > You replied: > > ... > Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my > patches we get > > | $ size u-boot spl/u-boot-spl > | text data bss dec hex filename > | 407173 45308 98352 550833 867b1 u-boot > | 58298 3360 65860 127518 1f21e spl/u-boot-spl > | .... > | but without, > | > | $ size u-boot spl/u-boot-spl > | text data bss dec hex filename > | 407173 45308 98352 550833 867b1 u-boot > | 52659 3360 280 56299 dbeb spl/u-boot-spl > > We can observe that > > - the text size of the SPL grows from 52659 to 58298, i. e. by about > 5.5 kB or more than 10% > - the BSS size explodes from 280 to 65860 bytes, i. e. it grows from > a few hndet bytes to more than 64 kB > > I can see where the increase in text size is coming from - your > removal of #ifdef's now unconditionally includes some code that was > omitted before, for example functions env_fat_save(), > env_ext4_save(), env_sf_save(), plus a few variables. As intended for CONFIG_SPL_SAVEENV=y, no? With my patches and CONFIG_SPL_SAVEENV=n, those env_fat_save, env_ext4_save etc. are compiled, but then discarded (being static, they are discarded already at compile-time, but otherwise they would be at link-time), instead of being ifdeffed out unconditionally just because of CONFIG_SPL_BUILD. I know that you share the opinion that one should use IS_ENABLED() in preference to preprocessor conditionals, so I really don't understand what you can possibly have against this approach. > It is not obvious to me but scary to see such an explosion of BSS > size. > It's difficult to comment here as it is not clear to me which exact > configuration you reported about, Huh? I wrote exactly what I used to obtain those numbers for the FAT case. Let me quote a bit more ==== With or without these patches, I get $ size u-boot spl/u-boot-spl text data bss dec hex filename 407173 45308 98352 550833 867b1 u-boot 52403 3360 276 56039 dae7 spl/u-boot-spl $ nm spl/u-boot-spl | grep env_fat 0090c5e8 t env_fat_load $ nm u-boot | grep env_fat 17826cb4 t env_fat_load 17826c10 t env_fat_save for a wandboard_defconfig modified by -CONFIG_SPL_FS_EXT4=y +CONFIG_SPL_FS_FAT=y +CONFIG_SPL_ENV_SUPPORT=y +CONFIG_ENV_IS_IN_FAT=y So in the "read-only environment access in SPL" case, everything is the same before and after. ==== That was the answer to "does it affect the generated code when one doesn't enable CONFIG_SPL_SAVEENV". It doesn't, not at all, the code is exactly the same. The next part then demonstrated how CONFIG_SPL_SAVEENV is currently being ignored because of the ifdeffery in fat.c: ==== Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my patches we get $ size u-boot spl/u-boot-spl text data bss dec hex filename 407173 45308 98352 550833 867b1 u-boot 58298 3360 65860 127518 1f21e spl/u-boot-spl $ nm spl/u-boot-spl | grep env_fat 0090c6e0 t env_fat_load 0090c63c t env_fat_save but without, $ size u-boot spl/u-boot-spl text data bss dec hex filename 407173 45308 98352 550833 867b1 u-boot 52659 3360 280 56299 dbeb spl/u-boot-spl $ nm spl/u-boot-spl | grep env_fat 0090c5e8 t env_fat_load So without the fat.c patch, CONFIG_SPL_SAVEENV is effectively ignored. ==== The .bss increase is simply due to the extra code that no longer gets discarded by the linker, more precisely the .map file says there's a .bss.tmpbuf_cluster 0x0000000000000000 0x10000 fs/built-in.o that gets discarded without my patches (but with the config options chosen so one would _expect_ to have save support in SPL). So yes, of course there's a price to pay for enabling environment save support in the SPL, with some backends being more expensive (in terms of footprint) than others. > and it's also not clear if this is > a typical result, of if it's the only configuration you ever > tested. > > > Your patch description sounds as if it was just a #ifdef cleanup > without actual impact on the generated code, but the SPL size > differences above make it clear that it is not - or that your > testing has issues. There is _no_ change in code size, u-boot or spl, when CONFIG_SPL_SAVEENV=n. My patches _only_ affect the case where CONFIG_SPL_SAVEENV=y, and only in a way that the developer most likely intended, namely actually allowing one to save the environment. It is a cleanup, it removes pointless and actively harmful ifdeffery that each storage driver has grown for no good reason (perhaps the "if it's an SPL build, we don't need the .save method" logic all predates the introduction of CONFIG_SPL_SAVEENV). > You also failed to comment on impact on other environment storage > configurations (NOR flash, NAND flash, UBI volume, ...). I don't touch those files at all, so they are not affected. Some still fail to honour CONFIG_SPL_SAVEENV (i.e., even if one sets CONFIG_SPL_SAVEENV, saving the environment in the SPL is not actually supported). If it's > only #ifdef changes without impact on function, then we should get > exactly the same images. You did not comment if you have verified > that. To repeat myself, for CONFIG_SPL_SAVEENV=n, these patches don't change anything, except get rid of a lot of pointless ifdefs. For CONFIG_SPL_SAVEENV=y, the patches serve to honour the developer's intention of actually being able to save the environment from SPL, at least for fat, ext4 and sf. Rasmus
Dear Rasmus, In message <51077b65-56c1-464a-8721-77b6a7bf385d at prevas.dk> you wrote: > > > > I think you did no really answer to some of my concerns. > > > > In Message <20200219132715.1F81A240036 at gemini.denx.de> I asked: > > > > | Have you tested that this works? How do the sizes of the > > | images differe before and after applying your changes? > > > > You replied: > > > > ... > > Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my > > patches we get > > > > | $ size u-boot spl/u-boot-spl > > | text data bss dec hex filename > > | 407173 45308 98352 550833 867b1 u-boot > > | 58298 3360 65860 127518 1f21e spl/u-boot-spl > > | .... > > | but without, > > | > > | $ size u-boot spl/u-boot-spl > > | text data bss dec hex filename > > | 407173 45308 98352 550833 867b1 u-boot > > | 52659 3360 280 56299 dbeb spl/u-boot-spl > > > > We can observe that > > > > - the text size of the SPL grows from 52659 to 58298, i. e. by about > > 5.5 kB or more than 10% > > - the BSS size explodes from 280 to 65860 bytes, i. e. it grows from > > a few hndet bytes to more than 64 kB > > > > I can see where the increase in text size is coming from - your > > removal of #ifdef's now unconditionally includes some code that was > > omitted before, for example functions env_fat_save(), > > env_ext4_save(), env_sf_save(), plus a few variables. > > As intended for CONFIG_SPL_SAVEENV=y, no? As you don't bother to mention which exact configuration you are testing against, it is impossible to tell what exactly you are doing. But you give here two versions _with_ and _without_ your patches, so I _assume_ all the other parameters are kept the same, and with your patches the BSS size explodes. Please elucidate, > With my patches and CONFIG_SPL_SAVEENV=n, those env_fat_save, > env_ext4_save etc. are compiled, but then discarded (being static, they > are discarded already at compile-time, but otherwise they would be at > link-time), instead of being ifdeffed out unconditionally just because > of CONFIG_SPL_BUILD. I know that you share the opinion that one should > use IS_ENABLED() in preference to preprocessor conditionals, so I really > don't understand what you can possibly have against this approach. Please explain why the memory footprint explodes. > > It's difficult to comment here as it is not clear to me which exact > > configuration you reported about, > > Huh? I wrote exactly what I used to obtain those numbers for the FAT > case. Let me quote a bit more And what exactly is "the FAT case"? You did not explain what exactly you are comparing... > for a wandboard_defconfig modified by > > -CONFIG_SPL_FS_EXT4=y > +CONFIG_SPL_FS_FAT=y > +CONFIG_SPL_ENV_SUPPORT=y > +CONFIG_ENV_IS_IN_FAT=y Is this your test case? I don't think you mentioned this before. You your test cases are mainline U-Boot, using wandboard_defconfig plus these 4 changes, and then either with and without your patches applied? > That was the answer to "does it affect the generated code when one > doesn't enable CONFIG_SPL_SAVEENV". It doesn't, not at all, the code is > exactly the same. The next part then demonstrated how CONFIG_SPL_SAVEENV > is currently being ignored because of the ifdeffery in fat.c: No. My question was if the code size differs for the same configurations with and without your patches applied. > Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my > patches we get > > $ size u-boot spl/u-boot-spl > text data bss dec hex filename > 407173 45308 98352 550833 867b1 u-boot > 58298 3360 65860 127518 1f21e spl/u-boot-spl > $ nm spl/u-boot-spl | grep env_fat > 0090c6e0 t env_fat_load > 0090c63c t env_fat_save > > but without, > > $ size u-boot spl/u-boot-spl > text data bss dec hex filename > 407173 45308 98352 550833 867b1 u-boot > 52659 3360 280 56299 dbeb spl/u-boot-spl OK, here you confirm it again - with your patches the BSS size explodes from 280 to 65860 bytes, that's a faxtor of more than 200. Don't you think that looks fishy? > The .bss increase is simply due to the extra code that no longer gets > discarded by the linker, more precisely the .map file says there's a This makes absolutley no sense. BSS does not include any code, it's uninitialized data. And iff we have the situation, that with your patches any "extra code ... no longer gets discarded by the linker", then something is broken with your patches, and this must be fixed. > .bss.tmpbuf_cluster > 0x0000000000000000 0x10000 fs/built-in.o > > that gets discarded without my patches (but with the config options > chosen so one would _expect_ to have save support in SPL). So yes, of > course there's a price to pay for enabling environment save support in > the SPL, with some backends being more expensive (in terms of footprint) > than others. A price of more than 64 kB additional memory footprint in the SPL is a strict no-go. This must be fixed, and I'm surprised that you did not even spend a thought about this after I explicitly mentioned it. This all makes no sense to me, as tmpbuf_cluster[] comes from fs/fat/fat_write.c, which should not be used for the SPL when you don't enable both FAT and SAVEENV support together. > > and it's also not clear if this is > > a typical result, of if it's the only configuration you ever > > tested. You continue to ignore this question. > > Your patch description sounds as if it was just a #ifdef cleanup > > without actual impact on the generated code, but the SPL size > > differences above make it clear that it is not - or that your > > testing has issues. > > There is _no_ change in code size, u-boot or spl, when > CONFIG_SPL_SAVEENV=n. My patches _only_ affect the case where > CONFIG_SPL_SAVEENV=y, and only in a way that the developer most likely > intended, namely actually allowing one to save the environment. This makes no sense either. If you compare the SAME configuration with and without your patches above, then we have this unacceptable BSS explosing, which is unacceptable. If you compare two different configurations above, one with CONFIG_SPL_SAVEENV=n and one with CONFIG_SPL_SAVEENV=y, then the whole comparion makes no sense. As long as we stick with the same single board (wandboard_defconfig), plus the 4 lines changed, there would be 4 different cases to test: - CONFIG_SPL_SAVEENV=n without your patches - CONFIG_SPL_SAVEENV=n with your patches - CONFIG_SPL_SAVEENV=y without your patches - CONFIG_SPL_SAVEENV=y with your patches Anything else is comparing apples and bicycles. > > You also failed to comment on impact on other environment storage > > configurations (NOR flash, NAND flash, UBI volume, ...). > > I don't touch those files at all, so they are not affected. Some still > fail to honour CONFIG_SPL_SAVEENV (i.e., even if one sets > CONFIG_SPL_SAVEENV, saving the environment in the SPL is not actually > supported). You _say_ they are not affected, and I accept that this is your intention. But my question was if you actually _tested_ that your patches behave as intented? I think there have been cases before where code changes had ... let's say unexpected side effects... You should build a few (if not all!) such boards with and without your patches applied and _verify_ the the code does not change. Just guessing is not good enough. > To repeat myself, for CONFIG_SPL_SAVEENV=n, these patches don't change > anything, except get rid of a lot of pointless ifdefs. For > CONFIG_SPL_SAVEENV=y, the patches serve to honour the developer's > intention of actually being able to save the environment from SPL, at > least for fat, ext4 and sf. You continue to fail to prove that. Best regards, Wolfgang Denk
On 26/03/2020 15.31, Wolfgang Denk wrote: > Dear Rasmus, > >>> >>> I can see where the increase in text size is coming from - your >>> removal of #ifdef's now unconditionally includes some code that was >>> omitted before, for example functions env_fat_save(), >>> env_ext4_save(), env_sf_save(), plus a few variables. >> >> As intended for CONFIG_SPL_SAVEENV=y, no? > > As you don't bother to mention which exact configuration you are > testing against, it is impossible to tell what exactly you are > doing. > > But you give here two versions _with_ and _without_ your patches, > so I _assume_ all the other parameters are kept the same, and with > your patches the BSS size explodes. > > Please elucidate, > >> With my patches and CONFIG_SPL_SAVEENV=n, those env_fat_save, >> env_ext4_save etc. are compiled, but then discarded (being static, they >> are discarded already at compile-time, but otherwise they would be at >> link-time), instead of being ifdeffed out unconditionally just because >> of CONFIG_SPL_BUILD. I know that you share the opinion that one should >> use IS_ENABLED() in preference to preprocessor conditionals, so I really >> don't understand what you can possibly have against this approach. > > Please explain why the memory footprint explodes. When CONFIG_SPL_FAT_WRITE is enabled, but nothing actually uses that functionality, the linker discards those functions, _and_ any data/bss elements which those discarded functions were the only user of. One of those elements happen to be a static 64K byte array. Now, with the current master branch, the logic for whether env/fat.c defines its own CMD_SAVEENV macro means that whether or not one enables CONFIG_SPL_SAVEENV, env_fat_save does not get compiled in SPL, so there's no user of file_fat_write(), and the linker then discards that along with the 64K static buffer. But of course that also means that saving the environment is not actually possible. >>> It's difficult to comment here as it is not clear to me which exact >>> configuration you reported about, >> >> Huh? I wrote exactly what I used to obtain those numbers for the FAT >> case. Let me quote a bit more > > And what exactly is "the FAT case"? The case where the storage backend is FAT, as opposed to ext4 and sf that are also in the series. You did not explain what > exactly you are comparing... > >> for a wandboard_defconfig modified by >> >> -CONFIG_SPL_FS_EXT4=y >> +CONFIG_SPL_FS_FAT=y >> +CONFIG_SPL_ENV_SUPPORT=y >> +CONFIG_ENV_IS_IN_FAT=y > > Is this your test case? I don't think you mentioned this before. I did, as is totally apparent if you cared to read back a bit in the thread to verify that I was indeed quoting myself. > You your test cases are mainline U-Boot, using wandboard_defconfig > plus these 4 changes, and then either with and without your patches > applied? That's one test case, in order to demonstrate how CONFIG_SPL_SAVEENV is currently vapourware when used with FAT as backend (and for that matter, a lot of other backends as well). >> That was the answer to "does it affect the generated code when one >> doesn't enable CONFIG_SPL_SAVEENV". It doesn't, not at all, the code is >> exactly the same. The next part then demonstrated how CONFIG_SPL_SAVEENV >> is currently being ignored because of the ifdeffery in fat.c: > > No. My question was if the code size differs for the same > configurations with and without your patches applied. For a configuration without CONFIG_SPL_SAVEENV, neither SPL or U-Boot binary size (or the .bss section) differs at all with my patches applied. For a configuration with CONFIG_SPL_SAVEENV (and SPL_FAT_WRITE, which is necessary to make it link), the SPL does change with my patches applied, in that env_fat_safe now gets compiled in, which means file_fat_write and everything that references no longer gets discarded. >> Now also enable CONFIG_SPL_SAVEENV and SPL_FAT_WRITE, then with my >> patches we get >> >> $ size u-boot spl/u-boot-spl >> text data bss dec hex filename >> 407173 45308 98352 550833 867b1 u-boot >> 58298 3360 65860 127518 1f21e spl/u-boot-spl >> $ nm spl/u-boot-spl | grep env_fat >> 0090c6e0 t env_fat_load >> 0090c63c t env_fat_save >> >> but without, >> >> $ size u-boot spl/u-boot-spl >> text data bss dec hex filename >> 407173 45308 98352 550833 867b1 u-boot >> 52659 3360 280 56299 dbeb spl/u-boot-spl > > OK, here you confirm it again - with your patches the BSS size > explodes from 280 to 65860 bytes, that's a faxtor of more than 200. > > Don't you think that looks fishy? No, not at all. >> The .bss increase is simply due to the extra code that no longer gets >> discarded by the linker, more precisely the .map file says there's a > > This makes absolutley no sense. BSS does not include any code, it's > uninitialized data. Read "code and/or data". > And iff we have the situation, that with your patches any "extra > code ... no longer gets discarded by the linker", then something is > broken with your patches, and this must be fixed. That all works just as usual, it's just that when env/fat.c is patched to not ignore CONFIG_SPL_SAVEENV, file_fat_write is no longer unreferenced. >> .bss.tmpbuf_cluster >> 0x0000000000000000 0x10000 fs/built-in.o >> >> that gets discarded without my patches (but with the config options >> chosen so one would _expect_ to have save support in SPL). So yes, of >> course there's a price to pay for enabling environment save support in >> the SPL, with some backends being more expensive (in terms of footprint) >> than others. > > A price of more than 64 kB additional memory footprint in the SPL is > a strict no-go. Well, then CONFIG_SPL_FAT_WRITE should be removed, or the code rewritten to not rely on a statically allocated buffer. Nothing to do with my patches. > This must be fixed, and I'm surprised that you did not even spend a > thought about this after I explicitly mentioned it. > > This all makes no sense to me, as tmpbuf_cluster[] comes from > fs/fat/fat_write.c, which should not be used for the SPL when you > don't enable both FAT and SAVEENV support together. Exactly, tmpbuf_cluster[] only gets compiled with CONFIG_SPL_FAT_WRITE. Without a user, it gets discarded. Enabling CONFIG_SPL_SAVEENV should create such a user, but it doesn't in current master. >>> Your patch description sounds as if it was just a #ifdef cleanup >>> without actual impact on the generated code, but the SPL size >>> differences above make it clear that it is not - or that your >>> testing has issues. >> >> There is _no_ change in code size, u-boot or spl, when >> CONFIG_SPL_SAVEENV=n. My patches _only_ affect the case where >> CONFIG_SPL_SAVEENV=y, and only in a way that the developer most likely >> intended, namely actually allowing one to save the environment. > > This makes no sense either. > > If you compare the SAME configuration with and without your patches > above, then we have this unacceptable BSS explosing, which is > unacceptable. I do (compare the same configuration with and without my patches). > > As long as we stick with the same single board (wandboard_defconfig), > plus the 4 lines changed, there would be 4 different cases to test: > > - CONFIG_SPL_SAVEENV=n without your patches > - CONFIG_SPL_SAVEENV=n with your patches > - CONFIG_SPL_SAVEENV=y without your patches > - CONFIG_SPL_SAVEENV=y with your patches > > Anything else is comparing apples and bicycles. These are exactly the cases I have shown. The first two are covered by (and I quote this again) ==== With or without these patches, I get $ size u-boot spl/u-boot-spl text data bss dec hex filename 407173 45308 98352 550833 867b1 u-boot 52403 3360 276 56039 dae7 spl/u-boot-spl $ nm spl/u-boot-spl | grep env_fat 0090c5e8 t env_fat_load $ nm u-boot | grep env_fat 17826cb4 t env_fat_load 17826c10 t env_fat_save for a wandboard_defconfig modified by -CONFIG_SPL_FS_EXT4=y +CONFIG_SPL_FS_FAT=y +CONFIG_SPL_ENV_SUPPORT=y +CONFIG_ENV_IS_IN_FAT=y So in the "read-only environment access in SPL" case, everything is the same before and after. ==== Note the "with or without", the sizes shown are for both cases, everything really is the same. Then when one enables CONFIG_SPL_SAVEENV and CONFIG_SPL_FAT_WRITE, without my patches, the result is $ size u-boot spl/u-boot-spl text data bss dec hex filename 407173 45308 98352 550833 867b1 u-boot 52659 3360 280 56299 dbeb spl/u-boot-spl $ nm spl/u-boot-spl | grep env_fat 0090c5e8 t env_fat_load So, because env/fat.c never builds env_fat_save for an SPL build, enabling those options don't add very much to the SPL - fs/fat/fat_write.o does get built, but most of it gets discarded. And yes, of course when file_fat_write does have a user, as it has with my patches applied and SPL_SAVEENV=y,SPL_FAT_WRITE=y, it no longer gets discarded. >>> You also failed to comment on impact on other environment storage >>> configurations (NOR flash, NAND flash, UBI volume, ...). >> >> I don't touch those files at all, so they are not affected. Some still >> fail to honour CONFIG_SPL_SAVEENV (i.e., even if one sets >> CONFIG_SPL_SAVEENV, saving the environment in the SPL is not actually >> supported). > > You _say_ they are not affected, and I accept that this is your > intention. But my question was if you actually _tested_ that your > patches behave as intented? I think there have been cases before > where code changes had ... let's say unexpected side effects... Sure, but let's be a little bit reasonable here. I add one CONFIG_* symbol (CONFIG_SAVEENV, a convenience alias for CONFIG_CMD_SAVEENV), and one macro, with a hitherto completely unused identifier, in include/env_internal.h. > You should build a few (if not all!) such boards with and without > your patches applied and _verify_ the the code does not change. > Just guessing is not good enough. OK, I will do that, though I don't know how to prove that I've done it. >> To repeat myself, for CONFIG_SPL_SAVEENV=n, these patches don't change >> anything, except get rid of a lot of pointless ifdefs. For >> CONFIG_SPL_SAVEENV=y, the patches serve to honour the developer's >> intention of actually being able to save the environment from SPL, at >> least for fat, ext4 and sf. > > You continue to fail to prove that. And how do you expect me carry out such a proof? I'm getting really tired of this, so I will resend a much simpler two-part series that only fixes the sf.c case, and without the ENV_SAVE_PTR macro which doesn't actually provide any value over just open-coding "CONFIG_IS_ENABLED(SAVEENV) ? env_sf_save : NULL". Rasmus
The various env storage drivers almost all have their own logic [1] for deciding whether to compile and provide the .save method, many of which fail to honour CONFIG_SPL_SAVEENV. For example, fat.c and sf.c define a CMD_SAVEENV macro only for !CONFIG_SPL_BUILD, while ext4.c "only" depends on CONFIG_CMD_SAVEENV - but CONFIG_SPL_SAVEENV=y, CONFIG_CMD_SAVEENV=n is a valid combination. A lot of that ifdeffery can be removed while at the same time providing the .save method if either CONFIG_SPL_SAVEENV (for an SPL build) or CONFIG_CMD_SAVEENV (for U-Boot proper) is set. The first two patches introduce infrastructure for that, while the last three are example conversions for the above-mentioned three storage drivers. The sf.c is the one I need to use in the SPL and have actually tested, ext4.c and fat.c are included mostly as low-hanging fruit. [1] Here's the current conditions for which these three drivers provide .save: SPL U-Boot ext4.c CONFIG_CMD_SAVEENV=y CONFIG_CMD_SAVEENV=y fat.c never CONFIG_CMD_SAVEENV=y sf.c never CONFIG_CMD_SAVEENV=y [2] [2] It always compiles env_sf_save for U-Boot proper, but then the use of env_save_ptr() ends up with a build warning in case CONFIG_CMD_SAVEENV=n - fat.c doesn't have that proplem. Rasmus Villemoes (5): env: add SAVEENV as an alias of the CMD_SAVEENV symbol env_internal.h: add alternative ENV_SAVE_PTR macro env/fat.c: remove private CMD_SAVEENV logic env/ext4.c: remove CONFIG_CMD_SAVEENV ifdef env/sf.c: drop private CMD_SAVEENV logic env/Kconfig | 3 +++ env/ext4.c | 4 +--- env/fat.c | 9 +-------- env/sf.c | 12 +----------- include/env_internal.h | 2 ++ 5 files changed, 8 insertions(+), 22 deletions(-)