[RFC] Raising the UBI version

[RFC] Raising the UBI version

[Richard Weinberger]

Dear MTD folks,

For the emerging MLC NAND support we need to change the UBI on-flash format.
Of course existing UBI images will keep working and remain fully supported.
Our approach to deal with MLC (and basically TLC) NAND is LEB consolidation.
In this operation mode a single PEB can host multiple LEBs. In the MLC case 2,
for TLC 3. For more details please refer to my announcement[0].
Hosting multiple LEBs in a single PEB means that beside of a single EC header,
a PEB will carry multiple VID headers, one for each LEB it contains.
This change needs to be annotated in the EC header.

Both EC and VID headers have a version field. Currently it is set to 1. Our
original plan was just raising UBI_VERSION to 2, and, of course, accept
version 1 image as well. The first hassle was that UBI_VERSION is exported
in /sys/class/ubi/version and libubi refuses to work if the version is not 1.
Breaking existing userspace tools is not acceptable, so we need another
approach.

LEB consolidation is not really a completely new UBI implementation, it is
an addon feature. So we came up with the idea of having feature flags in
the EC header. Maybe we need later more flags, who knows?

Boris and I sat down and came up with two possible ways to implement such
flags:

i) Rename ->version in EC and VID headers to ->features. ->features will
   be evaluated at attach time and UBI has to figure whether it supports
   all request features. The field is one byte long, therefore we can encode
   8 features.
   As starter two features would be supported:
      UBI_FEAT_BASE	= 1
      UBI_FEAT_CONSO	= 2
   That means regular UBI images on SLC would only have set UBI_FEAT_BASE and
   nothing else. Existing UBI implementations would see ->features with
   UBI_FEAT_BASE set as ->version = 1, so we're safe. On MLC NAND we'd set
   UBI_FEAT_BASE and UBI_FEAT_CONSO which would be seen as ->version = 3 and
   rejected by UBI implementations which do not support LEB consolidation.
   To not break userspace tools /sys/class/ubi/version would be hardcoded to 1
   and the ->features field exported in /sys/class/ubi/features and
   /sys/class/ubi/ubiX/features_used. The features sysfs file denotes what
   features this UBI implementation supports and features_used shows what
   features the attached UBI image requested. If we change the UBI on-flash
   format in a major way, UBI_FEAT_BASE would not be set.

ii) Keep ->version in EC and VID headers and use padding bytes from both headers
    to add a new ->features field. If ->version is 1, ->features will remain 0
    and not evaluated. If ->features should be evaluated, ->version will be 2.
    So, on MLC NAND ->version will be 2 and ->features has UBI_FEAT_CONSO set.
    This approach is less complicated but we have to claim padding bytes.
    Of course we also have to hardcode /sys/class/ubi/version to 1 too and having
    a features file in sysfs.

That said, we'd keep ->version and ->features between EC and VID headers in
sync. Supporting different versions of EC and VID headers at the same time
would complicate the code a lot.

Another thing to think about is storing the number of LEBs a PEB can contain
in the EC header. Usually this would be determined at runtime but maybe it
makes sense to store it also on-flash to detect misconfiguration,
like we do already for VID offset in the EC header.

What do you think? I'm happy with either i) or ii), the biggest concern we have
is that we cut something into stone and break stuff or make trouble unavoidable.

Thanks,
//richard

[0] http://lists.infradead.org/pipermail/linux-mtd/2016-April/067322.html

Re: [RFC] Raising the UBI version

[Boris Brezillon]

On Tue, 21 Jun 2016 21:19:50 +0200
Richard Weinberger <richard@nod.at> wrote:

> Dear MTD folks,
> 
> For the emerging MLC NAND support we need to change the UBI on-flash format.
> Of course existing UBI images will keep working and remain fully supported.
> Our approach to deal with MLC (and basically TLC) NAND is LEB consolidation.
> In this operation mode a single PEB can host multiple LEBs. In the MLC case 2,
> for TLC 3. For more details please refer to my announcement[0].
> Hosting multiple LEBs in a single PEB means that beside of a single EC header,
> a PEB will carry multiple VID headers, one for each LEB it contains.
> This change needs to be annotated in the EC header.
> 
> Both EC and VID headers have a version field. Currently it is set to 1. Our
> original plan was just raising UBI_VERSION to 2, and, of course, accept
> version 1 image as well. The first hassle was that UBI_VERSION is exported
> in /sys/class/ubi/version and libubi refuses to work if the version is not 1.
> Breaking existing userspace tools is not acceptable, so we need another
> approach.
> 
> LEB consolidation is not really a completely new UBI implementation, it is
> an addon feature. So we came up with the idea of having feature flags in
> the EC header. Maybe we need later more flags, who knows?
> 
> Boris and I sat down and came up with two possible ways to implement such
> flags:
> 
> i) Rename ->version in EC and VID headers to ->features. ->features will
>    be evaluated at attach time and UBI has to figure whether it supports
>    all request features. The field is one byte long, therefore we can encode
>    8 features.
>    As starter two features would be supported:
>       UBI_FEAT_BASE	= 1
>       UBI_FEAT_CONSO	= 2
>    That means regular UBI images on SLC would only have set UBI_FEAT_BASE and
>    nothing else. Existing UBI implementations would see ->features with
>    UBI_FEAT_BASE set as ->version = 1, so we're safe. On MLC NAND we'd set
>    UBI_FEAT_BASE and UBI_FEAT_CONSO which would be seen as ->version = 3 and
>    rejected by UBI implementations which do not support LEB consolidation.
>    To not break userspace tools /sys/class/ubi/version would be hardcoded to 1
>    and the ->features field exported in /sys/class/ubi/features and
>    /sys/class/ubi/ubiX/features_used. The features sysfs file denotes what
>    features this UBI implementation supports and features_used shows what
>    features the attached UBI image requested. If we change the UBI on-flash
>    format in a major way, UBI_FEAT_BASE would not be set.
> 
> ii) Keep ->version in EC and VID headers and use padding bytes from both headers
>     to add a new ->features field. If ->version is 1, ->features will remain 0
>     and not evaluated. If ->features should be evaluated, ->version will be 2.
>     So, on MLC NAND ->version will be 2 and ->features has UBI_FEAT_CONSO set.
>     This approach is less complicated but we have to claim padding bytes.
>     Of course we also have to hardcode /sys/class/ubi/version to 1 too and having
>     a features file in sysfs.

