linux-kernel - Re: [PATCH V2 05/20] nvdimm/region_label: Add region label updation routine

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20250910150322.00001ea4@huawei.com>
Date: Wed, 10 Sep 2025 15:03:22 +0100
From: Jonathan Cameron <jonathan.cameron@...wei.com>
To: Neeraj Kumar <s.neeraj@...sung.com>
CC: Dave Jiang <dave.jiang@...el.com>, <linux-cxl@...r.kernel.org>,
	<nvdimm@...ts.linux.dev>, <linux-kernel@...r.kernel.org>,
	<gost.dev@...sung.com>, <a.manzanares@...sung.com>, <vishak.g@...sung.com>,
	<neeraj.kernel@...il.com>, <cpgs@...sung.com>
Subject: Re: [PATCH V2 05/20] nvdimm/region_label: Add region label updation
 routine

On Thu, 4 Sep 2025 19:42:31 +0530
Neeraj Kumar <s.neeraj@...sung.com> wrote:

> On 15/08/25 02:55PM, Dave Jiang wrote:
> >
> >
> >On 7/30/25 5:11 AM, Neeraj Kumar wrote:  
> >> Added __pmem_region_label_update region label update routine to update
> >> region label.
> >>
> >> Also used guard(mutex)(&nd_mapping->lock) in place of mutex_lock() and
> >> mutex_unlock()
> >>
> >> Signed-off-by: Neeraj Kumar <s.neeraj@...sung.com>  
> >
> >Subject, s/updation/update/ ?  
> 
> Thanks Dave, Sure. Will fix it in next patch-set
> 

Hi Neeraj,
 
Really small process point.  We all get too many emails, so
when replying crop out anything that doesn't need more discussion.
Where you are just saying you agree, leave that for the change logs of the
next version (and appropriate thanks can go there as well).

I know not replying can feel little rude, but trust me when I say all
or almost all who review a lot appreciate efficiency!

It also makes it a lot harder to miss the more substantial replies.

> >> +int nd_pmem_region_label_update(struct nd_region *nd_region)
> >> +{
> >> +	int i, rc;
> >> +
> >> +	for (i = 0; i < nd_region->ndr_mappings; i++) {
> >> +		struct nd_mapping *nd_mapping = &nd_region->mapping[i];
> >> +		struct nvdimm_drvdata *ndd = to_ndd(nd_mapping);
> >> +
> >> +		/* No need to update region label for non cxl format */
> >> +		if (!ndd->cxl)
> >> +			continue;  
> >
> >Would there be a mix of different nd mappings? I wonder if you can just 'return 0' if you find ndd->cxl on the first one and just skip everything.  
> 
> When we create cxl region with two mem device, then we will have two separate
> nd_mapping for both mem devices. But Yes, I don't see difference in both device
> nd_mapping characters. So instead of "continue", I will just "return 0".
> 

This for instance is good discussion and well worth the reply!

Good discussion is great, but reducing the noise is key to keeping
things manageable.

Jonathan

p.s. I'm having a full day or reviewing code so getting a little grumpier
than I was first thing this morning when I might not have given this feedback!