* [Intel-wired-lan] [PATCH linux-next] e1000: Remove redundant statement @ 2021-10-18 8:53 ` luo penghao 0 siblings, 0 replies; 9+ messages in thread From: luo penghao @ 2021-10-18 8:53 UTC (permalink / raw) To: intel-wired-lan This statement is redundant in the context, because there will be an identical statement next. otherwise, the variable initialization is actually unnecessary. The clang_analyzer complains as follows: drivers/net/ethernet/intel/e1000/e1000_ethtool.c:1218:2 warning: Value stored to 'ctrl_reg' is never read. Reported-by: Zeal Robot <zealci@zte.com.cn> Signed-off-by: luo penghao <luo.penghao@zte.com.cn> --- drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c index 0a57172..8951f2a 100644 --- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c +++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c @@ -1215,8 +1215,6 @@ static int e1000_integrated_phy_loopback(struct e1000_adapter *adapter) e1000_write_phy_reg(hw, PHY_CTRL, 0x8140); } - ctrl_reg = er32(CTRL); - /* force 1000, set loopback */ e1000_write_phy_reg(hw, PHY_CTRL, 0x4140); -- 2.15.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH linux-next] e1000: Remove redundant statement @ 2021-10-18 8:53 ` luo penghao 0 siblings, 0 replies; 9+ messages in thread From: luo penghao @ 2021-10-18 8:53 UTC (permalink / raw) To: Jesse Brandeburg Cc: Tony Nguyen, David S . Miller, Jakub Kicinski, intel-wired-lan, netdev, linux-kernel, luo penghao, Zeal Robot This statement is redundant in the context, because there will be an identical statement next. otherwise, the variable initialization is actually unnecessary. The clang_analyzer complains as follows: drivers/net/ethernet/intel/e1000/e1000_ethtool.c:1218:2 warning: Value stored to 'ctrl_reg' is never read. Reported-by: Zeal Robot <zealci@zte.com.cn> Signed-off-by: luo penghao <luo.penghao@zte.com.cn> --- drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 -- 1 file changed, 2 deletions(-) diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c index 0a57172..8951f2a 100644 --- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c +++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c @@ -1215,8 +1215,6 @@ static int e1000_integrated_phy_loopback(struct e1000_adapter *adapter) e1000_write_phy_reg(hw, PHY_CTRL, 0x8140); } - ctrl_reg = er32(CTRL); - /* force 1000, set loopback */ e1000_write_phy_reg(hw, PHY_CTRL, 0x4140); -- 2.15.2 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [Intel-wired-lan] [PATCH linux-next] e1000: Remove redundant statement 2021-10-18 8:53 ` luo penghao (?) @ 2021-10-18 16:10 ` jesse.brandeburg 2021-10-20 17:51 ` Jesse Brandeburg -1 siblings, 1 reply; 9+ messages in thread From: jesse.brandeburg @ 2021-10-18 16:10 UTC (permalink / raw) To: intel-wired-lan On 10/18/21 1:53 AM, luo penghao <cgel.zte@gmail.com> wrote: > This statement is redundant in the context, because there will be > an identical statement next. otherwise, the variable initialization > is actually unnecessary. > > The clang_analyzer complains as follows: > > drivers/net/ethernet/intel/e1000/e1000_ethtool.c:1218:2 warning: > > Value stored to 'ctrl_reg' is never read. > > Reported-by: Zeal Robot <zealci@zte.com.cn> > Signed-off-by: luo penghao <luo.penghao@zte.com.cn> > --- > drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c > index 0a57172..8951f2a 100644 > --- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c > +++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c > @@ -1215,8 +1215,6 @@ static int e1000_integrated_phy_loopback(struct e1000_adapter *adapter) > e1000_write_phy_reg(hw, PHY_CTRL, 0x8140); > } > > - ctrl_reg = er32(CTRL); Thanks for your patch, but this change is not safe. you're removing a read that could do two things. The first is that the read "flushes" the write just above to PCI (it's a PCI barrier), and the second is that the read can have some side effects. If this change must be done, the code should be to remove the assignment to ctrl_reg, but leave the read, so the line would just look like: er32(CTRL); This will get rid of the warning and not change the flow from the hardware perspective. > - > /* force 1000, set loopback */ > e1000_write_phy_reg(hw, PHY_CTRL, 0x4140); > > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-wired-lan] [PATCH linux-next] e1000: Remove redundant statement 2021-10-18 16:10 ` [Intel-wired-lan] " jesse.brandeburg @ 2021-10-20 17:51 ` Jesse Brandeburg 0 siblings, 0 replies; 9+ messages in thread From: Jesse Brandeburg @ 2021-10-20 17:51 UTC (permalink / raw) To: intel-wired-lan On 10/18/2021 9:10 AM, jesse.brandeburg at intel.com wrote: > On 10/18/21 1:53 AM, luo penghao <cgel.zte@gmail.com> wrote: >> This statement is redundant in the context, because there will be >> an identical statement next. otherwise, the variable initialization >> is actually unnecessary. >> >> The clang_analyzer complains as follows: >> >> drivers/net/ethernet/intel/e1000/e1000_ethtool.c:1218:2 warning: >> >> Value stored to 'ctrl_reg' is never read. >> >> Reported-by: Zeal Robot <zealci@zte.com.cn> >> Signed-off-by: luo penghao <luo.penghao@zte.com.cn> >> --- >> ? drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 -- >> ? 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c >> b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c >> index 0a57172..8951f2a 100644 >> --- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c >> +++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c >> @@ -1215,8 +1215,6 @@ static int e1000_integrated_phy_loopback(struct >> e1000_adapter *adapter) >> ????????? e1000_write_phy_reg(hw, PHY_CTRL, 0x8140); >> ????? } >> -??? ctrl_reg = er32(CTRL); > Please see my reply below. > Thanks for your patch, but this change is not safe. you're removing a > read that could do two things. The first is that the read "flushes" the > write just above to PCI (it's a PCI barrier), and the second is that the > read can have some side effects. > > If this change must be done, the code should be to remove the assignment > to ctrl_reg, but leave the read, so the line would just look like: > ?????? er32(CTRL); > > This will get rid of the warning and not change the flow from the > hardware perspective. > >> - >> ????? /* force 1000, set loopback */ >> ????? e1000_write_phy_reg(hw, PHY_CTRL, 0x4140); my previous mail seems to have not made it to netdev/patchwork, so re-sending. NAK. Please respin if you must make this change. ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-wired-lan] [PATCH linux-next] e1000: Remove redundant statement 2021-10-18 8:53 ` luo penghao @ 2021-10-20 9:25 ` Simon Horman -1 siblings, 0 replies; 9+ messages in thread From: Simon Horman @ 2021-10-20 9:25 UTC (permalink / raw) To: intel-wired-lan On Mon, Oct 18, 2021 at 08:53:05AM +0000, luo penghao wrote: nit: s/linux-next/net-next/ in subject > This statement is redundant in the context, because there will be > an identical statement next. otherwise, the variable initialization > is actually unnecessary. > > The clang_analyzer complains as follows: > > drivers/net/ethernet/intel/e1000/e1000_ethtool.c:1218:2 warning: > > Value stored to 'ctrl_reg' is never read. I agree this does seem to be the case. > Reported-by: Zeal Robot <zealci@zte.com.cn> > Signed-off-by: luo penghao <luo.penghao@zte.com.cn> Reviewed-by: Simon Horman <horms@kernel.org> > --- > drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c > index 0a57172..8951f2a 100644 > --- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c > +++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c > @@ -1215,8 +1215,6 @@ static int e1000_integrated_phy_loopback(struct e1000_adapter *adapter) > e1000_write_phy_reg(hw, PHY_CTRL, 0x8140); > } > > - ctrl_reg = er32(CTRL); > - > /* force 1000, set loopback */ > e1000_write_phy_reg(hw, PHY_CTRL, 0x4140); > > -- > 2.15.2 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH linux-next] e1000: Remove redundant statement @ 2021-10-20 9:25 ` Simon Horman 0 siblings, 0 replies; 9+ messages in thread From: Simon Horman @ 2021-10-20 9:25 UTC (permalink / raw) To: luo penghao Cc: Jesse Brandeburg, Tony Nguyen, David S . Miller, Jakub Kicinski, intel-wired-lan, netdev, linux-kernel, luo penghao, Zeal Robot On Mon, Oct 18, 2021 at 08:53:05AM +0000, luo penghao wrote: nit: s/linux-next/net-next/ in subject > This statement is redundant in the context, because there will be > an identical statement next. otherwise, the variable initialization > is actually unnecessary. > > The clang_analyzer complains as follows: > > drivers/net/ethernet/intel/e1000/e1000_ethtool.c:1218:2 warning: > > Value stored to 'ctrl_reg' is never read. I agree this does seem to be the case. > Reported-by: Zeal Robot <zealci@zte.com.cn> > Signed-off-by: luo penghao <luo.penghao@zte.com.cn> Reviewed-by: Simon Horman <horms@kernel.org> > --- > drivers/net/ethernet/intel/e1000/e1000_ethtool.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c > index 0a57172..8951f2a 100644 > --- a/drivers/net/ethernet/intel/e1000/e1000_ethtool.c > +++ b/drivers/net/ethernet/intel/e1000/e1000_ethtool.c > @@ -1215,8 +1215,6 @@ static int e1000_integrated_phy_loopback(struct e1000_adapter *adapter) > e1000_write_phy_reg(hw, PHY_CTRL, 0x8140); > } > > - ctrl_reg = er32(CTRL); > - > /* force 1000, set loopback */ > e1000_write_phy_reg(hw, PHY_CTRL, 0x4140); > > -- > 2.15.2 > > ^ permalink raw reply [flat|nested] 9+ messages in thread
* [Intel-wired-lan] [PATCH linux-next] e1000: Remove redundant statement 2021-10-20 9:25 ` Simon Horman @ 2021-10-20 18:08 ` Jesse Brandeburg -1 siblings, 0 replies; 9+ messages in thread From: Jesse Brandeburg @ 2021-10-20 18:08 UTC (permalink / raw) To: intel-wired-lan Apologies for the duplicates, mail from my intel account going out through outlook.com is not being delivered. On Wed, Oct 20, 2021 at 7:00 AM Simon Horman <horms@kernel.org> wrote: > > Value stored to 'ctrl_reg' is never read. > > I agree this does seem to be the case. > > > Reported-by: Zeal Robot <zealci@zte.com.cn> > > Signed-off-by: luo penghao <luo.penghao@zte.com.cn> > > Reviewed-by: Simon Horman <horms@kernel.org> Thanks for the review, but (davem/kuba) please do not apply. > > @@ -1215,8 +1215,6 @@ static int e1000_integrated_phy_loopback(struct e1000_adapter *adapter) > > e1000_write_phy_reg(hw, PHY_CTRL, 0x8140); > > } > > > > - ctrl_reg = er32(CTRL); Thanks for your patch, but this change is not safe. you're removing a read that could do two things. The first is that the read "flushes" the write just above to PCI (it's a PCI barrier), and the second is that the read can have some side effects. If this change must be done, the code should be to remove the assignment to ctrl_reg, but leave the read, so the line would just look like: er32(CTRL); This will get rid of the warning and not change the flow from the hardware perspective. > > - > > /* force 1000, set loopback */ > > e1000_write_phy_reg(hw, PHY_CTRL, 0x4140); > > Please do not apply this. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Intel-wired-lan] [PATCH linux-next] e1000: Remove redundant statement @ 2021-10-20 18:08 ` Jesse Brandeburg 0 siblings, 0 replies; 9+ messages in thread From: Jesse Brandeburg @ 2021-10-20 18:08 UTC (permalink / raw) To: Simon Horman Cc: luo penghao, NetDEV list, Zeal Robot, linux-kernel@vger.kernel.org, intel-wired-lan, Jakub Kicinski, luo penghao, David S . Miller Apologies for the duplicates, mail from my intel account going out through outlook.com is not being delivered. On Wed, Oct 20, 2021 at 7:00 AM Simon Horman <horms@kernel.org> wrote: > > Value stored to 'ctrl_reg' is never read. > > I agree this does seem to be the case. > > > Reported-by: Zeal Robot <zealci@zte.com.cn> > > Signed-off-by: luo penghao <luo.penghao@zte.com.cn> > > Reviewed-by: Simon Horman <horms@kernel.org> Thanks for the review, but (davem/kuba) please do not apply. > > @@ -1215,8 +1215,6 @@ static int e1000_integrated_phy_loopback(struct e1000_adapter *adapter) > > e1000_write_phy_reg(hw, PHY_CTRL, 0x8140); > > } > > > > - ctrl_reg = er32(CTRL); Thanks for your patch, but this change is not safe. you're removing a read that could do two things. The first is that the read "flushes" the write just above to PCI (it's a PCI barrier), and the second is that the read can have some side effects. If this change must be done, the code should be to remove the assignment to ctrl_reg, but leave the read, so the line would just look like: er32(CTRL); This will get rid of the warning and not change the flow from the hardware perspective. > > - > > /* force 1000, set loopback */ > > e1000_write_phy_reg(hw, PHY_CTRL, 0x4140); > > Please do not apply this. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [Intel-wired-lan] [PATCH linux-next] e1000: Remove redundant statement 2021-10-20 18:08 ` Jesse Brandeburg (?) @ 2021-10-21 7:09 ` Simon Horman -1 siblings, 0 replies; 9+ messages in thread From: Simon Horman @ 2021-10-21 7:09 UTC (permalink / raw) To: Jesse Brandeburg Cc: luo penghao, NetDEV list, Zeal Robot, linux-kernel@vger.kernel.org, intel-wired-lan, Jakub Kicinski, luo penghao, David S . Miller On Wed, Oct 20, 2021 at 11:08:11AM -0700, Jesse Brandeburg wrote: > Apologies for the duplicates, mail from my intel account going out > through outlook.com is not being delivered. > > On Wed, Oct 20, 2021 at 7:00 AM Simon Horman <horms@kernel.org> wrote: > > > > Value stored to 'ctrl_reg' is never read. > > > > I agree this does seem to be the case. > > > > > Reported-by: Zeal Robot <zealci@zte.com.cn> > > > Signed-off-by: luo penghao <luo.penghao@zte.com.cn> > > > > Reviewed-by: Simon Horman <horms@kernel.org> > > Thanks for the review, but (davem/kuba) please do not apply. Thanks, and sorry for misunderstanding the patch. > > > > @@ -1215,8 +1215,6 @@ static int e1000_integrated_phy_loopback(struct e1000_adapter *adapter) > > > e1000_write_phy_reg(hw, PHY_CTRL, 0x8140); > > > } > > > > > > - ctrl_reg = er32(CTRL); > > Thanks for your patch, but this change is not safe. you're removing a > read that could do two things. The first is that the read "flushes" > the write just above to PCI (it's a PCI barrier), and the second is > that the read can have some side effects. > > If this change must be done, the code should be to remove the > assignment to ctrl_reg, but leave the read, so the line would just > look like: > er32(CTRL); > > This will get rid of the warning and not change the flow from the > hardware perspective. > > > > - > > > /* force 1000, set loopback */ > > > e1000_write_phy_reg(hw, PHY_CTRL, 0x4140); > > > > > Please do not apply this. > ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2021-10-21 7:09 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2021-10-18 8:53 [Intel-wired-lan] [PATCH linux-next] e1000: Remove redundant statement luo penghao 2021-10-18 8:53 ` luo penghao 2021-10-18 16:10 ` [Intel-wired-lan] " jesse.brandeburg 2021-10-20 17:51 ` Jesse Brandeburg 2021-10-20 9:25 ` Simon Horman 2021-10-20 9:25 ` Simon Horman 2021-10-20 18:08 ` [Intel-wired-lan] " Jesse Brandeburg 2021-10-20 18:08 ` Jesse Brandeburg 2021-10-21 7:09 ` Simon Horman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.