Why do we need to hardcode /sys/class/ubi/version to 1? We just need to
update the mtd-utils to support version 2. Am I missing something?

> 
> That said, we'd keep ->version and ->features between EC and VID headers in
> sync. Supporting different versions of EC and VID headers at the same time
> would complicate the code a lot.
> 
> Another thing to think about is storing the number of LEBs a PEB can contain
> in the EC header. Usually this would be determined at runtime but maybe it
> makes sense to store it also on-flash to detect misconfiguration,
> like we do already for VID offset in the EC header.
> 
> What do you think? I'm happy with either i) or ii), the biggest concern we have
> is that we cut something into stone and break stuff or make trouble unavoidable.
> 
> Thanks,
> //richard
> 
> [0] http://lists.infradead.org/pipermail/linux-mtd/2016-April/067322.html

Re: [RFC] Raising the UBI version

[Michal Suchanek]

On 22 June 2016 at 14:43, Boris Brezillon
<boris.brezillon@free-electrons.com> wrote:
> On Tue, 21 Jun 2016 21:19:50 +0200
> Richard Weinberger <richard@nod.at> wrote:
>
>> Dear MTD folks,
>>
>> For the emerging MLC NAND support we need to change the UBI on-flash format.
>> Of course existing UBI images will keep working and remain fully supported.
>> Our approach to deal with MLC (and basically TLC) NAND is LEB consolidation.
>> In this operation mode a single PEB can host multiple LEBs. In the MLC case 2,
>> for TLC 3. For more details please refer to my announcement[0].
>> Hosting multiple LEBs in a single PEB means that beside of a single EC header,
>> a PEB will carry multiple VID headers, one for each LEB it contains.
>> This change needs to be annotated in the EC header.
>>
>> Both EC and VID headers have a version field. Currently it is set to 1. Our
>> original plan was just raising UBI_VERSION to 2, and, of course, accept
>> version 1 image as well. The first hassle was that UBI_VERSION is exported
>> in /sys/class/ubi/version and libubi refuses to work if the version is not 1.
>> Breaking existing userspace tools is not acceptable, so we need another
>> approach.
>>
>> LEB consolidation is not really a completely new UBI implementation, it is
>> an addon feature. So we came up with the idea of having feature flags in
>> the EC header. Maybe we need later more flags, who knows?
>>
>> Boris and I sat down and came up with two possible ways to implement such
>> flags:
>>
>> i) Rename ->version in EC and VID headers to ->features. ->features will
>>    be evaluated at attach time and UBI has to figure whether it supports
>>    all request features. The field is one byte long, therefore we can encode
>>    8 features.
>>    As starter two features would be supported:
>>       UBI_FEAT_BASE   = 1
>>       UBI_FEAT_CONSO  = 2
>>    That means regular UBI images on SLC would only have set UBI_FEAT_BASE and
>>    nothing else. Existing UBI implementations would see ->features with
>>    UBI_FEAT_BASE set as ->version = 1, so we're safe. On MLC NAND we'd set
>>    UBI_FEAT_BASE and UBI_FEAT_CONSO which would be seen as ->version = 3 and
>>    rejected by UBI implementations which do not support LEB consolidation.
>>    To not break userspace tools /sys/class/ubi/version would be hardcoded to 1
>>    and the ->features field exported in /sys/class/ubi/features and
>>    /sys/class/ubi/ubiX/features_used. The features sysfs file denotes what
>>    features this UBI implementation supports and features_used shows what
>>    features the attached UBI image requested. If we change the UBI on-flash
>>    format in a major way, UBI_FEAT_BASE would not be set.
>>
>> ii) Keep ->version in EC and VID headers and use padding bytes from both headers
>>     to add a new ->features field. If ->version is 1, ->features will remain 0
>>     and not evaluated. If ->features should be evaluated, ->version will be 2.
>>     So, on MLC NAND ->version will be 2 and ->features has UBI_FEAT_CONSO set.
>>     This approach is less complicated but we have to claim padding bytes.
>>     Of course we also have to hardcode /sys/class/ubi/version to 1 too and having
>>     a features file in sysfs.
>
> Why do we need to hardcode /sys/class/ubi/version to 1? We just need to
> update the mtd-utils to support version 2. Am I missing something?

Is some code change required in mtd-utils other than the change in
version check?

If not why force everyone to patch their mtd-utils to support doing
the same thing when the file reads back 2 instead of 1?

Thanks

Michal

Re: [RFC] Raising the UBI version

[Boris Brezillon]

On Wed, 22 Jun 2016 15:09:59 +0200
Michal Suchanek <hramrach@gmail.com> wrote:

