RE: [PATCH 1/8] cxl: Add helper function to retrieve a feature entry

[Shiju Jose]

>-----Original Message-----
>From: Alison Schofield <alison.schofield@xxxxxxxxx>
>Sent: 07 March 2025 19:20
>To: Shiju Jose <shiju.jose@xxxxxxxxxx>
[...]
>> +struct cxl_feat_entry *cxl_get_feature_entry(struct cxl_dev_state *cxlds,
>> +					     const uuid_t *feat_uuid)
>> +{
>> +	struct cxl_features_state *cxlfs = to_cxlfs(cxlds);
>> +	struct cxl_feat_entry *feat_entry;
>> +	int count;
>> +
>> +	/*
>> +	 * Retrieve the feature entry from the supported features list,
>> +	 * if the feature is supported.
>> +	 */
>> +	feat_entry = cxlfs->entries->ent;
>
>Do we need some NULL checking here on cxlfs, entries

Hi Alison,

Thanks for the feedbacks.
We had check on cxlfs before
https://lore.kernel.org/all/20250122235159.2716036-5-dave.jiang@xxxxxxxxx/
but removed because of the following comment.
https://lore.kernel.org/all/20250124150150.GZ5556@xxxxxxxxxx/
>
>
>> +	for (count = 0; count < cxlfs->entries->num_features; count++,
>> +feat_entry++) {
>
>Was num_features previously validated?
Not in the caller. Had check for num_features here before in cxl_get_feature_entry()
as seen in the above link.
>
>> +		if (uuid_equal(&feat_entry->uuid, feat_uuid))
>> +			return feat_entry;
>> +	}
>> +
>> +	return ERR_PTR(-ENOENT);
>
>Why not just return NULL?
Will do.
>
>
>> +}
>> +
>>  size_t cxl_get_feature(struct cxl_mailbox *cxl_mbox, const uuid_t *feat_uuid,
>>  		       enum cxl_get_feat_selection selection,
>>  		       void *feat_out, size_t feat_out_size, u16 offset,
>> --
>> 2.43.0
>>

Thanks,
Shiju