RE: [Intel-wired-lan] [PATCH] net: intel: i40evf: use new api ethtool_{get|set}_link_ksettings

[Wyborny, Carolyn]

> -Original Message-
> From: Intel-wired-lan [mailto:intel-wired-lan-boun...@lists.osuosl.org] On
> Behalf Of Philippe Reynes
> Sent: Saturday, February 04, 2017 2:49 PM
> To: Kirsher, Jeffrey T ; da...@davemloft.net
> Cc: net...@vger.kernel.org; intel-wired-...@lists.osuosl.org; linux-
> ker...@vger.kernel.org; Philippe Reynes 
> Subject: [Intel-wired-lan] [PATCH] net: intel: i40evf: use new api
> ethtool_{get|set}_link_ksettings
> 
> The ethtool api {get|set}_settings is deprecated.
> We move this driver to new api {get|set}_link_ksettings.
> 
Thanks for the patch Phillippe,

We have an internal patch in process for this functionality.  It should be out 
very soon.  I thought it was upstream already but it must not be.  I'll find it 
and get it expedited.

Thanks again,

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation 

RE: [Intel-wired-lan] [PATCH] net: intel: i40evf: use new api ethtool_{get|set}_link_ksettings

[Wyborny, Carolyn]

> -Original Message-
> From: Intel-wired-lan [mailto:intel-wired-lan-boun...@lists.osuosl.org] On
> Behalf Of Philippe Reynes
> Sent: Saturday, February 04, 2017 2:49 PM
> To: Kirsher, Jeffrey T ; da...@davemloft.net
> Cc: net...@vger.kernel.org; intel-wired-...@lists.osuosl.org; linux-
> ker...@vger.kernel.org; Philippe Reynes 
> Subject: [Intel-wired-lan] [PATCH] net: intel: i40evf: use new api
> ethtool_{get|set}_link_ksettings
> 
> The ethtool api {get|set}_settings is deprecated.
> We move this driver to new api {get|set}_link_ksettings.
> 
Thanks for the patch Phillippe,

We have an internal patch in process for this functionality.  It should be out 
very soon.  I thought it was upstream already but it must not be.  I'll find it 
and get it expedited.

Thanks again,

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation 

RE: [PATCH v2 1/9] drivers: net: Remove unused function igb_get_eee_status_i354() in e1000_82575.c

[Wyborny, Carolyn]

> -Original Message-
> From: Josh Triplett [mailto:j...@joshtriplett.org]
> Sent: Monday, December 16, 2013 10:22 AM
> To: Wyborny, Carolyn
> Cc: Rashika Kheria; linux-kernel@vger.kernel.org; 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-de...@lists.sourceforge.net; net...@vger.kernel.org
> 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 06:17:40PM +, Wyborny, Carolyn wrote:
> >
> >
> > > -Original Message-
> > > From: Josh Triplett [mailto:j...@joshtriplett.org]
> > > Sent: Monday, December 16, 2013 9:43 AM
> > > To: Wyborny, Carolyn
> > > Cc: Rashika Kheria; linux-kernel@vger.kernel.org; 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-de...@lists.sourceforge.net; net...@vger.kernel.org
> > > 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 +, Wyborny, Carolyn wrote:
> > > > > -Original Message-
> > > > > From: Rashika Kheria [mailto:rashika.khe...@gmail.com]
> > > > > Sent: Saturday, December 14, 2013 4:15 AM
> > > > > To: linux-kernel@vger.kernel.org
> > > > > 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-
> > > > > de...@lists.sourceforge.net; net...@vger.kernel.org;
> > > > > j...@joshtriplett.org
> > > > > 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 
> > > > > Reviewed-by: Josh Triplett 
> > > > > ---
> > > > >  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 = >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,
> > > &

RE: [PATCH v2 1/9] drivers: net: Remove unused function igb_get_eee_status_i354() in e1000_82575.c

[Wyborny, Carolyn]

> -Original Message-
> From: Josh Triplett [mailto:j...@joshtriplett.org]
> Sent: Monday, December 16, 2013 9:43 AM
> To: Wyborny, Carolyn
> Cc: Rashika Kheria; linux-kernel@vger.kernel.org; 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-de...@lists.sourceforge.net; net...@vger.kernel.org
> 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 +, Wyborny, Carolyn wrote:
> > > -Original Message-
> > > From: Rashika Kheria [mailto:rashika.khe...@gmail.com]
> > > Sent: Saturday, December 14, 2013 4:15 AM
> > > To: linux-kernel@vger.kernel.org
> > > 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- de...@lists.sourceforge.net;
> > > net...@vger.kernel.org; j...@joshtriplett.org
> > > 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 
> > > Reviewed-by: Josh Triplett 
> > > ---
> > >  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 = >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,
> > > -  _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.

Thanks,

Carolyn
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�:+v���zZ+��+zf���h���~i���z��w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v2 1/9] drivers: net: Remove unused function igb_get_eee_status_i354() in e1000_82575.c

[Wyborny, Carolyn]

> -Original Message-
> From: Rashika Kheria [mailto:rashika.khe...@gmail.com]
> Sent: Saturday, December 14, 2013 4:15 AM
> To: linux-kernel@vger.kernel.org
> 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-
> de...@lists.sourceforge.net; net...@vger.kernel.org; j...@joshtriplett.org
> 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 
> Reviewed-by: Josh Triplett 
> ---
>  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 = >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,
> -  _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?

Thanks,

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation 

RE: [PATCH v2 1/9] drivers: net: Remove unused function igb_get_eee_status_i354() in e1000_82575.c

[Wyborny, Carolyn]

 -Original Message-
 From: Rashika Kheria [mailto:rashika.khe...@gmail.com]
 Sent: Saturday, December 14, 2013 4:15 AM
 To: linux-kernel@vger.kernel.org
 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-
 de...@lists.sourceforge.net; net...@vger.kernel.org; j...@joshtriplett.org
 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.khe...@gmail.com
 Reviewed-by: Josh Triplett j...@joshtriplett.org
 ---
  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?

Thanks,

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation 

RE: [PATCH v2 1/9] drivers: net: Remove unused function igb_get_eee_status_i354() in e1000_82575.c

[Wyborny, Carolyn]

 -Original Message-
 From: Josh Triplett [mailto:j...@joshtriplett.org]
 Sent: Monday, December 16, 2013 9:43 AM
 To: Wyborny, Carolyn
 Cc: Rashika Kheria; linux-kernel@vger.kernel.org; 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-de...@lists.sourceforge.net; net...@vger.kernel.org
 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 +, Wyborny, Carolyn wrote:
   -Original Message-
   From: Rashika Kheria [mailto:rashika.khe...@gmail.com]
   Sent: Saturday, December 14, 2013 4:15 AM
   To: linux-kernel@vger.kernel.org
   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- de...@lists.sourceforge.net;
   net...@vger.kernel.org; j...@joshtriplett.org
   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.khe...@gmail.com
   Reviewed-by: Josh Triplett j...@joshtriplett.org
   ---
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.

Thanks,

Carolyn
N�r��yb�X��ǧv�^�)޺{.n�+{zX����ܨ}���Ơz�j:+v���zZ+��+zf���h���~i���z��w���?��)ߢf��^jǫy�m��@A�a���
0��h���i

RE: [PATCH v2 1/9] drivers: net: Remove unused function igb_get_eee_status_i354() in e1000_82575.c

