[U-Boot] [PATCH 4/8] cmd: avb2.0: avb command for performing verification

[Simon Glass]

(Tom can you please comment on the CamelCase question?)

Hi Igor,

On 15 May 2018 at 11:31, Igor Opaniuk <igor.opaniuk at linaro.org> wrote:
> On 15 May 2018 at 19:26, Simon Glass <sjg at chromium.org> wrote:
>> Hi Igor,
>>
>> On 16 May 2018 at 01:44, Igor Opaniuk <igor.opaniuk at linaro.org> wrote:
>>> Hi Simon,
>>>
>>> I've dug into DriverModel documentation and even created a PoC for
>>> existing avb commands. The problem is that (maybe I missed out some
>>> key concepts) I'm still
>>> not sure if it makes sense to follow it driver mode in the context of
>>> AVB 2.0 feature and
>>> what kind of extra devices can be used within the same uclass in this case?
>>> Could you please explain in detail.
>>> Thanks
>>
>> avb_ops_alloc() is allocating a struct and then assigning operations
>> to its members.
>>
>> This is what driver model is designed to do. It handles the
>> allocations and the operations become part of the uclass interface.
>>
>> At present it looks like you only have one driver. I'm not sure if it
>> would make sense to have a second one.
>>
>> As a counter example see cros_ec uclass, which does things
>> differently. It models the EC interface (I2C, SPI, LPC) using DM, with
>> just a single impl on top.
>
> Right, I do understand what DriverModel is and why it should
> be used in the case of real-device drivers. But regarding Android Verified
> Boot 2.0 feature, which introduces verification capabilities
> and leverages only device-independent APIs,
> I see no reason why it should be used here.
> Could you please check [1] and confirm that this set of commands should
> really follow this model.

If there is no need for operations and indirection through function
pointers, why not just call the functions directly?

I do think (as the code is currently structured) that DM makes sense,
even though I understand that you are not creating a device. Perhaps
Tom might have thoughts on this though?

>
>> Also can you please drop the CamelCase in the patches? We don't use
>> that in U-Boot.
>
> Frankly, I don't like it also, bit all CamelCase instances in
> the code relate to libavb/libavb_ab library ([2]), which is planned to be
> deviated from AOSP upstream in the future.
> That's why a decision was made to not introduce any changes to simplify this
> process, as Google intensively introduces new changes to it.

Tom, what do you think? It's not how things usually work, but perhaps
there is precedent for this?

> (I've left a notice in the cover letter that checkpatch
> will fail on the commit, which introduces libavb; also, there is the ongoing
> discussion there regarding why libavb/libavb_ab should be
> kept as it's. Please join us, if you don't mind).

I do see the cover letter thread but could not find the camel case
discussion. Can you please point me to it?

>
> Thanks!
>
> Best regards,
> Igor
>
> [1] https://android.googlesource.com/platform/external/avb/+/master/README.md
> [2] https://android.googlesource.com/platform/external/avb/
>>>

[..]

Regards,
Simon