Re: [PATCH net-next v2 06/15] net/smc: Add diagnostic information to link structure

[Saeed Mahameed <saeed@kernel.org>]

On Tue, 2020-11-03 at 11:25 +0100, Karsten Graul wrote:
> From: Guvenc Gulce <guvenc@linux.ibm.com>
> 
> During link creation add network and ib-device name to
> link structure. This is needed for diagnostic purposes.
> 
> When diagnostic information is gathered, we need to traverse
> device, linkgroup and link structures, to be able to do that
> we need to hold a spinlock for the linkgroup list, without this
> diagnostic information in link structure, another device list
> mutex holding would be necessary to dereference the device
> pointer in the link structure which would be impossible when
> holding a spinlock already.
> 
> Signed-off-by: Guvenc Gulce <guvenc@linux.ibm.com>
> Signed-off-by: Karsten Graul <kgraul@linux.ibm.com>
> ---
>  net/smc/smc_core.c | 10 ++++++++++
>  net/smc/smc_core.h |  3 +++
>  2 files changed, 13 insertions(+)
> 
> diff --git a/net/smc/smc_core.c b/net/smc/smc_core.c
> index da94725deb09..28fc583d9033 100644
> --- a/net/smc/smc_core.c
> +++ b/net/smc/smc_core.c
> @@ -303,6 +303,15 @@ static u8 smcr_next_link_id(struct
> smc_link_group *lgr)
>  	return link_id;
>  }
>  
> +static inline void smcr_copy_dev_info_to_link(struct smc_link *link)
> +{
> +	struct smc_ib_device *smcibdev = link->smcibdev;
> +
> +	memcpy(link->ibname, smcibdev->ibdev->name, sizeof(link-
> >ibname));
> +	memcpy(link->ndevname, smcibdev->netdev[link->ibport - 1],
> +	       sizeof(link->ndevname));

snprintf() 

> +}
> +
>  int smcr_link_init(struct smc_link_group *lgr, struct smc_link *lnk,
>  		   u8 link_idx, struct smc_init_info *ini)
>  {
> @@ -317,6 +326,7 @@ int smcr_link_init(struct smc_link_group *lgr,
> struct smc_link *lnk,
>  	lnk->smcibdev = ini->ib_dev;
>  	lnk->ibport = ini->ib_port;
>  	atomic_inc(&ini->ib_dev->lnk_cnt_by_port[ini->ib_port - 1]);
> +	smcr_copy_dev_info_to_link(lnk);
>  	lnk->path_mtu = ini->ib_dev->pattr[ini->ib_port -
> 1].active_mtu;
>  	atomic_set(&lnk->conn_cnt, 0);
>  	smc_llc_link_set_uid(lnk);
> diff --git a/net/smc/smc_core.h b/net/smc/smc_core.h
> index 83a88a4635db..bd16d63c5222 100644
> --- a/net/smc/smc_core.h
> +++ b/net/smc/smc_core.h
> @@ -124,6 +124,9 @@ struct smc_link {
>  	u8			link_is_asym;	/* is link
> asymmetric? */
>  	struct smc_link_group	*lgr;		/* parent link group
> */
>  	struct work_struct	link_down_wrk;	/* wrk to bring link
> down */
> +	/* Diagnostic relevant link information */
> +	u8			ibname[IB_DEVICE_NAME_MAX];/* ib
> device name */
> +	u8			ndevname[IFNAMSIZ];/* network device
> name */
> 

IMHO, Comments are redundant.  and you could define name buffers as
char array for more natural string manipulation down the road.