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

[James Smart <jsmart2021@gmail.com>]

On 5/28/2024 3:32 PM, James Smart wrote:
> 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
> 

It's a little involved in the hba driver to do what I was suggesting. So 
I think the easiest is what was proposed.

Reviewed-by: James Smart <jsmart2021@gmail.com>

-- james