Re: [RFC] scsi_transport_fc: Add an additional flag to fc_host_fpin_rcv()

[James Smart <jsmart2021@gmail.com>]

On 4/13/2022 11:43 PM, Nilesh Javali wrote:
> From: Anil Gurumurthy <agurumurthy@marvell.com>
> 
> The LLDD and the stack currently process FPINs received from the fabric,
> but the stack is not aware of any action taken by the driver to alleviate
> congestion. The current interface between the driver and the SCSI stack is
> limited to passing the notification mainly for statistics and heuristics.
> 
> The reaction to an FPIN could be handled either by the driver or by the
> stack (marginal path and failover). This patch enhances the interface to
> indicate if action on an FPIN has already been taken by the LLDDs or not.
> Add an additional flag to fc_host_fpin_rcv() to indicate if the FPIN has
> been processed by the driver.
> 
> Signed-off-by: Anil Gurumurthy <agurumurthy@marvell.com>
> Signed-off-by: Nilesh Javali <njavali@marvell.com>
> ---
>   drivers/scsi/lpfc/lpfc_els.c     | 2 +-
>   drivers/scsi/qla2xxx/qla_isr.c   | 2 +-
>   drivers/scsi/scsi_transport_fc.c | 6 +++++-
>   include/scsi/scsi_transport_fc.h | 2 +-
>   4 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> index ef6e8cd8c26a..fd931b1781e3 100644
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c
> @@ -10102,7 +10102,7 @@ lpfc_els_rcv_fpin(struct lpfc_vport *vport, void *p, u32 fpin_length)
>   		/* Send every descriptor individually to the upper layer */
>   		if (deliver)
>   			fc_host_fpin_rcv(lpfc_shost_from_vport(vport),
> -					 fpin_length, (char *)fpin);
> +					 fpin_length, (char *)fpin, false);
>   		desc_cnt++;
>   	}
>   }



This is fine.  Thank you.


> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index 21b31d6359c8..e01d9a671749 100644
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
> @@ -45,7 +45,7 @@ qla27xx_process_purex_fpin(struct scsi_qla_host *vha, struct purex_item *item)
>   	ql_dump_buffer(ql_dbg_init + ql_dbg_verbose, vha, 0x508f,
>   		       pkt, pkt_size);
>   
> -	fc_host_fpin_rcv(vha->host, pkt_size, (char *)pkt);
> +	fc_host_fpin_rcv(vha->host, pkt_size, (char *)pkt, false);
>   }
>   
>   const char *const port_state_str[] = {
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> index a2524106206d..6de476f13512 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -892,12 +892,13 @@ fc_fpin_congn_stats_update(struct Scsi_Host *shost,
>    * @shost:		host the FPIN was received on
>    * @fpin_len:		length of FPIN payload, in bytes
>    * @fpin_buf:		pointer to FPIN payload
> + * @hba_process:	true if LLDD has acted on the FPIN
>    *
>    * Notes:
>    *	This routine assumes no locks are held on entry.
>    */
>   void
> -fc_host_fpin_rcv(struct Scsi_Host *shost, u32 fpin_len, char *fpin_buf)
> +fc_host_fpin_rcv(struct Scsi_Host *shost, u32 fpin_len, char *fpin_buf, bool hba_process)
>   {
>   	struct fc_els_fpin *fpin = (struct fc_els_fpin *)fpin_buf;
>   	struct fc_tlv_desc *tlv;
> @@ -925,6 +926,9 @@ fc_host_fpin_rcv(struct Scsi_Host *shost, u32 fpin_len, char *fpin_buf)
>   		case ELS_DTAG_CONGESTION:
>   			fc_fpin_congn_stats_update(shost, tlv);
>   		}
> +		/* If the event has not been processed, mark path as marginal */
> +		if (!hba_process)
> +			fc_host_port_state(shost) = FC_PORTSTATE_MARGINAL;

This doesn't seem right.  I would think the meaning of "processing" 
varies by FPIN type, thus the flag should be passed to the different 
xxx_update routines and any decision would be made there - or at least 
the decision is made within the type case statement above. For example: 
FC_PORT_STATE_MARGINAL should only be set with Link Integrity events.

However, we currently leave the decision of marginality to be determined 
and managed by the external daemon that processes the fpin events. So 
the patch needs to expand the fpin event to include the driver processed 
flag in the event data. Please add this to fc_host_post_fc_event().

Given we leave marginality to the daemon, who will now know whether the 
driver handled the fpin or not, I don't think fpin_rcv should include 
the changing of portstate to marginal.

-- james