Re: [PATCH 11/29] ibmvfc: send NVMe target discovery MAD

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Parallel SCSI and NVMe discover targets MADs race to advance the host initialization state machine, causing the slower protocol's targets to be permanently ignored and breaking retry handling.
- [Low] The newly added `proto_type` array is missing the `static` keyword, making it a globally visible symbol.
--

commit e89ce7e682793b58e5019c746ffde18602fe635b
Author: Tyrel Datwyler <tyreld@linux.ibm.com>

ibmvfc: send NVMe target discovery MAD

This commit extends target discovery to send protocol-specific
discover-target MADs for NVMe/FC. It introduces a parallel discovery flow
for NVMe alongside SCSI targets.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc-core.c b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> index efac82c48258a..53480d150042e 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc-core.c
> +++ b/drivers/scsi/ibmvscsi/ibmvfc-core.c
> @@ -173,6 +173,11 @@ static const struct {
>  	{ IBMVFC_FC_SCSI_ERROR, IBMVFC_COMMAND_FAILED, DID_ERROR, 0, 1, "PRLI to device failed." },
>  };
>  
> +const char *proto_type[] = {
> +	"SCSI",
> +	"NVMe",
> +};

[Severity: Low]
This isn't a bug, but can this array be declared as static? It is currently
exposed to the global kernel namespace which could lead to linker
collisions.

> +
>  static void ibmvfc_npiv_login(struct ibmvfc_host *);
>  static void ibmvfc_tgt_send_prli(struct ibmvfc_target *);
>  static void ibmvfc_tgt_send_plogi(struct ibmvfc_target *);

[ ... ]

> @@ -5001,19 +5006,30 @@ static void ibmvfc_discover_targets_done(struct ibmvfc_event *evt)
>  {
>  	struct ibmvfc_host *vhost = evt->vhost;
>  	struct ibmvfc_discover_targets *rsp = &evt->xfer_iu->discover_targets;
> +	struct ibmvfc_channels *channels;
>  	u32 mad_status = be16_to_cpu(rsp->common.status);
> +	u32 opcode = be32_to_cpu(rsp->common.opcode);
>  	int level = IBMVFC_DEFAULT_LOG_LEVEL;
>  
> +	if (opcode == IBMVFC_DISC_TARGETS)
> +		channels = &vhost->scsi_scrqs;
> +	else
> +		channels = &vhost->nvme_scrqs;
> +
>  	switch (mad_status) {
>  	case IBMVFC_MAD_SUCCESS:
> -		ibmvfc_dbg(vhost, "Discover Targets succeeded\n");
> -		vhost->scsi_scrqs.num_targets = min_t(u32, be32_to_cpu(rsp->num_written),
> -						      max_targets);
> +		ibmvfc_dbg(vhost, "Discover %s Targets succeeded\n",
> +			   proto_type[channels->protocol]);
> +		channels->num_targets = min_t(u32, be32_to_cpu(rsp->num_written),
> +					      max_targets);
> +		ibmvfc_dbg(vhost, "%d %s targets found\n", channels->num_targets,
> +			   proto_type[channels->protocol]);
>  		ibmvfc_set_host_action(vhost, IBMVFC_HOST_ACTION_ALLOC_TGTS);

[Severity: High]
Does this code create a race condition between the SCSI and NVMe discovery
MADs?

If both SCSI and NVMe discoveries are running asynchronously, whichever MAD
completes first will call ibmvfc_set_host_action() and advance the host
state from IBMVFC_HOST_ACTION_INIT_WAIT to IBMVFC_HOST_ACTION_ALLOC_TGTS.

When the slower MAD subsequently completes, ibmvfc_set_host_action()
will ignore the transition to IBMVFC_HOST_ACTION_ALLOC_TGTS because the
state is no longer IBMVFC_HOST_ACTION_INIT_WAIT. This causes the targets
from the slower protocol to be permanently ignored.

Additionally, if the first MAD fails, could the state machine revert to
IBMVFC_HOST_ACTION_INIT and issue a new batch of discovery commands
while the slower MAD is still in-flight?

>  		break;
>  	case IBMVFC_MAD_FAILED:
>  		level += ibmvfc_retry_host_init(vhost);

[ ... ]

> @@ -5074,9 +5090,29 @@ static void ibmvfc_discover_targets(struct ibmvfc_host *vhost)
>  	ibmvfc_set_host_action(vhost, IBMVFC_HOST_ACTION_INIT_WAIT);
>  
>  	if (!ibmvfc_send_event(evt, vhost, default_timeout))
> -		ibmvfc_dbg(vhost, "Sent discover targets\n");
> +		ibmvfc_dbg(vhost, "Sent discover SCSI targets\n");
>  	else
> -		ibmvfc_link_down(vhost, IBMVFC_LINK_DEAD);
> +		goto link_down;
> +
> +	if (!ibmvfc_nvme_active(vhost))
> +		return;
> +
> +	evt = ibmvfc_get_disc_event(&vhost->nvme_scrqs);
> +	if (!evt) {
> +		ibmvfc_log(vhost, level, "Discover NVMe Targets failed: no available events\n");
> +		ibmvfc_hard_reset_host(vhost);
> +		return;
> +	}
> +
> +	if (!ibmvfc_send_event(evt, vhost, default_timeout))
> +		ibmvfc_dbg(vhost, "Sent discover NVMe targets\n");
> +	else
> +		goto link_down;
> +
> +	return;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623013035.3436640-1-tyreld@linux.ibm.com?part=11