Re: [PATCH 09/20] lightnvm: use generic identify structure

["Matias Bjørling" <mb@lightnvm.io>]

On 02/22/2018 08:49 AM, Javier González wrote:
>> On 22 Feb 2018, at 08.47, Matias Bjørling <mb@lightnvm.io> wrote:
>>
>> On 02/21/2018 10:26 AM, Javier González wrote:
>>> Create a generic identify structure to collect the identify information
>>> before knowing the spec. version. This forces different version paths to
>>> cast the structure to their spec structure, thus making the code less
>>> error prone and more maintainable.
>>> Signed-off-by: Javier González <javier@cnexlabs.com>
>>> ---
>>>   drivers/nvme/host/lightnvm.c | 32 ++++++++++++++++++++------------
>>>   1 file changed, 20 insertions(+), 12 deletions(-)
>>> diff --git a/drivers/nvme/host/lightnvm.c b/drivers/nvme/host/lightnvm.c
>>> index 9c1f8225c4e1..70dc4740f0d3 100644
>>> --- a/drivers/nvme/host/lightnvm.c
>>> +++ b/drivers/nvme/host/lightnvm.c
>>> @@ -170,6 +170,12 @@ struct nvme_nvm_id12 {
>>>   	__u8			resv2[2880];
>>>   } __packed;
>>>   +/* Generic identification structure */
>>> +struct nvme_nvm_id {
>>> +	__u8			ver_id;
>>> +	__u8			resv[4095];
>>> +} __packed;
>>> +
>>>   struct nvme_nvm_bb_tbl {
>>>   	__u8	tblid[4];
>>>   	__le16	verid;
>>> @@ -279,9 +285,10 @@ static void nvme_nvm_set_addr_12(struct nvm_addr_format_12 *dst,
>>>   	dst->sec_mask = ((1ULL << dst->sec_len) - 1) << dst->sec_offset;
>>>   }
>>>   -static int nvme_nvm_setup_12(struct nvme_nvm_id12 *id,
>>> +static int nvme_nvm_setup_12(struct nvme_nvm_id *gen_id,
>>>   			     struct nvm_dev_geo *dev_geo)
>>>   {
>>> +	struct nvme_nvm_id12 *id = (struct nvme_nvm_id12 *)gen_id;
>>>   	struct nvme_nvm_id12_grp *src;
>>>   	int sec_per_pg, sec_per_pl, pg_per_blk;
>>>   @@ -380,9 +387,11 @@ static void nvme_nvm_set_addr_20(struct nvm_addr_format *dst,
>>>   	dst->sec_mask = ((1ULL << dst->sec_len) - 1) << dst->sec_offset;
>>>   }
>>>   -static int nvme_nvm_setup_20(struct nvme_nvm_id20 *id,
>>> +static int nvme_nvm_setup_20(struct nvme_nvm_id *gen_id,
>>>   			     struct nvm_dev_geo *dev_geo)
>>>   {
>>> +	struct nvme_nvm_id20 *id = (struct nvme_nvm_id20 *)gen_id;
>>> +
>>>   	dev_geo->major_ver_id = id->mjr;
>>>   	dev_geo->minor_ver_id = id->mnr;
>>>   @@ -427,19 +436,19 @@ static int nvme_nvm_setup_20(struct nvme_nvm_id20 *id,
>>>   static int nvme_nvm_identity(struct nvm_dev *nvmdev)
>>>   {
>>>   	struct nvme_ns *ns = nvmdev->q->queuedata;
>>> -	struct nvme_nvm_id12 *id;
>>> +	struct nvme_nvm_id *nvme_nvm_id;
>>>   	struct nvme_nvm_command c = {};
>>>   	int ret;
>>>     	c.identity.opcode = nvme_nvm_admin_identity;
>>>   	c.identity.nsid = cpu_to_le32(ns->head->ns_id);
>>>   -	id = kmalloc(sizeof(struct nvme_nvm_id12), GFP_KERNEL);
>>> -	if (!id)
>>> +	nvme_nvm_id = kmalloc(sizeof(struct nvme_nvm_id), GFP_KERNEL);
>>> +	if (!nvme_nvm_id)
>>>   		return -ENOMEM;
>>>     	ret = nvme_submit_sync_cmd(ns->ctrl->admin_q, (struct nvme_command *)&c,
>>> -				id, sizeof(struct nvme_nvm_id12));
>>> +				nvme_nvm_id, sizeof(struct nvme_nvm_id));
>>>   	if (ret) {
>>>   		ret = -EIO;
>>>   		goto out;
>>> @@ -449,22 +458,21 @@ static int nvme_nvm_identity(struct nvm_dev *nvmdev)
>>>   	 * The 1.2 and 2.0 specifications share the first byte in their geometry
>>>   	 * command to make it possible to know what version a device implements.
>>>   	 */
>>> -	switch (id->ver_id) {
>>> +	switch (nvme_nvm_id->ver_id) {
>>>   	case 1:
>>> -		ret = nvme_nvm_setup_12(id, &nvmdev->dev_geo);
>>> +		ret = nvme_nvm_setup_12(nvme_nvm_id, &nvmdev->dev_geo);
>>>   		break;
>>>   	case 2:
>>> -		ret = nvme_nvm_setup_20((struct nvme_nvm_id20 *)id,
>>> -							&nvmdev->dev_geo);
>>> +		ret = nvme_nvm_setup_20(nvme_nvm_id, &nvmdev->dev_geo);
>>>   		break;
>>>   	default:
>>>   		dev_err(ns->ctrl->device, "OCSSD revision not supported (%d)\n",
>>> -							id->ver_id);
>>> +							nvme_nvm_id->ver_id);
>>>   		ret = -EINVAL;
>>>   	}
>>>     out:
>>> -	kfree(id);
>>> +	kfree(nvme_nvm_id);
>>>   	return ret;
>>>   }
>>>
>>
>> Thanks for another way to represent it. I want to keep the original
>> path. If we are going that down road, then one should maybe look into
>> unifying the "three" data structures, and have the version as the base
>> property and the others in each their sub data structure.
> 
> That's what this structure aims to do, since the version is shared. But
> you call if you want to cast unrelated structures back an forth. I'll
> revert it...
> 
> Javier
> 

In that case, the patch could look like

struct nvm_id {
	__u8 ver_id;

	union {
		struct v1 {};
		struct v2 {};
	};
};

I'm good with just having the single cast instead of putting each under 
a v1/v2 variable or using nvm_id first, and then goto either v1 or v2.