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

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

[Andy Shevchenko]

Currently the find_dfls_by_vsec() opens code pci_find_vsec_capability().
Refactor the former to use the latter. No functional change intended.

Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
---
 drivers/fpga/dfl-pci.c | 17 +++++------------
 1 file changed, 5 insertions(+), 12 deletions(-)

diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
index 4d68719e608f..52b5e94db9c3 100644
--- a/drivers/fpga/dfl-pci.c
+++ b/drivers/fpga/dfl-pci.c
@@ -27,7 +27,7 @@
 #define DRV_VERSION	"0.8"
 #define DRV_NAME	"dfl-pci"
 
-#define PCI_VSEC_ID_INTEL_DFLS 0x43
+#define PCI_VSEC_ID_INTEL_DFLS	0x0043	/* FPGA Device Feature List */
 
 #define PCI_VNDR_DFLS_CNT 0x8
 #define PCI_VNDR_DFLS_RES 0xc
@@ -138,19 +138,12 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
 
 static int find_dfls_by_vsec(struct pci_dev *pcidev, struct dfl_fpga_enum_info *info)
 {
-	u32 bir, offset, vndr_hdr, dfl_cnt, dfl_res;
-	int dfl_res_off, i, bars, voff = 0;
+	u32 bir, offset, dfl_cnt, dfl_res;
 	resource_size_t start, len;
+	int dfl_res_off, i, bars;
+	u16 voff;
 
-	while ((voff = pci_find_next_ext_capability(pcidev, voff, PCI_EXT_CAP_ID_VNDR))) {
-		vndr_hdr = 0;
-		pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER, &vndr_hdr);
-
-		if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VSEC_ID_INTEL_DFLS &&
-		    pcidev->vendor == PCI_VENDOR_ID_INTEL)
-			break;
-	}
-
+	voff = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_DFLS);
 	if (!voff) {
 		dev_dbg(&pcidev->dev, "%s no DFL VSEC found\n", __func__);
 		return -ENODEV;
-- 
2.33.0

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

[Tom Rix]

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.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
>   drivers/fpga/dfl-pci.c | 17 +++++------------
>   1 file changed, 5 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c
> index 4d68719e608f..52b5e94db9c3 100644
> --- a/drivers/fpga/dfl-pci.c
> +++ b/drivers/fpga/dfl-pci.c
> @@ -27,7 +27,7 @@
>   #define DRV_VERSION	"0.8"
>   #define DRV_NAME	"dfl-pci"
>   
> -#define PCI_VSEC_ID_INTEL_DFLS 0x43
> +#define PCI_VSEC_ID_INTEL_DFLS	0x0043	/* FPGA Device Feature List */
>   
>   #define PCI_VNDR_DFLS_CNT 0x8
>   #define PCI_VNDR_DFLS_RES 0xc
> @@ -138,19 +138,12 @@ static int *cci_pci_create_irq_table(struct pci_dev *pcidev, unsigned int nvec)
>   
>   static int find_dfls_by_vsec(struct pci_dev *pcidev, struct dfl_fpga_enum_info *info)
>   {
> -	u32 bir, offset, vndr_hdr, dfl_cnt, dfl_res;
> -	int dfl_res_off, i, bars, voff = 0;
> +	u32 bir, offset, dfl_cnt, dfl_res;
>   	resource_size_t start, len;
> +	int dfl_res_off, i, bars;
> +	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.
>   
> -	while ((voff = pci_find_next_ext_capability(pcidev, voff, PCI_EXT_CAP_ID_VNDR))) {
> -		vndr_hdr = 0;
> -		pci_read_config_dword(pcidev, voff + PCI_VNDR_HEADER, &vndr_hdr);
> -
> -		if (PCI_VNDR_HEADER_ID(vndr_hdr) == PCI_VSEC_ID_INTEL_DFLS &&
> -		    pcidev->vendor == PCI_VENDOR_ID_INTEL)
> -			break;
> -	}
> -
> +	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.

Tom

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

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

[Andy Shevchenko]

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.

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

-- 
With Best Regards,
Andy Shevchenko

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

[Tom Rix]

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;

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

[matthew.gerlach]

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;
>
>

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

[Xu Yilun]

On Tue, Nov 09, 2021 at 10:51:33AM -0800, matthew.gerlach@linux.intel.com 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;
> > 
> > 

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

[Wu, Hao]

> 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;
> >
> >

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

[Andy Shevchenko]

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

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

[Tom Rix]

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

>

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

[Andy Shevchenko]

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

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

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

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

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

[Xu Yilun]

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

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

[Andy Shevchenko]

On Wed, Apr 03, 2024 at 11:10:31PM +0800, Xu Yilun wrote:
> 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 <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.
> > 
> > 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.

The job is done and this is good. Thank you.
One thing less to carry for me :-)

-- 
With Best Regards,
Andy Shevchenko

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

[-- Attachment #1: Type: text/plain, Size: 5170 bytes --]

Hi Andy,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.15 next-20211111]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/fpga-dfl-pci-Use-pci_find_vsec_capability-when-looking-for-DFL/20211109-234228
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git d2f38a3c6507b2520101f9a3807ed98f1bdc545a
config: i386-buildonly-randconfig-r005-20211112 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 63ef0e17e28827eae53133b3467bdac7d9729318)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/dfc10076ac7a63331954a33cabf94a1af3632210
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andy-Shevchenko/fpga-dfl-pci-Use-pci_find_vsec_capability-when-looking-for-DFL/20211109-234228
        git checkout dfc10076ac7a63331954a33cabf94a1af3632210
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/ sound/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/fpga/dfl-pci.c:146:34: error: use of undeclared identifier 'dev'
           voff = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_DFLS);
                                           ^
