| Message ID | 20210405175452.37578-7-its@irrelevant.dk |
|---|---|
| State | New |
| Headers | show |
| Series | hw/block/nvme: misc fixes | expand |
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'? Phil.
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".
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. HTH, Thomas
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!
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); } /*