[Wyborny, Carolyn]

 -Original Message-
 From: Josh Triplett [mailto:j...@joshtriplett.org]
 Sent: Monday, December 16, 2013 10:22 AM
 To: Wyborny, Carolyn
 Cc: Rashika Kheria; linux-kernel@vger.kernel.org; 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-de...@lists.sourceforge.net; net...@vger.kernel.org
 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 06:17:40PM +, Wyborny, Carolyn wrote:
 
 
   -Original Message-
   From: Josh Triplett [mailto:j...@joshtriplett.org]
   Sent: Monday, December 16, 2013 9:43 AM
   To: Wyborny, Carolyn
   Cc: Rashika Kheria; linux-kernel@vger.kernel.org; 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-de...@lists.sourceforge.net; net...@vger.kernel.org
   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 +, Wyborny, Carolyn wrote:
 -Original Message-
 From: Rashika Kheria [mailto:rashika.khe...@gmail.com]
 Sent: Saturday, December 14, 2013 4:15 AM
 To: linux-kernel@vger.kernel.org
 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-
 de...@lists.sourceforge.net; net...@vger.kernel.org;
 j...@joshtriplett.org
 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.khe...@gmail.com
 Reviewed-by: Josh Triplett j...@joshtriplett.org
 ---
  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

RE: [PATCH 07/11] igb: Avoid uninitialized advertised variable in eee_set_cur

[Wyborny, Carolyn]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of Andi Kleen
> Sent: Monday, September 30, 2013 1:29 PM
> To: linux-kernel@vger.kernel.org
> Cc: Andi Kleen; Kirsher, Jeffrey T; net...@vger.kernel.org
> Subject: [PATCH 07/11] igb: Avoid uninitialized advertised variable in
> eee_set_cur
> 
> From: Andi Kleen 
> 
> eee_get_cur assumes that the output data is already zeroed. It can 
> read-modify-
> write the advertised field:
> 
>   if (ipcnfg & E1000_IPCNFG_EEE_100M_AN)
> 2594  edata->advertised |= ADVERTISED_100baseT_Full;
> 
> This is ok for the normal ethtool eee_get call, which always zeroes the input
> data before.
> 
> But eee_set_cur also calls eee_get_cur and it did not zero the input field. 
> Later
> on it then compares agsinst the field, which can contain partial stack 
> garbage.
> 
> Zero the input field in eee_set_cur() too.
> 
> Cc: jeffrey.t.kirs...@intel.com
> Cc: net...@vger.kernel.org
> Signed-off-by: Andi Kleen 
> ---
>  drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> index 48cbc83..41e37ff 100644
> --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
> +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
> @@ -2652,6 +2652,8 @@ static int igb_set_eee(struct net_device *netdev,
>   (hw->phy.media_type != e1000_media_type_copper))
>   return -EOPNOTSUPP;
> 
> + memset(_curr, 0, sizeof(struct ethtool_eee));
> +
>   ret_val = igb_get_eee(netdev, _curr);
>   if (ret_val)
>   return ret_val;
> --
> 1.8.3.1
> 

ACK

Thanks,

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

RE: [PATCH 07/11] igb: Avoid uninitialized advertised variable in eee_set_cur

[Wyborny, Carolyn]

 -Original Message-
 From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
 On Behalf Of Andi Kleen
 Sent: Monday, September 30, 2013 1:29 PM
 To: linux-kernel@vger.kernel.org
 Cc: Andi Kleen; Kirsher, Jeffrey T; net...@vger.kernel.org
 Subject: [PATCH 07/11] igb: Avoid uninitialized advertised variable in
 eee_set_cur
 
 From: Andi Kleen a...@linux.intel.com
 
 eee_get_cur assumes that the output data is already zeroed. It can 
 read-modify-
 write the advertised field:
 
   if (ipcnfg  E1000_IPCNFG_EEE_100M_AN)
 2594  edata-advertised |= ADVERTISED_100baseT_Full;
 
 This is ok for the normal ethtool eee_get call, which always zeroes the input
 data before.
 
 But eee_set_cur also calls eee_get_cur and it did not zero the input field. 
 Later
 on it then compares agsinst the field, which can contain partial stack 
 garbage.
 
 Zero the input field in eee_set_cur() too.
 
 Cc: jeffrey.t.kirs...@intel.com
 Cc: net...@vger.kernel.org
 Signed-off-by: Andi Kleen a...@linux.intel.com
 ---
  drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 ++
  1 file changed, 2 insertions(+)
 
 diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c
 b/drivers/net/ethernet/intel/igb/igb_ethtool.c
 index 48cbc83..41e37ff 100644
 --- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
 +++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
 @@ -2652,6 +2652,8 @@ static int igb_set_eee(struct net_device *netdev,
   (hw-phy.media_type != e1000_media_type_copper))
   return -EOPNOTSUPP;
 
 + memset(eee_curr, 0, sizeof(struct ethtool_eee));
 +
   ret_val = igb_get_eee(netdev, eee_curr);
   if (ret_val)
   return ret_val;
 --
 1.8.3.1
 

ACK

Thanks,

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

RE: /sys/module/pcie_aspm/parameters/policy not writable?

[Wyborny, Carolyn]

> -Original Message-
> From: Pavel Machek [mailto:pa...@ucw.cz]
> Sent: Thursday, August 01, 2013 5:40 PM
> To: Wyborny, Carolyn
> Cc: Bjorn Helgaas; Greg KH; kernel list; Joe Lawrence; Myron Stowe; 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;
> e1000-de...@lists.sourceforge.net
> Subject: Re: /sys/module/pcie_aspm/parameters/policy not writable?
> 
> Hi!
> 
> > > > If there's patch, I'll gladly try it :-). Thanks,
> > > >
> > >   Pavel
> > > Thanks Pavel,
> >
> > Minor fix.  Missed a bracket removal  Please use this  for your testing 
> > instead.
> >
> 
> Seems to work here. (I did testing on 3.11-rc, but that should not change 
> things.)
> Latencies are in expected range.
> 
> Tested-by: Pavel Machek 

Thanks Pavel,

I'll submit a version of this to our internal queue.

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

RE: /sys/module/pcie_aspm/parameters/policy not writable?

[Wyborny, Carolyn]

 -Original Message-
 From: Pavel Machek [mailto:pa...@ucw.cz]
 Sent: Thursday, August 01, 2013 5:40 PM
 To: Wyborny, Carolyn
 Cc: Bjorn Helgaas; Greg KH; kernel list; Joe Lawrence; Myron Stowe; 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;
 e1000-de...@lists.sourceforge.net
 Subject: Re: /sys/module/pcie_aspm/parameters/policy not writable?
 
 Hi!
 
If there's patch, I'll gladly try it :-). Thanks,
   
 Pavel
   Thanks Pavel,
 
  Minor fix.  Missed a bracket removal  Please use this  for your testing 
  instead.
 
 
 Seems to work here. (I did testing on 3.11-rc, but that should not change 
 things.)
 Latencies are in expected range.
 
 Tested-by: Pavel Machek pa...@ucw.cz

Thanks Pavel,

I'll submit a version of this to our internal queue.

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

RE: /sys/module/pcie_aspm/parameters/policy not writable?

[Wyborny, Carolyn]

> -Original Message-
> From: Wyborny, Carolyn
> Sent: Thursday, August 01, 2013 7:56 AM
> To: 'Pavel Machek'
> Cc: Bjorn Helgaas; Greg KH; kernel list; Joe Lawrence; Myron Stowe; 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;
> e1000-de...@lists.sourceforge.net
> Subject: RE: /sys/module/pcie_aspm/parameters/policy not writable?
> 
> [..]
> > If there's patch, I'll gladly try it :-). Thanks,
> >
>   Pavel
> Thanks Pavel,

Minor fix.  Missed a bracket removal  Please use this  for your testing instead.

Thanks,

