Re: [PATCH v3 pci] PCI/MSI: pci-xgene-msi: Enable MSI support in ACPI boot for X-Gene v1

[Khuong Dinh]

Hi Marc,

On Mon, Aug 14, 2017 at 1:21 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote:
> On 12/08/17 01:02, Khuong Dinh wrote:
>> This patch makes pci-xgene-msi driver ACPI-aware and provides
>> MSI capability for X-Gene v1 PCIe controllers in ACPI boot mode.
>>
>> Signed-off-by: Khuong Dinh <kdinh@xxxxxxx>
>>         [Take over from Duc Dang]
>> Acked-by: Marc Zyngier <marc.zyngier@xxxxxxx>
>
> Given that the patch has changed very substantially, this Ack is no
> longer valid.

Yes, I'll remove it.

>> ---
>> v3:
>>  - Input X-Gene MSI base address for irq_domain_alloc_fwnode
>>  - Add a hook to enforce X-Gene MSI be probed prior acpi_bus_scan happens
>> v2:
>>  - Verify with BIOS version 3.06.25 and 3.07.09
>> v1:
>>  - Initial version
>> ---
>>  drivers/acpi/Makefile            |    2 +-
>>  drivers/acpi/acpi_msi.c          |   74 ++++++++++++++++++++++++++++++++++++++
>>  drivers/acpi/internal.h          |    1 +
>>  drivers/acpi/scan.c              |    1 +
>>  drivers/pci/host/pci-xgene-msi.c |   36 +++++++++++++++++--
>>  5 files changed, 110 insertions(+), 4 deletions(-)
>>  create mode 100644 drivers/acpi/acpi_msi.c
>>
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index b1aacfc..c15f5cd 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -40,7 +40,7 @@ acpi-y                              += ec.o
>>  acpi-$(CONFIG_ACPI_DOCK)     += dock.o
>>  acpi-y                               += pci_root.o pci_link.o pci_irq.o
>>  obj-$(CONFIG_ACPI_MCFG)              += pci_mcfg.o
>> -acpi-y                               += acpi_lpss.o acpi_apd.o
>> +acpi-y                               += acpi_lpss.o acpi_apd.o acpi_msi.o
>>  acpi-y                               += acpi_platform.o
>>  acpi-y                               += acpi_pnp.o
>>  acpi-$(CONFIG_ARM_AMBA)      += acpi_amba.o
>> diff --git a/drivers/acpi/acpi_msi.c b/drivers/acpi/acpi_msi.c
>> new file mode 100644
>> index 0000000..c2f8d26
>> --- /dev/null
>> +++ b/drivers/acpi/acpi_msi.c
>> @@ -0,0 +1,74 @@
>> +/*
>> + * Enforce MSI driver loaded by PCIe controller driver
>> + *
>> + * Copyright (c) 2017, MACOM Technology Solutions Corporation
>> + * Author: Khuong Dinh <kdinh@xxxxxxx>
>> + *
>> + * This program is free software; you can redistribute  it and/or modify it
>> + * under  the terms of  the GNU General  Public License as published by the
>> + * Free Software Foundation;  either version 2 of the  License, or (at your
>> + * option) any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> + * GNU General Public License for more details.
>> + */
>> +
>> +#include <linux/acpi.h>
>> +#include "internal.h"
>> +
>> +static acpi_status acpi_create_msi_device(acpi_handle handle, u32 Level,
>> +                                     void *context, void **retval)
>> +{
>> +     struct acpi_device *device = NULL;
>> +     int type = ACPI_BUS_TYPE_DEVICE;
>> +     unsigned long long sta;
>> +     acpi_status status;
>> +     int ret = 0;
>> +
>> +     acpi_bus_get_device(handle, &device);
>> +     status = acpi_bus_get_status_handle(handle, &sta);
>> +     if (ACPI_FAILURE(status))
>> +             sta = 0;
>> +
>> +     device = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
>> +     if (!device)
>> +             return -ENOMEM;
>> +
>> +     acpi_init_device_object(device, handle, type, sta);
>> +     ret = acpi_device_add(device, NULL);
>> +     if (ret)
>> +             return ret;
>
> Hello, memory leak.

I'll update the code to release the memory when the error happens.

>> +     device->parent = kzalloc(sizeof(struct acpi_device), GFP_KERNEL);
>> +     INIT_LIST_HEAD(&device->parent->physical_node_list);
>> +
>> +     acpi_device_add_finalize(device);
>> +
>> +     ret = device_attach(&device->dev);
>> +     if (ret < 0)
>> +             return ret;
>
> And another one.

I'll update the code to release the memory when the error happens.

>> +
>> +     acpi_create_platform_device(device, NULL);
>> +     acpi_device_set_enumerated(device);
>> +
>> +     return ret;
>> +}
>> +
>> +static const struct acpi_device_id acpi_msi_device_ids[] = {
>> +     {"APMC0D0E", 0},
>
> Isn't this an APM-specific thing? Is that supposed to be populated by
> all MSI controller drivers? If so, this would better be served by a
> separate linker section.

The code is generic for MSI controller drivers.
It will walk through the HID to get the MSI device and enforce it
happens before acpi_bus_scan.
This mechanism will help to avoid MSI driver from relying on ACPI
device nodes ordering.
For now, it just supports for APM X-Gene v1.

>> +     { }
>> +};
>> +
>> +int __init acpi_msi_init(void)
>> +{
>> +     acpi_status status;
>> +     int ret = 0;
>> +
>> +     status = acpi_get_devices(acpi_msi_device_ids[0].id,
>> +                     acpi_create_msi_device, NULL, NULL);
>> +     if (ACPI_FAILURE(status))
>> +             ret = -ENODEV;
>> +
>> +     return ret;
>> +}
>
> Overall, this part should be a separate patch, and you should start by
> explaining what it does exactly. Because so far, all I can see is that
> it pre-populates random ACPI device structures, and that definitely seem
> fishy to me.

