linux-fpga.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Tom Rix <trix@redhat.com>
To: Moritz Fischer <mdf@kernel.org>, "Wu, Hao" <hao.wu@intel.com>
Cc: "Weight, Russell H" <russell.h.weight@intel.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"linux-fpga@vger.kernel.org" <linux-fpga@vger.kernel.org>
Subject: Re: [PATCH] fpga: region: handle compat_id as an uuid
Date: Thu, 29 Jul 2021 12:16:47 -0700	[thread overview]
Message-ID: <a5b4b303-7d9b-27d7-4c1e-cd29fea8cdb9@redhat.com> (raw)
In-Reply-To: <YQL4qyAmqj322HTz@epycbox.lan>


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@redhat.com wrote:
>>>>> From: Tom Rix <trix@redhat.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
>


  reply	other threads:[~2021-07-29 19:16 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-26 20:26 trix
2021-07-26 22:12 ` Russ Weight
2021-07-27  0:16   ` Tom Rix
2021-07-28  1:36     ` Wu, Hao
2021-07-29 18:51       ` Moritz Fischer
2021-07-29 19:16         ` Tom Rix [this message]
2021-07-30  1:48           ` Xu Yilun
2021-07-30 12:07             ` Tom Rix
2021-08-03  3:32               ` Xu Yilun
2021-08-03 12:21                 ` Tom Rix
2021-08-04  1:44                   ` Xu Yilun
2021-08-04 13:21                     ` Tom Rix
2021-08-06  1:42                       ` Xu Yilun
2021-07-26 23:35 ` kernel test robot

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=a5b4b303-7d9b-27d7-4c1e-cd29fea8cdb9@redhat.com \
    --to=trix@redhat.com \
    --cc=hao.wu@intel.com \
    --cc=linux-fpga@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mdf@kernel.org \
    --cc=russell.h.weight@intel.com \
    --subject='Re: [PATCH] fpga: region: handle compat_id as an uuid' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).