Re: [dpdk-dev] [v2] net/fm10k: fix segment fault at calling fm10k_stats_get()

["Wang, Xiao W" <xiao.w.wang@intel.com>]

Hi

> -----Original Message-----
> From: luqiuwen@iie.ac.cn [mailto:luqiuwen@iie.ac.cn]
> Sent: Tuesday, August 6, 2019 2:34 PM
> To: stable@dpdk.org; dev@dpdk.org
> Cc: Zhang, Qi Z <qi.z.zhang@intel.com>; Wang, Xiao W
> <xiao.w.wang@intel.com>; yskoh@mellanox.com
> Subject: [v2] net/fm10k: fix segment fault at calling fm10k_stats_get()
> 
> From 5658ee7d69023cc4b22315cd9f8e10a832aafdb0 Mon Sep 17 00:00:00
> 2001
> From: Lu Qiuwen <luqiuwen@iie.ac.cn>
> Date: Sat, 15 Jun 2019 14:28:01 +0800
> Subject: [PATCH] net/fm10k: fix segment fault at calling fm10k_stats_get()
>  from secondary process.
> 
> The function pointers in fm10k_stats_get() which setup from primary process,
> when secondary process call these function pointers, a segment fault will
> happend.

The patch tile is too long, we can simplify it to like "fix stats ops in multi process".
Also, the commit log exceeds the maximum length of 80 characters. Also some typos.
devtools/check-git-log.sh can be used to check your commit log format.

Since this is a fix patch, a "Fixes" line is needed. Refer to the other patches.
As a v2 patch, you need to specify what change you have made to the previous v1.

I'm OK with the below code change. After the above issue fixed, please add my ack:
Acked-by: Xiao Wang <xiao.w.wang@intel.com>

Maybe @Qi can help to reword the commit log when committing the patch?

BRs,
Xiao

