* [RESEND PATCH 0/2] Bug fixes for FPGA DFL @ 2020-07-09 8:12 Xu Yilun 2020-07-09 8:12 ` [RESEND PATCH 1/2] fpga: dfl: pci: reduce the scope of variable 'ret' Xu Yilun 2020-07-09 8:12 ` [RESEND PATCH 2/2] fpga: dfl: fix bug in port reset handshake Xu Yilun 0 siblings, 2 replies; 7+ messages in thread From: Xu Yilun @ 2020-07-09 8:12 UTC (permalink / raw) To: mdf, linux-fpga, linux-kernel; +Cc: trix, lgoncalv, Xu Yilun Resend these 2 fix patches since the to-be-fixed patches have been merged to mainline. Matthew Gerlach (1): fpga: dfl: fix bug in port reset handshake Xu Yilun (1): fpga: dfl: pci: reduce the scope of variable 'ret' drivers/fpga/dfl-afu-main.c | 3 ++- drivers/fpga/dfl-pci.c | 3 ++- 2 files changed, 4 insertions(+), 2 deletions(-) -- 2.7.4 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [RESEND PATCH 1/2] fpga: dfl: pci: reduce the scope of variable 'ret' 2020-07-09 8:12 [RESEND PATCH 0/2] Bug fixes for FPGA DFL Xu Yilun @ 2020-07-09 8:12 ` Xu Yilun 2020-07-09 13:18 ` Tom Rix 2020-07-09 8:12 ` [RESEND PATCH 2/2] fpga: dfl: fix bug in port reset handshake Xu Yilun 1 sibling, 1 reply; 7+ messages in thread From: Xu Yilun @ 2020-07-09 8:12 UTC (permalink / raw) To: mdf, linux-fpga, linux-kernel; +Cc: trix, lgoncalv, Xu Yilun 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. -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RESEND PATCH 1/2] fpga: dfl: pci: reduce the scope of variable 'ret' 2020-07-09 8:12 ` [RESEND PATCH 1/2] fpga: dfl: pci: reduce the scope of variable 'ret' Xu Yilun @ 2020-07-09 13:18 ` Tom Rix 2020-07-10 5:16 ` Xu Yilun 0 siblings, 1 reply; 7+ messages in thread From: Tom Rix @ 2020-07-09 13:18 UTC (permalink / raw) To: Xu Yilun, mdf, linux-fpga, linux-kernel; +Cc: lgoncalv 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RESEND PATCH 1/2] fpga: dfl: pci: reduce the scope of variable 'ret' 2020-07-09 13:18 ` Tom Rix @ 2020-07-10 5:16 ` Xu Yilun 2020-07-11 16:21 ` Tom Rix 0 siblings, 1 reply; 7+ messages in thread From: Xu Yilun @ 2020-07-10 5:16 UTC (permalink / raw) To: Tom Rix; +Cc: mdf, linux-fpga, linux-kernel, lgoncalv 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [RESEND PATCH 1/2] fpga: dfl: pci: reduce the scope of variable 'ret' 2020-07-10 5:16 ` Xu Yilun @ 2020-07-11 16:21 ` Tom Rix 0 siblings, 0 replies; 7+ messages in thread From: Tom Rix @ 2020-07-11 16:21 UTC (permalink / raw) To: Xu Yilun; +Cc: mdf, linux-fpga, linux-kernel, lgoncalv 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* [RESEND PATCH 2/2] fpga: dfl: fix bug in port reset handshake 2020-07-09 8:12 [RESEND PATCH 0/2] Bug fixes for FPGA DFL Xu Yilun 2020-07-09 8:12 ` [RESEND PATCH 1/2] fpga: dfl: pci: reduce the scope of variable 'ret' Xu Yilun @ 2020-07-09 8:12 ` Xu Yilun 2020-07-09 13:20 ` Tom Rix 1 sibling, 1 reply; 7+ messages in thread From: Xu Yilun @ 2020-07-09 8:12 UTC (permalink / raw) To: mdf, linux-fpga, linux-kernel; +Cc: trix, lgoncalv, Matthew Gerlach, Xu Yilun From: Matthew Gerlach <matthew.gerlach@linux.intel.com> When putting the port in reset, driver must wait for the soft reset acknowledgment bit instead of the soft reset bit. Fixes: 47c1b19c160f (fpga: dfl: afu: add port ops support) Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> Signed-off-by: Xu Yilun <yilun.xu@intel.com> Acked-by: Wu Hao <hao.wu@intel.com> --- drivers/fpga/dfl-afu-main.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c index 7c84fee..753cda4 100644 --- a/drivers/fpga/dfl-afu-main.c +++ b/drivers/fpga/dfl-afu-main.c @@ -83,7 +83,8 @@ int __afu_port_disable(struct platform_device *pdev) * on this port and minimum soft reset pulse width has elapsed. * Driver polls port_soft_reset_ack to determine if reset done by HW. */ - if (readq_poll_timeout(base + PORT_HDR_CTRL, v, v & PORT_CTRL_SFTRST, + if (readq_poll_timeout(base + PORT_HDR_CTRL, v, + v & PORT_CTRL_SFTRST_ACK, RST_POLL_INVL, RST_POLL_TIMEOUT)) { dev_err(&pdev->dev, "timeout, fail to reset device\n"); return -ETIMEDOUT; -- 2.7.4 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [RESEND PATCH 2/2] fpga: dfl: fix bug in port reset handshake 2020-07-09 8:12 ` [RESEND PATCH 2/2] fpga: dfl: fix bug in port reset handshake Xu Yilun @ 2020-07-09 13:20 ` Tom Rix 0 siblings, 0 replies; 7+ messages in thread From: Tom Rix @ 2020-07-09 13:20 UTC (permalink / raw) To: Xu Yilun, mdf, linux-fpga, linux-kernel; +Cc: lgoncalv, Matthew Gerlach On 7/9/20 1:12 AM, Xu Yilun wrote: > From: Matthew Gerlach <matthew.gerlach@linux.intel.com> > > When putting the port in reset, driver must wait for the soft reset > acknowledgment bit instead of the soft reset bit. > > Fixes: 47c1b19c160f (fpga: dfl: afu: add port ops support) > Signed-off-by: Matthew Gerlach <matthew.gerlach@linux.intel.com> > Signed-off-by: Xu Yilun <yilun.xu@intel.com> > Acked-by: Wu Hao <hao.wu@intel.com> > --- > drivers/fpga/dfl-afu-main.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/fpga/dfl-afu-main.c b/drivers/fpga/dfl-afu-main.c > index 7c84fee..753cda4 100644 > --- a/drivers/fpga/dfl-afu-main.c > +++ b/drivers/fpga/dfl-afu-main.c > @@ -83,7 +83,8 @@ int __afu_port_disable(struct platform_device *pdev) > * on this port and minimum soft reset pulse width has elapsed. > * Driver polls port_soft_reset_ack to determine if reset done by HW. > */ > - if (readq_poll_timeout(base + PORT_HDR_CTRL, v, v & PORT_CTRL_SFTRST, > + if (readq_poll_timeout(base + PORT_HDR_CTRL, v, > + v & PORT_CTRL_SFTRST_ACK, > RST_POLL_INVL, RST_POLL_TIMEOUT)) { > dev_err(&pdev->dev, "timeout, fail to reset device\n"); > return -ETIMEDOUT; Looks ok to me. Reviewed-by: Tom Rix <trix@redhat.com> ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-07-11 16:21 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-07-09 8:12 [RESEND PATCH 0/2] Bug fixes for FPGA DFL Xu Yilun 2020-07-09 8:12 ` [RESEND PATCH 1/2] fpga: dfl: pci: reduce the scope of variable 'ret' Xu Yilun 2020-07-09 13:18 ` Tom Rix 2020-07-10 5:16 ` Xu Yilun 2020-07-11 16:21 ` Tom Rix 2020-07-09 8:12 ` [RESEND PATCH 2/2] fpga: dfl: fix bug in port reset handshake Xu Yilun 2020-07-09 13:20 ` Tom Rix
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).