Carolyn
> 
> It ended up taking more time than I thought.  The checking of whether ASPM is
> really enabled or not is, unfortunately, not as straightforward as I thought
> initially, so we ended up rewriting the function a bit.  In this patch we try 
> to use
> the pci_disable_link_state_locked() call, but if it fails, we then write to 
> pci config
> space of the device to disable it.
> 
> Please let me know if this actually disables the ASPM for your 82574 parts or
> not.  We can still continue the discussion on whether this is the best 
> approach or
> not, or what else could be done.
> 
> This patch attempts to work around a problem found with some systems where
> the call to pci_diable_link_state_locked() fails.  As a result, ASPM is not, 
> in fact,
> disabled.  Changing disable aspm code to check if state actually is disabled 
> after
> the call and, if not, try another way to disable it.
> 
> Signed-off-by: Carolyn Wyborny 
> ---
> 
>  drivers/net/ethernet/intel/e1000e/netdev.c |   86 --
> --
>  1 files changed, 60 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c
> b/drivers/net/ethernet/intel/e1000e/netdev.c
> index e6d2c0f..bc3025b 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -64,8 +64,6 @@ static int debug = -1;  module_param(debug, int, 0);
> MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
> 
> -static void e1000e_disable_aspm(struct pci_dev *pdev, u16 state);
> -
>  static const struct e1000_info *e1000_info_tbl[] = {
>   [board_82571]   = _82571_info,
>   [board_82572]   = _82572_info,
> @@ -6019,38 +6017,74 @@ static int __e1000_shutdown(struct pci_dev *pdev,
> bool runtime)
>   return 0;
>  }
> 
> -#ifdef CONFIG_PCIEASPM
> -static void __e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
> +/**
> + * e1000e_disable_aspm - Disable ASPM states
> + * @pdev: pointer to PCI device struct
> + * @state: bit-mask of ASPM states to disable
> + *
> + * Some devices *must* have certain ASPM states disabled per hardware
> errata.
> + **/
> +static void e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
>  {
> + struct pci_dev *parent = pdev->bus->self;
> + u16 aspm_dis_mask = 0;
> + u16 pdev_aspmc, parent_aspmc;
> +
> + switch (state) {
> + case PCIE_LINK_STATE_L0S:
> + case PCIE_LINK_STATE_L0S + PCIE_LINK_STATE_L1:
> + aspm_dis_mask |= PCI_EXP_LNKCTL_ASPM_L0S;
> + /* fall-through - can't have L1 without L0s */
> + case PCIE_LINK_STATE_L1:
> + aspm_dis_mask |= PCI_EXP_LNKCTL_ASPM_L1;
> + break;
> + default:
> + return;
> + }
> +
> + pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, _aspmc);
> + pdev_aspmc &= PCI_EXP_LNKCTL_ASPMC;
> +
> + if (parent) {
> + pcie_capability_read_word(parent, PCI_EXP_LNKCTL,
> +   _aspmc);
> + parent_aspmc &= PCI_EXP_LNKCTL_ASPMC;
> + }
> +
> + /* Nothing to do if the ASPM states to be disabled already are */
> + if (!(pdev_aspmc & aspm_dis_mask) &&
> + (!parent || !(parent_aspmc & aspm_dis_mask)))
> + return;
> +
> + dev_info(>dev, "Disabling ASPM %s %s\n",
> +  (aspm_dis_mask & pdev_aspmc &
> PCI_EXP_LNKCTL_ASPM_L0S) ?
> +  "L0s" : "",
> +  (aspm_dis_mask & pdev_aspmc & PCI_EXP_LNKCTL_ASPM_L1)
> ?
> +  "L1" : "");
> +
> +#ifdef CONFIG_PCIEASPM
>   pci_disable_link_state_locked(pdev, state); -} -#else -static void
> __e1000e_disable_aspm(struct pci_dev *pdev, u16 state) -{
> - u16 aspm_ctl = 0;
> 
> - if (state & PCIE_LINK_STATE_L0S)
> - aspm_ctl |= PCI_EXP_LNK

RE: /sys/module/pcie_aspm/parameters/policy not writable?

[Wyborny, Carolyn]

> -Original Message-
> From: Bjorn Helgaas [mailto:bhelg...@google.com]
> Sent: Wednesday, July 31, 2013 4:53 PM
> To: Pavel Machek
> Cc: Greg KH; kernel list; Joe Lawrence; Myron Stowe; 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; e1000-de...@lists.sourceforge.net
> Subject: Re: /sys/module/pcie_aspm/parameters/policy not writable?
> 
> On Fri, Jul 19, 2013 at 11:46 AM, Bjorn Helgaas  wrote:
> 
> > Just to make sure I understand you correctly: I think you are saying
> > that the NIC has the same problem under Windows.
> 
> Can you confirm or deny that the same problem occurs with Windows?

In our testing here we saw the same problem with Windows.
> 
> > But since the problem also occurs with Windows, it's pretty likely
> > that there's a BIOS update to fix it.  I notice on the X60 support
> > page that there are several versions newer than what you're running.
> 
> Do you have any interest in trying a newer BIOS to see if it's fixed there?  
> If not, I
> understand; BIOS updates are a hassle at best.
> You're running BIOS 2.14, and it looks like the current BIOS for an
> X60 1709 7HU is 2.19 (from http://support.lenovo.com).
> 
> Carolyn's patch will likely work, at least most of the time, but I think 
> there's a
> small possibility that it could cause a conflict between the BIOS and the OS 
> over
> ASPM control, so I'm not 100% in support of that approach.  A conflict may not
> happen on your machine, and not on my machine, but it may happen
> somewhere, and if it does happen, it's likely to be rare and difficult to 
> debug.
> 
I've proposed a patch for Pavel to test.  I understand your position, but do 
you have any other proposal?  We have a severe hw problem we are trying 
alleviate as best we can.  

Pavel, were you also able to try the updated BIOS to see if that resolved the 
issue without my patch?

Thanks,

Carolyn

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

RE: /sys/module/pcie_aspm/parameters/policy not writable?

[Wyborny, Carolyn]

[..] 
> If there's patch, I'll gladly try it :-). Thanks,
> 
Pavel
Thanks Pavel,

It ended up taking more time than I thought.  The checking of whether ASPM is 
really enabled or not is, unfortunately, not as straightforward as I thought 
initially, so we ended up rewriting the function a bit.  In this patch we try 
to use the pci_disable_link_state_locked() call, but if it fails, we then write 
to pci config space of the device to disable it.

Please let me know if this actually disables the ASPM for your 82574 parts or 
not.  We can still continue the discussion on whether this is the best approach 
or not, or what else could be done.

This patch attempts to work around a problem found with some systems where the 
call to pci_diable_link_state_locked() fails.  As a result, ASPM is not, in 
fact, disabled.  Changing disable aspm code to check if state actually is 
disabled after the call and, if not, try another way to disable it.

Signed-off-by: Carolyn Wyborny 
---

 drivers/net/ethernet/intel/e1000e/netdev.c |   86 
 1 files changed, 60 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c 
b/drivers/net/ethernet/intel/e1000e/netdev.c
index e6d2c0f..bc3025b 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -64,8 +64,6 @@ static int debug = -1;  module_param(debug, int, 0);  
MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 
-static void e1000e_disable_aspm(struct pci_dev *pdev, u16 state);
-
 static const struct e1000_info *e1000_info_tbl[] = {
[board_82571]   = _82571_info,
[board_82572]   = _82572_info,
@@ -6019,38 +6017,74 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool 
runtime)
return 0;
 }
 
