* [PATCH] acpi/nfit: Fix command-supported detection
@ 2019-01-11 23:59 Dan Williams
2019-01-14 7:21 ` Pandel, Sujith
2019-01-14 15:19 ` Jeff Moyer
0 siblings, 2 replies; 6+ messages in thread
From: Dan Williams @ 2019-01-11 23:59 UTC (permalink / raw)
To: linux-nvdimm; +Cc: stable, Sujith Pandel, linux-kernel, vishal.l.verma
The _DSM function number validation only happens to succeed when the
generic Linux command number translation corresponds with a
DSM-family-specific function number. This breaks NVDIMM-N
implementations that correctly implement _LSR, _LSW, and _LSI, but do
not happen to publish support for DSM function numbers 4, 5, and 6.
Recall that the support for _LS{I,R,W} family of methods results in the
DIMM being marked as supporting those command numbers at
acpi_nfit_register_dimms() time. The DSM function mask is only used for
ND_CMD_CALL support of non-NVDIMM_FAMILY_INTEL devices.
Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command...")
Cc: <stable@vger.kernel.org>
Link: https://github.com/pmem/ndctl/issues/78
Reported-by: Sujith Pandel <sujith_pandel@dell.com>
Signed-off-by: Dan Williams <dan.j.williams@intel.com>
---
Sujith, this is a larger change than what you originally tested, but it
should behave the same. I wanted to consolidate all the code that
handles Linux command number to DIMM _DSM function number translation.
If you have a chance to re-test with this it would be much appreciated.
Thanks for the report!
drivers/acpi/nfit/core.c | 43 +++++++++++++++++++++++++++++--------------
1 file changed, 29 insertions(+), 14 deletions(-)
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
index 790691d9a982..d5d64e90ae71 100644
--- a/drivers/acpi/nfit/core.c
+++ b/drivers/acpi/nfit/core.c
@@ -409,6 +409,29 @@ static bool payload_dumpable(struct nvdimm *nvdimm, unsigned int func)
return true;
}
+static int cmd_to_func(struct nvdimm *nvdimm, unsigned int cmd,
+ struct nd_cmd_pkg *call_pkg)
+{
+ struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
+
+ if (cmd == ND_CMD_CALL) {
+ int i;
+
+ if (call_pkg && nfit_mem->family != call_pkg->nd_family)
+ return -ENOTTY;
+
+ for (i = 0; i < ARRAY_SIZE(call_pkg->nd_reserved2); i++)
+ if (call_pkg->nd_reserved2[i])
+ return -EINVAL;
+ return call_pkg->nd_command;
+ }
+
+ /* Linux ND commands == NVDIMM_FAMILY_INTEL function numbers */
+ if (nfit_mem->family == NVDIMM_FAMILY_INTEL)
+ return cmd;
+ return 0;
+}
+
int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
unsigned int cmd, void *buf, unsigned int buf_len, int *cmd_rc)
{
@@ -422,30 +445,21 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
unsigned long cmd_mask, dsm_mask;
u32 offset, fw_status = 0;
acpi_handle handle;
- unsigned int func;
const guid_t *guid;
- int rc, i;
+ int func, rc, i;
if (cmd_rc)
*cmd_rc = -EINVAL;
- func = cmd;
- if (cmd == ND_CMD_CALL) {
- call_pkg = buf;
- func = call_pkg->nd_command;
-
- for (i = 0; i < ARRAY_SIZE(call_pkg->nd_reserved2); i++)
- if (call_pkg->nd_reserved2[i])
- return -EINVAL;
- }
if (nvdimm) {
struct acpi_device *adev = nfit_mem->adev;
if (!adev)
return -ENOTTY;
- if (call_pkg && nfit_mem->family != call_pkg->nd_family)
- return -ENOTTY;
+ func = cmd_to_func(nvdimm, cmd, buf);
+ if (func < 0)
+ return func;
dimm_name = nvdimm_name(nvdimm);
cmd_name = nvdimm_cmd_name(cmd);
cmd_mask = nvdimm_cmd_mask(nvdimm);
@@ -456,6 +470,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
} else {
struct acpi_device *adev = to_acpi_dev(acpi_desc);
+ func = cmd;
cmd_name = nvdimm_bus_cmd_name(cmd);
cmd_mask = nd_desc->cmd_mask;
dsm_mask = cmd_mask;
@@ -470,7 +485,7 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm,
if (!desc || (cmd && (desc->out_num + desc->in_num == 0)))
return -ENOTTY;
- if (!test_bit(cmd, &cmd_mask) || !test_bit(func, &dsm_mask))
+ if (!test_bit(cmd, &cmd_mask) && !test_bit(func, &dsm_mask))
return -ENOTTY;
in_obj.type = ACPI_TYPE_PACKAGE;
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] acpi/nfit: Fix command-supported detection
2019-01-11 23:59 [PATCH] acpi/nfit: Fix command-supported detection Dan Williams
@ 2019-01-14 7:21 ` Pandel, Sujith
2019-01-14 15:19 ` Jeff Moyer
1 sibling, 0 replies; 6+ messages in thread
From: Pandel, Sujith @ 2019-01-14 7:21 UTC (permalink / raw)
To: Dan Williams, linux-nvdimm; +Cc: stable, linux-kernel, vishal.l.verma
Hi Dan,
On 12-Jan-19 5:29 AM, Dan Williams wrote:
> The _DSM function number validation only happens to succeed when the
> generic Linux command number translation corresponds with a
> DSM-family-specific function number. This breaks NVDIMM-N
> implementations that correctly implement _LSR, _LSW, and _LSI, but do
> not happen to publish support for DSM function numbers 4, 5, and 6.
>
> Recall that the support for _LS{I,R,W} family of methods results in the
> DIMM being marked as supporting those command numbers at
> acpi_nfit_register_dimms() time. The DSM function mask is only used for
> ND_CMD_CALL support of non-NVDIMM_FAMILY_INTEL devices.
>
> Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command...")
> Cc: <stable@vger.kernel.org>
> Link: https://github.com/pmem/ndctl/issues/78
> Reported-by: Sujith Pandel <sujith_pandel@dell.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> Sujith, this is a larger change than what you originally tested, but it
> should behave the same. I wanted to consolidate all the code that
> handles Linux command number to DIMM _DSM function number translation.
>
> If you have a chance to re-test with this it would be much appreciated.
>
> Thanks for the report!
Re-verified by applying this patch over LTS kernel-4.20.2 with ndctl v62.
namespace creation and modification works fine on Dell NVDIMM-Ns.
Thanks for helping me on this!
Tested-by: Sujith Pandel <sujith_pandel@dell.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] acpi/nfit: Fix command-supported detection
2019-01-11 23:59 [PATCH] acpi/nfit: Fix command-supported detection Dan Williams
2019-01-14 7:21 ` Pandel, Sujith
@ 2019-01-14 15:19 ` Jeff Moyer
2019-01-14 16:43 ` Dan Williams
1 sibling, 1 reply; 6+ messages in thread
From: Jeff Moyer @ 2019-01-14 15:19 UTC (permalink / raw)
To: Dan Williams; +Cc: linux-nvdimm, Sujith Pandel, linux-kernel, stable
Dan Williams <dan.j.williams@intel.com> writes:
> The _DSM function number validation only happens to succeed when the
> generic Linux command number translation corresponds with a
> DSM-family-specific function number. This breaks NVDIMM-N
> implementations that correctly implement _LSR, _LSW, and _LSI, but do
> not happen to publish support for DSM function numbers 4, 5, and 6.
>
> Recall that the support for _LS{I,R,W} family of methods results in the
> DIMM being marked as supporting those command numbers at
> acpi_nfit_register_dimms() time. The DSM function mask is only used for
> ND_CMD_CALL support of non-NVDIMM_FAMILY_INTEL devices.
>
> Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command...")
> Cc: <stable@vger.kernel.org>
> Link: https://github.com/pmem/ndctl/issues/78
> Reported-by: Sujith Pandel <sujith_pandel@dell.com>
> Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> ---
> Sujith, this is a larger change than what you originally tested, but it
> should behave the same. I wanted to consolidate all the code that
> handles Linux command number to DIMM _DSM function number translation.
>
> If you have a chance to re-test with this it would be much appreciated.
>
> Thanks for the report!
>
> drivers/acpi/nfit/core.c | 43 +++++++++++++++++++++++++++++--------------
> 1 file changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> index 790691d9a982..d5d64e90ae71 100644
> --- a/drivers/acpi/nfit/core.c
> +++ b/drivers/acpi/nfit/core.c
> @@ -409,6 +409,29 @@ static bool payload_dumpable(struct nvdimm *nvdimm, unsigned int func)
> return true;
> }
>
> +static int cmd_to_func(struct nvdimm *nvdimm, unsigned int cmd,
> + struct nd_cmd_pkg *call_pkg)
> +{
> + struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
Minor nit: Seems like the function could take an nfit_mem parameter instead of an nvdimm.
> +
> + if (cmd == ND_CMD_CALL) {
> + int i;
> +
> + if (call_pkg && nfit_mem->family != call_pkg->nd_family)
> + return -ENOTTY;
> +
> + for (i = 0; i < ARRAY_SIZE(call_pkg->nd_reserved2); i++)
> + if (call_pkg->nd_reserved2[i])
> + return -EINVAL;
> + return call_pkg->nd_command;
> + }
> +
> + /* Linux ND commands == NVDIMM_FAMILY_INTEL function numbers */
> + if (nfit_mem->family == NVDIMM_FAMILY_INTEL)
> + return cmd;
> + return 0;
Function zero? Is that really the right thing to return here?
Cheers,
Jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] acpi/nfit: Fix command-supported detection
2019-01-14 15:19 ` Jeff Moyer
@ 2019-01-14 16:43 ` Dan Williams
2019-01-14 16:47 ` Dan Williams
0 siblings, 1 reply; 6+ messages in thread
From: Dan Williams @ 2019-01-14 16:43 UTC (permalink / raw)
To: Jeff Moyer; +Cc: linux-nvdimm, Sujith Pandel, Linux Kernel Mailing List, stable
On Mon, Jan 14, 2019 at 7:19 AM Jeff Moyer <jmoyer@redhat.com> wrote:
>
> Dan Williams <dan.j.williams@intel.com> writes:
>
> > The _DSM function number validation only happens to succeed when the
> > generic Linux command number translation corresponds with a
> > DSM-family-specific function number. This breaks NVDIMM-N
> > implementations that correctly implement _LSR, _LSW, and _LSI, but do
> > not happen to publish support for DSM function numbers 4, 5, and 6.
> >
> > Recall that the support for _LS{I,R,W} family of methods results in the
> > DIMM being marked as supporting those command numbers at
> > acpi_nfit_register_dimms() time. The DSM function mask is only used for
> > ND_CMD_CALL support of non-NVDIMM_FAMILY_INTEL devices.
> >
> > Fixes: 31eca76ba2fc ("nfit, libnvdimm: limited/whitelisted dimm command...")
> > Cc: <stable@vger.kernel.org>
> > Link: https://github.com/pmem/ndctl/issues/78
> > Reported-by: Sujith Pandel <sujith_pandel@dell.com>
> > Signed-off-by: Dan Williams <dan.j.williams@intel.com>
> > ---
> > Sujith, this is a larger change than what you originally tested, but it
> > should behave the same. I wanted to consolidate all the code that
> > handles Linux command number to DIMM _DSM function number translation.
> >
> > If you have a chance to re-test with this it would be much appreciated.
> >
> > Thanks for the report!
> >
> > drivers/acpi/nfit/core.c | 43 +++++++++++++++++++++++++++++--------------
> > 1 file changed, 29 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c
> > index 790691d9a982..d5d64e90ae71 100644
> > --- a/drivers/acpi/nfit/core.c
> > +++ b/drivers/acpi/nfit/core.c
> > @@ -409,6 +409,29 @@ static bool payload_dumpable(struct nvdimm *nvdimm, unsigned int func)
> > return true;
> > }
> >
> > +static int cmd_to_func(struct nvdimm *nvdimm, unsigned int cmd,
> > + struct nd_cmd_pkg *call_pkg)
> > +{
> > + struct nfit_mem *nfit_mem = nvdimm_provider_data(nvdimm);
>
> Minor nit: Seems like the function could take an nfit_mem parameter instead of an nvdimm.
I was making it symmetric with payload_dumpable()... but not for any
good reason, will change.
>
> > +
> > + if (cmd == ND_CMD_CALL) {
> > + int i;
> > +
> > + if (call_pkg && nfit_mem->family != call_pkg->nd_family)
> > + return -ENOTTY;
> > +
> > + for (i = 0; i < ARRAY_SIZE(call_pkg->nd_reserved2); i++)
> > + if (call_pkg->nd_reserved2[i])
> > + return -EINVAL;
> > + return call_pkg->nd_command;
> > + }
> > +
> > + /* Linux ND commands == NVDIMM_FAMILY_INTEL function numbers */
> > + if (nfit_mem->family == NVDIMM_FAMILY_INTEL)
> > + return cmd;
> > + return 0;
>
> Function zero? Is that really the right thing to return here?
Yes, function zero is never set in n
>
> Cheers,
> Jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] acpi/nfit: Fix command-supported detection
2019-01-14 16:43 ` Dan Williams
@ 2019-01-14 16:47 ` Dan Williams
2019-01-14 20:51 ` Jeff Moyer
0 siblings, 1 reply; 6+ messages in thread
From: Dan Williams @ 2019-01-14 16:47 UTC (permalink / raw)
To: Jeff Moyer; +Cc: linux-nvdimm, Sujith Pandel, Linux Kernel Mailing List, stable
On Mon, Jan 14, 2019 at 8:43 AM Dan Williams <dan.j.williams@intel.com> wrote:
> On Mon, Jan 14, 2019 at 7:19 AM Jeff Moyer <jmoyer@redhat.com> wrote:
[..]
> > > +
> > > + if (cmd == ND_CMD_CALL) {
> > > + int i;
> > > +
> > > + if (call_pkg && nfit_mem->family != call_pkg->nd_family)
> > > + return -ENOTTY;
> > > +
> > > + for (i = 0; i < ARRAY_SIZE(call_pkg->nd_reserved2); i++)
> > > + if (call_pkg->nd_reserved2[i])
> > > + return -EINVAL;
> > > + return call_pkg->nd_command;
> > > + }
> > > +
> > > + /* Linux ND commands == NVDIMM_FAMILY_INTEL function numbers */
> > > + if (nfit_mem->family == NVDIMM_FAMILY_INTEL)
> > > + return cmd;
> > > + return 0;
> >
> > Function zero? Is that really the right thing to return here?
>
> Yes, function zero is never set in n
...whoops fumble fingered "send"
Function zero should never be set in nfit_mem->dsm_mask, although the
NVDIMM_FAMILY_MSFT mask violates this assumption. I'll fix that up.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] acpi/nfit: Fix command-supported detection
2019-01-14 16:47 ` Dan Williams
@ 2019-01-14 20:51 ` Jeff Moyer
0 siblings, 0 replies; 6+ messages in thread
From: Jeff Moyer @ 2019-01-14 20:51 UTC (permalink / raw)
To: Dan Williams
Cc: linux-nvdimm, Sujith Pandel, Linux Kernel Mailing List, stable
Dan Williams <dan.j.williams@intel.com> writes:
> On Mon, Jan 14, 2019 at 8:43 AM Dan Williams <dan.j.williams@intel.com> wrote:
>> On Mon, Jan 14, 2019 at 7:19 AM Jeff Moyer <jmoyer@redhat.com> wrote:
> [..]
>> > > +
>> > > + if (cmd == ND_CMD_CALL) {
>> > > + int i;
>> > > +
>> > > + if (call_pkg && nfit_mem->family != call_pkg->nd_family)
>> > > + return -ENOTTY;
>> > > +
>> > > + for (i = 0; i < ARRAY_SIZE(call_pkg->nd_reserved2); i++)
>> > > + if (call_pkg->nd_reserved2[i])
>> > > + return -EINVAL;
>> > > + return call_pkg->nd_command;
>> > > + }
>> > > +
>> > > + /* Linux ND commands == NVDIMM_FAMILY_INTEL function numbers */
>> > > + if (nfit_mem->family == NVDIMM_FAMILY_INTEL)
>> > > + return cmd;
>> > > + return 0;
>> >
>> > Function zero? Is that really the right thing to return here?
>>
>> Yes, function zero is never set in n
>
> ...whoops fumble fingered "send"
>
> Function zero should never be set in nfit_mem->dsm_mask, although the
> NVDIMM_FAMILY_MSFT mask violates this assumption. I'll fix that up.
OK, I think I see how it all fits together now, thanks. It would be
nice if you documented this magical 0 return somehow.
Cheers,
Jeff
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-01-14 20:51 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-11 23:59 [PATCH] acpi/nfit: Fix command-supported detection Dan Williams
2019-01-14 7:21 ` Pandel, Sujith
2019-01-14 15:19 ` Jeff Moyer
2019-01-14 16:43 ` Dan Williams
2019-01-14 16:47 ` Dan Williams
2019-01-14 20:51 ` Jeff Moyer
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).