lkml.org 
[lkml]   [2018]   [Feb]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH 09/20] lightnvm: use generic identify structure
From
Date
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.

\
 
 \ /
  Last update: 2018-02-22 10:42    [W:0.072 / U:1.120 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site