I'm still following Lorenzo's approach to resolve the general ACPI
dependence issue.
As soon as it's fine, I'll re-visit my prior suggestion to separate
the original patch to enable ACPI support for PCIe MSI and the general
ACPI dependence issue.
The patch is to add a hook in drivers/acpi/scan.c to enforce MSI
driver to be initialized and registered before acpi_bus_scan happens,
so that MSI driver will not rely on ACPI device nodes ordering.
It will walk through the HID to get the MSI device (e.g:
acip_get_devices). When the device is found, the callback will be
called and we will initialize MSI device by its handle, start to
attach MSI device to a driver, create ACPI platform device for MSI
APCI device node and marked it as visited.
In result, it will happen on top of acpi_bus_scan.

>> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
>> index 58dd7ab..3da50d3 100644
>> --- a/drivers/acpi/internal.h
>> +++ b/drivers/acpi/internal.h
>> @@ -80,6 +80,7 @@ int acpi_scan_add_handler_with_hotplug(struct acpi_scan_handler *handler,
>>  void acpi_lpss_init(void);
>>
>>  void acpi_apd_init(void);
>> +int acpi_msi_init(void);
>>
>>  acpi_status acpi_hotplug_schedule(struct acpi_device *adev, u32 src);
>>  bool acpi_queue_hotplug_work(struct work_struct *work);
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 3389729..8ec4d39 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -2029,6 +2029,7 @@ int __init acpi_scan_init(void)
>>       acpi_status status;
>>       struct acpi_table_stao *stao_ptr;
>>
>> +     acpi_msi_init();
>>       acpi_pci_root_init();
>>       acpi_pci_link_init();
>>       acpi_processor_init();
>> diff --git a/drivers/pci/host/pci-xgene-msi.c b/drivers/pci/host/pci-xgene-msi.c
>> index f1b633b..b1768a1 100644
>> --- a/drivers/pci/host/pci-xgene-msi.c
>> +++ b/drivers/pci/host/pci-xgene-msi.c
>> @@ -24,6 +24,7 @@
>>  #include <linux/pci.h>
>>  #include <linux/platform_device.h>
>>  #include <linux/of_pci.h>
>> +#include <linux/acpi.h>
>>
>>  #define MSI_IR0                      0x000000
>>  #define MSI_INT0             0x800000
>> @@ -39,7 +40,7 @@ struct xgene_msi_group {
>>  };
>>
>>  struct xgene_msi {
>> -     struct device_node      *node;
>> +     struct fwnode_handle    *fwnode;
>>       struct irq_domain       *inner_domain;
>>       struct irq_domain       *msi_domain;
>>       u64                     msi_addr;
>> @@ -249,6 +250,13 @@ static void xgene_irq_domain_free(struct irq_domain *domain,
>>       .free   = xgene_irq_domain_free,
>>  };
>>
>> +#ifdef CONFIG_ACPI
>> +static struct fwnode_handle *xgene_msi_get_fwnode(struct device *dev)
>> +{
>> +     return xgene_msi_ctrl.fwnode;
>> +}
>> +#endif
>> +
>>  static int xgene_allocate_domains(struct xgene_msi *msi)
>>  {
>>       msi->inner_domain = irq_domain_add_linear(NULL, NR_MSI_VEC,
>> @@ -256,7 +264,7 @@ static int xgene_allocate_domains(struct xgene_msi *msi)
>>       if (!msi->inner_domain)
>>               return -ENOMEM;
>>
>> -     msi->msi_domain = pci_msi_create_irq_domain(of_node_to_fwnode(msi->node),
>> +     msi->msi_domain = pci_msi_create_irq_domain(msi->fwnode,
>>                                                   &xgene_msi_domain_info,
>>                                                   msi->inner_domain);
>>
>> @@ -265,6 +273,9 @@ static int xgene_allocate_domains(struct xgene_msi *msi)
>>               return -ENOMEM;
>>       }
>>
>> +#ifdef CONFIG_ACPI
>> +     pci_msi_register_fwnode_provider(&xgene_msi_get_fwnode);
>> +#endif
>>       return 0;
>>  }
>>
>> @@ -449,6 +460,13 @@ static int xgene_msi_hwirq_free(unsigned int cpu)
>>       {},
>>  };
>>
>> +#ifdef CONFIG_ACPI
>> +static const struct acpi_device_id xgene_msi_acpi_ids[] = {
>> +     {"APMC0D0E", 0},
>> +     { },
>> +};
>> +#endif
>> +
>>  static int xgene_msi_probe(struct platform_device *pdev)
>>  {
>>       struct resource *res;
>> @@ -469,7 +487,18 @@ static int xgene_msi_probe(struct platform_device *pdev)
>>               goto error;
>>       }
>>       xgene_msi->msi_addr = res->start;
>> -     xgene_msi->node = pdev->dev.of_node;
>> +
>> +     xgene_msi->fwnode = of_node_to_fwnode(pdev->dev.of_node);
>> +     if (!xgene_msi->fwnode) {
>> +             xgene_msi->fwnode =
>> +                     irq_domain_alloc_fwnode((void *)xgene_msi->msi_addr);
>> +             if (!xgene_msi->fwnode) {
>> +                     dev_err(&pdev->dev, "Failed to create fwnode\n");
>> +                     rc = ENOMEM;
>> +                     goto error;
>> +             }
>> +     }
>> +
>>       xgene_msi->num_cpus = num_possible_cpus();
>>
>>       rc = xgene_msi_init_allocator(xgene_msi);
>> @@ -540,6 +569,7 @@ static int xgene_msi_probe(struct platform_device *pdev)
>>       .driver = {
>>               .name = "xgene-msi",
>>               .of_match_table = xgene_msi_match_table,
>> +             .acpi_match_table = ACPI_PTR(xgene_msi_acpi_ids),
>>       },
>>       .probe = xgene_msi_probe,
>>       .remove = xgene_msi_remove,
>>
>
> Thanks,
>
>         M.
> --
> Jazz is not dead. It just smells funny...