Re: [PATCH v2 1/9] drivers: net: Remove unused functionigb_get_eee_status_i354() in e1000_82575.c

[Josh Triplett]

On Mon, Dec 16, 2013 at 06:17:40PM +0000, Wyborny, Carolyn wrote:
> 
> 
> > -----Original Message-----
> > From: Josh Triplett [mailto:josh@xxxxxxxxxxxxxxxx]
> > Sent: Monday, December 16, 2013 9:43 AM
> > To: Wyborny, Carolyn
> > Cc: Rashika Kheria; linux-kernel@xxxxxxxxxxxxxxx; Kirsher, Jeffrey T; Brandeburg,
> > Jesse; Allan, Bruce W; Skidmore, Donald C; Rose, Gregory V; Waskiewicz Jr,
> > Peter P; Duyck, Alexander H; Ronciak, John; Dave, Tushar N; Abodunrin, Akeem
> > G; Vick, Matthew; e1000-devel@xxxxxxxxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH v2 1/9] drivers: net: Remove unused function
> > igb_get_eee_status_i354() in e1000_82575.c
> > 
> > On Mon, Dec 16, 2013 at 05:11:25PM +0000, Wyborny, Carolyn wrote:
> > > > -----Original Message-----
> > > > From: Rashika Kheria [mailto:rashika.kheria@xxxxxxxxx]
> > > > Sent: Saturday, December 14, 2013 4:15 AM
> > > > To: linux-kernel@xxxxxxxxxxxxxxx
> > > > Cc: Kirsher, Jeffrey T; Brandeburg, Jesse; Allan, Bruce W; Wyborny,
> > > > Carolyn; Skidmore, Donald C; Rose, Gregory V; Waskiewicz Jr, Peter
> > > > P; Duyck, Alexander H; Ronciak, John; Dave, Tushar N; Abodunrin,
> > > > Akeem G; Vick, Matthew; e1000- devel@xxxxxxxxxxxxxxxxxxxxx;
> > > > netdev@xxxxxxxxxxxxxxx; josh@xxxxxxxxxxxxxxxx
> > > > Subject: [PATCH v2 1/9] drivers: net: Remove unused function
> > > > igb_get_eee_status_i354() in e1000_82575.c
> > > >
> > > > This patch removes the function igb_get_eee_status_i354() in
> > > > e1000_82575.c because it is unused.
> > > >
> > > > It thus eliminates the following warning in
> > > > ethernet/intel/igb/e1000_82575.c:
> > > > drivers/net/ethernet/intel/igb/e1000_82575.c:2591:5: warning: no
> > > > previous prototype for âigb_get_eee_status_i354â
> > > > [-Wmissing-prototypes]
> > > >
> > > > Signed-off-by: Rashika Kheria <rashika.kheria@xxxxxxxxx>
> > > > Reviewed-by: Josh Triplett <josh@xxxxxxxxxxxxxxxx>
> > > > ---
> > > >  drivers/net/ethernet/intel/igb/e1000_82575.c |   32 --------------------------
> > > >  1 file changed, 32 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/intel/igb/e1000_82575.c
> > > > b/drivers/net/ethernet/intel/igb/e1000_82575.c
> > > > index 47c2d10..18e5200 100644
> > > > --- a/drivers/net/ethernet/intel/igb/e1000_82575.c
> > > > +++ b/drivers/net/ethernet/intel/igb/e1000_82575.c
> > > > @@ -2580,38 +2580,6 @@ out:
> > > >  	return ret_val;
> > > >  }
> > > >
> > > > -/**
> > > > - *  igb_get_eee_status_i354 - Get EEE status
> > > > - *  @hw: pointer to the HW structure
> > > > - *  @status: EEE status
> > > > - *
> > > > - *  Get EEE status by guessing based on whether Tx or Rx LPI
> > > > indications have
> > > > - *  been received.
> > > > - **/
> > > > -s32 igb_get_eee_status_i354(struct e1000_hw *hw, bool *status) -{
> > > > -	struct e1000_phy_info *phy = &hw->phy;
> > > > -	s32 ret_val = 0;
> > > > -	u16 phy_data;
> > > > -
> > > > -	/* Check if EEE is supported on this device. */
> > > > -	if ((hw->phy.media_type != e1000_media_type_copper) ||
> > > > -	    (phy->id != M88E1543_E_PHY_ID))
> > > > -		goto out;
> > > > -
> > > > -	ret_val = igb_read_xmdio_reg(hw, E1000_PCS_STATUS_ADDR_I354,
> > > > -				     E1000_PCS_STATUS_DEV_I354,
> > > > -				     &phy_data);
> > > > -	if (ret_val)
> > > > -		goto out;
> > > > -
> > > > -	*status = phy_data & (E1000_PCS_STATUS_TX_LPI_RCVD |
> > > > -			      E1000_PCS_STATUS_RX_LPI_RCVD) ? true : false;
> > > > -
> > > > -out:
> > > > -	return ret_val;
> > > > -}
> > > > -
> > > >  static const u8 e1000_emc_temp_data[4] = {
> > > >  	E1000_EMC_INTERNAL_DATA,
> > > >  	E1000_EMC_DIODE1_DATA,
> > > > --
> > > > 1.7.9.5
> > >
> > > NACK.
> > >
> > > Thanks for the patch Rashika, but this is the incorrect fix for this
> > > warning  The function is called in the igb_probe function, so you
> > > cannot remove it and I see the prototype in the e1000_82575.h file.
> > > Can you double check your source pull?
> > 
> > From top-of-tree linux.git:
> > 
> > ~/src/linux$ git grep igb_get_eee_status_i354
> > drivers/net/ethernet/intel/igb/e1000_82575.c: *  igb_get_eee_status_i354 - Get
> > EEE status
> > drivers/net/ethernet/intel/igb/e1000_82575.c:s32jjj
> > igb_get_eee_status_i354(struct e1000_hw *hw, bool *status)
> > 
> > A comment and the function itself; no other references.  In what tree are you
> > seeing a reference to igb_get_eee_status_i354?
> > 
> > - Josh Triplett
> 
> Thanks Josh,
> 
> My mistake.  I was looking at the "get" not "set" function.  However, this is still not the right fix for the driver as the actual problem is the missing code that is supposed to be calling that function in igb_get_eee in igb_ethtool.c.  The function needs to stay.  I will submit the correct fix ASAP.

Ah, that makes sense.  Can you please Cc Rashika and myself on that
patch, with a Reported-by for Rashika?

- Josh Triplett
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/