> On 22 June 2016 at 14:43, Boris Brezillon
> <boris.brezillon@free-electrons.com> wrote:
> > On Tue, 21 Jun 2016 21:19:50 +0200
> > Richard Weinberger <richard@nod.at> wrote:
> >  
> >> Dear MTD folks,
> >>
> >> For the emerging MLC NAND support we need to change the UBI on-flash format.
> >> Of course existing UBI images will keep working and remain fully supported.
> >> Our approach to deal with MLC (and basically TLC) NAND is LEB consolidation.
> >> In this operation mode a single PEB can host multiple LEBs. In the MLC case 2,
> >> for TLC 3. For more details please refer to my announcement[0].
> >> Hosting multiple LEBs in a single PEB means that beside of a single EC header,
> >> a PEB will carry multiple VID headers, one for each LEB it contains.
> >> This change needs to be annotated in the EC header.
> >>
> >> Both EC and VID headers have a version field. Currently it is set to 1. Our
> >> original plan was just raising UBI_VERSION to 2, and, of course, accept
> >> version 1 image as well. The first hassle was that UBI_VERSION is exported
> >> in /sys/class/ubi/version and libubi refuses to work if the version is not 1.
> >> Breaking existing userspace tools is not acceptable, so we need another
> >> approach.
> >>
> >> LEB consolidation is not really a completely new UBI implementation, it is
> >> an addon feature. So we came up with the idea of having feature flags in
> >> the EC header. Maybe we need later more flags, who knows?
> >>
> >> Boris and I sat down and came up with two possible ways to implement such
> >> flags:
> >>
> >> i) Rename ->version in EC and VID headers to ->features. ->features will
> >>    be evaluated at attach time and UBI has to figure whether it supports
> >>    all request features. The field is one byte long, therefore we can encode
> >>    8 features.
> >>    As starter two features would be supported:
> >>       UBI_FEAT_BASE   = 1
> >>       UBI_FEAT_CONSO  = 2
> >>    That means regular UBI images on SLC would only have set UBI_FEAT_BASE and
> >>    nothing else. Existing UBI implementations would see ->features with
> >>    UBI_FEAT_BASE set as ->version = 1, so we're safe. On MLC NAND we'd set
> >>    UBI_FEAT_BASE and UBI_FEAT_CONSO which would be seen as ->version = 3 and
> >>    rejected by UBI implementations which do not support LEB consolidation.
> >>    To not break userspace tools /sys/class/ubi/version would be hardcoded to 1
> >>    and the ->features field exported in /sys/class/ubi/features and
> >>    /sys/class/ubi/ubiX/features_used. The features sysfs file denotes what
> >>    features this UBI implementation supports and features_used shows what
> >>    features the attached UBI image requested. If we change the UBI on-flash
> >>    format in a major way, UBI_FEAT_BASE would not be set.
> >>
> >> ii) Keep ->version in EC and VID headers and use padding bytes from both headers
> >>     to add a new ->features field. If ->version is 1, ->features will remain 0
> >>     and not evaluated. If ->features should be evaluated, ->version will be 2.
> >>     So, on MLC NAND ->version will be 2 and ->features has UBI_FEAT_CONSO set.
> >>     This approach is less complicated but we have to claim padding bytes.
> >>     Of course we also have to hardcode /sys/class/ubi/version to 1 too and having
> >>     a features file in sysfs.  
> >
> > Why do we need to hardcode /sys/class/ubi/version to 1? We just need to
> > update the mtd-utils to support version 2. Am I missing something?  
> 
> Is some code change required in mtd-utils other than the change in
> version check?
> 
> If not why force everyone to patch their mtd-utils to support doing
> the same thing when the file reads back 2 instead of 1?

Old UBI users will keep exposing version 1. Version 2 will only be used
for those wanting to enable support for UBI consolidation, so old
mtd-utils should keep working, unless the user explicitly tried to
create a consolidated UBI image, or attached the UBI layer to an empty
partition on an MLC NAND after enabling consolidation support. That's
rather unlikely, and if it happens it's the end-user fault.

Re: [RFC] Raising the UBI version

[Boris Brezillon]

On Wed, 22 Jun 2016 15:17:32 +0200
Boris Brezillon <boris.brezillon@free-electrons.com> wrote:

