| Message ID | 1493073410-80159-1-git-send-email-jerry.hoemann@hpe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Mon, Apr 24, 2017 at 3:36 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > nd_cmd_out_size is called by __nd_ioctl to size the buffer passed to > acpi_nfit_ctl. If the DSM function being called has a variable > sized output, nd_cmd_out_size will look at the return field to > determine buffer size. However, the DSM call hasn't been made yet, > so output size is core residue. > > Have nd_cmd_out_size be bimodal with version "early" to be called > before the DSM call is made. For variable sized output fields > have it return ND_IOCTL_MAX_BUFLEN. __nd_ioctl sees new return > values and adjust buffer size accordingly. > > The downside to this approach are: > 1) Requires user buffer input size to be ND_IOCTL_MAX_BUFLEN (4 MB) for > calls to DSM with variable return. > 2) The needless copyin of 4MB > > An alternative approach (not yet prototyped) would move the call > to nd_cmd_out_size until after the return from acpi_nfit_ctl. Here, > the call has been made and return size would be known. > > The size of the buffer allocated in would always be ND_IOCTL_MAX_BUFLEN. > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > --- > drivers/nvdimm/bus.c | 30 ++++++++++++++++++++++++------ > 1 file changed, 24 insertions(+), 6 deletions(-) > > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c > index 23d4a17..50f8cc6 100644 > --- a/drivers/nvdimm/bus.c > +++ b/drivers/nvdimm/bus.c > @@ -713,9 +713,9 @@ u32 nd_cmd_in_size(struct nvdimm *nvdimm, int cmd, > } > EXPORT_SYMBOL_GPL(nd_cmd_in_size); > > -u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, > +u32 nd_cmd_out_size_early(struct nvdimm *nvdimm, int cmd, > const struct nd_cmd_desc *desc, int idx, const u32 *in_field, > - const u32 *out_field, unsigned long remainder) > + const u32 *out_field, unsigned long remainder, int early) > { > if (idx >= desc->out_num) > return UINT_MAX; > @@ -725,9 +725,13 @@ u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, > > if (nvdimm && cmd == ND_CMD_GET_CONFIG_DATA && idx == 1) > return in_field[1]; > - else if (nvdimm && cmd == ND_CMD_VENDOR && idx == 2) > + else if (nvdimm && cmd == ND_CMD_VENDOR && idx == 2) { > + if (early) > + return ND_IOCTL_MAX_BUFLEN; > return out_field[1]; Are you using ndctl_dimm_cmd_new_vendor_specific() to submit a vendor specific command? It already pre-populates out_field[1] with the initial output size.
On Mon, Apr 24, 2017 at 03:56:50PM -0700, Dan Williams wrote: > On Mon, Apr 24, 2017 at 3:36 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > > nd_cmd_out_size is called by __nd_ioctl to size the buffer passed to > > acpi_nfit_ctl. If the DSM function being called has a variable > > sized output, nd_cmd_out_size will look at the return field to > > determine buffer size. However, the DSM call hasn't been made yet, > > so output size is core residue. > > > > Have nd_cmd_out_size be bimodal with version "early" to be called > > before the DSM call is made. For variable sized output fields > > have it return ND_IOCTL_MAX_BUFLEN. __nd_ioctl sees new return > > values and adjust buffer size accordingly. > > > > The downside to this approach are: > > 1) Requires user buffer input size to be ND_IOCTL_MAX_BUFLEN (4 MB) for > > calls to DSM with variable return. > > 2) The needless copyin of 4MB > > > > An alternative approach (not yet prototyped) would move the call > > to nd_cmd_out_size until after the return from acpi_nfit_ctl. Here, > > the call has been made and return size would be known. > > > > The size of the buffer allocated in would always be ND_IOCTL_MAX_BUFLEN. > > > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > > --- > > drivers/nvdimm/bus.c | 30 ++++++++++++++++++++++++------ > > 1 file changed, 24 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c > > index 23d4a17..50f8cc6 100644 > > --- a/drivers/nvdimm/bus.c > > +++ b/drivers/nvdimm/bus.c > > @@ -713,9 +713,9 @@ u32 nd_cmd_in_size(struct nvdimm *nvdimm, int cmd, > > } > > EXPORT_SYMBOL_GPL(nd_cmd_in_size); > > > > -u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, > > +u32 nd_cmd_out_size_early(struct nvdimm *nvdimm, int cmd, > > const struct nd_cmd_desc *desc, int idx, const u32 *in_field, > > - const u32 *out_field, unsigned long remainder) > > + const u32 *out_field, unsigned long remainder, int early) > > { > > if (idx >= desc->out_num) > > return UINT_MAX; > > @@ -725,9 +725,13 @@ u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, > > > > if (nvdimm && cmd == ND_CMD_GET_CONFIG_DATA && idx == 1) > > return in_field[1]; > > - else if (nvdimm && cmd == ND_CMD_VENDOR && idx == 2) > > + else if (nvdimm && cmd == ND_CMD_VENDOR && idx == 2) { > > + if (early) > > + return ND_IOCTL_MAX_BUFLEN; > > return out_field[1]; > > Are you using ndctl_dimm_cmd_new_vendor_specific() to submit a vendor > specific command? It already pre-populates out_field[1] with the > initial output size. I was actually concerned w/ calling the DSM for nd_cmd_ars_status and changed ND_CMD_VENDOR on spec as it looked like it had the same pattern. Hit this using a stand alone test tool I have. For nd_cmd_ars_status, does the caller want to set the output field "num_records" before making the IOCTL call? I.e. setting the maximum number of records the caller is prepared to accept? I took a quick look at the ndctl source and its not obvious that is what the tool is doing. But I might have missed it.
On Mon, Apr 24, 2017 at 05:30:32PM -0600, Jerry Hoemann wrote: > On Mon, Apr 24, 2017 at 03:56:50PM -0700, Dan Williams wrote: > > On Mon, Apr 24, 2017 at 3:36 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > > > nd_cmd_out_size is called by __nd_ioctl to size the buffer passed to > > > acpi_nfit_ctl. If the DSM function being called has a variable > > > sized output, nd_cmd_out_size will look at the return field to > > > determine buffer size. However, the DSM call hasn't been made yet, > > > so output size is core residue. > > > > > > Have nd_cmd_out_size be bimodal with version "early" to be called > > > before the DSM call is made. For variable sized output fields > > > have it return ND_IOCTL_MAX_BUFLEN. __nd_ioctl sees new return > > > values and adjust buffer size accordingly. > > > > > > The downside to this approach are: > > > 1) Requires user buffer input size to be ND_IOCTL_MAX_BUFLEN (4 MB) for > > > calls to DSM with variable return. > > > 2) The needless copyin of 4MB > > > > > > An alternative approach (not yet prototyped) would move the call > > > to nd_cmd_out_size until after the return from acpi_nfit_ctl. Here, > > > the call has been made and return size would be known. > > > > > > The size of the buffer allocated in would always be ND_IOCTL_MAX_BUFLEN. > > > > > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > > > --- > > > drivers/nvdimm/bus.c | 30 ++++++++++++++++++++++++------ > > > 1 file changed, 24 insertions(+), 6 deletions(-) > > > > > > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c > > > index 23d4a17..50f8cc6 100644 > > > --- a/drivers/nvdimm/bus.c > > > +++ b/drivers/nvdimm/bus.c > > > @@ -713,9 +713,9 @@ u32 nd_cmd_in_size(struct nvdimm *nvdimm, int cmd, > > > } > > > EXPORT_SYMBOL_GPL(nd_cmd_in_size); > > > > > > -u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, > > > +u32 nd_cmd_out_size_early(struct nvdimm *nvdimm, int cmd, > > > const struct nd_cmd_desc *desc, int idx, const u32 *in_field, > > > - const u32 *out_field, unsigned long remainder) > > > + const u32 *out_field, unsigned long remainder, int early) > > > { > > > if (idx >= desc->out_num) > > > return UINT_MAX; > > > @@ -725,9 +725,13 @@ u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, > > > > > > if (nvdimm && cmd == ND_CMD_GET_CONFIG_DATA && idx == 1) > > > return in_field[1]; > > > - else if (nvdimm && cmd == ND_CMD_VENDOR && idx == 2) > > > + else if (nvdimm && cmd == ND_CMD_VENDOR && idx == 2) { > > > + if (early) > > > + return ND_IOCTL_MAX_BUFLEN; > > > return out_field[1]; > > > > Are you using ndctl_dimm_cmd_new_vendor_specific() to submit a vendor > > specific command? It already pre-populates out_field[1] with the > > initial output size. > > > I was actually concerned w/ calling the DSM for nd_cmd_ars_status and changed > ND_CMD_VENDOR on spec as it looked like it had the same pattern. > > Hit this using a stand alone test tool I have. > > For nd_cmd_ars_status, does the caller want to set the output field > "num_records" before making the IOCTL call? I.e. setting the maximum number > of records the caller is prepared to accept? sorry, that should have been out_length not num_records. The ioctl caller sets the output field out_length to be maximum return size allowed prior to making the ioctl call. > > I took a quick look at the ndctl source and its not obvious that > is what the tool is doing. But I might have missed it. > > > -- >
On Mon, Apr 24, 2017 at 9:45 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > On Mon, Apr 24, 2017 at 05:30:32PM -0600, Jerry Hoemann wrote: >> On Mon, Apr 24, 2017 at 03:56:50PM -0700, Dan Williams wrote: >> > On Mon, Apr 24, 2017 at 3:36 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: >> > > nd_cmd_out_size is called by __nd_ioctl to size the buffer passed to >> > > acpi_nfit_ctl. If the DSM function being called has a variable >> > > sized output, nd_cmd_out_size will look at the return field to >> > > determine buffer size. However, the DSM call hasn't been made yet, >> > > so output size is core residue. >> > > >> > > Have nd_cmd_out_size be bimodal with version "early" to be called >> > > before the DSM call is made. For variable sized output fields >> > > have it return ND_IOCTL_MAX_BUFLEN. __nd_ioctl sees new return >> > > values and adjust buffer size accordingly. >> > > >> > > The downside to this approach are: >> > > 1) Requires user buffer input size to be ND_IOCTL_MAX_BUFLEN (4 MB) for >> > > calls to DSM with variable return. >> > > 2) The needless copyin of 4MB >> > > >> > > An alternative approach (not yet prototyped) would move the call >> > > to nd_cmd_out_size until after the return from acpi_nfit_ctl. Here, >> > > the call has been made and return size would be known. >> > > >> > > The size of the buffer allocated in would always be ND_IOCTL_MAX_BUFLEN. >> > > >> > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> >> > > --- >> > > drivers/nvdimm/bus.c | 30 ++++++++++++++++++++++++------ >> > > 1 file changed, 24 insertions(+), 6 deletions(-) >> > > >> > > diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c >> > > index 23d4a17..50f8cc6 100644 >> > > --- a/drivers/nvdimm/bus.c >> > > +++ b/drivers/nvdimm/bus.c >> > > @@ -713,9 +713,9 @@ u32 nd_cmd_in_size(struct nvdimm *nvdimm, int cmd, >> > > } >> > > EXPORT_SYMBOL_GPL(nd_cmd_in_size); >> > > >> > > -u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, >> > > +u32 nd_cmd_out_size_early(struct nvdimm *nvdimm, int cmd, >> > > const struct nd_cmd_desc *desc, int idx, const u32 *in_field, >> > > - const u32 *out_field, unsigned long remainder) >> > > + const u32 *out_field, unsigned long remainder, int early) >> > > { >> > > if (idx >= desc->out_num) >> > > return UINT_MAX; >> > > @@ -725,9 +725,13 @@ u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, >> > > >> > > if (nvdimm && cmd == ND_CMD_GET_CONFIG_DATA && idx == 1) >> > > return in_field[1]; >> > > - else if (nvdimm && cmd == ND_CMD_VENDOR && idx == 2) >> > > + else if (nvdimm && cmd == ND_CMD_VENDOR && idx == 2) { >> > > + if (early) >> > > + return ND_IOCTL_MAX_BUFLEN; >> > > return out_field[1]; >> > >> > Are you using ndctl_dimm_cmd_new_vendor_specific() to submit a vendor >> > specific command? It already pre-populates out_field[1] with the >> > initial output size. >> >> >> I was actually concerned w/ calling the DSM for nd_cmd_ars_status and changed >> ND_CMD_VENDOR on spec as it looked like it had the same pattern. >> >> Hit this using a stand alone test tool I have. >> >> For nd_cmd_ars_status, does the caller want to set the output field >> "num_records" before making the IOCTL call? I.e. setting the maximum number >> of records the caller is prepared to accept? > > sorry, that should have been out_length not num_records. > > The ioctl caller sets the output field out_length to be maximum > return size allowed prior to making the ioctl call. > >> >> I took a quick look at the ndctl source and its not obvious that >> is what the tool is doing. But I might have missed it. In the ARS status case since we don't know how big the output will be we always tell the kernel that we can handle the max ars_status payload size. See this line in ndctl_bus_cmd_new_ars_status(): cmd->ars_status->out_length = ars_cap_cmd->max_ars_out;
diff --git a/drivers/nvdimm/bus.c b/drivers/nvdimm/bus.c index 23d4a17..50f8cc6 100644 --- a/drivers/nvdimm/bus.c +++ b/drivers/nvdimm/bus.c @@ -713,9 +713,9 @@ u32 nd_cmd_in_size(struct nvdimm *nvdimm, int cmd, } EXPORT_SYMBOL_GPL(nd_cmd_in_size); -u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, +u32 nd_cmd_out_size_early(struct nvdimm *nvdimm, int cmd, const struct nd_cmd_desc *desc, int idx, const u32 *in_field, - const u32 *out_field, unsigned long remainder) + const u32 *out_field, unsigned long remainder, int early) { if (idx >= desc->out_num) return UINT_MAX; @@ -725,9 +725,13 @@ u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, if (nvdimm && cmd == ND_CMD_GET_CONFIG_DATA && idx == 1) return in_field[1]; - else if (nvdimm && cmd == ND_CMD_VENDOR && idx == 2) + else if (nvdimm && cmd == ND_CMD_VENDOR && idx == 2) { + if (early) + return ND_IOCTL_MAX_BUFLEN; return out_field[1]; - else if (!nvdimm && cmd == ND_CMD_ARS_STATUS && idx == 2) { + } else if (!nvdimm && cmd == ND_CMD_ARS_STATUS && idx == 2) { + if (early) + return ND_IOCTL_MAX_BUFLEN; /* * Per table 9-276 ARS Data in ACPI 6.1, out_field[1] is * "Size of Output Buffer in bytes, including this @@ -753,6 +757,13 @@ u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, return UINT_MAX; } + +u32 nd_cmd_out_size(struct nvdimm *nvdimm, int cmd, + const struct nd_cmd_desc *desc, int idx, const u32 *in_field, + const u32 *out_field, unsigned long remainder) +{ + return nd_cmd_out_size_early(nvdimm, cmd, desc, idx, in_field, out_field, remainder, 0); +} EXPORT_SYMBOL_GPL(nd_cmd_out_size); void wait_nvdimm_bus_probe_idle(struct device *dev) @@ -890,10 +901,17 @@ static int __nd_ioctl(struct nvdimm_bus *nvdimm_bus, struct nvdimm *nvdimm, /* process an output envelope */ for (i = 0; i < desc->out_num; i++) { - u32 out_size = nd_cmd_out_size(nvdimm, cmd, desc, i, - (u32 *) in_env, (u32 *) out_env, 0); + u32 out_size = nd_cmd_out_size_early(nvdimm, cmd, desc, i, + (u32 *) in_env, (u32 *) out_env, 0, 1); u32 copy; + if (out_size == ND_IOCTL_MAX_BUFLEN) { + /* variable sized output */ + out_len = out_size; + out_len -= in_len; + break; + } + if (out_size == UINT_MAX) { dev_dbg(dev, "%s:%s unknown output size cmd: %s field: %d\n", __func__, dimm_name, cmd_name, i);
nd_cmd_out_size is called by __nd_ioctl to size the buffer passed to acpi_nfit_ctl. If the DSM function being called has a variable sized output, nd_cmd_out_size will look at the return field to determine buffer size. However, the DSM call hasn't been made yet, so output size is core residue. Have nd_cmd_out_size be bimodal with version "early" to be called before the DSM call is made. For variable sized output fields have it return ND_IOCTL_MAX_BUFLEN. __nd_ioctl sees new return values and adjust buffer size accordingly. The downside to this approach are: 1) Requires user buffer input size to be ND_IOCTL_MAX_BUFLEN (4 MB) for calls to DSM with variable return. 2) The needless copyin of 4MB An alternative approach (not yet prototyped) would move the call to nd_cmd_out_size until after the return from acpi_nfit_ctl. Here, the call has been made and return size would be known. The size of the buffer allocated in would always be ND_IOCTL_MAX_BUFLEN. Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> --- drivers/nvdimm/bus.c | 30 ++++++++++++++++++++++++------ 1 file changed, 24 insertions(+), 6 deletions(-)