Re: [PATCH v5 6/7] nvme-fcloop: implement 'host_traddr'

[James Smart <jsmart2021@gmail.com>]

On 5/26/2024 10:15 PM, Daniel Wagner wrote:
> From: Hannes Reinecke <hare@kernel.org>
> 
> Implement the 'host_traddr' callback to display the host transport
> address for nvmet debugfs.
> 
> Signed-off-by: Hannes Reinecke <hare@kernel.org>
> Reviewed-by: Sagi Grimberg <sagi@grimberg.me>
> Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
>   drivers/nvme/target/fcloop.c | 11 +++++++++++
>   1 file changed, 11 insertions(+)
> 
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index 913cd2ec7a6f..e1abb27927ff 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -492,6 +492,16 @@ fcloop_t2h_host_release(void *hosthandle)
>   	/* host handle ignored for now */
>   }
>   
> +static int
> +fcloop_t2h_host_traddr(void *hosthandle, u64 *wwnn, u64 *wwpn)
> +{
> +	struct fcloop_rport *rport = hosthandle;
> +
> +	*wwnn = rport->lport->localport->node_name;
> +	*wwpn = rport->lport->localport->port_name;
> +	return 0;
> +}
> +
>   /*
>    * Simulate reception of RSCN and converting it to a initiator transport
>    * call to rescan a remote port.
> @@ -1074,6 +1084,7 @@ static struct nvmet_fc_target_template tgttemplate = {
>   	.ls_req			= fcloop_t2h_ls_req,
>   	.ls_abort		= fcloop_t2h_ls_abort,
>   	.host_release		= fcloop_t2h_host_release,
> +	.host_traddr		= fcloop_t2h_host_traddr,
>   	.max_hw_queues		= FCLOOP_HW_QUEUES,
>   	.max_sgl_segments	= FCLOOP_SGL_SEGS,
>   	.max_dif_sgl_segments	= FCLOOP_SGL_SEGS,

Daniel,

I found it odd that you had to modify fcloop, but now see what the issue 
is.  We should have had the WWN of the initiator passed to the fc tgt 
transport at the same time hosthandle was first passed. There's several 
ways to do this, such as the one you coded or modifying the ls_rcv call. 
Downside to these are they change the template or calling routines and 
breaks the driver/os binary api in an os that picks them up.

I'd like to look at doing this a little differently. Change hosthandle, 
instead of pointing to a void, points to a host structure, with the host 
structure containing WWPN, WWNN, and N_Port_ID. We can set a 
target_features bit to know if the structure is a void or a host struct. 
This keeps the driver/os api in place.  I'll put something together.

Thoughts ?

-- james