> On Wed, 22 Jun 2016 15:09:59 +0200
> Michal Suchanek <hramrach@gmail.com> wrote:
> 
> > On 22 June 2016 at 14:43, Boris Brezillon
> > <boris.brezillon@free-electrons.com> wrote:  
> > > On Tue, 21 Jun 2016 21:19:50 +0200
> > > Richard Weinberger <richard@nod.at> wrote:
> > >    
> > >> Dear MTD folks,
> > >>
> > >> For the emerging MLC NAND support we need to change the UBI on-flash format.
> > >> Of course existing UBI images will keep working and remain fully supported.
> > >> Our approach to deal with MLC (and basically TLC) NAND is LEB consolidation.
> > >> In this operation mode a single PEB can host multiple LEBs. In the MLC case 2,
> > >> for TLC 3. For more details please refer to my announcement[0].
> > >> Hosting multiple LEBs in a single PEB means that beside of a single EC header,
> > >> a PEB will carry multiple VID headers, one for each LEB it contains.
> > >> This change needs to be annotated in the EC header.
> > >>
> > >> Both EC and VID headers have a version field. Currently it is set to 1. Our
> > >> original plan was just raising UBI_VERSION to 2, and, of course, accept
> > >> version 1 image as well. The first hassle was that UBI_VERSION is exported
> > >> in /sys/class/ubi/version and libubi refuses to work if the version is not 1.
> > >> Breaking existing userspace tools is not acceptable, so we need another
> > >> approach.
> > >>
> > >> LEB consolidation is not really a completely new UBI implementation, it is
> > >> an addon feature. So we came up with the idea of having feature flags in
> > >> the EC header. Maybe we need later more flags, who knows?
> > >>
> > >> Boris and I sat down and came up with two possible ways to implement such
> > >> flags:
> > >>
> > >> i) Rename ->version in EC and VID headers to ->features. ->features will
> > >>    be evaluated at attach time and UBI has to figure whether it supports
> > >>    all request features. The field is one byte long, therefore we can encode
> > >>    8 features.
> > >>    As starter two features would be supported:
> > >>       UBI_FEAT_BASE   = 1
> > >>       UBI_FEAT_CONSO  = 2
> > >>    That means regular UBI images on SLC would only have set UBI_FEAT_BASE and
> > >>    nothing else. Existing UBI implementations would see ->features with
> > >>    UBI_FEAT_BASE set as ->version = 1, so we're safe. On MLC NAND we'd set
> > >>    UBI_FEAT_BASE and UBI_FEAT_CONSO which would be seen as ->version = 3 and
> > >>    rejected by UBI implementations which do not support LEB consolidation.
> > >>    To not break userspace tools /sys/class/ubi/version would be hardcoded to 1
> > >>    and the ->features field exported in /sys/class/ubi/features and
> > >>    /sys/class/ubi/ubiX/features_used. The features sysfs file denotes what
> > >>    features this UBI implementation supports and features_used shows what
> > >>    features the attached UBI image requested. If we change the UBI on-flash
> > >>    format in a major way, UBI_FEAT_BASE would not be set.
> > >>
> > >> ii) Keep ->version in EC and VID headers and use padding bytes from both headers
> > >>     to add a new ->features field. If ->version is 1, ->features will remain 0
> > >>     and not evaluated. If ->features should be evaluated, ->version will be 2.
> > >>     So, on MLC NAND ->version will be 2 and ->features has UBI_FEAT_CONSO set.
> > >>     This approach is less complicated but we have to claim padding bytes.
> > >>     Of course we also have to hardcode /sys/class/ubi/version to 1 too and having
> > >>     a features file in sysfs.    
> > >
> > > Why do we need to hardcode /sys/class/ubi/version to 1? We just need to
> > > update the mtd-utils to support version 2. Am I missing something?    
> > 
> > Is some code change required in mtd-utils other than the change in
> > version check?

Sorry, forgot to answer this question. Yes, at least ubiformat will
change. The ubimk/rm/rs/...vol should not change.

> > 
> > If not why force everyone to patch their mtd-utils to support doing
> > the same thing when the file reads back 2 instead of 1?  
> 
> Old UBI users will keep exposing version 1. Version 2 will only be used
> for those wanting to enable support for UBI consolidation, so old
> mtd-utils should keep working, unless the user explicitly tried to
> create a consolidated UBI image, or attached the UBI layer to an empty
> partition on an MLC NAND after enabling consolidation support. That's
> rather unlikely, and if it happens it's the end-user fault.
> 
> 

Re: [RFC] Raising the UBI version

[Richard Weinberger]

Am 22.06.2016 um 14:43 schrieb Boris Brezillon:
> Why do we need to hardcode /sys/class/ubi/version to 1? We just need to
> update the mtd-utils to support version 2. Am I missing something?

We don't want to break existing userspace.
Why should ubimkvol or ubiattach fail on a system with SLC NAND and
CONFIG_MTD_UBI_CONSOLIDATE=y?

Especially since existing tools *will* work with CONFIG_MTD_UBI_CONSOLIDATE=y.
Rasing /sys/class/ubi/version and breaking existing tools is only acceptable
when we change all UBI ioctl() and sysfs files in a way such that version 1
userspace cannot work. Which is not the case here.

This is a nice example why version numbers is bad and feature flags should be used.
Currently UBI mixes the implementation version and the on-flash version.
We're changing only the on-flash version. The user visible ABI stays and will only
get extended.

Thanks,
//richard

Re: [RFC] Raising the UBI version

[Boris Brezillon]

On Wed, 22 Jun 2016 15:54:34 +0200
Richard Weinberger <richard@nod.at> wrote:

> Am 22.06.2016 um 14:43 schrieb Boris Brezillon:
> > Why do we need to hardcode /sys/class/ubi/version to 1? We just need to
> > update the mtd-utils to support version 2. Am I missing something?  
> 
> We don't want to break existing userspace.
> Why should ubimkvol or ubiattach fail on a system with SLC NAND and
> CONFIG_MTD_UBI_CONSOLIDATE=y?

But version won't be set to 2 in this case. If it's an SLC NAND we
don't need to use UBI version 2, do we.

> 
> Especially since existing tools *will* work with CONFIG_MTD_UBI_CONSOLIDATE=y.
> Rasing /sys/class/ubi/version and breaking existing tools is only acceptable
> when we change all UBI ioctl() and sysfs files in a way such that version 1
> userspace cannot work. Which is not the case here.
> 
> This is a nice example why version numbers is bad and feature flags should be used.
> Currently UBI mixes the implementation version and the on-flash version.
> We're changing only the on-flash version. The user visible ABI stays and will only
> get extended.

Correct. So /sys/class/ubi/version is representing the user-space ABI
version, right? Maybe we should expose the on-flash version in a
different file then.

Re: [RFC] Raising the UBI version

[Richard Weinberger]

Am 22.06.2016 um 16:01 schrieb Boris Brezillon:
> On Wed, 22 Jun 2016 15:54:34 +0200
> Richard Weinberger <richard@nod.at> wrote:
> 
>> Am 22.06.2016 um 14:43 schrieb Boris Brezillon:
>>> Why do we need to hardcode /sys/class/ubi/version to 1? We just need to
>>> update the mtd-utils to support version 2. Am I missing something?  
>>
>> We don't want to break existing userspace.
>> Why should ubimkvol or ubiattach fail on a system with SLC NAND and
>> CONFIG_MTD_UBI_CONSOLIDATE=y?
> 
> But version won't be set to 2 in this case. If it's an SLC NAND we
> don't need to use UBI version 2, do we.

