On 4/27/2022 8:44 PM, Anil Gurumurthy wrote:
-----Original Message-----
From: James Smart <jsmart2021@xxxxxxxxx>
...
@@ -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
