Re: [Intel-wired-lan] [PATCH] intel: ice: Do not enable NAPI on q_vectors that have no rings

[Venkataramanan, Anirudh]

On Thu, 2018-11-29 at 01:54 +, Yang Xiao wrote:
> From: Young Xiao 
> 
> If ice driver has q_vectors w/ active NAPI that has no rings,
> then this will result in a divide by zero error. To correct it
> I am updating the driver code so that we only support NAPI on
> q_vectors that have 1 or more rings allocated to them.
> 
> See commit 13a8cd191a2b ("i40e: Do not enable NAPI on q_vectors
> that have no rings") for detail.
> 
> Signed-off-by: Young Xiao 
> ---
>  drivers/net/ethernet/intel/ice/ice_main.c | 16 
>  1 file changed, 12 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
> b/drivers/net/ethernet/intel/ice/ice_main.c
> index 12a..9450004 100644
> --- a/drivers/net/ethernet/intel/ice/ice_main.c
> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> @@ -2563,8 +2563,12 @@ static void ice_napi_enable_all(struct ice_vsi
> *vsi)
>   if (!vsi->netdev)
>   return;
>  
> - for (q_idx = 0; q_idx < vsi->num_q_vectors; q_idx++)
> - napi_enable(>q_vectors[q_idx]->napi);
> + for (q_idx = 0; q_idx < vsi->num_q_vectors; q_idx++) {
> + struct ice_q_vector *q_vector = vsi-
> >q_vectors[q_idx];
> +
> + if (q_vector->rx.ring || q_vector->tx.ring)
> + napi_enable(_vector->napi);
> + }
>  }
>  
>  /**
> @@ -2931,8 +2935,12 @@ static void ice_napi_disable_all(struct
> ice_vsi *vsi)
>   if (!vsi->netdev)
>   return;
>  
> - for (q_idx = 0; q_idx < vsi->num_q_vectors; q_idx++)
> - napi_disable(>q_vectors[q_idx]->napi);
> + for (q_idx = 0; q_idx < vsi->num_q_vectors; q_idx++) {
> + struct ice_q_vector *q_vector = vsi-
> >q_vectors[q_idx];
> +
> + if (q_vector->rx.ring || q_vector->tx.ring)
> + napi_disable(_vector->napi);
> + }
>  }
>  
>  /**

Acked-by: Anirudh Venkataramanan 

Thanks for the patch, Yang!

- Ani


smime.p7s
Description: S/MIME cryptographic signature

Re: [Intel-wired-lan] [PATCH net-next] ice: Fix error return code in ice_init_hw()

[Venkataramanan, Anirudh]

On Wed, 2018-03-28 at 12:50 +, Wei Yongjun wrote:
> Fix to return error code ICE_ERR_NO_MEMORY from the alloc error
> handling case instead of 0, as done elsewhere in this function.
> 
> Fixes: dc49c7723676 ("ice: Get MAC/PHY/link info and scheduler
> topology")
> Signed-off-by: Wei Yongjun 
> ---
>  drivers/net/ethernet/intel/ice/ice_common.c | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/net/ethernet/intel/ice/ice_common.c
> b/drivers/net/ethernet/intel/ice/ice_common.c
> index 385f5d4..21977ec 100644
> --- a/drivers/net/ethernet/intel/ice/ice_common.c
> +++ b/drivers/net/ethernet/intel/ice/ice_common.c
> @@ -468,8 +468,10 @@ enum ice_status ice_init_hw(struct ice_hw *hw)
>   mac_buf_len = sizeof(struct ice_aqc_manage_mac_read_resp);
>   mac_buf = devm_kzalloc(ice_hw_to_dev(hw), mac_buf_len,
> GFP_KERNEL);
>  
> - if (!mac_buf)
> + if (!mac_buf) {
> + status = ICE_ERR_NO_MEMORY;
>   goto err_unroll_fltr_mgmt_struct;
> + }
>  
>   status = ice_aq_manage_mac_read(hw, mac_buf, mac_buf_len,
> NULL);
>   devm_kfree(ice_hw_to_dev(hw), mac_buf);
> 

Acked-by: Anirudh Venkataramanan 

Thanks!
Ani

smime.p7s
Description: S/MIME cryptographic signature

Re: [Intel-wired-lan] [PATCH 03/15] ice: Start hardware initialization

[Venkataramanan, Anirudh]

On Thu, 2018-03-15 at 10:00 -0700, Shannon Nelson wrote:
> On 3/14/2018 3:05 PM, Venkataramanan, Anirudh wrote:
> > On Mon, 2018-03-12 at 19:05 -0700, Shannon Nelson wrote:
> > > On 3/9/2018 9:21 AM, Anirudh Venkataramanan wrote:
> 
> 
> > > > +
> > > > +/**
> > > > + * ice_read_sr_aq - Read Shadow RAM.
> > > > + * @hw: pointer to the HW structure
> > > > + * @offset: offset in words from module start
> > > > + * @words: number of words to read
> > > > + * @data: buffer for words reads from Shadow RAM
> > > > + * @last_command: tells the AdminQ that this is the last
> > > > command
> > > > + *
> > > > + * Reads 16-bit word buffers from the Shadow RAM using the
> > > > admin
> > > > command.
> > > > + */
> > > > +static enum ice_status
> > > > +ice_read_sr_aq(struct ice_hw *hw, u32 offset, u16 words, u16
> > > > *data,
> > > > +  bool last_command)
> > > > +{
> > > > +   enum ice_status status;
> > > > +
> > > > +   status = ice_check_sr_access_params(hw, offset,
> > > > words);
> > > > +   if (!status)
> > > > +   status = ice_aq_read_nvm(hw, 0, 2 * offset, 2
> > > > *
> > > > words, data,
> > > 
> > > Why the doubling of offset and words?  If this is some general
> > > adjustment made for the AQ interface, it should be made in
> > > ice_aq_read_nvm().  If not, then some explanation is needed here.
> > 
> > ice_read_sr_aq expects a word offset and size in words. The
> > ice_aq_read_nvm interface expects offset and size in bytes. The
> > doubling is a conversion from word offset/size to byte offset/size.
> 
> In that case, this might be a good place for a small comment for
> readers 
> like me who don't have the spec available.

Sure thing! :-)

> 
> sln
> 

smime.p7s
Description: S/MIME cryptographic signature

Re: [Intel-wired-lan] [PATCH 03/15] ice: Start hardware initialization

[Venkataramanan, Anirudh]

On Mon, 2018-03-12 at 19:05 -0700, Shannon Nelson wrote:
> On 3/9/2018 9:21 AM, Anirudh Venkataramanan wrote:
> > This patch implements multiple pieces of the initialization flow
> > as follows:
> > 
> > 1) A reset is issued to ensure a clean device state, followed
> > by initialization of admin queue interface.
> > 
> > 2) Once the admin queue interface is up, clear the PF config
> > and transition the device to non-PXE mode.
> > 
> > 3) Get the NVM configuration stored in the device's non-volatile
> > memory (NVM) using ice_init_nvm.
> > 
> > Signed-off-by: Anirudh Venkataramanan  > .com>
> > ---
> >   drivers/net/ethernet/intel/ice/Makefile |   3 +-
> >   drivers/net/ethernet/intel/ice/ice.h|   2 +
> >   drivers/net/ethernet/intel/ice/ice_adminq_cmd.h |  79 +
> >   drivers/net/ethernet/intel/ice/ice_common.c | 410
> > 
> >   drivers/net/ethernet/intel/ice/ice_common.h |  11 +
> >   drivers/net/ethernet/intel/ice/ice_controlq.h   |   3 +
> >   drivers/net/ethernet/intel/ice/ice_hw_autogen.h |  30 ++
> >   drivers/net/ethernet/intel/ice/ice_main.c   |  31 ++
> >   drivers/net/ethernet/intel/ice/ice_nvm.c| 245
> > ++
> >   drivers/net/ethernet/intel/ice/ice_osdep.h  |   1 +
> >   drivers/net/ethernet/intel/ice/ice_status.h |   5 +
> >   drivers/net/ethernet/intel/ice/ice_type.h   |  49 +++
> >   12 files changed, 868 insertions(+), 1 deletion(-)
> >   create mode 100644 drivers/net/ethernet/intel/ice/ice_nvm.c
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/Makefile
> > b/drivers/net/ethernet/intel/ice/Makefile
> > index eebf619e84a8..373d481dbb25 100644
> > --- a/drivers/net/ethernet/intel/ice/Makefile
> > +++ b/drivers/net/ethernet/intel/ice/Makefile
> > @@ -26,4 +26,5 @@ obj-$(CONFIG_ICE) += ice.o
> >   
> >   ice-y := ice_main.o   \
> >  ice_controlq.o \
> > -ice_common.o
> > +ice_common.o   \
> > +ice_nvm.o
> > diff --git a/drivers/net/ethernet/intel/ice/ice.h
> > b/drivers/net/ethernet/intel/ice/ice.h
> > index ea2fb63bb095..ab2800c31906 100644
> > --- a/drivers/net/ethernet/intel/ice/ice.h
> > +++ b/drivers/net/ethernet/intel/ice/ice.h
> > @@ -30,8 +30,10 @@
> >   #include 
> >   #include "ice_devids.h"
> >   #include "ice_type.h"
> > +#include "ice_common.h"
> >   
> >   #define ICE_BAR0  0
> > +#define ICE_AQ_LEN 64
> >   
> >   #define ICE_DFLT_NETIF_M (NETIF_MSG_DRV | NETIF_MSG_PROBE |
> > NETIF_MSG_LINK)
> >   
> > diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> > b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> > index 885fa3c6fec4..05b22a1ffd70 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> > +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h
> > @@ -50,6 +50,67 @@ struct ice_aqc_q_shutdown {
> > u8 reserved[12];
> >   };
> >   
> > +/* Request resource ownership (direct 0x0008)
> > + * Release resource ownership (direct 0x0009)
> > + */
> > +struct ice_aqc_req_res {
> > +   __le16 res_id;
> > +#define ICE_AQC_RES_ID_NVM 1
> > +#define ICE_AQC_RES_ID_SDP 2
> > +#define ICE_AQC_RES_ID_CHNG_LOCK   3
> > +#define ICE_AQC_RES_ID_GLBL_LOCK   4
> > +   __le16 access_type;
> > +#define ICE_AQC_RES_ACCESS_READ1
> > +#define ICE_AQC_RES_ACCESS_WRITE   2
> > +
> > +   /* Upon successful completion, FW writes this value and
> > driver is
> > +* expected to release resource before timeout. This value
> > is provided
> > +* in milliseconds.
> > +*/
> > +   __le32 timeout;
> > +#define ICE_AQ_RES_NVM_READ_DFLT_TIMEOUT_MS3000
> > +#define ICE_AQ_RES_NVM_WRITE_DFLT_TIMEOUT_MS   18
> > +#define ICE_AQ_RES_CHNG_LOCK_DFLT_TIMEOUT_MS   1000
> > +#define ICE_AQ_RES_GLBL_LOCK_DFLT_TIMEOUT_MS   3000
> > +   /* For SDP: pin id of the SDP */
> > +   __le32 res_number;
> > +   /* Status is only used for ICE_AQC_RES_ID_GLBL_LOCK */
> > +   __le16 status;
> > +#define ICE_AQ_RES_GLBL_SUCCESS0
> > +#define ICE_AQ_RES_GLBL_IN_PROG1
> > +#define ICE_AQ_RES_GLBL_DONE   2
> > +   u8 reserved[2];
> 
> Since these structs all become part of the descriptor's param union, 
> perhaps adding reserved space to the end is not necessary.
> 
> > +};
> > +
> > +/* Clear PXE Command and response (direct 0x0110) */
> > +struct ice_aqc_clear_pxe {
> > +   u8 rx_cnt;
> > +#define ICE_AQC_CLEAR_PXE_RX_CNT   0x2
> > +   u8 reserved[15];
> > +};
> > +
> > +/* NVM Read command (indirect 0x0701)
> > + * NVM Erase commands (direct 0x0702)
> > + * NVM Update commands (indirect 0x0703)
> > + */
> > +struct ice_aqc_nvm {
> > +   u8  cmd_flags;
> > +#define ICE_AQC_NVM_LAST_CMD   BIT(0)
> > +#define ICE_AQC_NVM_PCIR_REQ   BIT(0)  /* Used
> > by NVM Update reply */
> > +#define ICE_AQC_NVM_PRESERVATION_S 1
> > +#define ICE_AQC_NVM_PRESERVATION_M (3 <<
> > CSR_AQ_NVM_PRESERVATION_S)
> > +#define 