/sys/class/ubi/version is the version of the UBI implementation,
not the version of the attached UBI image.
It will the here as soon you load the UBI module.

Thanks,
//richard

Re: [RFC] Raising the UBI version

[Boris Brezillon]

On Wed, 22 Jun 2016 16:05:17 +0200
Richard Weinberger <richard@nod.at> wrote:

> Am 22.06.2016 um 16:01 schrieb Boris Brezillon:
> > On Wed, 22 Jun 2016 15:54:34 +0200
> > Richard Weinberger <richard@nod.at> wrote:
> >   
> >> Am 22.06.2016 um 14:43 schrieb Boris Brezillon:  
> >>> Why do we need to hardcode /sys/class/ubi/version to 1? We just need to
> >>> update the mtd-utils to support version 2. Am I missing something?    
> >>
> >> We don't want to break existing userspace.
> >> Why should ubimkvol or ubiattach fail on a system with SLC NAND and
> >> CONFIG_MTD_UBI_CONSOLIDATE=y?  
> > 
> > But version won't be set to 2 in this case. If it's an SLC NAND we
> > don't need to use UBI version 2, do we.  
> 
> /sys/class/ubi/version is the version of the UBI implementation,
> not the version of the attached UBI image.
> It will the here as soon you load the UBI module.

Do we have /sys/class/ubi/ubiX/version for the UBI image version?
This is still unclear to me why we need to version the
user-space/kernel-space ABI, since it's supposed to be backward
compatible, so adding new features requires adding new ioctls and
keeping the old ones in a working state.

What is /sys/class/ubi/version actually encoding? Isn't it encoding the
fact that a specific UBI implementation is supporting all UBI on-flash
formats up to format version X (that was my understanding)?

Re: [RFC] Raising the UBI version

[Richard Weinberger]

Am 22.06.2016 um 16:13 schrieb Boris Brezillon:
>> /sys/class/ubi/version is the version of the UBI implementation,
>> not the version of the attached UBI image.
>> It will the here as soon you load the UBI module.
> 
> Do we have /sys/class/ubi/ubiX/version for the UBI image version?

No. That's why I plan to add /sys/class/ubi/ubiX/features_used to
show which features the attached UBI image requested.
And having a /sys/class/ubi/ubi/features which denotes what features
the _implementation_ supports.

> This is still unclear to me why we need to version the
> user-space/kernel-space ABI, since it's supposed to be backward
> compatible, so adding new features requires adding new ioctls and
> keeping the old ones in a working state.
> 
> What is /sys/class/ubi/version actually encoding? Isn't it encoding the
> fact that a specific UBI implementation is supporting all UBI on-flash
> formats up to format version X (that was my understanding)?
> 

Well, /sys/class/ubi/version exports UBI_VERSION from ubi-media.h.
It is (ab)used to encode the ABI version *and* the on-flash version.
The problem is that mtd-utils libubi will refuse to work with
/sys/class/ubi/version unequal 1.

Here the gem from libubi:
        if (read_positive_int(lib->ubi_version, &version))
                goto out_error;
        if (version != LIBUBI_UBI_VERSION) {
                errmsg("this library was made for UBI version %d, but UBI "
                       "version %d is detected\n", LIBUBI_UBI_VERSION, version);
                goto out_error;
        }

This is why I want to hardcode it to 1.
Everything else will break existing user space in some way.
10 years ago /sys/class/ubi/version seemed like a good idea
but now it hits us hard.

Thanks,
//richard

Re: [RFC] Raising the UBI version

[Boris Brezillon]

On Wed, 22 Jun 2016 16:21:36 +0200
Richard Weinberger <richard@nod.at> wrote:

> Am 22.06.2016 um 16:13 schrieb Boris Brezillon:
> >> /sys/class/ubi/version is the version of the UBI implementation,
> >> not the version of the attached UBI image.
> >> It will the here as soon you load the UBI module.  
> > 
> > Do we have /sys/class/ubi/ubiX/version for the UBI image version?  
> 
> No. That's why I plan to add /sys/class/ubi/ubiX/features_used to
> show which features the attached UBI image requested.
> And having a /sys/class/ubi/ubi/features which denotes what features
> the _implementation_ supports.

Still the version and features are encoding different things IMO.
Incrementing the on-flash version means that the on-flash format has
changed in an incompatible way, while features denotes the fact that
the existing format has been extended with new features but is backward
compatible.

> 
> > This is still unclear to me why we need to version the
> > user-space/kernel-space ABI, since it's supposed to be backward
> > compatible, so adding new features requires adding new ioctls and
> > keeping the old ones in a working state.
> > 
> > What is /sys/class/ubi/version actually encoding? Isn't it encoding the
> > fact that a specific UBI implementation is supporting all UBI on-flash
> > formats up to format version X (that was my understanding)?
> >   
> 
> Well, /sys/class/ubi/version exports UBI_VERSION from ubi-media.h.
> It is (ab)used to encode the ABI version *and* the on-flash version.

The on-flash versions supported by the implementation is a useful
information.

> The problem is that mtd-utils libubi will refuse to work with
> /sys/class/ubi/version unequal 1.
> 
> Here the gem from libubi:
>         if (read_positive_int(lib->ubi_version, &version))
>                 goto out_error;
>         if (version != LIBUBI_UBI_VERSION) {
>                 errmsg("this library was made for UBI version %d, but UBI "
>                        "version %d is detected\n", LIBUBI_UBI_VERSION, version);
>                 goto out_error;
>         }

And this is where the problem is: libubi does not make proper use of
this information. We should either have

	if (version >= LIBUBI_UBI_VERSION)

