| Message ID | 20211018085305.853996-1-luo.penghao@zte.com.cn (mailing list archive) |
|---|---|
| State | Awaiting Upstream |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [linux-next] e1000: Remove redundant statement | expand |
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 > >
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.
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. >
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);
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(-)