Re: [PATCH v1 1/1] fpga: dfl: pci: Use pci_find_vsec_capability() when looking for DFL

From: Andy Shevchenko <andy.shevchenko@gmail.com>
To: Tom Rix <trix@redhat.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
	Moritz Fischer <mdf@kernel.org>,
	Matthew Gerlach <matthew.gerlach@linux.intel.com>,
	linux-fpga@vger.kernel.org,
	Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
	Wu Hao <hao.wu@intel.com>, Xu Yilun <yilun.xu@intel.com>
Subject: Re: [PATCH v1 1/1] fpga: dfl: pci: Use pci_find_vsec_capability() when looking for DFL
Date: Wed, 10 Nov 2021 18:59:25 +0200	[thread overview]
Message-ID: <CAHp75Vf16mH4KQ232rip9MPLoSE1TmJ_jeiwVUzqxOH5b0RFJQ@mail.gmail.com> (raw)
In-Reply-To: <3106bd57-9144-6a4d-8ad9-3ebf804018ab@redhat.com>

On Wed, Nov 10, 2021 at 2:28 PM Tom Rix <trix@redhat.com> 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