or, if we decide that version is a bitfield directly encoding which
versions are supported by the implementation

#define VERSION_SUPPORTED(version, x)	((version) & BIT((x)-1)))

	if (VERSION_SUPPORTED(version, 1))

> 
> This is why I want to hardcode it to 1.
> Everything else will break existing user space in some way.
> 10 years ago /sys/class/ubi/version seemed like a good idea
> but now it hits us hard.

Yes, I understand that, but this also means /sys/class/ubi/version is
just a dummy file which only purpose is to make libubi happy :).
If this is the case, then I think we should have another file encoding
the supported on-flash formats...

Re: [RFC] Raising the UBI version

[Richard Weinberger]

Am 22.06.2016 um 16:39 schrieb Boris Brezillon:
> On Wed, 22 Jun 2016 16:21:36 +0200
> Richard Weinberger <richard@nod.at> wrote:
> 
>> Am 22.06.2016 um 16:13 schrieb Boris Brezillon:
>>>> /sys/class/ubi/version is the version of the UBI implementation,
>>>> not the version of the attached UBI image.
>>>> It will the here as soon you load the UBI module.  
>>>
>>> Do we have /sys/class/ubi/ubiX/version for the UBI image version?  
>>
>> No. That's why I plan to add /sys/class/ubi/ubiX/features_used to
>> show which features the attached UBI image requested.
>> And having a /sys/class/ubi/ubi/features which denotes what features
>> the _implementation_ supports.
> 
> Still the version and features are encoding different things IMO.
> Incrementing the on-flash version means that the on-flash format has
> changed in an incompatible way, while features denotes the fact that
> the existing format has been extended with new features but is backward
> compatible.

Yes. But now we have a mix of both. ;-\

>>
>>> This is still unclear to me why we need to version the
>>> user-space/kernel-space ABI, since it's supposed to be backward
>>> compatible, so adding new features requires adding new ioctls and
>>> keeping the old ones in a working state.
>>>
>>> What is /sys/class/ubi/version actually encoding? Isn't it encoding the
>>> fact that a specific UBI implementation is supporting all UBI on-flash
>>> formats up to format version X (that was my understanding)?
>>>   
>>
>> Well, /sys/class/ubi/version exports UBI_VERSION from ubi-media.h.
>> It is (ab)used to encode the ABI version *and* the on-flash version.
> 
> The on-flash versions supported by the implementation is a useful
> information.
> 
>> The problem is that mtd-utils libubi will refuse to work with
>> /sys/class/ubi/version unequal 1.
>>
>> Here the gem from libubi:
>>         if (read_positive_int(lib->ubi_version, &version))
>>                 goto out_error;
>>         if (version != LIBUBI_UBI_VERSION) {
>>                 errmsg("this library was made for UBI version %d, but UBI "
>>                        "version %d is detected\n", LIBUBI_UBI_VERSION, version);
>>                 goto out_error;
>>         }
> 
> And this is where the problem is: libubi does not make proper use of
> this information. We should either have
> 
> 	if (version >= LIBUBI_UBI_VERSION)
> 
> or, if we decide that version is a bitfield directly encoding which
> versions are supported by the implementation
> 
> #define VERSION_SUPPORTED(version, x)	((version) & BIT((x)-1)))
> 
> 	if (VERSION_SUPPORTED(version, 1))

Yep. But we cannot change already compiled and shipped code.

>>
>> This is why I want to hardcode it to 1.
>> Everything else will break existing user space in some way.
>> 10 years ago /sys/class/ubi/version seemed like a good idea
>> but now it hits us hard.
> 
> Yes, I understand that, but this also means /sys/class/ubi/version is
> just a dummy file which only purpose is to make libubi happy :).

That's the plan.

> If this is the case, then I think we should have another file encoding
> the supported on-flash formats...

This is what /sys/class/ubi/features was supposed to do.

Thanks,
//richard

Re: [RFC] Raising the UBI version

[Boris Brezillon]

On Wed, 22 Jun 2016 16:43:49 +0200
Richard Weinberger <richard@nod.at> wrote:

