Re: [PATCH 12/13] nvme: fcloop: Add set_rport_marginal to sysfs

[Jesse Taube]

On Wed, Aug 12, 2026 at 2:14 PM Jesse Taube <jtaubepe@xxxxxxxxxx> 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@xxxxxxxxxx>
> Signed-off-by: Jesse Taube <jtaubepe@xxxxxxxxxx>
> 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
>