Date:   Thu, 29 Jul 2021 12:16:47 -0700
From:   Tom Rix <trix@...hat.com>
To:     Moritz Fischer <mdf@...nel.org>, "Wu, Hao" <hao.wu@...el.com>
Cc:     "Weight, Russell H" <russell.h.weight@...el.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-fpga@...r.kernel.org" <linux-fpga@...r.kernel.org>
Subject: Re: [PATCH] fpga: region: handle compat_id as an uuid


On 7/29/21 11:51 AM, Moritz Fischer wrote:
> On Wed, Jul 28, 2021 at 01:36:56AM +0000, Wu, Hao wrote:
>>> On 7/26/21 3:12 PM, Russ Weight wrote:
>>>> On 7/26/21 1:26 PM, trix@...hat.com wrote:
>>>>> From: Tom Rix <trix@...hat.com>
>>>>>
>>>>> An fpga region's compat_id is exported by the sysfs
>>>>> as a 128 bit hex string formed by concatenating two
>>>>> 64 bit values together.
>>>>>
>>>>> The only user of compat_id is dfl.  Its user library
>>>>> opae converts this value into a uuid.
>>>>>
>>>>> ex/
>>>>> $ cat /sys/class/fpga_region/region1/compat_id
>>>>> f3c9941350814aadbced07eb84a6d0bb
>>>>>
>>>>> Is reported as
>>>>> $ fpgainfo bmc
>>>>> ...
>>>>> Pr Interface Id                  : f3c99413-5081-4aad-bced-07eb84a6d0bb
>>>>>
>>>>> Storing a uuid as 2 64 bit values is vendor specific.
>>>>> And concatenating them together is vendor specific.
>>>>>
>>>>> It is better to store and print out as a vendor neutral uuid.
>>>>>
>>>>> Change fpga_compat_id from a struct to a union.
>>>>> Keep the old 64 bit values for dfl.
>>>>> Sysfs output is now
>>>>> f3c99413-5081-4aad-bced-07eb84a6d0bb
>>>> I'm fowarding feedback from Tim Whisonant, one of the OPAE userspace
>>>> developers:
>>>>
>>>> I think that this change to the sysfs for the compat_id node will
>>>> end up breaking the SDK, which does not expect the '-' characters to
>>>> be included when parsing the sysfs value. Currently, it is parsed as
>>>> a raw hex string without regard to any '-' characters. This goes for
>>>> any "guid" currently exported by sysfs and for what we read in the
>>>> device MMIO space.
>>> Yes, it will.
>>>
>>> And there are other places, like dfl-afu-main.c:afu_id_show()
>>>
>>> outputs raw hex that sdk turns into a uuid.
>>>
>>>
>>> Some options.
>>>
>>> If no one but dfl will ever use it, then v1 of patchset.
>>>
>>> If others can use it but don't want to change dfl, then v2 of patchset,
>>> my favorite.
>>>
>>> Or this one for uuid for everyone, what have been v3 but changed too much.
>>>
>>>
>>> could dfl change generally to output uuid's to the sysfs ?
>>>
>>> this would be generally helpful and a one time disruption to the sdk.
>> This change limited the output format to uuid_t, but if any hardware doesn't
>> use uuid_t on hardware may have to convert it back from the sysfs output in
>> userspace. Leave it to print hardware values (e.g. from register), and convert
>> it in userspace should be fine too I think.
> I'm not entirely sure. I seem to recall there being examples of sysfs
> files returning different things for different drivers.
>
> That being said it seems largely cosmetic to add the '-' in between.
>
> If it breaks userspace, I'm against it. If you *need* it make a
> compat_uuid entry or something in that case?

My gripe is

For a nominally common interface, compat_id has a vendor specific output.

If for example another vendor wanted to use this field but their natural 
format was an OF string.

16 bytes of raw hex would not work for them, so they would roll their own.

which defeats the purpose of a common interface.


The language in the docs as-is is vague on the output format.

DFL is the only user of the interface.

So ver 2

https://lore.kernel.org/linux-fpga/4ab7dd2d-c215-6333-6860-6f7d0ac64c3d@redhat.com/

Keeps the output as-is for dfl, so nothing breaks in userspace

And adds flexibility for vendors to output their appropriate natural form.

So compat_id becomes generally useful.


Tom


>
> - Moritz
>

Powered by blists - more mailing lists