| Message ID | 1519205218-26994-10-git-send-email-javier@cnexlabs.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
> 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
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.
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; }
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(-)