From: Minwoo Im <minwoo.im.dev@gmail.com> To: Weiping Zhang <zhangweiping@didiglobal.com> Cc: axboe@kernel.dk, tj@kernel.org, hch@lst.de, bvanassche@acm.org, keith.busch@intel.com, minwoo.im.dev@gmail.com, linux-block@vger.kernel.org, cgroups@vger.kernel.org, linux-nvme@lists.infradead.org Subject: Re: [PATCH v3 2/5] nvme: add get_ams for nvme_ctrl_ops Date: Tue, 25 Jun 2019 05:12:56 +0900 [thread overview] Message-ID: <20190624201256.GC6526@minwooim-desktop> (raw) In-Reply-To: <2c916063e19cc3f33376d7bb0fb8a5ff10ea42db.1561385989.git.zhangweiping@didiglobal.com> On 19-06-24 22:29:05, Weiping Zhang wrote: > The get_ams() will return the AMS(Arbitration Mechanism Selected) > from the driver. > > Signed-off-by: Weiping Zhang <zhangweiping@didiglobal.com> Hello, Weiping. Sorry, but I don't really get what your point is here. Could you please elaborate this patch a little bit more? The commit description just says a function would just return the AMS from nowhere.. > --- > drivers/nvme/host/core.c | 9 ++++++++- > drivers/nvme/host/nvme.h | 1 + > drivers/nvme/host/pci.c | 6 ++++++ > include/linux/nvme.h | 1 + > 4 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index b2dd4e391f5c..4cb781e73123 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -1943,6 +1943,7 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl, u64 cap) > */ > unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12, page_shift = 12; > int ret; > + u32 ams = NVME_CC_AMS_RR; > > if (page_shift < dev_page_min) { > dev_err(ctrl->device, > @@ -1951,11 +1952,17 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl, u64 cap) > return -ENODEV; > } > > + /* get Arbitration Mechanism Selected */ > + if (ctrl->ops->get_ams) { I just wonder if this check will be valid because this patch just register the function nvme_pci_get_ams() without any conditions. > + ctrl->ops->get_ams(ctrl, &ams); > + ams &= NVME_CC_AMS_MASK; > + } > + > ctrl->page_size = 1 << page_shift; > > ctrl->ctrl_config = NVME_CC_CSS_NVM; > ctrl->ctrl_config |= (page_shift - 12) << NVME_CC_MPS_SHIFT; > - ctrl->ctrl_config |= NVME_CC_AMS_RR | NVME_CC_SHN_NONE; > + ctrl->ctrl_config |= ams | NVME_CC_SHN_NONE; > ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES; > ctrl->ctrl_config |= NVME_CC_ENABLE; > > diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h > index ea45d7d393ad..9c7e9217f78b 100644 > --- a/drivers/nvme/host/nvme.h > +++ b/drivers/nvme/host/nvme.h > @@ -369,6 +369,7 @@ struct nvme_ctrl_ops { > void (*submit_async_event)(struct nvme_ctrl *ctrl); > void (*delete_ctrl)(struct nvme_ctrl *ctrl); > int (*get_address)(struct nvme_ctrl *ctrl, char *buf, int size); > + void (*get_ams)(struct nvme_ctrl *ctrl, u32 *ams); Can we just have a return value for the AMS value? > }; > > #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index 189352081994..5d84241f0214 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -2627,6 +2627,11 @@ static int nvme_pci_get_address(struct nvme_ctrl *ctrl, char *buf, int size) > return snprintf(buf, size, "%s", dev_name(&pdev->dev)); > } > > +static void nvme_pci_get_ams(struct nvme_ctrl *ctrl, u32 *ams) > +{ > + *ams = NVME_CC_AMS_RR; > +} > + > static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = { > .name = "pcie", > .module = THIS_MODULE, > @@ -2638,6 +2643,7 @@ static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = { > .free_ctrl = nvme_pci_free_ctrl, > .submit_async_event = nvme_pci_submit_async_event, > .get_address = nvme_pci_get_address, > + .get_ams = nvme_pci_get_ams, Question: Do we really need this being added to nvme_ctrl_ops? Also If 5th patch will make this function much more than this, then it would be great if you describe this kind of situation in description :) > }; > > static int nvme_dev_map(struct nvme_dev *dev) > diff --git a/include/linux/nvme.h b/include/linux/nvme.h > index da5f696aec00..8f71451fc2fa 100644 > --- a/include/linux/nvme.h > +++ b/include/linux/nvme.h > @@ -156,6 +156,7 @@ enum { > NVME_CC_AMS_RR = 0 << NVME_CC_AMS_SHIFT, > NVME_CC_AMS_WRRU = 1 << NVME_CC_AMS_SHIFT, > NVME_CC_AMS_VS = 7 << NVME_CC_AMS_SHIFT, > + NVME_CC_AMS_MASK = 7 << NVME_CC_AMS_SHIFT, > NVME_CC_SHN_NONE = 0 << NVME_CC_SHN_SHIFT, > NVME_CC_SHN_NORMAL = 1 << NVME_CC_SHN_SHIFT, > NVME_CC_SHN_ABRUPT = 2 << NVME_CC_SHN_SHIFT, > -- > 2.14.1 >
WARNING: multiple messages have this Message-ID (diff)
From: minwoo.im.dev@gmail.com (Minwoo Im) Subject: [PATCH v3 2/5] nvme: add get_ams for nvme_ctrl_ops Date: Tue, 25 Jun 2019 05:12:56 +0900 [thread overview] Message-ID: <20190624201256.GC6526@minwooim-desktop> (raw) In-Reply-To: <2c916063e19cc3f33376d7bb0fb8a5ff10ea42db.1561385989.git.zhangweiping@didiglobal.com> On 19-06-24 22:29:05, Weiping Zhang wrote: > The get_ams() will return the AMS(Arbitration Mechanism Selected) > from the driver. > > Signed-off-by: Weiping Zhang <zhangweiping at didiglobal.com> Hello, Weiping. Sorry, but I don't really get what your point is here. Could you please elaborate this patch a little bit more? The commit description just says a function would just return the AMS from nowhere.. > --- > drivers/nvme/host/core.c | 9 ++++++++- > drivers/nvme/host/nvme.h | 1 + > drivers/nvme/host/pci.c | 6 ++++++ > include/linux/nvme.h | 1 + > 4 files changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c > index b2dd4e391f5c..4cb781e73123 100644 > --- a/drivers/nvme/host/core.c > +++ b/drivers/nvme/host/core.c > @@ -1943,6 +1943,7 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl, u64 cap) > */ > unsigned dev_page_min = NVME_CAP_MPSMIN(cap) + 12, page_shift = 12; > int ret; > + u32 ams = NVME_CC_AMS_RR; > > if (page_shift < dev_page_min) { > dev_err(ctrl->device, > @@ -1951,11 +1952,17 @@ int nvme_enable_ctrl(struct nvme_ctrl *ctrl, u64 cap) > return -ENODEV; > } > > + /* get Arbitration Mechanism Selected */ > + if (ctrl->ops->get_ams) { I just wonder if this check will be valid because this patch just register the function nvme_pci_get_ams() without any conditions. > + ctrl->ops->get_ams(ctrl, &ams); > + ams &= NVME_CC_AMS_MASK; > + } > + > ctrl->page_size = 1 << page_shift; > > ctrl->ctrl_config = NVME_CC_CSS_NVM; > ctrl->ctrl_config |= (page_shift - 12) << NVME_CC_MPS_SHIFT; > - ctrl->ctrl_config |= NVME_CC_AMS_RR | NVME_CC_SHN_NONE; > + ctrl->ctrl_config |= ams | NVME_CC_SHN_NONE; > ctrl->ctrl_config |= NVME_CC_IOSQES | NVME_CC_IOCQES; > ctrl->ctrl_config |= NVME_CC_ENABLE; > > diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h > index ea45d7d393ad..9c7e9217f78b 100644 > --- a/drivers/nvme/host/nvme.h > +++ b/drivers/nvme/host/nvme.h > @@ -369,6 +369,7 @@ struct nvme_ctrl_ops { > void (*submit_async_event)(struct nvme_ctrl *ctrl); > void (*delete_ctrl)(struct nvme_ctrl *ctrl); > int (*get_address)(struct nvme_ctrl *ctrl, char *buf, int size); > + void (*get_ams)(struct nvme_ctrl *ctrl, u32 *ams); Can we just have a return value for the AMS value? > }; > > #ifdef CONFIG_FAULT_INJECTION_DEBUG_FS > diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c > index 189352081994..5d84241f0214 100644 > --- a/drivers/nvme/host/pci.c > +++ b/drivers/nvme/host/pci.c > @@ -2627,6 +2627,11 @@ static int nvme_pci_get_address(struct nvme_ctrl *ctrl, char *buf, int size) > return snprintf(buf, size, "%s", dev_name(&pdev->dev)); > } > > +static void nvme_pci_get_ams(struct nvme_ctrl *ctrl, u32 *ams) > +{ > + *ams = NVME_CC_AMS_RR; > +} > + > static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = { > .name = "pcie", > .module = THIS_MODULE, > @@ -2638,6 +2643,7 @@ static const struct nvme_ctrl_ops nvme_pci_ctrl_ops = { > .free_ctrl = nvme_pci_free_ctrl, > .submit_async_event = nvme_pci_submit_async_event, > .get_address = nvme_pci_get_address, > + .get_ams = nvme_pci_get_ams, Question: Do we really need this being added to nvme_ctrl_ops? Also If 5th patch will make this function much more than this, then it would be great if you describe this kind of situation in description :) > }; > > static int nvme_dev_map(struct nvme_dev *dev) > diff --git a/include/linux/nvme.h b/include/linux/nvme.h > index da5f696aec00..8f71451fc2fa 100644 > --- a/include/linux/nvme.h > +++ b/include/linux/nvme.h > @@ -156,6 +156,7 @@ enum { > NVME_CC_AMS_RR = 0 << NVME_CC_AMS_SHIFT, > NVME_CC_AMS_WRRU = 1 << NVME_CC_AMS_SHIFT, > NVME_CC_AMS_VS = 7 << NVME_CC_AMS_SHIFT, > + NVME_CC_AMS_MASK = 7 << NVME_CC_AMS_SHIFT, > NVME_CC_SHN_NONE = 0 << NVME_CC_SHN_SHIFT, > NVME_CC_SHN_NORMAL = 1 << NVME_CC_SHN_SHIFT, > NVME_CC_SHN_ABRUPT = 2 << NVME_CC_SHN_SHIFT, > -- > 2.14.1 >
next prev parent reply other threads:[~2019-06-24 20:13 UTC|newest] Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top [not found] <cover.1561385989.git.zhangweiping@didiglobal.com> 2019-06-24 14:28 ` [PATCH v3 1/5] block: add weighted round robin for blkcgroup Weiping Zhang 2019-06-24 14:28 ` Weiping Zhang 2019-07-18 13:59 ` Tejun Heo 2019-07-18 13:59 ` Tejun Heo 2019-07-23 14:29 ` Weiping Zhang 2019-07-23 14:29 ` Weiping Zhang 2019-06-24 14:29 ` [PATCH v3 2/5] nvme: add get_ams for nvme_ctrl_ops Weiping Zhang 2019-06-24 14:29 ` Weiping Zhang 2019-06-24 20:12 ` Minwoo Im [this message] 2019-06-24 20:12 ` Minwoo Im 2019-06-25 14:46 ` Weiping Zhang 2019-06-25 14:46 ` Weiping Zhang 2019-06-24 14:29 ` [PATCH v3 3/5] nvme-pci: rename module parameter write_queues to read_queues Weiping Zhang 2019-06-24 14:29 ` Weiping Zhang 2019-06-24 20:04 ` Minwoo Im 2019-06-24 20:04 ` Minwoo Im 2019-06-25 14:48 ` Weiping Zhang 2019-06-25 14:48 ` Weiping Zhang 2019-06-26 20:27 ` Minwoo Im 2019-06-26 20:27 ` Minwoo Im 2019-07-14 11:36 ` Minwoo Im 2019-06-24 14:29 ` [PATCH v3 4/5] genirq/affinity: allow driver's discontigous affinity set Weiping Zhang 2019-06-24 14:29 ` Weiping Zhang 2019-06-24 14:29 ` Weiping Zhang 2019-06-24 15:42 ` Thomas Gleixner 2019-06-24 15:42 ` Thomas Gleixner 2019-06-25 2:14 ` Ming Lei 2019-06-25 2:14 ` Ming Lei 2019-06-25 6:13 ` Thomas Gleixner 2019-06-25 6:13 ` Thomas Gleixner 2019-06-25 14:55 ` Weiping Zhang 2019-06-25 14:55 ` Weiping Zhang 2019-06-24 14:29 ` [PATCH v3 5/5] nvme: add support weighted round robin queue Weiping Zhang 2019-06-24 14:29 ` Weiping Zhang 2019-06-24 20:21 ` Minwoo Im 2019-06-24 20:21 ` Minwoo Im 2019-06-25 15:06 ` Weiping Zhang 2019-06-25 15:06 ` Weiping Zhang 2019-06-27 10:37 ` Minwoo Im 2019-06-27 10:37 ` Minwoo Im 2019-06-27 11:03 ` Christoph Hellwig 2019-06-27 11:03 ` Christoph Hellwig 2019-06-28 15:57 ` Weiping Zhang 2019-06-28 15:57 ` Weiping Zhang 2019-07-10 14:20 ` Weiping Zhang 2019-07-10 14:20 ` Weiping Zhang 2019-07-29 10:22 ` Weiping Zhang 2019-07-29 10:22 ` Weiping Zhang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20190624201256.GC6526@minwooim-desktop \ --to=minwoo.im.dev@gmail.com \ --cc=axboe@kernel.dk \ --cc=bvanassche@acm.org \ --cc=cgroups@vger.kernel.org \ --cc=hch@lst.de \ --cc=keith.busch@intel.com \ --cc=linux-block@vger.kernel.org \ --cc=linux-nvme@lists.infradead.org \ --cc=tj@kernel.org \ --cc=zhangweiping@didiglobal.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.