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

[Klaus Jensen <its@irrelevant.dk>]

[-- Attachment #1: Type: text/plain, Size: 3457 bytes --]

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".

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]