* [PATCH] nvme: define struct for __nvme_submit_sync_cmd()
@ 2019-10-01 23:13 Chaitanya Kulkarni
2019-10-02 5:47 ` Javier González
` (2 more replies)
0 siblings, 3 replies; 8+ messages in thread
From: Chaitanya Kulkarni @ 2019-10-01 23:13 UTC (permalink / raw)
To: linux-nvme; +Cc: Chaitanya Kulkarni
Over the period of time __nvme_submit_sync_cmd() function has grown to
accept large number of paratements. The function __nvme_submit_sync_cmd()
now takes 10 parameters. This patch consolidates all the parameters into
one defined structure.
This makes calls to the same function easy to read and improves overall
code readability.
Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
---
Hi,
I've compile tested this patch. Once we agree on this change
I'll send out a tested version.
Regards,
Chaitanya
---
drivers/nvme/host/core.c | 78 +++++++++++++++++++++---------
drivers/nvme/host/fabrics.c | 96 ++++++++++++++++++++++++++++---------
drivers/nvme/host/nvme.h | 18 +++++--
3 files changed, 142 insertions(+), 50 deletions(-)
diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e35615940365..9be51df8fb2f 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -769,32 +769,30 @@ static void nvme_execute_rq_polled(struct request_queue *q,
* Returns 0 on success. If the result is negative, it's a Linux error code;
* if the result is positive, it's an NVM Express status code
*/
-int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
- union nvme_result *result, void *buffer, unsigned bufflen,
- unsigned timeout, int qid, int at_head,
- blk_mq_req_flags_t flags, bool poll)
+int __nvme_submit_sync_cmd(struct nvme_submit_sync_data *d)
{
struct request *req;
int ret;
- req = nvme_alloc_request(q, cmd, flags, qid);
+ req = nvme_alloc_request(d->q, d->cmd, d->flags, d->qid);
if (IS_ERR(req))
return PTR_ERR(req);
- req->timeout = timeout ? timeout : ADMIN_TIMEOUT;
+ req->timeout = d->timeout ? d->timeout : ADMIN_TIMEOUT;
- if (buffer && bufflen) {
- ret = blk_rq_map_kern(q, req, buffer, bufflen, GFP_KERNEL);
+ if (d->buffer && d->bufflen) {
+ ret = blk_rq_map_kern(d->q, req, d->buffer, d->bufflen,
+ GFP_KERNEL);
if (ret)
goto out;
}
- if (poll)
- nvme_execute_rq_polled(req->q, NULL, req, at_head);
+ if (d->poll)
+ nvme_execute_rq_polled(req->q, NULL, req, d->at_head);
else
- blk_execute_rq(req->q, NULL, req, at_head);
- if (result)
- *result = nvme_req(req)->result;
+ blk_execute_rq(req->q, NULL, req, d->at_head);
+ if (d->result)
+ *(d->result) = nvme_req(req)->result;
if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
ret = -EINTR;
else
@@ -808,8 +806,20 @@ EXPORT_SYMBOL_GPL(__nvme_submit_sync_cmd);
int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
void *buffer, unsigned bufflen)
{
- return __nvme_submit_sync_cmd(q, cmd, NULL, buffer, bufflen, 0,
- NVME_QID_ANY, 0, 0, false);
+ struct nvme_submit_sync_data d = {
+ .q = q,
+ .cmd = cmd,
+ .result = NULL,
+ .buffer = buffer,
+ .bufflen = bufflen,
+ .timeout= 0,
+ .qid = NVME_QID_ANY,
+ .at_head = 0,
+ .flags = 0,
+ .poll = false
+ };
+
+ return __nvme_submit_sync_cmd(&d);
}
EXPORT_SYMBOL_GPL(nvme_submit_sync_cmd);
@@ -1114,19 +1124,30 @@ static int nvme_identify_ns(struct nvme_ctrl *ctrl,
}
static int nvme_features(struct nvme_ctrl *dev, u8 op, unsigned int fid,
- unsigned int dword11, void *buffer, size_t buflen, u32 *result)
+ unsigned int dword11, void *buffer, size_t buflen,
+ u32 *result)
{
- struct nvme_command c;
+ struct nvme_command c = {};
union nvme_result res;
int ret;
+ struct nvme_submit_sync_data d = {
+ .q = dev->admin_q,
+ .cmd = &c,
+ .result = &res,
+ .buffer = buffer,
+ .bufflen = buflen,
+ .timeout = 0,
+ .qid = NVME_QID_ANY,
+ .at_head = 0,
+ .flags = 0,
+ .poll = false,
+ };
- memset(&c, 0, sizeof(c));
c.features.opcode = op;
c.features.fid = cpu_to_le32(fid);
c.features.dword11 = cpu_to_le32(dword11);
- ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &res,
- buffer, buflen, 0, NVME_QID_ANY, 0, 0, false);
+ ret = __nvme_submit_sync_cmd(&d);
if (ret >= 0 && result)
*result = le32_to_cpu(res.u32);
return ret;
@@ -1954,10 +1975,22 @@ static const struct pr_ops nvme_pr_ops = {
#ifdef CONFIG_BLK_SED_OPAL
int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
- bool send)
+ bool send)
{
struct nvme_ctrl *ctrl = data;
struct nvme_command cmd;
+ struct nvme_submit_sync_data d = {
+ .q = dev->admin_q,
+ .cmd = &cmd,
+ .result = NULL,
+ .buffer = buffer,
+ .bufflen = len,
+ .timeout = ADMIN_TIMEOUT,
+ .qid = NVME_QID_ANY,
+ .at_head = 1;
+ .flags = 0;
+ .poll = false;
+ };
memset(&cmd, 0, sizeof(cmd));
if (send)
@@ -1968,8 +2001,7 @@ int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
cmd.common.cdw10 = cpu_to_le32(((u32)secp) << 24 | ((u32)spsp) << 8);
cmd.common.cdw11 = cpu_to_le32(len);
- return __nvme_submit_sync_cmd(ctrl->admin_q, &cmd, NULL, buffer, len,
- ADMIN_TIMEOUT, NVME_QID_ANY, 1, 0, false);
+ return __nvme_submit_sync_cmd(&d);
}
EXPORT_SYMBOL_GPL(nvme_sec_submit);
#endif /* CONFIG_BLK_SED_OPAL */
diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 74b8818ac9a1..c5a8aec5f046 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -144,14 +144,25 @@ int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val)
struct nvme_command cmd;
union nvme_result res;
int ret;
+ struct nvme_submit_sync_data d = {
+ .q = ctrl->fabrics_q,
+ .cmd = &cmd,
+ .result = &res,
+ .buffer = NULL,
+ .bufflen = 0,
+ .timeout = 0,
+ .qid = NVME_QID_ANY,
+ .at_head = 0,
+ .flags = 0,
+ .poll = false
+ };
memset(&cmd, 0, sizeof(cmd));
cmd.prop_get.opcode = nvme_fabrics_command;
cmd.prop_get.fctype = nvme_fabrics_type_property_get;
cmd.prop_get.offset = cpu_to_le32(off);
- ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0, 0,
- NVME_QID_ANY, 0, 0, false);
+ ret = __nvme_submit_sync_cmd(&d);
if (ret >= 0)
*val = le64_to_cpu(res.u64);
@@ -190,6 +201,18 @@ int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val)
struct nvme_command cmd;
union nvme_result res;
int ret;
+ struct nvme_submit_sync_data d = {
+ .q = ctrl->fabrics_q,
+ .cmd = &cmd,
+ .result = &res,
+ .buffer = NULL,
+ .bufflen = 0,
+ .timeout = 0,
+ .qid = NVME_QID_ANY,
+ .at_head = 0,
+ .flags = 0,
+ .poll = false
+ };
memset(&cmd, 0, sizeof(cmd));
cmd.prop_get.opcode = nvme_fabrics_command;
@@ -197,9 +220,7 @@ int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val)
cmd.prop_get.attrib = 1;
cmd.prop_get.offset = cpu_to_le32(off);
- ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0, 0,
- NVME_QID_ANY, 0, 0, false);
-
+ ret = __nvme_submit_sync_cmd(&d);
if (ret >= 0)
*val = le64_to_cpu(res.u64);
if (unlikely(ret != 0))
@@ -235,6 +256,18 @@ int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
{
struct nvme_command cmd;
int ret;
+ struct nvme_submit_sync_data d = {
+ .q = ctrl->fabrics_q,
+ .cmd = &cmd,
+ .result = NULL,
+ .buffer = NULL,
+ .bufflen = 0,
+ .timeout = 0,
+ .qid = NVME_QID_ANY,
+ .at_head = 0,
+ .flags = 0,
+ .poll = false
+ };
memset(&cmd, 0, sizeof(cmd));
cmd.prop_set.opcode = nvme_fabrics_command;
@@ -243,8 +276,7 @@ int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
cmd.prop_set.offset = cpu_to_le32(off);
cmd.prop_set.value = cpu_to_le64(val);
- ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, NULL, NULL, 0, 0,
- NVME_QID_ANY, 0, 0, false);
+ ret = __nvme_submit_sync_cmd(&d);
if (unlikely(ret))
dev_err(ctrl->device,
"Property Set error: %d, offset %#x\n",
@@ -366,10 +398,25 @@ static void nvmf_log_connect_error(struct nvme_ctrl *ctrl,
*/
int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
{
+ struct nvmf_connect_data *data = kzalloc(sizeof(*data), GFP_KERNEL);
struct nvme_command cmd;
union nvme_result res;
- struct nvmf_connect_data *data;
int ret;
+ struct nvme_submit_sync_data d = {
+ .q = ctrl->fabrics_q,
+ .cmd = &cmd,
+ .result = &res,
+ .buffer = data,
+ .bufflen = sizeof(data),
+ .timeout = 0,
+ .qid = NVME_QID_ANY,
+ .at_head = 1,
+ .flags = BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT,
+ .poll = false
+ };
+
+ if (!data)
+ return -ENOMEM;
memset(&cmd, 0, sizeof(cmd));
cmd.connect.opcode = nvme_fabrics_command;
@@ -387,18 +434,12 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
if (ctrl->opts->disable_sqflow)
cmd.connect.cattr |= NVME_CONNECT_DISABLE_SQFLOW;
- data = kzalloc(sizeof(*data), GFP_KERNEL);
- if (!data)
- return -ENOMEM;
-
uuid_copy(&data->hostid, &ctrl->opts->host->id);
data->cntlid = cpu_to_le16(0xffff);
strncpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE);
strncpy(data->hostnqn, ctrl->opts->host->nqn, NVMF_NQN_SIZE);
- ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res,
- data, sizeof(*data), 0, NVME_QID_ANY, 1,
- BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT, false);
+ ret = __nvme_submit_sync_cmd(&d);
if (ret) {
nvmf_log_connect_error(ctrl, ret, le32_to_cpu(res.u32),
&cmd, data);
@@ -436,10 +477,25 @@ EXPORT_SYMBOL_GPL(nvmf_connect_admin_queue);
*/
int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid, bool poll)
{
+ struct nvmf_connect_data *data = kzalloc(sizeof(*data), GFP_KERNEL);
struct nvme_command cmd;
- struct nvmf_connect_data *data;
union nvme_result res;
int ret;
+ struct nvme_submit_sync_data d = {
+ .q = ctrl->connect_q,
+ .cmd = &cmd,
+ .result = &res,
+ .buffer = data,
+ .bufflen = sizeof(*data),
+ .timeout = 0,
+ .qid = qid,
+ .at_head = 1,
+ .flags = BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT,
+ .poll = poll
+ };
+
+ if (!data)
+ return -ENOMEM;
memset(&cmd, 0, sizeof(cmd));
cmd.connect.opcode = nvme_fabrics_command;
@@ -450,18 +506,12 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid, bool poll)
if (ctrl->opts->disable_sqflow)
cmd.connect.cattr |= NVME_CONNECT_DISABLE_SQFLOW;
- data = kzalloc(sizeof(*data), GFP_KERNEL);
- if (!data)
- return -ENOMEM;
-
uuid_copy(&data->hostid, &ctrl->opts->host->id);
data->cntlid = cpu_to_le16(ctrl->cntlid);
strncpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE);
strncpy(data->hostnqn, ctrl->opts->host->nqn, NVMF_NQN_SIZE);
- ret = __nvme_submit_sync_cmd(ctrl->connect_q, &cmd, &res,
- data, sizeof(*data), 0, qid, 1,
- BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT, poll);
+ ret = __nvme_submit_sync_cmd(&d);
if (ret) {
nvmf_log_connect_error(ctrl, ret, le32_to_cpu(res.u32),
&cmd, data);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 38a83ef5bcd3..f172d8e02fc6 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -395,6 +395,19 @@ struct nvme_ctrl_ops {
int (*get_address)(struct nvme_ctrl *ctrl, char *buf, int size);
};
+struct nvme_submit_sync_data {
+ struct request_queue *q;
+ struct nvme_command *cmd;
+ union nvme_result *result;
+ void *buffer;
+ unsigned bufflen;
+ unsigned timeout;
+ int qid;
+ int at_head;
+ blk_mq_req_flags_t flags;
+ bool poll;
+};
+
#ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
void nvme_fault_inject_init(struct nvme_fault_inject *fault_inj,
const char *dev_name);
@@ -485,10 +498,7 @@ blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req,
struct nvme_command *cmd);
int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
void *buf, unsigned bufflen);
-int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
- union nvme_result *result, void *buffer, unsigned bufflen,
- unsigned timeout, int qid, int at_head,
- blk_mq_req_flags_t flags, bool poll);
+int __nvme_submit_sync_cmd(struct nvme_submit_sync_data *d);
int nvme_set_features(struct nvme_ctrl *dev, unsigned int fid,
unsigned int dword11, void *buffer, size_t buflen,
u32 *result);
--
2.22.1
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] nvme: define struct for __nvme_submit_sync_cmd()
2019-10-01 23:13 [PATCH] nvme: define struct for __nvme_submit_sync_cmd() Chaitanya Kulkarni
@ 2019-10-02 5:47 ` Javier González
2019-10-02 6:10 ` Chaitanya Kulkarni
2019-10-05 0:09 ` Sagi Grimberg
2019-10-06 10:12 ` Christoph Hellwig
2 siblings, 1 reply; 8+ messages in thread
From: Javier González @ 2019-10-02 5:47 UTC (permalink / raw)
To: Chaitanya Kulkarni; +Cc: linux-nvme
[-- Attachment #1.1: Type: text/plain, Size: 13271 bytes --]
> On 2 Oct 2019, at 01.13, Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com> wrote:
>
> Over the period of time __nvme_submit_sync_cmd() function has grown to
> accept large number of paratements. The function __nvme_submit_sync_cmd()
> now takes 10 parameters. This patch consolidates all the parameters into
> one defined structure.
>
> This makes calls to the same function easy to read and improves overall
> code readability.
>
> Signed-off-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>
> ---
> Hi,
>
> I've compile tested this patch. Once we agree on this change
> I'll send out a tested version.
>
> Regards,
> Chaitanya
> ---
> drivers/nvme/host/core.c | 78 +++++++++++++++++++++---------
> drivers/nvme/host/fabrics.c | 96 ++++++++++++++++++++++++++++---------
> drivers/nvme/host/nvme.h | 18 +++++--
> 3 files changed, 142 insertions(+), 50 deletions(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index e35615940365..9be51df8fb2f 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -769,32 +769,30 @@ static void nvme_execute_rq_polled(struct request_queue *q,
> * Returns 0 on success. If the result is negative, it's a Linux error code;
> * if the result is positive, it's an NVM Express status code
> */
> -int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
> - union nvme_result *result, void *buffer, unsigned bufflen,
> - unsigned timeout, int qid, int at_head,
> - blk_mq_req_flags_t flags, bool poll)
> +int __nvme_submit_sync_cmd(struct nvme_submit_sync_data *d)
> {
> struct request *req;
> int ret;
>
> - req = nvme_alloc_request(q, cmd, flags, qid);
> + req = nvme_alloc_request(d->q, d->cmd, d->flags, d->qid);
> if (IS_ERR(req))
> return PTR_ERR(req);
>
> - req->timeout = timeout ? timeout : ADMIN_TIMEOUT;
> + req->timeout = d->timeout ? d->timeout : ADMIN_TIMEOUT;
>
> - if (buffer && bufflen) {
> - ret = blk_rq_map_kern(q, req, buffer, bufflen, GFP_KERNEL);
> + if (d->buffer && d->bufflen) {
> + ret = blk_rq_map_kern(d->q, req, d->buffer, d->bufflen,
> + GFP_KERNEL);
> if (ret)
> goto out;
> }
>
> - if (poll)
> - nvme_execute_rq_polled(req->q, NULL, req, at_head);
> + if (d->poll)
> + nvme_execute_rq_polled(req->q, NULL, req, d->at_head);
> else
> - blk_execute_rq(req->q, NULL, req, at_head);
> - if (result)
> - *result = nvme_req(req)->result;
> + blk_execute_rq(req->q, NULL, req, d->at_head);
> + if (d->result)
> + *(d->result) = nvme_req(req)->result;
> if (nvme_req(req)->flags & NVME_REQ_CANCELLED)
> ret = -EINTR;
> else
> @@ -808,8 +806,20 @@ EXPORT_SYMBOL_GPL(__nvme_submit_sync_cmd);
> int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
> void *buffer, unsigned bufflen)
> {
> - return __nvme_submit_sync_cmd(q, cmd, NULL, buffer, bufflen, 0,
> - NVME_QID_ANY, 0, 0, false);
> + struct nvme_submit_sync_data d = {
> + .q = q,
> + .cmd = cmd,
> + .result = NULL,
> + .buffer = buffer,
> + .bufflen = bufflen,
> + .timeout= 0,
> + .qid = NVME_QID_ANY,
> + .at_head = 0,
> + .flags = 0,
> + .poll = false
> + };
> +
> + return __nvme_submit_sync_cmd(&d);
> }
> EXPORT_SYMBOL_GPL(nvme_submit_sync_cmd);
>
> @@ -1114,19 +1124,30 @@ static int nvme_identify_ns(struct nvme_ctrl *ctrl,
> }
>
> static int nvme_features(struct nvme_ctrl *dev, u8 op, unsigned int fid,
> - unsigned int dword11, void *buffer, size_t buflen, u32 *result)
> + unsigned int dword11, void *buffer, size_t buflen,
> + u32 *result)
> {
> - struct nvme_command c;
> + struct nvme_command c = {};
> union nvme_result res;
> int ret;
> + struct nvme_submit_sync_data d = {
> + .q = dev->admin_q,
> + .cmd = &c,
> + .result = &res,
> + .buffer = buffer,
> + .bufflen = buflen,
> + .timeout = 0,
> + .qid = NVME_QID_ANY,
> + .at_head = 0,
> + .flags = 0,
> + .poll = false,
> + };
>
> - memset(&c, 0, sizeof(c));
> c.features.opcode = op;
> c.features.fid = cpu_to_le32(fid);
> c.features.dword11 = cpu_to_le32(dword11);
>
> - ret = __nvme_submit_sync_cmd(dev->admin_q, &c, &res,
> - buffer, buflen, 0, NVME_QID_ANY, 0, 0, false);
> + ret = __nvme_submit_sync_cmd(&d);
> if (ret >= 0 && result)
> *result = le32_to_cpu(res.u32);
> return ret;
> @@ -1954,10 +1975,22 @@ static const struct pr_ops nvme_pr_ops = {
>
> #ifdef CONFIG_BLK_SED_OPAL
> int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
> - bool send)
> + bool send)
> {
> struct nvme_ctrl *ctrl = data;
> struct nvme_command cmd;
> + struct nvme_submit_sync_data d = {
> + .q = dev->admin_q,
> + .cmd = &cmd,
> + .result = NULL,
> + .buffer = buffer,
> + .bufflen = len,
> + .timeout = ADMIN_TIMEOUT,
> + .qid = NVME_QID_ANY,
> + .at_head = 1;
> + .flags = 0;
> + .poll = false;
> + };
>
> memset(&cmd, 0, sizeof(cmd));
> if (send)
> @@ -1968,8 +2001,7 @@ int nvme_sec_submit(void *data, u16 spsp, u8 secp, void *buffer, size_t len,
> cmd.common.cdw10 = cpu_to_le32(((u32)secp) << 24 | ((u32)spsp) << 8);
> cmd.common.cdw11 = cpu_to_le32(len);
>
> - return __nvme_submit_sync_cmd(ctrl->admin_q, &cmd, NULL, buffer, len,
> - ADMIN_TIMEOUT, NVME_QID_ANY, 1, 0, false);
> + return __nvme_submit_sync_cmd(&d);
> }
> EXPORT_SYMBOL_GPL(nvme_sec_submit);
> #endif /* CONFIG_BLK_SED_OPAL */
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 74b8818ac9a1..c5a8aec5f046 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -144,14 +144,25 @@ int nvmf_reg_read32(struct nvme_ctrl *ctrl, u32 off, u32 *val)
> struct nvme_command cmd;
> union nvme_result res;
> int ret;
> + struct nvme_submit_sync_data d = {
> + .q = ctrl->fabrics_q,
> + .cmd = &cmd,
> + .result = &res,
> + .buffer = NULL,
> + .bufflen = 0,
> + .timeout = 0,
> + .qid = NVME_QID_ANY,
> + .at_head = 0,
> + .flags = 0,
> + .poll = false
> + };
>
> memset(&cmd, 0, sizeof(cmd));
> cmd.prop_get.opcode = nvme_fabrics_command;
> cmd.prop_get.fctype = nvme_fabrics_type_property_get;
> cmd.prop_get.offset = cpu_to_le32(off);
>
> - ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0, 0,
> - NVME_QID_ANY, 0, 0, false);
> + ret = __nvme_submit_sync_cmd(&d);
>
> if (ret >= 0)
> *val = le64_to_cpu(res.u64);
> @@ -190,6 +201,18 @@ int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val)
> struct nvme_command cmd;
> union nvme_result res;
> int ret;
> + struct nvme_submit_sync_data d = {
> + .q = ctrl->fabrics_q,
> + .cmd = &cmd,
> + .result = &res,
> + .buffer = NULL,
> + .bufflen = 0,
> + .timeout = 0,
> + .qid = NVME_QID_ANY,
> + .at_head = 0,
> + .flags = 0,
> + .poll = false
> + };
>
> memset(&cmd, 0, sizeof(cmd));
> cmd.prop_get.opcode = nvme_fabrics_command;
> @@ -197,9 +220,7 @@ int nvmf_reg_read64(struct nvme_ctrl *ctrl, u32 off, u64 *val)
> cmd.prop_get.attrib = 1;
> cmd.prop_get.offset = cpu_to_le32(off);
>
> - ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res, NULL, 0, 0,
> - NVME_QID_ANY, 0, 0, false);
> -
> + ret = __nvme_submit_sync_cmd(&d);
> if (ret >= 0)
> *val = le64_to_cpu(res.u64);
> if (unlikely(ret != 0))
> @@ -235,6 +256,18 @@ int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
> {
> struct nvme_command cmd;
> int ret;
> + struct nvme_submit_sync_data d = {
> + .q = ctrl->fabrics_q,
> + .cmd = &cmd,
> + .result = NULL,
> + .buffer = NULL,
> + .bufflen = 0,
> + .timeout = 0,
> + .qid = NVME_QID_ANY,
> + .at_head = 0,
> + .flags = 0,
> + .poll = false
> + };
>
> memset(&cmd, 0, sizeof(cmd));
> cmd.prop_set.opcode = nvme_fabrics_command;
> @@ -243,8 +276,7 @@ int nvmf_reg_write32(struct nvme_ctrl *ctrl, u32 off, u32 val)
> cmd.prop_set.offset = cpu_to_le32(off);
> cmd.prop_set.value = cpu_to_le64(val);
>
> - ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, NULL, NULL, 0, 0,
> - NVME_QID_ANY, 0, 0, false);
> + ret = __nvme_submit_sync_cmd(&d);
> if (unlikely(ret))
> dev_err(ctrl->device,
> "Property Set error: %d, offset %#x\n",
> @@ -366,10 +398,25 @@ static void nvmf_log_connect_error(struct nvme_ctrl *ctrl,
> */
> int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
> {
> + struct nvmf_connect_data *data = kzalloc(sizeof(*data), GFP_KERNEL);
> struct nvme_command cmd;
> union nvme_result res;
> - struct nvmf_connect_data *data;
> int ret;
> + struct nvme_submit_sync_data d = {
> + .q = ctrl->fabrics_q,
> + .cmd = &cmd,
> + .result = &res,
> + .buffer = data,
> + .bufflen = sizeof(data),
> + .timeout = 0,
> + .qid = NVME_QID_ANY,
> + .at_head = 1,
> + .flags = BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT,
> + .poll = false
> + };
> +
> + if (!data)
> + return -ENOMEM;
>
> memset(&cmd, 0, sizeof(cmd));
> cmd.connect.opcode = nvme_fabrics_command;
> @@ -387,18 +434,12 @@ int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
> if (ctrl->opts->disable_sqflow)
> cmd.connect.cattr |= NVME_CONNECT_DISABLE_SQFLOW;
>
> - data = kzalloc(sizeof(*data), GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> -
> uuid_copy(&data->hostid, &ctrl->opts->host->id);
> data->cntlid = cpu_to_le16(0xffff);
> strncpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE);
> strncpy(data->hostnqn, ctrl->opts->host->nqn, NVMF_NQN_SIZE);
>
> - ret = __nvme_submit_sync_cmd(ctrl->fabrics_q, &cmd, &res,
> - data, sizeof(*data), 0, NVME_QID_ANY, 1,
> - BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT, false);
> + ret = __nvme_submit_sync_cmd(&d);
> if (ret) {
> nvmf_log_connect_error(ctrl, ret, le32_to_cpu(res.u32),
> &cmd, data);
> @@ -436,10 +477,25 @@ EXPORT_SYMBOL_GPL(nvmf_connect_admin_queue);
> */
> int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid, bool poll)
> {
> + struct nvmf_connect_data *data = kzalloc(sizeof(*data), GFP_KERNEL);
> struct nvme_command cmd;
> - struct nvmf_connect_data *data;
> union nvme_result res;
> int ret;
> + struct nvme_submit_sync_data d = {
> + .q = ctrl->connect_q,
> + .cmd = &cmd,
> + .result = &res,
> + .buffer = data,
> + .bufflen = sizeof(*data),
> + .timeout = 0,
> + .qid = qid,
> + .at_head = 1,
> + .flags = BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT,
> + .poll = poll
> + };
> +
> + if (!data)
> + return -ENOMEM;
>
> memset(&cmd, 0, sizeof(cmd));
> cmd.connect.opcode = nvme_fabrics_command;
> @@ -450,18 +506,12 @@ int nvmf_connect_io_queue(struct nvme_ctrl *ctrl, u16 qid, bool poll)
> if (ctrl->opts->disable_sqflow)
> cmd.connect.cattr |= NVME_CONNECT_DISABLE_SQFLOW;
>
> - data = kzalloc(sizeof(*data), GFP_KERNEL);
> - if (!data)
> - return -ENOMEM;
> -
> uuid_copy(&data->hostid, &ctrl->opts->host->id);
> data->cntlid = cpu_to_le16(ctrl->cntlid);
> strncpy(data->subsysnqn, ctrl->opts->subsysnqn, NVMF_NQN_SIZE);
> strncpy(data->hostnqn, ctrl->opts->host->nqn, NVMF_NQN_SIZE);
>
> - ret = __nvme_submit_sync_cmd(ctrl->connect_q, &cmd, &res,
> - data, sizeof(*data), 0, qid, 1,
> - BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT, poll);
> + ret = __nvme_submit_sync_cmd(&d);
> if (ret) {
> nvmf_log_connect_error(ctrl, ret, le32_to_cpu(res.u32),
> &cmd, data);
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 38a83ef5bcd3..f172d8e02fc6 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -395,6 +395,19 @@ struct nvme_ctrl_ops {
> int (*get_address)(struct nvme_ctrl *ctrl, char *buf, int size);
> };
>
> +struct nvme_submit_sync_data {
> + struct request_queue *q;
> + struct nvme_command *cmd;
> + union nvme_result *result;
> + void *buffer;
> + unsigned bufflen;
> + unsigned timeout;
> + int qid;
> + int at_head;
> + blk_mq_req_flags_t flags;
> + bool poll;
> +};
> +
> #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS
> void nvme_fault_inject_init(struct nvme_fault_inject *fault_inj,
> const char *dev_name);
> @@ -485,10 +498,7 @@ blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req,
> struct nvme_command *cmd);
> int nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
> void *buf, unsigned bufflen);
> -int __nvme_submit_sync_cmd(struct request_queue *q, struct nvme_command *cmd,
> - union nvme_result *result, void *buffer, unsigned bufflen,
> - unsigned timeout, int qid, int at_head,
> - blk_mq_req_flags_t flags, bool poll);
> +int __nvme_submit_sync_cmd(struct nvme_submit_sync_data *d);
> int nvme_set_features(struct nvme_ctrl *dev, unsigned int fid,
> unsigned int dword11, void *buffer, size_t buflen,
> u32 *result);
> --
> 2.22.1
>
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
Makes sense to me. We have other places with similar arguments (e.g.,
nvme_submit_user_cmd). Would it make sense to unify this too if we move
in this direction?
Javier
[-- Attachment #1.2: Message signed with OpenPGP --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
[-- Attachment #2: Type: text/plain, Size: 158 bytes --]
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nvme: define struct for __nvme_submit_sync_cmd()
2019-10-02 5:47 ` Javier González
@ 2019-10-02 6:10 ` Chaitanya Kulkarni
2019-10-05 14:48 ` Keith Busch
0 siblings, 1 reply; 8+ messages in thread
From: Chaitanya Kulkarni @ 2019-10-02 6:10 UTC (permalink / raw)
To: Javier González; +Cc: linux-nvme
On 10/1/19 10:47 PM, Javier González wrote:
> Makes sense to me. We have other places with similar arguments (e.g.,
> nvme_submit_user_cmd). Would it make sense to unify this too if we move
> in this direction?
>
Thanks for the feedback, I'll add that too in the next version.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nvme: define struct for __nvme_submit_sync_cmd()
2019-10-01 23:13 [PATCH] nvme: define struct for __nvme_submit_sync_cmd() Chaitanya Kulkarni
2019-10-02 5:47 ` Javier González
@ 2019-10-05 0:09 ` Sagi Grimberg
2019-10-05 2:09 ` Balbir Singh
2019-10-06 10:12 ` Christoph Hellwig
2 siblings, 1 reply; 8+ messages in thread
From: Sagi Grimberg @ 2019-10-05 0:09 UTC (permalink / raw)
To: Chaitanya Kulkarni, linux-nvme
> Over the period of time __nvme_submit_sync_cmd() function has grown to
> accept large number of paratements. The function __nvme_submit_sync_cmd()
> now takes 10 parameters. This patch consolidates all the parameters into
> one defined structure.
>
> This makes calls to the same function easy to read and improves overall
> code readability.
Personally I'm not a big fan... But I'm not going to reject it either...
What do others think?
[...]
> @@ -366,10 +398,25 @@ static void nvmf_log_connect_error(struct nvme_ctrl *ctrl,
> */
> int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
> {
> + struct nvmf_connect_data *data = kzalloc(sizeof(*data), GFP_KERNEL);
Please don't. I'm allergic to allocation on declaration.
Just set d.buffer after the initializer.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nvme: define struct for __nvme_submit_sync_cmd()
2019-10-05 0:09 ` Sagi Grimberg
@ 2019-10-05 2:09 ` Balbir Singh
0 siblings, 0 replies; 8+ messages in thread
From: Balbir Singh @ 2019-10-05 2:09 UTC (permalink / raw)
To: Sagi Grimberg, Chaitanya Kulkarni, linux-nvme
On Fri, 2019-10-04 at 17:09 -0700, Sagi Grimberg wrote:
> > Over the period of time __nvme_submit_sync_cmd() function has grown to
> > accept large number of paratements. The function __nvme_submit_sync_cmd()
> > now takes 10 parameters. This patch consolidates all the parameters into
> > one defined structure.
> >
> > This makes calls to the same function easy to read and improves overall
> > code readability.
>
> Personally I'm not a big fan... But I'm not going to reject it either...
>
> What do others think?
>
> [...]
>
The patch itself is not the cleanest, smaller well defined refactoring would
be much better - at first glance
Balbir Singh.
> > @@ -366,10 +398,25 @@ static void nvmf_log_connect_error(struct nvme_ctrl
> > *ctrl,
> > */
> > int nvmf_connect_admin_queue(struct nvme_ctrl *ctrl)
> > {
> > + struct nvmf_connect_data *data = kzalloc(sizeof(*data), GFP_KERNEL);
>
> Please don't. I'm allergic to allocation on declaration.
> Just set d.buffer after the initializer.
>
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nvme: define struct for __nvme_submit_sync_cmd()
2019-10-02 6:10 ` Chaitanya Kulkarni
@ 2019-10-05 14:48 ` Keith Busch
2019-10-06 10:13 ` Christoph Hellwig
0 siblings, 1 reply; 8+ messages in thread
From: Keith Busch @ 2019-10-05 14:48 UTC (permalink / raw)
To: Chaitanya Kulkarni; +Cc: Javier González, linux-nvme
On Wed, Oct 02, 2019 at 06:10:09AM +0000, Chaitanya Kulkarni wrote:
> On 10/1/19 10:47 PM, Javier González wrote:
> > Makes sense to me. We have other places with similar arguments (e.g.,
> > nvme_submit_user_cmd). Would it make sense to unify this too if we move
> > in this direction?
> >
> Thanks for the feedback, I'll add that too in the next version.
It also looks like much of the callers can use the much simpler
nvme_submit_user_cmd() that already provides defaults to the numerous
parameters.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nvme: define struct for __nvme_submit_sync_cmd()
2019-10-01 23:13 [PATCH] nvme: define struct for __nvme_submit_sync_cmd() Chaitanya Kulkarni
2019-10-02 5:47 ` Javier González
2019-10-05 0:09 ` Sagi Grimberg
@ 2019-10-06 10:12 ` Christoph Hellwig
2 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2019-10-06 10:12 UTC (permalink / raw)
To: Chaitanya Kulkarni; +Cc: linux-nvme
On Tue, Oct 01, 2019 at 04:13:46PM -0700, Chaitanya Kulkarni wrote:
> Over the period of time __nvme_submit_sync_cmd() function has grown to
> accept large number of paratements. The function __nvme_submit_sync_cmd()
> now takes 10 parameters. This patch consolidates all the parameters into
> one defined structure.
>
> This makes calls to the same function easy to read and improves overall
> code readability.
This makes the code both a lot more unreadable and a lot larger. What
is the point?
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] nvme: define struct for __nvme_submit_sync_cmd()
2019-10-05 14:48 ` Keith Busch
@ 2019-10-06 10:13 ` Christoph Hellwig
0 siblings, 0 replies; 8+ messages in thread
From: Christoph Hellwig @ 2019-10-06 10:13 UTC (permalink / raw)
To: Keith Busch; +Cc: Javier González, linux-nvme, Chaitanya Kulkarni
On Sat, Oct 05, 2019 at 08:48:12AM -0600, Keith Busch wrote:
> On Wed, Oct 02, 2019 at 06:10:09AM +0000, Chaitanya Kulkarni wrote:
> > On 10/1/19 10:47 PM, Javier González wrote:
> > > Makes sense to me. We have other places with similar arguments (e.g.,
> > > nvme_submit_user_cmd). Would it make sense to unify this too if we move
> > > in this direction?
> > >
> > Thanks for the feedback, I'll add that too in the next version.
>
> It also looks like much of the callers can use the much simpler
> nvme_submit_user_cmd() that already provides defaults to the numerous
> parameters.
That on the other hand actually seems like a useful change.
_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2019-10-06 10:13 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-01 23:13 [PATCH] nvme: define struct for __nvme_submit_sync_cmd() Chaitanya Kulkarni
2019-10-02 5:47 ` Javier González
2019-10-02 6:10 ` Chaitanya Kulkarni
2019-10-05 14:48 ` Keith Busch
2019-10-06 10:13 ` Christoph Hellwig
2019-10-05 0:09 ` Sagi Grimberg
2019-10-05 2:09 ` Balbir Singh
2019-10-06 10:12 ` Christoph Hellwig
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).