Re: [PATCH] platform/x86/intel/vsec: Explicitly enable capabilities

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 3/17/23 13:58, Andy Shevchenko wrote:
> On Fri, Mar 17, 2023 at 12:46 AM David E. Box
> <david.e.box@linux.intel.com> wrote:
>>
>> Discovered Intel VSEC/DVSEC capabilities are enabled by default and only
>> get disabled by quirk. Instead, remove such quirks and only enable support
>> for capabilities that have been explicitly added to a new capabilities
>> field. While here, also reorder the device info structures alphabetically.
> 
> This looks better indeed.
> Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com>

Thank you both for the patch and review, I've applied this patch
to my review-hans branch:
https://git.kernel.org/pub/scm/linux/kernel/git/pdx86/platform-drivers-x86.git/log/?h=review-hans

Note it will show up in my review-hans branch once I've pushed my
local branch there, which might take a while.

Once I've run some tests on this branch the patches there will be
added to the platform-drivers-x86/for-next branch and eventually
will be included in the pdx86 pull-request to Linus for the next
merge-window.

Regards,

Hans

> 
>> Signed-off-by: David E. Box <david.e.box@linux.intel.com>
>> ---
>>  drivers/platform/x86/intel/vsec.c | 69 ++++++++++++++-----------------
>>  drivers/platform/x86/intel/vsec.h |  9 +++-
>>  2 files changed, 38 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/platform/x86/intel/vsec.c b/drivers/platform/x86/intel/vsec.c
>> index 13decf36c6de..01b1f6480e5c 100644
>> --- a/drivers/platform/x86/intel/vsec.c
>> +++ b/drivers/platform/x86/intel/vsec.c
>> @@ -67,14 +67,6 @@ enum intel_vsec_id {
>>         VSEC_ID_TPMI            = 66,
>>  };
>>
>> -static enum intel_vsec_id intel_vsec_allow_list[] = {
>> -       VSEC_ID_TELEMETRY,
>> -       VSEC_ID_WATCHER,
>> -       VSEC_ID_CRASHLOG,
>> -       VSEC_ID_SDSI,
>> -       VSEC_ID_TPMI,
>> -};
>> -
>>  static const char *intel_vsec_name(enum intel_vsec_id id)
>>  {
>>         switch (id) {
>> @@ -98,26 +90,19 @@ static const char *intel_vsec_name(enum intel_vsec_id id)
>>         }
>>  }
>>
>> -static bool intel_vsec_allowed(u16 id)
>> -{
>> -       int i;
>> -
>> -       for (i = 0; i < ARRAY_SIZE(intel_vsec_allow_list); i++)
>> -               if (intel_vsec_allow_list[i] == id)
>> -                       return true;
>> -
>> -       return false;
>> -}
>> -
>> -static bool intel_vsec_disabled(u16 id, unsigned long quirks)
>> +static bool intel_vsec_supported(u16 id, unsigned long caps)
>>  {
>>         switch (id) {
>> +       case VSEC_ID_TELEMETRY:
>> +               return !!(caps & VSEC_CAP_TELEMETRY);
>>         case VSEC_ID_WATCHER:
>> -               return !!(quirks & VSEC_QUIRK_NO_WATCHER);
>> -
>> +               return !!(caps & VSEC_CAP_WATCHER);
>>         case VSEC_ID_CRASHLOG:
>> -               return !!(quirks & VSEC_QUIRK_NO_CRASHLOG);
>> -
>> +               return !!(caps & VSEC_CAP_CRASHLOG);
>> +       case VSEC_ID_SDSI:
>> +               return !!(caps & VSEC_CAP_SDSI);
>> +       case VSEC_ID_TPMI:
>> +               return !!(caps & VSEC_CAP_TPMI);
>>         default:
>>                 return false;
>>         }
>> @@ -205,7 +190,7 @@ static int intel_vsec_add_dev(struct pci_dev *pdev, struct intel_vsec_header *he
>>         unsigned long quirks = info->quirks;
>>         int i;
>>
>> -       if (!intel_vsec_allowed(header->id) || intel_vsec_disabled(header->id, quirks))
>> +       if (!intel_vsec_supported(header->id, info->caps))
>>                 return -EINVAL;
>>
>>         if (!header->num_entries) {
>> @@ -260,14 +245,14 @@ static int intel_vsec_add_dev(struct pci_dev *pdev, struct intel_vsec_header *he
>>  static bool intel_vsec_walk_header(struct pci_dev *pdev,
>>                                    struct intel_vsec_platform_info *info)
>>  {
>> -       struct intel_vsec_header **header = info->capabilities;
>> +       struct intel_vsec_header **header = info->headers;
>>         bool have_devices = false;
>>         int ret;
>>
>>         for ( ; *header; header++) {
>>                 ret = intel_vsec_add_dev(pdev, *header, info);
>>                 if (ret)
>> -                       dev_info(&pdev->dev, "Could not add device for DVSEC id %d\n",
>> +                       dev_info(&pdev->dev, "Could not add device for VSEC id %d\n",
>>                                  (*header)->id);
>>                 else
>>                         have_devices = true;
>> @@ -402,14 +387,8 @@ static int intel_vsec_pci_probe(struct pci_dev *pdev, const struct pci_device_id
>>         return 0;
>>  }
>>
>> -/* TGL info */
>> -static const struct intel_vsec_platform_info tgl_info = {
>> -       .quirks = VSEC_QUIRK_NO_WATCHER | VSEC_QUIRK_NO_CRASHLOG |
>> -                 VSEC_QUIRK_TABLE_SHIFT | VSEC_QUIRK_EARLY_HW,
>> -};
>> -
>>  /* DG1 info */
>> -static struct intel_vsec_header dg1_telemetry = {
>> +static struct intel_vsec_header dg1_header = {
>>         .length = 0x10,
>>         .id = 2,
>>         .num_entries = 1,
>> @@ -418,19 +397,31 @@ static struct intel_vsec_header dg1_telemetry = {
>>         .offset = 0x466000,
>>  };
>>
>> -static struct intel_vsec_header *dg1_capabilities[] = {
>> -       &dg1_telemetry,
>> +static struct intel_vsec_header *dg1_headers[] = {
>> +       &dg1_header,
>>         NULL
>>  };
>>
>>  static const struct intel_vsec_platform_info dg1_info = {
>> -       .capabilities = dg1_capabilities,
>> +       .caps = VSEC_CAP_TELEMETRY,
>> +       .headers = dg1_headers,
>>         .quirks = VSEC_QUIRK_NO_DVSEC | VSEC_QUIRK_EARLY_HW,
>>  };
>>
>>  /* MTL info */
>>  static const struct intel_vsec_platform_info mtl_info = {
>> -       .quirks = VSEC_QUIRK_NO_WATCHER | VSEC_QUIRK_NO_CRASHLOG,
>> +       .caps = VSEC_CAP_TELEMETRY,
>> +};
>> +
>> +/* OOBMSM info */
>> +static const struct intel_vsec_platform_info oobmsm_info = {
>> +       .caps = VSEC_CAP_TELEMETRY | VSEC_CAP_SDSI | VSEC_CAP_TPMI,
>> +};
>> +
>> +/* TGL info */
>> +static const struct intel_vsec_platform_info tgl_info = {
>> +       .caps = VSEC_CAP_TELEMETRY,
>> +       .quirks = VSEC_QUIRK_TABLE_SHIFT | VSEC_QUIRK_EARLY_HW,
>>  };
>>
>>  #define PCI_DEVICE_ID_INTEL_VSEC_ADL           0x467d
>> @@ -445,7 +436,7 @@ static const struct pci_device_id intel_vsec_pci_ids[] = {
>>         { PCI_DEVICE_DATA(INTEL, VSEC_DG1, &dg1_info) },
>>         { PCI_DEVICE_DATA(INTEL, VSEC_MTL_M, &mtl_info) },
>>         { PCI_DEVICE_DATA(INTEL, VSEC_MTL_S, &mtl_info) },
>> -       { PCI_DEVICE_DATA(INTEL, VSEC_OOBMSM, &(struct intel_vsec_platform_info) {}) },
>> +       { PCI_DEVICE_DATA(INTEL, VSEC_OOBMSM, &oobmsm_info) },
>>         { PCI_DEVICE_DATA(INTEL, VSEC_RPL, &tgl_info) },
>>         { PCI_DEVICE_DATA(INTEL, VSEC_TGL, &tgl_info) },
>>         { }
>> diff --git a/drivers/platform/x86/intel/vsec.h b/drivers/platform/x86/intel/vsec.h
>> index ae8fe92c5595..0fd042c171ba 100644
>> --- a/drivers/platform/x86/intel/vsec.h
>> +++ b/drivers/platform/x86/intel/vsec.h
>> @@ -5,6 +5,12 @@
>>  #include <linux/auxiliary_bus.h>
>>  #include <linux/bits.h>
>>
>> +#define VSEC_CAP_TELEMETRY     BIT(0)
>> +#define VSEC_CAP_WATCHER       BIT(1)
>> +#define VSEC_CAP_CRASHLOG      BIT(2)
>> +#define VSEC_CAP_SDSI          BIT(3)
>> +#define VSEC_CAP_TPMI          BIT(4)
>> +
>>  struct pci_dev;
>>  struct resource;
>>
>> @@ -27,7 +33,8 @@ enum intel_vsec_quirks {
>>
>>  /* Platform specific data */
>>  struct intel_vsec_platform_info {
>> -       struct intel_vsec_header **capabilities;
>> +       struct intel_vsec_header **headers;
>> +       unsigned long caps;
>>         unsigned long quirks;
>>  };
>>
>>
>> base-commit: 02c464b73645404654359ad21f368a13735e2850
>> --
>> 2.34.1
>>
> 
>