lkml.org 
[lkml]   [2021]   [Nov]   [9]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v1 1/1] fpga: dfl: pci: Use pci_find_vsec_capability() when looking for DFL


On Tue, 9 Nov 2021, Tom Rix wrote:

>
> On 11/9/21 10:05 AM, Andy Shevchenko wrote:
>> On Tue, Nov 09, 2021 at 07:55:43AM -0800, Tom Rix wrote:
>>> On 11/9/21 7:41 AM, Andy Shevchenko wrote:
>>>> Currently the find_dfls_by_vsec() opens code pci_find_vsec_capability().
>>>> Refactor the former to use the latter. No functional change intended.
>> Thanks for review, my answers below.
>>
>> ...
>>
>>>> + u16 voff;
>>> The later use of voff in pci_read_config_dword is of type 'int', it may be
>>> better to keep voff as an int.
>> I don't think so. The rule of thumb that the types should match the value
>> they
>> got in the first place. In this case it's u16. Compiler will implicitly
>> cast it
>> to whatever is needed as long as the type is good for integer promotion.
>>

I think u16 is more precise than int, but I think it'll get promoted to an
int anywhen when used with calls to pci_read_config_dword(). Was this
change tested on real or emulated HW?

>> ...
>>
>>>> + voff = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL,
>>>> PCI_VSEC_ID_INTEL_DFLS);
>>> This may be a weakness in the origin code, but intel isn't the exclusive
>>> user of DFL.
>> This does not change the original code. If you think so, this can be
>> extended
>> later on.
>
> I would rather see this fixed now or explained why this isn't a problem.

I agree that a single Vendor/VSEC id being supported is a problem,
but I think fixing it should be a separate patch. Do we need to change
this a table lookup of Vendor/VSEC id's, or do we need to reserve a more
generic Vendor/VSEC pair?

>
> Tom
>
>>
>>>> if (!voff) {
>>>> dev_dbg(&pcidev->dev, "%s no DFL VSEC found\n", __func__);
>>>> return -ENODEV;
>
>

\
 
 \ /
  Last update: 2021-11-09 19:49    [W:0.137 / U:0.144 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site