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

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

[Nilesh Javali]

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++;
 	}
 }
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;
 
 		desc_cnt++;
 		bytes_remain -= FC_TLV_DESC_SZ_FROM_LENGTH(tlv);
diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
index e80a7c542c88..a987db5c7d63 100644
--- a/include/scsi/scsi_transport_fc.h
+++ b/include/scsi/scsi_transport_fc.h
@@ -856,7 +856,7 @@ void fc_host_post_fc_event(struct Scsi_Host *shost, u32 event_number,
 	 * Note: when calling fc_host_post_fc_event(), vendor_id may be
 	 *   specified as 0.
 	 */
-void fc_host_fpin_rcv(struct Scsi_Host *shost, u32 fpin_len, char *fpin_buf);
+void fc_host_fpin_rcv(struct Scsi_Host *shost, u32 fpin_len, char *fpin_buf, bool hba_process);
 struct fc_vport *fc_vport_create(struct Scsi_Host *shost, int channel,
 		struct fc_vport_identifiers *);
 int fc_vport_terminate(struct fc_vport *vport);
-- 
2.23.1

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

[James Smart]

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

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

[Anil Gurumurthy]

-----Original Message-----
From: James Smart <jsmart2021@gmail.com>
Sent: Friday, April 15, 2022 10:02 PM
To: Nilesh Javali <njavali@marvell.com>; martin.petersen@oracle.com
Cc: linux-scsi@vger.kernel.org; GR-QLogic-Storage-Upstream <GR-QLogic-Storage-Upstream@marvell.com>; Muneendra Kumar M <muneendra.kumar@broadcom.com>; James Smart <jsmart2021@gmail.com>
Subject: [EXT] Re: [RFC] scsi_transport_fc: Add an additional flag to fc_host_fpin_rcv()

External Email

----------------------------------------------------------------------
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.

The thought process here was that the driver sets the values according to way it has processed. So if the driver does not process Link Integrity FPIN, the same can be set accordingly.

>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.

Sure, we could do that, but I thought the daemon was not a permanent solution and eventually the decision on marginality would be taken within the kernel.

>-- james

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

[James Smart]

On 4/27/2022 8:44 PM, Anil Gurumurthy wrote:
> 
> -----Original Message-----
> From: James Smart <jsmart2021@gmail.com>
...
>> @@ -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.
> 
> The thought process here was that the driver sets the values according to way it has processed. So if the driver does not process Link Integrity FPIN, the same can be set accordingly.

ok - but that didn't address what I said.  As defined right now, 
"hba_process" means "if set to 0, mark port state MARGINAL. if set 
non-zero, ignore".  There is no validation of what type of FPIN, etc.

Given "hba_process" is ambigous, I think you should change that name to 
"info_only" or "log_only" or something similar - e.g. what we want to 
communicate is the following: normally fpin_rcv is normally called with 
this flag set to false and that the transport or daemon can act on the 
event.  But the new flag, when true, should only log or display the 
event but not act on it.

>> 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.
> 
> Sure, we could do that, but I thought the daemon was not a permanent solution and eventually the decision on marginality would be taken within the kernel.

Currently the daemon is the only thing in existence and it was only 
recently added.  When the functionality goes into the kernel, we can 
adapt anything then. What does not make sense is immediately changing 
state as of an (any) FPIN receive.

Regardless - to support the deamon and as "log only or not" is 
meaningful, please add it to the event.

-- james