Re: [PATCH RFC 1/2] dt-bindings: mtd: ubi-volume: add 'volume-is-critical' property

[Krzysztof Kozlowski <krzk@kernel.org>]

On 28/09/2024 15:09, Daniel Golle wrote:
> On Sat, Sep 28, 2024 at 03:02:47PM +0200, Krzysztof Kozlowski wrote:
>> On 28/09/2024 14:47, Daniel Golle wrote:
>>> Add the 'volume-is-critical' boolean property which marks a UBI volume
>>> as critical for the device to boot. If set it prevents the user from
>>> all kinds of write access to the volume as well as from renaming it or
>>> detaching the UBI device it is located on.
>>>
>>> Signed-off-by: Daniel Golle <daniel@makrotopia.org>
>>> ---
>>>  .../devicetree/bindings/mtd/partitions/ubi-volume.yaml   | 9 +++++++++
>>>  1 file changed, 9 insertions(+)
>>>
>>> diff --git a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
>>> index 19736b26056b..2bd751bb7f9e 100644
>>> --- a/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
>>> +++ b/Documentation/devicetree/bindings/mtd/partitions/ubi-volume.yaml
>>> @@ -29,6 +29,15 @@ properties:
>>>      description:
>>>        This container may reference an NVMEM layout parser.
>>>  
>>> +  volume-is-critical:
>>> +    description: This parameter, if present, indicates that the UBI volume
>>> +      contains early-boot firmware images or data which should not be clobbered.
>>> +      If set, it prevents the user from renaming the volume, writing to it or
>>> +      making any changes affecting it, as well as detaching the UBI device it is
>>> +      located on, so direct access to the underlying MTD device is prevented as
>>> +      well.
>>> +    type: boolean
>>
>> UBI volumes are mapping to partitions 1-to-1, right? So rather I would
>> propose to use partition.yaml - we already have read-only there with
>> very similar description.
> 
> No, that's not the case.
> 
> An MTD partition can be used as UBI device. A UBI device (and hence MTD
> partition) can host *several* UBI volumes.
> 
> Marking the MTD partition as 'read-only' won't work, as UBI needs
> read-write access to perform bad block relocation, scrubbing, ...

OK, so not partition but read-only volume.

> 
> Also, typically not all UBI volumes on a UBI device are
> read-only/critical but only a subset of them.
> 
> But you are right that the description is inspired by the description
> of the 'read-only' property in partition.yaml ;)
> 
> I initially thought to also name the property 'read-only', just like
> for MTD partitions. However, as the desired effect goes beyond
> preventing write access to the volume itself, I thought it'd be
> better to use a new name.

Yeah, maybe... critical indeed covers multiple cases but is also
subjective. For some bootloader is critical, for other bootloader still
might be fully A/B updateable thus could be modifiable. For others, they
want to use fw_setenv from user-space so not critical at all.

I am in general not so happy in describing flash layout in DT, because
it sounds way too policy or one-use-case specific.

UBI volume is purely SW construct. One OS can partition MTD in multiple
ways, so maybe I just do not understand why we have in the first place.

Best regards,
Krzysztof