Minwoo Im <minwoo.im.dev@xxxxxxxxx> 于2019年6月25日周二 上午6:01写道：
>
> > @@ -2627,7 +2752,30 @@ static int nvme_pci_get_address(struct nvme_ctrl *ctrl, char *buf, int size)
> >
> > static void nvme_pci_get_ams(struct nvme_ctrl *ctrl, u32 *ams)
> > {
> > - *ams = NVME_CC_AMS_RR;
> > + /* if deivce doesn't support WRR, force reset wrr queues to 0 */
> > + if (!NVME_CAP_AMS_WRRU(ctrl->cap)) {
> > + wrr_low_queues = 0;
> > + wrr_medium_queues = 0;
> > + wrr_high_queues = 0;
> > + wrr_urgent_queues = 0;
>
> Could we avoid this kind of reset variables in get_XXX() function? I
> guess it would be great if it just tries to get some value which is
> mainly focused to do.
I think its ok, when we use these variables in nvme_setup_irqs,
we can check ctrl->wrr_enabled, if it was false, skip all wrr_xxx_queues.
In other words, if ctrl->wrr_enabled is true, at least we have one wrr queue.
>
> > +
> > + *ams = NVME_CC_AMS_RR;
> > + ctrl->wrr_enabled = false;
> > + return;
> > + }
> > +
> > + /*
> > + * if device support WRR, check wrr queue count, all wrr queues are
> > + * 0, don't enable device's WRR.
> > + */
> > + if ((wrr_low_queues + wrr_medium_queues + wrr_high_queues +
> > + wrr_urgent_queues) > 0) {
> > + *ams = NVME_CC_AMS_WRRU;
> > + ctrl->wrr_enabled = true;
> > + } else {
> > + *ams = NVME_CC_AMS_RR;
> > + ctrl->wrr_enabled = false;
>
> These two line can be merged into above condition:
It's ok, and merge comments for NVME_CC_AMS_RR.
>
> if (!NVME_CAP_AMS_WRRU(ctrl->cap) ||
> wrr_low_queues + wrr_medium_queues + wrr_high_queues +
> wrr_urgent_queues <= 0) {
> *ams = NVME_CC_AMS_RR;
> ctrl->wrr_enabled = false;
> }