Re: [PATCH 12/15] ice: Add stats and ethtool support

[Venkataramanan, Anirudh]

On Sat, 2018-03-10 at 08:42 -0800, Stephen Hemminger wrote:
> On Fri,  9 Mar 2018 09:21:33 -0800
> Anirudh Venkataramanan  wrote:
> 
> > +   /* VSI stats */
> > +   struct rtnl_link_stats64 net_stats;
> > +   struct rtnl_link_stats64 net_stats_prev;
> > +   struct ice_eth_stats eth_stats;
> > +   struct ice_eth_stats eth_stats_prev;
> 
> You also don't need current and previous as separate copies since
> previous is only
> used while computing the current values.

Thanks for the feedback, Stephen.

eth_stats_prev is used in ice_update_eth_stats when we update
eth_stats.

While looking into this though, I found that net_stats_prev field in
struct ice_vsi (and consequently *prev_ns and *prev_es pointers in
ice_update_vsi_stats) may not be needed. Is this what you meant?

Thanks!
Ani


smime.p7s
Description: S/MIME cryptographic signature

Re: [PATCH 12/15] ice: Add stats and ethtool support

[Venkataramanan, Anirudh]

On Fri, 2018-03-09 at 15:14 -0800, Jakub Kicinski wrote:
> On Fri,  9 Mar 2018 09:21:33 -0800, Anirudh Venkataramanan wrote:
> > +static const struct ice_stats ice_net_stats[] = {
> > +   ICE_NETDEV_STAT(rx_packets),
> > +   ICE_NETDEV_STAT(tx_packets),
> > +   ICE_NETDEV_STAT(rx_bytes),
> > +   ICE_NETDEV_STAT(tx_bytes),
> > +   ICE_NETDEV_STAT(rx_errors),
> > +   ICE_NETDEV_STAT(tx_errors),
> > +   ICE_NETDEV_STAT(rx_dropped),
> > +   ICE_NETDEV_STAT(tx_dropped),
> > +   ICE_NETDEV_STAT(multicast),
> > +   ICE_NETDEV_STAT(rx_length_errors),
> > +   ICE_NETDEV_STAT(rx_crc_errors),
> > +};
> 
> Please don't duplicate standard netdev stats in ethtool -S.

Jacub,

Thanks for the feedback. I am not sure I understand what's being asked
here. Do you mean to say that standard netdev stats should not be
printed when we do ethtool -S or something else?

Thanks!
Ani

smime.p7s
Description: S/MIME cryptographic signature