> Am 22.06.2016 um 16:39 schrieb Boris Brezillon:
> > On Wed, 22 Jun 2016 16:21:36 +0200
> > Richard Weinberger <richard@nod.at> wrote:
> >   
> >> Am 22.06.2016 um 16:13 schrieb Boris Brezillon:  
> >>>> /sys/class/ubi/version is the version of the UBI implementation,
> >>>> not the version of the attached UBI image.
> >>>> It will the here as soon you load the UBI module.    
> >>>
> >>> Do we have /sys/class/ubi/ubiX/version for the UBI image version?    
> >>
> >> No. That's why I plan to add /sys/class/ubi/ubiX/features_used to
> >> show which features the attached UBI image requested.
> >> And having a /sys/class/ubi/ubi/features which denotes what features
> >> the _implementation_ supports.  
> > 
> > Still the version and features are encoding different things IMO.
> > Incrementing the on-flash version means that the on-flash format has
> > changed in an incompatible way, while features denotes the fact that
> > the existing format has been extended with new features but is backward
> > compatible.  
> 
> Yes. But now we have a mix of both. ;-\
> 
> >>  
> >>> This is still unclear to me why we need to version the
> >>> user-space/kernel-space ABI, since it's supposed to be backward
> >>> compatible, so adding new features requires adding new ioctls and
> >>> keeping the old ones in a working state.
> >>>
> >>> What is /sys/class/ubi/version actually encoding? Isn't it encoding the
> >>> fact that a specific UBI implementation is supporting all UBI on-flash
> >>> formats up to format version X (that was my understanding)?
> >>>     
> >>
> >> Well, /sys/class/ubi/version exports UBI_VERSION from ubi-media.h.
> >> It is (ab)used to encode the ABI version *and* the on-flash version.  
> > 
> > The on-flash versions supported by the implementation is a useful
> > information.
> >   
> >> The problem is that mtd-utils libubi will refuse to work with
> >> /sys/class/ubi/version unequal 1.
> >>
> >> Here the gem from libubi:
> >>         if (read_positive_int(lib->ubi_version, &version))
> >>                 goto out_error;
> >>         if (version != LIBUBI_UBI_VERSION) {
> >>                 errmsg("this library was made for UBI version %d, but UBI "
> >>                        "version %d is detected\n", LIBUBI_UBI_VERSION, version);
> >>                 goto out_error;
> >>         }  
> > 
> > And this is where the problem is: libubi does not make proper use of
> > this information. We should either have
> > 
> > 	if (version >= LIBUBI_UBI_VERSION)
> > 
> > or, if we decide that version is a bitfield directly encoding which
> > versions are supported by the implementation
> > 
> > #define VERSION_SUPPORTED(version, x)	((version) & BIT((x)-1)))
> > 
> > 	if (VERSION_SUPPORTED(version, 1))  
> 
> Yep. But we cannot change already compiled and shipped code.
> 
> >>
> >> This is why I want to hardcode it to 1.
> >> Everything else will break existing user space in some way.
> >> 10 years ago /sys/class/ubi/version seemed like a good idea
> >> but now it hits us hard.  
> > 
> > Yes, I understand that, but this also means /sys/class/ubi/version is
> > just a dummy file which only purpose is to make libubi happy :).  
> 
> That's the plan.
> 
> > If this is the case, then I think we should have another file encoding
> > the supported on-flash formats...  
> 
> This is what /sys/class/ubi/features was supposed to do.

Then you still miss the version concept. Again, versions and features
are orthogonal, and you're not guaranteed that we'll never change the
on-flash format in an incompatible way...

So how about defining the following:
- /sys/class/ubi/version: user-space ABI version (should always be one)
- /sys/class/ubi/supported-on-flash-formats: either a bitfield or an
  integer representing the higher on-flash format version supported by
  the implementation (which implies that implementations have to
  support all on-flash formats up-to supported-on-flash-formats)
- /sys/class/ubi/supported-features: the features supported by the
  implementation (each bit is a specific feature or a set of features).
  The features may or may not be version dependent.
- /sys/class/ubi/ubiX/on-flash-format: the on-flash format used on the
  UBIX device
- /sys/class/ubi/ubiX/features: the features exposed by this UBIX
  device

Re: [RFC] Raising the UBI version

[Richard Weinberger]

Am 22.06.2016 um 16:52 schrieb Boris Brezillon:
> So how about defining the following:
> - /sys/class/ubi/version: user-space ABI version (should always be one)

Yes. To not break libubi.

> - /sys/class/ubi/supported-on-flash-formats: either a bitfield or an
>   integer representing the higher on-flash format version supported by
>   the implementation (which implies that implementations have to
>   support all on-flash formats up-to supported-on-flash-formats)

We can do that. But who will evaluate this file?

> - /sys/class/ubi/supported-features: the features supported by the
>   implementation (each bit is a specific feature or a set of features).
>   The features may or may not be version dependent.
> - /sys/class/ubi/ubiX/on-flash-format: the on-flash format used on the
>   UBIX device
> - /sys/class/ubi/ubiX/features: the features exposed by this UBIX
>   device

I think we should make features depend on version = 2.
IOW when we change the UBI format in a major way version will be 3 and
we maybe use something else to expose features.

Thanks,
//richard

Re: [RFC] Raising the UBI version

[Boris Brezillon]

On Wed, 22 Jun 2016 16:59:50 +0200
Richard Weinberger <richard@nod.at> wrote:

> Am 22.06.2016 um 16:52 schrieb Boris Brezillon:
> > So how about defining the following:
> > - /sys/class/ubi/version: user-space ABI version (should always be one)  
> 
> Yes. To not break libubi.
> 
> > - /sys/class/ubi/supported-on-flash-formats: either a bitfield or an
> >   integer representing the higher on-flash format version supported by
> >   the implementation (which implies that implementations have to
> >   support all on-flash formats up-to supported-on-flash-formats)  
> 
> We can do that. But who will evaluate this file?

ubiformat may need that one to check if the provided ubi image is
supported by the implementation.

> 
> > - /sys/class/ubi/supported-features: the features supported by the
> >   implementation (each bit is a specific feature or a set of features).
> >   The features may or may not be version dependent.
> > - /sys/class/ubi/ubiX/on-flash-format: the on-flash format used on the
> >   UBIX device
> > - /sys/class/ubi/ubiX/features: the features exposed by this UBIX
> >   device  
> 
> I think we should make features depend on version = 2.
> IOW when we change the UBI format in a major way version will be 3 and
> we maybe use something else to expose features.

IIUC, we move to version 2 now, because we're using one of the padding
byte (word?) to expose the feature flags, correct?
It makes sense.

Re: [RFC] Raising the UBI version

[Richard Weinberger]

Am 22.06.2016 um 17:06 schrieb Boris Brezillon:
> On Wed, 22 Jun 2016 16:59:50 +0200
> Richard Weinberger <richard@nod.at> wrote:
> 
>> Am 22.06.2016 um 16:52 schrieb Boris Brezillon:
>>> So how about defining the following:
>>> - /sys/class/ubi/version: user-space ABI version (should always be one)  
>>
>> Yes. To not break libubi.
>>
>>> - /sys/class/ubi/supported-on-flash-formats: either a bitfield or an
>>>   integer representing the higher on-flash format version supported by
>>>   the implementation (which implies that implementations have to
>>>   support all on-flash formats up-to supported-on-flash-formats)  
>>
>> We can do that. But who will evaluate this file?
> 
> ubiformat may need that one to check if the provided ubi image is
> supported by the implementation.

