| Message ID | 20211109154127.18455-1-andriy.shevchenko@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/1] fpga: dfl: pci: Use pci_find_vsec_capability() when looking for DFL | expand |
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;
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;
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, 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; > > > >
> 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.
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 <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.
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
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
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
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...
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 > > >
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 :-)
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;
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(-)