Re: [RFC PATCH V2 02/11] hw/block/nvme: open code for volatile write cache

[Sai Pavan Boddu]

Hi Minwoo,

Please ignore this mail, I see a fix already floating around in the list.

Regards,
Sai Pavan
On Thu, Feb 11, 2021 at 12:15:57PM +0530, Sai Pavan Boddu wrote:
> Hi Minwoo,
> 
> On Sun, Jan 17, 2021 at 11:53:32PM +0900, Minwoo Im wrote:
> > Volatile Write Cache(VWC) feature is set in nvme_ns_setup() in the
> > initial time.  This feature is related to block device backed,  but this
> > feature is controlled in controller level via Set/Get Features command.
> > 
> > This patch removed dependency between nvme and nvme-ns to manage the VWC
> > flag value.  Also, it open coded the Get Features for VWC to check all
> > namespaces attached to the controller, and if false detected, return
> > directly false.
> > 
> > Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
> > ---
> >  hw/block/nvme-ns.c |  4 ----
> >  hw/block/nvme.c    | 15 ++++++++++++---
> >  hw/block/nvme.h    |  1 -
> >  3 files changed, 12 insertions(+), 8 deletions(-)
> > 
> > diff --git a/hw/block/nvme-ns.c b/hw/block/nvme-ns.c
> > index 32662230130b..c403cd36b6bd 100644
> > --- a/hw/block/nvme-ns.c
> > +++ b/hw/block/nvme-ns.c
> > @@ -89,10 +89,6 @@ static int nvme_ns_init_blk(NvmeCtrl *n, NvmeNamespace 
> > *ns, Error **errp)
> >          return -1;
> >      }
> >  
> > -    if (blk_enable_write_cache(ns->blkconf.blk)) {
> > -        n->features.vwc = 0x1;
> > -    }
> > -
> >      return 0;
> >  }
> >  
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index cf0fe28fe6eb..b2a9c48a9d81 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -3033,6 +3033,7 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, 
> > NvmeRequest *req)
> >      NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
> >      uint16_t iv;
> >      NvmeNamespace *ns;
> > +    int i;
> >  
> >      static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
> >          [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> > @@ -3108,7 +3109,17 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, 
> > NvmeRequest *req)
> >          result = ns->features.err_rec;
> >          goto out;
> >      case NVME_VOLATILE_WRITE_CACHE:
> > -        result = n->features.vwc;
> >
> > This change breaks the build on ubuntu 16.04, gcc is detecting error
> > as below
> > ../hw/block/nvme.c: In function ‘nvme_process_sq’:
> > ../hw/block/nvme.c:3242:9: error: ‘result’ may be used uninitialized
> > in this function [-Werror=maybe-uninitialized]
> >          trace_pci_nvme_getfeat_vwcache(result ? "enabled" :
> >          "disabled");
> >                   ^
> >                   ../hw/block/nvme.c:3150:14: note: ‘result’ was
> >                   declared here
> >                        uint32_t result;
> >                                      ^
> >GCC version  5.4.0.
> >Tested initilizing result variable, build looks good then.
> >
> >Regards,
> >Sai Pavan
> >
> > +        for (i = 1; i <= n->num_namespaces; i++) {
> > +            ns = nvme_ns(n, i);
> > +            if (!ns) {
> > +                continue;
> > +            }
> > +
> > +            result = blk_enable_write_cache(ns->blkconf.blk);
> > +            if (!result) {
> > +                break;
> > +            }
> > +        }
> >          trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> >          goto out;
> >      case NVME_ASYNCHRONOUS_EVENT_CONF:
> > @@ -3271,8 +3282,6 @@ static uint16_t nvme_set_feature(NvmeCtrl *n, 
> > NvmeRequest *req)
> >          ns->features.err_rec = dw11;
> >          break;
> >      case NVME_VOLATILE_WRITE_CACHE:
> > -        n->features.vwc = dw11 & 0x1;
> > -
> >          for (i = 1; i <= n->num_namespaces; i++) {
> >              ns = nvme_ns(n, i);
> >              if (!ns) {
> > diff --git a/hw/block/nvme.h b/hw/block/nvme.h
> > index b7fbcca39d9f..5ba83ee77841 100644
> > --- a/hw/block/nvme.h
> > +++ b/hw/block/nvme.h
> > @@ -117,7 +117,6 @@ typedef struct NvmeFeatureVal {
> >          uint16_t temp_thresh_low;
> >      };
> >      uint32_t    async_config;
> > -    uint32_t    vwc;
> >  } NvmeFeatureVal;
> >  
> >  typedef struct NvmeCtrl {
> > -- 
> > 2.17.1
> > 
> > 
>