> 
> Signed-off-by: Lu Qiuwen <luqiuwen@iie.ac.cn>
> ---
>  drivers/net/fm10k/base/fm10k_api.c | 20 ++++++++++++++++----
>  drivers/net/fm10k/base/fm10k_pf.c  |  4 ++--
>  drivers/net/fm10k/base/fm10k_pf.h  |  6 ++++++
>  drivers/net/fm10k/base/fm10k_vf.c  |  4 ++--
>  drivers/net/fm10k/base/fm10k_vf.h  |  5 +++++
>  5 files changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/fm10k/base/fm10k_api.c
> b/drivers/net/fm10k/base/fm10k_api.c
> index c49d20dfb..e7b2fe710 100644
> --- a/drivers/net/fm10k/base/fm10k_api.c
> +++ b/drivers/net/fm10k/base/fm10k_api.c
> @@ -234,8 +234,14 @@ s32 fm10k_read_mac_addr(struct fm10k_hw *hw)
>   * */
>  void fm10k_update_hw_stats(struct fm10k_hw *hw, struct fm10k_hw_stats
> *stats)
>  {
> -	if (hw->mac.ops.update_hw_stats)
> -		hw->mac.ops.update_hw_stats(hw, stats);
> +	switch (hw->mac.type) {
> +	case fm10k_mac_pf:
> +		return fm10k_update_hw_stats_pf(hw, stats);
> +	case fm10k_mac_vf:
> +		return fm10k_update_hw_stats_vf(hw, stats);
> +	default:
> +		break;
> +	}
>  }
> 
>  /**
> @@ -246,8 +252,14 @@ void fm10k_update_hw_stats(struct fm10k_hw *hw,
> struct fm10k_hw_stats *stats)
>   * */
>  void fm10k_rebind_hw_stats(struct fm10k_hw *hw, struct fm10k_hw_stats
> *stats)
>  {
> -	if (hw->mac.ops.rebind_hw_stats)
> -		hw->mac.ops.rebind_hw_stats(hw, stats);
> +	switch (hw->mac.type) {
> +	case fm10k_mac_pf:
> +		return fm10k_rebind_hw_stats_pf(hw, stats);
> +	case fm10k_mac_vf:
> +		return fm10k_rebind_hw_stats_vf(hw, stats);
> +	default:
> +		break;
> +	}
>  }
> 
>  /**
> diff --git a/drivers/net/fm10k/base/fm10k_pf.c
> b/drivers/net/fm10k/base/fm10k_pf.c
> index db5f4912f..f5b6a9e2e 100644
> --- a/drivers/net/fm10k/base/fm10k_pf.c
> +++ b/drivers/net/fm10k/base/fm10k_pf.c
> @@ -1511,7 +1511,7 @@ const struct fm10k_msg_data
> fm10k_iov_msg_data_pf[] = {
>   *  This function collects and aggregates global and per queue hardware
>   *  statistics.
>   **/
> -STATIC void fm10k_update_hw_stats_pf(struct fm10k_hw *hw,
> +void fm10k_update_hw_stats_pf(struct fm10k_hw *hw,
>  				     struct fm10k_hw_stats *stats)
>  {
>  	u32 timeout, ur, ca, um, xec, vlan_drop, loopback_drop,
> nodesc_drop;
> @@ -1584,7 +1584,7 @@ STATIC void fm10k_update_hw_stats_pf(struct
> fm10k_hw *hw,
>   *  This function resets the base for global and per queue hardware
>   *  statistics.
>   **/
> -STATIC void fm10k_rebind_hw_stats_pf(struct fm10k_hw *hw,
> +void fm10k_rebind_hw_stats_pf(struct fm10k_hw *hw,
>  				     struct fm10k_hw_stats *stats)
>  {
>  	DEBUGFUNC("fm10k_rebind_hw_stats_pf");
> diff --git a/drivers/net/fm10k/base/fm10k_pf.h
> b/drivers/net/fm10k/base/fm10k_pf.h
> index ca125c273..2c22bdd02 100644
> --- a/drivers/net/fm10k/base/fm10k_pf.h
> +++ b/drivers/net/fm10k/base/fm10k_pf.h
> @@ -184,4 +184,10 @@ extern const struct fm10k_msg_data
> fm10k_iov_msg_data_pf[];
>  #endif
> 
>  s32 fm10k_init_ops_pf(struct fm10k_hw *hw);
> +
> +void fm10k_update_hw_stats_pf(struct fm10k_hw *hw,
> +				     struct fm10k_hw_stats *stats);
> +
> +void fm10k_rebind_hw_stats_pf(struct fm10k_hw *hw,
> +				     struct fm10k_hw_stats *stats);
>  #endif /* _FM10K_PF_H */
> diff --git a/drivers/net/fm10k/base/fm10k_vf.c
> b/drivers/net/fm10k/base/fm10k_vf.c
> index bd449773a..2f4b5f5d2 100644
> --- a/drivers/net/fm10k/base/fm10k_vf.c
> +++ b/drivers/net/fm10k/base/fm10k_vf.c
> @@ -526,7 +526,7 @@ const struct fm10k_tlv_attr fm10k_1588_msg_attr[] =
> {
>   *
>   *  This function collects and aggregates per queue hardware statistics.
>   **/
> -STATIC void fm10k_update_hw_stats_vf(struct fm10k_hw *hw,
> +void fm10k_update_hw_stats_vf(struct fm10k_hw *hw,
>  				     struct fm10k_hw_stats *stats)
>  {
>  	DEBUGFUNC("fm10k_update_hw_stats_vf");
> @@ -541,7 +541,7 @@ STATIC void fm10k_update_hw_stats_vf(struct
> fm10k_hw *hw,
>   *
>   *  This function resets the base for queue hardware statistics.
>   **/
> -STATIC void fm10k_rebind_hw_stats_vf(struct fm10k_hw *hw,
> +void fm10k_rebind_hw_stats_vf(struct fm10k_hw *hw,
>  				     struct fm10k_hw_stats *stats)
>  {
>  	DEBUGFUNC("fm10k_rebind_hw_stats_vf");
> diff --git a/drivers/net/fm10k/base/fm10k_vf.h
> b/drivers/net/fm10k/base/fm10k_vf.h
> index 116c56fcc..d4edd330e 100644
> --- a/drivers/net/fm10k/base/fm10k_vf.h
> +++ b/drivers/net/fm10k/base/fm10k_vf.h
> @@ -89,4 +89,9 @@ extern const struct fm10k_tlv_attr
> fm10k_1588_msg_attr[];
>  	FM10K_MSG_HANDLER(FM10K_VF_MSG_ID_1588,
> fm10k_1588_msg_attr, func)
> 
>  s32 fm10k_init_ops_vf(struct fm10k_hw *hw);
> +
> +void fm10k_update_hw_stats_vf(struct fm10k_hw *hw,
> +				     struct fm10k_hw_stats *stats);
> +void fm10k_rebind_hw_stats_vf(struct fm10k_hw *hw,
> +				     struct fm10k_hw_stats *stats);
>  #endif /* _FM10K_VF_H */
> --
> 2.20.1.windows.1
>