[GIT] Pull request: u-boot-dfu (26.01.2020)

[GIT] Pull request: u-boot-dfu (26.01.2020)

[Lukasz Majewski]

Dear Marek,

The following changes since commit
40521a6c90d4adfb7f3041033c8cbbeff087a5ca:

  Merge https://gitlab.denx.de/u-boot/custodians/u-boot-fsl-qoriq
  (2020-01-25 12:20:51 -0500)

are available in the Git repository at:

  git at gitlab.denx.de:u-boot/custodians/u-boot-dfu.git 

for you to fetch changes up to d34375267f521dc76b1875a905cec914a47b0712:

  dfu: Add option to skip empty pages when flashing UBI images to NAND
  (2020-01-26 11:44:15 +0100)

----------------------------------------------------------------
Guillermo Rodríguez (1):
      dfu: Add option to skip empty pages when flashing UBI images to
NAND

 drivers/dfu/Kconfig    | 7 +++++++
 drivers/dfu/dfu_nand.c | 7 ++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

BRANCH: master
https://gitlab.denx.de/u-boot/custodians/u-boot-dfu/commits/master

Merge tag info:
- Add option to DFU NAND to skip empty pages when flashing UBI images


Travis-CI:
https://travis-ci.org/lmajewski/u-boot-dfu/builds/618507384

Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200126/d29ebb80/attachment.sig>

[GIT] Pull request: u-boot-dfu (26.01.2020)

[Marek Vasut]

On 1/26/20 9:26 PM, Lukasz Majewski wrote:
Hi,

[...]

> ----------------------------------------------------------------
> Guillermo Rodríguez (1):
>       dfu: Add option to skip empty pages when flashing UBI images to

Can that option be enabled/disabled at runtime instead of being hardcoded?

[GIT] Pull request: u-boot-dfu (26.01.2020)

[Lukasz Majewski]

Hi Marek,

> On 1/26/20 9:26 PM, Lukasz Majewski wrote:
> Hi,
> 
> [...]
> 
> > ----------------------------------------------------------------
> > Guillermo Rodríguez (1):
> >       dfu: Add option to skip empty pages when flashing UBI images
> > to  
> 
> Can that option be enabled/disabled at runtime instead of being
> hardcoded?

It has been designed in a similar way to Android's existing
FASTBOOT_FLASH_NAND_TRIMFFS option.


Best regards,

Lukasz Majewski

--

DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-59 Fax: (+49)-8142-66989-80 Email: lukma at denx.de
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200127/ee70c739/attachment.sig>

[GIT] Pull request: u-boot-dfu (26.01.2020)

[Guillermo Rodriguez Garcia]

Hi Marek,

El lun., 27 ene. 2020 a las 8:52, Lukasz Majewski (<lukma@denx.de>) escribió:
>
> Hi Marek,
>
> > On 1/26/20 9:26 PM, Lukasz Majewski wrote:
> > Hi,
> >
> > [...]
> >
> > > ----------------------------------------------------------------
> > > Guillermo Rodríguez (1):
> > >       dfu: Add option to skip empty pages when flashing UBI images
> > > to
> >
> > Can that option be enabled/disabled at runtime instead of being
> > hardcoded?
>
> It has been designed in a similar way to Android's existing
> FASTBOOT_FLASH_NAND_TRIMFFS option.

Without this option, UBI images need to be built with --space-fixup
[1] so that the kernel can "fix" the NAND on first mount.
When this option is used, --space-fixup is no longer necessary because
dfu knows how to correctly flash UBI images. However, UBI images built
with --space-fixup will still work fine.
In other words, enabling this option at buildtime has no countereffects.
So there is no point in making it configurable at runtime, if support
has been built into U-boot.

 [1]: http://www.linux-mtd.infradead.org/faq/ubifs.html#L_free_space_fixup

Best regards,

Guillermo Rodriguez Garcia
guille.rodriguez at gmail.com

[GIT] Pull request: u-boot-dfu (26.01.2020)

[Marek Vasut]

On 1/27/20 8:52 AM, Lukasz Majewski wrote:
> Hi Marek,

Hi,

