| Message ID | 20210519111340.20613-4-smalin@marvell.com |
|---|---|
| State | New |
| Headers | show |
| Series | NVMeTCP Offload ULP and QEDN Device Driver | expand |
On 5/19/21 6:13 AM, Shai Malin wrote: > From: Dean Balandin <dbalandin@marvell.com> > > As part of create_ctrl(), it scans the registered devices and calls > the claim_dev op on each of them, to find the first devices that matches > the connection params. Once the correct devices is found (claim_dev > returns true), we raise the refcnt of that device and return that device > as the device to be used for ctrl currently being created. > > Acked-by: Igor Russkikh <irusskikh@marvell.com> > Signed-off-by: Dean Balandin <dbalandin@marvell.com> > Signed-off-by: Prabhakar Kushwaha <pkushwaha@marvell.com> > Signed-off-by: Omkar Kulkarni <okulkarni@marvell.com> > Signed-off-by: Michal Kalderon <mkalderon@marvell.com> > Signed-off-by: Ariel Elior <aelior@marvell.com> > Signed-off-by: Shai Malin <smalin@marvell.com> > --- > drivers/nvme/host/tcp-offload.c | 94 +++++++++++++++++++++++++++++++++ > 1 file changed, 94 insertions(+) > > diff --git a/drivers/nvme/host/tcp-offload.c b/drivers/nvme/host/tcp-offload.c > index 711232eba339..aa7cc239abf2 100644 > --- a/drivers/nvme/host/tcp-offload.c > +++ b/drivers/nvme/host/tcp-offload.c > @@ -13,6 +13,11 @@ > static LIST_HEAD(nvme_tcp_ofld_devices); > static DECLARE_RWSEM(nvme_tcp_ofld_devices_rwsem); > > +static inline struct nvme_tcp_ofld_ctrl *to_tcp_ofld_ctrl(struct nvme_ctrl *nctrl) > +{ > + return container_of(nctrl, struct nvme_tcp_ofld_ctrl, nctrl); > +} > + > /** > * nvme_tcp_ofld_register_dev() - NVMeTCP Offload Library registration > * function. > @@ -98,6 +103,94 @@ void nvme_tcp_ofld_req_done(struct nvme_tcp_ofld_req *req, > /* Placeholder - complete request with/without error */ > } > > +struct nvme_tcp_ofld_dev * > +nvme_tcp_ofld_lookup_dev(struct nvme_tcp_ofld_ctrl *ctrl) > +{ > + struct nvme_tcp_ofld_dev *dev; > + > + down_read(&nvme_tcp_ofld_devices_rwsem); > + list_for_each_entry(dev, &nvme_tcp_ofld_devices, entry) { > + if (dev->ops->claim_dev(dev, &ctrl->conn_params)) { > + /* Increase driver refcnt */ > + if (!try_module_get(dev->ops->module)) { > + pr_err("try_module_get failed\n"); > + dev = NULL; > + } > + > + goto out; > + } > + } > + > + dev = NULL; > +out: > + up_read(&nvme_tcp_ofld_devices_rwsem); > + > + return dev; > +} > + > +static int nvme_tcp_ofld_setup_ctrl(struct nvme_ctrl *nctrl, bool new) > +{ > + /* Placeholder - validates inputs and creates admin and IO queues */ > + > + return 0; > +} > + > +static struct nvme_ctrl * > +nvme_tcp_ofld_create_ctrl(struct device *ndev, struct nvmf_ctrl_options *opts) > +{ > + struct nvme_tcp_ofld_ctrl *ctrl; > + struct nvme_tcp_ofld_dev *dev; > + struct nvme_ctrl *nctrl; > + int rc = 0; > + > + ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL); > + if (!ctrl) > + return ERR_PTR(-ENOMEM); > + > + nctrl = &ctrl->nctrl; > + > + /* Init nvme_tcp_ofld_ctrl and nvme_ctrl params based on received opts */ > + > + /* Find device that can reach the dest addr */ > + dev = nvme_tcp_ofld_lookup_dev(ctrl); > + if (!dev) { > + pr_info("no device found for addr %s:%s.\n", > + opts->traddr, opts->trsvcid); > + rc = -EINVAL; > + goto out_free_ctrl; > + } > + > + ctrl->dev = dev; > + > + if (ctrl->dev->ops->max_hw_sectors) > + nctrl->max_hw_sectors = ctrl->dev->ops->max_hw_sectors; > + if (ctrl->dev->ops->max_segments) > + nctrl->max_segments = ctrl->dev->ops->max_segments; > + > + /* Init queues */ > + > + /* Call nvme_init_ctrl */ > + > + rc = ctrl->dev->ops->setup_ctrl(ctrl, true); > + if (rc) > + goto out_module_put; > + > + rc = nvme_tcp_ofld_setup_ctrl(nctrl, true); > + if (rc) > + goto out_uninit_ctrl; > + > + return nctrl; > + > +out_uninit_ctrl: > + ctrl->dev->ops->release_ctrl(ctrl); > +out_module_put: > + module_put(dev->ops->module); > +out_free_ctrl: > + kfree(ctrl); > + > + return ERR_PTR(rc); > +} > + > static struct nvmf_transport_ops nvme_tcp_ofld_transport = { > .name = "tcp_offload", > .module = THIS_MODULE, > @@ -107,6 +200,7 @@ static struct nvmf_transport_ops nvme_tcp_ofld_transport = { > NVMF_OPT_RECONNECT_DELAY | NVMF_OPT_HDR_DIGEST | > NVMF_OPT_DATA_DIGEST | NVMF_OPT_NR_POLL_QUEUES | > NVMF_OPT_TOS, > + .create_ctrl = nvme_tcp_ofld_create_ctrl, > }; > > static int __init nvme_tcp_ofld_init_module(void) > Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com> -- Himanshu Madhani Oracle Linux Engineering
On 5/19/21 4:13 AM, Shai Malin wrote: > From: Dean Balandin <dbalandin@marvell.com> > > As part of create_ctrl(), it scans the registered devices and calls > the claim_dev op on each of them, to find the first devices that matches > the connection params. Once the correct devices is found (claim_dev > returns true), we raise the refcnt of that device and return that device > as the device to be used for ctrl currently being created. > > Acked-by: Igor Russkikh <irusskikh@marvell.com> > Signed-off-by: Dean Balandin <dbalandin@marvell.com> > Signed-off-by: Prabhakar Kushwaha <pkushwaha@marvell.com> > Signed-off-by: Omkar Kulkarni <okulkarni@marvell.com> > Signed-off-by: Michal Kalderon <mkalderon@marvell.com> > Signed-off-by: Ariel Elior <aelior@marvell.com> > Signed-off-by: Shai Malin <smalin@marvell.com> > --- > drivers/nvme/host/tcp-offload.c | 94 +++++++++++++++++++++++++++++++++ > 1 file changed, 94 insertions(+) > > diff --git a/drivers/nvme/host/tcp-offload.c b/drivers/nvme/host/tcp-offload.c > index 711232eba339..aa7cc239abf2 100644 > --- a/drivers/nvme/host/tcp-offload.c > +++ b/drivers/nvme/host/tcp-offload.c > @@ -13,6 +13,11 @@ > static LIST_HEAD(nvme_tcp_ofld_devices); > static DECLARE_RWSEM(nvme_tcp_ofld_devices_rwsem); > > +static inline struct nvme_tcp_ofld_ctrl *to_tcp_ofld_ctrl(struct nvme_ctrl *nctrl) > +{ > + return container_of(nctrl, struct nvme_tcp_ofld_ctrl, nctrl); > +} > + > /** > * nvme_tcp_ofld_register_dev() - NVMeTCP Offload Library registration > * function. > @@ -98,6 +103,94 @@ void nvme_tcp_ofld_req_done(struct nvme_tcp_ofld_req *req, > /* Placeholder - complete request with/without error */ > } > > +struct nvme_tcp_ofld_dev * > +nvme_tcp_ofld_lookup_dev(struct nvme_tcp_ofld_ctrl *ctrl) > +{ > + struct nvme_tcp_ofld_dev *dev; > + > + down_read(&nvme_tcp_ofld_devices_rwsem); > + list_for_each_entry(dev, &nvme_tcp_ofld_devices, entry) { > + if (dev->ops->claim_dev(dev, &ctrl->conn_params)) { > + /* Increase driver refcnt */ > + if (!try_module_get(dev->ops->module)) { This means that every controller will take a long-lived reference on the module? Why? > + pr_err("try_module_get failed\n"); > + dev = NULL; > + } > + > + goto out; > + } > + } > + > + dev = NULL; > +out: > + up_read(&nvme_tcp_ofld_devices_rwsem); > + > + return dev; > +} > + > +static int nvme_tcp_ofld_setup_ctrl(struct nvme_ctrl *nctrl, bool new) > +{ > + /* Placeholder - validates inputs and creates admin and IO queues */ > + > + return 0; > +} > + > +static struct nvme_ctrl * > +nvme_tcp_ofld_create_ctrl(struct device *ndev, struct nvmf_ctrl_options *opts) > +{ > + struct nvme_tcp_ofld_ctrl *ctrl; > + struct nvme_tcp_ofld_dev *dev; > + struct nvme_ctrl *nctrl; > + int rc = 0; > + > + ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL); > + if (!ctrl) > + return ERR_PTR(-ENOMEM); > + > + nctrl = &ctrl->nctrl; > + > + /* Init nvme_tcp_ofld_ctrl and nvme_ctrl params based on received opts */ > + > + /* Find device that can reach the dest addr */ > + dev = nvme_tcp_ofld_lookup_dev(ctrl); > + if (!dev) { > + pr_info("no device found for addr %s:%s.\n", > + opts->traddr, opts->trsvcid); > + rc = -EINVAL; > + goto out_free_ctrl; > + } > + > + ctrl->dev = dev; > + > + if (ctrl->dev->ops->max_hw_sectors) > + nctrl->max_hw_sectors = ctrl->dev->ops->max_hw_sectors; > + if (ctrl->dev->ops->max_segments) > + nctrl->max_segments = ctrl->dev->ops->max_segments; > + > + /* Init queues */ > + > + /* Call nvme_init_ctrl */ > + > + rc = ctrl->dev->ops->setup_ctrl(ctrl, true); > + if (rc) > + goto out_module_put; goto module_put without an explicit module_get is confusing. > + > + rc = nvme_tcp_ofld_setup_ctrl(nctrl, true); > + if (rc) > + goto out_uninit_ctrl; ops->setup_ctrl and then call to nvme_tcp_ofld_setup_ctrl? Looks weird, why are these separated? > + > + return nctrl; > + > +out_uninit_ctrl: > + ctrl->dev->ops->release_ctrl(ctrl); > +out_module_put: > + module_put(dev->ops->module); > +out_free_ctrl: > + kfree(ctrl); > + > + return ERR_PTR(rc); > +} > + > static struct nvmf_transport_ops nvme_tcp_ofld_transport = { > .name = "tcp_offload", > .module = THIS_MODULE, > @@ -107,6 +200,7 @@ static struct nvmf_transport_ops nvme_tcp_ofld_transport = { > NVMF_OPT_RECONNECT_DELAY | NVMF_OPT_HDR_DIGEST | > NVMF_OPT_DATA_DIGEST | NVMF_OPT_NR_POLL_QUEUES | > NVMF_OPT_TOS, > + .create_ctrl = nvme_tcp_ofld_create_ctrl, > }; > > static int __init nvme_tcp_ofld_init_module(void) >
On 5/22/21 1:22 AM, Sagi Grimberg wrote: > On 5/19/21 4:13 AM, Shai Malin wrote: > > From: Dean Balandin <dbalandin@marvell.com> > > > > As part of create_ctrl(), it scans the registered devices and calls > > the claim_dev op on each of them, to find the first devices that matches > > the connection params. Once the correct devices is found (claim_dev > > returns true), we raise the refcnt of that device and return that device > > as the device to be used for ctrl currently being created. > > > > Acked-by: Igor Russkikh <irusskikh@marvell.com> > > Signed-off-by: Dean Balandin <dbalandin@marvell.com> > > Signed-off-by: Prabhakar Kushwaha <pkushwaha@marvell.com> > > Signed-off-by: Omkar Kulkarni <okulkarni@marvell.com> > > Signed-off-by: Michal Kalderon <mkalderon@marvell.com> > > Signed-off-by: Ariel Elior <aelior@marvell.com> > > Signed-off-by: Shai Malin <smalin@marvell.com> > > --- > > drivers/nvme/host/tcp-offload.c | 94 +++++++++++++++++++++++++++++++++ > > 1 file changed, 94 insertions(+) > > > > diff --git a/drivers/nvme/host/tcp-offload.c b/drivers/nvme/host/tcp-offload.c > > index 711232eba339..aa7cc239abf2 100644 > > --- a/drivers/nvme/host/tcp-offload.c > > +++ b/drivers/nvme/host/tcp-offload.c > > @@ -13,6 +13,11 @@ > > static LIST_HEAD(nvme_tcp_ofld_devices); > > static DECLARE_RWSEM(nvme_tcp_ofld_devices_rwsem); > > > > +static inline struct nvme_tcp_ofld_ctrl *to_tcp_ofld_ctrl(struct nvme_ctrl *nctrl) > > +{ > > + return container_of(nctrl, struct nvme_tcp_ofld_ctrl, nctrl); > > +} > > + > > /** > > * nvme_tcp_ofld_register_dev() - NVMeTCP Offload Library registration > > * function. > > @@ -98,6 +103,94 @@ void nvme_tcp_ofld_req_done(struct nvme_tcp_ofld_req *req, > > /* Placeholder - complete request with/without error */ > > } > > > > +struct nvme_tcp_ofld_dev * > > +nvme_tcp_ofld_lookup_dev(struct nvme_tcp_ofld_ctrl *ctrl) > > +{ > > + struct nvme_tcp_ofld_dev *dev; > > + > > + down_read(&nvme_tcp_ofld_devices_rwsem); > > + list_for_each_entry(dev, &nvme_tcp_ofld_devices, entry) { > > + if (dev->ops->claim_dev(dev, &ctrl->conn_params)) { > > + /* Increase driver refcnt */ > > + if (!try_module_get(dev->ops->module)) { > > This means that every controller will take a long-lived reference on the > module? Why? This is in order to create a per controller dependency between the nvme-tcp-offload and the vendor driver which is currently used. We believe that the vendor driver which offloads a controller should exist as long as the controller exists. > > > + pr_err("try_module_get failed\n"); > > + dev = NULL; > > + } > > + > > + goto out; > > + } > > + } > > + > > + dev = NULL; > > +out: > > + up_read(&nvme_tcp_ofld_devices_rwsem); > > + > > + return dev; > > +} > > + > > +static int nvme_tcp_ofld_setup_ctrl(struct nvme_ctrl *nctrl, bool new) > > +{ > > + /* Placeholder - validates inputs and creates admin and IO queues */ > > + > > + return 0; > > +} > > + > > +static struct nvme_ctrl * > > +nvme_tcp_ofld_create_ctrl(struct device *ndev, struct nvmf_ctrl_options *opts) > > +{ > > + struct nvme_tcp_ofld_ctrl *ctrl; > > + struct nvme_tcp_ofld_dev *dev; > > + struct nvme_ctrl *nctrl; > > + int rc = 0; > > + > > + ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL); > > + if (!ctrl) > > + return ERR_PTR(-ENOMEM); > > + > > + nctrl = &ctrl->nctrl; > > + > > + /* Init nvme_tcp_ofld_ctrl and nvme_ctrl params based on received opts */ > > + > > + /* Find device that can reach the dest addr */ > > + dev = nvme_tcp_ofld_lookup_dev(ctrl); > > + if (!dev) { > > + pr_info("no device found for addr %s:%s.\n", > > + opts->traddr, opts->trsvcid); > > + rc = -EINVAL; > > + goto out_free_ctrl; > > + } > > + > > + ctrl->dev = dev; > > + > > + if (ctrl->dev->ops->max_hw_sectors) > > + nctrl->max_hw_sectors = ctrl->dev->ops->max_hw_sectors; > > + if (ctrl->dev->ops->max_segments) > > + nctrl->max_segments = ctrl->dev->ops->max_segments; > > + > > + /* Init queues */ > > + > > + /* Call nvme_init_ctrl */ > > + > > + rc = ctrl->dev->ops->setup_ctrl(ctrl, true); > > + if (rc) > > + goto out_module_put; > > goto module_put without an explicit module_get is confusing. We will modify the function to use explicit module_get so it will be clear. > > > + > > + rc = nvme_tcp_ofld_setup_ctrl(nctrl, true); > > + if (rc) > > + goto out_uninit_ctrl; > > ops->setup_ctrl and then call to nvme_tcp_ofld_setup_ctrl? > Looks weird, why are these separated? We will move the vendor specific setup_ctrl() call into nvme_tcp_ofld_setup_ctrl(). > > > + > > + return nctrl; > > + > > +out_uninit_ctrl: > > + ctrl->dev->ops->release_ctrl(ctrl); > > +out_module_put: > > + module_put(dev->ops->module); > > +out_free_ctrl: > > + kfree(ctrl); > > + > > + return ERR_PTR(rc); > > +} > > + > > static struct nvmf_transport_ops nvme_tcp_ofld_transport = { > > .name = "tcp_offload", > > .module = THIS_MODULE, > > @@ -107,6 +200,7 @@ static struct nvmf_transport_ops nvme_tcp_ofld_transport = { > > NVMF_OPT_RECONNECT_DELAY | NVMF_OPT_HDR_DIGEST | > > NVMF_OPT_DATA_DIGEST | NVMF_OPT_NR_POLL_QUEUES | > > NVMF_OPT_TOS, > > + .create_ctrl = nvme_tcp_ofld_create_ctrl, > > }; > > > > static int __init nvme_tcp_ofld_init_module(void) > >
diff --git a/drivers/nvme/host/tcp-offload.c b/drivers/nvme/host/tcp-offload.c index 711232eba339..aa7cc239abf2 100644 --- a/drivers/nvme/host/tcp-offload.c +++ b/drivers/nvme/host/tcp-offload.c @@ -13,6 +13,11 @@ static LIST_HEAD(nvme_tcp_ofld_devices); static DECLARE_RWSEM(nvme_tcp_ofld_devices_rwsem); +static inline struct nvme_tcp_ofld_ctrl *to_tcp_ofld_ctrl(struct nvme_ctrl *nctrl) +{ + return container_of(nctrl, struct nvme_tcp_ofld_ctrl, nctrl); +} + /** * nvme_tcp_ofld_register_dev() - NVMeTCP Offload Library registration * function. @@ -98,6 +103,94 @@ void nvme_tcp_ofld_req_done(struct nvme_tcp_ofld_req *req, /* Placeholder - complete request with/without error */ } +struct nvme_tcp_ofld_dev * +nvme_tcp_ofld_lookup_dev(struct nvme_tcp_ofld_ctrl *ctrl) +{ + struct nvme_tcp_ofld_dev *dev; + + down_read(&nvme_tcp_ofld_devices_rwsem); + list_for_each_entry(dev, &nvme_tcp_ofld_devices, entry) { + if (dev->ops->claim_dev(dev, &ctrl->conn_params)) { + /* Increase driver refcnt */ + if (!try_module_get(dev->ops->module)) { + pr_err("try_module_get failed\n"); + dev = NULL; + } + + goto out; + } + } + + dev = NULL; +out: + up_read(&nvme_tcp_ofld_devices_rwsem); + + return dev; +} + +static int nvme_tcp_ofld_setup_ctrl(struct nvme_ctrl *nctrl, bool new) +{ + /* Placeholder - validates inputs and creates admin and IO queues */ + + return 0; +} + +static struct nvme_ctrl * +nvme_tcp_ofld_create_ctrl(struct device *ndev, struct nvmf_ctrl_options *opts) +{ + struct nvme_tcp_ofld_ctrl *ctrl; + struct nvme_tcp_ofld_dev *dev; + struct nvme_ctrl *nctrl; + int rc = 0; + + ctrl = kzalloc(sizeof(*ctrl), GFP_KERNEL); + if (!ctrl) + return ERR_PTR(-ENOMEM); + + nctrl = &ctrl->nctrl; + + /* Init nvme_tcp_ofld_ctrl and nvme_ctrl params based on received opts */ + + /* Find device that can reach the dest addr */ + dev = nvme_tcp_ofld_lookup_dev(ctrl); + if (!dev) { + pr_info("no device found for addr %s:%s.\n", + opts->traddr, opts->trsvcid); + rc = -EINVAL; + goto out_free_ctrl; + } + + ctrl->dev = dev; + + if (ctrl->dev->ops->max_hw_sectors) + nctrl->max_hw_sectors = ctrl->dev->ops->max_hw_sectors; + if (ctrl->dev->ops->max_segments) + nctrl->max_segments = ctrl->dev->ops->max_segments; + + /* Init queues */ + + /* Call nvme_init_ctrl */ + + rc = ctrl->dev->ops->setup_ctrl(ctrl, true); + if (rc) + goto out_module_put; + + rc = nvme_tcp_ofld_setup_ctrl(nctrl, true); + if (rc) + goto out_uninit_ctrl; + + return nctrl; + +out_uninit_ctrl: + ctrl->dev->ops->release_ctrl(ctrl); +out_module_put: + module_put(dev->ops->module); +out_free_ctrl: + kfree(ctrl); + + return ERR_PTR(rc); +} + static struct nvmf_transport_ops nvme_tcp_ofld_transport = { .name = "tcp_offload", .module = THIS_MODULE, @@ -107,6 +200,7 @@ static struct nvmf_transport_ops nvme_tcp_ofld_transport = { NVMF_OPT_RECONNECT_DELAY | NVMF_OPT_HDR_DIGEST | NVMF_OPT_DATA_DIGEST | NVMF_OPT_NR_POLL_QUEUES | NVMF_OPT_TOS, + .create_ctrl = nvme_tcp_ofld_create_ctrl, }; static int __init nvme_tcp_ofld_init_module(void)