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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <CAHp75VdEE0jFrvhcNjwTVsKDf9OMJiXRPuUb5w03LaewQDJmYw@mail.gmail.com>
Date:   Thu, 8 Jun 2017 02:38:55 +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 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.

>>> +       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.

>>> +       /* clock to data ratio for uncompressed and unencrypted images */
>>> +       conf->numclks = 1;
>>
>>To else branch of below?
>>
>>> +       if (info) {
>>
>>> +       }
>
> then, the default numclks value would have to be set in the if branch
> as well. It is easier to set it before.

See the magic below:

        u32 iflags = info ? info->flags : 0;
...
        if (iflags & FPGA_MGR_PARTIAL_RECONFIG) {
                 dev_err(&mgr->dev, "Partial reconfiguration not supported.\n");
                 return -EINVAL;
        }

        if (iflags & FPGA_MGR_COMPRESSED_BITSTREAM)
                conf->numclks = 8; /* ratio for all compressed images */
        else if (iflags & FPGA_MGR_ENCRYPTED_BITSTREAM)
                conf->numclks = 4; /* ratio for encrypted images */
        else
                conf->numclks = 1; /* clock to data ratio for
uncompressed and unencrypted images */

I would really recommend to double check the code every time you are
about to send.
A little thought can make a beauteful result!

>>> +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.

-- 
With Best Regards,
Andy Shevchenko