| Message ID | f332778c416d01fd76ce8e474acd583e1b787c29.1498754314.git.jerry.hoemann@hpe.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Jun 29, 2017 at 9:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > Populate bus_dsm_mask and use it to filter dsm calls that user can > make through the pass thru interface. > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > --- > drivers/acpi/nfit/core.c | 5 +++++ > 1 file changed, 5 insertions(+) > > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > index b46fca2..971002b 100644 > --- a/drivers/acpi/nfit/core.c > +++ b/drivers/acpi/nfit/core.c > @@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm, > cmd_name = nvdimm_bus_cmd_name(cmd); > cmd_mask = nd_desc->cmd_mask; > dsm_mask = cmd_mask; > + if (cmd == ND_CMD_CALL) > + dsm_mask = nd_desc->bus_dsm_mask; > desc = nd_cmd_bus_desc(cmd); > uuid = to_nfit_uuid(NFIT_DEV_BUS); > handle = adev->handle; > @@ -1624,6 +1626,9 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i)) > set_bit(i, &nd_desc->cmd_mask); > set_bit(ND_CMD_CALL, &nd_desc->cmd_mask); > + for (i = 0; i < ND_CMD_CALL; i++) > + if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i)) > + set_bit(i, &nd_desc->bus_dsm_mask); > } This loop checks for function 6 which is specified as reserved. Lets explicitly test for the known good function numbers with something like this: /* this should be private in drivers/acpi/nfit/nfit.h */ enum nfit_aux_cmds { NFIT_CMD_TRANSLATE_SPA = 5, NFIT_CMD_ARS_INJECT_SET = 7, NFIT_CMD_ARS_INJECT_CLEAR = 8, NFIT_CMD_ARS_INJECT_GET = 9, }; bus_dsm_mask = (1 << ND_CMD_ARS_CAP) | (1 << ND_CMD_ARS_START) | (1 << ND_CMD_ARS_STATUS) | (1 << ND_CMD_CLEAR_ERROR) | (1 << NFIT_CMD_TRANSLATE_SPA) | (1 << NFIT_CMD_ARS_INJECT_SET) | (1 << NFIT_CMD_ARS_INJECT_CLEAR) | (1 << NFIT_CMD_ARS_INJECT_GET); for_each_set_bit(i, &bus_dsm_mask...
On Thu, Jun 29, 2017 at 02:35:14PM -0700, Dan Williams wrote: > On Thu, Jun 29, 2017 at 9:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > > Populate bus_dsm_mask and use it to filter dsm calls that user can > > make through the pass thru interface. > > > > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > > --- > > drivers/acpi/nfit/core.c | 5 +++++ > > 1 file changed, 5 insertions(+) > > > > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > > index b46fca2..971002b 100644 > > --- a/drivers/acpi/nfit/core.c > > +++ b/drivers/acpi/nfit/core.c > > @@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm, > > cmd_name = nvdimm_bus_cmd_name(cmd); > > cmd_mask = nd_desc->cmd_mask; > > dsm_mask = cmd_mask; > > + if (cmd == ND_CMD_CALL) > > + dsm_mask = nd_desc->bus_dsm_mask; > > desc = nd_cmd_bus_desc(cmd); > > uuid = to_nfit_uuid(NFIT_DEV_BUS); > > handle = adev->handle; > > @@ -1624,6 +1626,9 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) > > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i)) > > set_bit(i, &nd_desc->cmd_mask); > > set_bit(ND_CMD_CALL, &nd_desc->cmd_mask); > > + for (i = 0; i < ND_CMD_CALL; i++) > > + if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i)) > > + set_bit(i, &nd_desc->bus_dsm_mask); > > } > > This loop checks for function 6 which is specified as reserved. Lets > explicitly test for the known good function numbers with something > like this: > > /* this should be private in drivers/acpi/nfit/nfit.h */ > enum nfit_aux_cmds { > NFIT_CMD_TRANSLATE_SPA = 5, > NFIT_CMD_ARS_INJECT_SET = 7, > NFIT_CMD_ARS_INJECT_CLEAR = 8, > NFIT_CMD_ARS_INJECT_GET = 9, > }; > > bus_dsm_mask = > (1 << ND_CMD_ARS_CAP) | > (1 << ND_CMD_ARS_START) | > (1 << ND_CMD_ARS_STATUS) | > (1 << ND_CMD_CLEAR_ERROR) | > (1 << NFIT_CMD_TRANSLATE_SPA) | > (1 << NFIT_CMD_ARS_INJECT_SET) | > (1 << NFIT_CMD_ARS_INJECT_CLEAR) | > (1 << NFIT_CMD_ARS_INJECT_GET); > > for_each_set_bit(i, &bus_dsm_mask... I added the for_each_set_bit check in patch 7 of the series.
On Thu, Jun 29, 2017 at 2:47 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > On Thu, Jun 29, 2017 at 02:35:14PM -0700, Dan Williams wrote: >> On Thu, Jun 29, 2017 at 9:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: >> > Populate bus_dsm_mask and use it to filter dsm calls that user can >> > make through the pass thru interface. >> > >> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> >> > --- >> > drivers/acpi/nfit/core.c | 5 +++++ >> > 1 file changed, 5 insertions(+) >> > >> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c >> > index b46fca2..971002b 100644 >> > --- a/drivers/acpi/nfit/core.c >> > +++ b/drivers/acpi/nfit/core.c >> > @@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm, >> > cmd_name = nvdimm_bus_cmd_name(cmd); >> > cmd_mask = nd_desc->cmd_mask; >> > dsm_mask = cmd_mask; >> > + if (cmd == ND_CMD_CALL) >> > + dsm_mask = nd_desc->bus_dsm_mask; >> > desc = nd_cmd_bus_desc(cmd); >> > uuid = to_nfit_uuid(NFIT_DEV_BUS); >> > handle = adev->handle; >> > @@ -1624,6 +1626,9 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) >> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i)) >> > set_bit(i, &nd_desc->cmd_mask); >> > set_bit(ND_CMD_CALL, &nd_desc->cmd_mask); >> > + for (i = 0; i < ND_CMD_CALL; i++) >> > + if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i)) >> > + set_bit(i, &nd_desc->bus_dsm_mask); >> > } >> >> This loop checks for function 6 which is specified as reserved. Lets >> explicitly test for the known good function numbers with something >> like this: >> >> /* this should be private in drivers/acpi/nfit/nfit.h */ >> enum nfit_aux_cmds { >> NFIT_CMD_TRANSLATE_SPA = 5, >> NFIT_CMD_ARS_INJECT_SET = 7, >> NFIT_CMD_ARS_INJECT_CLEAR = 8, >> NFIT_CMD_ARS_INJECT_GET = 9, >> }; >> >> bus_dsm_mask = >> (1 << ND_CMD_ARS_CAP) | >> (1 << ND_CMD_ARS_START) | >> (1 << ND_CMD_ARS_STATUS) | >> (1 << ND_CMD_CLEAR_ERROR) | >> (1 << NFIT_CMD_TRANSLATE_SPA) | >> (1 << NFIT_CMD_ARS_INJECT_SET) | >> (1 << NFIT_CMD_ARS_INJECT_CLEAR) | >> (1 << NFIT_CMD_ARS_INJECT_GET); >> >> for_each_set_bit(i, &bus_dsm_mask... > > > I added the for_each_set_bit check in patch 7 of the series. True, but in a patch series we shouldn't introduce a bug in one patch and fix it later in the same series. Also, if patch7 goes away we would need to fold that enabling in here. Part of trying to parse what 0x3bf meant lead me to this, and I'm wondering if we should do the same for the other magic vendor dsm_mask constants, but that's a patch for another time.
On Thu, Jun 29, 2017 at 02:55:55PM -0700, Dan Williams wrote: > On Thu, Jun 29, 2017 at 2:47 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > > On Thu, Jun 29, 2017 at 02:35:14PM -0700, Dan Williams wrote: > >> On Thu, Jun 29, 2017 at 9:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > >> > Populate bus_dsm_mask and use it to filter dsm calls that user can > >> > make through the pass thru interface. > >> > > >> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> > >> > --- > >> > drivers/acpi/nfit/core.c | 5 +++++ > >> > 1 file changed, 5 insertions(+) > >> > > >> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c > >> > index b46fca2..971002b 100644 > >> > --- a/drivers/acpi/nfit/core.c > >> > +++ b/drivers/acpi/nfit/core.c > >> > @@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm, > >> > cmd_name = nvdimm_bus_cmd_name(cmd); > >> > cmd_mask = nd_desc->cmd_mask; > >> > dsm_mask = cmd_mask; > >> > + if (cmd == ND_CMD_CALL) > >> > + dsm_mask = nd_desc->bus_dsm_mask; > >> > desc = nd_cmd_bus_desc(cmd); > >> > uuid = to_nfit_uuid(NFIT_DEV_BUS); > >> > handle = adev->handle; > >> > @@ -1624,6 +1626,9 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) > >> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i)) > >> > set_bit(i, &nd_desc->cmd_mask); > >> > set_bit(ND_CMD_CALL, &nd_desc->cmd_mask); > >> > + for (i = 0; i < ND_CMD_CALL; i++) > >> > + if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i)) > >> > + set_bit(i, &nd_desc->bus_dsm_mask); > >> > } > >> > >> This loop checks for function 6 which is specified as reserved. Lets > >> explicitly test for the known good function numbers with something > >> like this: > >> > >> /* this should be private in drivers/acpi/nfit/nfit.h */ > >> enum nfit_aux_cmds { > >> NFIT_CMD_TRANSLATE_SPA = 5, > >> NFIT_CMD_ARS_INJECT_SET = 7, > >> NFIT_CMD_ARS_INJECT_CLEAR = 8, > >> NFIT_CMD_ARS_INJECT_GET = 9, > >> }; > >> > >> bus_dsm_mask = > >> (1 << ND_CMD_ARS_CAP) | > >> (1 << ND_CMD_ARS_START) | > >> (1 << ND_CMD_ARS_STATUS) | > >> (1 << ND_CMD_CLEAR_ERROR) | > >> (1 << NFIT_CMD_TRANSLATE_SPA) | > >> (1 << NFIT_CMD_ARS_INJECT_SET) | > >> (1 << NFIT_CMD_ARS_INJECT_CLEAR) | > >> (1 << NFIT_CMD_ARS_INJECT_GET); > >> > >> for_each_set_bit(i, &bus_dsm_mask... > > > > > > I added the for_each_set_bit check in patch 7 of the series. > > True, but in a patch series we shouldn't introduce a bug in one patch > and fix it later in the same series. Also, if patch7 goes away we > would need to fold that enabling in here. > A bug? What bad thing happens?
On Thu, Jun 29, 2017 at 4:26 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: > On Thu, Jun 29, 2017 at 02:55:55PM -0700, Dan Williams wrote: >> On Thu, Jun 29, 2017 at 2:47 PM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: >> > On Thu, Jun 29, 2017 at 02:35:14PM -0700, Dan Williams wrote: >> >> On Thu, Jun 29, 2017 at 9:56 AM, Jerry Hoemann <jerry.hoemann@hpe.com> wrote: >> >> > Populate bus_dsm_mask and use it to filter dsm calls that user can >> >> > make through the pass thru interface. >> >> > >> >> > Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> >> >> > --- >> >> > drivers/acpi/nfit/core.c | 5 +++++ >> >> > 1 file changed, 5 insertions(+) >> >> > >> >> > diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c >> >> > index b46fca2..971002b 100644 >> >> > --- a/drivers/acpi/nfit/core.c >> >> > +++ b/drivers/acpi/nfit/core.c >> >> > @@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm, >> >> > cmd_name = nvdimm_bus_cmd_name(cmd); >> >> > cmd_mask = nd_desc->cmd_mask; >> >> > dsm_mask = cmd_mask; >> >> > + if (cmd == ND_CMD_CALL) >> >> > + dsm_mask = nd_desc->bus_dsm_mask; >> >> > desc = nd_cmd_bus_desc(cmd); >> >> > uuid = to_nfit_uuid(NFIT_DEV_BUS); >> >> > handle = adev->handle; >> >> > @@ -1624,6 +1626,9 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) >> >> > if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i)) >> >> > set_bit(i, &nd_desc->cmd_mask); >> >> > set_bit(ND_CMD_CALL, &nd_desc->cmd_mask); >> >> > + for (i = 0; i < ND_CMD_CALL; i++) >> >> > + if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i)) >> >> > + set_bit(i, &nd_desc->bus_dsm_mask); >> >> > } >> >> >> >> This loop checks for function 6 which is specified as reserved. Lets >> >> explicitly test for the known good function numbers with something >> >> like this: >> >> >> >> /* this should be private in drivers/acpi/nfit/nfit.h */ >> >> enum nfit_aux_cmds { >> >> NFIT_CMD_TRANSLATE_SPA = 5, >> >> NFIT_CMD_ARS_INJECT_SET = 7, >> >> NFIT_CMD_ARS_INJECT_CLEAR = 8, >> >> NFIT_CMD_ARS_INJECT_GET = 9, >> >> }; >> >> >> >> bus_dsm_mask = >> >> (1 << ND_CMD_ARS_CAP) | >> >> (1 << ND_CMD_ARS_START) | >> >> (1 << ND_CMD_ARS_STATUS) | >> >> (1 << ND_CMD_CLEAR_ERROR) | >> >> (1 << NFIT_CMD_TRANSLATE_SPA) | >> >> (1 << NFIT_CMD_ARS_INJECT_SET) | >> >> (1 << NFIT_CMD_ARS_INJECT_CLEAR) | >> >> (1 << NFIT_CMD_ARS_INJECT_GET); >> >> >> >> for_each_set_bit(i, &bus_dsm_mask... >> > >> > >> > I added the for_each_set_bit check in patch 7 of the series. >> >> True, but in a patch series we shouldn't introduce a bug in one patch >> and fix it later in the same series. Also, if patch7 goes away we >> would need to fold that enabling in here. >> > > A bug? What bad thing happens? > Ok, yes, not a bug, but in general terms if the review feedback is "patch4 has an issue" the answer (usually) can't be "but I I fix it in patch7". That instead means the fix in patch7 needs to move to patch4. So this is purely a Linux patch-review / process comment.
diff --git a/drivers/acpi/nfit/core.c b/drivers/acpi/nfit/core.c index b46fca2..971002b 100644 --- a/drivers/acpi/nfit/core.c +++ b/drivers/acpi/nfit/core.c @@ -253,6 +253,8 @@ int acpi_nfit_ctl(struct nvdimm_bus_descriptor *nd_desc, struct nvdimm *nvdimm, cmd_name = nvdimm_bus_cmd_name(cmd); cmd_mask = nd_desc->cmd_mask; dsm_mask = cmd_mask; + if (cmd == ND_CMD_CALL) + dsm_mask = nd_desc->bus_dsm_mask; desc = nd_cmd_bus_desc(cmd); uuid = to_nfit_uuid(NFIT_DEV_BUS); handle = adev->handle; @@ -1624,6 +1626,9 @@ static void acpi_nfit_init_dsms(struct acpi_nfit_desc *acpi_desc) if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i)) set_bit(i, &nd_desc->cmd_mask); set_bit(ND_CMD_CALL, &nd_desc->cmd_mask); + for (i = 0; i < ND_CMD_CALL; i++) + if (acpi_check_dsm(adev->handle, uuid, 1, 1ULL << i)) + set_bit(i, &nd_desc->bus_dsm_mask); } static ssize_t range_index_show(struct device *dev,
Populate bus_dsm_mask and use it to filter dsm calls that user can make through the pass thru interface. Signed-off-by: Jerry Hoemann <jerry.hoemann@hpe.com> --- drivers/acpi/nfit/core.c | 5 +++++ 1 file changed, 5 insertions(+)