Re: [PATCH v2 2/2] firmware/arm_ffa: remove __le64_to_cpu() when set uuid for direct msg v2

[Sudeep Holla <sudeep.holla@arm.com>]

On Tue, Dec 10, 2024 at 09:45:51AM +0100, Arnd Bergmann wrote:
> On Tue, Dec 10, 2024, at 08:36, Yeoreum Yun wrote:
> >> On Mon, Dec 9, 2024, at 17:59, Sudeep Holla wrote:
> >> > On Mon, Dec 09, 2024 at 04:27:14PM +0100, Arnd Bergmann wrote:
> >>
> >> Looking through the other functions in drivers/firmware/arm_ffa/driver.c,
> >> I see that most of them just match the specification. One exception
> >> is ffa_notification_info_get(), which incorrectly casts the
> >> argument response arguments to an array of 'u16' values. Using
> >> the correct bit shifts according to the specification would
> >> make that work on big-endian and also more readable and
> >> robust. Another one is __ffa_partition_info_get_regs(), which
> >> does an incorrect memcpy() instead of decoding the values.
> >>
> > Conclusionly, Yes. But the RFC 4122 said with network byte order.
> > to describe how uuid is saved.
> >
> > but I think the endianess to load the register is not a choice.
> > because the spec says:
> >
> >     UUID Lo  x2  Bytes[0...7] of UUID with byte 0 in the low-order bits.
> >     UUID Hi  x3  Bytes[8...15] of UUID with byte 8 in the low-order bits.
> >
> > this means UUID.bytes[0] should be loaded to x2.bytes[0].
> >            UUID.bytes[1] should be loaded to x2,bytes[1]
> >            ...
> 
> I meant they had the choice and chose to specify little-endian
> 64-bit word to encode the sequence of bytes of the standard
> in-memory representation of UUIDs. 
> 
> > That's why other software spec (i.e tf-a) doesn't loads UUID from register
> > wihtout swapping byte with endianess but just copy it.
> 
> If the uuid is transferred in memory, you obviously don't want to
> swap it. If they pass it in registers across different endianess
> code without specifying the byteorder in the caller, then they
> would have the same bug.
> 
> > The bug is "not send UUID according to spec" in kernel side
> > That's why it fails when I send message  with direct message version 2.
> > So, it''s not change code unportable to portable but it fixes according
> > to spec (load UUID as it is in register wihtout endianess).
> 
> Sorry, but you are not making sense here.
>

Agreed. The patch is clearly wrong. Thanks for refreshing my knowledge and
helping me to clear my confusion.

> Sudeep, should I just cherry-pick your other fix from the pull
> request and ignore this patch?
>

I will remove the wrong patch and retag and send PR if that helps. It should
be quick.

-- 
Regards,
Sudeep