-#ifdef CONFIG_PCIEASPM
-static void __e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
+/**
+ * e1000e_disable_aspm - Disable ASPM states
+ * @pdev: pointer to PCI device struct
+ * @state: bit-mask of ASPM states to disable
+ *
+ * Some devices *must* have certain ASPM states disabled per hardware errata.
+ **/
+static void e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
 {
+   struct pci_dev *parent = pdev->bus->self;
+   u16 aspm_dis_mask = 0;
+   u16 pdev_aspmc, parent_aspmc;
+
+   switch (state) {
+   case PCIE_LINK_STATE_L0S:
+   case PCIE_LINK_STATE_L0S + PCIE_LINK_STATE_L1:
+   aspm_dis_mask |= PCI_EXP_LNKCTL_ASPM_L0S;
+   /* fall-through - can't have L1 without L0s */
+   case PCIE_LINK_STATE_L1:
+   aspm_dis_mask |= PCI_EXP_LNKCTL_ASPM_L1;
+   break;
+   default:
+   return;
+   }
+
+   pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, _aspmc);
+   pdev_aspmc &= PCI_EXP_LNKCTL_ASPMC;
+
+   if (parent) {
+   pcie_capability_read_word(parent, PCI_EXP_LNKCTL,
+ _aspmc);
+   parent_aspmc &= PCI_EXP_LNKCTL_ASPMC;
+   }
+
+   /* Nothing to do if the ASPM states to be disabled already are */
+   if (!(pdev_aspmc & aspm_dis_mask) &&
+   (!parent || !(parent_aspmc & aspm_dis_mask)))
+   return;
+
+   dev_info(>dev, "Disabling ASPM %s %s\n",
+(aspm_dis_mask & pdev_aspmc & PCI_EXP_LNKCTL_ASPM_L0S) ?
+"L0s" : "",
+(aspm_dis_mask & pdev_aspmc & PCI_EXP_LNKCTL_ASPM_L1) ?
+"L1" : "");
+
+#ifdef CONFIG_PCIEASPM
pci_disable_link_state_locked(pdev, state); -} -#else -static void 
__e1000e_disable_aspm(struct pci_dev *pdev, u16 state) -{
-   u16 aspm_ctl = 0;
 
-   if (state & PCIE_LINK_STATE_L0S)
-   aspm_ctl |= PCI_EXP_LNKCTL_ASPM_L0S;
-   if (state & PCIE_LINK_STATE_L1)
-   aspm_ctl |= PCI_EXP_LNKCTL_ASPM_L1;
+   /* Double-check ASPM control.  If not disabled by the above, the
+* BIOS is preventing that from happening (or CONFIG_PCIEASPM is
+* not enabled); override by writing PCI config space directly.
+*/
+   pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, _aspmc);
+   pdev_aspmc &= PCI_EXP_LNKCTL_ASPMC;
+
+   if (!(aspm_dis_mask & pdev_aspmc))
+   return;
+   }
+#endif
 
/* Both device and parent should have the same ASPM setting.
 * Disable ASPM in downstream component first and then upstream.
 */
-   pcie_capability_clear_word(pdev, PCI_EXP_LNKCTL, aspm_ctl);
-
-   if (pdev->bus->self)
-   pcie_capability_clear_word(pdev->bus->self, PCI_EXP_LNKCTL,
-  aspm_ctl);
-}
-#endif
-static void e1000e_disable_aspm(struct pci_dev *pdev, u16 state) -{
-   dev_info(>dev, "Disabling ASPM %s %s\n",
-(state & PCIE_LINK_STATE_L0S) ? "L0s" : "",
-(state & PCIE_LINK_STATE_L1) ? "L1" : "");
+ 

RE: /sys/module/pcie_aspm/parameters/policy not writable?

[Wyborny, Carolyn]

[..] 
 If there's patch, I'll gladly try it :-). Thanks,
 
Pavel
Thanks Pavel,

It ended up taking more time than I thought.  The checking of whether ASPM is 
really enabled or not is, unfortunately, not as straightforward as I thought 
initially, so we ended up rewriting the function a bit.  In this patch we try 
to use the pci_disable_link_state_locked() call, but if it fails, we then write 
to pci config space of the device to disable it.

Please let me know if this actually disables the ASPM for your 82574 parts or 
not.  We can still continue the discussion on whether this is the best approach 
or not, or what else could be done.

This patch attempts to work around a problem found with some systems where the 
call to pci_diable_link_state_locked() fails.  As a result, ASPM is not, in 
fact, disabled.  Changing disable aspm code to check if state actually is 
disabled after the call and, if not, try another way to disable it.

Signed-off-by: Carolyn Wyborny carolyn.wybo...@intel.com
---

 drivers/net/ethernet/intel/e1000e/netdev.c |   86 
 1 files changed, 60 insertions(+), 26 deletions(-)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c 
b/drivers/net/ethernet/intel/e1000e/netdev.c
index e6d2c0f..bc3025b 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -64,8 +64,6 @@ static int debug = -1;  module_param(debug, int, 0);  
MODULE_PARM_DESC(debug, Debug level (0=none,...,16=all));
 
-static void e1000e_disable_aspm(struct pci_dev *pdev, u16 state);
-
 static const struct e1000_info *e1000_info_tbl[] = {
[board_82571]   = e1000_82571_info,
[board_82572]   = e1000_82572_info,
@@ -6019,38 +6017,74 @@ static int __e1000_shutdown(struct pci_dev *pdev, bool 
runtime)
return 0;
 }
 
-#ifdef CONFIG_PCIEASPM
-static void __e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
+/**
+ * e1000e_disable_aspm - Disable ASPM states
+ * @pdev: pointer to PCI device struct
+ * @state: bit-mask of ASPM states to disable
+ *
+ * Some devices *must* have certain ASPM states disabled per hardware errata.
+ **/
+static void e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
 {
+   struct pci_dev *parent = pdev-bus-self;
+   u16 aspm_dis_mask = 0;
+   u16 pdev_aspmc, parent_aspmc;
+
+   switch (state) {
+   case PCIE_LINK_STATE_L0S:
+   case PCIE_LINK_STATE_L0S + PCIE_LINK_STATE_L1:
+   aspm_dis_mask |= PCI_EXP_LNKCTL_ASPM_L0S;
+   /* fall-through - can't have L1 without L0s */
+   case PCIE_LINK_STATE_L1:
+   aspm_dis_mask |= PCI_EXP_LNKCTL_ASPM_L1;
+   break;
+   default:
+   return;
+   }
+
+   pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, pdev_aspmc);
+   pdev_aspmc = PCI_EXP_LNKCTL_ASPMC;
+
+   if (parent) {
+   pcie_capability_read_word(parent, PCI_EXP_LNKCTL,
+ parent_aspmc);
+   parent_aspmc = PCI_EXP_LNKCTL_ASPMC;
+   }
+
+   /* Nothing to do if the ASPM states to be disabled already are */
+   if (!(pdev_aspmc  aspm_dis_mask) 
+   (!parent || !(parent_aspmc  aspm_dis_mask)))
+   return;
+
+   dev_info(pdev-dev, Disabling ASPM %s %s\n,
+(aspm_dis_mask  pdev_aspmc  PCI_EXP_LNKCTL_ASPM_L0S) ?
+L0s : ,
+(aspm_dis_mask  pdev_aspmc  PCI_EXP_LNKCTL_ASPM_L1) ?
+L1 : );
+
+#ifdef CONFIG_PCIEASPM
pci_disable_link_state_locked(pdev, state); -} -#else -static void 
__e1000e_disable_aspm(struct pci_dev *pdev, u16 state) -{
-   u16 aspm_ctl = 0;
 
-   if (state  PCIE_LINK_STATE_L0S)
-   aspm_ctl |= PCI_EXP_LNKCTL_ASPM_L0S;
-   if (state  PCIE_LINK_STATE_L1)
-   aspm_ctl |= PCI_EXP_LNKCTL_ASPM_L1;
+   /* Double-check ASPM control.  If not disabled by the above, the
+* BIOS is preventing that from happening (or CONFIG_PCIEASPM is
+* not enabled); override by writing PCI config space directly.
+*/
+   pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, pdev_aspmc);
+   pdev_aspmc = PCI_EXP_LNKCTL_ASPMC;
+
+   if (!(aspm_dis_mask  pdev_aspmc))
+   return;
+   }
+#endif
 
/* Both device and parent should have the same ASPM setting.
 * Disable ASPM in downstream component first and then upstream.
 */
