* [PATCH 01/13] fc_els: use 'union fc_tlv_desc'
2026-08-12 18:12 [PATCH 00/13] nvme-fc: FPIN link integrity handling Jesse Taube
@ 2026-08-12 18:12 ` Jesse Taube
2026-08-12 18:26 ` sashiko-bot
2026-08-12 18:12 ` [PATCH 02/13] nvme: add NVME_CTRL_MARGINAL flag Jesse Taube
` (11 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Jesse Taube @ 2026-08-12 18:12 UTC (permalink / raw)
To: linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Justin Tee, Naresh Gottumukkala,
Paul Ely, Chaitanya Kulkarni, James E.J. Bottomley,
Martin K. Petersen, Nilesh Javali, GR-QLogic-Storage-Upstream,
Hannes Reinecke, Jesse Taube, Gustavo A. R. Silva, John Meneghini,
Bryan Gurney, Chris Leech, Ewan D . Milne, shinichiro.kawasaki,
linux-doc, linux-kernel, linux-nvme, linux-block, Hannes Reinecke
From: Hannes Reinecke <hare@kernel.org>
Introduce 'union fc_tlv_desc' to have a common structure for all FC
ELS TLV structures and avoid type casts.
Commit 44b6169ada7f ("scsi: fc: Avoid -Wflex-array-member-not-at-end
warnings")
conflicts with this change. To solve the problem I moved the
fc_els_rdf_hdr and fc_els_rdf_resp_hdr definitions out of fc_els.h and
into lpfc_hw4.h.
Fixes: 44b6169ada7f ("scsi: fc: Avoid -Wflex-array-member-not-at-end warnings")
Cc: Gustavo A. R. Silva <gustavoars@kernel.org>
Co-developed-by: John Meneghini <jmeneghi@redhat.com>
Signed-off-by: John Meneghini <jmeneghi@redhat.com>
Signed-off-by: Hannes Reinecke <hare@kernel.org>
Tested-by: Bryan Gurney <bgurney@redhat.com>
---
V10 -> V11:
- No change/Rebase
---
drivers/scsi/lpfc/lpfc_els.c | 77 +++++++-------
drivers/scsi/lpfc/lpfc_hw4.h | 12 ++-
drivers/scsi/scsi_transport_fc.c | 27 ++---
include/uapi/scsi/fc/fc_els.h | 174 ++++++++++++++++---------------
4 files changed, 146 insertions(+), 144 deletions(-)
diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
index 52fc5058976d..11143da24b54 100644
--- a/drivers/scsi/lpfc/lpfc_els.c
+++ b/drivers/scsi/lpfc/lpfc_els.c
@@ -3888,7 +3888,7 @@ lpfc_issue_els_rdf(struct lpfc_vport *vport, uint8_t retry)
memset(prdf, 0, cmdsize);
prdf->rdf.fpin_cmd = ELS_RDF;
prdf->rdf.desc_len = cpu_to_be32(sizeof(struct lpfc_els_rdf_req) -
- sizeof(struct fc_els_rdf_hdr));
+ offsetof(struct fc_els_rdf, desc));
prdf->reg_d1.reg_desc.desc_tag = cpu_to_be32(ELS_DTAG_FPIN_REGISTER);
prdf->reg_d1.reg_desc.desc_len = cpu_to_be32(
FC_TLV_DESC_LENGTH_FROM_SZ(prdf->reg_d1));
@@ -4075,7 +4075,7 @@ lpfc_cmpl_els_edc(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
{
IOCB_t *irsp_iocb;
struct fc_els_edc_resp *edc_rsp;
- struct fc_tlv_desc *tlv;
+ union fc_tlv_desc *tlv;
struct fc_diag_cg_sig_desc *pcgd;
struct fc_diag_lnkflt_desc *plnkflt;
struct lpfc_dmabuf *pcmd, *prsp;
@@ -4166,7 +4166,7 @@ lpfc_cmpl_els_edc(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
goto out;
}
- dtag = be32_to_cpu(tlv->desc_tag);
+ dtag = be32_to_cpu(tlv->hdr.desc_tag);
switch (dtag) {
case ELS_DTAG_LNK_FAULT_CAP:
if (bytes_remain < FC_TLV_DESC_SZ_FROM_LENGTH(tlv) ||
@@ -4181,7 +4181,7 @@ lpfc_cmpl_els_edc(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
sizeof(struct fc_diag_lnkflt_desc));
goto out;
}
- plnkflt = (struct fc_diag_lnkflt_desc *)tlv;
+ plnkflt = &tlv->lnkflt;
lpfc_printf_log(phba, KERN_INFO,
LOG_ELS | LOG_LDS_EVENT,
"4617 Link Fault Desc Data: 0x%08x 0x%08x "
@@ -4208,7 +4208,7 @@ lpfc_cmpl_els_edc(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
goto out;
}
- pcgd = (struct fc_diag_cg_sig_desc *)tlv;
+ pcgd = &tlv->cg_sig;
lpfc_printf_log(
phba, KERN_INFO, LOG_ELS | LOG_CGN_MGMT,
"4616 CGN Desc Data: 0x%08x 0x%08x "
@@ -4263,10 +4263,8 @@ lpfc_cmpl_els_edc(struct lpfc_hba *phba, struct lpfc_iocbq *cmdiocb,
}
static void
-lpfc_format_edc_lft_desc(struct lpfc_hba *phba, struct fc_tlv_desc *tlv)
+lpfc_format_edc_lft_desc(struct lpfc_hba *phba, struct fc_diag_lnkflt_desc *lft)
{
- struct fc_diag_lnkflt_desc *lft = (struct fc_diag_lnkflt_desc *)tlv;
-
lft->desc_tag = cpu_to_be32(ELS_DTAG_LNK_FAULT_CAP);
lft->desc_len = cpu_to_be32(
FC_TLV_DESC_LENGTH_FROM_SZ(struct fc_diag_lnkflt_desc));
@@ -4279,10 +4277,8 @@ lpfc_format_edc_lft_desc(struct lpfc_hba *phba, struct fc_tlv_desc *tlv)
}
static void
-lpfc_format_edc_cgn_desc(struct lpfc_hba *phba, struct fc_tlv_desc *tlv)
+lpfc_format_edc_cgn_desc(struct lpfc_hba *phba, struct fc_diag_cg_sig_desc *cgd)
{
- struct fc_diag_cg_sig_desc *cgd = (struct fc_diag_cg_sig_desc *)tlv;
-
/* We are assuming cgd was zero'ed before calling this routine */
/* Configure the congestion detection capability */
@@ -4381,7 +4377,7 @@ lpfc_issue_els_edc(struct lpfc_vport *vport, uint8_t retry)
struct lpfc_hba *phba = vport->phba;
struct lpfc_iocbq *elsiocb;
struct fc_els_edc *edc_req;
- struct fc_tlv_desc *tlv;
+ union fc_tlv_desc *tlv;
u16 cmdsize;
struct lpfc_nodelist *ndlp;
u8 *pcmd = NULL;
@@ -4420,13 +4416,13 @@ lpfc_issue_els_edc(struct lpfc_vport *vport, uint8_t retry)
tlv = edc_req->desc;
if (cgn_desc_size) {
- lpfc_format_edc_cgn_desc(phba, tlv);
+ lpfc_format_edc_cgn_desc(phba, &tlv->cg_sig);
phba->cgn_sig_freq = lpfc_fabric_cgn_frequency;
tlv = fc_tlv_next_desc(tlv);
}
if (lft_desc_size)
- lpfc_format_edc_lft_desc(phba, tlv);
+ lpfc_format_edc_lft_desc(phba, &tlv->lnkflt);
lpfc_printf_vlog(vport, KERN_INFO, LOG_ELS | LOG_CGN_MGMT,
"4623 Xmit EDC to remote "
@@ -5999,7 +5995,7 @@ lpfc_issue_els_edc_rsp(struct lpfc_vport *vport, struct lpfc_iocbq *cmdiocb,
{
struct lpfc_hba *phba = vport->phba;
struct fc_els_edc_resp *edc_rsp;
- struct fc_tlv_desc *tlv;
+ union fc_tlv_desc *tlv;
struct lpfc_iocbq *elsiocb;
IOCB_t *icmd, *cmd;
union lpfc_wqe128 *wqe;
@@ -6043,10 +6039,10 @@ lpfc_issue_els_edc_rsp(struct lpfc_vport *vport, struct lpfc_iocbq *cmdiocb,
FC_TLV_DESC_LENGTH_FROM_SZ(struct fc_els_lsri_desc));
edc_rsp->lsri.rqst_w0.cmd = ELS_EDC;
tlv = edc_rsp->desc;
- lpfc_format_edc_cgn_desc(phba, tlv);
+ lpfc_format_edc_cgn_desc(phba, &tlv->cg_sig);
tlv = fc_tlv_next_desc(tlv);
if (lft_desc_size)
- lpfc_format_edc_lft_desc(phba, tlv);
+ lpfc_format_edc_lft_desc(phba, &tlv->lnkflt);
lpfc_debugfs_disc_trc(vport, LPFC_DISC_TRC_ELS_RSP,
"Issue EDC ACC: did:x%x flg:x%lx refcnt %d",
@@ -9439,7 +9435,7 @@ lpfc_els_rcv_edc(struct lpfc_vport *vport, struct lpfc_iocbq *cmdiocb,
{
struct lpfc_hba *phba = vport->phba;
struct fc_els_edc *edc_req;
- struct fc_tlv_desc *tlv;
+ union fc_tlv_desc *tlv;
uint8_t *payload;
uint32_t *ptr, dtag;
const char *dtag_nm;
@@ -9482,7 +9478,7 @@ lpfc_els_rcv_edc(struct lpfc_vport *vport, struct lpfc_iocbq *cmdiocb,
goto out;
}
- dtag = be32_to_cpu(tlv->desc_tag);
+ dtag = be32_to_cpu(tlv->hdr.desc_tag);
switch (dtag) {
case ELS_DTAG_LNK_FAULT_CAP:
if (bytes_remain < FC_TLV_DESC_SZ_FROM_LENGTH(tlv) ||
@@ -9497,7 +9493,7 @@ lpfc_els_rcv_edc(struct lpfc_vport *vport, struct lpfc_iocbq *cmdiocb,
sizeof(struct fc_diag_lnkflt_desc));
goto out;
}
- plnkflt = (struct fc_diag_lnkflt_desc *)tlv;
+ plnkflt = &tlv->lnkflt;
lpfc_printf_log(phba, KERN_INFO,
LOG_ELS | LOG_LDS_EVENT,
"4626 Link Fault Desc Data: x%08x len x%x "
@@ -9534,7 +9530,7 @@ lpfc_els_rcv_edc(struct lpfc_vport *vport, struct lpfc_iocbq *cmdiocb,
phba->cgn_sig_freq = lpfc_fabric_cgn_frequency;
lpfc_least_capable_settings(
- phba, (struct fc_diag_cg_sig_desc *)tlv);
+ phba, &tlv->cg_sig);
break;
default:
dtag_nm = lpfc_get_tlv_dtag_nm(dtag);
@@ -10124,14 +10120,13 @@ lpfc_display_fpin_wwpn(struct lpfc_hba *phba, __be64 *wwnlist, u32 cnt)
/**
* lpfc_els_rcv_fpin_li - Process an FPIN Link Integrity Event.
* @phba: Pointer to phba object.
- * @tlv: Pointer to the Link Integrity Notification Descriptor.
+ * @li: Pointer to the Link Integrity Notification Descriptor.
*
* This function processes a Link Integrity FPIN event by logging a message.
**/
static void
-lpfc_els_rcv_fpin_li(struct lpfc_hba *phba, struct fc_tlv_desc *tlv)
+lpfc_els_rcv_fpin_li(struct lpfc_hba *phba, struct fc_fn_li_desc *li)
{
- struct fc_fn_li_desc *li = (struct fc_fn_li_desc *)tlv;
const char *li_evt_str;
u32 li_evt, cnt;
@@ -10155,14 +10150,13 @@ lpfc_els_rcv_fpin_li(struct lpfc_hba *phba, struct fc_tlv_desc *tlv)
/**
* lpfc_els_rcv_fpin_del - Process an FPIN Delivery Event.
* @phba: Pointer to hba object.
- * @tlv: Pointer to the Delivery Notification Descriptor TLV
+ * @del: Pointer to the Delivery Notification Descriptor TLV
*
* This function processes a Delivery FPIN event by logging a message.
**/
static void
-lpfc_els_rcv_fpin_del(struct lpfc_hba *phba, struct fc_tlv_desc *tlv)
+lpfc_els_rcv_fpin_del(struct lpfc_hba *phba, struct fc_fn_deli_desc *del)
{
- struct fc_fn_deli_desc *del = (struct fc_fn_deli_desc *)tlv;
const char *del_rsn_str;
u32 del_rsn;
__be32 *frame;
@@ -10193,14 +10187,14 @@ lpfc_els_rcv_fpin_del(struct lpfc_hba *phba, struct fc_tlv_desc *tlv)
/**
* lpfc_els_rcv_fpin_peer_cgn - Process a FPIN Peer Congestion Event.
* @phba: Pointer to hba object.
- * @tlv: Pointer to the Peer Congestion Notification Descriptor TLV
+ * @pc: Pointer to the Peer Congestion Notification Descriptor TLV
*
* This function processes a Peer Congestion FPIN event by logging a message.
**/
static void
-lpfc_els_rcv_fpin_peer_cgn(struct lpfc_hba *phba, struct fc_tlv_desc *tlv)
+lpfc_els_rcv_fpin_peer_cgn(struct lpfc_hba *phba,
+ struct fc_fn_peer_congn_desc *pc)
{
- struct fc_fn_peer_congn_desc *pc = (struct fc_fn_peer_congn_desc *)tlv;
const char *pc_evt_str;
u32 pc_evt, cnt;
@@ -10228,7 +10222,7 @@ lpfc_els_rcv_fpin_peer_cgn(struct lpfc_hba *phba, struct fc_tlv_desc *tlv)
/**
* lpfc_els_rcv_fpin_cgn - Process an FPIN Congestion notification
* @phba: Pointer to hba object.
- * @tlv: Pointer to the Congestion Notification Descriptor TLV
+ * @cgn: Pointer to the Congestion Notification Descriptor TLV
*
* This function processes an FPIN Congestion Notifiction. The notification
* could be an Alarm or Warning. This routine feeds that data into driver's
@@ -10237,10 +10231,9 @@ lpfc_els_rcv_fpin_peer_cgn(struct lpfc_hba *phba, struct fc_tlv_desc *tlv)
* to the upper layer or 0 to indicate don't deliver it.
**/
static int
-lpfc_els_rcv_fpin_cgn(struct lpfc_hba *phba, struct fc_tlv_desc *tlv)
+lpfc_els_rcv_fpin_cgn(struct lpfc_hba *phba, struct fc_fn_congn_desc *cgn)
{
struct lpfc_cgn_info *cp;
- struct fc_fn_congn_desc *cgn = (struct fc_fn_congn_desc *)tlv;
const char *cgn_evt_str;
u32 cgn_evt;
const char *cgn_sev_str;
@@ -10341,7 +10334,7 @@ lpfc_els_rcv_fpin(struct lpfc_vport *vport, void *p, u32 fpin_length)
{
struct lpfc_hba *phba = vport->phba;
struct fc_els_fpin *fpin = (struct fc_els_fpin *)p;
- struct fc_tlv_desc *tlv, *first_tlv, *current_tlv;
+ union fc_tlv_desc *tlv, *first_tlv, *current_tlv;
const char *dtag_nm;
int desc_cnt = 0, bytes_remain, cnt;
u32 dtag, deliver = 0;
@@ -10366,7 +10359,7 @@ lpfc_els_rcv_fpin(struct lpfc_vport *vport, void *p, u32 fpin_length)
return;
}
- tlv = (struct fc_tlv_desc *)&fpin->fpin_desc[0];
+ tlv = &fpin->fpin_desc[0];
first_tlv = tlv;
bytes_remain = fpin_length - offsetof(struct fc_els_fpin, fpin_desc);
bytes_remain = min_t(u32, bytes_remain, be32_to_cpu(fpin->desc_len));
@@ -10374,22 +10367,22 @@ lpfc_els_rcv_fpin(struct lpfc_vport *vport, void *p, u32 fpin_length)
/* process each descriptor separately */
while (bytes_remain >= FC_TLV_DESC_HDR_SZ &&
bytes_remain >= FC_TLV_DESC_SZ_FROM_LENGTH(tlv)) {
- dtag = be32_to_cpu(tlv->desc_tag);
+ dtag = be32_to_cpu(tlv->hdr.desc_tag);
switch (dtag) {
case ELS_DTAG_LNK_INTEGRITY:
- lpfc_els_rcv_fpin_li(phba, tlv);
+ lpfc_els_rcv_fpin_li(phba, &tlv->li);
deliver = 1;
break;
case ELS_DTAG_DELIVERY:
- lpfc_els_rcv_fpin_del(phba, tlv);
+ lpfc_els_rcv_fpin_del(phba, &tlv->deli);
deliver = 1;
break;
case ELS_DTAG_PEER_CONGEST:
- lpfc_els_rcv_fpin_peer_cgn(phba, tlv);
+ lpfc_els_rcv_fpin_peer_cgn(phba, &tlv->peer_congn);
deliver = 1;
break;
case ELS_DTAG_CONGESTION:
- deliver = lpfc_els_rcv_fpin_cgn(phba, tlv);
+ deliver = lpfc_els_rcv_fpin_cgn(phba, &tlv->congn);
break;
default:
dtag_nm = lpfc_get_tlv_dtag_nm(dtag);
@@ -10402,12 +10395,12 @@ lpfc_els_rcv_fpin(struct lpfc_vport *vport, void *p, u32 fpin_length)
return;
}
lpfc_cgn_update_stat(phba, dtag);
- cnt = be32_to_cpu(tlv->desc_len);
+ cnt = be32_to_cpu(tlv->hdr.desc_len);
/* Sanity check descriptor length. The desc_len value does not
* include space for the desc_tag and the desc_len fields.
*/
- len -= (cnt + sizeof(struct fc_tlv_desc));
+ len -= (cnt + sizeof(struct fc_tlv_desc_hdr));
if (len < 0) {
dtag_nm = lpfc_get_tlv_dtag_nm(dtag);
lpfc_printf_log(phba, KERN_WARNING, LOG_CGN_MGMT,
diff --git a/drivers/scsi/lpfc/lpfc_hw4.h b/drivers/scsi/lpfc/lpfc_hw4.h
index 41fa8f3329da..10490b3a05cd 100644
--- a/drivers/scsi/lpfc/lpfc_hw4.h
+++ b/drivers/scsi/lpfc/lpfc_hw4.h
@@ -4926,12 +4926,20 @@ struct lpfc_els_rdf_reg_desc {
};
struct lpfc_els_rdf_req {
- struct fc_els_rdf_hdr rdf; /* hdr up to descriptors */
+ struct {
+ __u8 fpin_cmd; /* command (0x19) */
+ __u8 fpin_zero[3]; /* specified as zero - part of cmd */
+ __be32 desc_len; /* Length of Descriptor List (in bytes) */
+ } rdf; /* hdr up to descriptors */
struct lpfc_els_rdf_reg_desc reg_d1; /* 1st descriptor */
};
struct lpfc_els_rdf_rsp {
- struct fc_els_rdf_resp_hdr rdf_resp; /* hdr up to descriptors */
+ struct {
+ struct fc_els_ls_acc acc_hdr;
+ __be32 desc_list_len; /* Length of response (in bytes) */
+ struct fc_els_lsri_desc lsri;
+ } rdf_resp; /* hdr up to descriptors */
struct lpfc_els_rdf_reg_desc reg_d1; /* 1st descriptor */
};
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 173ed6373f04..02c58f4591d8 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -776,12 +776,11 @@ fc_fpin_pname_stats_update(struct Scsi_Host *shost,
*
*/
static void
-fc_fpin_li_stats_update(struct Scsi_Host *shost, struct fc_tlv_desc *tlv)
+fc_fpin_li_stats_update(struct Scsi_Host *shost, struct fc_fn_li_desc *li_desc)
{
struct fc_rport *rport = NULL;
struct fc_rport *attach_rport = NULL;
struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
- struct fc_fn_li_desc *li_desc = (struct fc_fn_li_desc *)tlv;
u16 event_type = be16_to_cpu(li_desc->event_type);
rport = fc_find_rport_by_wwpn(shost,
@@ -812,12 +811,11 @@ fc_fpin_li_stats_update(struct Scsi_Host *shost, struct fc_tlv_desc *tlv)
*/
static void
fc_fpin_delivery_stats_update(struct Scsi_Host *shost,
- struct fc_tlv_desc *tlv)
+ struct fc_fn_deli_desc *dn_desc)
{
struct fc_rport *rport = NULL;
struct fc_rport *attach_rport = NULL;
struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
- struct fc_fn_deli_desc *dn_desc = (struct fc_fn_deli_desc *)tlv;
u32 reason_code = be32_to_cpu(dn_desc->deli_reason_code);
rport = fc_find_rport_by_wwpn(shost,
@@ -843,12 +841,10 @@ fc_fpin_delivery_stats_update(struct Scsi_Host *shost,
*/
static void
fc_fpin_peer_congn_stats_update(struct Scsi_Host *shost,
- struct fc_tlv_desc *tlv)
+ struct fc_fn_peer_congn_desc *pc_desc)
{
struct fc_rport *rport = NULL;
struct fc_rport *attach_rport = NULL;
- struct fc_fn_peer_congn_desc *pc_desc =
- (struct fc_fn_peer_congn_desc *)tlv;
u16 event_type = be16_to_cpu(pc_desc->event_type);
rport = fc_find_rport_by_wwpn(shost,
@@ -876,10 +872,9 @@ fc_fpin_peer_congn_stats_update(struct Scsi_Host *shost,
*/
static void
fc_fpin_congn_stats_update(struct Scsi_Host *shost,
- struct fc_tlv_desc *tlv)
+ struct fc_fn_congn_desc *congn)
{
struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
- struct fc_fn_congn_desc *congn = (struct fc_fn_congn_desc *)tlv;
fc_cn_stats_update(be16_to_cpu(congn->event_type),
&fc_host->fpin_stats);
@@ -899,32 +894,32 @@ fc_host_fpin_rcv(struct Scsi_Host *shost, u32 fpin_len, char *fpin_buf,
u8 event_acknowledge)
{
struct fc_els_fpin *fpin = (struct fc_els_fpin *)fpin_buf;
- struct fc_tlv_desc *tlv;
+ union fc_tlv_desc *tlv;
u32 bytes_remain;
u32 dtag;
enum fc_host_event_code event_code =
event_acknowledge ? FCH_EVT_LINK_FPIN_ACK : FCH_EVT_LINK_FPIN;
/* Update Statistics */
- tlv = (struct fc_tlv_desc *)&fpin->fpin_desc[0];
+ tlv = &fpin->fpin_desc[0];
bytes_remain = fpin_len - offsetof(struct fc_els_fpin, fpin_desc);
bytes_remain = min_t(u32, bytes_remain, be32_to_cpu(fpin->desc_len));
while (bytes_remain >= FC_TLV_DESC_HDR_SZ &&
bytes_remain >= FC_TLV_DESC_SZ_FROM_LENGTH(tlv)) {
- dtag = be32_to_cpu(tlv->desc_tag);
+ dtag = be32_to_cpu(tlv->hdr.desc_tag);
switch (dtag) {
case ELS_DTAG_LNK_INTEGRITY:
- fc_fpin_li_stats_update(shost, tlv);
+ fc_fpin_li_stats_update(shost, &tlv->li);
break;
case ELS_DTAG_DELIVERY:
- fc_fpin_delivery_stats_update(shost, tlv);
+ fc_fpin_delivery_stats_update(shost, &tlv->deli);
break;
case ELS_DTAG_PEER_CONGEST:
- fc_fpin_peer_congn_stats_update(shost, tlv);
+ fc_fpin_peer_congn_stats_update(shost, &tlv->peer_congn);
break;
case ELS_DTAG_CONGESTION:
- fc_fpin_congn_stats_update(shost, tlv);
+ fc_fpin_congn_stats_update(shost, &tlv->congn);
}
bytes_remain -= FC_TLV_DESC_SZ_FROM_LENGTH(tlv);
diff --git a/include/uapi/scsi/fc/fc_els.h b/include/uapi/scsi/fc/fc_els.h
index dca6a28f4e86..b5653e1dee13 100644
--- a/include/uapi/scsi/fc/fc_els.h
+++ b/include/uapi/scsi/fc/fc_els.h
@@ -259,12 +259,12 @@ enum fc_ls_tlv_dtag {
/*
- * Generic Link Service TLV Descriptor format
+ * Generic Link Service TLV Descriptor header
*
* This structure, as it defines no payload, will also be referred to
* as the "tlv header" - which contains the tag and len fields.
*/
-struct fc_tlv_desc {
+struct fc_tlv_desc_hdr {
__be32 desc_tag; /* Notification Descriptor Tag */
__be32 desc_len; /* Length of Descriptor (in bytes).
* Size of descriptor excluding
@@ -273,36 +273,6 @@ struct fc_tlv_desc {
__u8 desc_value[]; /* Descriptor Value */
};
-/* Descriptor tag and len fields are considered the mandatory header
- * for a descriptor
- */
-#define FC_TLV_DESC_HDR_SZ sizeof(struct fc_tlv_desc)
-
-/*
- * Macro, used when initializing payloads, to return the descriptor length.
- * Length is size of descriptor minus the tag and len fields.
- */
-#define FC_TLV_DESC_LENGTH_FROM_SZ(desc) \
- (sizeof(desc) - FC_TLV_DESC_HDR_SZ)
-
-/* Macro, used on received payloads, to return the descriptor length */
-#define FC_TLV_DESC_SZ_FROM_LENGTH(tlv) \
- (__be32_to_cpu((tlv)->desc_len) + FC_TLV_DESC_HDR_SZ)
-
-/*
- * This helper is used to walk descriptors in a descriptor list.
- * Given the address of the current descriptor, which minimally contains a
- * tag and len field, calculate the address of the next descriptor based
- * on the len field.
- */
-static inline void *fc_tlv_next_desc(void *desc)
-{
- struct fc_tlv_desc *tlv = desc;
-
- return (desc + FC_TLV_DESC_SZ_FROM_LENGTH(tlv));
-}
-
-
/*
* Link Service Request Information Descriptor
*/
@@ -1100,19 +1070,6 @@ struct fc_fn_congn_desc {
__u8 resv[3]; /* reserved - must be zero */
};
-/*
- * ELS_FPIN - Fabric Performance Impact Notification
- */
-struct fc_els_fpin {
- __u8 fpin_cmd; /* command (0x16) */
- __u8 fpin_zero[3]; /* specified as zero - part of cmd */
- __be32 desc_len; /* Length of Descriptor List (in bytes).
- * Size of ELS excluding fpin_cmd,
- * fpin_zero and desc_len fields.
- */
- struct fc_tlv_desc fpin_desc[]; /* Descriptor list */
-};
-
/* Diagnostic Function Descriptor - FPIN Registration */
struct fc_df_desc_fpin_reg {
/* New members MUST be added within the __struct_group() macro below. */
@@ -1136,42 +1093,6 @@ struct fc_df_desc_fpin_reg {
_Static_assert(offsetof(struct fc_df_desc_fpin_reg, desc_tags) == sizeof(struct fc_df_desc_fpin_reg_hdr),
"struct member likely outside of __struct_group()");
-/*
- * ELS_RDF - Register Diagnostic Functions
- */
-struct fc_els_rdf {
- /* New members MUST be added within the __struct_group() macro below. */
- __struct_group(fc_els_rdf_hdr, __hdr, /* no attrs */,
- __u8 fpin_cmd; /* command (0x19) */
- __u8 fpin_zero[3]; /* specified as zero - part of cmd */
- __be32 desc_len; /* Length of Descriptor List (in bytes).
- * Size of ELS excluding fpin_cmd,
- * fpin_zero and desc_len fields.
- */
- );
- struct fc_tlv_desc desc[]; /* Descriptor list */
-};
-_Static_assert(offsetof(struct fc_els_rdf, desc) == sizeof(struct fc_els_rdf_hdr),
- "struct member likely outside of __struct_group()");
-
-/*
- * ELS RDF LS_ACC Response.
- */
-struct fc_els_rdf_resp {
- /* New members MUST be added within the __struct_group() macro below. */
- __struct_group(fc_els_rdf_resp_hdr, __hdr, /* no attrs */,
- struct fc_els_ls_acc acc_hdr;
- __be32 desc_list_len; /* Length of response (in
- * bytes). Excludes acc_hdr
- * and desc_list_len fields.
- */
- struct fc_els_lsri_desc lsri;
- );
- struct fc_tlv_desc desc[]; /* Supported Descriptor list */
-};
-_Static_assert(offsetof(struct fc_els_rdf_resp, desc) == sizeof(struct fc_els_rdf_resp_hdr),
- "struct member likely outside of __struct_group()");
-
/*
* Diagnostic Capability Descriptors for EDC ELS
*/
@@ -1241,6 +1162,65 @@ struct fc_diag_cg_sig_desc {
struct fc_diag_cg_sig_freq rcv_signal_frequency;
};
+/*
+ * Generic Link Service TLV Descriptor format
+ *
+ * This structure, as it defines no payload, will also be referred to
+ * as the "tlv header" - which contains the tag and len fields.
+ */
+union fc_tlv_desc {
+ struct fc_tlv_desc_hdr hdr;
+ struct fc_els_lsri_desc lsri;
+ struct fc_fn_li_desc li;
+ struct fc_fn_deli_desc deli;
+ struct fc_fn_peer_congn_desc peer_congn;
+ struct fc_fn_congn_desc congn;
+ struct fc_df_desc_fpin_reg fpin_reg;
+ struct fc_diag_lnkflt_desc lnkflt;
+ struct fc_diag_cg_sig_desc cg_sig;
+};
+
+/* Descriptor tag and len fields are considered the mandatory header
+ * for a descriptor
+ */
+#define FC_TLV_DESC_HDR_SZ sizeof(struct fc_tlv_desc_hdr)
+
+/*
+ * Macro, used when initializing payloads, to return the descriptor length.
+ * Length is size of descriptor minus the tag and len fields.
+ */
+#define FC_TLV_DESC_LENGTH_FROM_SZ(desc) \
+ (sizeof(desc) - FC_TLV_DESC_HDR_SZ)
+
+/* Macro, used on received payloads, to return the descriptor length */
+#define FC_TLV_DESC_SZ_FROM_LENGTH(tlv) \
+ (__be32_to_cpu((tlv)->hdr.desc_len) + FC_TLV_DESC_HDR_SZ)
+
+/*
+ * This helper is used to walk descriptors in a descriptor list.
+ * Given the address of the current descriptor, which minimally contains a
+ * tag and len field, calculate the address of the next descriptor based
+ * on the len field.
+ */
+static inline union fc_tlv_desc *fc_tlv_next_desc(union fc_tlv_desc *desc)
+{
+ return (union fc_tlv_desc *)((__u8 *)desc + FC_TLV_DESC_SZ_FROM_LENGTH(desc));
+}
+
+
+/*
+ * ELS_FPIN - Fabric Performance Impact Notification
+ */
+struct fc_els_fpin {
+ __u8 fpin_cmd; /* command (0x16) */
+ __u8 fpin_zero[3]; /* specified as zero - part of cmd */
+ __be32 desc_len; /* Length of Descriptor List (in bytes).
+ * Size of ELS excluding fpin_cmd,
+ * fpin_zero and desc_len fields.
+ */
+ union fc_tlv_desc fpin_desc[]; /* Descriptor list */
+};
+
/*
* ELS_EDC - Exchange Diagnostic Capabilities
*/
@@ -1251,10 +1231,37 @@ struct fc_els_edc {
* Size of ELS excluding edc_cmd,
* edc_zero and desc_len fields.
*/
- struct fc_tlv_desc desc[];
+ union fc_tlv_desc desc[];
/* Diagnostic Descriptor list */
};
+/*
+ * ELS_RDF - Register Diagnostic Functions
+ */
+struct fc_els_rdf {
+ __u8 fpin_cmd; /* command (0x19) */
+ __u8 fpin_zero[3]; /* specified as zero - part of cmd */
+ __be32 desc_len; /* Length of Descriptor List (in bytes).
+ * Size of ELS excluding fpin_cmd,
+ * fpin_zero and desc_len fields.
+ */
+ union fc_tlv_desc desc[]; /* Descriptor list */
+};
+
+/*
+ * ELS RDF LS_ACC Response.
+ */
+struct fc_els_rdf_resp {
+ struct fc_els_ls_acc acc_hdr;
+ __be32 desc_list_len; /* Length of response (in
+ * bytes). Excludes acc_hdr
+ * and desc_list_len fields.
+ */
+ struct fc_els_lsri_desc lsri;
+ union fc_tlv_desc desc[]; /* Supported Descriptor list */
+};
+
+
/*
* ELS EDC LS_ACC Response.
*/
@@ -1265,9 +1272,8 @@ struct fc_els_edc_resp {
* and desc_list_len fields.
*/
struct fc_els_lsri_desc lsri;
- struct fc_tlv_desc desc[];
+ union fc_tlv_desc desc[];
/* Supported Diagnostic Descriptor list */
};
-
#endif /* _FC_ELS_H_ */
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 01/13] fc_els: use 'union fc_tlv_desc'
2026-08-12 18:12 ` [PATCH 01/13] fc_els: use 'union fc_tlv_desc' Jesse Taube
@ 2026-08-12 18:26 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-08-12 18:26 UTC (permalink / raw)
To: Jesse Taube; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
New issues:
- [High] ABI and Wire Protocol Breakage: `union fc_tlv_desc` introduces 8-byte alignment, inserting 4-byte padding before `desc[]` in ELS response structures.
- [High] Build Failure: Flexible array members nested inside a union and declared as an array violate the C standard.
- [High] UAPI Source Breakage: Redefining `struct fc_tlv_desc` as `union fc_tlv_desc` breaks userspace compilation.
- [High] Undefined Behavior: Casting unaligned network offsets to a union requiring 8-byte alignment crashes strict-alignment architectures.
Pre-existing issues:
- [High] Undefined Behavior: Overlapping memory regions in `memcpy` during descriptor shifting.
- [High] Race Condition / Use-After-Free: `fc_find_rport_by_wwpn()` returns an rport pointer without acquiring a reference.
- [High] Out-of-Bounds Read: Signed integer underflow in `bytes_remain` logic bypasses length validation.
- [High] Out-of-Bounds Read: Missing minimum payload length validation prior to parsing specific descriptors.
--
--- Patch [1]: [PATCH 01/13] fc_els: use 'union fc_tlv_desc' ---
commit 96c48d7bf5ab3d34f4f3bb76d985ca1b29c2c877
Author: Hannes Reinecke <hare@kernel.org>
Subject: fc_els: use 'union fc_tlv_desc'
This commit introduces union fc_tlv_desc to provide a common structure for
all FC ELS TLV structures, helping to avoid type casts when managing these
descriptors.
> diff --git a/include/uapi/scsi/fc/fc_els.h b/include/uapi/scsi/fc/fc_els.h
> --- a/include/uapi/scsi/fc/fc_els.h
> +++ b/include/uapi/scsi/fc/fc_els.h
> @@ -259,12 +259,12 @@ enum fc_ls_tlv_dtag {
[ ... ]
> +union fc_tlv_desc {
> + struct fc_tlv_desc_hdr hdr;
> + struct fc_els_lsri_desc lsri;
> + struct fc_fn_li_desc li;
> + struct fc_fn_deli_desc deli;
> + struct fc_fn_peer_congn_desc peer_congn;
> + struct fc_fn_congn_desc congn;
> + struct fc_df_desc_fpin_reg fpin_reg;
> + struct fc_diag_lnkflt_desc lnkflt;
> + struct fc_diag_cg_sig_desc cg_sig;
> +};
[Severity: High]
Does changing struct fc_tlv_desc into a union break UAPI source
compatibility? Userspace applications that were previously compiling with
struct fc_tlv_desc will now fail to compile against these headers.
[ ... ]
> +static inline union fc_tlv_desc *fc_tlv_next_desc(union fc_tlv_desc *desc)
> +{
> + return (union fc_tlv_desc *)((__u8 *)desc + FC_TLV_DESC_SZ_FROM_LENGTH(desc));
> +}
[Severity: High]
Can this cast lead to unaligned access panics on strict-alignment
architectures? The pointer math calculates an offset that is only guaranteed
to be 4-byte aligned, but it is then cast to union fc_tlv_desc. Because this
union contains 64-bit fields, the compiler expects 8-byte alignment, which
might trigger undefined behavior when dereferenced later in paths like
lpfc_els_rcv_edc().
[ ... ]
> struct fc_els_fpin {
> __u8 fpin_cmd; /* command (0x16) */
> __u8 fpin_zero[3]; /* specified as zero - part of cmd */
> __be32 desc_len; /* Length of Descriptor List (in bytes).
> * Size of ELS excluding fpin_cmd,
> * fpin_zero and desc_len fields.
> */
> - struct fc_tlv_desc fpin_desc[]; /* Descriptor list */
> + union fc_tlv_desc fpin_desc[]; /* Descriptor list */
> };
[Severity: High]
Will this build successfully on standard C compilers? The C standard forbids
unions containing flexible array members from being elements of an array.
Nesting flexible array members inside the union fc_tlv_desc and then
declaring an array of that union might trigger a build constraint violation.
[ ... ]
> struct fc_els_rdf_resp {
> struct fc_els_ls_acc acc_hdr;
> __be32 desc_list_len; /* Length of response (in
> * bytes). Excludes acc_hdr
> * and desc_list_len fields.
> */
> struct fc_els_lsri_desc lsri;
> - struct fc_tlv_desc desc[]; /* Supported Descriptor list */
> + union fc_tlv_desc desc[]; /* Supported Descriptor list */
> };
[Severity: High]
Does this structure layout change break the wire protocol? By introducing
union fc_tlv_desc containing 64-bit members, the union adopts an 8-byte
alignment requirement. Because fc_els_lsri_desc ends at a 4-byte aligned
boundary, the compiler will silently insert 4 bytes of padding before the
desc array. This could corrupt the network payload offsets in ELS responses.
This same padding issue applies to the modified fc_els_edc_resp structure.
> diff --git a/drivers/scsi/lpfc/lpfc_els.c b/drivers/scsi/lpfc/lpfc_els.c
> --- a/drivers/scsi/lpfc/lpfc_els.c
> +++ b/drivers/scsi/lpfc/lpfc_els.c
[ ... ]
> @@ -9482,7 +9478,7 @@ lpfc_els_rcv_edc(struct lpfc_vport *vport, struct lpfc_iocbq *cmdiocb,
> goto out;
> }
>
> - dtag = be32_to_cpu(tlv->desc_tag);
> + dtag = be32_to_cpu(tlv->hdr.desc_tag);
[Severity: High]
This isn't a bug introduced by this patch, but does the bytes_remain loop
logic properly handle underflows? If a malicious or malformed payload causes
bytes_remain to become negative, it might bypass the length check in the while
loop because the negative value could be implicitly cast to size_t when
compared against FC_TLV_DESC_HDR_SZ, resulting in out-of-bounds kernel memory
reads.
[ ... ]
> @@ -10374,22 +10367,22 @@ lpfc_els_rcv_fpin(struct lpfc_vport *vport, void *p, u32 fpin_length)
> /* process each descriptor separately */
> while (bytes_remain >= FC_TLV_DESC_HDR_SZ &&
> bytes_remain >= FC_TLV_DESC_SZ_FROM_LENGTH(tlv)) {
> - dtag = be32_to_cpu(tlv->desc_tag);
> + dtag = be32_to_cpu(tlv->hdr.desc_tag);
> switch (dtag) {
> case ELS_DTAG_LNK_INTEGRITY:
> - lpfc_els_rcv_fpin_li(phba, tlv);
> + lpfc_els_rcv_fpin_li(phba, &tlv->li);
[Severity: High]
This is a pre-existing issue, but does this parsing code ensure the TLV
payload is large enough before casting and accessing the specific structure?
The loop validates the TLV header length, but immediately dispatches to
parsing routines like lpfc_els_rcv_fpin_li which map the payload to larger
structures. This missing minimum payload length validation might lead to
reading out of bounds during structural extraction.
[ ... ]
> /* Sanity check descriptor length. The desc_len value does not
> * include space for the desc_tag and the desc_len fields.
> */
> - len -= (cnt + sizeof(struct fc_tlv_desc));
> + len -= (cnt + sizeof(struct fc_tlv_desc_hdr));
> if (len < 0) {
[Severity: High]
This is a pre-existing issue, but could the memcpy further down in this
routine cause undefined behavior?
If desc_cnt is true, the code calls:
memcpy(first_tlv, current_tlv, (cnt + sizeof(struct fc_els_fpin)));
The source and destination memory regions for the descriptors overlap here
since they are being shifted in-place within the same buffer. This should
probably use memmove instead.
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -776,12 +776,11 @@ fc_fpin_pname_stats_update(struct Scsi_Host *shost,
> *
> */
> static void
> -fc_fpin_li_stats_update(struct Scsi_Host *shost, struct fc_tlv_desc *tlv)
> +fc_fpin_li_stats_update(struct Scsi_Host *shost, struct fc_fn_li_desc *li_desc)
> {
> struct fc_rport *rport = NULL;
> struct fc_rport *attach_rport = NULL;
> struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
> - struct fc_fn_li_desc *li_desc = (struct fc_fn_li_desc *)tlv;
> u16 event_type = be16_to_cpu(li_desc->event_type);
>
> rport = fc_find_rport_by_wwpn(shost,
> be64_to_cpu(li_desc->attached_wwpn));
> if (rport &&
[Severity: High]
This isn't a bug introduced by this patch, but does fc_find_rport_by_wwpn()
safely manage the returned pointer?
It appears to return an rport pointer without acquiring a reference. Since it
drops the host_lock before returning:
if (rport->port_name == wwpn) {
spin_unlock_irqrestore(shost->host_lock, flags);
return rport;
}
Concurrent destruction pathways could potentially free the structure while
it is being used here in fc_fpin_li_stats_update(), leading to a
use-after-free condition.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260812181300.3712426-1-jtaubepe@redhat.com?part=1
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 02/13] nvme: add NVME_CTRL_MARGINAL flag
2026-08-12 18:12 [PATCH 00/13] nvme-fc: FPIN link integrity handling Jesse Taube
2026-08-12 18:12 ` [PATCH 01/13] fc_els: use 'union fc_tlv_desc' Jesse Taube
@ 2026-08-12 18:12 ` Jesse Taube
2026-08-12 18:21 ` sashiko-bot
2026-08-12 18:12 ` [PATCH 03/13] nvme-multipath: numa support for marginal paths Jesse Taube
` (10 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Jesse Taube @ 2026-08-12 18:12 UTC (permalink / raw)
To: linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Justin Tee, Naresh Gottumukkala,
Paul Ely, Chaitanya Kulkarni, James E.J. Bottomley,
Martin K. Petersen, Nilesh Javali, GR-QLogic-Storage-Upstream,
Hannes Reinecke, Jesse Taube, Gustavo A. R. Silva, John Meneghini,
Bryan Gurney, Chris Leech, Ewan D . Milne, shinichiro.kawasaki,
linux-doc, linux-kernel, linux-nvme, linux-block
From: Bryan Gurney <bgurney@redhat.com>
Add a new controller flag, NVME_CTRL_MARGINAL, to help multipath I/O
policies to react to a path that is set to a "marginal" state.
The flag is cleared on controller reset, which is often the case when
faulty cabling or transceiver hardware is replaced.
Signed-off-by: Bryan Gurney <bgurney@redhat.com>
---
V10 -> V11:
- No change
---
drivers/nvme/host/core.c | 1 +
drivers/nvme/host/fc.c | 4 ++++
drivers/nvme/host/nvme.h | 6 ++++++
3 files changed, 11 insertions(+)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 453c1f0b2dd0..957090af2171 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -5165,6 +5165,7 @@ int nvme_init_ctrl(struct nvme_ctrl *ctrl, struct device *dev,
WRITE_ONCE(ctrl->state, NVME_CTRL_NEW);
ctrl->passthru_err_log_enabled = false;
clear_bit(NVME_CTRL_FAILFAST_EXPIRED, &ctrl->flags);
+ clear_bit(NVME_CTRL_MARGINAL, &ctrl->flags);
spin_lock_init(&ctrl->lock);
mutex_init(&ctrl->namespaces_lock);
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 04363b9c4489..d2ca65f70a0c 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -788,6 +788,10 @@ nvme_fc_ctrl_connectivity_loss(struct nvme_fc_ctrl *ctrl)
"Reconnect", ctrl->cnum);
set_bit(ASSOC_FAILED, &ctrl->flags);
+
+ /* clear 'marginal' flag as controller will be reset */
+ clear_bit(NVME_CTRL_MARGINAL, &ctrl->flags);
+
nvme_reset_ctrl(&ctrl->ctrl);
}
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 824651cc898d..3616742d6788 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -329,6 +329,7 @@ enum nvme_ctrl_flags {
NVME_CTRL_SKIP_ID_CNS_CS = 4,
NVME_CTRL_DIRTY_CAPABILITY = 5,
NVME_CTRL_FROZEN = 6,
+ NVME_CTRL_MARGINAL = 7,
};
struct nvme_ctrl {
@@ -479,6 +480,11 @@ static inline enum nvme_ctrl_state nvme_ctrl_state(struct nvme_ctrl *ctrl)
return READ_ONCE(ctrl->state);
}
+static inline bool nvme_ctrl_is_marginal(struct nvme_ctrl *ctrl)
+{
+ return test_bit(NVME_CTRL_MARGINAL, &ctrl->flags);
+}
+
enum nvme_iopolicy {
NVME_IOPOLICY_NUMA,
NVME_IOPOLICY_RR,
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 02/13] nvme: add NVME_CTRL_MARGINAL flag
2026-08-12 18:12 ` [PATCH 02/13] nvme: add NVME_CTRL_MARGINAL flag Jesse Taube
@ 2026-08-12 18:21 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-08-12 18:21 UTC (permalink / raw)
To: Jesse Taube; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The NVME_CTRL_MARGINAL flag is cleared on the wrong structure's flags field in the Fibre Channel transport.
- [Medium] The NVME_CTRL_MARGINAL flag is not cleared universally on all controller resets, leaving non-FC transports and manual resets permanently in a marginal state.
--
--- Patch [2]: [PATCH 02/13] nvme: add NVME_CTRL_MARGINAL flag ---
commit f9c336bbdc7993d3daa484b869cd6845d7fce7a7
Author: Bryan Gurney <bgurney@redhat.com>
nvme: add NVME_CTRL_MARGINAL flag
Add a new controller flag, NVME_CTRL_MARGINAL, to help multipath I/O
policies to react to a path that is set to a "marginal" state.
The flag is cleared on controller reset, which is often the case when
faulty cabling or transceiver hardware is replaced.
[ ... ]
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index 04363b9c4489e..d2ca65f70a0c5 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -788,6 +788,10 @@ nvme_fc_ctrl_connectivity_loss(struct nvme_fc_ctrl *ctrl)
> "Reconnect", ctrl->cnum);
>
> set_bit(ASSOC_FAILED, &ctrl->flags);
> +
> + /* clear 'marginal' flag as controller will be reset */
> + clear_bit(NVME_CTRL_MARGINAL, &ctrl->flags);
[Severity: High]
Does this clear the flag on the wrong structure?
Here, ctrl is a pointer to struct nvme_fc_ctrl. Clearing the bit on
&ctrl->flags will affect the FC-specific flags field instead of the intended
core NVMe controller flags (&ctrl->ctrl.flags).
[Severity: Medium]
Will other transports remain in a marginal state indefinitely?
The commit message mentions the flag is cleared on controller reset, but the
clearing logic is only added here in the FC-specific connectivity loss handler.
Should this flag instead be cleared universally inside nvme_reset_ctrl() in
drivers/nvme/host/core.c to ensure PCIe, TCP, and RDMA paths also recover
after a reset?
drivers/nvme/host/core.c:nvme_reset_ctrl() {
if (!nvme_change_ctrl_state(ctrl, NVME_CTRL_RESETTING))
return -EBUSY;
...
> +
> nvme_reset_ctrl(&ctrl->ctrl);
> }
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260812181300.3712426-1-jtaubepe@redhat.com?part=2
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 03/13] nvme-multipath: numa support for marginal paths
2026-08-12 18:12 [PATCH 00/13] nvme-fc: FPIN link integrity handling Jesse Taube
2026-08-12 18:12 ` [PATCH 01/13] fc_els: use 'union fc_tlv_desc' Jesse Taube
2026-08-12 18:12 ` [PATCH 02/13] nvme: add NVME_CTRL_MARGINAL flag Jesse Taube
@ 2026-08-12 18:12 ` Jesse Taube
2026-08-12 18:29 ` sashiko-bot
2026-08-12 18:12 ` [PATCH 04/13] nvme-multipath: queue-depth " Jesse Taube
` (9 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Jesse Taube @ 2026-08-12 18:12 UTC (permalink / raw)
To: linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Justin Tee, Naresh Gottumukkala,
Paul Ely, Chaitanya Kulkarni, James E.J. Bottomley,
Martin K. Petersen, Nilesh Javali, GR-QLogic-Storage-Upstream,
Hannes Reinecke, Jesse Taube, Gustavo A. R. Silva, John Meneghini,
Bryan Gurney, Chris Leech, Ewan D . Milne, shinichiro.kawasaki,
linux-doc, linux-kernel, linux-nvme, linux-block
FPIN LI (link integrity) messages are received when the attached
fabric detects hardware errors. In response to these messages I/O
should be directed away from the affected ports, and only used
if no other non-marginal paths are available.
To handle this a new controller flag 'NVME_CTRL_MARGINAL' is added
which will cause the multipath scheduler to skip these paths when
checking for 'optimized' paths.
Signed-off-by: Jesse Taube <jtaubepe@redhat.com>
---
This is a distinct change from the previous commit
which treated marginal paths as non-optimized but still usable.
This changes the priority of marginal paths to be lower than
non-optimized paths.
V10 -> V11:
- New commit
---
drivers/nvme/host/multipath.c | 53 +++++++++++++++++++++++++++++++++--
1 file changed, 50 insertions(+), 3 deletions(-)
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 9b9a657fa330..02936c580a37 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -305,10 +305,44 @@ static bool nvme_path_is_disabled(struct nvme_ns *ns)
return false;
}
+/*
+ * Returns true if the new distance is better than the old one.
+ */
+static bool is_best_distance(bool found_is_marginal, bool marginal,
+ int old_distance, int distance)
+{
+ if (found_is_marginal) {
+ if (marginal) {
+ /*
+ * A marginal path has already been found,
+ * or this is the first path found.
+ * This one is also marginal, but closer
+ * to the NUMA node, so prefer it.
+ */
+ if (distance < old_distance)
+ return true;
+ } else {
+ /* Found a non-marginal path, use it over a marginal one. */
+ return true;
+ }
+ } else {
+ /* A non-marginal path has already found. This one is marginal, so skip it. */
+ if (marginal)
+ return false;
+
+ /* Found a closer non-marginal path, use it. */
+ if (distance < old_distance)
+ return true;
+ }
+
+ return false;
+}
+
static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
{
int found_distance = INT_MAX, fallback_distance = INT_MAX, distance;
struct nvme_ns *found = NULL, *fallback = NULL, *ns;
+ bool found_is_marginal = true, fallback_is_marginal = true;
list_for_each_entry_srcu(ns, &head->list, siblings,
srcu_read_lock_held(&head->srcu)) {
@@ -323,15 +357,19 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
switch (ns->ana_state) {
case NVME_ANA_OPTIMIZED:
- if (distance < found_distance) {
+ if (is_best_distance(found_is_marginal, nvme_ctrl_is_marginal(ns->ctrl),
+ found_distance, distance)) {
found_distance = distance;
found = ns;
+ found_is_marginal = nvme_ctrl_is_marginal(ns->ctrl);
}
break;
case NVME_ANA_NONOPTIMIZED:
- if (distance < fallback_distance) {
+ if (is_best_distance(fallback_is_marginal, nvme_ctrl_is_marginal(ns->ctrl),
+ fallback_distance, distance)) {
fallback_distance = distance;
fallback = ns;
+ fallback_is_marginal = nvme_ctrl_is_marginal(ns->ctrl);
}
break;
default:
@@ -339,6 +377,14 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
}
}
+ /*
+ * Use non-optimized path only if it is not marginal
+ * and no optimized path is marginal.
+ */
+ if (found_is_marginal && !fallback_is_marginal)
+ found = fallback;
+
+ /* No optimized path found, use the fallback */
if (!found)
found = fallback;
if (found)
@@ -444,7 +490,8 @@ static struct nvme_ns *nvme_queue_depth_path(struct nvme_ns_head *head)
static inline bool nvme_path_is_optimized(struct nvme_ns *ns)
{
return nvme_ctrl_state(ns->ctrl) == NVME_CTRL_LIVE &&
- ns->ana_state == NVME_ANA_OPTIMIZED;
+ ns->ana_state == NVME_ANA_OPTIMIZED &&
+ !nvme_ctrl_is_marginal(ns->ctrl);
}
static struct nvme_ns *nvme_numa_path(struct nvme_ns_head *head)
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 03/13] nvme-multipath: numa support for marginal paths
2026-08-12 18:12 ` [PATCH 03/13] nvme-multipath: numa support for marginal paths Jesse Taube
@ 2026-08-12 18:29 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-08-12 18:29 UTC (permalink / raw)
To: Jesse Taube; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Medium] Double evaluation of volatile controller state creates a TOCTOU race that can select a sub-optimal marginal path over a better non-marginal path.
- [Medium] The round-robin multipath I/O policy (`nvme_round_robin_path`) was not updated to handle marginal paths, bypassing the new `NVME_CTRL_MARGINAL` priority logic.
- [Low] The comment explaining the fallback path selection has inverted logic.
--
--- Patch [3]: [PATCH 03/13] nvme-multipath: numa support for marginal paths ---
commit 820a8648fdbfc8327ddaf233abd513210c265d17
Author: Jesse Taube <jtaubepe@redhat.com>
nvme-multipath: numa support for marginal paths
This patch introduces the NVME_CTRL_MARGINAL flag to handle FPIN LI
(link integrity) messages indicating hardware errors. The multipath
scheduler is updated to direct I/O away from affected marginal ports by
skipping them when non-marginal optimized paths are available.
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index 9b9a657fa330f..02936c580a373 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
[ ... ]
> @@ -323,15 +357,19 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
>
> switch (ns->ana_state) {
> case NVME_ANA_OPTIMIZED:
> - if (distance < found_distance) {
> + if (is_best_distance(found_is_marginal, nvme_ctrl_is_marginal(ns->ctrl),
> + found_distance, distance)) {
> found_distance = distance;
> found = ns;
> + found_is_marginal = nvme_ctrl_is_marginal(ns->ctrl);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Could this double evaluation of nvme_ctrl_is_marginal() cause a race
condition?
nvme_ctrl_is_marginal() uses test_bit(), which performs a volatile memory
read. If the controller's marginal state transitions from false to true between
these two reads, a previously found non-marginal path will be incorrectly
replaced by this new path, which is then subsequently recorded as marginal.
Would it be better to read the marginal state into a local variable once per
iteration?
> }
> break;
> case NVME_ANA_NONOPTIMIZED:
> - if (distance < fallback_distance) {
> + if (is_best_distance(fallback_is_marginal, nvme_ctrl_is_marginal(ns->ctrl),
> + fallback_distance, distance)) {
> fallback_distance = distance;
> fallback = ns;
> + fallback_is_marginal = nvme_ctrl_is_marginal(ns->ctrl);
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Medium]
Does this have the same race condition as the NVME_ANA_OPTIMIZED case above?
Using a local variable for the marginal state might prevent inconsistencies
if the bit changes between the two checks.
> }
> break;
> default:
[ ... ]
> @@ -339,6 +377,14 @@ static struct nvme_ns *__nvme_find_path(struct nvme_ns_head *head, int node)
> }
> }
>
> + /*
> + * Use non-optimized path only if it is not marginal
> + * and no optimized path is marginal.
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[Severity: Low]
Is the logic in this comment inverted?
The code correctly falls back to a non-marginal path only when the optimized
path is marginal (found_is_marginal && !fallback_is_marginal). However, the
comment incorrectly states it falls back if "no optimized path is marginal",
which contradicts the implemented boolean logic.
> + */
> + if (found_is_marginal && !fallback_is_marginal)
> + found = fallback;
> +
> + /* No optimized path found, use the fallback */
> if (!found)
> found = fallback;
> if (found)
[ ... ]
> @@ -444,7 +490,8 @@ static struct nvme_ns *nvme_queue_depth_path(struct nvme_ns_head *head)
> static inline bool nvme_path_is_optimized(struct nvme_ns *ns)
> {
> return nvme_ctrl_state(ns->ctrl) == NVME_CTRL_LIVE &&
> - ns->ana_state == NVME_ANA_OPTIMIZED;
> + ns->ana_state == NVME_ANA_OPTIMIZED &&
> + !nvme_ctrl_is_marginal(ns->ctrl);
> }
[Severity: Medium]
Since marginal paths are now supported and skipped in the NUMA and queue depth
policies, does nvme_round_robin_path() also need to be updated?
If a user configures iopolicy=round-robin, the multipath scheduler seems to
actively route I/O to marginal paths without checking the state:
drivers/nvme/host/multipath.c:nvme_round_robin_path() {
...
if (ns->ana_state == NVME_ANA_OPTIMIZED) {
found = ns;
goto out;
}
...
}
Could this risk I/O timeouts and dropped packets on unreliable hardware links
by bypassing the new NVME_CTRL_MARGINAL priority logic?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260812181300.3712426-1-jtaubepe@redhat.com?part=3
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 04/13] nvme-multipath: queue-depth support for marginal paths
2026-08-12 18:12 [PATCH 00/13] nvme-fc: FPIN link integrity handling Jesse Taube
` (2 preceding siblings ...)
2026-08-12 18:12 ` [PATCH 03/13] nvme-multipath: numa support for marginal paths Jesse Taube
@ 2026-08-12 18:12 ` Jesse Taube
2026-08-12 18:12 ` [PATCH 05/13] nvme-multipath: round-robin " Jesse Taube
` (8 subsequent siblings)
12 siblings, 0 replies; 30+ messages in thread
From: Jesse Taube @ 2026-08-12 18:12 UTC (permalink / raw)
To: linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Justin Tee, Naresh Gottumukkala,
Paul Ely, Chaitanya Kulkarni, James E.J. Bottomley,
Martin K. Petersen, Nilesh Javali, GR-QLogic-Storage-Upstream,
Hannes Reinecke, Jesse Taube, Gustavo A. R. Silva, John Meneghini,
Bryan Gurney, Chris Leech, Ewan D . Milne, shinichiro.kawasaki,
linux-doc, linux-kernel, linux-nvme, linux-block
From: John Meneghini <jmeneghi@redhat.com>
Exclude marginal paths from queue-depth io policy. In the case where all
paths are marginal and no optimized or non-optimized path is found, we
fall back and select the best marginal path.
Signed-off-by: Jesse Taube <jtaubepe@redhat.com>
Signed-off-by: John Meneghini <jmeneghi@redhat.com>
---
V10 -> V11:
- New commit
---
drivers/nvme/host/multipath.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index 02936c580a37..dc34634842be 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -453,7 +453,9 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head)
static struct nvme_ns *nvme_queue_depth_path(struct nvme_ns_head *head)
{
struct nvme_ns *best_opt = NULL, *best_nonopt = NULL, *ns;
- unsigned int min_depth_opt = UINT_MAX, min_depth_nonopt = UINT_MAX;
+ int min_depth_opt = INT_MAX, min_depth_nonopt = INT_MAX;
+ bool opt_is_marginal = true, nonopt_is_marginal = true, marginal;
+
unsigned int depth;
list_for_each_entry_srcu(ns, &head->list, siblings,
@@ -462,28 +464,40 @@ static struct nvme_ns *nvme_queue_depth_path(struct nvme_ns_head *head)
continue;
depth = atomic_read(&ns->ctrl->nr_active);
+ marginal = nvme_ctrl_is_marginal(ns->ctrl);
switch (ns->ana_state) {
case NVME_ANA_OPTIMIZED:
- if (depth < min_depth_opt) {
+ if (is_best_distance(opt_is_marginal, marginal,
+ min_depth_opt, depth)) {
min_depth_opt = depth;
best_opt = ns;
+ opt_is_marginal = marginal;
}
break;
case NVME_ANA_NONOPTIMIZED:
- if (depth < min_depth_nonopt) {
+ if (is_best_distance(nonopt_is_marginal, marginal,
+ min_depth_nonopt, depth)) {
min_depth_nonopt = depth;
best_nonopt = ns;
+ nonopt_is_marginal = marginal;
}
break;
default:
break;
}
- if (min_depth_opt == 0)
+ if (min_depth_opt == 0 && !opt_is_marginal)
return best_opt;
}
+ /*
+ * Prefer non-marginal non-optimized path
+ * over a marginal optimized path.
+ */
+ if (opt_is_marginal && !nonopt_is_marginal && best_nonopt)
+ return best_nonopt;
+
return best_opt ? best_opt : best_nonopt;
}
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* [PATCH 05/13] nvme-multipath: round-robin support for marginal paths
2026-08-12 18:12 [PATCH 00/13] nvme-fc: FPIN link integrity handling Jesse Taube
` (3 preceding siblings ...)
2026-08-12 18:12 ` [PATCH 04/13] nvme-multipath: queue-depth " Jesse Taube
@ 2026-08-12 18:12 ` Jesse Taube
2026-08-12 18:26 ` sashiko-bot
2026-08-12 18:12 ` [PATCH 06/13] nvme: sysfs: emit the marginal path state in show_state() Jesse Taube
` (7 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Jesse Taube @ 2026-08-12 18:12 UTC (permalink / raw)
To: linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Justin Tee, Naresh Gottumukkala,
Paul Ely, Chaitanya Kulkarni, James E.J. Bottomley,
Martin K. Petersen, Nilesh Javali, GR-QLogic-Storage-Upstream,
Hannes Reinecke, Jesse Taube, Gustavo A. R. Silva, John Meneghini,
Bryan Gurney, Chris Leech, Ewan D . Milne, shinichiro.kawasaki,
linux-doc, linux-kernel, linux-nvme, linux-block
Exclude marginal paths from round-robin io policy. In the case where all
paths are marginal and no optimized or non-optimized path is found, we
fall back and perform round-robin on the marginal paths.
Signed-off-by: Jesse Taube <jtaubepe@redhat.com>
---
V10 -> V11:
- New commit
---
drivers/nvme/host/multipath.c | 47 +++++++++++++++++++++++++++++++----
1 file changed, 42 insertions(+), 5 deletions(-)
diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
index dc34634842be..11f474704fdd 100644
--- a/drivers/nvme/host/multipath.c
+++ b/drivers/nvme/host/multipath.c
@@ -405,6 +405,7 @@ static struct nvme_ns *nvme_next_ns(struct nvme_ns_head *head,
static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head)
{
struct nvme_ns *ns, *found = NULL;
+ bool found_is_marginal = true;
int node = numa_node_id();
struct nvme_ns *old = srcu_dereference(head->current_path[node],
&head->srcu);
@@ -425,22 +426,58 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head)
continue;
if (ns->ana_state == NVME_ANA_OPTIMIZED) {
+ if (found_is_marginal && nvme_ctrl_is_marginal(ns->ctrl)) {
+ /*
+ * A marginal path has already found,
+ * or this is the first path found.
+ * This one is also marginal, but optimized,
+ * so prefer it.
+ */
+ found = ns;
+ found_is_marginal = 1;
+ continue;
+ }
+
+
+ /*
+ * A non-marginal path has already found.
+ * This one is marginal, so skip it.
+ */
+ if (nvme_ctrl_is_marginal(ns->ctrl))
+ continue;
+
+ /* Found a non-marginal, optimized path use it. */
found = ns;
goto out;
}
- if (ns->ana_state == NVME_ANA_NONOPTIMIZED)
+ if (ns->ana_state == NVME_ANA_NONOPTIMIZED) {
+ /*
+ * A path has already found. This one is marginal,
+ * so skip it.
+ */
+ if (found && nvme_ctrl_is_marginal(ns->ctrl))
+ continue;
found = ns;
+ found_is_marginal = nvme_ctrl_is_marginal(ns->ctrl);
+ }
}
/*
* The loop above skips the current path for round-robin semantics.
* Fall back to the current path if either:
- * - no other optimized path found and current is optimized,
+ * - no other non-marginal optimized path found and current is,
+ * optimized and not marginal.
* - no other usable path found and current is usable.
*/
- if (!nvme_path_is_disabled(old) &&
- (old->ana_state == NVME_ANA_OPTIMIZED ||
- (!found && old->ana_state == NVME_ANA_NONOPTIMIZED)))
+ /* no other usable path found and current is usable. */
+ if (!nvme_path_is_disabled(old) && !found)
+ return old;
+ /*
+ * no other non-marginal optimized path found and current is,
+ * optimized and not marginal.
+ */
+ if (!nvme_path_is_disabled(old) && !nvme_ctrl_is_marginal(old->ctrl) &&
+ (old->ana_state == NVME_ANA_OPTIMIZED || found_is_marginal))
return old;
if (!found)
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 05/13] nvme-multipath: round-robin support for marginal paths
2026-08-12 18:12 ` [PATCH 05/13] nvme-multipath: round-robin " Jesse Taube
@ 2026-08-12 18:26 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-08-12 18:26 UTC (permalink / raw)
To: Jesse Taube; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues:
- [High] Improper round-robin fallback logic causes path thrashing between marginal OPTIMIZED and marginal NONOPTIMIZED paths.
- [High] Path selection logic may incorrectly return paths in `INACCESSIBLE`, `CHANGE`, or `PERSISTENT_LOSS` ANA states.
Pre-existing issues:
- [High] The round-robin path selection can enter an infinite loop inside an SRCU read-side critical section if the current path is removed concurrently.
--
--- Patch [5]: [PATCH 05/13] nvme-multipath: round-robin support for marginal paths ---
commit f4b9e08d0e0306a7a08b9e274efbbe8c287e6dde
Author: Jesse Taube <jtaubepe@redhat.com>
nvme-multipath: round-robin support for marginal paths
This commit modifies the NVMe round-robin path selection to exclude
marginal paths by default. It introduces logic to fall back to round-robin
on marginal paths only if no optimized or non-optimized non-marginal paths
are found.
> diff --git a/drivers/nvme/host/multipath.c b/drivers/nvme/host/multipath.c
> index dc34634842be8..11f474704fdd7 100644
> --- a/drivers/nvme/host/multipath.c
> +++ b/drivers/nvme/host/multipath.c
[ ... ]
> @@ -425,22 +426,58 @@ static struct nvme_ns *nvme_round_robin_path(struct nvme_ns_head *head)
> for (ns = nvme_next_ns(head, old);
> ns && ns != old;
> ns = nvme_next_ns(head, ns)) {
[Severity: High]
This is a pre-existing issue, but does this loop condition risk an infinite
loop if the current path is concurrently removed?
If old (the current path) is concurrently unlinked via list_del_rcu(),
nvme_next_ns() will continue into the active list, wrap around to the head,
and traverse indefinitely. Because old is no longer in the list, ns != old
remains true forever.
Since this patch makes it easier to fall through without hitting a goto out
(for example, when all paths are marginal), could this cause a hard lockup
in the SRCU read-side critical section?
[ ... ]
> /*
> * The loop above skips the current path for round-robin semantics.
> * Fall back to the current path if either:
> - * - no other optimized path found and current is optimized,
> + * - no other non-marginal optimized path found and current is,
> + * optimized and not marginal.
> * - no other usable path found and current is usable.
> */
> - if (!nvme_path_is_disabled(old) &&
> - (old->ana_state == NVME_ANA_OPTIMIZED ||
> - (!found && old->ana_state == NVME_ANA_NONOPTIMIZED)))
> + /* no other usable path found and current is usable. */
> + if (!nvme_path_is_disabled(old) && !found)
> + return old;
[Severity: High]
Could this fallback condition incorrectly return paths that are in
inaccessible, change, or persistent loss ANA states?
When the current path transitions to an inaccessible state and no other
usable paths exist, the loop finds no paths (found is NULL). Because
nvme_path_is_disabled() does not check the ANA state, this fallback will
return the old path.
Will this lead to continuous submission of I/O to inaccessible paths and
unnecessary ANA log reads?
> + /*
> + * no other non-marginal optimized path found and current is,
> + * optimized and not marginal.
> + */
> + if (!nvme_path_is_disabled(old) && !nvme_ctrl_is_marginal(old->ctrl) &&
> + (old->ana_state == NVME_ANA_OPTIMIZED || found_is_marginal))
> return old;
[Severity: High]
Does this logic cause path thrashing between marginal optimized and
marginal non-optimized paths?
If the current path is marginal and optimized, and the loop finds a
marginal non-optimized path, this fallback unconditionally skips retaining
the current marginal path because of the !nvme_ctrl_is_marginal(old->ctrl)
check.
It then selects the non-optimized path instead. On the next I/O, the
reverse happens, causing a bounce between the two paths. Should marginal
optimized paths be retained over marginal non-optimized ones?
>
> if (!found)
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260812181300.3712426-1-jtaubepe@redhat.com?part=5
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 06/13] nvme: sysfs: emit the marginal path state in show_state()
2026-08-12 18:12 [PATCH 00/13] nvme-fc: FPIN link integrity handling Jesse Taube
` (4 preceding siblings ...)
2026-08-12 18:12 ` [PATCH 05/13] nvme-multipath: round-robin " Jesse Taube
@ 2026-08-12 18:12 ` Jesse Taube
2026-08-12 18:21 ` sashiko-bot
2026-08-12 18:12 ` [PATCH 07/13] scsi: scsi_transport_fc: Add set_rport_marginal to fc_function_template Jesse Taube
` (6 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Jesse Taube @ 2026-08-12 18:12 UTC (permalink / raw)
To: linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Justin Tee, Naresh Gottumukkala,
Paul Ely, Chaitanya Kulkarni, James E.J. Bottomley,
Martin K. Petersen, Nilesh Javali, GR-QLogic-Storage-Upstream,
Hannes Reinecke, Jesse Taube, Gustavo A. R. Silva, John Meneghini,
Bryan Gurney, Chris Leech, Ewan D . Milne, shinichiro.kawasaki,
linux-doc, linux-kernel, linux-nvme, linux-block, Muneendra Kumar
From: Bryan Gurney <bgurney@redhat.com>
If a controller has received a link integrity or congestion event, and
has the NVME_CTRL_MARGINAL flag set, emit "marginal" in the state
instead of "live", to identify the marginal paths.
Co-developed-by: John Meneghini <jmeneghi@redhat.com>
Signed-off-by: John Meneghini <jmeneghi@redhat.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Chaitanya Kulkarni <kch@nvidia.com>
Tested-by: Muneendra Kumar <muneendra.kumar@broadcom.com>
Signed-off-by: Bryan Gurney <bgurney@redhat.com>
---
V10 -> V11:
- No change
---
drivers/nvme/host/sysfs.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
index 75b2d69b5957..01c771dc88cb 100644
--- a/drivers/nvme/host/sysfs.c
+++ b/drivers/nvme/host/sysfs.c
@@ -527,7 +527,9 @@ static ssize_t nvme_sysfs_show_state(struct device *dev,
};
if (state < ARRAY_SIZE(state_name) && state_name[state])
- return sysfs_emit(buf, "%s\n", state_name[state]);
+ return sysfs_emit(buf, "%s\n",
+ (nvme_ctrl_is_marginal(ctrl)) ? "marginal" :
+ state_name[state]);
return sysfs_emit(buf, "unknown state\n");
}
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 06/13] nvme: sysfs: emit the marginal path state in show_state()
2026-08-12 18:12 ` [PATCH 06/13] nvme: sysfs: emit the marginal path state in show_state() Jesse Taube
@ 2026-08-12 18:21 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-08-12 18:21 UTC (permalink / raw)
To: Jesse Taube; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unconditional override of sysfs state attribute with 'marginal', masking critical controller states.
--
--- Patch [6]: [PATCH 06/13] nvme: sysfs: emit the marginal path state in show_state() ---
commit 881ed7168d40f086dcba6f49059055d962ce09aa
Author: Bryan Gurney <bgurney@redhat.com>
nvme: sysfs: emit the marginal path state in show_state()
If a controller has received a link integrity or congestion event, and
has the NVME_CTRL_MARGINAL flag set, emit "marginal" in the state
instead of "live", to identify the marginal paths.
> diff --git a/drivers/nvme/host/sysfs.c b/drivers/nvme/host/sysfs.c
> --- a/drivers/nvme/host/sysfs.c
> +++ b/drivers/nvme/host/sysfs.c
> @@ -527,7 +527,9 @@ static ssize_t nvme_sysfs_show_state(struct device *dev,
> };
>
> if (state < ARRAY_SIZE(state_name) && state_name[state])
> - return sysfs_emit(buf, "%s\n", state_name[state]);
> + return sysfs_emit(buf, "%s\n",
> + (nvme_ctrl_is_marginal(ctrl)) ? "marginal" :
> + state_name[state]);
[Severity: High]
The commit message mentions emitting "marginal" in the state instead of
"live", but does this implementation unconditionally override all valid
states if the NVME_CTRL_MARGINAL flag is set?
If a marginal controller encounters a fatal error or is deleted, changing its
internal state to NVME_CTRL_DEAD, NVME_CTRL_RESETTING, or NVME_CTRL_DELETING,
won't this still emit "marginal" as long as the flag is set?
Could this mask critical controller states and cause userspace tools that
poll this sysfs attribute for teardown or error handling to hang while waiting
for the state to change?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260812181300.3712426-1-jtaubepe@redhat.com?part=6
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 07/13] scsi: scsi_transport_fc: Add set_rport_marginal to fc_function_template
2026-08-12 18:12 [PATCH 00/13] nvme-fc: FPIN link integrity handling Jesse Taube
` (5 preceding siblings ...)
2026-08-12 18:12 ` [PATCH 06/13] nvme: sysfs: emit the marginal path state in show_state() Jesse Taube
@ 2026-08-12 18:12 ` Jesse Taube
2026-08-12 18:28 ` sashiko-bot
2026-08-12 18:12 ` [PATCH 08/13] scsi: scsi_transport_fc: user support for clearing NVME_CTRL_MARGINAL Jesse Taube
` (5 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Jesse Taube @ 2026-08-12 18:12 UTC (permalink / raw)
To: linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Justin Tee, Naresh Gottumukkala,
Paul Ely, Chaitanya Kulkarni, James E.J. Bottomley,
Martin K. Petersen, Nilesh Javali, GR-QLogic-Storage-Upstream,
Hannes Reinecke, Jesse Taube, Gustavo A. R. Silva, John Meneghini,
Bryan Gurney, Chris Leech, Ewan D . Milne, shinichiro.kawasaki,
linux-doc, linux-kernel, linux-nvme, linux-block
Add fc_fpin_set_marginal function to evaluate the FPIN LI TLV
information and set the 'marginal' path status for all
affected nvme rports. Then call set_rport_marginal() callback added
to fc_function_template.
The implementation of set_rport_marginal() is almost identical to
set_rport_dev_loss_tmo implementation.
Signed-off-by: Jesse Taube <jtaubepe@redhat.com>
---
V10 -> V11:
- New commit
- Replaces scsi: scsi_transport_fc:
add fc_host_fpin_set_nvme_rport_marginal()
---
drivers/scsi/scsi_transport_fc.c | 20 ++++++++++++++++++++
include/scsi/scsi_transport_fc.h | 1 +
2 files changed, 21 insertions(+)
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index 02c58f4591d8..df1edf3c23fa 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -737,6 +737,25 @@ fc_cn_stats_update(u16 event_type, struct fc_fpin_stats *stats)
}
}
+static void fc_fpin_set_marginal(struct Scsi_Host *shost, struct fc_rport *rport)
+{
+ struct fc_internal *i = to_fc_internal(shost->transportt);
+ unsigned long flags;
+
+ spin_lock_irqsave(shost->host_lock, flags);
+
+ if (rport->port_state == FC_PORTSTATE_ONLINE &&
+ rport->roles & FC_PORT_ROLE_NVME_TARGET) {
+ rport->port_state = FC_PORTSTATE_MARGINAL;
+ spin_unlock_irqrestore(shost->host_lock, flags);
+ if (i->f->set_rport_marginal)
+ i->f->set_rport_marginal(rport, true);
+ return;
+ }
+
+ spin_unlock_irqrestore(shost->host_lock, flags);
+}
+
static void
fc_fpin_pname_stats_update(struct Scsi_Host *shost,
struct fc_rport *attach_rport, u16 event_type,
@@ -764,6 +783,7 @@ fc_fpin_pname_stats_update(struct Scsi_Host *shost,
if (rport == attach_rport)
continue;
stats_update(event_type, &rport->fpin_stats);
+ fc_fpin_set_marginal(shost, rport);
}
}
}
diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
index 9f30625aa0d3..bc7c3f832399 100644
--- a/include/scsi/scsi_transport_fc.h
+++ b/include/scsi/scsi_transport_fc.h
@@ -684,6 +684,7 @@ struct fc_host_attrs {
struct fc_function_template {
void (*get_rport_dev_loss_tmo)(struct fc_rport *);
void (*set_rport_dev_loss_tmo)(struct fc_rport *, u32);
+ void (*set_rport_marginal)(struct fc_rport *rport, bool marginal);
void (*get_starget_node_name)(struct scsi_target *);
void (*get_starget_port_name)(struct scsi_target *);
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 07/13] scsi: scsi_transport_fc: Add set_rport_marginal to fc_function_template
2026-08-12 18:12 ` [PATCH 07/13] scsi: scsi_transport_fc: Add set_rport_marginal to fc_function_template Jesse Taube
@ 2026-08-12 18:28 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-08-12 18:28 UTC (permalink / raw)
To: Jesse Taube; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
New issues:
- [High] The primary affected port (`attach_rport`) in a Link Integrity event is excluded from being marked as marginal.
- [High] NVMe targets are erroneously placed into the 'marginal' state during Peer Congestion (PC) events.
- [High] The `set_rport_marginal` callback is not invoked when the marginal state is manually cleared or set via sysfs.
Pre-existing issues:
- [Critical] The `fc_rport` pointer returned by `fc_find_rport_by_wwpn` is used without a reference count, leading to a Use-After-Free.
- [Critical] Integer underflow in `fc_host_fpin_rcv` when `fpin_len` is small, leading to massive OOB read.
--
--- Patch [7]: [PATCH 07/13] scsi: scsi_transport_fc: Add set_rport_marginal to fc_function_template ---
commit 978d2e847b2a738287194acf215a8cbbe297f65a
Author: Jesse Taube <jtaubepe@redhat.com>
scsi: scsi_transport_fc: Add set_rport_marginal to fc_function_template
Add fc_fpin_set_marginal function to evaluate the FPIN LI TLV
information and set the 'marginal' path status for all
affected nvme rports. Then call set_rport_marginal() callback added
to fc_function_template.
The implementation of set_rport_marginal() is almost identical to
set_rport_dev_loss_tmo implementation.
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> index 02c58f4591d83..df1edf3c23fa8 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -737,6 +737,25 @@ fc_cn_stats_update(u16 event_type, struct fc_fpin_stats *stats)
> }
> }
>
> +static void fc_fpin_set_marginal(struct Scsi_Host *shost, struct fc_rport *rport)
> +{
> + struct fc_internal *i = to_fc_internal(shost->transportt);
> + unsigned long flags;
> +
> + spin_lock_irqsave(shost->host_lock, flags);
> +
> + if (rport->port_state == FC_PORTSTATE_ONLINE &&
> + rport->roles & FC_PORT_ROLE_NVME_TARGET) {
> + rport->port_state = FC_PORTSTATE_MARGINAL;
> + spin_unlock_irqrestore(shost->host_lock, flags);
> + if (i->f->set_rport_marginal)
> + i->f->set_rport_marginal(rport, true);
[Severity: Critical]
This is a pre-existing issue, but does passing this rport pointer out to the
driver callback outside of the lock risk a use-after-free?
The rport pointer passed into fc_fpin_set_marginal() is retrieved by
fc_find_rport_by_wwpn(). Looking at fc_find_rport_by_wwpn(), it returns the
pointer without incrementing its reference count (e.g., via get_device()).
If a concurrent fc_remote_port_delete() runs, the rport memory could be
freed as soon as the internal list lock is dropped. When the host_lock is
dropped here, the unreferenced rport pointer is then passed to
set_rport_marginal(), potentially dereferencing freed memory.
> + return;
> + }
> +
> + spin_unlock_irqrestore(shost->host_lock, flags);
> +}
> +
> static void
> fc_fpin_pname_stats_update(struct Scsi_Host *shost,
> struct fc_rport *attach_rport, u16 event_type,
> @@ -764,6 +783,7 @@ fc_fpin_pname_stats_update(struct Scsi_Host *shost,
> if (rport == attach_rport)
> continue;
> stats_update(event_type, &rport->fpin_stats);
> + fc_fpin_set_marginal(shost, rport);
[Severity: High]
Does this continue statement unintentionally bypass the newly added
fc_fpin_set_marginal() call for the primary attach_rport?
When a Link Integrity FPIN is processed via fc_fpin_li_stats_update(), it
locates the attach_rport and passes it to fc_fpin_pname_stats_update().
Because of this continue, the primary port experiencing the Link Integrity
failure won't be transitioned to the marginal state.
[Severity: High]
Will this also place NVMe targets into the marginal state during Peer
Congestion (PC) events?
fc_fpin_peer_congn_stats_update() processes Peer Congestion events by
calling fc_fpin_pname_stats_update(). Since fc_fpin_set_marginal() is now
called unconditionally here, it seems ports involved in transient congestion
events might be incorrectly marked as marginal, potentially causing
unwarranted path failovers.
> }
> }
> }
> diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
> index 9f30625aa0d30..bc7c3f8323992 100644
> --- a/include/scsi/scsi_transport_fc.h
> +++ b/include/scsi/scsi_transport_fc.h
> @@ -684,6 +684,7 @@ struct fc_host_attrs {
> struct fc_function_template {
> void (*get_rport_dev_loss_tmo)(struct fc_rport *);
> void (*set_rport_dev_loss_tmo)(struct fc_rport *, u32);
> + void (*set_rport_marginal)(struct fc_rport *rport, bool marginal);
[Severity: High]
Should this callback also be invoked when the marginal state is manually
cleared or set via sysfs?
When a user writes 'Online' to the port_state sysfs attribute, it calls
fc_rport_set_marginal_state() which modifies rport->port_state, but it does
not call this new set_rport_marginal() callback.
Will this leave the Low-Level Device Driver (LLDD) permanently out of sync
with the transport class state when a path is recovered manually?
>
> void (*get_starget_node_name)(struct scsi_target *);
> void (*get_starget_port_name)(struct scsi_target *);
[Severity: Critical]
This is a pre-existing issue, but is there an integer underflow in
fc_host_fpin_rcv() when fpin_len is small?
In fc_host_fpin_rcv(), bytes_remain is calculated without validating that
fpin_len is large enough:
bytes_remain = fpin_len - offsetof(struct fc_els_fpin, fpin_desc);
bytes_remain = min_t(u32, bytes_remain, be32_to_cpu(fpin->desc_len));
Since offsetof(struct fc_els_fpin, fpin_desc) is 8, if an FPIN payload is
received with fpin_len < 8, the subtraction underflows to a massive u32
value. The parsing loop will then read well past the end of fpin_buf,
causing an out-of-bounds read.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260812181300.3712426-1-jtaubepe@redhat.com?part=7
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 08/13] scsi: scsi_transport_fc: user support for clearing NVME_CTRL_MARGINAL
2026-08-12 18:12 [PATCH 00/13] nvme-fc: FPIN link integrity handling Jesse Taube
` (6 preceding siblings ...)
2026-08-12 18:12 ` [PATCH 07/13] scsi: scsi_transport_fc: Add set_rport_marginal to fc_function_template Jesse Taube
@ 2026-08-12 18:12 ` Jesse Taube
2026-08-12 18:24 ` sashiko-bot
2026-08-12 18:12 ` [PATCH 09/13] nvme-fc: add nvme_fc_set_remoteport_fpin() Jesse Taube
` (4 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Jesse Taube @ 2026-08-12 18:12 UTC (permalink / raw)
To: linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Justin Tee, Naresh Gottumukkala,
Paul Ely, Chaitanya Kulkarni, James E.J. Bottomley,
Martin K. Petersen, Nilesh Javali, GR-QLogic-Storage-Upstream,
Hannes Reinecke, Jesse Taube, Gustavo A. R. Silva, John Meneghini,
Bryan Gurney, Chris Leech, Ewan D . Milne, shinichiro.kawasaki,
linux-doc, linux-kernel, linux-nvme, linux-block
From: John Meneghini <jmeneghi@redhat.com>
Refactor and fc_rport_set_marginal_state smp safe by holding
`shost->host_lock` around all `rport->port_state` accesses.
Call nvme_fc_modify_rport_fpin_state() when FC_PORTSTATE_MARGINAL is set
or cleared. This allows the user to quickly set or clear the
NVME_CTRL_MARGINAL state from sysfs.
E.g.:
echo "Marginal" > /sys/class/fc_remote_ports/rport-13:0-5/port_state
echo "Online" > /sys/class/fc_remote_ports/rport-13:0-5/port_state
Note: nvme_fc_modify_rport_fpin_state() will only affect rports that
have FC_PORT_ROLE_NVME_TARGET set.
Signed-off-by: John Meneghini <jmeneghi@redhat.com>
---
V10 -> V11:
- No change
---
drivers/scsi/scsi_transport_fc.c | 42 +++++++++++++++++++++++++-------
1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
index df1edf3c23fa..af309a7bdc22 100644
--- a/drivers/scsi/scsi_transport_fc.c
+++ b/drivers/scsi/scsi_transport_fc.c
@@ -1251,34 +1251,58 @@ static ssize_t fc_rport_set_marginal_state(struct device *dev,
const char *buf, size_t count)
{
struct fc_rport *rport = transport_class_to_rport(dev);
+ struct Scsi_Host *shost = rport_to_shost(rport);
+ struct fc_internal *i = to_fc_internal(shost->transportt);
enum fc_port_state port_state;
int ret = 0;
+ unsigned long flags;
ret = get_fc_port_state_match(buf, &port_state);
if (ret)
return -EINVAL;
- if (port_state == FC_PORTSTATE_MARGINAL) {
+
+ spin_lock_irqsave(shost->host_lock, flags);
+
+ switch (port_state) {
+ case FC_PORTSTATE_MARGINAL:
/*
* Change the state to Marginal only if the
* current rport state is Online
* Allow only Online->Marginal
*/
- if (rport->port_state == FC_PORTSTATE_ONLINE)
+ if (rport->port_state == FC_PORTSTATE_ONLINE) {
rport->port_state = port_state;
- else if (port_state != rport->port_state)
- return -EINVAL;
- } else if (port_state == FC_PORTSTATE_ONLINE) {
+ spin_unlock_irqrestore(shost->host_lock, flags);
+ if (i->f->set_rport_marginal)
+ i->f->set_rport_marginal(rport, true);
+ return count;
+ }
+ break;
+
+ case FC_PORTSTATE_ONLINE:
/*
* Change the state to Online only if the
* current rport state is Marginal
* Allow only Marginal->Online
*/
- if (rport->port_state == FC_PORTSTATE_MARGINAL)
+ if (rport->port_state == FC_PORTSTATE_MARGINAL) {
rport->port_state = port_state;
- else if (port_state != rport->port_state)
- return -EINVAL;
- } else
+ spin_unlock_irqrestore(shost->host_lock, flags);
+ if (i->f->set_rport_marginal)
+ i->f->set_rport_marginal(rport, false);
+ return count;
+ }
+ break;
+ default:
+ break;
+ }
+
+ if (port_state != rport->port_state) {
+ spin_unlock_irqrestore(shost->host_lock, flags);
return -EINVAL;
+ }
+
+ spin_unlock_irqrestore(shost->host_lock, flags);
return count;
}
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 08/13] scsi: scsi_transport_fc: user support for clearing NVME_CTRL_MARGINAL
2026-08-12 18:12 ` [PATCH 08/13] scsi: scsi_transport_fc: user support for clearing NVME_CTRL_MARGINAL Jesse Taube
@ 2026-08-12 18:24 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-08-12 18:24 UTC (permalink / raw)
To: Jesse Taube; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] A TOCTOU / race condition in `fc_rport_set_marginal_state` allows the software remote port state (`rport->port_state`) to become permanently desynchronized from the low-level driver's hardware state.
- [Medium] Meaningful mismatch between the commit message and the implemented code regarding the callback function used.
--
--- Patch [8]: [PATCH 08/13] scsi: scsi_transport_fc: user support for clearing NVME_CTRL_MARGINAL ---
commit 22fbcc98c1f2a6b4c603d5765d66e355a3468638
Author: John Meneghini <jmeneghi@redhat.com>
scsi: scsi_transport_fc: user support for clearing NVME_CTRL_MARGINAL
This commit refactors fc_rport_set_marginal_state to hold the host_lock
around port_state accesses. It also introduces a callback mechanism to
propagate the marginal state changes to the low-level driver when modified
via sysfs.
> Call nvme_fc_modify_rport_fpin_state() when FC_PORTSTATE_MARGINAL is set
> or cleared. This allows the user to quickly set or clear the
> NVME_CTRL_MARGINAL state from sysfs.
[Severity: Medium]
Does the commit message match the implemented code?
The message explicitly states that nvme_fc_modify_rport_fpin_state() is
called, but the code relies on the generic transport template callback
set_rport_marginal() instead.
> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> index df1edf3c23fa8..af309a7bdc221 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
[ ... ]
> @@ -1251,34 +1251,58 @@ static ssize_t fc_rport_set_marginal_state(struct device *dev,
[ ... ]
> + spin_lock_irqsave(shost->host_lock, flags);
> +
> + switch (port_state) {
> + case FC_PORTSTATE_MARGINAL:
> /*
> * Change the state to Marginal only if the
> * current rport state is Online
> * Allow only Online->Marginal
> */
> - if (rport->port_state == FC_PORTSTATE_ONLINE)
> + if (rport->port_state == FC_PORTSTATE_ONLINE) {
> rport->port_state = port_state;
> - else if (port_state != rport->port_state)
> - return -EINVAL;
> - } else if (port_state == FC_PORTSTATE_ONLINE) {
> + spin_unlock_irqrestore(shost->host_lock, flags);
> + if (i->f->set_rport_marginal)
> + i->f->set_rport_marginal(rport, true);
> + return count;
[Severity: High]
Could this sequence introduce a regression where the transport layer state
becomes permanently desynchronized from the hardware state?
By dropping the host_lock before calling set_rport_marginal(), a race window
is opened. If one thread sets the state to FC_PORTSTATE_MARGINAL and drops
the lock, and another thread concurrently sets the state to
FC_PORTSTATE_ONLINE, drops the lock, and executes set_rport_marginal() with
false, the first thread might then execute its callback with true. The
transport software state would be online, but the hardware state would be left
in a marginal state.
> + }
> + break;
> +
> + case FC_PORTSTATE_ONLINE:
> /*
> * Change the state to Online only if the
> * current rport state is Marginal
> * Allow only Marginal->Online
> */
> - if (rport->port_state == FC_PORTSTATE_MARGINAL)
> + if (rport->port_state == FC_PORTSTATE_MARGINAL) {
> rport->port_state = port_state;
> - else if (port_state != rport->port_state)
> - return -EINVAL;
> - } else
> + spin_unlock_irqrestore(shost->host_lock, flags);
> + if (i->f->set_rport_marginal)
> + i->f->set_rport_marginal(rport, false);
> + return count;
[Severity: High]
Is this code also vulnerable to the same regression? Dropping the lock prior
to executing set_rport_marginal() permits concurrent sysfs writes to reorder
the callback executions, which can lead to a state mismatch.
> + }
> + break;
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260812181300.3712426-1-jtaubepe@redhat.com?part=8
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 09/13] nvme-fc: add nvme_fc_set_remoteport_fpin()
2026-08-12 18:12 [PATCH 00/13] nvme-fc: FPIN link integrity handling Jesse Taube
` (7 preceding siblings ...)
2026-08-12 18:12 ` [PATCH 08/13] scsi: scsi_transport_fc: user support for clearing NVME_CTRL_MARGINAL Jesse Taube
@ 2026-08-12 18:12 ` Jesse Taube
2026-08-12 18:27 ` sashiko-bot
2026-08-12 18:12 ` [PATCH 10/13] scsi: qla2xxx: enable FPIN notification for NVMe Jesse Taube
` (3 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Jesse Taube @ 2026-08-12 18:12 UTC (permalink / raw)
To: linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Justin Tee, Naresh Gottumukkala,
Paul Ely, Chaitanya Kulkarni, James E.J. Bottomley,
Martin K. Petersen, Nilesh Javali, GR-QLogic-Storage-Upstream,
Hannes Reinecke, Jesse Taube, Gustavo A. R. Silva, John Meneghini,
Bryan Gurney, Chris Leech, Ewan D . Milne, shinichiro.kawasaki,
linux-doc, linux-kernel, linux-nvme, linux-block, Hannes Reinecke
Add nvme_fc_set_remoteport_fpin() and supporting functions. This
function is called by the SCSI FC transport and driver layer to set or
clear the 'marginal' path status for a specific rport.
Co-developed-by: Hannes Reinecke <hare@kernel.org>
Signed-off-by: Hannes Reinecke <hare@kernel.org>
Signed-off-by: John Meneghini <jmeneghi@redhat.com>
Signed-off-by: Jesse Taube <jtaubepe@redhat.com>
---
V10 -> V11:
- Remove nvme_fc_modify_rport_fpin_state
- Use struct nvme_fc_remote_port instead of wwpn and wwnn
---
drivers/nvme/host/fc.c | 16 ++++++++++++++++
include/linux/nvme-fc-driver.h | 2 ++
2 files changed, 18 insertions(+)
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index d2ca65f70a0c..4acf23ea8aea 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -895,6 +895,22 @@ nvme_fc_set_remoteport_devloss(struct nvme_fc_remote_port *portptr,
}
EXPORT_SYMBOL_GPL(nvme_fc_set_remoteport_devloss);
+void
+nvme_fc_set_remoteport_fpin(struct nvme_fc_remote_port *portptr, bool marginal)
+{
+ struct nvme_fc_rport *rport = remoteport_to_rport(portptr);
+ struct nvme_fc_ctrl *ctrl;
+
+ spin_lock_irq(&rport->lock);
+ list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
+ if (marginal)
+ set_bit(NVME_CTRL_MARGINAL, &ctrl->ctrl.flags);
+ else
+ clear_bit(NVME_CTRL_MARGINAL, &ctrl->ctrl.flags);
+ }
+ spin_unlock_irq(&rport->lock);
+}
+EXPORT_SYMBOL_GPL(nvme_fc_set_remoteport_fpin);
/* *********************** FC-NVME DMA Handling **************************** */
diff --git a/include/linux/nvme-fc-driver.h b/include/linux/nvme-fc-driver.h
index 9f6acadfe0c8..95d79386d126 100644
--- a/include/linux/nvme-fc-driver.h
+++ b/include/linux/nvme-fc-driver.h
@@ -536,6 +536,8 @@ void nvme_fc_rescan_remoteport(struct nvme_fc_remote_port *remoteport);
int nvme_fc_set_remoteport_devloss(struct nvme_fc_remote_port *remoteport,
u32 dev_loss_tmo);
+void nvme_fc_set_remoteport_fpin(struct nvme_fc_remote_port *portptr, bool marginal);
+
/*
* Routine called to pass a NVME-FC LS request, received by the lldd,
* to the nvme-fc transport.
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 09/13] nvme-fc: add nvme_fc_set_remoteport_fpin()
2026-08-12 18:12 ` [PATCH 09/13] nvme-fc: add nvme_fc_set_remoteport_fpin() Jesse Taube
@ 2026-08-12 18:27 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-08-12 18:27 UTC (permalink / raw)
To: Jesse Taube; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] `nvme_fc_ctrl_connectivity_loss()` clears the `NVME_CTRL_MARGINAL` bit on the wrong structure's flags field, causing state corruption and permanent marginal state.
--
--- Patch [9]: [PATCH 09/13] nvme-fc: add nvme_fc_set_remoteport_fpin() ---
commit a1aaa3c652a2b842059c76da3884f9c0dcb9925a
Author: Jesse Taube <jtaubepe@redhat.com>
nvme-fc: add nvme_fc_set_remoteport_fpin()
Add nvme_fc_set_remoteport_fpin() and supporting functions. This
function is called by the SCSI FC transport and driver layer to set or
clear the 'marginal' path status for a specific rport.
> diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
> index d2ca65f70a0c5..4acf23ea8aea2 100644
> --- a/drivers/nvme/host/fc.c
> +++ b/drivers/nvme/host/fc.c
> @@ -895,6 +895,22 @@ nvme_fc_set_remoteport_devloss(struct nvme_fc_remote_port *portptr,
> }
> EXPORT_SYMBOL_GPL(nvme_fc_set_remoteport_devloss);
>
> +void
> +nvme_fc_set_remoteport_fpin(struct nvme_fc_remote_port *portptr, bool marginal)
> +{
> + struct nvme_fc_rport *rport = remoteport_to_rport(portptr);
> + struct nvme_fc_ctrl *ctrl;
> +
> + spin_lock_irq(&rport->lock);
> + list_for_each_entry(ctrl, &rport->ctrl_list, ctrl_list) {
> + if (marginal)
> + set_bit(NVME_CTRL_MARGINAL, &ctrl->ctrl.flags);
> + else
> + clear_bit(NVME_CTRL_MARGINAL, &ctrl->ctrl.flags);
[Severity: High]
This code correctly uses &ctrl->ctrl.flags for NVME_CTRL_MARGINAL. However, in
nvme_fc_ctrl_connectivity_loss() introduced in the earlier commit "nvme: add
NVME_CTRL_MARGINAL flag", the marginal state is cleared like this:
drivers/nvme/host/fc.c:nvme_fc_ctrl_connectivity_loss() {
...
/* clear 'marginal' flag as controller will be reset */
clear_bit(NVME_CTRL_MARGINAL, &ctrl->flags);
...
}
Could clearing NVME_CTRL_MARGINAL on the transport-specific &ctrl->flags
instead of &ctrl->ctrl.flags leave the NVMe controller permanently marked as
marginal across reconnects, preventing the multipath scheduler from fully
utilizing the repaired path?
> + }
> + spin_unlock_irq(&rport->lock);
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260812181300.3712426-1-jtaubepe@redhat.com?part=9
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 10/13] scsi: qla2xxx: enable FPIN notification for NVMe
2026-08-12 18:12 [PATCH 00/13] nvme-fc: FPIN link integrity handling Jesse Taube
` (8 preceding siblings ...)
2026-08-12 18:12 ` [PATCH 09/13] nvme-fc: add nvme_fc_set_remoteport_fpin() Jesse Taube
@ 2026-08-12 18:12 ` Jesse Taube
2026-08-12 18:34 ` sashiko-bot
2026-08-12 18:12 ` [PATCH 11/13] scsi: lpfc: " Jesse Taube
` (2 subsequent siblings)
12 siblings, 1 reply; 30+ messages in thread
From: Jesse Taube @ 2026-08-12 18:12 UTC (permalink / raw)
To: linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Justin Tee, Naresh Gottumukkala,
Paul Ely, Chaitanya Kulkarni, James E.J. Bottomley,
Martin K. Petersen, Nilesh Javali, GR-QLogic-Storage-Upstream,
Hannes Reinecke, Jesse Taube, Gustavo A. R. Silva, John Meneghini,
Bryan Gurney, Chris Leech, Ewan D . Milne, shinichiro.kawasaki,
linux-doc, linux-kernel, linux-nvme, linux-block
Implement set_rport_marginal callback using
nvme_fc_set_remoteport_fpin() to enable FPIN notifications for NVMe.
Signed-off-by: Jesse Taube <jtaubepe@redhat.com>
---
V10 -> V11:
- Rewrite
---
drivers/scsi/qla2xxx/qla_attr.c | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
index a4ca22024ede..92db7d584eeb 100644
--- a/drivers/scsi/qla2xxx/qla_attr.c
+++ b/drivers/scsi/qla2xxx/qla_attr.c
@@ -2789,6 +2789,15 @@ qla2x00_set_rport_loss_tmo(struct fc_rport *rport, uint32_t timeout)
rport->dev_loss_tmo);
}
+static inline void
+qla2x00_set_rport_marginal(struct fc_rport *rport, bool marginal)
+{
+ fc_port_t *fcport = *(fc_port_t **)rport->dd_data;
+
+ if (IS_ENABLED(CONFIG_NVME_FC) && fcport && fcport->nvme_remote_port)
+ nvme_fc_set_remoteport_fpin(fcport->nvme_remote_port, marginal);
+}
+
static void
qla2x00_dev_loss_tmo_callbk(struct fc_rport *rport)
{
@@ -3379,6 +3388,8 @@ struct fc_function_template qla2xxx_transport_functions = {
.set_rport_dev_loss_tmo = qla2x00_set_rport_loss_tmo,
.show_rport_dev_loss_tmo = 1,
+ .set_rport_marginal = qla2x00_set_rport_marginal,
+
.issue_fc_host_lip = qla2x00_issue_lip,
.dev_loss_tmo_callbk = qla2x00_dev_loss_tmo_callbk,
.terminate_rport_io = qla2x00_terminate_rport_io,
@@ -3427,6 +3438,8 @@ struct fc_function_template qla2xxx_transport_vport_functions = {
.set_rport_dev_loss_tmo = qla2x00_set_rport_loss_tmo,
.show_rport_dev_loss_tmo = 1,
+ .set_rport_marginal = qla2x00_set_rport_marginal,
+
.issue_fc_host_lip = qla2x00_issue_lip,
.dev_loss_tmo_callbk = qla2x00_dev_loss_tmo_callbk,
.terminate_rport_io = qla2x00_terminate_rport_io,
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 10/13] scsi: qla2xxx: enable FPIN notification for NVMe
2026-08-12 18:12 ` [PATCH 10/13] scsi: qla2xxx: enable FPIN notification for NVMe Jesse Taube
@ 2026-08-12 18:34 ` sashiko-bot
2026-08-12 19:38 ` Jesse Taube
0 siblings, 1 reply; 30+ messages in thread
From: sashiko-bot @ 2026-08-12 18:34 UTC (permalink / raw)
To: Jesse Taube; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Lockless access to `fcport->nvme_remote_port` in `qla2x00_set_rport_marginal()` introduces TOCTOU Use-After-Free and NULL pointer dereferences.
--
--- Patch [10]: [PATCH 10/13] scsi: qla2xxx: enable FPIN notification for NVMe ---
commit 1b0bf8a2abd4965d07b3b9fd652b0926356c721b
Author: Jesse Taube <jtaubepe@redhat.com>
scsi: qla2xxx: enable FPIN notification for NVMe
Implement set_rport_marginal callback using nvme_fc_set_remoteport_fpin()
to enable FPIN notifications for NVMe.
> diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
> index a4ca22024edeb..92db7d584eeb8 100644
> --- a/drivers/scsi/qla2xxx/qla_attr.c
> +++ b/drivers/scsi/qla2xxx/qla_attr.c
> @@ -2789,6 +2789,15 @@ qla2x00_set_rport_loss_tmo(struct fc_rport *rport, uint32_t timeout)
> rport->dev_loss_tmo);
> }
>
> +static inline void
> +qla2x00_set_rport_marginal(struct fc_rport *rport, bool marginal)
> +{
> + fc_port_t *fcport = *(fc_port_t **)rport->dd_data;
> +
> + if (IS_ENABLED(CONFIG_NVME_FC) && fcport && fcport->nvme_remote_port)
> + nvme_fc_set_remoteport_fpin(fcport->nvme_remote_port, marginal);
[Severity: High]
Does this code introduce a Time-Of-Check to Time-Of-Use (TOCTOU) race condition?
The nvme_remote_port pointer is read and dereferenced locklessly here. If a
device loss event triggers qla_nvme_remoteport_delete() concurrently, it sets
fcport->nvme_remote_port = NULL and allows the NVMe FC transport to free the
remote port memory.
If qla2x00_set_rport_marginal() checks that nvme_remote_port is non-NULL but
is preempted before calling nvme_fc_set_remoteport_fpin(), it will pass a stale
pointer to freed memory, causing a use-after-free.
Alternatively, because the pointer is fetched without READ_ONCE(), the compiler
could reload the now-NULL pointer from memory, leading to a NULL pointer
dereference inside nvme_fc_set_remoteport_fpin().
This pre-existing vulnerability is also present in qla2x00_set_rport_loss_tmo()
just above this new function.
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260812181300.3712426-1-jtaubepe@redhat.com?part=10
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 10/13] scsi: qla2xxx: enable FPIN notification for NVMe
2026-08-12 18:34 ` sashiko-bot
@ 2026-08-12 19:38 ` Jesse Taube
0 siblings, 0 replies; 30+ messages in thread
From: Jesse Taube @ 2026-08-12 19:38 UTC (permalink / raw)
To: sashiko-reviews; +Cc: linux-scsi
On Wed, Aug 12, 2026 at 2:35 PM <sashiko-bot@kernel.org> wrote:
>
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] Lockless access to `fcport->nvme_remote_port` in `qla2x00_set_rport_marginal()` introduces TOCTOU Use-After-Free and NULL pointer dereferences.
> --
>
> --- Patch [10]: [PATCH 10/13] scsi: qla2xxx: enable FPIN notification for NVMe ---
> commit 1b0bf8a2abd4965d07b3b9fd652b0926356c721b
> Author: Jesse Taube <jtaubepe@redhat.com>
>
> scsi: qla2xxx: enable FPIN notification for NVMe
>
> Implement set_rport_marginal callback using nvme_fc_set_remoteport_fpin()
> to enable FPIN notifications for NVMe.
>
> > diff --git a/drivers/scsi/qla2xxx/qla_attr.c b/drivers/scsi/qla2xxx/qla_attr.c
> > index a4ca22024edeb..92db7d584eeb8 100644
> > --- a/drivers/scsi/qla2xxx/qla_attr.c
> > +++ b/drivers/scsi/qla2xxx/qla_attr.c
> > @@ -2789,6 +2789,15 @@ qla2x00_set_rport_loss_tmo(struct fc_rport *rport, uint32_t timeout)
> > rport->dev_loss_tmo);
> > }
> >
> > +static inline void
> > +qla2x00_set_rport_marginal(struct fc_rport *rport, bool marginal)
> > +{
> > + fc_port_t *fcport = *(fc_port_t **)rport->dd_data;
> > +
> > + if (IS_ENABLED(CONFIG_NVME_FC) && fcport && fcport->nvme_remote_port)
> > + nvme_fc_set_remoteport_fpin(fcport->nvme_remote_port, marginal);
>
> [Severity: High]
> Does this code introduce a Time-Of-Check to Time-Of-Use (TOCTOU) race condition?
>
> The nvme_remote_port pointer is read and dereferenced locklessly here. If a
> device loss event triggers qla_nvme_remoteport_delete() concurrently, it sets
> fcport->nvme_remote_port = NULL and allows the NVMe FC transport to free the
> remote port memory.
>
> If qla2x00_set_rport_marginal() checks that nvme_remote_port is non-NULL but
> is preempted before calling nvme_fc_set_remoteport_fpin(), it will pass a stale
> pointer to freed memory, causing a use-after-free.
>
> Alternatively, because the pointer is fetched without READ_ONCE(), the compiler
This is not a valid solution, there would need to be a lock in
fc_port_t or else we would
get a use-after-free. Unless there is a better solution im unaware of
im going to ignore this
as it already exists in the driver.
Thanks,
Jesse Taube
> could reload the now-NULL pointer from memory, leading to a NULL pointer
> dereference inside nvme_fc_set_remoteport_fpin().
>
> This pre-existing vulnerability is also present in qla2x00_set_rport_loss_tmo()
> just above this new function.
>
> > +}
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/20260812181300.3712426-1-jtaubepe@redhat.com?part=10
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 11/13] scsi: lpfc: enable FPIN notification for NVMe
2026-08-12 18:12 [PATCH 00/13] nvme-fc: FPIN link integrity handling Jesse Taube
` (9 preceding siblings ...)
2026-08-12 18:12 ` [PATCH 10/13] scsi: qla2xxx: enable FPIN notification for NVMe Jesse Taube
@ 2026-08-12 18:12 ` Jesse Taube
2026-08-12 18:35 ` sashiko-bot
2026-08-12 18:12 ` [PATCH 12/13] nvme: fcloop: Add set_rport_marginal to sysfs Jesse Taube
2026-08-12 18:13 ` [PATCH 13/13] docs: nvme-multipath: Add FC-NVMe marginal state Jesse Taube
12 siblings, 1 reply; 30+ messages in thread
From: Jesse Taube @ 2026-08-12 18:12 UTC (permalink / raw)
To: linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Justin Tee, Naresh Gottumukkala,
Paul Ely, Chaitanya Kulkarni, James E.J. Bottomley,
Martin K. Petersen, Nilesh Javali, GR-QLogic-Storage-Upstream,
Hannes Reinecke, Jesse Taube, Gustavo A. R. Silva, John Meneghini,
Bryan Gurney, Chris Leech, Ewan D . Milne, shinichiro.kawasaki,
linux-doc, linux-kernel, linux-nvme, linux-block
Implement set_rport_marginal callback using
nvme_fc_set_remoteport_fpin() to enable FPIN notifications for NVMe.
Signed-off-by: Jesse Taube <jtaubepe@redhat.com>
---
V10 -> V11
- Rewrite
---
drivers/scsi/lpfc/lpfc_attr.c | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/drivers/scsi/lpfc/lpfc_attr.c b/drivers/scsi/lpfc/lpfc_attr.c
index f4e8164b94ab..d54899cceed9 100644
--- a/drivers/scsi/lpfc/lpfc_attr.c
+++ b/drivers/scsi/lpfc/lpfc_attr.c
@@ -7134,6 +7134,30 @@ lpfc_set_rport_loss_tmo(struct fc_rport *rport, uint32_t timeout)
#endif
}
+static void
+lpfc_set_rport_marginal(struct fc_rport *rport, bool marginal)
+{
+ struct lpfc_rport_data *rdata = rport->dd_data;
+ struct lpfc_nodelist *ndlp = rdata->pnode;
+ struct lpfc_nvme_rport *nrport = NULL;
+
+ /* Break early if NVME_FC is not enabled */
+ if (!IS_ENABLED(CONFIG_NVME_FC))
+ return;
+
+ if (!ndlp) {
+ dev_info(&rport->dev, "Cannot find remote node to ");
+ dev_info(&rport->dev, "set rport marginal, port_id x%x\n",
+ rport->port_id);
+ return;
+ }
+
+ nrport = lpfc_ndlp_get_nrport(ndlp);
+
+ if (nrport && nrport->remoteport)
+ nvme_fc_set_remoteport_fpin(nrport->remoteport, marginal);
+}
+
/*
* lpfc_rport_show_function - Return rport target information
*
@@ -7244,6 +7268,8 @@ struct fc_function_template lpfc_transport_functions = {
.set_rport_dev_loss_tmo = lpfc_set_rport_loss_tmo,
.show_rport_dev_loss_tmo = 1,
+ .set_rport_marginal = lpfc_set_rport_marginal,
+
.get_starget_port_id = lpfc_get_starget_port_id,
.show_starget_port_id = 1,
@@ -7315,6 +7341,8 @@ struct fc_function_template lpfc_vport_transport_functions = {
.set_rport_dev_loss_tmo = lpfc_set_rport_loss_tmo,
.show_rport_dev_loss_tmo = 1,
+ .set_rport_marginal = lpfc_set_rport_marginal,
+
.get_starget_port_id = lpfc_get_starget_port_id,
.show_starget_port_id = 1,
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 11/13] scsi: lpfc: enable FPIN notification for NVMe
2026-08-12 18:12 ` [PATCH 11/13] scsi: lpfc: " Jesse Taube
@ 2026-08-12 18:35 ` sashiko-bot
0 siblings, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-08-12 18:35 UTC (permalink / raw)
To: Jesse Taube; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Low] Logging Formatting Defect: Error message is improperly split across two consecutive `dev_info()` calls.
Pre-existing issues:
- [High] Concurrency / Use-After-Free: Lockless access to `ndlp->nrport` and `nrport->remoteport` can lead to a Use-After-Free due to concurrent unregistration.
--
--- Patch [11]: [PATCH 11/13] scsi: lpfc: enable FPIN notification for NVMe ---
commit 6d41021b653095acd49d1f7828d07c1405835a16
Author: Jesse Taube <jtaubepe@redhat.com>
scsi: lpfc: enable FPIN notification for NVMe
Implement set_rport_marginal callback using nvme_fc_set_remoteport_fpin() to
enable FPIN notifications for NVMe.
> diff --git a/drivers/scsi/lpfc/lpfc_attr.c b/drivers/scsi/lpfc/lpfc_attr.c
> index f4e8164b94abb..d54899cceed99 100644
> --- a/drivers/scsi/lpfc/lpfc_attr.c
> +++ b/drivers/scsi/lpfc/lpfc_attr.c
[ ... ]
> @@ -7134,6 +7134,30 @@ lpfc_set_rport_loss_tmo(struct fc_rport *rport, uint32_t timeout)
> #endif
> }
>
> +static void
> +lpfc_set_rport_marginal(struct fc_rport *rport, bool marginal)
> +{
> + struct lpfc_rport_data *rdata = rport->dd_data;
> + struct lpfc_nodelist *ndlp = rdata->pnode;
> + struct lpfc_nvme_rport *nrport = NULL;
> +
> + /* Break early if NVME_FC is not enabled */
> + if (!IS_ENABLED(CONFIG_NVME_FC))
> + return;
> +
> + if (!ndlp) {
> + dev_info(&rport->dev, "Cannot find remote node to ");
> + dev_info(&rport->dev, "set rport marginal, port_id x%x\n",
> + rport->port_id);
[Severity: Low]
Does this code format the output correctly?
Because dev_info() prepends the device name and KERN_INFO to every
invocation, splitting the message across two calls without a newline on the
first will cause the prefix to be injected in the middle of the sentence in
the kernel log.
> + return;
> + }
> +
> + nrport = lpfc_ndlp_get_nrport(ndlp);
> +
> + if (nrport && nrport->remoteport)
> + nvme_fc_set_remoteport_fpin(nrport->remoteport, marginal);
[Severity: High]
This is a pre-existing issue, but does this lockless retrieval of nrport via
lpfc_ndlp_get_nrport() safely protect against concurrent unregistration?
The macro performs a lockless double-fetch of ndlp->nrport without acquiring
ndlp->lock:
drivers/scsi/lpfc/lpfc_nvme.h:
#define lpfc_ndlp_get_nrport(ndlp) \
((!ndlp->nrport || (ndlp->fc4_xpt_flags & NVME_XPT_UNREG_WAIT))\
? NULL : ndlp->nrport)
If a concurrent remote port loss triggers lpfc_nvme_unregister_port(), it
could free the NVMe remote port memory before setting ndlp->nrport = NULL.
Could this code then read a dangling pointer and pass nrport->remoteport
to nvme_fc_set_remoteport_fpin(), resulting in a Use-After-Free?
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260812181300.3712426-1-jtaubepe@redhat.com?part=11
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 12/13] nvme: fcloop: Add set_rport_marginal to sysfs
2026-08-12 18:12 [PATCH 00/13] nvme-fc: FPIN link integrity handling Jesse Taube
` (10 preceding siblings ...)
2026-08-12 18:12 ` [PATCH 11/13] scsi: lpfc: " Jesse Taube
@ 2026-08-12 18:12 ` Jesse Taube
2026-08-12 18:31 ` sashiko-bot
2026-08-12 18:34 ` Jesse Taube
2026-08-12 18:13 ` [PATCH 13/13] docs: nvme-multipath: Add FC-NVMe marginal state Jesse Taube
12 siblings, 2 replies; 30+ messages in thread
From: Jesse Taube @ 2026-08-12 18:12 UTC (permalink / raw)
To: linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Justin Tee, Naresh Gottumukkala,
Paul Ely, Chaitanya Kulkarni, James E.J. Bottomley,
Martin K. Petersen, Nilesh Javali, GR-QLogic-Storage-Upstream,
Hannes Reinecke, Jesse Taube, Gustavo A. R. Silva, John Meneghini,
Bryan Gurney, Chris Leech, Ewan D . Milne, shinichiro.kawasaki,
linux-doc, linux-kernel, linux-nvme, linux-block
To allow testing of multipath failover, add a sysfs attribute to set a
remote port as marginal. This will allow the fcloop LLDD to set the
marginal flag on a remote port, simulating a marginal link.
Example:
Turn on marginal for a remote port matching wwnn and wwpn:
`echo 'wwnn=0x200000109b5f2956,wwpn=0x100000109b5f2956,marginal=1' >
/sys/class/fcloop/ctl/set_rport_marginal`
Turn off marginal for a remote port matching wwnn and wwpn:
`echo 'wwnn=0x200000109b5f2956,wwpn=0x100000109b5f2956,marginal=0' >
/sys/class/fcloop/ctl/set_rport_marginal`
Suggested-by: John Meneghini <jmeneghi@redhat.com>
Signed-off-by: Jesse Taube <jtaubepe@redhat.com>
V10 -> V11
- New patch
---
drivers/nvme/target/fcloop.c | 41 ++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
index b63af3b643a6..51a50743128d 100644
--- a/drivers/nvme/target/fcloop.c
+++ b/drivers/nvme/target/fcloop.c
@@ -6,6 +6,7 @@
#include <linux/module.h>
#include <linux/parser.h>
#include <uapi/scsi/fc/fc_fs.h>
+#include <uapi/scsi/fc/fc_els.h>
#include "../host/nvme.h"
#include "../target/nvmet.h"
@@ -21,6 +22,7 @@ enum {
NVMF_OPT_FCADDR = 1 << 3,
NVMF_OPT_LPWWNN = 1 << 4,
NVMF_OPT_LPWWPN = 1 << 5,
+ NVMF_OPT_MARGINAL = 1 << 6,
};
struct fcloop_ctrl_options {
@@ -31,6 +33,7 @@ struct fcloop_ctrl_options {
u32 fcaddr;
u64 lpwwnn;
u64 lpwwpn;
+ u32 marginal;
};
static const match_table_t opt_tokens = {
@@ -40,6 +43,7 @@ static const match_table_t opt_tokens = {
{ NVMF_OPT_FCADDR, "fcaddr=%x" },
{ NVMF_OPT_LPWWNN, "lpwwnn=%s" },
{ NVMF_OPT_LPWWPN, "lpwwpn=%s" },
+ { NVMF_OPT_MARGINAL, "marginal=%d" },
{ NVMF_OPT_ERR, NULL }
};
@@ -120,6 +124,13 @@ fcloop_parse_options(struct fcloop_ctrl_options *opts,
}
opts->lpwwpn = token64;
break;
+ case NVMF_OPT_MARGINAL:
+ if (match_int(args, &token)) {
+ ret = -EINVAL;
+ goto out_free_options;
+ }
+ opts->marginal = token;
+ break;
default:
pr_warn("unknown parameter or missing value '%s'\n", p);
ret = -EINVAL;
@@ -199,6 +210,9 @@ fcloop_parse_nm_options(struct device *dev, u64 *nname, u64 *pname,
#define TGTPORT_OPTS (NVMF_OPT_WWNN | NVMF_OPT_WWPN)
+#define MARGINAL_OPTS (NVMF_OPT_WWNN | NVMF_OPT_WWPN | \
+ NVMF_OPT_MARGINAL)
+
static DEFINE_SPINLOCK(fcloop_lock);
static LIST_HEAD(fcloop_lports);
@@ -1663,6 +1677,31 @@ fcloop_set_cmd_drop(struct device *dev, struct device_attribute *attr,
return count;
}
+static ssize_t
+fcloop_set_marginal_rport(struct device *dev, struct device_attribute *attr,
+ const char *buf, size_t count)
+{
+ struct fcloop_nport *nport;
+ struct fcloop_ctrl_options opts;
+ int ret;
+
+ ret = fcloop_parse_options(&opts, buf);
+ if (ret)
+ return ret;
+
+ /* everything there ? */
+ if ((opts.mask & MARGINAL_OPTS) != MARGINAL_OPTS)
+ return -EINVAL;
+
+ nport = fcloop_nport_lookup(opts.wwnn, opts.wwpn);
+ if (!nport || !nport->tport || !nport->tport->remoteport)
+ return -ENOENT;
+
+ nvme_fc_set_remoteport_fpin(nport->tport->remoteport, opts.marginal);
+ fcloop_nport_put(nport);
+
+ return count;
+}
static DEVICE_ATTR(add_local_port, 0200, NULL, fcloop_create_local_port);
static DEVICE_ATTR(del_local_port, 0200, NULL, fcloop_delete_local_port);
@@ -1671,6 +1710,7 @@ static DEVICE_ATTR(del_remote_port, 0200, NULL, fcloop_delete_remote_port);
static DEVICE_ATTR(add_target_port, 0200, NULL, fcloop_create_target_port);
static DEVICE_ATTR(del_target_port, 0200, NULL, fcloop_delete_target_port);
static DEVICE_ATTR(set_cmd_drop, 0200, NULL, fcloop_set_cmd_drop);
+static DEVICE_ATTR(set_marginal_rport, 0200, NULL, fcloop_set_marginal_rport);
static struct attribute *fcloop_dev_attrs[] = {
&dev_attr_add_local_port.attr,
@@ -1680,6 +1720,7 @@ static struct attribute *fcloop_dev_attrs[] = {
&dev_attr_add_target_port.attr,
&dev_attr_del_target_port.attr,
&dev_attr_set_cmd_drop.attr,
+ &dev_attr_set_marginal_rport.attr,
NULL
};
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 12/13] nvme: fcloop: Add set_rport_marginal to sysfs
2026-08-12 18:12 ` [PATCH 12/13] nvme: fcloop: Add set_rport_marginal to sysfs Jesse Taube
@ 2026-08-12 18:31 ` sashiko-bot
2026-08-12 18:34 ` Jesse Taube
1 sibling, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-08-12 18:31 UTC (permalink / raw)
To: Jesse Taube; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The `opts` structure is allocated on the stack and passed uninitialized to `fcloop_parse_options`, leading to undefined behavior and use of stack garbage.
- [High] A reference count leak occurs on the error path in `fcloop_set_marginal_rport` when the target port or remote port is missing.
- [High] Lockless access to `nport->tport` and `nport->tport->remoteport` creates a TOCTOU bug that can lead to a Use-After-Free.
--
--- Patch [12]: [PATCH 12/13] nvme: fcloop: Add set_rport_marginal to sysfs ---
commit 0c2a40a45b26fd619c8713ddb24578a051043b92
Author: Jesse Taube <jtaubepe@redhat.com>
nvme: fcloop: Add set_rport_marginal to sysfs
This commit adds a sysfs attribute to allow setting a remote port as marginal.
This helps facilitate testing of multipath failover by simulating a marginal
link in the fcloop LLDD.
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index b63af3b643a64..51a50743128db 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
[ ... ]
> @@ -1663,6 +1677,31 @@ fcloop_set_cmd_drop(struct device *dev, struct device_attribute *attr,
> return count;
> }
>
> +static ssize_t
> +fcloop_set_marginal_rport(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct fcloop_nport *nport;
> + struct fcloop_ctrl_options opts;
[Severity: High]
Does this leave the opts structure uninitialized?
Looking at fcloop_parse_options(), it uses opts->mask |= token during parsing,
which performs a read-modify-write operation. Since opts is allocated on the
stack here without initialization, could this use stack garbage and result in
false validation bypass or incorrect functional behavior?
> + int ret;
> +
> + ret = fcloop_parse_options(&opts, buf);
> + if (ret)
> + return ret;
> +
> + /* everything there ? */
> + if ((opts.mask & MARGINAL_OPTS) != MARGINAL_OPTS)
> + return -EINVAL;
> +
> + nport = fcloop_nport_lookup(opts.wwnn, opts.wwpn);
> + if (!nport || !nport->tport || !nport->tport->remoteport)
> + return -ENOENT;
[Severity: High]
If nport is successfully found but !nport->tport or !nport->tport->remoteport
is true, does this leak the reference count on nport?
The fcloop_nport_lookup() function increments the reference count of the
returned nport. Returning -ENOENT early here skips the fcloop_nport_put()
call, which would permanently prevent the nport from being freed.
[Severity: High]
Is it safe to access nport->tport and nport->tport->remoteport locklessly
here?
Could a concurrent write to del_target_port take fcloop_lock, clear these
pointers, and free the targetport memory while this function is actively
dereferencing them, resulting in a use-after-free?
> +
> + nvme_fc_set_remoteport_fpin(nport->tport->remoteport, opts.marginal);
> + fcloop_nport_put(nport);
> +
> + return count;
> +}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260812181300.3712426-1-jtaubepe@redhat.com?part=12
^ permalink raw reply [flat|nested] 30+ messages in thread* Re: [PATCH 12/13] nvme: fcloop: Add set_rport_marginal to sysfs
2026-08-12 18:12 ` [PATCH 12/13] nvme: fcloop: Add set_rport_marginal to sysfs Jesse Taube
2026-08-12 18:31 ` sashiko-bot
@ 2026-08-12 18:34 ` Jesse Taube
1 sibling, 0 replies; 30+ messages in thread
From: Jesse Taube @ 2026-08-12 18:34 UTC (permalink / raw)
To: linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Naresh Gottumukkala, Paul Ely,
Chaitanya Kulkarni, James E.J. Bottomley, Martin K. Petersen,
Nilesh Javali, GR-QLogic-Storage-Upstream, Hannes Reinecke,
Gustavo A. R. Silva, John Meneghini, Bryan Gurney, Chris Leech,
Ewan D . Milne, shinichiro.kawasaki, linux-doc, linux-kernel,
linux-nvme, linux-block
On Wed, Aug 12, 2026 at 2:14 PM Jesse Taube <jtaubepe@redhat.com> wrote:
>
> To allow testing of multipath failover, add a sysfs attribute to set a
> remote port as marginal. This will allow the fcloop LLDD to set the
> marginal flag on a remote port, simulating a marginal link.
>
> Example:
> Turn on marginal for a remote port matching wwnn and wwpn:
> `echo 'wwnn=0x200000109b5f2956,wwpn=0x100000109b5f2956,marginal=1' >
> /sys/class/fcloop/ctl/set_rport_marginal`
>
> Turn off marginal for a remote port matching wwnn and wwpn:
> `echo 'wwnn=0x200000109b5f2956,wwpn=0x100000109b5f2956,marginal=0' >
> /sys/class/fcloop/ctl/set_rport_marginal`
>
> Suggested-by: John Meneghini <jmeneghi@redhat.com>
> Signed-off-by: Jesse Taube <jtaubepe@redhat.com>
> V10 -> V11
> - New patch
> ---
> drivers/nvme/target/fcloop.c | 41 ++++++++++++++++++++++++++++++++++++
> 1 file changed, 41 insertions(+)
>
> diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c
> index b63af3b643a6..51a50743128d 100644
> --- a/drivers/nvme/target/fcloop.c
> +++ b/drivers/nvme/target/fcloop.c
> @@ -6,6 +6,7 @@
> #include <linux/module.h>
> #include <linux/parser.h>
> #include <uapi/scsi/fc/fc_fs.h>
> +#include <uapi/scsi/fc/fc_els.h>
>
> #include "../host/nvme.h"
> #include "../target/nvmet.h"
> @@ -21,6 +22,7 @@ enum {
> NVMF_OPT_FCADDR = 1 << 3,
> NVMF_OPT_LPWWNN = 1 << 4,
> NVMF_OPT_LPWWPN = 1 << 5,
> + NVMF_OPT_MARGINAL = 1 << 6,
> };
>
> struct fcloop_ctrl_options {
> @@ -31,6 +33,7 @@ struct fcloop_ctrl_options {
> u32 fcaddr;
> u64 lpwwnn;
> u64 lpwwpn;
> + u32 marginal;
> };
>
> static const match_table_t opt_tokens = {
> @@ -40,6 +43,7 @@ static const match_table_t opt_tokens = {
> { NVMF_OPT_FCADDR, "fcaddr=%x" },
> { NVMF_OPT_LPWWNN, "lpwwnn=%s" },
> { NVMF_OPT_LPWWPN, "lpwwpn=%s" },
> + { NVMF_OPT_MARGINAL, "marginal=%d" },
> { NVMF_OPT_ERR, NULL }
> };
>
> @@ -120,6 +124,13 @@ fcloop_parse_options(struct fcloop_ctrl_options *opts,
> }
> opts->lpwwpn = token64;
> break;
> + case NVMF_OPT_MARGINAL:
> + if (match_int(args, &token)) {
> + ret = -EINVAL;
> + goto out_free_options;
> + }
> + opts->marginal = token;
> + break;
> default:
> pr_warn("unknown parameter or missing value '%s'\n", p);
> ret = -EINVAL;
> @@ -199,6 +210,9 @@ fcloop_parse_nm_options(struct device *dev, u64 *nname, u64 *pname,
>
> #define TGTPORT_OPTS (NVMF_OPT_WWNN | NVMF_OPT_WWPN)
>
> +#define MARGINAL_OPTS (NVMF_OPT_WWNN | NVMF_OPT_WWPN | \
> + NVMF_OPT_MARGINAL)
> +
>
> static DEFINE_SPINLOCK(fcloop_lock);
> static LIST_HEAD(fcloop_lports);
> @@ -1663,6 +1677,31 @@ fcloop_set_cmd_drop(struct device *dev, struct device_attribute *attr,
> return count;
> }
>
> +static ssize_t
> +fcloop_set_marginal_rport(struct device *dev, struct device_attribute *attr,
> + const char *buf, size_t count)
> +{
> + struct fcloop_nport *nport;
> + struct fcloop_ctrl_options opts;
> + int ret;
> +
> + ret = fcloop_parse_options(&opts, buf);
> + if (ret)
> + return ret;
> +
> + /* everything there ? */
> + if ((opts.mask & MARGINAL_OPTS) != MARGINAL_OPTS)
> + return -EINVAL;
> +
> + nport = fcloop_nport_lookup(opts.wwnn, opts.wwpn);
> + if (!nport || !nport->tport || !nport->tport->remoteport)
> + return -ENOENT;
> +
> + nvme_fc_set_remoteport_fpin(nport->tport->remoteport, opts.marginal);
> + fcloop_nport_put(nport);
> +
> + return count;
> +}
>
> static DEVICE_ATTR(add_local_port, 0200, NULL, fcloop_create_local_port);
> static DEVICE_ATTR(del_local_port, 0200, NULL, fcloop_delete_local_port);
> @@ -1671,6 +1710,7 @@ static DEVICE_ATTR(del_remote_port, 0200, NULL, fcloop_delete_remote_port);
> static DEVICE_ATTR(add_target_port, 0200, NULL, fcloop_create_target_port);
> static DEVICE_ATTR(del_target_port, 0200, NULL, fcloop_delete_target_port);
> static DEVICE_ATTR(set_cmd_drop, 0200, NULL, fcloop_set_cmd_drop);
> +static DEVICE_ATTR(set_marginal_rport, 0200, NULL, fcloop_set_marginal_rport);
Checkpatch is complaining about:
Consider renaming function(s) 'fcloop_set_marginal_rport' to
'set_marginal_rport_store'
I dont think deviating from the existing naming scheme to appease
checkpatch is a good idea though.
Thanks,
Jesse Taube
>
> static struct attribute *fcloop_dev_attrs[] = {
> &dev_attr_add_local_port.attr,
> @@ -1680,6 +1720,7 @@ static struct attribute *fcloop_dev_attrs[] = {
> &dev_attr_add_target_port.attr,
> &dev_attr_del_target_port.attr,
> &dev_attr_set_cmd_drop.attr,
> + &dev_attr_set_marginal_rport.attr,
> NULL
> };
>
> --
> 2.54.0
>
^ permalink raw reply [flat|nested] 30+ messages in thread
* [PATCH 13/13] docs: nvme-multipath: Add FC-NVMe marginal state
2026-08-12 18:12 [PATCH 00/13] nvme-fc: FPIN link integrity handling Jesse Taube
` (11 preceding siblings ...)
2026-08-12 18:12 ` [PATCH 12/13] nvme: fcloop: Add set_rport_marginal to sysfs Jesse Taube
@ 2026-08-12 18:13 ` Jesse Taube
2026-08-12 18:26 ` sashiko-bot
2026-08-12 18:46 ` Randy Dunlap
12 siblings, 2 replies; 30+ messages in thread
From: Jesse Taube @ 2026-08-12 18:13 UTC (permalink / raw)
To: linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Justin Tee, Naresh Gottumukkala,
Paul Ely, Chaitanya Kulkarni, James E.J. Bottomley,
Martin K. Petersen, Nilesh Javali, GR-QLogic-Storage-Upstream,
Hannes Reinecke, Jesse Taube, Gustavo A. R. Silva, John Meneghini,
Bryan Gurney, Chris Leech, Ewan D . Milne, shinichiro.kawasaki,
linux-doc, linux-kernel, linux-nvme, linux-block
Adds documentation for the new marginal state for
NVMe over Fibre Channel. Documents how to check and reset the state,
as well as a description of how it affects multipath policies.
Signed-off-by: Jesse Taube <jtaubepe@redhat.com>
---
Documentation/admin-guide/nvme-multipath.rst | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/Documentation/admin-guide/nvme-multipath.rst b/Documentation/admin-guide/nvme-multipath.rst
index 97ca1ccef459..8df4235d4d10 100644
--- a/Documentation/admin-guide/nvme-multipath.rst
+++ b/Documentation/admin-guide/nvme-multipath.rst
@@ -31,6 +31,23 @@ To set the desired policy (e.g., round-robin), use one of the following methods:
1. echo -n "round-robin" > /sys/module/nvme_core/parameters/iopolicy
2. or add the "nvme_core.iopolicy=round-robin" to cmdline.
+FC-NVMe
+----
+On NVMe over Fibre Channel there is an additional state called `marginal`
+All policies will treat this path as offline and will not use it unless
+all other paths are marginal or unavailable. In that case, the policy will
+select the best path based on the policy.
+
+To see the current path state, use one of the following methods:
+ 1. cat /sys/class/fc_host/host*/device/rport-*/fc_remote_ports/rport-*/port_state
+ 2. cat /sys/devices/virtual/nvme-subsystem/nvme-subsys*/nvme*/state
+To reset a path to online use:
+ 1. echo "Online" > /sys/class/fc_host/host*/device/rport-*/fc_remote_ports/rport-*/port_state
+
+The marginal state is caused by a
+FPIN-LI (Fabric Performance Impact Notification - Link Impact) event.
+FPIN-LI event counters can be seen with:
+ 1. cat /sys/class/fc_host/host*/statistics/fpin_li*
NUMA
----
--
2.54.0
^ permalink raw reply related [flat|nested] 30+ messages in thread* Re: [PATCH 13/13] docs: nvme-multipath: Add FC-NVMe marginal state
2026-08-12 18:13 ` [PATCH 13/13] docs: nvme-multipath: Add FC-NVMe marginal state Jesse Taube
@ 2026-08-12 18:26 ` sashiko-bot
2026-08-12 18:46 ` Randy Dunlap
1 sibling, 0 replies; 30+ messages in thread
From: sashiko-bot @ 2026-08-12 18:26 UTC (permalink / raw)
To: Jesse Taube; +Cc: linux-scsi
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] reStructuredText syntax errors and typographical omissions in the new documentation section will cause Sphinx build warnings and incorrect rendering.
--
--- Patch [13]: [PATCH 13/13] docs: nvme-multipath: Add FC-NVMe marginal state ---
commit f3dba11cc9add673447a799cef2fefc827b2c2d5
Author: Jesse Taube <jtaubepe@redhat.com>
docs: nvme-multipath: Add FC-NVMe marginal state
This commit adds documentation for a new marginal state in NVMe over Fibre
Channel. It explains how to check and reset this state, and describes its
impact on multipath policies.
> diff --git a/Documentation/admin-guide/nvme-multipath.rst b/Documentation/admin-guide/nvme-multipath.rst
> index 97ca1ccef459b..8df4235d4d104 100644
> --- a/Documentation/admin-guide/nvme-multipath.rst
> +++ b/Documentation/admin-guide/nvme-multipath.rst
[ ... ]
> @@ -31,6 +31,23 @@ To set the desired policy (e.g., round-robin), use one of the following methods:
> 1. echo -n "round-robin" > /sys/module/nvme_core/parameters/iopolicy
> 2. or add the "nvme_core.iopolicy=round-robin" to cmdline.
>
> +FC-NVMe
> +----
[Severity: Low]
This isn't a bug, but the underline is 4 characters long while the title is
7 characters. Could this cause a Sphinx warning about the title underline
being too short when building the kernel documentation?
> +On NVMe over Fibre Channel there is an additional state called `marginal`
[Severity: Low]
This isn't a bug, but it looks like this sentence is missing a terminal
period at the end of the line.
> +All policies will treat this path as offline and will not use it unless
> +all other paths are marginal or unavailable. In that case, the policy will
> +select the best path based on the policy.
> +
> +To see the current path state, use one of the following methods:
> + 1. cat /sys/class/fc_host/host*/device/rport-*/fc_remote_ports/rport-*/port_state
> + 2. cat /sys/devices/virtual/nvme-subsystem/nvme-subsys*/nvme*/state
> +To reset a path to online use:
[Severity: Low]
This isn't a bug, but reStructuredText usually expects a blank line to
terminate a list. Will omitting a blank line before "To reset a path to
online use:" cause formatting issues or incorrect list rendering in the
generated HTML documentation?
> + 1. echo "Online" > /sys/class/fc_host/host*/device/rport-*/fc_remote_ports/rport-*/port_state
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260812181300.3712426-1-jtaubepe@redhat.com?part=13
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 13/13] docs: nvme-multipath: Add FC-NVMe marginal state
2026-08-12 18:13 ` [PATCH 13/13] docs: nvme-multipath: Add FC-NVMe marginal state Jesse Taube
2026-08-12 18:26 ` sashiko-bot
@ 2026-08-12 18:46 ` Randy Dunlap
2026-08-12 18:50 ` Randy Dunlap
1 sibling, 1 reply; 30+ messages in thread
From: Randy Dunlap @ 2026-08-12 18:46 UTC (permalink / raw)
To: Jesse Taube, linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Justin Tee, Naresh Gottumukkala,
Paul Ely, Chaitanya Kulkarni, James E.J. Bottomley,
Martin K. Petersen, Nilesh Javali, GR-QLogic-Storage-Upstream,
Hannes Reinecke, Gustavo A. R. Silva, John Meneghini,
Bryan Gurney, Chris Leech, Ewan D . Milne, shinichiro.kawasaki,
linux-doc, linux-kernel, linux-nvme, linux-block
On 8/12/26 11:13 AM, Jesse Taube wrote:
> Adds documentation for the new marginal state for
> NVMe over Fibre Channel. Documents how to check and reset the state,
> as well as a description of how it affects multipath policies.
>
> Signed-off-by: Jesse Taube <jtaubepe@redhat.com>
> ---
> Documentation/admin-guide/nvme-multipath.rst | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
'make htmldocs' build warnings:
linux-next/Documentation/admin-guide/nvme-multipath.rst:35: WARNING: Title underline too short.
FC-NVMe
---- [docutils]
linux-next/Documentation/admin-guide/nvme-multipath.rst:50: ERROR: Unexpected indentation. [docutils]
> diff --git a/Documentation/admin-guide/nvme-multipath.rst b/Documentation/admin-guide/nvme-multipath.rst
> index 97ca1ccef459..8df4235d4d10 100644
> --- a/Documentation/admin-guide/nvme-multipath.rst
> +++ b/Documentation/admin-guide/nvme-multipath.rst
> @@ -31,6 +31,23 @@ To set the desired policy (e.g., round-robin), use one of the following methods:
> 1. echo -n "round-robin" > /sys/module/nvme_core/parameters/iopolicy
> 2. or add the "nvme_core.iopolicy=round-robin" to cmdline.
>
> +FC-NVMe
> +----
> +On NVMe over Fibre Channel there is an additional state called `marginal`
> +All policies will treat this path as offline and will not use it unless
> +all other paths are marginal or unavailable. In that case, the policy will
> +select the best path based on the policy.
> +
> +To see the current path state, use one of the following methods:
> + 1. cat /sys/class/fc_host/host*/device/rport-*/fc_remote_ports/rport-*/port_state
> + 2. cat /sys/devices/virtual/nvme-subsystem/nvme-subsys*/nvme*/state
> +To reset a path to online use:
> + 1. echo "Online" > /sys/class/fc_host/host*/device/rport-*/fc_remote_ports/rport-*/port_state
> +
> +The marginal state is caused by a
> +FPIN-LI (Fabric Performance Impact Notification - Link Impact) event.
> +FPIN-LI event counters can be seen with:
> + 1. cat /sys/class/fc_host/host*/statistics/fpin_li*
>
> NUMA
> ----
--
~Randy
^ permalink raw reply [flat|nested] 30+ messages in thread
* Re: [PATCH 13/13] docs: nvme-multipath: Add FC-NVMe marginal state
2026-08-12 18:46 ` Randy Dunlap
@ 2026-08-12 18:50 ` Randy Dunlap
0 siblings, 0 replies; 30+ messages in thread
From: Randy Dunlap @ 2026-08-12 18:50 UTC (permalink / raw)
To: Jesse Taube, linux-scsi
Cc: Jonathan Corbet, Shuah Khan, Keith Busch, Jens Axboe,
Christoph Hellwig, Sagi Grimberg, Justin Tee, Naresh Gottumukkala,
Paul Ely, Chaitanya Kulkarni, James E.J. Bottomley,
Martin K. Petersen, Nilesh Javali, GR-QLogic-Storage-Upstream,
Hannes Reinecke, Gustavo A. R. Silva, John Meneghini,
Bryan Gurney, Chris Leech, Ewan D . Milne, shinichiro.kawasaki,
linux-doc, linux-kernel, linux-nvme, linux-block
On 8/12/26 11:46 AM, Randy Dunlap wrote:
>
>
> On 8/12/26 11:13 AM, Jesse Taube wrote:
>> Adds documentation for the new marginal state for
>> NVMe over Fibre Channel. Documents how to check and reset the state,
>> as well as a description of how it affects multipath policies.
>>
>> Signed-off-by: Jesse Taube <jtaubepe@redhat.com>
>> ---
>> Documentation/admin-guide/nvme-multipath.rst | 17 +++++++++++++++++
>> 1 file changed, 17 insertions(+)
>>
>
> 'make htmldocs' build warnings:
>
> linux-next/Documentation/admin-guide/nvme-multipath.rst:35: WARNING: Title underline too short.
>
> FC-NVMe
> ---- [docutils]
> linux-next/Documentation/admin-guide/nvme-multipath.rst:50: ERROR: Unexpected indentation. [docutils]
Some fixes below.
>> diff --git a/Documentation/admin-guide/nvme-multipath.rst b/Documentation/admin-guide/nvme-multipath.rst
>> index 97ca1ccef459..8df4235d4d10 100644
>> --- a/Documentation/admin-guide/nvme-multipath.rst
>> +++ b/Documentation/admin-guide/nvme-multipath.rst
>> @@ -31,6 +31,23 @@ To set the desired policy (e.g., round-robin), use one of the following methods:
>> 1. echo -n "round-robin" > /sys/module/nvme_core/parameters/iopolicy
>> 2. or add the "nvme_core.iopolicy=round-robin" to cmdline.
>>
>> +FC-NVMe
>> +----
-------
>> +On NVMe over Fibre Channel there is an additional state called `marginal`
>> +All policies will treat this path as offline and will not use it unless
>> +all other paths are marginal or unavailable. In that case, the policy will
>> +select the best path based on the policy.
>> +
>> +To see the current path state, use one of the following methods:
>> + 1. cat /sys/class/fc_host/host*/device/rport-*/fc_remote_ports/rport-*/port_state
>> + 2. cat /sys/devices/virtual/nvme-subsystem/nvme-subsys*/nvme*/state
>> +To reset a path to online use:
>> + 1. echo "Online" > /sys/class/fc_host/host*/device/rport-*/fc_remote_ports/rport-*/port_state
>> +
>> +The marginal state is caused by a
>> +FPIN-LI (Fabric Performance Impact Notification - Link Impact) event.
>> +FPIN-LI event counters can be seen with:
>> + 1. cat /sys/class/fc_host/host*/statistics/fpin_li*
The marginal state is caused by a
FPIN-LI (Fabric Performance Impact Notification - Link Impact) event.
FPIN-LI event counters can be seen with::
cat /sys/class/fc_host/host*/statistics/fpin_li*
>> NUMA
>> ----
>
--
~Randy
^ permalink raw reply [flat|nested] 30+ messages in thread