>> On 1/26/20 9:26 PM, Lukasz Majewski wrote:
>> Hi,
>>
>> [...]
>>
>>> ----------------------------------------------------------------
>>> Guillermo Rodríguez (1):
>>>       dfu: Add option to skip empty pages when flashing UBI images
>>> to  
>>
>> Can that option be enabled/disabled at runtime instead of being
>> hardcoded?
> 
> It has been designed in a similar way to Android's existing
> FASTBOOT_FLASH_NAND_TRIMFFS option.

Which doesn't really answer my question, it only shows that another
piece of code suffers from the same problem.

[GIT] Pull request: u-boot-dfu (26.01.2020)

[Marek Vasut]

On 1/27/20 9:14 AM, Guillermo Rodriguez Garcia wrote:
> Hi Marek,

Hi,

> El lun., 27 ene. 2020 a las 8:52, Lukasz Majewski (<lukma@denx.de>) escribió:
>>
>> Hi Marek,
>>
>>> On 1/26/20 9:26 PM, Lukasz Majewski wrote:
>>> Hi,
>>>
>>> [...]
>>>
>>>> ----------------------------------------------------------------
>>>> Guillermo Rodríguez (1):
>>>>       dfu: Add option to skip empty pages when flashing UBI images
>>>> to
>>>
>>> Can that option be enabled/disabled at runtime instead of being
>>> hardcoded?
>>
>> It has been designed in a similar way to Android's existing
>> FASTBOOT_FLASH_NAND_TRIMFFS option.
> 
> Without this option, UBI images need to be built with --space-fixup
> [1] so that the kernel can "fix" the NAND on first mount.
> When this option is used, --space-fixup is no longer necessary because
> dfu knows how to correctly flash UBI images. However, UBI images built
> with --space-fixup will still work fine.

Does NAND.TRIMFFS preserve UBI erase counters in the NAND ? I don't
think so, so I don't think "correctly flash UBI images" is the correct
formulation here.

> In other words, enabling this option at buildtime has no countereffects.
> So there is no point in making it configurable at runtime, if support
> has been built into U-boot.
> 
>  [1]: http://www.linux-mtd.infradead.org/faq/ubifs.html#L_free_space_fixup

So what if I want to write raw NAND image without "trimffs" on such a
system via DFU, e.g. a bootloader ? How can I do that ?

[GIT] Pull request: u-boot-dfu (26.01.2020)

[Guillermo Rodriguez]

Hi,

El lunes, 27 de enero de 2020, Marek Vasut <marex@denx.de> escribió:

> On 1/27/20 9:14 AM, Guillermo Rodriguez Garcia wrote:
> > Hi Marek,
>
> Hi,
>
> > El lun., 27 ene. 2020 a las 8:52, Lukasz Majewski (<lukma@denx.de>)
> escribió:
> >>
> >> Hi Marek,
> >>
> >>> On 1/26/20 9:26 PM, Lukasz Majewski wrote:
> >>> Hi,
> >>>
> >>> [...]
> >>>
> >>>> ----------------------------------------------------------------
> >>>> Guillermo Rodríguez (1):
> >>>>       dfu: Add option to skip empty pages when flashing UBI images
> >>>> to
> >>>
> >>> Can that option be enabled/disabled at runtime instead of being
> >>> hardcoded?
> >>
> >> It has been designed in a similar way to Android's existing
> >> FASTBOOT_FLASH_NAND_TRIMFFS option.
> >
> > Without this option, UBI images need to be built with --space-fixup
> > [1] so that the kernel can "fix" the NAND on first mount.
> > When this option is used, --space-fixup is no longer necessary because
> > dfu knows how to correctly flash UBI images. However, UBI images built
> > with --space-fixup will still work fine.
>
> Does NAND.TRIMFFS preserve UBI erase counters in the NAND ? I don't
> think so, so I don't think "correctly flash UBI images" is the correct
> formulation here.
>
> Yes, you are right.


> > In other words, enabling this option at buildtime has no countereffects.
> > So there is no point in making it configurable at runtime, if support
> > has been built into U-boot.
> >
> >  [1]: http://www.linux-mtd.infradead.org/faq/ubifs.html#
> L_free_space_fixup
>
> So what if I want to write raw NAND image without "trimffs" on such a
> system via DFU, e.g. a bootloader ? How can I do that ?
>

You mean on a non-ubi partition? Then you don't need to do anything
special. This code is only triggered for ubi partitions.

BR,

Guillermo

[GIT] Pull request: u-boot-dfu (26.01.2020)

