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.
>
> ...
>
>>> + 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.
Tom
>
>>> if (!voff) {
>>> dev_dbg(&pcidev->dev, "%s no DFL VSEC found\n", __func__);
>>> return -ENODEV;
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;
>
>
On Tue, Nov 09, 2021 at 10:51:33AM -0800, [email protected] wrote:
>
>
> 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
I agree u16 is OK.
A minor concern, is it better we also change the dfl_res_off to u16?
dfl_res_off & voff are the same type of variables needed on positioning
the DFL, so I'd like them listed together.
> 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
I agree. The vendor_id should be checked before VSEC ID is meaningful,
and now this Vendor/VSEC pair is the only supported one, so this piece of
code is good to me.
> table lookup of Vendor/VSEC id's, or do we need to reserve a more generic
> Vendor/VSEC pair?
A generic Vendor/VSEC pair means all vendors must use the unified
vendor_id if they want to use DFL. I'm not sure if this is proper.
Thanks,
Yilun
>
> >
> > Tom
> >
> > >
> > > > > if (!voff) {
> > > > > dev_dbg(&pcidev->dev, "%s no DFL VSEC found\n", __func__);
> > > > > return -ENODEV;
> >
> >
> 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.
Yes, I think that 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?
No, we don't want to maintain another table in DFL code. Let everybody apply
some new ids sounds like waste of resources too.
What about using DVSEC here? then other devices can reuse the same DVSEC
without conflicts. This is what is done for other Intel tech, e.g. Intel SIOV. But
anyway this must be captured in DFL spec firstly.
How do you guys think?
Thanks
Hao
>
> >
> > Tom
> >
> >>
> >>>> if (!voff) {
> >>>> dev_dbg(&pcidev->dev, "%s no DFL VSEC found\n", __func__);
> >>>> return -ENODEV;
> >
> >
On Tue, Nov 09, 2021 at 10:27:58AM -0800, 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:
...
> > > > + 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.
This is out of scope of this change in a few ways:
- we don't do 2+ things in one patch
- the change doesn't change behaviour
- the change is a simple cleanup
- another vendor may well have quite different VSEC ID for DFL
If you think that it should be needed, one can come up with it later on.
--
With Best Regards,
Andy Shevchenko
On 11/10/21 12:24 AM, Andy Shevchenko wrote:
> On Tue, Nov 09, 2021 at 10:27:58AM -0800, 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:
> ...
>
>>>>> + 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.
> This is out of scope of this change in a few ways:
> - we don't do 2+ things in one patch
> - the change doesn't change behaviour
> - the change is a simple cleanup
> - another vendor may well have quite different VSEC ID for DFL
>
> If you think that it should be needed, one can come up with it later on.
Fixing a problem is more useful than a cleanup. The fix should come first.
Tom
>
On Wed, Nov 10, 2021 at 2:28 PM Tom Rix <[email protected]> wrote:
> On 11/10/21 12:24 AM, Andy Shevchenko wrote:
> > On Tue, Nov 09, 2021 at 10:27:58AM -0800, 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:
> > ...
> >
> >>>>> + 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.
> > This is out of scope of this change in a few ways:
> > - we don't do 2+ things in one patch
> > - the change doesn't change behaviour
> > - the change is a simple cleanup
> > - another vendor may well have quite different VSEC ID for DFL
> >
> > If you think that it should be needed, one can come up with it later on.
>
> Fixing a problem is more useful than a cleanup. The fix should come first.
What do you mean by that? The original code never worked with what you
are suggesting. There is nothing to fix in terms of "fix". What you
are proposing is a feature. And as we know the features are going into
the kernel in a natural order, means fixes - priority 1, cleanups /
refactoring as prerequisites to the feature enabling - priority 2,
feature - priority 3, other cleanups and code improvements - priority
4.
That said, the proposed change definitely falls into category 2. It
makes the proposed feature to be easily realized.
Also, do not forget that vendor specific stuff is _by definition_
vendor specific, and the proposed feature is doubtful until you prove
there is another vendor-id pair.
--
With Best Regards,
Andy Shevchenko
On Wed, Nov 10, 2021 at 06:59:25PM +0200, Andy Shevchenko wrote:
> On Wed, Nov 10, 2021 at 2:28 PM Tom Rix <[email protected]> wrote:
> > On 11/10/21 12:24 AM, Andy Shevchenko wrote:
> > > On Tue, Nov 09, 2021 at 10:27:58AM -0800, 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:
..
> > >>>>> + 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.
> > > This is out of scope of this change in a few ways:
> > > - we don't do 2+ things in one patch
> > > - the change doesn't change behaviour
> > > - the change is a simple cleanup
> > > - another vendor may well have quite different VSEC ID for DFL
> > >
> > > If you think that it should be needed, one can come up with it later on.
> >
> > Fixing a problem is more useful than a cleanup. The fix should come first.
>
> What do you mean by that? The original code never worked with what you
> are suggesting. There is nothing to fix in terms of "fix". What you
> are proposing is a feature. And as we know the features are going into
> the kernel in a natural order, means fixes - priority 1, cleanups /
> refactoring as prerequisites to the feature enabling - priority 2,
> feature - priority 3, other cleanups and code improvements - priority
> 4.
>
> That said, the proposed change definitely falls into category 2. It
> makes the proposed feature to be easily realized.
>
> Also, do not forget that vendor specific stuff is _by definition_
> vendor specific, and the proposed feature is doubtful until you prove
> there is another vendor-id pair.
Interestingly that you included
8607d9c1bd57 ("fpga: dfl-pci: Use pci_find_vsec_capability() to simplify the code")
without even letting me know...
--
With Best Regards,
Andy Shevchenko
On Wed, Apr 03, 2024 at 02:01:25PM +0300, Andy Shevchenko wrote:
> On Wed, Nov 10, 2021 at 06:59:25PM +0200, Andy Shevchenko wrote:
> > On Wed, Nov 10, 2021 at 2:28 PM Tom Rix <[email protected]> wrote:
> > > On 11/10/21 12:24 AM, Andy Shevchenko wrote:
> > > > On Tue, Nov 09, 2021 at 10:27:58AM -0800, 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:
>
> ...
>
> > > >>>>> + 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.
> > > > This is out of scope of this change in a few ways:
> > > > - we don't do 2+ things in one patch
> > > > - the change doesn't change behaviour
> > > > - the change is a simple cleanup
> > > > - another vendor may well have quite different VSEC ID for DFL
> > > >
> > > > If you think that it should be needed, one can come up with it later on.
> > >
> > > Fixing a problem is more useful than a cleanup. The fix should come first.
> >
> > What do you mean by that? The original code never worked with what you
> > are suggesting. There is nothing to fix in terms of "fix". What you
> > are proposing is a feature. And as we know the features are going into
> > the kernel in a natural order, means fixes - priority 1, cleanups /
> > refactoring as prerequisites to the feature enabling - priority 2,
> > feature - priority 3, other cleanups and code improvements - priority
> > 4.
> >
> > That said, the proposed change definitely falls into category 2. It
> > makes the proposed feature to be easily realized.
> >
> > Also, do not forget that vendor specific stuff is _by definition_
> > vendor specific, and the proposed feature is doubtful until you prove
> > there is another vendor-id pair.
>
> Interestingly that you included
> 8607d9c1bd57 ("fpga: dfl-pci: Use pci_find_vsec_capability() to simplify the code")
> without even letting me know...
I'm sorry. Apparently I forgot what we've discussed in 2021.
In 2021, I was waiting for some more comments although I was already
good at your patch, but sadly I didn't follow up and missed it. In
2023, I was pretty sure no more comment and I could just apply.
Thanks,
Yilun
>
> --
> With Best Regards,
> Andy Shevchenko
>
>
>