>> drivers/fpga/dfl-pci.c:350:32: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
           if (!pci_set_dma_mask(pcidev, DMA_BIT_MASK(64))) {
                                         ^~~~~~~~~~~~~~~~
   include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK'
   #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
                                                        ^ ~~~
   drivers/fpga/dfl-pci.c:351:45: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
                   ret = pci_set_consistent_dma_mask(pcidev, DMA_BIT_MASK(64));
                                                             ^~~~~~~~~~~~~~~~
   include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK'
   #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
                                                        ^ ~~~
   3 errors generated.


vim +/dev +146 drivers/fpga/dfl-pci.c

   138	
   139	static int find_dfls_by_vsec(struct pci_dev *pcidev, struct dfl_fpga_enum_info *info)
   140	{
   141		u32 bir, offset, dfl_cnt, dfl_res;
   142		resource_size_t start, len;
   143		int dfl_res_off, i, bars;
   144		u16 voff;
   145	
 > 146		voff = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_DFLS);
   147		if (!voff) {
   148			dev_dbg(&pcidev->dev, "%s no DFL VSEC found\n", __func__);
   149			return -ENODEV;
   150		}
   151	
   152		dfl_cnt = 0;
   153		pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT, &dfl_cnt);
   154		if (dfl_cnt > PCI_STD_NUM_BARS) {
   155			dev_err(&pcidev->dev, "%s too many DFLs %d > %d\n",
   156				__func__, dfl_cnt, PCI_STD_NUM_BARS);
   157			return -EINVAL;
   158		}
   159	
   160		dfl_res_off = voff + PCI_VNDR_DFLS_RES;
   161		if (dfl_res_off + (dfl_cnt * sizeof(u32)) > PCI_CFG_SPACE_EXP_SIZE) {
   162			dev_err(&pcidev->dev, "%s DFL VSEC too big for PCIe config space\n",
   163				__func__);
   164			return -EINVAL;
   165		}
   166	
   167		for (i = 0, bars = 0; i < dfl_cnt; i++, dfl_res_off += sizeof(u32)) {
   168			dfl_res = GENMASK(31, 0);
   169			pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
   170	
   171			bir = dfl_res & PCI_VNDR_DFLS_RES_BAR_MASK;
   172			if (bir >= PCI_STD_NUM_BARS) {
   173				dev_err(&pcidev->dev, "%s bad bir number %d\n",
   174					__func__, bir);
   175				return -EINVAL;
   176			}
   177	
   178			if (bars & BIT(bir)) {
   179				dev_err(&pcidev->dev, "%s DFL for BAR %d already specified\n",
   180					__func__, bir);
   181				return -EINVAL;
   182			}
   183	
   184			bars |= BIT(bir);
   185	
   186			len = pci_resource_len(pcidev, bir);
   187			offset = dfl_res & PCI_VNDR_DFLS_RES_OFF_MASK;
   188			if (offset >= len) {
   189				dev_err(&pcidev->dev, "%s bad offset %u >= %pa\n",
   190					__func__, offset, &len);
   191				return -EINVAL;
   192			}
   193	
   194			dev_dbg(&pcidev->dev, "%s BAR %d offset 0x%x\n", __func__, bir, offset);
   195	
   196			len -= offset;
   197	
   198			start = pci_resource_start(pcidev, bir) + offset;
   199	
   200			dfl_fpga_enum_info_add_dfl(info, start, len);
   201		}
   202	
   203		return 0;
   204	}
   205	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 50395 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 5294 bytes --]