[Marek Vasut]

On 1/27/20 2:26 PM, Guillermo Rodriguez wrote:
> Hi,

Hi,

> El lunes, 27 de enero de 2020, Marek Vasut <marex@denx.de> escribió:
> 
>> On 1/27/20 9:14 AM, Guillermo Rodriguez Garcia wrote:
>>> Hi Marek,
>>
>> Hi,
>>
>>> El lun., 27 ene. 2020 a las 8:52, Lukasz Majewski (<lukma@denx.de>)
>> escribió:
>>>>
>>>> Hi Marek,
>>>>
>>>>> On 1/26/20 9:26 PM, Lukasz Majewski wrote:
>>>>> Hi,
>>>>>
>>>>> [...]
>>>>>
>>>>>> ----------------------------------------------------------------
>>>>>> Guillermo Rodríguez (1):
>>>>>>       dfu: Add option to skip empty pages when flashing UBI images
>>>>>> to
>>>>>
>>>>> Can that option be enabled/disabled at runtime instead of being
>>>>> hardcoded?
>>>>
>>>> It has been designed in a similar way to Android's existing
>>>> FASTBOOT_FLASH_NAND_TRIMFFS option.
>>>
>>> Without this option, UBI images need to be built with --space-fixup
>>> [1] so that the kernel can "fix" the NAND on first mount.
>>> When this option is used, --space-fixup is no longer necessary because
>>> dfu knows how to correctly flash UBI images. However, UBI images built
>>> with --space-fixup will still work fine.
>>
>> Does NAND.TRIMFFS preserve UBI erase counters in the NAND ? I don't
>> think so, so I don't think "correctly flash UBI images" is the correct
>> formulation here.
>>
>> Yes, you are right.
> 
> 
>>> In other words, enabling this option at buildtime has no countereffects.
>>> So there is no point in making it configurable at runtime, if support
>>> has been built into U-boot.
>>>
>>>  [1]: http://www.linux-mtd.infradead.org/faq/ubifs.html#
>> L_free_space_fixup
>>
>> So what if I want to write raw NAND image without "trimffs" on such a
>> system via DFU, e.g. a bootloader ? How can I do that ?
>>
> 
> You mean on a non-ubi partition? Then you don't need to do anything
> special. This code is only triggered for ubi partitions.

And what about partitions which were already built with the --space-fixup ?

[GIT] Pull request: u-boot-dfu (26.01.2020)

[Guillermo Rodriguez]

El lunes, 27 de enero de 2020, Marek Vasut <marex@denx.de> escribió:

> On 1/27/20 2:26 PM, Guillermo Rodriguez wrote:
> > Hi,
>
> Hi,
>
> > El lunes, 27 de enero de 2020, Marek Vasut <marex@denx.de> escribió:
> >
> >> On 1/27/20 9:14 AM, Guillermo Rodriguez Garcia wrote:
> >>> Hi Marek,
> >>
> >> Hi,
> >>
> >>> El lun., 27 ene. 2020 a las 8:52, Lukasz Majewski (<lukma@denx.de>)
> >> escribió:
> >>>>
> >>>> Hi Marek,
> >>>>
> >>>>> On 1/26/20 9:26 PM, Lukasz Majewski wrote:
> >>>>> Hi,
> >>>>>
> >>>>> [...]
> >>>>>
> >>>>>> ----------------------------------------------------------------
> >>>>>> Guillermo Rodríguez (1):
> >>>>>>       dfu: Add option to skip empty pages when flashing UBI images
> >>>>>> to
> >>>>>
> >>>>> Can that option be enabled/disabled at runtime instead of being
> >>>>> hardcoded?
> >>>>
> >>>> It has been designed in a similar way to Android's existing
> >>>> FASTBOOT_FLASH_NAND_TRIMFFS option.
> >>>
> >>> Without this option, UBI images need to be built with --space-fixup
> >>> [1] so that the kernel can "fix" the NAND on first mount.
> >>> When this option is used, --space-fixup is no longer necessary because
> >>> dfu knows how to correctly flash UBI images. However, UBI images built
> >>> with --space-fixup will still work fine.
> >>
> >> Does NAND.TRIMFFS preserve UBI erase counters in the NAND ? I don't
> >> think so, so I don't think "correctly flash UBI images" is the correct
> >> formulation here.
> >>
> >> Yes, you are right.
> >
> >
> >>> In other words, enabling this option at buildtime has no
> countereffects.
> >>> So there is no point in making it configurable at runtime, if support
> >>> has been built into U-boot.
> >>>
> >>>  [1]: http://www.linux-mtd.infradead.org/faq/ubifs.html#
> >> L_free_space_fixup
> >>
> >> So what if I want to write raw NAND image without "trimffs" on such a
> >> system via DFU, e.g. a bootloader ? How can I do that ?
> >>
> >
> > You mean on a non-ubi partition? Then you don't need to do anything
> > special. This code is only triggered for ubi partitions.
>
> And what about partitions which were already built with the --space-fixup ?
>