-   pcie_capability_clear_word(pdev, PCI_EXP_LNKCTL, aspm_ctl);
-
-   if (pdev-bus-self)
-   pcie_capability_clear_word(pdev-bus-self, PCI_EXP_LNKCTL,
-  aspm_ctl);
-}
-#endif
-static void e1000e_disable_aspm(struct pci_dev *pdev, u16 state) -{
-   dev_info(pdev-dev, Disabling ASPM %s %s\n,
-(state  PCIE_LINK_STATE_L0S) ? L0s : ,
-(state  PCIE_LINK_STATE_L1) ? L1 

RE: /sys/module/pcie_aspm/parameters/policy not writable?

[Wyborny, Carolyn]

 -Original Message-
 From: Bjorn Helgaas [mailto:bhelg...@google.com]
 Sent: Wednesday, July 31, 2013 4:53 PM
 To: Pavel Machek
 Cc: Greg KH; kernel list; Joe Lawrence; Myron Stowe; 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; e1000-de...@lists.sourceforge.net
 Subject: Re: /sys/module/pcie_aspm/parameters/policy not writable?
 
 On Fri, Jul 19, 2013 at 11:46 AM, Bjorn Helgaas bhelg...@google.com wrote:
 
  Just to make sure I understand you correctly: I think you are saying
  that the NIC has the same problem under Windows.
 
 Can you confirm or deny that the same problem occurs with Windows?

In our testing here we saw the same problem with Windows.
 
  But since the problem also occurs with Windows, it's pretty likely
  that there's a BIOS update to fix it.  I notice on the X60 support
  page that there are several versions newer than what you're running.
 
 Do you have any interest in trying a newer BIOS to see if it's fixed there?  
 If not, I
 understand; BIOS updates are a hassle at best.
 You're running BIOS 2.14, and it looks like the current BIOS for an
 X60 1709 7HU is 2.19 (from http://support.lenovo.com).
 
 Carolyn's patch will likely work, at least most of the time, but I think 
 there's a
 small possibility that it could cause a conflict between the BIOS and the OS 
 over
 ASPM control, so I'm not 100% in support of that approach.  A conflict may not
 happen on your machine, and not on my machine, but it may happen
 somewhere, and if it does happen, it's likely to be rare and difficult to 
 debug.
 
I've proposed a patch for Pavel to test.  I understand your position, but do 
you have any other proposal?  We have a severe hw problem we are trying 
alleviate as best we can.  

Pavel, were you also able to try the updated BIOS to see if that resolved the 
issue without my patch?

Thanks,

Carolyn

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

RE: /sys/module/pcie_aspm/parameters/policy not writable?

[Wyborny, Carolyn]

 -Original Message-
 From: Wyborny, Carolyn
 Sent: Thursday, August 01, 2013 7:56 AM
 To: 'Pavel Machek'
 Cc: Bjorn Helgaas; Greg KH; kernel list; Joe Lawrence; Myron Stowe; 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;
 e1000-de...@lists.sourceforge.net
 Subject: RE: /sys/module/pcie_aspm/parameters/policy not writable?
 
 [..]
  If there's patch, I'll gladly try it :-). Thanks,
 
   Pavel
 Thanks Pavel,

Minor fix.  Missed a bracket removal  Please use this  for your testing instead.

Thanks,

Carolyn
 
 It ended up taking more time than I thought.  The checking of whether ASPM is
 really enabled or not is, unfortunately, not as straightforward as I thought
 initially, so we ended up rewriting the function a bit.  In this patch we try 
 to use
 the pci_disable_link_state_locked() call, but if it fails, we then write to 
 pci config
 space of the device to disable it.
 
 Please let me know if this actually disables the ASPM for your 82574 parts or
 not.  We can still continue the discussion on whether this is the best 
 approach or
 not, or what else could be done.
 
 This patch attempts to work around a problem found with some systems where
 the call to pci_diable_link_state_locked() fails.  As a result, ASPM is not, 
 in fact,
 disabled.  Changing disable aspm code to check if state actually is disabled 
 after
 the call and, if not, try another way to disable it.
 
 Signed-off-by: Carolyn Wyborny carolyn.wybo...@intel.com
 ---
 
  drivers/net/ethernet/intel/e1000e/netdev.c |   86 --
 --
  1 files changed, 60 insertions(+), 26 deletions(-)
 
 diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c
 b/drivers/net/ethernet/intel/e1000e/netdev.c
 index e6d2c0f..bc3025b 100644
 --- a/drivers/net/ethernet/intel/e1000e/netdev.c
 +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
 @@ -64,8 +64,6 @@ static int debug = -1;  module_param(debug, int, 0);
 MODULE_PARM_DESC(debug, Debug level (0=none,...,16=all));
 
 -static void e1000e_disable_aspm(struct pci_dev *pdev, u16 state);
 -
  static const struct e1000_info *e1000_info_tbl[] = {
   [board_82571]   = e1000_82571_info,
   [board_82572]   = e1000_82572_info,
 @@ -6019,38 +6017,74 @@ static int __e1000_shutdown(struct pci_dev *pdev,
 bool runtime)
   return 0;
  }
 
 -#ifdef CONFIG_PCIEASPM
 -static void __e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
 +/**
 + * e1000e_disable_aspm - Disable ASPM states
 + * @pdev: pointer to PCI device struct
 + * @state: bit-mask of ASPM states to disable
 + *
 + * Some devices *must* have certain ASPM states disabled per hardware
 errata.
 + **/
 +static void e1000e_disable_aspm(struct pci_dev *pdev, u16 state)
  {
 + struct pci_dev *parent = pdev-bus-self;
 + u16 aspm_dis_mask = 0;
 + u16 pdev_aspmc, parent_aspmc;
 +
 + switch (state) {
 + case PCIE_LINK_STATE_L0S:
 + case PCIE_LINK_STATE_L0S + PCIE_LINK_STATE_L1:
 + aspm_dis_mask |= PCI_EXP_LNKCTL_ASPM_L0S;
 + /* fall-through - can't have L1 without L0s */
 + case PCIE_LINK_STATE_L1:
 + aspm_dis_mask |= PCI_EXP_LNKCTL_ASPM_L1;
 + break;
 + default:
 + return;
 + }
 +
 + pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, pdev_aspmc);
 + pdev_aspmc = PCI_EXP_LNKCTL_ASPMC;
 +
 + if (parent) {
 + pcie_capability_read_word(parent, PCI_EXP_LNKCTL,
 +   parent_aspmc);
 + parent_aspmc = PCI_EXP_LNKCTL_ASPMC;
 + }
 +
 + /* Nothing to do if the ASPM states to be disabled already are */
 + if (!(pdev_aspmc  aspm_dis_mask) 
 + (!parent || !(parent_aspmc  aspm_dis_mask)))
 + return;
 +
 + dev_info(pdev-dev, Disabling ASPM %s %s\n,
 +  (aspm_dis_mask  pdev_aspmc 
 PCI_EXP_LNKCTL_ASPM_L0S) ?
 +  L0s : ,
 +  (aspm_dis_mask  pdev_aspmc  PCI_EXP_LNKCTL_ASPM_L1)
 ?
 +  L1 : );
 +
 +#ifdef CONFIG_PCIEASPM
   pci_disable_link_state_locked(pdev, state); -} -#else -static void
 __e1000e_disable_aspm(struct pci_dev *pdev, u16 state) -{
 - u16 aspm_ctl = 0;
 
 - if (state  PCIE_LINK_STATE_L0S)
 - aspm_ctl |= PCI_EXP_LNKCTL_ASPM_L0S;
 - if (state  PCIE_LINK_STATE_L1)
 - aspm_ctl |= PCI_EXP_LNKCTL_ASPM_L1;
 + /* Double-check ASPM control.  If not disabled by the above, the
 +  * BIOS is preventing that from happening (or CONFIG_PCIEASPM is
 +  * not enabled); override by writing PCI config space directly.
 +  */
 + pcie_capability_read_word(pdev, PCI_EXP_LNKCTL, pdev_aspmc);
 + pdev_aspmc = PCI_EXP_LNKCTL_ASPMC;
 +
 + if (!(aspm_dis_mask  pdev_aspmc))
 + return;
 +#endif
 
   /* Both device and parent should have the same

RE: /sys/module/pcie_aspm/parameters/policy not writable?

[Wyborny, Carolyn]

> -Original Message-
> From: Bjorn Helgaas [mailto:bhelg...@google.com]
[..}
>
> We *could* consider something like this, since its scope is limited.
> But since the problem also occurs with Windows, it's pretty likely that 
> there's a
> BIOS update to fix it.  I notice on the X60 support page that there are 
> several
> versions newer than what you're running.
> 
> Bjorn

I believe that some BIOS don't allow user control on this feature, thus we end 
up unable to disable it from driverspace or from userspace, with the method we 
use today in this scenario.

I have a patch almost ready that attempts to disable using the 
pci_disable_link_state() call but then checks again to see if its still enabled 
and if not, then changes the pci config space on the devices on either side of 
the pcie link.  I plan to send it to this list for Pavel's testing as soon as I 
have it finished, at worst end of week.

Thanks,

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation 


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

RE: /sys/module/pcie_aspm/parameters/policy not writable?

[Wyborny, Carolyn]

 -Original Message-
 From: Bjorn Helgaas [mailto:bhelg...@google.com]
[..}

 We *could* consider something like this, since its scope is limited.
 But since the problem also occurs with Windows, it's pretty likely that 
 there's a
 BIOS update to fix it.  I notice on the X60 support page that there are 
 several
 versions newer than what you're running.
 
 Bjorn

I believe that some BIOS don't allow user control on this feature, thus we end 
up unable to disable it from driverspace or from userspace, with the method we 
use today in this scenario.

I have a patch almost ready that attempts to disable using the 
pci_disable_link_state() call but then checks again to see if its still enabled 
and if not, then changes the pci config space on the devices on either side of 
the pcie link.  I plan to send it to this list for Pavel's testing as soon as I 
have it finished, at worst end of week.

Thanks,

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation 


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

RE: /sys/module/pcie_aspm/parameters/policy not writable?

[Wyborny, Carolyn]

> -Original Message-
> From: Bjorn Helgaas [mailto:bhelg...@google.com]
> Sent: Wednesday, July 10, 2013 3:57 PM
> To: Wyborny, Carolyn
> Cc: Pavel Machek; Greg KH; kernel list; Joe Lawrence; Myron Stowe; 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;
> e1000-de...@lists.sourceforge.net; linux-...@vger.kernel.org
> Subject: Re: /sys/module/pcie_aspm/parameters/policy not writable?
> 
> [+cc linux-pci]
> 
> On Wed, Jul 10, 2013 at 10:21:32PM +, Wyborny, Carolyn wrote:
> > > -Original Message-
> > > From: Bjorn Helgaas [mailto:bhelg...@google.com]
> > [..]
> >
> > > Pavel's ThinkPad X60 has two NICs: Intel 82573L and Intel
> > > PRO/Wireless 3945ABG.  I'm pretty sure the problem he's reporting is
> > > with the 82573L.  Ping times are bad (~100msec) when ASPM is enabled, as
> reported by lspci.
> > >
> > > On Pavel's system, the FADT says we shouldn't enable OSPM control of
> > > ASPM (ACPI_FADT_NO_ASPM is set), so we set "aspm_disabled = 1".  One
> > > effect is that we don't blacklist the pre-1.1 82573L device, which I
> > > think results in it being left with the BIOS configuration, which
> > > apparently has ASPM enabled.  (Pavel, could you confirm the BIOS
> > > config, e.g., with "pci=earlydump"?)
> > >
> > > e1000e claims to disable ASPM, but because aspm_disabled is set, the
> > > driver's call to pci_disable_link_state_locked() actually does nothing 
> > > [1].
> >
> > Yes, this is the problem we run into.  It would help if the call to
> pci_disable_link_state_locked() returned an error if ASPM is not disabled as
> requested so that drivers can then do the brute force disabling of it 
> themselves.
> 
> I considered returning an error, but resisted because I think drivers will 
> just
> handle the error by doing the brute-force disable themselves, and then we 
> might
> as well drop the pci_disable_link_state() interface completely.
> 
> I proposed a patch [3] a while ago that made pci_disable_link_state() turn off
> ASPM unconditionally.  That would have the same effect as returning failure 
> and
> having drivers disable ASPM themselves.  But Rafael and Matthew thought it was
> too risky [4] (and I think they're probably right because it does not match 
> the
> Windows behavior).
> 
> So by extension, I guess it would also be risky to return an error and have 
> the
> driver disable ASPM.
> 
> > > I experimented [2] with Windows and found that when a driver
> > > requests PciASPMOptOut, Windows will not touch ASPM config if the
> > > _OSC method fails, i.e., the BIOS declines to grant ASPM control to the 
> > > OS.
> > > However, I do not know if Windows similarly ignores PciASPMOptOut
> > > when the FADT ACPI_FADT_NO_ASPM bit is set.
> > >
> > > The PCI core has failed spectacularly at providing useful ASPM
> > > interfaces.  Do you Intel folks have any suggestions about how to
> > > resolve this?  I assume that the Windows driver for the 82573L must
> > > disable ASPM somehow, even though ACPI_FADT_NO_ASPM is set.  Does it
> > > just use brute-force, as in the version of
> > > __e1000e_disable_aspm() that's used when CONFIG_PCIEASPM is not set?
> >
> > My friends in our Windows development team told me that the driver doesn't
> try to disable  ASPM basically because we can't.  I'm not sure if the same 
> issue
> presents in Windows the same way or not.
> 
> So the Windows driver *never* disables ASPM, not even with its own register
> writes?  So on a machine like Pavel's, it would run with ASPM enabled?  (I'm
> assuming his BIOS leaves ASPM enabled; hopefully Pavel can confirm that.
> 
> If the Windows driver works with ASPM enabled but the Linux driver on the same
> hardware requires ASPM to be disabled, it sounds like the Linux driver just 
> needs
> to be fixed.

So, to clarify, Windows *does* have a problem with these parts if ASPM cannot 
be disabled.  We tell users to disable ASPM  with these parts.  There are 
systems that have BIOS that do not truly disable ASPM even if the user tries, 
even with Windows and the symptoms are as bad as Linux, there's no big 
difference there.  The difference is that Windows doesn't interact with the 
BIOS very much and the Windows driver cannot access PCIconfig space as we can 
with Linux.  

I would argue for the error message so that drivers are not brute-forcing the 
change unless they have to.  Today, a solution would be for us to jus

RE: /sys/module/pcie_aspm/parameters/policy not writable?

[Wyborny, Carolyn]

 -Original Message-
 From: Bjorn Helgaas [mailto:bhelg...@google.com]
 Sent: Wednesday, July 10, 2013 3:57 PM
 To: Wyborny, Carolyn
 Cc: Pavel Machek; Greg KH; kernel list; Joe Lawrence; Myron Stowe; 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;
 e1000-de...@lists.sourceforge.net; linux-...@vger.kernel.org
 Subject: Re: /sys/module/pcie_aspm/parameters/policy not writable?
 
 [+cc linux-pci]
 
 On Wed, Jul 10, 2013 at 10:21:32PM +, Wyborny, Carolyn wrote:
   -Original Message-
   From: Bjorn Helgaas [mailto:bhelg...@google.com]
  [..]
 
   Pavel's ThinkPad X60 has two NICs: Intel 82573L and Intel
   PRO/Wireless 3945ABG.  I'm pretty sure the problem he's reporting is
   with the 82573L.  Ping times are bad (~100msec) when ASPM is enabled, as
 reported by lspci.
  
   On Pavel's system, the FADT says we shouldn't enable OSPM control of
   ASPM (ACPI_FADT_NO_ASPM is set), so we set aspm_disabled = 1.  One
   effect is that we don't blacklist the pre-1.1 82573L device, which I
   think results in it being left with the BIOS configuration, which
   apparently has ASPM enabled.  (Pavel, could you confirm the BIOS
   config, e.g., with pci=earlydump?)
  
   e1000e claims to disable ASPM, but because aspm_disabled is set, the
   driver's call to pci_disable_link_state_locked() actually does nothing 
   [1].
 
  Yes, this is the problem we run into.  It would help if the call to
 pci_disable_link_state_locked() returned an error if ASPM is not disabled as
 requested so that drivers can then do the brute force disabling of it 
 themselves.
 
 I considered returning an error, but resisted because I think drivers will 
 just
 handle the error by doing the brute-force disable themselves, and then we 
 might
 as well drop the pci_disable_link_state() interface completely.
 
 I proposed a patch [3] a while ago that made pci_disable_link_state() turn off
 ASPM unconditionally.  That would have the same effect as returning failure 
 and
 having drivers disable ASPM themselves.  But Rafael and Matthew thought it was
 too risky [4] (and I think they're probably right because it does not match 
 the
 Windows behavior).
 
 So by extension, I guess it would also be risky to return an error and have 
 the
 driver disable ASPM.
 
   I experimented [2] with Windows and found that when a driver
   requests PciASPMOptOut, Windows will not touch ASPM config if the
   _OSC method fails, i.e., the BIOS declines to grant ASPM control to the 
   OS.
   However, I do not know if Windows similarly ignores PciASPMOptOut
   when the FADT ACPI_FADT_NO_ASPM bit is set.
  
   The PCI core has failed spectacularly at providing useful ASPM
   interfaces.  Do you Intel folks have any suggestions about how to
   resolve this?  I assume that the Windows driver for the 82573L must
   disable ASPM somehow, even though ACPI_FADT_NO_ASPM is set.  Does it
   just use brute-force, as in the version of
   __e1000e_disable_aspm() that's used when CONFIG_PCIEASPM is not set?
 
  My friends in our Windows development team told me that the driver doesn't
 try to disable  ASPM basically because we can't.  I'm not sure if the same 
 issue
 presents in Windows the same way or not.
 
 So the Windows driver *never* disables ASPM, not even with its own register
 writes?  So on a machine like Pavel's, it would run with ASPM enabled?  (I'm
 assuming his BIOS leaves ASPM enabled; hopefully Pavel can confirm that.
 
 If the Windows driver works with ASPM enabled but the Linux driver on the same
 hardware requires ASPM to be disabled, it sounds like the Linux driver just 
 needs
 to be fixed.

So, to clarify, Windows *does* have a problem with these parts if ASPM cannot 
be disabled.  We tell users to disable ASPM  with these parts.  There are 
systems that have BIOS that do not truly disable ASPM even if the user tries, 
even with Windows and the symptoms are as bad as Linux, there's no big 
difference there.  The difference is that Windows doesn't interact with the 
BIOS very much and the Windows driver cannot access PCIconfig space as we can 
with Linux.  

I would argue for the error message so that drivers are not brute-forcing the 
change unless they have to.  Today, a solution would be for us to just skip the 
pcie_disable_link_state call altogether, since we can't guarantee it will work  
and just brute force it no matter what and perhaps we should consider doing 
this.  But I think not having the error message would make it more likely to 
skip the call altogether.  Can you explain more why you think proving an error 
message or doing the force disabling in the function,if unable to complete the 
disabling, is risky?

Thanks,

Carolyn
  
 
 [3] https://lkml.kernel.org/r/20130510225257.ga10...@google.com
 [4] https://lkml.kernel.org/r/20130516225535.ga27...@google.com
--
To unsubscribe from this list: send the line

RE: /sys/module/pcie_aspm/parameters/policy not writable?

[Wyborny, Carolyn]

> -Original Message-
> From: Bjorn Helgaas [mailto:bhelg...@google.com]
[..]

> Holy cow, you guys have a lot of folks listed in MAINTAINERS for Intel 
> drivers :)
> This is an ASPM question, if that helps narrow down the folks interested.

Well, we try to have everyone involved, I guess.  I work on the server parts in 
the e1000e driver which includes 82573/4 and 82583.

[..]
> Pavel's ThinkPad X60 has two NICs: Intel 82573L and Intel PRO/Wireless
> 3945ABG.  I'm pretty sure the problem he's reporting is with the 82573L.  Ping
> times are bad (~100msec) when ASPM is enabled, as reported by lspci.
> 
> On Pavel's system, the FADT says we shouldn't enable OSPM control of ASPM
> (ACPI_FADT_NO_ASPM is set), so we set "aspm_disabled = 1".  One effect is that
> we don't blacklist the pre-1.1 82573L device, which I think results in it 
> being left
> with the BIOS configuration, which apparently has ASPM enabled.  (Pavel, could
> you confirm the BIOS config, e.g., with "pci=earlydump"?)
>
> e1000e claims to disable ASPM, but because aspm_disabled is set, the driver's
> call to pci_disable_link_state_locked() actually does nothing [1].

Yes, this is the problem we run into.  It would help if the call to 
pci_disable_link_state_locked() returned an error if ASPM is not disabled as 
requested so that drivers can then do the brute force disabling of it 
themselves.

> 
> I experimented [2] with Windows and found that when a driver requests
> PciASPMOptOut, Windows will not touch ASPM config if the _OSC method fails,
> i.e., the BIOS declines to grant ASPM control to the OS.
> However, I do not know if Windows similarly ignores PciASPMOptOut when the
> FADT ACPI_FADT_NO_ASPM bit is set.
> 
> The PCI core has failed spectacularly at providing useful ASPM interfaces.  Do
> you Intel folks have any suggestions about how to resolve this?  I assume that
> the Windows driver for the 82573L must disable ASPM somehow, even though
> ACPI_FADT_NO_ASPM is set.  Does it just use brute-force, as in the version of
> __e1000e_disable_aspm() that's used when CONFIG_PCIEASPM is not set? 

My friends in our Windows development team told me that the driver doesn't try 
to disable  ASPM basically because we can't.  I'm not sure if the same issue 
presents in Windows the same way or not.

Hope this helps.

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation 

> 
> Bjorn
> 
> [1] We just merged 2add0ec1, which adds a "can't disable ASPM; OS doesn't
> have ASPM control" message in this case, but I don't think Pavel's kernel has 
> this
> change.  It doesn't change the behavior anyway.
> 
> [2] https://bugzilla.kernel.org/show_bug.cgi?id=57331
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: /sys/module/pcie_aspm/parameters/policy not writable?

[Wyborny, Carolyn]

 -Original Message-
 From: Bjorn Helgaas [mailto:bhelg...@google.com]
[..]

 Holy cow, you guys have a lot of folks listed in MAINTAINERS for Intel 
 drivers :)
 This is an ASPM question, if that helps narrow down the folks interested.

Well, we try to have everyone involved, I guess.  I work on the server parts in 
the e1000e driver which includes 82573/4 and 82583.

[..]
 Pavel's ThinkPad X60 has two NICs: Intel 82573L and Intel PRO/Wireless
 3945ABG.  I'm pretty sure the problem he's reporting is with the 82573L.  Ping
 times are bad (~100msec) when ASPM is enabled, as reported by lspci.
 
 On Pavel's system, the FADT says we shouldn't enable OSPM control of ASPM
 (ACPI_FADT_NO_ASPM is set), so we set aspm_disabled = 1.  One effect is that
 we don't blacklist the pre-1.1 82573L device, which I think results in it 
 being left
 with the BIOS configuration, which apparently has ASPM enabled.  (Pavel, could
 you confirm the BIOS config, e.g., with pci=earlydump?)

 e1000e claims to disable ASPM, but because aspm_disabled is set, the driver's
 call to pci_disable_link_state_locked() actually does nothing [1].

Yes, this is the problem we run into.  It would help if the call to 
pci_disable_link_state_locked() returned an error if ASPM is not disabled as 
requested so that drivers can then do the brute force disabling of it 
themselves.

 
 I experimented [2] with Windows and found that when a driver requests
 PciASPMOptOut, Windows will not touch ASPM config if the _OSC method fails,
 i.e., the BIOS declines to grant ASPM control to the OS.
 However, I do not know if Windows similarly ignores PciASPMOptOut when the
 FADT ACPI_FADT_NO_ASPM bit is set.
 
 The PCI core has failed spectacularly at providing useful ASPM interfaces.  Do
 you Intel folks have any suggestions about how to resolve this?  I assume that
 the Windows driver for the 82573L must disable ASPM somehow, even though
 ACPI_FADT_NO_ASPM is set.  Does it just use brute-force, as in the version of
 __e1000e_disable_aspm() that's used when CONFIG_PCIEASPM is not set? 

My friends in our Windows development team told me that the driver doesn't try 
to disable  ASPM basically because we can't.  I'm not sure if the same issue 
presents in Windows the same way or not.

Hope this helps.

Carolyn

Carolyn Wyborny 
Linux Development 
Networking Division 
Intel Corporation 

 
 Bjorn
 
 [1] We just merged 2add0ec1, which adds a can't disable ASPM; OS doesn't
 have ASPM control message in this case, but I don't think Pavel's kernel has 
 this
 change.  It doesn't change the behavior anyway.
 
 [2] https://bugzilla.kernel.org/show_bug.cgi?id=57331
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

RE: [PATCH] igb: correct hardware type (i210/i211) check in igb_loopback_test()

[Wyborny, Carolyn]

-Original Message-
From: Jesper Juhl [mailto:j...@chaosbits.net] 
Sent: Wednesday, July 25, 2012 12:06 PM
To: linux-kernel@vger.kernel.org
Cc: net...@vger.kernel.org; e1000-de...@lists.sourceforge.net; Wyborny, 
Carolyn; Pieper, Jeffrey E; Kirsher, Jeffrey T; Rick Jones; Ronciak, John; 
Brandeburg, Jesse; Allan, Bruce W; Skidmore, Donald C; Rose, Gregory V; 
Waskiewicz Jr, Peter P; Duyck, Alexander H; David S. Miller
Subject: [PATCH] igb: correct hardware type (i210/i211) check in 
igb_loopback_test()

In the original code
...
if ((adapter->hw.mac.type == e1000_i210)
|| (adapter->hw.mac.type == e1000_i210)) { ...
the second check of 'adapter->hw.mac.type' is pointless since it tests for the 
exact same value as the first.

After reading through a few other parts of the driver I believe that the second 
check was actually intended to check for 'e1000_i211'
rather than 'e1000_i210', but I admit that I'm not certain so someone with more 
knowledge about this driver should ACK the patch before it gets merged.

Unfortunately I have no hardware to actually test this on, so it is compile 
tested only.

Signed-off-by: Jesper Juhl 
---
 drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c 
b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index a19c84c..ad489b7 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -1783,7 +1783,7 @@ static int igb_loopback_test(struct igb_adapter *adapter, 
u64 *data)
goto out;
}
if ((adapter->hw.mac.type == e1000_i210)
-   || (adapter->hw.mac.type == e1000_i210)) {
+   || (adapter->hw.mac.type == e1000_i211)) {
dev_err(>pdev->dev,
"Loopback test not supported "
"on this part at this time.\n");
--
1.7.11.3


-- 
Jesper Juhlhttp://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

ACK.

Good catch.  Thanks Jesper!

Carolyn

Carolyn Wyborny
Linux Development
LAN Access Division
Intel Corporation



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

RE: [PATCH] igb: correct hardware type (i210/i211) check in igb_loopback_test()

[Wyborny, Carolyn]

-Original Message-
From: Jesper Juhl [mailto:j...@chaosbits.net] 
Sent: Wednesday, July 25, 2012 12:06 PM
To: linux-kernel@vger.kernel.org
Cc: net...@vger.kernel.org; e1000-de...@lists.sourceforge.net; Wyborny, 
Carolyn; Pieper, Jeffrey E; Kirsher, Jeffrey T; Rick Jones; Ronciak, John; 
Brandeburg, Jesse; Allan, Bruce W; Skidmore, Donald C; Rose, Gregory V; 
Waskiewicz Jr, Peter P; Duyck, Alexander H; David S. Miller
Subject: [PATCH] igb: correct hardware type (i210/i211) check in 
igb_loopback_test()

In the original code
...
if ((adapter-hw.mac.type == e1000_i210)
|| (adapter-hw.mac.type == e1000_i210)) { ...
the second check of 'adapter-hw.mac.type' is pointless since it tests for the 
exact same value as the first.

After reading through a few other parts of the driver I believe that the second 
check was actually intended to check for 'e1000_i211'
rather than 'e1000_i210', but I admit that I'm not certain so someone with more 
knowledge about this driver should ACK the patch before it gets merged.

Unfortunately I have no hardware to actually test this on, so it is compile 
tested only.

Signed-off-by: Jesper Juhl j...@chaosbits.net
---
 drivers/net/ethernet/intel/igb/igb_ethtool.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/intel/igb/igb_ethtool.c 
b/drivers/net/ethernet/intel/igb/igb_ethtool.c
index a19c84c..ad489b7 100644
--- a/drivers/net/ethernet/intel/igb/igb_ethtool.c
+++ b/drivers/net/ethernet/intel/igb/igb_ethtool.c
@@ -1783,7 +1783,7 @@ static int igb_loopback_test(struct igb_adapter *adapter, 
u64 *data)
goto out;
}
if ((adapter-hw.mac.type == e1000_i210)
-   || (adapter-hw.mac.type == e1000_i210)) {
+   || (adapter-hw.mac.type == e1000_i211)) {
dev_err(adapter-pdev-dev,
Loopback test not supported 
on this part at this time.\n);
--
1.7.11.3


-- 
Jesper Juhl j...@chaosbits.net   http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.

ACK.

Good catch.  Thanks Jesper!

Carolyn

Carolyn Wyborny
Linux Development
LAN Access Division
Intel Corporation



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

RE: 82571EB: Detected Hardware Unit Hang

[Wyborny, Carolyn]

>-Original Message-
>From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
>On Behalf Of Joe Jin
>Sent: Tuesday, July 10, 2012 12:40 AM
>To: Joe Jin
>Cc: e1000-de...@lists.sf.net; net...@vger.kernel.org; linux-
>ker...@vger.kernel.org
>Subject: Re: 82571EB: Detected Hardware Unit Hang
[..]
>I checked all driver codes I did not found anywhere will set the
>upper.data with
>E1000_TXD_STAT_DD, I guess upper.data be set by hardware?

Yes, the hw sets this bit after transmit, its how the driver knows to reclaim 
it.

>> Would you please help on this?

Yes, we'll attempt to reproduce it and get back to you with any more needed 
info.  I'm sorry you're having problems with our parts.

Thanks for the report,

Carolyn

Carolyn Wyborny
Linux Development
LAN Access Division
Intel Corporation


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

RE: 82571EB: Detected Hardware Unit Hang

[Wyborny, Carolyn]

-Original Message-
From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
On Behalf Of Joe Jin
Sent: Tuesday, July 10, 2012 12:40 AM
To: Joe Jin
Cc: e1000-de...@lists.sf.net; net...@vger.kernel.org; linux-
ker...@vger.kernel.org
Subject: Re: 82571EB: Detected Hardware Unit Hang
[..]
I checked all driver codes I did not found anywhere will set the
upper.data with
E1000_TXD_STAT_DD, I guess upper.data be set by hardware?

Yes, the hw sets this bit after transmit, its how the driver knows to reclaim 
it.

 Would you please help on this?

Yes, we'll attempt to reproduce it and get back to you with any more needed 
info.  I'm sorry you're having problems with our parts.

Thanks for the report,

Carolyn

Carolyn Wyborny
Linux Development
LAN Access Division
Intel Corporation


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