Message ID | 1594282337-32125-2-git-send-email-yilun.xu@intel.com (mailing list archive) |
---|---|
State | Mainlined, archived |
Headers | show |
Series | Bug fixes for FPGA DFL | expand |
I think a better change is to use the ret variable, like this --- a/drivers/fpga/dfl-pci.c +++ b/drivers/fpga/dfl-pci.c @@ -312,7 +312,7 @@ static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs) } } - return num_vfs; + return ret; } The existing use of returning num_vfs is not right, the function should return 0/err not num_vfs. currently it is reusing the 0 passed in with num_vfs to mean disable as the 0 return status. it should be properly returning ret. Tom On 7/9/20 1:12 AM, Xu Yilun wrote: > This is to fix lkp cppcheck warnings: > > drivers/fpga/dfl-pci.c:230:6: warning: The scope of the variable 'ret' can be reduced. [variableScope] > int ret = 0; > ^ > > drivers/fpga/dfl-pci.c:230:10: warning: Variable 'ret' is assigned a value that is never used. [unreadVariable] > int ret = 0; > ^ > > Fixes: 3c2760b78f90 ("fpga: dfl: pci: fix return value of cci_pci_sriov_configure") > Reported-by: kbuild test robot <lkp@intel.com> > Signed-off-by: Xu Yilun <yilun.xu@intel.com> > Acked-by: Wu Hao <hao.wu@intel.com> > --- > drivers/fpga/dfl-pci.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c > index 4a14a24..73b5153 100644 > --- a/drivers/fpga/dfl-pci.c > +++ b/drivers/fpga/dfl-pci.c > @@ -285,7 +285,6 @@ static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs) > { > struct cci_drvdata *drvdata = pci_get_drvdata(pcidev); > struct dfl_fpga_cdev *cdev = drvdata->cdev; > - int ret = 0; > > if (!num_vfs) { > /* > @@ -297,6 +296,8 @@ static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs) > dfl_fpga_cdev_config_ports_pf(cdev); > > } else { > + int ret; > + > /* > * before enable SRIOV, put released ports into VF access mode > * first of all.
On Thu, Jul 09, 2020 at 06:18:18AM -0700, Tom Rix wrote: > I think a better change is to use the ret variable, like this > > --- a/drivers/fpga/dfl-pci.c > +++ b/drivers/fpga/dfl-pci.c > @@ -312,7 +312,7 @@ static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs) > } > } > > - return num_vfs; > + return ret; > } > > The existing use of returning num_vfs is not right, the function should return 0/err not num_vfs. currently it is reusing the 0 passed in with num_vfs to mean disable as the 0 return status. it should be properly returning ret. The sriov_configure callback should return negative value for error, and return num_vfs if success. See the Documentation/PCI/pci-iov-howto.rst also in drivers/pci/iov.c: static ssize_t sriov_numvfs_store(struct device *dev, ...) { ... ret = pdev->driver->sriov_configure(pdev, num_vfs); if (ret < 0) goto exit; if (ret != num_vfs) pci_warn(pdev, "%d VFs requested; only %d enabled\n", num_vfs, ret); ... } > > Tom > > On 7/9/20 1:12 AM, Xu Yilun wrote: > > This is to fix lkp cppcheck warnings: > > > > drivers/fpga/dfl-pci.c:230:6: warning: The scope of the variable 'ret' can be reduced. [variableScope] > > int ret = 0; > > ^ > > > > drivers/fpga/dfl-pci.c:230:10: warning: Variable 'ret' is assigned a value that is never used. [unreadVariable] > > int ret = 0; > > ^ > > > > Fixes: 3c2760b78f90 ("fpga: dfl: pci: fix return value of cci_pci_sriov_configure") > > Reported-by: kbuild test robot <lkp@intel.com> > > Signed-off-by: Xu Yilun <yilun.xu@intel.com> > > Acked-by: Wu Hao <hao.wu@intel.com> > > --- > > drivers/fpga/dfl-pci.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c > > index 4a14a24..73b5153 100644 > > --- a/drivers/fpga/dfl-pci.c > > +++ b/drivers/fpga/dfl-pci.c > > @@ -285,7 +285,6 @@ static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs) > > { > > struct cci_drvdata *drvdata = pci_get_drvdata(pcidev); > > struct dfl_fpga_cdev *cdev = drvdata->cdev; > > - int ret = 0; > > > > if (!num_vfs) { > > /* > > @@ -297,6 +296,8 @@ static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs) > > dfl_fpga_cdev_config_ports_pf(cdev); > > > > } else { > > + int ret; > > + > > /* > > * before enable SRIOV, put released ports into VF access mode > > * first of all.
Ok, i take your point on the api. However I still feel that this change should be improved so the return is not defined inside a block. I think this could be done better with int ret = num_vfs; ... return ret; There is no consistency on this check with the other drivers' sriov_configure. I do not feel strongly about this change, your original change will work. So you may add. Reviewed-by: Tom Rix <trix@redhat.com> Tom On 7/9/20 10:16 PM, Xu Yilun wrote: > On Thu, Jul 09, 2020 at 06:18:18AM -0700, Tom Rix wrote: >> I think a better change is to use the ret variable, like this >> >> --- a/drivers/fpga/dfl-pci.c >> +++ b/drivers/fpga/dfl-pci.c >> @@ -312,7 +312,7 @@ static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs) >>                } >>        } >>  >> -      return num_vfs; >> +      return ret; >>  } >> >> The existing use of returning num_vfs is not right, the function should return 0/err not num_vfs. currently it is reusing the 0 passed in with num_vfs to mean disable as the 0 return status. it should be properly returning ret. > The sriov_configure callback should return negative value for error, and > return num_vfs if success. > > See the Documentation/PCI/pci-iov-howto.rst > > also in drivers/pci/iov.c: > > static ssize_t sriov_numvfs_store(struct device *dev, ...) > { > ... > > ret = pdev->driver->sriov_configure(pdev, num_vfs); > if (ret < 0) > goto exit; > > if (ret != num_vfs) > pci_warn(pdev, "%d VFs requested; only %d enabled\n", > num_vfs, ret); > > ... > } > >> Tom >> >> On 7/9/20 1:12 AM, Xu Yilun wrote: >>> This is to fix lkp cppcheck warnings: >>> >>> drivers/fpga/dfl-pci.c:230:6: warning: The scope of the variable 'ret' can be reduced. [variableScope] >>> int ret = 0; >>> ^ >>> >>> drivers/fpga/dfl-pci.c:230:10: warning: Variable 'ret' is assigned a value that is never used. [unreadVariable] >>> int ret = 0; >>> ^ >>> >>> Fixes: 3c2760b78f90 ("fpga: dfl: pci: fix return value of cci_pci_sriov_configure") >>> Reported-by: kbuild test robot <lkp@intel.com> >>> Signed-off-by: Xu Yilun <yilun.xu@intel.com> >>> Acked-by: Wu Hao <hao.wu@intel.com> >>> --- >>> drivers/fpga/dfl-pci.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c >>> index 4a14a24..73b5153 100644 >>> --- a/drivers/fpga/dfl-pci.c >>> +++ b/drivers/fpga/dfl-pci.c >>> @@ -285,7 +285,6 @@ static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs) >>> { >>> struct cci_drvdata *drvdata = pci_get_drvdata(pcidev); >>> struct dfl_fpga_cdev *cdev = drvdata->cdev; >>> - int ret = 0; >>> >>> if (!num_vfs) { >>> /* >>> @@ -297,6 +296,8 @@ static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs) >>> dfl_fpga_cdev_config_ports_pf(cdev); >>> >>> } else { >>> + int ret; >>> + >>> /* >>> * before enable SRIOV, put released ports into VF access mode >>> * first of all.
diff --git a/drivers/fpga/dfl-pci.c b/drivers/fpga/dfl-pci.c index 4a14a24..73b5153 100644 --- a/drivers/fpga/dfl-pci.c +++ b/drivers/fpga/dfl-pci.c @@ -285,7 +285,6 @@ static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs) { struct cci_drvdata *drvdata = pci_get_drvdata(pcidev); struct dfl_fpga_cdev *cdev = drvdata->cdev; - int ret = 0; if (!num_vfs) { /* @@ -297,6 +296,8 @@ static int cci_pci_sriov_configure(struct pci_dev *pcidev, int num_vfs) dfl_fpga_cdev_config_ports_pf(cdev); } else { + int ret; + /* * before enable SRIOV, put released ports into VF access mode * first of all.