What do you mean with "partitions built with --space-fixup"? If you mean
ubi *images* built with --space-fixup, these will still work fine.
Otherwise, could you please clarify?

Guillermo

[GIT] Pull request: u-boot-dfu (26.01.2020)

[Marek Vasut]

On 1/27/20 2:35 PM, Guillermo Rodriguez wrote:
> El lunes, 27 de enero de 2020, Marek Vasut <marex@denx.de> escribió:
> 
>> On 1/27/20 2:26 PM, Guillermo Rodriguez wrote:
>>> Hi,
>>
>> Hi,
>>
>>> El lunes, 27 de enero de 2020, Marek Vasut <marex@denx.de> escribió:
>>>
>>>> On 1/27/20 9:14 AM, Guillermo Rodriguez Garcia wrote:
>>>>> Hi Marek,
>>>>
>>>> Hi,
>>>>
>>>>> El lun., 27 ene. 2020 a las 8:52, Lukasz Majewski (<lukma@denx.de>)
>>>> escribió:
>>>>>>
>>>>>> Hi Marek,
>>>>>>
>>>>>>> On 1/26/20 9:26 PM, Lukasz Majewski wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> [...]
>>>>>>>
>>>>>>>> ----------------------------------------------------------------
>>>>>>>> Guillermo Rodríguez (1):
>>>>>>>>       dfu: Add option to skip empty pages when flashing UBI images
>>>>>>>> to
>>>>>>>
>>>>>>> Can that option be enabled/disabled at runtime instead of being
>>>>>>> hardcoded?
>>>>>>
>>>>>> It has been designed in a similar way to Android's existing
>>>>>> FASTBOOT_FLASH_NAND_TRIMFFS option.
>>>>>
>>>>> Without this option, UBI images need to be built with --space-fixup
>>>>> [1] so that the kernel can "fix" the NAND on first mount.
>>>>> When this option is used, --space-fixup is no longer necessary because
>>>>> dfu knows how to correctly flash UBI images. However, UBI images built
>>>>> with --space-fixup will still work fine.
>>>>
>>>> Does NAND.TRIMFFS preserve UBI erase counters in the NAND ? I don't
>>>> think so, so I don't think "correctly flash UBI images" is the correct
>>>> formulation here.
>>>>
>>>> Yes, you are right.
>>>
>>>
>>>>> In other words, enabling this option at buildtime has no
>> countereffects.
>>>>> So there is no point in making it configurable at runtime, if support
>>>>> has been built into U-boot.
>>>>>
>>>>>  [1]: http://www.linux-mtd.infradead.org/faq/ubifs.html#
>>>> L_free_space_fixup
>>>>
>>>> So what if I want to write raw NAND image without "trimffs" on such a
>>>> system via DFU, e.g. a bootloader ? How can I do that ?
>>>>
>>>
>>> You mean on a non-ubi partition? Then you don't need to do anything
>>> special. This code is only triggered for ubi partitions.
>>
>> And what about partitions which were already built with the --space-fixup ?
>>
> 
> What do you mean with "partitions built with --space-fixup"? If you mean
> ubi *images* built with --space-fixup, these will still work fine.
> Otherwise, could you please clarify?

I'm just curious whether we're changing behavior here, which might pose
a problem. I am also not super fond of hard-coding things this way, but
maybe this is fine in this case ?

[GIT] Pull request: u-boot-dfu (26.01.2020)

[Guillermo Rodriguez]