Hi Andy,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.15 next-20211111]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/fpga-dfl-pci-Use-pci_find_vsec_capability-when-looking-for-DFL/20211109-234228
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git d2f38a3c6507b2520101f9a3807ed98f1bdc545a
config: i386-buildonly-randconfig-r005-20211112 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project 63ef0e17e28827eae53133b3467bdac7d9729318)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/dfc10076ac7a63331954a33cabf94a1af3632210
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andy-Shevchenko/fpga-dfl-pci-Use-pci_find_vsec_capability-when-looking-for-DFL/20211109-234228
        git checkout dfc10076ac7a63331954a33cabf94a1af3632210
        # save the attached .config to linux build tree
        mkdir build_dir
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/ sound/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

>> drivers/fpga/dfl-pci.c:146:34: error: use of undeclared identifier 'dev'
           voff = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_DFLS);
                                           ^
>> drivers/fpga/dfl-pci.c:350:32: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
           if (!pci_set_dma_mask(pcidev, DMA_BIT_MASK(64))) {
                                         ^~~~~~~~~~~~~~~~
   include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK'
   #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
                                                        ^ ~~~
   drivers/fpga/dfl-pci.c:351:45: error: shift count >= width of type [-Werror,-Wshift-count-overflow]
                   ret = pci_set_consistent_dma_mask(pcidev, DMA_BIT_MASK(64));
                                                             ^~~~~~~~~~~~~~~~
   include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK'
   #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
                                                        ^ ~~~
   3 errors generated.


vim +/dev +146 drivers/fpga/dfl-pci.c

   138	
   139	static int find_dfls_by_vsec(struct pci_dev *pcidev, struct dfl_fpga_enum_info *info)
   140	{
   141		u32 bir, offset, dfl_cnt, dfl_res;
   142		resource_size_t start, len;
   143		int dfl_res_off, i, bars;
   144		u16 voff;
   145	
 > 146		voff = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_DFLS);
   147		if (!voff) {
   148			dev_dbg(&pcidev->dev, "%s no DFL VSEC found\n", __func__);
   149			return -ENODEV;
   150		}
   151	
   152		dfl_cnt = 0;
   153		pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT, &dfl_cnt);
   154		if (dfl_cnt > PCI_STD_NUM_BARS) {
   155			dev_err(&pcidev->dev, "%s too many DFLs %d > %d\n",
   156				__func__, dfl_cnt, PCI_STD_NUM_BARS);
   157			return -EINVAL;
   158		}
   159	
   160		dfl_res_off = voff + PCI_VNDR_DFLS_RES;
   161		if (dfl_res_off + (dfl_cnt * sizeof(u32)) > PCI_CFG_SPACE_EXP_SIZE) {
   162			dev_err(&pcidev->dev, "%s DFL VSEC too big for PCIe config space\n",
   163				__func__);
   164			return -EINVAL;
   165		}
   166	
   167		for (i = 0, bars = 0; i < dfl_cnt; i++, dfl_res_off += sizeof(u32)) {
   168			dfl_res = GENMASK(31, 0);
   169			pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
   170	
   171			bir = dfl_res & PCI_VNDR_DFLS_RES_BAR_MASK;
   172			if (bir >= PCI_STD_NUM_BARS) {
   173				dev_err(&pcidev->dev, "%s bad bir number %d\n",
   174					__func__, bir);
   175				return -EINVAL;
   176			}
   177	
   178			if (bars & BIT(bir)) {
   179				dev_err(&pcidev->dev, "%s DFL for BAR %d already specified\n",
   180					__func__, bir);
   181				return -EINVAL;
   182			}
   183	
   184			bars |= BIT(bir);
   185	
   186			len = pci_resource_len(pcidev, bir);
   187			offset = dfl_res & PCI_VNDR_DFLS_RES_OFF_MASK;
   188			if (offset >= len) {
   189				dev_err(&pcidev->dev, "%s bad offset %u >= %pa\n",
   190					__func__, offset, &len);
   191				return -EINVAL;
   192			}
   193	
   194			dev_dbg(&pcidev->dev, "%s BAR %d offset 0x%x\n", __func__, bir, offset);
   195	
   196			len -= offset;
   197	
   198			start = pci_resource_start(pcidev, bir) + offset;
   199	
   200			dfl_fpga_enum_info_add_dfl(info, start, len);
   201		}
   202	
   203		return 0;
   204	}
   205	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 50395 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4214 bytes --]

