On 8/2/21 8:32 PM, Xu Yilun wrote:
On Fri, Jul 30, 2021 at 05:07:00AM -0700, Tom Rix wrote:
On 7/29/21 6:48 PM, Xu Yilun wrote:
On Thu, Jul 29, 2021 at 12:16:47PM -0700, Tom Rix wrote:
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@xxxxxxxxxx wrote:
From: Tom Rix <trix@xxxxxxxxxx>
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@xxxxxxxxxx/
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.
Mixing types seems be strongly against in Documentation/filesystems/sysfs.rst.
So in my opinion there should be a determined format for the output. The
concern for this patch is which one is a better format, uuid style or
128 bit raw hex?
And I vote for 128 bit raw hex, as other vendors may not use uuid_t as
the identifier, may be an OF string. So we don't have to force them
decorate it as the uuid style.
So you would be ok with v2 of this patchset ?
No. I prefer we keep the current implementation. I mean 128 bit raw hex
could be a more compatible output format for all vendors, uuid style,
string style or others.
How would 128 bit raw hex be compatible if a vendor's natural format was
an OF string ?
Why would they even use compat_id ?
Tom
Thanks,
Yilun
Tom
Thanks
Yilun
Tom
- Moritz