El lun., 27 ene. 2020 a las 14:51, Marek Vasut (<marex@denx.de>) escribió:
>
> On 1/27/20 2:35 PM, Guillermo Rodriguez wrote:
> > El lunes, 27 de enero de 2020, Marek Vasut <marex@denx.de> escribió:
> >
> >> On 1/27/20 2:26 PM, Guillermo Rodriguez wrote:
> >>> Hi,
> >>
> >> Hi,
> >>
> >>> El lunes, 27 de enero de 2020, Marek Vasut <marex@denx.de> escribió:
> >>>
> >>>> On 1/27/20 9:14 AM, Guillermo Rodriguez Garcia wrote:
> >>>>> Hi Marek,
> >>>>
> >>>> Hi,
> >>>>
> >>>>> El lun., 27 ene. 2020 a las 8:52, Lukasz Majewski (<lukma@denx.de>)
> >>>> escribió:
> >>>>>>
> >>>>>> Hi Marek,
> >>>>>>
> >>>>>>> On 1/26/20 9:26 PM, Lukasz Majewski wrote:
> >>>>>>> Hi,
> >>>>>>>
> >>>>>>> [...]
> >>>>>>>
> >>>>>>>> ----------------------------------------------------------------
> >>>>>>>> Guillermo Rodríguez (1):
> >>>>>>>>       dfu: Add option to skip empty pages when flashing UBI images
> >>>>>>>> to
> >>>>>>>
> >>>>>>> Can that option be enabled/disabled at runtime instead of being
> >>>>>>> hardcoded?
> >>>>>>
> >>>>>> It has been designed in a similar way to Android's existing
> >>>>>> FASTBOOT_FLASH_NAND_TRIMFFS option.
> >>>>>
> >>>>> Without this option, UBI images need to be built with --space-fixup
> >>>>> [1] so that the kernel can "fix" the NAND on first mount.
> >>>>> When this option is used, --space-fixup is no longer necessary because
> >>>>> dfu knows how to correctly flash UBI images. However, UBI images built
> >>>>> with --space-fixup will still work fine.
> >>>>
> >>>> Does NAND.TRIMFFS preserve UBI erase counters in the NAND ? I don't
> >>>> think so, so I don't think "correctly flash UBI images" is the correct
> >>>> formulation here.
> >>>>
> >>>> Yes, you are right.
> >>>
> >>>
> >>>>> In other words, enabling this option at buildtime has no
> >> countereffects.
> >>>>> So there is no point in making it configurable at runtime, if support
> >>>>> has been built into U-boot.
> >>>>>
> >>>>>  [1]: http://www.linux-mtd.infradead.org/faq/ubifs.html#
> >>>> L_free_space_fixup
> >>>>
> >>>> So what if I want to write raw NAND image without "trimffs" on such a
> >>>> system via DFU, e.g. a bootloader ? How can I do that ?
> >>>>
> >>>
> >>> You mean on a non-ubi partition? Then you don't need to do anything
> >>> special. This code is only triggered for ubi partitions.
> >>
> >> And what about partitions which were already built with the --space-fixup ?
> >>
> >
> > What do you mean with "partitions built with --space-fixup"? If you mean
> > ubi *images* built with --space-fixup, these will still work fine.
> > Otherwise, could you please clarify?
>
> I'm just curious whether we're changing behavior here, which might pose
> a problem. I am also not super fond of hard-coding things this way, but
> maybe this is fine in this case ?

OK, let me try to summarize what we're doing here.

The recommended way to flash UBI images is using ubiformat [1].
When this is not possible, then the UBI FAQ describes how UBI images
should be flashed to NAND [2]. Basically you need to make sure that
any trailing all-0xFF pages in each eraseblock are dropped (i.e. not
written to flash). This is what "nand write.trimffs" does (see
description in [3])
If you cannot do that, then an alternative is to make sure that the
UBI image is built with --space-fixup [4]; this sets a flag so that
the Linux kernel will "fix" the NAND on first mount. This is somewhat
expensive, though, as the kernel may need to rewrite several PEBs.

In the current u-boot, dfu does not follow the procedure stated in the
UBI FAQ. This forces you to make sure UBI images are always built with
--space-fixup.
With this patch, when dfu writes to a UBI partition it will use the
procedure outlined in the UBI FAQ. So --space-fixup is no longer
required.

Two important things to note here:
- This only makes a difference for UBI partitions (see check in
dfu_nand [5]). Behaviour for non-UBI partitions does not change.
- Images built with --space-fixup will continue to work just fine (the
kernel will do just its stuff the first time the file system is
mounted. This is unnecessary, but also harmless).