Hi Andy,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.15 next-20211112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/fpga-dfl-pci-Use-pci_find_vsec_capability-when-looking-for-DFL/20211109-234228
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git d2f38a3c6507b2520101f9a3807ed98f1bdc545a
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/dfc10076ac7a63331954a33cabf94a1af3632210
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andy-Shevchenko/fpga-dfl-pci-Use-pci_find_vsec_capability-when-looking-for-DFL/20211109-234228
        git checkout dfc10076ac7a63331954a33cabf94a1af3632210
        # save the attached .config to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/fpga/dfl-pci.c: In function 'find_dfls_by_vsec':
>> drivers/fpga/dfl-pci.c:146:34: error: 'dev' undeclared (first use in this function); did you mean 'cdev'?
     146 |  voff = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_DFLS);
         |                                  ^~~
         |                                  cdev
   drivers/fpga/dfl-pci.c:146:34: note: each undeclared identifier is reported only once for each function it appears in


vim +146 drivers/fpga/dfl-pci.c

   138	
   139	static int find_dfls_by_vsec(struct pci_dev *pcidev, struct dfl_fpga_enum_info *info)
   140	{
   141		u32 bir, offset, dfl_cnt, dfl_res;
   142		resource_size_t start, len;
   143		int dfl_res_off, i, bars;
   144		u16 voff;
   145	
 > 146		voff = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_DFLS);
   147		if (!voff) {
   148			dev_dbg(&pcidev->dev, "%s no DFL VSEC found\n", __func__);
   149			return -ENODEV;
   150		}
   151	
   152		dfl_cnt = 0;
   153		pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT, &dfl_cnt);
   154		if (dfl_cnt > PCI_STD_NUM_BARS) {
   155			dev_err(&pcidev->dev, "%s too many DFLs %d > %d\n",
   156				__func__, dfl_cnt, PCI_STD_NUM_BARS);
   157			return -EINVAL;
   158		}
   159	
   160		dfl_res_off = voff + PCI_VNDR_DFLS_RES;
   161		if (dfl_res_off + (dfl_cnt * sizeof(u32)) > PCI_CFG_SPACE_EXP_SIZE) {
   162			dev_err(&pcidev->dev, "%s DFL VSEC too big for PCIe config space\n",
   163				__func__);
   164			return -EINVAL;
   165		}
   166	
   167		for (i = 0, bars = 0; i < dfl_cnt; i++, dfl_res_off += sizeof(u32)) {
   168			dfl_res = GENMASK(31, 0);
   169			pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
   170	
   171			bir = dfl_res & PCI_VNDR_DFLS_RES_BAR_MASK;
   172			if (bir >= PCI_STD_NUM_BARS) {
   173				dev_err(&pcidev->dev, "%s bad bir number %d\n",
   174					__func__, bir);
   175				return -EINVAL;
   176			}
   177	
   178			if (bars & BIT(bir)) {
   179				dev_err(&pcidev->dev, "%s DFL for BAR %d already specified\n",
   180					__func__, bir);
   181				return -EINVAL;
   182			}
   183	
   184			bars |= BIT(bir);
   185	
   186			len = pci_resource_len(pcidev, bir);
   187			offset = dfl_res & PCI_VNDR_DFLS_RES_OFF_MASK;
   188			if (offset >= len) {
   189				dev_err(&pcidev->dev, "%s bad offset %u >= %pa\n",
   190					__func__, offset, &len);
   191				return -EINVAL;
   192			}
   193	
   194			dev_dbg(&pcidev->dev, "%s BAR %d offset 0x%x\n", __func__, bir, offset);
   195	
   196			len -= offset;
   197	
   198			start = pci_resource_start(pcidev, bir) + offset;
   199	
   200			dfl_fpga_enum_info_add_dfl(info, start, len);
   201		}
   202	
   203		return 0;
   204	}
   205	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 66381 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 4326 bytes --]

Hi Andy,

I love your patch! Yet something to improve:

[auto build test ERROR on linus/master]
[also build test ERROR on v5.15 next-20211112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/fpga-dfl-pci-Use-pci_find_vsec_capability-when-looking-for-DFL/20211109-234228
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git d2f38a3c6507b2520101f9a3807ed98f1bdc545a
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
        # https://github.com/0day-ci/linux/commit/dfc10076ac7a63331954a33cabf94a1af3632210
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andy-Shevchenko/fpga-dfl-pci-Use-pci_find_vsec_capability-when-looking-for-DFL/20211109-234228
        git checkout dfc10076ac7a63331954a33cabf94a1af3632210
        # save the attached .config to linux build tree
        mkdir build_dir
        make W=1 O=build_dir ARCH=i386 SHELL=/bin/bash drivers/

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   drivers/fpga/dfl-pci.c: In function 'find_dfls_by_vsec':
>> drivers/fpga/dfl-pci.c:146:34: error: 'dev' undeclared (first use in this function); did you mean 'cdev'?
     146 |  voff = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_DFLS);
         |                                  ^~~
         |                                  cdev
   drivers/fpga/dfl-pci.c:146:34: note: each undeclared identifier is reported only once for each function it appears in


vim +146 drivers/fpga/dfl-pci.c

   138	
   139	static int find_dfls_by_vsec(struct pci_dev *pcidev, struct dfl_fpga_enum_info *info)
   140	{
   141		u32 bir, offset, dfl_cnt, dfl_res;
   142		resource_size_t start, len;
   143		int dfl_res_off, i, bars;
   144		u16 voff;
   145	
 > 146		voff = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_DFLS);
   147		if (!voff) {
   148			dev_dbg(&pcidev->dev, "%s no DFL VSEC found\n", __func__);
   149			return -ENODEV;
   150		}
   151	
   152		dfl_cnt = 0;
   153		pci_read_config_dword(pcidev, voff + PCI_VNDR_DFLS_CNT, &dfl_cnt);
   154		if (dfl_cnt > PCI_STD_NUM_BARS) {
   155			dev_err(&pcidev->dev, "%s too many DFLs %d > %d\n",
   156				__func__, dfl_cnt, PCI_STD_NUM_BARS);
   157			return -EINVAL;
   158		}
   159	
   160		dfl_res_off = voff + PCI_VNDR_DFLS_RES;
   161		if (dfl_res_off + (dfl_cnt * sizeof(u32)) > PCI_CFG_SPACE_EXP_SIZE) {
   162			dev_err(&pcidev->dev, "%s DFL VSEC too big for PCIe config space\n",
   163				__func__);
   164			return -EINVAL;
   165		}
   166	
   167		for (i = 0, bars = 0; i < dfl_cnt; i++, dfl_res_off += sizeof(u32)) {
   168			dfl_res = GENMASK(31, 0);
   169			pci_read_config_dword(pcidev, dfl_res_off, &dfl_res);
   170	
   171			bir = dfl_res & PCI_VNDR_DFLS_RES_BAR_MASK;
   172			if (bir >= PCI_STD_NUM_BARS) {
   173				dev_err(&pcidev->dev, "%s bad bir number %d\n",
   174					__func__, bir);
   175				return -EINVAL;
   176			}
   177	
   178			if (bars & BIT(bir)) {
   179				dev_err(&pcidev->dev, "%s DFL for BAR %d already specified\n",
   180					__func__, bir);
   181				return -EINVAL;
   182			}
   183	
   184			bars |= BIT(bir);
   185	
   186			len = pci_resource_len(pcidev, bir);
   187			offset = dfl_res & PCI_VNDR_DFLS_RES_OFF_MASK;
   188			if (offset >= len) {
   189				dev_err(&pcidev->dev, "%s bad offset %u >= %pa\n",
   190					__func__, offset, &len);
   191				return -EINVAL;
   192			}
   193	
   194			dev_dbg(&pcidev->dev, "%s BAR %d offset 0x%x\n", __func__, bir, offset);
   195	
   196			len -= offset;
   197	
   198			start = pci_resource_start(pcidev, bir) + offset;
   199	
   200			dfl_fpga_enum_info_add_dfl(info, start, len);
   201		}
   202	
   203		return 0;
   204	}
   205	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 66381 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 6102 bytes --]