What if someone wants to ubiformat (with MLC support) his mtd0 on an old
kernel and then reboots into the new one with MLC support?
I'm not sure if it is wise do forbid that use case since allowing it does not
hurt.

Don't get me wrong, I just want to be sure that all new sysfs files we add
have a clear defined use case and won't hurt us later.

>>
>>> - /sys/class/ubi/supported-features: the features supported by the
>>>   implementation (each bit is a specific feature or a set of features).
>>>   The features may or may not be version dependent.
>>> - /sys/class/ubi/ubiX/on-flash-format: the on-flash format used on the
>>>   UBIX device
>>> - /sys/class/ubi/ubiX/features: the features exposed by this UBIX
>>>   device  
>>
>> I think we should make features depend on version = 2.
>> IOW when we change the UBI format in a major way version will be 3 and
>> we maybe use something else to expose features.
> 
> IIUC, we move to version 2 now, because we're using one of the padding
> byte (word?) to expose the feature flags, correct?
> It makes sense.

I'm not absolutely sure but keeping ->version and adding ->features seems
the most feasible solution so far.

Thanks,
//richard

Re: [RFC] Raising the UBI version

[Boris Brezillon]

On Wed, 22 Jun 2016 21:59:46 +0200
Richard Weinberger <richard@nod.at> wrote:

> Am 22.06.2016 um 17:06 schrieb Boris Brezillon:
> > On Wed, 22 Jun 2016 16:59:50 +0200
> > Richard Weinberger <richard@nod.at> wrote:
> >   
> >> Am 22.06.2016 um 16:52 schrieb Boris Brezillon:  
> >>> So how about defining the following:
> >>> - /sys/class/ubi/version: user-space ABI version (should always be one)    
> >>
> >> Yes. To not break libubi.
> >>  
> >>> - /sys/class/ubi/supported-on-flash-formats: either a bitfield or an
> >>>   integer representing the higher on-flash format version supported by
> >>>   the implementation (which implies that implementations have to
> >>>   support all on-flash formats up-to supported-on-flash-formats)    
> >>
> >> We can do that. But who will evaluate this file?  
> > 
> > ubiformat may need that one to check if the provided ubi image is
> > supported by the implementation.  
> 
> What if someone wants to ubiformat (with MLC support) his mtd0 on an old
> kernel and then reboots into the new one with MLC support?
> I'm not sure if it is wise do forbid that use case since allowing it does not
> hurt.
> 
> Don't get me wrong, I just want to be sure that all new sysfs files we add
> have a clear defined use case and won't hurt us later.

Hm, maybe you're right, exposing this information through sysfs is not
a requirement, but I think we need to store this information in the EC
and/or VID headers, so that the UBI implementation can recognize the
on-flash format.

> 
> >>  
> >>> - /sys/class/ubi/supported-features: the features supported by the
> >>>   implementation (each bit is a specific feature or a set of features).
> >>>   The features may or may not be version dependent.
> >>> - /sys/class/ubi/ubiX/on-flash-format: the on-flash format used on the
> >>>   UBIX device
> >>> - /sys/class/ubi/ubiX/features: the features exposed by this UBIX
> >>>   device    
> >>
> >> I think we should make features depend on version = 2.
> >> IOW when we change the UBI format in a major way version will be 3 and
> >> we maybe use something else to expose features.  
> > 
> > IIUC, we move to version 2 now, because we're using one of the padding
> > byte (word?) to expose the feature flags, correct?
> > It makes sense.  
> 
> I'm not absolutely sure but keeping ->version and adding ->features seems
> the most feasible solution so far.

Okay, let's forget what we expose through sysfs for a moment and focus
on the on-flash format. Do we agree that it's better to keep a version
field encoding the 'major' version of the on-flash format, and
introducing a feature field to allow minor backward-compatible
improvements of a given 'major' version?

Re: [RFC] Raising the UBI version

[Richard Weinberger]

Am 22.06.2016 um 22:12 schrieb Boris Brezillon:
>> Don't get me wrong, I just want to be sure that all new sysfs files we add
>> have a clear defined use case and won't hurt us later.
> 
> Hm, maybe you're right, exposing this information through sysfs is not
> a requirement, but I think we need to store this information in the EC
> and/or VID headers, so that the UBI implementation can recognize the
> on-flash format.

Sure.

>>
>>>>  
>>>>> - /sys/class/ubi/supported-features: the features supported by the
>>>>>   implementation (each bit is a specific feature or a set of features).
>>>>>   The features may or may not be version dependent.
>>>>> - /sys/class/ubi/ubiX/on-flash-format: the on-flash format used on the
>>>>>   UBIX device
>>>>> - /sys/class/ubi/ubiX/features: the features exposed by this UBIX
>>>>>   device    
>>>>
>>>> I think we should make features depend on version = 2.
>>>> IOW when we change the UBI format in a major way version will be 3 and
>>>> we maybe use something else to expose features.  
>>>
>>> IIUC, we move to version 2 now, because we're using one of the padding
>>> byte (word?) to expose the feature flags, correct?
>>> It makes sense.  
>>
>> I'm not absolutely sure but keeping ->version and adding ->features seems
>> the most feasible solution so far.
> 
> Okay, let's forget what we expose through sysfs for a moment and focus
> on the on-flash format. Do we agree that it's better to keep a version
> field encoding the 'major' version of the on-flash format, and
> introducing a feature field to allow minor backward-compatible
> improvements of a given 'major' version?

Yes. That would be variant ii).

Thanks,
//richard