So, to summarize:
 1. Without the patch dfu only works if --space-fixup was used when
the UBI image was generated; with the patch it will work for images
with and without --space-fixup
 2. Nothing changes for non-UBI partitions

 [1]: http://www.linux-mtd.infradead.org/faq/ubifs.html#L_why_ubiformat
 [2]: http://www.linux-mtd.infradead.org/doc/ubi.html#L_flasher_algo
 [3]: https://gitlab.com/u-boot/u-boot/blob/master/doc/README.nand#L65
 [4]: http://www.linux-mtd.infradead.org/faq/ubifs.html#L_free_space_fixup
 [5]: https://gitlab.com/u-boot/u-boot/blob/master/drivers/dfu/dfu_nand.c#L231

Let me know if you have any other questions.

Thanks,

Guillermo

[GIT] Pull request: u-boot-dfu (26.01.2020)

[Tom Rini]

On Mon, Jan 27, 2020 at 04:00:28PM +0100, Guillermo Rodriguez wrote:
> El lun., 27 ene. 2020 a las 14:51, Marek Vasut (<marex@denx.de>) escribió:
> >
> > On 1/27/20 2:35 PM, Guillermo Rodriguez wrote:
> > > El lunes, 27 de enero de 2020, Marek Vasut <marex@denx.de> escribió:
> > >
> > >> On 1/27/20 2:26 PM, Guillermo Rodriguez wrote:
> > >>> Hi,
> > >>
> > >> Hi,
> > >>
> > >>> El lunes, 27 de enero de 2020, Marek Vasut <marex@denx.de> escribió:
> > >>>
> > >>>> On 1/27/20 9:14 AM, Guillermo Rodriguez Garcia wrote:
> > >>>>> Hi Marek,
> > >>>>
> > >>>> Hi,
> > >>>>
> > >>>>> El lun., 27 ene. 2020 a las 8:52, Lukasz Majewski (<lukma@denx.de>)
> > >>>> escribió:
> > >>>>>>
> > >>>>>> Hi Marek,
> > >>>>>>
> > >>>>>>> On 1/26/20 9:26 PM, Lukasz Majewski wrote:
> > >>>>>>> Hi,
> > >>>>>>>
> > >>>>>>> [...]
> > >>>>>>>
> > >>>>>>>> ----------------------------------------------------------------
> > >>>>>>>> Guillermo Rodríguez (1):
> > >>>>>>>>       dfu: Add option to skip empty pages when flashing UBI images
> > >>>>>>>> to
> > >>>>>>>
> > >>>>>>> Can that option be enabled/disabled at runtime instead of being
> > >>>>>>> hardcoded?
> > >>>>>>
> > >>>>>> It has been designed in a similar way to Android's existing
> > >>>>>> FASTBOOT_FLASH_NAND_TRIMFFS option.
> > >>>>>
> > >>>>> Without this option, UBI images need to be built with --space-fixup
> > >>>>> [1] so that the kernel can "fix" the NAND on first mount.
> > >>>>> When this option is used, --space-fixup is no longer necessary because
> > >>>>> dfu knows how to correctly flash UBI images. However, UBI images built
> > >>>>> with --space-fixup will still work fine.
> > >>>>
> > >>>> Does NAND.TRIMFFS preserve UBI erase counters in the NAND ? I don't
> > >>>> think so, so I don't think "correctly flash UBI images" is the correct
> > >>>> formulation here.
> > >>>>
> > >>>> Yes, you are right.
> > >>>
> > >>>
> > >>>>> In other words, enabling this option at buildtime has no
> > >> countereffects.
> > >>>>> So there is no point in making it configurable at runtime, if support
> > >>>>> has been built into U-boot.
> > >>>>>
> > >>>>>  [1]: http://www.linux-mtd.infradead.org/faq/ubifs.html#
> > >>>> L_free_space_fixup
> > >>>>
> > >>>> So what if I want to write raw NAND image without "trimffs" on such a
> > >>>> system via DFU, e.g. a bootloader ? How can I do that ?
> > >>>>
> > >>>
> > >>> You mean on a non-ubi partition? Then you don't need to do anything
> > >>> special. This code is only triggered for ubi partitions.
> > >>
> > >> And what about partitions which were already built with the --space-fixup ?
> > >>
> > >
> > > What do you mean with "partitions built with --space-fixup"? If you mean
> > > ubi *images* built with --space-fixup, these will still work fine.
> > > Otherwise, could you please clarify?
> >
> > I'm just curious whether we're changing behavior here, which might pose
> > a problem. I am also not super fond of hard-coding things this way, but
> > maybe this is fine in this case ?
> 
> OK, let me try to summarize what we're doing here.
> 
> The recommended way to flash UBI images is using ubiformat [1].
> When this is not possible, then the UBI FAQ describes how UBI images
> should be flashed to NAND [2]. Basically you need to make sure that
> any trailing all-0xFF pages in each eraseblock are dropped (i.e. not
> written to flash). This is what "nand write.trimffs" does (see
> description in [3])
> If you cannot do that, then an alternative is to make sure that the
> UBI image is built with --space-fixup [4]; this sets a flag so that
> the Linux kernel will "fix" the NAND on first mount. This is somewhat
> expensive, though, as the kernel may need to rewrite several PEBs.
> 
> In the current u-boot, dfu does not follow the procedure stated in the
> UBI FAQ. This forces you to make sure UBI images are always built with
> --space-fixup.
> With this patch, when dfu writes to a UBI partition it will use the
> procedure outlined in the UBI FAQ. So --space-fixup is no longer
> required.
> 
> Two important things to note here:
> - This only makes a difference for UBI partitions (see check in
> dfu_nand [5]). Behaviour for non-UBI partitions does not change.
> - Images built with --space-fixup will continue to work just fine (the
> kernel will do just its stuff the first time the file system is
> mounted. This is unnecessary, but also harmless).
> 
> So, to summarize:
>  1. Without the patch dfu only works if --space-fixup was used when
> the UBI image was generated; with the patch it will work for images
> with and without --space-fixup
>  2. Nothing changes for non-UBI partitions
> 
>  [1]: http://www.linux-mtd.infradead.org/faq/ubifs.html#L_why_ubiformat
>  [2]: http://www.linux-mtd.infradead.org/doc/ubi.html#L_flasher_algo
>  [3]: https://gitlab.com/u-boot/u-boot/blob/master/doc/README.nand#L65
>  [4]: http://www.linux-mtd.infradead.org/faq/ubifs.html#L_free_space_fixup
>  [5]: https://gitlab.com/u-boot/u-boot/blob/master/drivers/dfu/dfu_nand.c#L231
> 
> Let me know if you have any other questions.