Hi Andy,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.16-rc1 next-20211115]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/fpga-dfl-pci-Use-pci_find_vsec_capability-when-looking-for-DFL/20211109-234228
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git d2f38a3c6507b2520101f9a3807ed98f1bdc545a
config: i386-randconfig-a014-20211115 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project fbe72e41b99dc7994daac300d208a955be3e4a0a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/dfc10076ac7a63331954a33cabf94a1af3632210
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andy-Shevchenko/fpga-dfl-pci-Use-pci_find_vsec_capability-when-looking-for-DFL/20211109-234228
        git checkout dfc10076ac7a63331954a33cabf94a1af3632210
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/fpga/dfl-pci.c:146:34: error: use of undeclared identifier 'dev'
           voff = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_DFLS);
                                           ^
>> drivers/fpga/dfl-pci.c:350:32: warning: shift count >= width of type [-Wshift-count-overflow]
           if (!pci_set_dma_mask(pcidev, DMA_BIT_MASK(64))) {
                                         ^~~~~~~~~~~~~~~~
   include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK'
   #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
                                                        ^ ~~~
   drivers/fpga/dfl-pci.c:351:45: warning: shift count >= width of type [-Wshift-count-overflow]
                   ret = pci_set_consistent_dma_mask(pcidev, DMA_BIT_MASK(64));
                                                             ^~~~~~~~~~~~~~~~
   include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK'
   #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
                                                        ^ ~~~
   2 warnings and 1 error generated.


vim +350 drivers/fpga/dfl-pci.c

968b8199e2585a Wu Hao   2018-06-30  332  
72ddd9f34040a4 Zhang Yi 2018-06-30  333  static
72ddd9f34040a4 Zhang Yi 2018-06-30  334  int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
72ddd9f34040a4 Zhang Yi 2018-06-30  335  {
72ddd9f34040a4 Zhang Yi 2018-06-30  336  	int ret;
72ddd9f34040a4 Zhang Yi 2018-06-30  337  
72ddd9f34040a4 Zhang Yi 2018-06-30  338  	ret = pcim_enable_device(pcidev);
72ddd9f34040a4 Zhang Yi 2018-06-30  339  	if (ret < 0) {
72ddd9f34040a4 Zhang Yi 2018-06-30  340  		dev_err(&pcidev->dev, "Failed to enable device %d.\n", ret);
72ddd9f34040a4 Zhang Yi 2018-06-30  341  		return ret;
72ddd9f34040a4 Zhang Yi 2018-06-30  342  	}
72ddd9f34040a4 Zhang Yi 2018-06-30  343  
72ddd9f34040a4 Zhang Yi 2018-06-30  344  	ret = pci_enable_pcie_error_reporting(pcidev);
72ddd9f34040a4 Zhang Yi 2018-06-30  345  	if (ret && ret != -EINVAL)
72ddd9f34040a4 Zhang Yi 2018-06-30  346  		dev_info(&pcidev->dev, "PCIE AER unavailable %d.\n", ret);
72ddd9f34040a4 Zhang Yi 2018-06-30  347  
72ddd9f34040a4 Zhang Yi 2018-06-30  348  	pci_set_master(pcidev);
72ddd9f34040a4 Zhang Yi 2018-06-30  349  
72ddd9f34040a4 Zhang Yi 2018-06-30 @350  	if (!pci_set_dma_mask(pcidev, DMA_BIT_MASK(64))) {
72ddd9f34040a4 Zhang Yi 2018-06-30  351  		ret = pci_set_consistent_dma_mask(pcidev, DMA_BIT_MASK(64));
72ddd9f34040a4 Zhang Yi 2018-06-30  352  		if (ret)
72ddd9f34040a4 Zhang Yi 2018-06-30  353  			goto disable_error_report_exit;
72ddd9f34040a4 Zhang Yi 2018-06-30  354  	} else if (!pci_set_dma_mask(pcidev, DMA_BIT_MASK(32))) {
72ddd9f34040a4 Zhang Yi 2018-06-30  355  		ret = pci_set_consistent_dma_mask(pcidev, DMA_BIT_MASK(32));
72ddd9f34040a4 Zhang Yi 2018-06-30  356  		if (ret)
72ddd9f34040a4 Zhang Yi 2018-06-30  357  			goto disable_error_report_exit;
72ddd9f34040a4 Zhang Yi 2018-06-30  358  	} else {
72ddd9f34040a4 Zhang Yi 2018-06-30  359  		ret = -EIO;
72ddd9f34040a4 Zhang Yi 2018-06-30  360  		dev_err(&pcidev->dev, "No suitable DMA support available.\n");
72ddd9f34040a4 Zhang Yi 2018-06-30  361  		goto disable_error_report_exit;
72ddd9f34040a4 Zhang Yi 2018-06-30  362  	}
72ddd9f34040a4 Zhang Yi 2018-06-30  363  
968b8199e2585a Wu Hao   2018-06-30  364  	ret = cci_init_drvdata(pcidev);
968b8199e2585a Wu Hao   2018-06-30  365  	if (ret) {
968b8199e2585a Wu Hao   2018-06-30  366  		dev_err(&pcidev->dev, "Fail to init drvdata %d.\n", ret);
968b8199e2585a Wu Hao   2018-06-30  367  		goto disable_error_report_exit;
968b8199e2585a Wu Hao   2018-06-30  368  	}
968b8199e2585a Wu Hao   2018-06-30  369  
968b8199e2585a Wu Hao   2018-06-30  370  	ret = cci_enumerate_feature_devs(pcidev);
bfef946dbe1bbe Xu Yilun 2020-06-16  371  	if (!ret)
968b8199e2585a Wu Hao   2018-06-30  372  		return ret;
72ddd9f34040a4 Zhang Yi 2018-06-30  373  
bfef946dbe1bbe Xu Yilun 2020-06-16  374  	dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
bfef946dbe1bbe Xu Yilun 2020-06-16  375  
72ddd9f34040a4 Zhang Yi 2018-06-30  376  disable_error_report_exit:
72ddd9f34040a4 Zhang Yi 2018-06-30  377  	pci_disable_pcie_error_reporting(pcidev);
72ddd9f34040a4 Zhang Yi 2018-06-30  378  	return ret;
72ddd9f34040a4 Zhang Yi 2018-06-30  379  }
72ddd9f34040a4 Zhang Yi 2018-06-30  380  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 35532 bytes --]

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

