Re: [PATCH v5 3/7] scsi: ibmvfc: define asynchronous sub-queue

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The patch is heavily flawed due to a botched rebase or split: it introduces dead, duplicate structures and an unused pointer, while the commit message describes changes that are completely absent from the diff.
--

--- Patch [3]: [PATCH v5 3/7] scsi: ibmvfc: define asynchronous sub-queue ---
commit 3e3cb1faf7e598eb3299f36518825168d60237a2
Author: Dave Marquardt <[email protected]>

scsi: ibmvfc: define asynchronous sub-queue

This commit defines data structures for asynchronous sub-queue support required
for full and extended FPIN functionality.

> Update ibmvfc_channel_setup structure to include async_subq_handle field
> and reduce IBMVFC_MAX_CHANNELS from 502 to 501 to accommodate the async
> sub-queue. Add async_scrq pointer to ibmvfc_channels structure.
> 
> Add capability flags IBMVFC_USE_ASYNC_SUBQ and IBMVFC_SUPPORT_ASYNC_SUBQ
> for negotiating async sub-queue support with VIOS during login.

[Severity: Medium]
Are the changes described in these paragraphs of the commit message missing
from this patch? The updates to ibmvfc_channel_setup, the reduction of
IBMVFC_MAX_CHANNELS, and the new capability flags do not appear in the
diff below.

> diff --git a/drivers/scsi/ibmvscsi/ibmvfc.h b/drivers/scsi/ibmvscsi/ibmvfc.h
> index e8ca0d30f483f..3711be4b6748c 100644
> --- a/drivers/scsi/ibmvscsi/ibmvfc.h
> +++ b/drivers/scsi/ibmvscsi/ibmvfc.h
> @@ -791,6 +791,25 @@ struct ibmvfc_async_work {
>  	struct work_struct async_work_s;
>  };
>  
> +struct ibmvfc_async_subq {
> +	volatile u8 valid;
> +#define IBMVFC_ASYNC_ID_IS_ASSOC_ID	0x01
> +#define IBMVFC_FC_EEH			0x04
> +#define IBMVFC_FC_FW_UPDATE		0x08
> +#define IBMVFC_FC_FW_DUMP		0x10
> +	u8 flags;

[Severity: Medium]
Is this new ibmvfc_async_subq structure actually needed, or could this be an
accidental duplicate from a rebase?

It appears to be functionally identical to the pre-existing
ibmvfc_async_sub_crq defined earlier in the same file:

struct ibmvfc_async_sub_crq {
	volatile u8 valid;
	u8 flags;
#define IBMVFC_ASYNC_ID_IS_ASSOC_ID	0x01
	u8 link_state;
	u8 fpin_status;
	...

This newly added structure doesn't appear to be used anywhere.

[ ... ]

> @@ -935,6 +954,7 @@ struct ibmvfc_queue {
>  
>  struct ibmvfc_channels {
>  	struct ibmvfc_queue *scrqs;
> +	struct ibmvfc_queue *async_scrq;

[Severity: Medium]
Is this async_scrq pointer intended to be used? It is added to
ibmvfc_channels here but appears completely unused.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3