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

[Thomas Huth <thuth@redhat.com>]

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