As I've just been reading along, thanks for the detailed summary of the
issues!

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <https://lists.denx.de/pipermail/u-boot/attachments/20200127/5ad9edc9/attachment.sig>

[GIT] Pull request: u-boot-dfu (26.01.2020)

[Marek Vasut]

On 1/27/20 4:00 PM, Guillermo Rodriguez wrote:
> El lun., 27 ene. 2020 a las 14:51, Marek Vasut (<marex@denx.de>) escribió:
>>
>> On 1/27/20 2:35 PM, Guillermo Rodriguez wrote:
>>> El lunes, 27 de enero de 2020, Marek Vasut <marex@denx.de> escribió:
>>>
>>>> On 1/27/20 2:26 PM, Guillermo Rodriguez wrote:
>>>>> Hi,
>>>>
>>>> Hi,
>>>>
>>>>> El lunes, 27 de enero de 2020, Marek Vasut <marex@denx.de> escribió:
>>>>>
>>>>>> On 1/27/20 9:14 AM, Guillermo Rodriguez Garcia wrote:
>>>>>>> Hi Marek,
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>>> El lun., 27 ene. 2020 a las 8:52, Lukasz Majewski (<lukma@denx.de>)
>>>>>> escribió:
>>>>>>>>
>>>>>>>> Hi Marek,
>>>>>>>>
>>>>>>>>> On 1/26/20 9:26 PM, Lukasz Majewski wrote:
>>>>>>>>> Hi,
>>>>>>>>>
>>>>>>>>> [...]
>>>>>>>>>
>>>>>>>>>> ----------------------------------------------------------------
>>>>>>>>>> Guillermo Rodríguez (1):
>>>>>>>>>>       dfu: Add option to skip empty pages when flashing UBI images
>>>>>>>>>> to
>>>>>>>>>
>>>>>>>>> Can that option be enabled/disabled at runtime instead of being
>>>>>>>>> hardcoded?
>>>>>>>>
>>>>>>>> It has been designed in a similar way to Android's existing
>>>>>>>> FASTBOOT_FLASH_NAND_TRIMFFS option.
>>>>>>>
>>>>>>> Without this option, UBI images need to be built with --space-fixup
>>>>>>> [1] so that the kernel can "fix" the NAND on first mount.
>>>>>>> When this option is used, --space-fixup is no longer necessary because
>>>>>>> dfu knows how to correctly flash UBI images. However, UBI images built
>>>>>>> with --space-fixup will still work fine.
>>>>>>
>>>>>> Does NAND.TRIMFFS preserve UBI erase counters in the NAND ? I don't
>>>>>> think so, so I don't think "correctly flash UBI images" is the correct
>>>>>> formulation here.
>>>>>>
>>>>>> Yes, you are right.
>>>>>
>>>>>
>>>>>>> In other words, enabling this option at buildtime has no
>>>> countereffects.
>>>>>>> So there is no point in making it configurable at runtime, if support
>>>>>>> has been built into U-boot.
>>>>>>>
>>>>>>>  [1]: http://www.linux-mtd.infradead.org/faq/ubifs.html#
>>>>>> L_free_space_fixup
>>>>>>
>>>>>> So what if I want to write raw NAND image without "trimffs" on such a
>>>>>> system via DFU, e.g. a bootloader ? How can I do that ?
>>>>>>
>>>>>
>>>>> You mean on a non-ubi partition? Then you don't need to do anything
>>>>> special. This code is only triggered for ubi partitions.
>>>>
>>>> And what about partitions which were already built with the --space-fixup ?
>>>>
>>>
>>> What do you mean with "partitions built with --space-fixup"? If you mean
>>> ubi *images* built with --space-fixup, these will still work fine.
>>> Otherwise, could you please clarify?
>>
>> I'm just curious whether we're changing behavior here, which might pose
>> a problem. I am also not super fond of hard-coding things this way, but
>> maybe this is fine in this case ?
> 
> OK, let me try to summarize what we're doing here.
> 
> The recommended way to flash UBI images is using ubiformat [1].
> When this is not possible, then the UBI FAQ describes how UBI images
> should be flashed to NAND [2]. Basically you need to make sure that
> any trailing all-0xFF pages in each eraseblock are dropped (i.e. not
> written to flash). This is what "nand write.trimffs" does (see
> description in [3])
> If you cannot do that, then an alternative is to make sure that the
> UBI image is built with --space-fixup [4]; this sets a flag so that
> the Linux kernel will "fix" the NAND on first mount. This is somewhat
> expensive, though, as the kernel may need to rewrite several PEBs.
> 
> In the current u-boot, dfu does not follow the procedure stated in the
> UBI FAQ. This forces you to make sure UBI images are always built with
> --space-fixup.
> With this patch, when dfu writes to a UBI partition it will use the
> procedure outlined in the UBI FAQ. So --space-fixup is no longer
> required.
> 
> Two important things to note here:
> - This only makes a difference for UBI partitions (see check in
> dfu_nand [5]). Behaviour for non-UBI partitions does not change.
> - Images built with --space-fixup will continue to work just fine (the
> kernel will do just its stuff the first time the file system is
> mounted. This is unnecessary, but also harmless).
> 
> So, to summarize:
>  1. Without the patch dfu only works if --space-fixup was used when
> the UBI image was generated; with the patch it will work for images
> with and without --space-fixup
>  2. Nothing changes for non-UBI partitions
> 
>  [1]: http://www.linux-mtd.infradead.org/faq/ubifs.html#L_why_ubiformat
>  [2]: http://www.linux-mtd.infradead.org/doc/ubi.html#L_flasher_algo
>  [3]: https://gitlab.com/u-boot/u-boot/blob/master/doc/README.nand#L65
>  [4]: http://www.linux-mtd.infradead.org/faq/ubifs.html#L_free_space_fixup
>  [5]: https://gitlab.com/u-boot/u-boot/blob/master/drivers/dfu/dfu_nand.c#L231
> 
> Let me know if you have any other questions.

I think it's OK. I just wanted to be sure we're not breaking anything
there. Thanks!

Applied.