On 9/22/2020 5:14 AM, Hannes Reinecke wrote: > Use an IDA to register port IDs to avoid duplicate port IDs. It > also will generate a unique port ID if none is specified. I'd prefer a little more description as I didn't pick up what this actually did from the comment.  Meaning: Add auto-address assignment if one isn't specified. Use an ida for allocation for uniqueness. Use user-supplied addresses as long as not in use and within range supported for ida. > Signed-off-by: Hannes Reinecke > --- > drivers/nvme/target/fcloop.c | 51 +++++++++++++++++++++++++++++++++++++------- > 1 file changed, 43 insertions(+), 8 deletions(-) > > diff --git a/drivers/nvme/target/fcloop.c b/drivers/nvme/target/fcloop.c > index 082aa4dee406..e56f323fa7d4 100644 > --- a/drivers/nvme/target/fcloop.c > +++ b/drivers/nvme/target/fcloop.c > @@ -12,6 +12,7 @@ > #include > #include > > +#define FCLOOP_MAX_AL_PA 0xEF This is odd - no reason to bring in ALPA's and arb loop.   Please rename it to state it's the max address value allowed.  I'd prefer if this was a 24 bit value, but that won't matter much and 255 is fine. > > enum { > NVMF_OPT_ERR = 0, > @@ -63,6 +64,9 @@ fcloop_parse_options(struct fcloop_ctrl_options *opts, > int token, ret = 0; > u64 token64; > > + /* Default setting */ > + opts->fcaddr = 1; > + > options = o = kstrdup(buf, GFP_KERNEL); > if (!options) > return -ENOMEM; > @@ -102,6 +106,10 @@ fcloop_parse_options(struct fcloop_ctrl_options *opts, > ret = -EINVAL; > goto out_free_options; > } > + if (!token || token > FCLOOP_MAX_AL_PA) { > + ret = -EINVAL; > + goto out_free_options; > + } > opts->fcaddr = token; > break; > case NVMF_OPT_LPWWNN: > @@ -203,11 +211,13 @@ fcloop_parse_nm_options(struct device *dev, u64 *nname, u64 *pname, > static DEFINE_SPINLOCK(fcloop_lock); > static LIST_HEAD(fcloop_lports); > static LIST_HEAD(fcloop_nports); > +static DEFINE_IDA(fcloop_portid_ida); > > struct fcloop_lport { > struct nvme_fc_local_port *localport; > struct list_head lport_list; > struct completion unreg_done; > + u32 port_id; > }; > > struct fcloop_lport_priv { > @@ -949,6 +959,8 @@ fcloop_nport_free(struct kref *ref) > list_del(&nport->nport_list); > spin_unlock_irqrestore(&fcloop_lock, flags); > > + ida_free(&fcloop_portid_ida, nport->port_id); > + > kfree(nport); > } > > @@ -1047,7 +1059,7 @@ fcloop_create_local_port(struct device *dev, struct device_attribute *attr, > struct fcloop_lport *lport; > struct fcloop_lport_priv *lport_priv; > unsigned long flags; > - int ret = -ENOMEM; > + int ret = -ENOMEM, port_id; > > lport = kzalloc(sizeof(*lport), GFP_KERNEL); > if (!lport) > @@ -1067,11 +1079,22 @@ fcloop_create_local_port(struct device *dev, struct device_attribute *attr, > goto out_free_opts; > } > > + port_id = ida_alloc_range(&fcloop_portid_ida, > + opts->fcaddr, FCLOOP_MAX_AL_PA, GFP_KERNEL); > + if (port_id < 0) { > + ret = port_id; > + goto out_free_opts; > + } > + if ((opts->mask & NVMF_OPT_FCADDR) && opts->fcaddr != port_id) { > + ret = -EINVAL; > + goto out_free_ida; > + } > + > memset(&pinfo, 0, sizeof(pinfo)); > pinfo.node_name = opts->wwnn; > pinfo.port_name = opts->wwpn; > pinfo.port_role = opts->roles; > - pinfo.port_id = opts->fcaddr; > + pinfo.port_id = port_id; > > ret = nvme_fc_register_localport(&pinfo, &fctemplate, NULL, &localport); > if (!ret) { > @@ -1086,7 +1109,9 @@ fcloop_create_local_port(struct device *dev, struct device_attribute *attr, > list_add_tail(&lport->lport_list, &fcloop_lports); > spin_unlock_irqrestore(&fcloop_lock, flags); > } > - > +out_free_ida: > + if (ret) > + ida_free(&fcloop_portid_ida, port_id); > out_free_opts: > kfree(opts); > out_free_lport: > @@ -1115,6 +1140,8 @@ __wait_localport_unreg(struct fcloop_lport *lport) > > wait_for_completion(&lport->unreg_done); > > + ida_free(&fcloop_portid_ida, lport->port_id); > + > kfree(lport); > > return ret; > @@ -1162,7 +1189,7 @@ fcloop_alloc_nport(const char *buf, size_t count, bool remoteport) > struct fcloop_ctrl_options *opts; > unsigned long flags; > u32 opts_mask = (remoteport) ? RPORT_OPTS : TGTPORT_OPTS; > - int ret; > + int ret, port_id; > > opts = kzalloc(sizeof(*opts), GFP_KERNEL); > if (!opts) > @@ -1182,13 +1209,21 @@ fcloop_alloc_nport(const char *buf, size_t count, bool remoteport) > if (!newnport) > goto out_free_opts; > > + port_id = ida_alloc_range(&fcloop_portid_ida, > + opts->fcaddr, FCLOOP_MAX_AL_PA, GFP_KERNEL); > + if (port_id < 0) > + goto out_free_newnport; > + > + if ((opts->mask & NVMF_OPT_FCADDR) && opts->fcaddr != port_id) > + goto out_free_ida; > + > INIT_LIST_HEAD(&newnport->nport_list); > newnport->node_name = opts->wwnn; > newnport->port_name = opts->wwpn; > + newnport->port_id = port_id; > if (opts->mask & NVMF_OPT_ROLES) > newnport->port_role = opts->roles; > - if (opts->mask & NVMF_OPT_FCADDR) > - newnport->port_id = opts->fcaddr; > + > kref_init(&newnport->ref); > > spin_lock_irqsave(&fcloop_lock, flags); > @@ -1226,8 +1261,6 @@ fcloop_alloc_nport(const char *buf, size_t count, bool remoteport) > nport->lport = lport; > if (opts->mask & NVMF_OPT_ROLES) > nport->port_role = opts->roles; > - if (opts->mask & NVMF_OPT_FCADDR) > - nport->port_id = opts->fcaddr; > goto out_free_newnport; > } > } > @@ -1241,6 +1274,8 @@ fcloop_alloc_nport(const char *buf, size_t count, bool remoteport) > > out_invalid_opts: > spin_unlock_irqrestore(&fcloop_lock, flags); > +out_free_ida: > + ida_free(&fcloop_portid_ida, port_id); > out_free_newnport: > kfree(newnport); > out_free_opts: These are minor nits - I'm good with the patch Reviewed-by: James Smart -- james