[PATCH v3 12/18] hw/block/nvme: support the get/set features select and save fields
From: Klaus Jensen
Since the device does not have any persistent state storage, no
features are "saveable" and setting the Save (SV) field in any Set
Features command will result in a Feature Identifier Not Saveable status
code.
Similarly, if the Select (SEL) field is set to request saved values, the
devices will (as it should) return the default values instead.
Since this also introduces "Supported Capabilities", the nsid field is
now also checked for validity wrt. the feature being get/set'ed.
Signed-off-by: Klaus Jensen
---
hw/block/nvme.c | 103 +-
hw/block/trace-events | 4 +-
include/block/nvme.h | 27 ++-
3 files changed, 119 insertions(+), 15 deletions(-)
diff --git a/hw/block/nvme.c b/hw/block/nvme.c
index 2d85e853403f..df8b786e4875 100644
--- a/hw/block/nvme.c
+++ b/hw/block/nvme.c
@@ -85,6 +85,14 @@ static const bool nvme_feature_support[NVME_FID_MAX] = {
[NVME_TIMESTAMP]= true,
};
+static const uint32_t nvme_feature_cap[NVME_FID_MAX] = {
+[NVME_TEMPERATURE_THRESHOLD]= NVME_FEAT_CAP_CHANGE,
+[NVME_VOLATILE_WRITE_CACHE] = NVME_FEAT_CAP_CHANGE,
+[NVME_NUMBER_OF_QUEUES] = NVME_FEAT_CAP_CHANGE,
+[NVME_ASYNCHRONOUS_EVENT_CONF] = NVME_FEAT_CAP_CHANGE,
+[NVME_TIMESTAMP]= NVME_FEAT_CAP_CHANGE,
+};
+
static void nvme_process_sq(void *opaque);
static uint16_t nvme_cid(NvmeRequest *req)
@@ -1083,20 +1091,47 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
*cmd, NvmeRequest *req)
{
uint32_t dw10 = le32_to_cpu(cmd->cdw10);
uint32_t dw11 = le32_to_cpu(cmd->cdw11);
+uint32_t nsid = le32_to_cpu(cmd->nsid);
uint32_t result;
uint8_t fid = NVME_GETSETFEAT_FID(dw10);
+NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
uint16_t iv;
static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
[NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
};
-trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
+trace_pci_nvme_getfeat(nvme_cid(req), fid, sel, dw11);
if (!nvme_feature_support[fid]) {
return NVME_INVALID_FIELD | NVME_DNR;
}
+if (nvme_feature_cap[fid] & NVME_FEAT_CAP_NS) {
+if (!nsid || nsid > n->num_namespaces) {
+/*
+ * The Reservation Notification Mask and Reservation Persistence
+ * features require a status code of Invalid Field in Command when
+ * NSID is 0x. Since the device does not support those
+ * features we can always return Invalid Namespace or Format as we
+ * should do for all other features.
+ */
+return NVME_INVALID_NSID | NVME_DNR;
+}
+}
+
+switch (sel) {
+case NVME_GETFEAT_SELECT_CURRENT:
+break;
+case NVME_GETFEAT_SELECT_SAVED:
+/* no features are saveable by the controller; fallthrough */
+case NVME_GETFEAT_SELECT_DEFAULT:
+goto defaults;
+case NVME_GETFEAT_SELECT_CAP:
+result = nvme_feature_cap[fid];
+goto out;
+}
+
switch (fid) {
case NVME_TEMPERATURE_THRESHOLD:
result = 0;
@@ -1106,22 +1141,45 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
*cmd, NvmeRequest *req)
* return 0 for all other sensors.
*/
if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
-break;
+goto out;
}
switch (NVME_TEMP_THSEL(dw11)) {
case NVME_TEMP_THSEL_OVER:
result = n->features.temp_thresh_hi;
-break;
+goto out;
case NVME_TEMP_THSEL_UNDER:
result = n->features.temp_thresh_low;
-break;
+goto out;
}
-break;
+return NVME_INVALID_FIELD | NVME_DNR;
case NVME_VOLATILE_WRITE_CACHE:
result = blk_enable_write_cache(n->conf.blk);
trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
+goto out;
+case NVME_ASYNCHRONOUS_EVENT_CONF:
+result = n->features.async_config;
+goto out;
+case NVME_TIMESTAMP:
+return nvme_get_feature_timestamp(n, cmd);
+default:
+break;
+}
+
+defaults:
+switch (fid) {
+case NVME_TEMPERATURE_THRESHOLD:
+result = 0;
+
+if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
+break;
+}
+
+if (NVME_TEMP_THSEL(dw11) == NVME_TEMP_THSEL_OVER) {
+result = NVME_TEMPERATURE_WARNING;
+}
+
break;
case NVME_NUMBER_OF_QUEUES:
result = (n->params.max_ioqpairs - 1) |
@@ -1140,16 +1198,12 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
*cmd, NvmeRequest *req)
}
break;
-case NVME_ASYNCHRONOUS_EVENT_CONF:
-result = n->features.async_config;
-break;
-case NVME_TIMESTAMP:
-return nvme_get_feature_timestamp(n, cmd);
default:
Re: [PATCH v3 12/18] hw/block/nvme: support the get/set features select and save fields
Looks good,
Reviewed-by: Dmitry Fomichev
On Mon, 2020-07-06 at 08:12 +0200, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Since the device does not have any persistent state storage, no
> features are "saveable" and setting the Save (SV) field in any Set
> Features command will result in a Feature Identifier Not Saveable status
> code.
>
> Similarly, if the Select (SEL) field is set to request saved values, the
> devices will (as it should) return the default values instead.
>
> Since this also introduces "Supported Capabilities", the nsid field is
> now also checked for validity wrt. the feature being get/set'ed.
>
> Signed-off-by: Klaus Jensen
> ---
> hw/block/nvme.c | 103 +-
> hw/block/trace-events | 4 +-
> include/block/nvme.h | 27 ++-
> 3 files changed, 119 insertions(+), 15 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 2d85e853403f..df8b786e4875 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -85,6 +85,14 @@ static const bool nvme_feature_support[NVME_FID_MAX] = {
> [NVME_TIMESTAMP]= true,
> };
>
> +static const uint32_t nvme_feature_cap[NVME_FID_MAX] = {
> +[NVME_TEMPERATURE_THRESHOLD]= NVME_FEAT_CAP_CHANGE,
> +[NVME_VOLATILE_WRITE_CACHE] = NVME_FEAT_CAP_CHANGE,
> +[NVME_NUMBER_OF_QUEUES] = NVME_FEAT_CAP_CHANGE,
> +[NVME_ASYNCHRONOUS_EVENT_CONF] = NVME_FEAT_CAP_CHANGE,
> +[NVME_TIMESTAMP]= NVME_FEAT_CAP_CHANGE,
> +};
> +
> static void nvme_process_sq(void *opaque);
>
> static uint16_t nvme_cid(NvmeRequest *req)
> @@ -1083,20 +1091,47 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> {
> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> +uint32_t nsid = le32_to_cpu(cmd->nsid);
> uint32_t result;
> uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> +NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
> uint16_t iv;
>
> static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
> [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> };
>
> -trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
> +trace_pci_nvme_getfeat(nvme_cid(req), fid, sel, dw11);
>
> if (!nvme_feature_support[fid]) {
> return NVME_INVALID_FIELD | NVME_DNR;
> }
>
> +if (nvme_feature_cap[fid] & NVME_FEAT_CAP_NS) {
> +if (!nsid || nsid > n->num_namespaces) {
> +/*
> + * The Reservation Notification Mask and Reservation Persistence
> + * features require a status code of Invalid Field in Command
> when
> + * NSID is 0x. Since the device does not support those
> + * features we can always return Invalid Namespace or Format as
> we
> + * should do for all other features.
> + */
> +return NVME_INVALID_NSID | NVME_DNR;
> +}
> +}
> +
> +switch (sel) {
> +case NVME_GETFEAT_SELECT_CURRENT:
> +break;
> +case NVME_GETFEAT_SELECT_SAVED:
> +/* no features are saveable by the controller; fallthrough */
> +case NVME_GETFEAT_SELECT_DEFAULT:
> +goto defaults;
> +case NVME_GETFEAT_SELECT_CAP:
> +result = nvme_feature_cap[fid];
> +goto out;
> +}
> +
> switch (fid) {
> case NVME_TEMPERATURE_THRESHOLD:
> result = 0;
> @@ -1106,22 +1141,45 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> * return 0 for all other sensors.
> */
> if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
> -break;
> +goto out;
> }
>
> switch (NVME_TEMP_THSEL(dw11)) {
> case NVME_TEMP_THSEL_OVER:
> result = n->features.temp_thresh_hi;
> -break;
> +goto out;
> case NVME_TEMP_THSEL_UNDER:
> result = n->features.temp_thresh_low;
> -break;
> +goto out;
> }
>
> -break;
> +return NVME_INVALID_FIELD | NVME_DNR;
> case NVME_VOLATILE_WRITE_CACHE:
> result = blk_enable_write_cache(n->conf.blk);
> trace_pci_nvme_getfeat_vwcache(result ? "enabled" : "disabled");
> +goto out;
> +case NVME_ASYNCHRONOUS_EVENT_CONF:
> +result = n->features.async_config;
> +goto out;
> +case NVME_TIMESTAMP:
> +return nvme_get_feature_timestamp(n, cmd);
> +default:
> +break;
> +}
> +
> +defaults:
> +switch (fid) {
> +case NVME_TEMPERATURE_THRESHOLD:
> +result = 0;
> +
> +if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
> +break;
> +}
> +
> +if (NVME_TEMP_THSEL(dw11) == NVME_TEMP_THSEL_OVER) {
> +result = NVME_TEMPERATURE_WARNING;
> +}
> +
> break;
> case NVME_NUMBER_OF_QUEUES:
Re: [PATCH v3 12/18] hw/block/nvme: support the get/set features select and save fields
On Mon, 2020-07-06 at 08:12 +0200, Klaus Jensen wrote:
> From: Klaus Jensen
>
> Since the device does not have any persistent state storage, no
> features are "saveable" and setting the Save (SV) field in any Set
> Features command will result in a Feature Identifier Not Saveable status
> code.
>
> Similarly, if the Select (SEL) field is set to request saved values, the
> devices will (as it should) return the default values instead.
>
> Since this also introduces "Supported Capabilities", the nsid field is
> now also checked for validity wrt. the feature being get/set'ed.
>
> Signed-off-by: Klaus Jensen
> ---
> hw/block/nvme.c | 103 +-
> hw/block/trace-events | 4 +-
> include/block/nvme.h | 27 ++-
> 3 files changed, 119 insertions(+), 15 deletions(-)
>
> diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> index 2d85e853403f..df8b786e4875 100644
> --- a/hw/block/nvme.c
> +++ b/hw/block/nvme.c
> @@ -85,6 +85,14 @@ static const bool nvme_feature_support[NVME_FID_MAX] = {
> [NVME_TIMESTAMP]= true,
> };
>
> +static const uint32_t nvme_feature_cap[NVME_FID_MAX] = {
> +[NVME_TEMPERATURE_THRESHOLD]= NVME_FEAT_CAP_CHANGE,
> +[NVME_VOLATILE_WRITE_CACHE] = NVME_FEAT_CAP_CHANGE,
> +[NVME_NUMBER_OF_QUEUES] = NVME_FEAT_CAP_CHANGE,
> +[NVME_ASYNCHRONOUS_EVENT_CONF] = NVME_FEAT_CAP_CHANGE,
> +[NVME_TIMESTAMP]= NVME_FEAT_CAP_CHANGE,
> +};
> +
> static void nvme_process_sq(void *opaque);
>
> static uint16_t nvme_cid(NvmeRequest *req)
> @@ -1083,20 +1091,47 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> {
> uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> +uint32_t nsid = le32_to_cpu(cmd->nsid);
> uint32_t result;
> uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> +NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
> uint16_t iv;
>
> static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
> [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> };
>
> -trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
> +trace_pci_nvme_getfeat(nvme_cid(req), fid, sel, dw11);
>
> if (!nvme_feature_support[fid]) {
> return NVME_INVALID_FIELD | NVME_DNR;
> }
>
> +if (nvme_feature_cap[fid] & NVME_FEAT_CAP_NS) {
> +if (!nsid || nsid > n->num_namespaces) {
> +/*
> + * The Reservation Notification Mask and Reservation Persistence
> + * features require a status code of Invalid Field in Command
> when
> + * NSID is 0x. Since the device does not support those
> + * features we can always return Invalid Namespace or Format as
> we
> + * should do for all other features.
> + */
> +return NVME_INVALID_NSID | NVME_DNR;
> +}
> +}
> +
> +switch (sel) {
> +case NVME_GETFEAT_SELECT_CURRENT:
> +break;
> +case NVME_GETFEAT_SELECT_SAVED:
> +/* no features are saveable by the controller; fallthrough */
> +case NVME_GETFEAT_SELECT_DEFAULT:
> +goto defaults;
I hate to say it, but while I have nothing against using 'goto' (unlike some
types I met),
In this particular case it feels like it would be better to have a separate
function for
defaults, or have even have a a separate function per feature and have it
return current/default/saved/whatever
value. The later would allow to have each feature self contained in its own
function.
But on the other hand I see that you fail back to defaults for unchangeble
features, which does make
sense. In other words, I don't have strong opinion against using goto here
after all.
When feature code will be getting more features in the future (pun intended)
you probably will have to split it,\
like I suggest to keep code complexity low.
> +case NVME_GETFEAT_SELECT_CAP:
> +result = nvme_feature_cap[fid];
> +goto out;
> +}
> +
> switch (fid) {
> case NVME_TEMPERATURE_THRESHOLD:
> result = 0;
> @@ -1106,22 +1141,45 @@ static uint16_t nvme_get_feature(NvmeCtrl *n, NvmeCmd
> *cmd, NvmeRequest *req)
> * return 0 for all other sensors.
> */
> if (NVME_TEMP_TMPSEL(dw11) != NVME_TEMP_TMPSEL_COMPOSITE) {
> -break;
> +goto out;
> }
>
> switch (NVME_TEMP_THSEL(dw11)) {
> case NVME_TEMP_THSEL_OVER:
> result = n->features.temp_thresh_hi;
> -break;
> +goto out;
> case NVME_TEMP_THSEL_UNDER:
> result = n->features.temp_thresh_low;
> -break;
> +goto out;
> }
>
> -break;
> +return NVME_INVALID_FIELD | NVME_DNR;
> case NVME_VOLATILE_WRITE_CACHE:
> result = blk_enable_write_cache(n->conf.blk);
> trace_pci_nvme_getfeat_vwcac
Re: [PATCH v3 12/18] hw/block/nvme: support the get/set features select and save fields
On Jul 29 16:17, Maxim Levitsky wrote:
> On Mon, 2020-07-06 at 08:12 +0200, Klaus Jensen wrote:
> > From: Klaus Jensen
> >
> > Since the device does not have any persistent state storage, no
> > features are "saveable" and setting the Save (SV) field in any Set
> > Features command will result in a Feature Identifier Not Saveable status
> > code.
> >
> > Similarly, if the Select (SEL) field is set to request saved values, the
> > devices will (as it should) return the default values instead.
> >
> > Since this also introduces "Supported Capabilities", the nsid field is
> > now also checked for validity wrt. the feature being get/set'ed.
> >
> > Signed-off-by: Klaus Jensen
> > ---
> > hw/block/nvme.c | 103 +-
> > hw/block/trace-events | 4 +-
> > include/block/nvme.h | 27 ++-
> > 3 files changed, 119 insertions(+), 15 deletions(-)
> >
> > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > index 2d85e853403f..df8b786e4875 100644
> > --- a/hw/block/nvme.c
> > +++ b/hw/block/nvme.c
> > @@ -1083,20 +1091,47 @@ static uint16_t nvme_get_feature(NvmeCtrl *n,
> > NvmeCmd *cmd, NvmeRequest *req)
> > {
> > uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> > uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> > +uint32_t nsid = le32_to_cpu(cmd->nsid);
> > uint32_t result;
> > uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> > +NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
> > uint16_t iv;
> >
> > static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
> > [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> > };
> >
> > -trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
> > +trace_pci_nvme_getfeat(nvme_cid(req), fid, sel, dw11);
> >
> > if (!nvme_feature_support[fid]) {
> > return NVME_INVALID_FIELD | NVME_DNR;
> > }
> >
> > +if (nvme_feature_cap[fid] & NVME_FEAT_CAP_NS) {
> > +if (!nsid || nsid > n->num_namespaces) {
> > +/*
> > + * The Reservation Notification Mask and Reservation
> > Persistence
> > + * features require a status code of Invalid Field in Command
> > when
> > + * NSID is 0x. Since the device does not support those
> > + * features we can always return Invalid Namespace or Format
> > as we
> > + * should do for all other features.
> > + */
> > +return NVME_INVALID_NSID | NVME_DNR;
> > +}
> > +}
> > +
> > +switch (sel) {
> > +case NVME_GETFEAT_SELECT_CURRENT:
> > +break;
> > +case NVME_GETFEAT_SELECT_SAVED:
> > +/* no features are saveable by the controller; fallthrough */
> > +case NVME_GETFEAT_SELECT_DEFAULT:
> > +goto defaults;
>
> I hate to say it, but while I have nothing against using 'goto' (unlike some
> types I met),
> In this particular case it feels like it would be better to have a separate
> function for
> defaults, or have even have a a separate function per feature and have it
> return current/default/saved/whatever
> value. The later would allow to have each feature self contained in its own
> function.
>
> But on the other hand I see that you fail back to defaults for unchangeble
> features, which does make
> sense. In other words, I don't have strong opinion against using goto here
> after all.
>
> When feature code will be getting more features in the future (pun intended)
> you probably will have to split it,\
> like I suggest to keep code complexity low.
>
Argh... I know you are right.
Since you are "accepting" the current state with your R-b and it already
carries one from Dmitry I think I'll let this stay for now, but I will
fix this in a follow up patch for sure.
> > @@ -926,6 +949,8 @@ typedef struct NvmeLBAF {
> > uint8_t rp;
> > } NvmeLBAF;
> >
> > +#define NVME_NSID_BROADCAST 0x
>
> Cool, you probably want eventually to go over code and
> change all places that use the number to the define.
> (No need to do this now)
>
True. Noted :)
Re: [PATCH v3 12/18] hw/block/nvme: support the get/set features select and save fields
On Wed, 2020-07-29 at 15:48 +0200, Klaus Jensen wrote:
> On Jul 29 16:17, Maxim Levitsky wrote:
> > On Mon, 2020-07-06 at 08:12 +0200, Klaus Jensen wrote:
> > > From: Klaus Jensen
> > >
> > > Since the device does not have any persistent state storage, no
> > > features are "saveable" and setting the Save (SV) field in any Set
> > > Features command will result in a Feature Identifier Not Saveable status
> > > code.
> > >
> > > Similarly, if the Select (SEL) field is set to request saved values, the
> > > devices will (as it should) return the default values instead.
> > >
> > > Since this also introduces "Supported Capabilities", the nsid field is
> > > now also checked for validity wrt. the feature being get/set'ed.
> > >
> > > Signed-off-by: Klaus Jensen
> > > ---
> > > hw/block/nvme.c | 103 +-
> > > hw/block/trace-events | 4 +-
> > > include/block/nvme.h | 27 ++-
> > > 3 files changed, 119 insertions(+), 15 deletions(-)
> > >
> > > diff --git a/hw/block/nvme.c b/hw/block/nvme.c
> > > index 2d85e853403f..df8b786e4875 100644
> > > --- a/hw/block/nvme.c
> > > +++ b/hw/block/nvme.c
> > > @@ -1083,20 +1091,47 @@ static uint16_t nvme_get_feature(NvmeCtrl *n,
> > > NvmeCmd *cmd, NvmeRequest *req)
> > > {
> > > uint32_t dw10 = le32_to_cpu(cmd->cdw10);
> > > uint32_t dw11 = le32_to_cpu(cmd->cdw11);
> > > +uint32_t nsid = le32_to_cpu(cmd->nsid);
> > > uint32_t result;
> > > uint8_t fid = NVME_GETSETFEAT_FID(dw10);
> > > +NvmeGetFeatureSelect sel = NVME_GETFEAT_SELECT(dw10);
> > > uint16_t iv;
> > >
> > > static const uint32_t nvme_feature_default[NVME_FID_MAX] = {
> > > [NVME_ARBITRATION] = NVME_ARB_AB_NOLIMIT,
> > > };
> > >
> > > -trace_pci_nvme_getfeat(nvme_cid(req), fid, dw11);
> > > +trace_pci_nvme_getfeat(nvme_cid(req), fid, sel, dw11);
> > >
> > > if (!nvme_feature_support[fid]) {
> > > return NVME_INVALID_FIELD | NVME_DNR;
> > > }
> > >
> > > +if (nvme_feature_cap[fid] & NVME_FEAT_CAP_NS) {
> > > +if (!nsid || nsid > n->num_namespaces) {
> > > +/*
> > > + * The Reservation Notification Mask and Reservation
> > > Persistence
> > > + * features require a status code of Invalid Field in
> > > Command when
> > > + * NSID is 0x. Since the device does not support
> > > those
> > > + * features we can always return Invalid Namespace or Format
> > > as we
> > > + * should do for all other features.
> > > + */
> > > +return NVME_INVALID_NSID | NVME_DNR;
> > > +}
> > > +}
> > > +
> > > +switch (sel) {
> > > +case NVME_GETFEAT_SELECT_CURRENT:
> > > +break;
> > > +case NVME_GETFEAT_SELECT_SAVED:
> > > +/* no features are saveable by the controller; fallthrough */
> > > +case NVME_GETFEAT_SELECT_DEFAULT:
> > > +goto defaults;
> >
> > I hate to say it, but while I have nothing against using 'goto' (unlike
> > some types I met),
> > In this particular case it feels like it would be better to have a
> > separate function for
> > defaults, or have even have a a separate function per feature and have it
> > return current/default/saved/whatever
> > value. The later would allow to have each feature self contained in its own
> > function.
> >
> > But on the other hand I see that you fail back to defaults for unchangeble
> > features, which does make
> > sense. In other words, I don't have strong opinion against using goto here
> > after all.
> >
> > When feature code will be getting more features in the future (pun
> > intended) you probably will have to split it,\
> > like I suggest to keep code complexity low.
> >
>
> Argh... I know you are right.
>
> Since you are "accepting" the current state with your R-b and it already
> carries one from Dmitry I think I'll let this stay for now, but I will
> fix this in a follow up patch for sure.
Yep, this is exactly what I was thinking.
Best regards,
Maxim Levitsky
>
> > > @@ -926,6 +949,8 @@ typedef struct NvmeLBAF {
> > > uint8_t rp;
> > > } NvmeLBAF;
> > >
> > > +#define NVME_NSID_BROADCAST 0x
> >
> > Cool, you probably want eventually to go over code and
> > change all places that use the number to the define.
> > (No need to do this now)
> >
>
> True. Noted :)
>
