Re: [PATCH v3 4/7] acpi, nfit: Use bus_dsm_mask for passthru

[Dan Williams]

On Thu, Jun 29, 2017 at 2:47 PM, Jerry Hoemann <jerry.hoemann@xxxxxxx> 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@xxxxxxx> 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@xxxxxxx>
>> > ---
>> >  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.