[kernel test robot]

[-- Attachment #1: Type: text/plain, Size: 6206 bytes --]

Hi Andy,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linus/master]
[also build test WARNING on v5.16-rc1 next-20211115]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Andy-Shevchenko/fpga-dfl-pci-Use-pci_find_vsec_capability-when-looking-for-DFL/20211109-234228
base:   https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git d2f38a3c6507b2520101f9a3807ed98f1bdc545a
config: i386-randconfig-a014-20211115 (attached as .config)
compiler: clang version 14.0.0 (https://github.com/llvm/llvm-project fbe72e41b99dc7994daac300d208a955be3e4a0a)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/dfc10076ac7a63331954a33cabf94a1af3632210
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Andy-Shevchenko/fpga-dfl-pci-Use-pci_find_vsec_capability-when-looking-for-DFL/20211109-234228
        git checkout dfc10076ac7a63331954a33cabf94a1af3632210
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross W=1 ARCH=i386 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   drivers/fpga/dfl-pci.c:146:34: error: use of undeclared identifier 'dev'
           voff = pci_find_vsec_capability(dev, PCI_VENDOR_ID_INTEL, PCI_VSEC_ID_INTEL_DFLS);
                                           ^
>> drivers/fpga/dfl-pci.c:350:32: warning: shift count >= width of type [-Wshift-count-overflow]
           if (!pci_set_dma_mask(pcidev, DMA_BIT_MASK(64))) {
                                         ^~~~~~~~~~~~~~~~
   include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK'
   #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
                                                        ^ ~~~
   drivers/fpga/dfl-pci.c:351:45: warning: shift count >= width of type [-Wshift-count-overflow]
                   ret = pci_set_consistent_dma_mask(pcidev, DMA_BIT_MASK(64));
                                                             ^~~~~~~~~~~~~~~~
   include/linux/dma-mapping.h:76:54: note: expanded from macro 'DMA_BIT_MASK'
   #define DMA_BIT_MASK(n) (((n) == 64) ? ~0ULL : ((1ULL<<(n))-1))
                                                        ^ ~~~
   2 warnings and 1 error generated.


vim +350 drivers/fpga/dfl-pci.c

968b8199e2585a Wu Hao   2018-06-30  332  
72ddd9f34040a4 Zhang Yi 2018-06-30  333  static
72ddd9f34040a4 Zhang Yi 2018-06-30  334  int cci_pci_probe(struct pci_dev *pcidev, const struct pci_device_id *pcidevid)
72ddd9f34040a4 Zhang Yi 2018-06-30  335  {
72ddd9f34040a4 Zhang Yi 2018-06-30  336  	int ret;
72ddd9f34040a4 Zhang Yi 2018-06-30  337  
72ddd9f34040a4 Zhang Yi 2018-06-30  338  	ret = pcim_enable_device(pcidev);
72ddd9f34040a4 Zhang Yi 2018-06-30  339  	if (ret < 0) {
72ddd9f34040a4 Zhang Yi 2018-06-30  340  		dev_err(&pcidev->dev, "Failed to enable device %d.\n", ret);
72ddd9f34040a4 Zhang Yi 2018-06-30  341  		return ret;
72ddd9f34040a4 Zhang Yi 2018-06-30  342  	}
72ddd9f34040a4 Zhang Yi 2018-06-30  343  
72ddd9f34040a4 Zhang Yi 2018-06-30  344  	ret = pci_enable_pcie_error_reporting(pcidev);
72ddd9f34040a4 Zhang Yi 2018-06-30  345  	if (ret && ret != -EINVAL)
72ddd9f34040a4 Zhang Yi 2018-06-30  346  		dev_info(&pcidev->dev, "PCIE AER unavailable %d.\n", ret);
72ddd9f34040a4 Zhang Yi 2018-06-30  347  
72ddd9f34040a4 Zhang Yi 2018-06-30  348  	pci_set_master(pcidev);
72ddd9f34040a4 Zhang Yi 2018-06-30  349  
72ddd9f34040a4 Zhang Yi 2018-06-30 @350  	if (!pci_set_dma_mask(pcidev, DMA_BIT_MASK(64))) {
72ddd9f34040a4 Zhang Yi 2018-06-30  351  		ret = pci_set_consistent_dma_mask(pcidev, DMA_BIT_MASK(64));
72ddd9f34040a4 Zhang Yi 2018-06-30  352  		if (ret)
72ddd9f34040a4 Zhang Yi 2018-06-30  353  			goto disable_error_report_exit;
72ddd9f34040a4 Zhang Yi 2018-06-30  354  	} else if (!pci_set_dma_mask(pcidev, DMA_BIT_MASK(32))) {
72ddd9f34040a4 Zhang Yi 2018-06-30  355  		ret = pci_set_consistent_dma_mask(pcidev, DMA_BIT_MASK(32));
72ddd9f34040a4 Zhang Yi 2018-06-30  356  		if (ret)
72ddd9f34040a4 Zhang Yi 2018-06-30  357  			goto disable_error_report_exit;
72ddd9f34040a4 Zhang Yi 2018-06-30  358  	} else {
72ddd9f34040a4 Zhang Yi 2018-06-30  359  		ret = -EIO;
72ddd9f34040a4 Zhang Yi 2018-06-30  360  		dev_err(&pcidev->dev, "No suitable DMA support available.\n");
72ddd9f34040a4 Zhang Yi 2018-06-30  361  		goto disable_error_report_exit;
72ddd9f34040a4 Zhang Yi 2018-06-30  362  	}
72ddd9f34040a4 Zhang Yi 2018-06-30  363  
968b8199e2585a Wu Hao   2018-06-30  364  	ret = cci_init_drvdata(pcidev);
968b8199e2585a Wu Hao   2018-06-30  365  	if (ret) {
968b8199e2585a Wu Hao   2018-06-30  366  		dev_err(&pcidev->dev, "Fail to init drvdata %d.\n", ret);
968b8199e2585a Wu Hao   2018-06-30  367  		goto disable_error_report_exit;
968b8199e2585a Wu Hao   2018-06-30  368  	}
968b8199e2585a Wu Hao   2018-06-30  369  
968b8199e2585a Wu Hao   2018-06-30  370  	ret = cci_enumerate_feature_devs(pcidev);
bfef946dbe1bbe Xu Yilun 2020-06-16  371  	if (!ret)
968b8199e2585a Wu Hao   2018-06-30  372  		return ret;
72ddd9f34040a4 Zhang Yi 2018-06-30  373  
bfef946dbe1bbe Xu Yilun 2020-06-16  374  	dev_err(&pcidev->dev, "enumeration failure %d.\n", ret);
bfef946dbe1bbe Xu Yilun 2020-06-16  375  
72ddd9f34040a4 Zhang Yi 2018-06-30  376  disable_error_report_exit:
72ddd9f34040a4 Zhang Yi 2018-06-30  377  	pci_disable_pcie_error_reporting(pcidev);
72ddd9f34040a4 Zhang Yi 2018-06-30  378  	return ret;
72ddd9f34040a4 Zhang Yi 2018-06-30  379  }
72ddd9f34040a4 Zhang Yi 2018-06-30  380  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 35532 bytes --]