-----Original Message-----
From: James Smart <jsmart2021@xxxxxxxxx>
Sent: Friday, April 15, 2022 10:02 PM
To: Nilesh Javali <njavali@xxxxxxxxxxx>; martin.petersen@xxxxxxxxxx
Cc: linux-scsi@xxxxxxxxxxxxxxx; GR-QLogic-Storage-Upstream <GR-QLogic-Storage-Upstream@xxxxxxxxxxx>; Muneendra Kumar M <muneendra.kumar@xxxxxxxxxxxx>; James Smart <jsmart2021@xxxxxxxxx>
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@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxx>
> Signed-off-by: Nilesh Javali <njavali@xxxxxxxxxxx>
> ---
> 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
