Re: [PATCH V5 04/17] nvdimm/label: Include region label in slot validation

[Neeraj Kumar <s.neeraj@samsung.com>]

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

On 14/01/26 03:20PM, Ira Weiny wrote:
>Neeraj Kumar wrote:
>> Prior to LSA 2.1 Support, label in slot means only namespace
>> label. But with LSA 2.1 a label can be either namespace or
>> region label.
>>
>> Slot validation routine validates label slot by calculating
>> label checksum. It was only validating namespace label.
>> This changeset also validates region label if present.
>>
>> In previous patch to_lsa_label() was introduced along with
>> to_label(). to_label() returns only namespace label whereas
>> to_lsa_label() returns union nd_lsa_label*
>>
>> In this patch We have converted all usage of to_label()
>
>NIT: don't use 'We'

Fixed it in V6

>
>> to to_lsa_label()
>>
>> Reviewed-by: Jonathan Cameron <jonathan.cameron@huawei.com>
>> Reviewed-by: Dave Jiang <dave.jiang@intel.com>
>> Signed-off-by: Neeraj Kumar <s.neeraj@samsung.com>
>> ---
>>  drivers/nvdimm/label.c | 94 ++++++++++++++++++++++++++++--------------
>>  1 file changed, 64 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/nvdimm/label.c b/drivers/nvdimm/label.c
>> index 17e2a1f5a6da..9854cb45fb62 100644
>> --- a/drivers/nvdimm/label.c
>> +++ b/drivers/nvdimm/label.c
>> @@ -312,16 +312,6 @@ static union nd_lsa_label *to_lsa_label(struct nvdimm_drvdata *ndd, int slot)
>>  	return (union nd_lsa_label *) label;
>>  }
>>
>> -static struct nd_namespace_label *to_label(struct nvdimm_drvdata *ndd, int slot)
>> -{
>> -	unsigned long label, base;
>> -
>> -	base = (unsigned long) nd_label_base(ndd);
>> -	label = base + sizeof_namespace_label(ndd) * slot;
>> -
>> -	return (struct nd_namespace_label *) label;
>> -}
>> -
>>  #define for_each_clear_bit_le(bit, addr, size) \
>>  	for ((bit) = find_next_zero_bit_le((addr), (size), 0);  \
>>  	     (bit) < (size);                                    \
>> @@ -382,7 +372,7 @@ static bool nsl_validate_checksum(struct nvdimm_drvdata *ndd,
>>  {
>>  	u64 sum, sum_save;
>>
>> -	if (!ndd->cxl && !efi_namespace_label_has(ndd, checksum))
>> +	if (!efi_namespace_label_has(ndd, checksum))
>
>What does this change have to do with region label validation during slot
>validation?
>
>>  		return true;
>>
>>  	sum_save = nsl_get_checksum(ndd, nd_label);
>> @@ -397,13 +387,25 @@ static void nsl_calculate_checksum(struct nvdimm_drvdata *ndd,
>>  {
>>  	u64 sum;
>>
>> -	if (!ndd->cxl && !efi_namespace_label_has(ndd, checksum))
>> +	if (!efi_namespace_label_has(ndd, checksum))
>
>This and the above seem like cleanups because efi_namespace_label_has()
>already checks !ndd->cxl?  Was that the intent?  Perhaps as a separate
>cleanup?

Hi Ira,

Actually above is required changes and not the cleanup. 
Earlier condition (!ndd->cxl && !efi_namespace_label_has(ndd, checksum))
was getting true (no further processing) in first case only if its a
region label which we don't want in current case.

And this condition (!efi_namespace_label_has(ndd, checksum)) returns false
(means proceed further) in case if its a region label (!!ndd->cxl).



Regards,
Neeraj

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]