linux-kernel - Re: [PATCH v5] fpga manager: Add Altera CvP driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 8 Jun 2017 17:44:19 +0300
From:   Andy Shevchenko <andy.shevchenko@...il.com>
To:     Anatolij Gustschin <agust@...x.de>
Cc:     linux-fpga@...r.kernel.org, Alan Tull <atull@...nel.org>,
        Moritz Fischer <moritz.fischer@...us.com>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        matthew.gerlach@...ux.intel.com, yi1.li@...ux.intel.com
Subject: Re: [PATCH v5] fpga manager: Add Altera CvP driver

On Thu, Jun 8, 2017 at 5:15 PM, Anatolij Gustschin <agust@...x.de> wrote:
> On Thu, 8 Jun 2017 02:38:55 +0300
> Andy Shevchenko andy.shevchenko@...il.com wrote:
>>On Thu, Jun 8, 2017 at 2:09 AM, Anatolij Gustschin <agust@...x.de> wrote:
>>> On Fri, 2 Jun 2017 20:43:21 +0300
>>> Andy Shevchenko andy.shevchenko@...il.com wrote:

>>Besides below comments, please, do
>>
>>s/VSEC_/VSE_/g
>>
>>for entire file.
>>
>>We are following PCI and Thunderbolt pattern for use of Vendor
>>Specific Extended Capability.
>
> I can do it, but I'm just not getting why. The registers are named as VSEC
> registers in the documentation, why should the code name them differently?

Does your documentation decode VSEC abbreviation?
What C stands for? Capability?

In PCI and Thunderbolt we agreed to use word capability separately,
so, either XXX_... or XXX_CAP_... to use.

>>>>> +       if (!timeout_us)
>>>>> +               return -ETIMEDOUT;
>>>>
>>>>Hmm...
>>>>What as a user I would expect here is at least one attempt (0 -- no
>>>>timeout, but try once).
>>>
>>> yes, this first attempt is above, please see original patch for full
>>> context.
>>
>>Ah, it means you don't correctly use do {} while approach.
>>
>>Remove everything above do { and move usleep after check for the
>>status inside the loop.
>
> Unfortunately, suggested approach has an unwanted side effect:

How come? See below.

>
>         do {
>                 check and return if done;
>
>                 usleep_range(10, 11);
>                 tout -= 10;
>
>         } while (tout > 0);
>
> For simplicity, let's say we were asked to wait with 20 µs timeout.
> Assume, that the device reports ready status after 17 µs. The first
> check is done, we don't return and sleep approx. 10 µs. Then, the 2nd
> check is done and we continue to wait another 10 µs and the loop ends
> signalling a timeout. But in the meantime the device reported ready
> status. Additional check would be needed after the loop.
> In some cases the device reports ready status immediately. That's
> the reason why I check first and then loop with more wait&check cycles.

Just look to the rest of the code in kernel
Most of the timeout related loops we have the following pattern:

unsigned int retries = XXX;

do {
...check for something...
if (yes)
  return YY;

...sleep for a while...
} while (--retries);
if (!retries)
 return -ETIMEDOUT;

What I'm suggesting is to follow the pattern (adjust it for your exact
conditions and so on).

>>>>> +static struct pci_device_id altera_cvp_id_tbl[] = {
>>>>> +       { PCI_VDEVICE(ALTERA, PCI_ANY_ID) },
>>>>
>>>>Does it have dedicated PCI class?
>>>>
>>>>PCI_ANY_ID usually is too broad.
>>>
>>> no, it doesn't have dedicated class.
>>
>>Hmm... It means any device of this vendor will jump into this
>>driver... Not good.
>
> in an early patch version I was asked by Intel people to use PCI_ANY_ID
> because these devices are not set in stone. The implemented FPGA PCIe
> devices can have varying IDs. probe() checks for expected capability ID
> and stops if we hit a not supported device.

Yeah, the problem is that every device with a such Vendor ID would be
considered by this driver and PCI class would be helpful here just to
reduce an impact.
Capability approach works, though it's slightly more error prone.

I have no other comment on this. For now it seems the only choice
since such IPs are on the market, right?

-- 
With Best Regards,
Andy Shevchenko