Re: [PATCH for-6.0 v2 6/8] hw/block/nvme: update dmsrl limit on namespace detachment

[Klaus Jensen]

On Apr  9 19:39, Thomas Huth wrote:
> On 06/04/2021 09.24, Klaus Jensen wrote:
> > On Apr  6 09:10, Philippe Mathieu-Daudé wrote:
> > > On 4/5/21 7:54 PM, Klaus Jensen wrote:
> > > > From: Klaus Jensen <k.jensen@samsung.com>
> > > >
> > > > The Non-MDTS DMSRL limit must be recomputed when namespaces are
> > > > detached.
> > > >
> > > > Fixes: 645ce1a70cb6 ("hw/block/nvme: support namespace attachment command")
> > > > Signed-off-by: Klaus Jensen <k.jensen@samsung.com>
> > > > Reviewed-by: Gollu Appalanaidu <anaidu.gollu@samsung.com>
> > > > ---
> > > >  hw/block/nvme.c | 17 +++++++++++++++++
> > > >  1 file changed, 17 insertions(+)
> > > >
> > > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > > index de0e726dfdd8..3dc51f407671 100644
> > > > --- a/hw/block/nvme.c
> > > > +++ b/hw/block/nvme.c
> > > > @@ -4876,6 +4876,21 @@ static uint16_t nvme_aer(NvmeCtrl *n, NvmeRequest *req)
> > > >      return NVME_NO_COMPLETE;
> > > >  }
> > > > +static void __nvme_update_dmrsl(NvmeCtrl *n)
> > > > +{
> > > > +    int nsid;
> > > > +
> > > > +    for (nsid = 1; nsid <= NVME_MAX_NAMESPACES; nsid++) {
> > > > +        NvmeNamespace *ns = nvme_ns(n, nsid);
> > > > +        if (!ns) {
> > > > +            continue;
> > > > +        }
> > > > +
> > > > +        n->dmrsl = MIN_NON_ZERO(n->dmrsl,
> > > > +                                BDRV_REQUEST_MAX_BYTES / nvme_l2b(ns, 1));
> > > > +    }
> > > > +}
> > > > +
> > > >  static void __nvme_select_ns_iocs(NvmeCtrl *n, NvmeNamespace *ns);
> > > >  static uint16_t nvme_ns_attachment(NvmeCtrl *n, NvmeRequest *req)
> > > >  {
> > > > @@ -4925,6 +4940,8 @@ static uint16_t nvme_ns_attachment(NvmeCtrl *n, 
> > > > NvmeRequest *req)
> > > >              }
> > > >              nvme_ns_detach(ctrl, ns);
> > > > +
> > > > +            __nvme_update_dmrsl(ctrl);
> > > >          }
> > >
> > > Why the '__' prefix? It doesn't seem clearer (I'm not sure there is
> > > a convention, it makes me think of a internal macro expansion use
> > > for preprocessor).
> > >
> > > There are very few uses of this prefix:
> > >
> > > hw/9pfs/cofs.c:21:static ssize_t __readlink(V9fsState *s, V9fsPath
> > > *path, V9fsString *buf)
> > > hw/block/nvme.c:1683:static uint16_t __nvme_zrm_open(NvmeNamespace *ns,
> > > NvmeZone *zone,
> > > hw/block/nvme.c:1742:static void __nvme_advance_zone_wp(NvmeNamespace
> > > *ns, NvmeZone *zone,
> > > hw/block/nvme.c:5213:static void __nvme_select_ns_iocs(NvmeCtrl *n,
> > > NvmeNamespace *ns)
> > > hw/i386/amd_iommu.c:1160:static int __amdvi_int_remap_msi(AMDVIState *iommu,
> > > hw/intc/s390_flic_kvm.c:255:static int __get_all_irqs(KVMS390FLICState
> > > *flic,
> > > hw/net/rocker/rocker_desc.c:199:static bool
> > > __desc_ring_post_desc(DescRing *ring, int err)
> > > hw/net/sungem.c:766:static uint16_t __sungem_mii_read(SunGEMState *s,
> > > uint8_t phy_addr,
> > > hw/ppc/ppc.c:867:static void __cpu_ppc_store_decr(PowerPCCPU *cpu,
> > > uint64_t *nextp,
> > > hw/s390x/pv.c:25:static int __s390_pv_cmd(uint32_t cmd, const char
> > > *cmdname, void *data)
> > > pc-bios/s390-ccw/cio.c:315:static int __do_cio(SubChannelId schid,
> > > uint32_t ccw_addr, int fmt, Irb *irb)
> > > target/ppc/mmu-hash64.c:170:static void __helper_slbie(CPUPPCState *env,
> > > target_ulong addr,
> > >
> > > Thomas, Eric, is it worth cleaning these and updating the
> > > 'CODESTYLE.rst'?
> >
> > Yeah ok, I think you are right that there is no clear convention on when
> > to use this or not. I typically just use it for functions that are
> > normally not supposed to be called directly.
> >
> > But I don't even think its consistent in the nvme device. For my sake,
> > we can clean it up, I'll drop it in this case since there is no good
> > reason for it other than my own idea of "style".
>
> IIRC all identifiers that start with two underscores are reserved by 
> the C standard:
>
> https://busybox.net/~landley/c99-draft.html#7.1.3
>
> Thus you should not use two underscores at the beginning here at all.

I'll clean up the remaining double underscores in the next cycle!

signature.asc
Description: PGP signature