Re: [PATCH v2] ACPI: VIOT: Fix ACS setup
On Thu, Jun 30, 2022 at 11:59 AM Jean-Philippe Brucker
wrote:
>
> On Thu, Jun 30, 2022 at 11:40:59AM +0200, Eric Auger wrote:
> > Currently acpi_viot_init() gets called after the pci
> > device has been scanned and pci_enable_acs() has been called.
> > So pci_request_acs() fails to be taken into account leading
> > to wrong single iommu group topologies when dealing with
> > multi-function root ports for instance.
> >
> > We cannot simply move the acpi_viot_init() earlier, similarly
> > as the IORT init because the VIOT parsing relies on the pci
> > scan. However we can detect VIOT is present earlier and in
> > such a case, request ACS. Introduce a new acpi_viot_early_init()
> > routine that allows to call pci_request_acs() before the scan.
> >
> > While at it, guard the call to pci_request_acs() with #ifdef
> > CONFIG_PCI.
> >
> > Fixes: 3cf485540e7b ("ACPI: Add driver for the VIOT table")
> > Signed-off-by: Eric Auger
> > Reported-by: Jin Liu
>
> Reviewed-by: Jean-Philippe Brucker
> Tested-by: Jean-Philippe Brucker
Applied as 5.20 material, thanks!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 2/2] of: base: Avoid console probe delay when fw_devlink.strict=1
On Thu, Jun 23, 2022 at 6:39 PM Andy Shevchenko wrote: > > On Thu, Jun 23, 2022 at 12:04:21PM +0200, sascha hauer wrote: > > On Thu, Jun 23, 2022 at 01:03:43AM -0700, Saravana Kannan wrote: > > ... > > > I wonder if it wouldn't be a better approach to just probe all devices > > and record the device(node) they are waiting on. Then you know that you > > don't need to probe them again until the device they are waiting for > > is available. > > There may be no device, but resource. And we become again to the something > like > deferred probe ugly hack. > > The real solution is to rework device driver model in the kernel that it will > create a graph of dependencies and then simply follow it. But actually it > should > be more than 1 graph, because there are resources and there are power, clock > and > resets that may be orthogonal to the higher dependencies (like driver X > provides > a resource to driver Y). There is one graph, or it wouldn't be possible to shut down the system orderly. The problem is that this graph is generally dynamic, especially during system init, and following dependencies in transient states is generally hard. Device links allow the already known dependencies to be recorded and taken into account later, so we already have a graph for those. The unknown dependencies obviously cannot be used for creating a graph of any sort, though, and here we are in the business of guessing what the unknown dependencies may be IIUC. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 2/2] PCI: Rename pci_dev->untrusted to pci_dev->untrusted_dma
On Fri, Mar 25, 2022 at 7:46 PM Rajat Jain wrote:
>
> Rename the field to make it more clear, that the device can execute DMA
> attacks on the system, and thus the system may need protection from
> such attacks from this device.
>
> No functional change intended.
>
> Signed-off-by: Rajat Jain
> Reviewed-by: Mika Westerberg
Acked-by: Rafael J. Wysocki
> ---
> v5: Use "untrusted_dma" as property name, based on feedback.
> Reorder the patches in the series.
> v4: Initial version, created based on comments on other patch
>
> drivers/iommu/dma-iommu.c | 6 +++---
> drivers/iommu/intel/iommu.c | 2 +-
> drivers/iommu/iommu.c | 2 +-
> drivers/pci/ats.c | 2 +-
> drivers/pci/pci-acpi.c | 2 +-
> drivers/pci/pci.c | 2 +-
> drivers/pci/probe.c | 8
> drivers/pci/quirks.c| 2 +-
> include/linux/pci.h | 5 +++--
> 9 files changed, 16 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/iommu/dma-iommu.c b/drivers/iommu/dma-iommu.c
> index d85d54f2b549..7cbe300fe907 100644
> --- a/drivers/iommu/dma-iommu.c
> +++ b/drivers/iommu/dma-iommu.c
> @@ -497,14 +497,14 @@ static int iova_reserve_iommu_regions(struct device
> *dev,
> return ret;
> }
>
> -static bool dev_is_untrusted(struct device *dev)
> +static bool dev_has_untrusted_dma(struct device *dev)
> {
> - return dev_is_pci(dev) && to_pci_dev(dev)->untrusted;
> + return dev_is_pci(dev) && to_pci_dev(dev)->untrusted_dma;
> }
>
> static bool dev_use_swiotlb(struct device *dev)
> {
> - return IS_ENABLED(CONFIG_SWIOTLB) && dev_is_untrusted(dev);
> + return IS_ENABLED(CONFIG_SWIOTLB) && dev_has_untrusted_dma(dev);
> }
>
> /**
> diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
> index 92fea3fbbb11..9246b7c9ab46 100644
> --- a/drivers/iommu/intel/iommu.c
> +++ b/drivers/iommu/intel/iommu.c
> @@ -5570,7 +5570,7 @@ intel_iommu_enable_nesting(struct iommu_domain *domain)
> */
> static bool risky_device(struct pci_dev *pdev)
> {
> - if (pdev->untrusted) {
> + if (pdev->untrusted_dma) {
> pci_info(pdev,
> "Skipping IOMMU quirk for dev [%04X:%04X] on
> untrusted PCI link\n",
> pdev->vendor, pdev->device);
> diff --git a/drivers/iommu/iommu.c b/drivers/iommu/iommu.c
> index 8b86406b7162..79fb66af2e68 100644
> --- a/drivers/iommu/iommu.c
> +++ b/drivers/iommu/iommu.c
> @@ -1522,7 +1522,7 @@ static int iommu_get_def_domain_type(struct device *dev)
> {
> const struct iommu_ops *ops = dev->bus->iommu_ops;
>
> - if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted)
> + if (dev_is_pci(dev) && to_pci_dev(dev)->untrusted_dma)
> return IOMMU_DOMAIN_DMA;
>
> if (ops->def_domain_type)
> diff --git a/drivers/pci/ats.c b/drivers/pci/ats.c
> index c967ad6e2626..477c16ba9341 100644
> --- a/drivers/pci/ats.c
> +++ b/drivers/pci/ats.c
> @@ -42,7 +42,7 @@ bool pci_ats_supported(struct pci_dev *dev)
> if (!dev->ats_cap)
> return false;
>
> - return (dev->untrusted == 0);
> + return (dev->untrusted_dma == 0);
> }
> EXPORT_SYMBOL_GPL(pci_ats_supported);
>
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 378e05096c52..1d5a284c3661 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1362,7 +1362,7 @@ static void pci_acpi_check_for_dma_protection(struct
> pci_dev *dev)
> return;
>
> if (val)
> - dev->untrusted = 1;
> + dev->untrusted_dma = 1;
> }
>
> void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 9ecce435fb3f..1fb0eb8646c8 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -958,7 +958,7 @@ static void pci_std_enable_acs(struct pci_dev *dev)
> ctrl |= (cap & PCI_ACS_UF);
>
> /* Enable Translation Blocking for external devices and noats */
> - if (pci_ats_disabled() || dev->external_facing || dev->untrusted)
> + if (pci_ats_disabled() || dev->external_facing || dev->untrusted_dma)
> ctrl |= (cap & PCI_ACS_TB);
>
> pci_write_config_word(dev, pos + PCI_ACS_CTRL, ctrl);
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index 17a969942d37..d2a9b26fcede 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1587,7 +1587,7 @@ static void set_pcie_thunderbolt(
Re: [PATCH v5 1/2] PCI: ACPI: Support Microsoft's "DmaProperty"
On Fri, Mar 25, 2022 at 7:46 PM Rajat Jain wrote:
>
> The "DmaProperty" is supported and documented by Microsoft here:
> https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports
> They use this property for DMA protection:
> https://docs.microsoft.com/en-us/windows/security/information-protection/kernel-dma-protection-for-thunderbolt
>
> Support the "DmaProperty" with the same semantics. This is useful for
> internal PCI devices that do not hang off a PCIe rootport, but offer
> an attack surface for DMA attacks (e.g. internal network devices).
>
> Signed-off-by: Rajat Jain
> Reviewed-by: Mika Westerberg
Acked-by: Rafael J. Wysocki
> ---
> v5: * Reorder the patches in the series
> v4: * Add the GUID.
> * Update the comment and commitlog.
> v3: * Use Microsoft's documented property "DmaProperty"
> * Resctrict to ACPI only
>
> drivers/acpi/property.c | 3 +++
> drivers/pci/pci-acpi.c | 16
> 2 files changed, 19 insertions(+)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index d0986bda2964..20603cacc28d 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -48,6 +48,9 @@ static const guid_t prp_guids[] = {
> /* Storage device needs D3 GUID: 5025030f-842f-4ab4-a561-99a5189762d0
> */
> GUID_INIT(0x5025030f, 0x842f, 0x4ab4,
> 0xa5, 0x61, 0x99, 0xa5, 0x18, 0x97, 0x62, 0xd0),
> + /* DmaProperty for PCI devices GUID:
> 70d24161-6dd5-4c9e-8070-705531292865 */
> + GUID_INIT(0x70d24161, 0x6dd5, 0x4c9e,
> + 0x80, 0x70, 0x70, 0x55, 0x31, 0x29, 0x28, 0x65),
> };
>
> /* ACPI _DSD data subnodes GUID: dbb8e3e6-5886-4ba6-8795-1319f52a966b */
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 1f15ab7eabf8..378e05096c52 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -1350,12 +1350,28 @@ static void pci_acpi_set_external_facing(struct
> pci_dev *dev)
> dev->external_facing = 1;
> }
>
> +static void pci_acpi_check_for_dma_protection(struct pci_dev *dev)
> +{
> + u8 val;
> +
> + /*
> +* Property also used by Microsoft Windows for same purpose,
> +* (to implement DMA protection from a device, using the IOMMU).
> +*/
> + if (device_property_read_u8(&dev->dev, "DmaProperty", &val))
> + return;
> +
> + if (val)
> + dev->untrusted = 1;
> +}
> +
> void pci_acpi_setup(struct device *dev, struct acpi_device *adev)
> {
> struct pci_dev *pci_dev = to_pci_dev(dev);
>
> pci_acpi_optimize_delay(pci_dev, adev->handle);
> pci_acpi_set_external_facing(pci_dev);
> + pci_acpi_check_for_dma_protection(pci_dev);
> pci_acpi_add_edr_notifier(pci_dev);
>
> pci_acpi_add_pm_notifier(adev, pci_dev);
> --
> 2.35.1.1021.g381101b075-goog
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 1/2] PCI: Rename "pci_dev->untrusted" to "pci_dev->poses_dma_risk"
On Tue, Mar 22, 2022 at 10:02 AM Christoph Hellwig wrote: > > On Sat, Mar 19, 2022 at 11:29:05PM -0700, Rajat Jain wrote: > > Rename the field to make it more clear, that the device can execute DMA > > attacks on the system, and thus the system may need protection from > > such attacks from this device. > > > > No functional change intended. > > > > Signed-off-by: Rajat Jain > > --- > > v4: Initial version, created based on comments on other patch > > What a horrible name. Why not untrusted_dma which captures the > intent much better? FWIW, I like this one much better too. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] IOMMU: Intel: DMAR: Replace acpi_bus_get_device()
From: Rafael J. Wysocki
Replace acpi_bus_get_device() that is going to be dropped with
acpi_fetch_acpi_dev().
No intentional functional impact.
Signed-off-by: Rafael J. Wysocki
---
drivers/iommu/intel/dmar.c |3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
Index: linux-pm/drivers/iommu/intel/dmar.c
===
--- linux-pm.orig/drivers/iommu/intel/dmar.c
+++ linux-pm/drivers/iommu/intel/dmar.c
@@ -789,7 +789,8 @@ static int __init dmar_acpi_dev_scope_in
andd->device_name);
continue;
}
- if (acpi_bus_get_device(h, &adev)) {
+ adev = acpi_fetch_acpi_dev(h);
+ if (!adev) {
pr_err("Failed to get device for ACPI object
%s\n",
andd->device_name);
continue;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 3/5] ACPI: Add driver for the VIOT table
On Fri, Jun 18, 2021 at 5:33 PM Jean-Philippe Brucker
wrote:
>
> The ACPI Virtual I/O Translation Table describes topology of
> para-virtual platforms, similarly to vendor tables DMAR, IVRS and IORT.
> For now it describes the relation between virtio-iommu and the endpoints
> it manages.
>
> Three steps are needed to configure DMA of endpoints:
>
> (1) acpi_viot_init(): parse the VIOT table, find or create the fwnode
> associated to each vIOMMU device. This needs to happen after
> acpi_scan_init(), because it relies on the struct device and their
> fwnode to be available.
>
> (2) When probing the vIOMMU device, the driver registers its IOMMU ops
> within the IOMMU subsystem. This step doesn't require any
> intervention from the VIOT driver.
>
> (3) viot_iommu_configure(): before binding the endpoint to a driver,
> find the associated IOMMU ops. Register them, along with the
> endpoint ID, into the device's iommu_fwspec.
>
> If step (3) happens before step (2), it is deferred until the IOMMU is
> initialized, then retried.
>
> Tested-by: Eric Auger
> Reviewed-by: Eric Auger
> Signed-off-by: Jean-Philippe Brucker
>From the general ACPI perspective
Acked-by: Rafael J. Wysocki
and I'm assuming that it will be routed through a different tree.
> ---
> drivers/acpi/Kconfig | 3 +
> drivers/iommu/Kconfig | 1 +
> drivers/acpi/Makefile | 2 +
> include/linux/acpi_viot.h | 19 ++
> drivers/acpi/bus.c| 2 +
> drivers/acpi/scan.c | 3 +
> drivers/acpi/viot.c | 366 ++
> MAINTAINERS | 8 +
> 8 files changed, 404 insertions(+)
> create mode 100644 include/linux/acpi_viot.h
> create mode 100644 drivers/acpi/viot.c
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index eedec61e3476..3758c6940ed7 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -526,6 +526,9 @@ endif
>
> source "drivers/acpi/pmic/Kconfig"
>
> +config ACPI_VIOT
> + bool
> +
> endif # ACPI
>
> config X86_PM_TIMER
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 1f111b399bca..aff8a4830dd1 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -403,6 +403,7 @@ config VIRTIO_IOMMU
> depends on ARM64
> select IOMMU_API
> select INTERVAL_TREE
> + select ACPI_VIOT if ACPI
> help
> Para-virtualised IOMMU driver with virtio.
>
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 700b41adf2db..a6e644c48987 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -118,3 +118,5 @@ video-objs += acpi_video.o video_detect.o
> obj-y += dptf/
>
> obj-$(CONFIG_ARM64)+= arm64/
> +
> +obj-$(CONFIG_ACPI_VIOT)+= viot.o
> diff --git a/include/linux/acpi_viot.h b/include/linux/acpi_viot.h
> new file mode 100644
> index ..1eb8ee5b0e5f
> --- /dev/null
> +++ b/include/linux/acpi_viot.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ACPI_VIOT_H__
> +#define __ACPI_VIOT_H__
> +
> +#include
> +
> +#ifdef CONFIG_ACPI_VIOT
> +void __init acpi_viot_init(void);
> +int viot_iommu_configure(struct device *dev);
> +#else
> +static inline void acpi_viot_init(void) {}
> +static inline int viot_iommu_configure(struct device *dev)
> +{
> + return -ENODEV;
> +}
> +#endif
> +
> +#endif /* __ACPI_VIOT_H__ */
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index a4bd673934c0..d6f4e2f06fdb 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -27,6 +27,7 @@
> #include
> #endif
> #include
> +#include
> #include
> #include
> #include
> @@ -1334,6 +1335,7 @@ static int __init acpi_init(void)
> acpi_wakeup_device_init();
> acpi_debugger_init();
> acpi_setup_sb_notify_handler();
> + acpi_viot_init();
> return 0;
> }
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 2a2e690040e9..3e2bb04ab528 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -1556,6 +1557,8 @@ static const struct iommu_ops
> *acpi_iommu_configure_id(struct device *dev,
> return ops;
>
> err = iort_iommu_configure_id(dev, id_in);
> + if (err && err != -EPROBE_DEFER)
> + err = viot_iommu_configure(dev);
>
>
Re: [PATCH v4 3/6] ACPI: Add driver for the VIOT table
On Thu, Jun 10, 2021 at 10:03 AM Jean-Philippe Brucker
wrote:
>
> The ACPI Virtual I/O Translation Table describes topology of
> para-virtual platforms, similarly to vendor tables DMAR, IVRS and IORT.
> For now it describes the relation between virtio-iommu and the endpoints
> it manages.
>
> Three steps are needed to configure DMA of endpoints:
>
> (1) acpi_viot_init(): parse the VIOT table, find or create the fwnode
> associated to each vIOMMU device.
>
> (2) When probing the vIOMMU device, the driver registers its IOMMU ops
> within the IOMMU subsystem. This step doesn't require any
> intervention from the VIOT driver.
>
> (3) viot_iommu_configure(): before binding the endpoint to a driver,
> find the associated IOMMU ops. Register them, along with the
> endpoint ID, into the device's iommu_fwspec.
>
> If step (3) happens before step (2), it is deferred until the IOMMU is
> initialized, then retried.
>
> Signed-off-by: Jean-Philippe Brucker
> ---
> drivers/acpi/Kconfig | 3 +
> drivers/iommu/Kconfig | 1 +
> drivers/acpi/Makefile | 2 +
> include/linux/acpi_viot.h | 19 ++
> drivers/acpi/bus.c| 2 +
> drivers/acpi/scan.c | 3 +
> drivers/acpi/viot.c | 364 ++
> MAINTAINERS | 8 +
> 8 files changed, 402 insertions(+)
> create mode 100644 include/linux/acpi_viot.h
> create mode 100644 drivers/acpi/viot.c
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index eedec61e3476..3758c6940ed7 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -526,6 +526,9 @@ endif
>
> source "drivers/acpi/pmic/Kconfig"
>
> +config ACPI_VIOT
> + bool
> +
> endif # ACPI
>
> config X86_PM_TIMER
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 1f111b399bca..aff8a4830dd1 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -403,6 +403,7 @@ config VIRTIO_IOMMU
> depends on ARM64
> select IOMMU_API
> select INTERVAL_TREE
> + select ACPI_VIOT if ACPI
> help
> Para-virtualised IOMMU driver with virtio.
>
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 700b41adf2db..a6e644c48987 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -118,3 +118,5 @@ video-objs += acpi_video.o video_detect.o
> obj-y += dptf/
>
> obj-$(CONFIG_ARM64)+= arm64/
> +
> +obj-$(CONFIG_ACPI_VIOT)+= viot.o
> diff --git a/include/linux/acpi_viot.h b/include/linux/acpi_viot.h
> new file mode 100644
> index ..1eb8ee5b0e5f
> --- /dev/null
> +++ b/include/linux/acpi_viot.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ACPI_VIOT_H__
> +#define __ACPI_VIOT_H__
> +
> +#include
> +
> +#ifdef CONFIG_ACPI_VIOT
> +void __init acpi_viot_init(void);
> +int viot_iommu_configure(struct device *dev);
> +#else
> +static inline void acpi_viot_init(void) {}
> +static inline int viot_iommu_configure(struct device *dev)
> +{
> + return -ENODEV;
> +}
> +#endif
> +
> +#endif /* __ACPI_VIOT_H__ */
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index be7da23fad76..b835ca702ff0 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -27,6 +27,7 @@
> #include
> #endif
> #include
> +#include
> #include
> #include
> #include
> @@ -1339,6 +1340,7 @@ static int __init acpi_init(void)
> pci_mmcfg_late_init();
> acpi_iort_init();
> acpi_scan_init();
> + acpi_viot_init();
Is there a specific reason why to call it right here?
In particular, does it need to be called after acpi_scan_init()? And
does it need to be called before the subsequent functions? If so,
then why?
> acpi_ec_init();
> acpi_debugfs_init();
> acpi_sleep_proc_init();
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 0c53c8533300..4fa684fdfda8 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -1556,6 +1557,8 @@ static const struct iommu_ops
> *acpi_iommu_configure_id(struct device *dev,
> return ops;
>
> err = iort_iommu_configure_id(dev, id_in);
> + if (err && err != -EPROBE_DEFER)
> + err = viot_iommu_configure(dev);
>
> /*
> * If we have reason to believe the IOMMU driver missed the initial
> diff --git a/drivers/acpi/viot.c b/drivers/acpi/viot.c
> new file mode 100644
> index ..892cd9fa7b6d
> --- /dev/null
> +++ b/drivers/acpi/viot.c
> @@ -0,0 +1,364 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Virtual I/O topology
> + *
> + * The Virtual I/O Translation Table (VIOT) describes the topology of
> + * para-virtual IOMMUs and the endpoints they manage. The OS uses it to
> + * initialize devices in the right order, pr
Re: [PATCH v2 2/6] ACPI: Move IOMMU setup code out of IORT
On Fri, Apr 23, 2021 at 1:57 PM Jean-Philippe Brucker wrote: > > Some of the IOMMU setup code in IORT is fairly generic and can be reused > by VIOT. Extract it from IORT. Except that iort_iommu_configure_id() is not really generic AFAICS. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 3/6] ACPI: Add driver for the VIOT table
On Fri, Apr 23, 2021 at 1:57 PM Jean-Philippe Brucker
wrote:
>
> The ACPI Virtual I/O Translation Table describes topology of
> para-virtual platforms, similarly to vendor tables DMAR, IVRS and IORT.
> For now it describes the relation between virtio-iommu and the endpoints
> it manages.
>
> Three steps are needed to configure DMA of endpoints:
>
> (1) acpi_viot_init(): parse the VIOT table, find or create the fwnode
> associated to each vIOMMU device.
>
> (2) When probing the vIOMMU device, the driver registers its IOMMU ops
> within the IOMMU subsystem. This step doesn't require any
> intervention from the VIOT driver.
>
> (3) viot_iommu_configure(): before binding the endpoint to a driver,
> find the associated IOMMU ops. Register them, along with the
> endpoint ID, into the device's iommu_fwspec.
>
> If step (3) happens before step (2), it is deferred until the IOMMU is
> initialized, then retried.
>
> Signed-off-by: Jean-Philippe Brucker
> ---
> drivers/acpi/Kconfig | 3 +
> drivers/iommu/Kconfig | 1 +
> drivers/acpi/Makefile | 2 +
> include/linux/acpi_viot.h | 19 +++
> drivers/acpi/bus.c| 2 +
> drivers/acpi/scan.c | 3 +
> drivers/acpi/viot.c | 350 ++
> MAINTAINERS | 8 +
> 8 files changed, 388 insertions(+)
> create mode 100644 include/linux/acpi_viot.h
> create mode 100644 drivers/acpi/viot.c
>
> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig
> index eedec61e3476..3758c6940ed7 100644
> --- a/drivers/acpi/Kconfig
> +++ b/drivers/acpi/Kconfig
> @@ -526,6 +526,9 @@ endif
>
> source "drivers/acpi/pmic/Kconfig"
>
> +config ACPI_VIOT
> + bool
> +
> endif # ACPI
>
> config X86_PM_TIMER
> diff --git a/drivers/iommu/Kconfig b/drivers/iommu/Kconfig
> index 1f111b399bca..aff8a4830dd1 100644
> --- a/drivers/iommu/Kconfig
> +++ b/drivers/iommu/Kconfig
> @@ -403,6 +403,7 @@ config VIRTIO_IOMMU
> depends on ARM64
> select IOMMU_API
> select INTERVAL_TREE
> + select ACPI_VIOT if ACPI
> help
> Para-virtualised IOMMU driver with virtio.
>
> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
> index 700b41adf2db..a6e644c48987 100644
> --- a/drivers/acpi/Makefile
> +++ b/drivers/acpi/Makefile
> @@ -118,3 +118,5 @@ video-objs += acpi_video.o video_detect.o
> obj-y += dptf/
>
> obj-$(CONFIG_ARM64)+= arm64/
> +
> +obj-$(CONFIG_ACPI_VIOT)+= viot.o
> diff --git a/include/linux/acpi_viot.h b/include/linux/acpi_viot.h
> new file mode 100644
> index ..1eb8ee5b0e5f
> --- /dev/null
> +++ b/include/linux/acpi_viot.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0-only */
> +
> +#ifndef __ACPI_VIOT_H__
> +#define __ACPI_VIOT_H__
> +
> +#include
> +
> +#ifdef CONFIG_ACPI_VIOT
> +void __init acpi_viot_init(void);
> +int viot_iommu_configure(struct device *dev);
> +#else
> +static inline void acpi_viot_init(void) {}
> +static inline int viot_iommu_configure(struct device *dev)
> +{
> + return -ENODEV;
> +}
> +#endif
> +
> +#endif /* __ACPI_VIOT_H__ */
> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
> index be7da23fad76..b835ca702ff0 100644
> --- a/drivers/acpi/bus.c
> +++ b/drivers/acpi/bus.c
> @@ -27,6 +27,7 @@
> #include
> #endif
> #include
> +#include
> #include
> #include
> #include
> @@ -1339,6 +1340,7 @@ static int __init acpi_init(void)
> pci_mmcfg_late_init();
> acpi_iort_init();
> acpi_scan_init();
> + acpi_viot_init();
> acpi_ec_init();
> acpi_debugfs_init();
> acpi_sleep_proc_init();
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 5924421075f6..4db43c822ee7 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -9,6 +9,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include
> @@ -1554,6 +1555,8 @@ static const struct iommu_ops
> *acpi_iommu_configure_id(struct device *dev,
> return ops;
>
> err = iort_iommu_configure_id(dev, id_in);
> + if (err && err != -EPROBE_DEFER)
> + err = viot_iommu_configure(dev);
>
> /*
> * If we have reason to believe the IOMMU driver missed the initial
> diff --git a/drivers/acpi/viot.c b/drivers/acpi/viot.c
> new file mode 100644
> index ..710e5a5eac70
> --- /dev/null
> +++ b/drivers/acpi/viot.c
> @@ -0,0 +1,350 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Virtual I/O topology
In the first place, more information on what this is all about, please.
What it does and how it is used.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 05/12] ACPI/IORT: Add an input ID to acpi_dma_configure()
On Tue, Jul 28, 2020 at 2:48 PM Lorenzo Pieralisi wrote: > > On Wed, Jul 15, 2020 at 10:13:26AM +0100, Lorenzo Pieralisi wrote: > > On Thu, Jul 09, 2020 at 10:35:14AM +0100, Lorenzo Pieralisi wrote: > > > On Fri, Jun 19, 2020 at 09:20:06AM +0100, Lorenzo Pieralisi wrote: > > > > Some HW devices are created as child devices of proprietary busses, > > > > that have a bus specific policy defining how the child devices > > > > wires representing the devices ID are translated into IOMMU and > > > > IRQ controllers device IDs. > > > > > > > > Current IORT code provides translations for: > > > > > > > > - PCI devices, where the device ID is well identified at bus level > > > > as the requester ID (RID) > > > > - Platform devices that are endpoint devices where the device ID is > > > > retrieved from the ACPI object IORT mappings (Named components single > > > > mappings). A platform device is represented in IORT as a named > > > > component node > > > > > > > > For devices that are child devices of proprietary busses the IORT > > > > firmware represents the bus node as a named component node in IORT > > > > and it is up to that named component node to define in/out bus > > > > specific ID translations for the bus child devices that are > > > > allocated and created in a bus specific manner. > > > > > > > > In order to make IORT ID translations available for proprietary > > > > bus child devices, the current ACPI (and IORT) code must be > > > > augmented to provide an additional ID parameter to acpi_dma_configure() > > > > representing the child devices input ID. This ID is bus specific > > > > and it is retrieved in bus specific code. > > > > > > > > By adding an ID parameter to acpi_dma_configure(), the IORT > > > > code can map the child device ID to an IOMMU stream ID through > > > > the IORT named component representing the bus in/out ID mappings. > > > > > > > > Signed-off-by: Lorenzo Pieralisi > > > > Cc: Will Deacon > > > > Cc: Hanjun Guo > > > > Cc: Sudeep Holla > > > > Cc: Catalin Marinas > > > > Cc: Robin Murphy > > > > Cc: "Rafael J. Wysocki" > > > > --- > > > > drivers/acpi/arm64/iort.c | 59 +-- > > > > drivers/acpi/scan.c | 8 -- > > > > include/acpi/acpi_bus.h | 9 -- > > > > include/linux/acpi.h | 7 + > > > > include/linux/acpi_iort.h | 7 +++-- > > > > 5 files changed, 67 insertions(+), 23 deletions(-) > > > > > > Hi Rafael, > > > > > > just to ask if the ACPI core changes in this patch are OK with you, > > > thank you very much. Sorry for the delay, I was offline last week. > > Hi Rafael, > > > > are you OK with ACPI core changes in this patch ? Yes, I am. Please feel free to route it through whatever tree you think would be appropriate. Thanks! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 5/7] driver core: Add device location to "struct device" and expose it in sysfs
On Tue, Jun 30, 2020 at 5:38 PM Greg Kroah-Hartman
wrote:
>
> On Tue, Jun 30, 2020 at 03:00:34PM +0200, Rafael J. Wysocki wrote:
> > On Tue, Jun 30, 2020 at 2:52 PM Greg Kroah-Hartman
> > wrote:
> > >
> > > On Tue, Jun 30, 2020 at 01:49:48PM +0300, Heikki Krogerus wrote:
> > > > On Mon, Jun 29, 2020 at 09:49:41PM -0700, Rajat Jain wrote:
> > > > > Add a new (optional) field to denote the physical location of a device
> > > > > in the system, and expose it in sysfs. This was discussed here:
> > > > > https://lore.kernel.org/linux-acpi/20200618184621.ga446...@kroah.com/
> > > > >
> > > > > (The primary choice for attribute name i.e. "location" is already
> > > > > exposed as an ABI elsewhere, so settled for "site"). Individual buses
> > > > > that want to support this new attribute can opt-in by setting a flag
> > > > > in
> > > > > bus_type, and then populating the location of device while enumerating
> > > > > it.
> > > >
> > > > So why not just call it "physical_location"?
> > >
> > > That's better, and will allow us to put "3rd blue plug from the left,
> > > 4th row down" in there someday :)
> > >
> > > All of this is "relative" to the CPU, right? But what CPU? Again, how
> > > are the systems with drawers of PCI and CPUs and memory that can be
> > > added/removed at any point in time being handled here? What is
> > > "internal" and "external" for them?
> > >
> > > What exactly is the physical boundry here that is attempting to be
> > > described?
> >
> > Also, where is the "physical location" information going to come from?
>
> Who knows? :)
>
> Some BIOS seem to provide this, but do you trust that?
>
> > If that is the platform firmware (which I suspect is the anticipated
> > case), there may be problems with reliability related to that.
>
> s/may/will/
>
> which means making the kernel inact a policy like this patch series
> tries to add, will result in a lot of broken systems, which is why I
> keep saying that it needs to be done in userspace.
>
> It's as if some of us haven't been down this road before and just keep
> being ignored...
>
> {sigh}
Well, to be honest, if you are a "vertical" vendor and you control the
entire stack, *including* the platform firmware, it would be kind of
OK for you to do that in a product kernel.
However, this is not a practical thing to do in the mainline kernel
which must work for everybody, including people who happen to use
systems with broken or even actively unfriendly firmware on them.
So I'm inclined to say that IMO this series "as is" would not be an
improvement from the mainline perspective.
I guess it would make sense to have an attribute for user space to
write to in order to make the kernel reject device plug-in events
coming from a given port or connector, but the kernel has no reliable
means to determine *which* ports or connectors are "safe", and even if
there was a way for it to do that, it still may not agree with user
space on which ports or connectors should be regarded as "safe".
Cheers!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 5/7] driver core: Add device location to "struct device" and expose it in sysfs
On Tue, Jun 30, 2020 at 2:52 PM Greg Kroah-Hartman wrote: > > On Tue, Jun 30, 2020 at 01:49:48PM +0300, Heikki Krogerus wrote: > > On Mon, Jun 29, 2020 at 09:49:41PM -0700, Rajat Jain wrote: > > > Add a new (optional) field to denote the physical location of a device > > > in the system, and expose it in sysfs. This was discussed here: > > > https://lore.kernel.org/linux-acpi/20200618184621.ga446...@kroah.com/ > > > > > > (The primary choice for attribute name i.e. "location" is already > > > exposed as an ABI elsewhere, so settled for "site"). Individual buses > > > that want to support this new attribute can opt-in by setting a flag in > > > bus_type, and then populating the location of device while enumerating > > > it. > > > > So why not just call it "physical_location"? > > That's better, and will allow us to put "3rd blue plug from the left, > 4th row down" in there someday :) > > All of this is "relative" to the CPU, right? But what CPU? Again, how > are the systems with drawers of PCI and CPUs and memory that can be > added/removed at any point in time being handled here? What is > "internal" and "external" for them? > > What exactly is the physical boundry here that is attempting to be > described? Also, where is the "physical location" information going to come from? If that is the platform firmware (which I suspect is the anticipated case), there may be problems with reliability related to that. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 0/6] ACPI / utils: add new helper for HID/UID match
On Tuesday, October 1, 2019 4:27:19 PM CEST Andy Shevchenko wrote: > There are few users outside of ACPI realm that re-introduce a custom > solution to match ACPI device against HID/UID. Add a generic helper for > them. > > The series is supposed to go via linux-pm tree. > > In v3: > - correct logic in sdhci-acpi for qcom devices (Adrian) > - add Mika's Ack > > In v2: > - add patch 2 due to latent issue in the header (lkp) > - get rid of match_hid_uid() completely in patch 6 > > Andy Shevchenko (6): > ACPI / utils: Describe function parameters in kernel-doc > ACPI / utils: Move acpi_dev_get_first_match_dev() under CONFIG_ACPI > ACPI / utils: Introduce acpi_dev_hid_uid_match() helper > ACPI / LPSS: Switch to use acpi_dev_hid_uid_match() > mmc: sdhci-acpi: Switch to use acpi_dev_hid_uid_match() > iommu/amd: Switch to use acpi_dev_hid_uid_match() > > drivers/acpi/acpi_lpss.c | 21 +++ > drivers/acpi/utils.c | 32 +++ > drivers/iommu/amd_iommu.c | 30 - > drivers/mmc/host/sdhci-acpi.c | 49 --- > include/acpi/acpi_bus.h | 8 +++--- > include/linux/acpi.h | 6 + > 6 files changed, 67 insertions(+), 79 deletions(-) > > Applying the series (with the tags given so far) as 5.5 material, thanks! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2] driver: core: Allow subsystems to continue deferring probe
On Fri, Jun 14, 2019 at 11:39 AM Thierry Reding
wrote:
>
> On Fri, Jun 14, 2019 at 11:10:58AM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jun 13, 2019 at 07:00:11PM +0200, Thierry Reding wrote:
> > > From: Thierry Reding
> > >
[cut]
>
> To avoid further back and forth, what exactly is it that you would have
> me do? That is, what do you consider to be the correct way to do this?
>
> Would you prefer me to add another function with a different name that
> reimplements the functionality only with the exception? Something along
> the lines of:
>
> int driver_deferred_probe_check_state_continue(struct device *dev)
> {
> int ret;
>
> ret = driver_deferred_probe_check_state(dev);
> if (ret == -ENODEV)
> return -EPROBE_DEFER;
>
> return ret;
> }
>
> ? I'd need to split that up some more to avoid the warning that the
> inner function prints before returning -ENODEV, but that's a minor
> detail. Would that API be more to your liking?
Well, why don't you do
static int deferred_probe_check_state_internal(struct device *dev)
{
if (!initcalls_done)
return -EPROBE_DEFER;
if (!deferred_probe_timeout) {
dev_WARN(dev, "deferred probe timeout, ignoring dependency");
return -ETIMEDOUT;
}
return 0;
}
int driver_deferred_probe_check_state(struct device *dev)
{
int ret = deferred_probe_check_state_internal(dev);
if (ret)
return ret;
dev_warn(dev, "ignoring dependency for device, assuming no driver");
return -ENODEV;
}
int driver_deferred_probe_check_state_continue(struct device *dev)
{
int ret = deferred_probe_check_state_internal(dev);
if (ret)
return ret;
return -EPROBE_DEFER;
}
Re: [PATCH v2] driver: core: Allow subsystems to continue deferring probe
On Thu, Jun 13, 2019 at 7:00 PM Thierry Reding wrote:
>
> From: Thierry Reding
>
> Some subsystems, such as pinctrl, allow continuing to defer probe
> indefinitely. This is useful for devices that depend on resources
> provided by devices that are only probed after the init stage.
>
> One example of this can be seen on Tegra, where the DPAUX hardware
> contains pinmuxing controls for pins that it shares with an I2C
> controller. The I2C controller is typically used for communication
> with a monitor over HDMI (DDC). However, other instances of the I2C
> controller are used to access system critical components, such as a
> PMIC. The I2C controller driver will therefore usually be a builtin
> driver, whereas the DPAUX driver is part of the display driver that
> is loaded from a module to avoid bloating the kernel image with all
> of the DRM/KMS subsystem.
>
> In this particular case the pins used by this I2C/DDC controller
> become accessible very late in the boot process. However, since the
> controller is only used in conjunction with display, that's not an
> issue.
>
> Unfortunately the driver core currently outputs a warning message
> when a device fails to get the pinctrl before the end of the init
> stage. That can be confusing for the user because it may sound like
> an unwanted error occurred, whereas it's really an expected and
> harmless situation.
>
> In order to eliminate this warning, this patch allows callers of the
> driver_deferred_probe_check_state() helper to specify that they want
> to continue deferring probe, regardless of whether we're past the
> init stage or not. All of the callers of that function are updated
> for the new signature, but only the pinctrl subsystem passes a true
> value in the new persist parameter if appropriate.
>
> Signed-off-by: Thierry Reding
> ---
> Changes in v2:
> - pass persist flag via flags parameter to make the function call easier
> to understand
>
> drivers/base/dd.c| 19 ++-
> drivers/base/power/domain.c | 2 +-
> drivers/iommu/of_iommu.c | 2 +-
> drivers/pinctrl/devicetree.c | 9 +
> include/linux/device.h | 18 +-
> 5 files changed, 38 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/base/dd.c b/drivers/base/dd.c
> index 0df9b4461766..0399a6f6c479 100644
> --- a/drivers/base/dd.c
> +++ b/drivers/base/dd.c
> @@ -238,23 +238,32 @@ __setup("deferred_probe_timeout=",
> deferred_probe_timeout_setup);
> /**
> * driver_deferred_probe_check_state() - Check deferred probe state
> * @dev: device to check
> + * @flags: Flags used to control the behavior of this function. Drivers can
> + * set the DRIVER_DEFER_PROBE_PERSIST flag to indicate that they want to
What about calling this flag DRIVER_DEFER_PROBE_CONTINUE ?
Also, I would just say
@flags: Flags used to control the behavior of this function.
here and added the description of the flag below.
> + * keep trying to probe after built-in drivers have had a chance to probe.
> + * This is useful for built-in drivers that rely on resources provided by
> + * modular drivers.
> *
> * Returns -ENODEV if init is done and all built-in drivers have had a chance
> - * to probe (i.e. initcalls are done), -ETIMEDOUT if deferred probe debug
> - * timeout has expired, or -EPROBE_DEFER if none of those conditions are met.
> + * to probe (i.e. initcalls are done) and unless the
> DRIVER_DEFER_PROBE_PERSIST
"unless DRIVER_DEFER_PROBE_CONTINUE is set in @flags"
> + * flag is set, -ETIMEDOUT if deferred probe debug timeout has expired, or
> + * -EPROBE_DEFER if none of those conditions are met.
> *
> * Drivers or subsystems can opt-in to calling this function instead of
> directly
> * returning -EPROBE_DEFER.
And here:
"In that case, passing DRIVER_DEFER_PROBE_CONTINUE in @flags indicates
that the caller wants to keep trying to probe after built-in drivers
have had a chance to probe, which is useful for built-in drivers that
rely on resources provided by modular drivers."
> */
> -int driver_deferred_probe_check_state(struct device *dev)
> +int driver_deferred_probe_check_state(struct device *dev, unsigned long
> flags)
> {
> if (initcalls_done) {
> if (!deferred_probe_timeout) {
> dev_WARN(dev, "deferred probe timeout, ignoring
> dependency");
> return -ETIMEDOUT;
> }
> - dev_warn(dev, "ignoring dependency for device, assuming no
> driver");
> - return -ENODEV;
> +
> + if ((flags & DRIVER_DEFER_PROBE_PERSIST) == 0) {
What about
if (!(flags & DRIVER_DEFER_PROBE_PERSIST)) {
> + dev_warn(dev, "ignoring dependency for device,
> assuming no driver");
> + return -ENODEV;
> + }
> }
> return -EPROBE_DEFER;
> }
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index 33c30c1e6a30..6198c6a30f
Re: [PATCH/RFC] driver core: Postpone DMA tear-down until after devres release
On Thu, Feb 7, 2019 at 8:42 PM Geert Uytterhoeven
wrote:
>
> When unbinding the (IOMMU-enabled) R-Car SATA device on Salvator-XS
> (R-Car H3 ES2.0), in preparation of rebinding against vfio-platform for
> device pass-through for virtualization:
>
> echo ee30.sata > /sys/bus/platform/drivers/sata_rcar/unbind
>
> the kernel crashes with:
>
> Unable to handle kernel paging request at virtual address ffbf029c
> Mem abort info:
> ESR = 0x9606
> Exception class = DABT (current EL), IL = 32 bits
> SET = 0, FnV = 0
> EA = 0, S1PTW = 0
> Data abort info:
> ISV = 0, ISS = 0x0006
> CM = 0, WnR = 0
> swapper pgtable: 4k pages, 39-bit VAs, pgdp = 7e8c586c
> [ffbf029c] pgd=00073bfc6003, pud=00073bfc6003,
> pmd=
> Internal error: Oops: 9606 [#1] SMP
> Modules linked in:
> CPU: 0 PID: 1098 Comm: bash Not tainted
> 5.0.0-rc5-salvator-x-00452-g37596f884f4318ef #287
> Hardware name: Renesas Salvator-X 2nd version board based on r8a7795
> ES2.0+ (DT)
> pstate: 6045 (nZCv daif +PAN -UAO)
> pc : __free_pages+0x8/0x58
> lr : __dma_direct_free_pages+0x50/0x5c
> sp : ff801268baa0
> x29: ff801268baa0 x28:
> x27: ffc6f9c60bf0 x26: ffc6f9c60bf0
> x25: ffc6f9c60810 x24:
> x23: f000 x22: ff8012145000
> x21: 0800 x20: ffbf029fffc8
> x19: x18: ffc6f86c42c8
> x17: x16: 0070
> x15: 0003 x14:
> x13: ff801103d7f8 x12: 0028
> x11: ff807604 x10: 9ad8
> x9 : ff80110126d0 x8 : ffc6f7563000
> x7 : 6b6b6b6b6b6b6b6b x6 : 0018
> x5 : ff8011cf3cc8 x4 : 4000
> x3 : 0008 x2 : 0001
> x1 : x0 : ffbf029fffc8
> Process bash (pid: 1098, stack limit = 0xc38e3e32)
> Call trace:
> __free_pages+0x8/0x58
> __dma_direct_free_pages+0x50/0x5c
> arch_dma_free+0x1c/0x98
> dma_direct_free+0x14/0x24
> dma_free_attrs+0x9c/0xdc
> dmam_release+0x18/0x20
> release_nodes+0x25c/0x28c
> devres_release_all+0x48/0x4c
> device_release_driver_internal+0x184/0x1f0
> device_release_driver+0x14/0x1c
> unbind_store+0x70/0xb8
> drv_attr_store+0x24/0x34
> sysfs_kf_write+0x4c/0x64
> kernfs_fop_write+0x154/0x1c4
> __vfs_write+0x34/0x164
> vfs_write+0xb4/0x16c
> ksys_write+0x5c/0xbc
> __arm64_sys_write+0x14/0x1c
> el0_svc_common+0x98/0x114
> el0_svc_handler+0x1c/0x24
> el0_svc+0x8/0xc
> Code: d51b4234 17fa a9bf7bfd 910003fd (b9403404)
> ---[ end trace 8c564cdd3a1a840f ]---
>
> While I've bisected this to commit e8e683ae9a736407 ("iommu/of: Fix
> probe-deferral"), and reverting that commit on post-v5.0-rc4 kernels
> does fix the problem, this turned out to be a red herring.
>
> On arm64, arch_teardown_dma_ops() resets dev->dma_ops to NULL.
> Hence if a driver has used a managed DMA allocation API, the allocated
> DMA memory will be freed using the direct DMA ops, while it may have
> been allocated using a custom DMA ops (iommu_dma_ops in this case).
>
> Fix this by reversing the order of the calls to devres_release_all() and
> arch_teardown_dma_ops().
>
> Signed-off-by: Geert Uytterhoeven
Reviewed-by: Rafael J. Wysocki
> ---
> Question:
> Is this safe on arm32, which calls arm_teardown_iommu_dma_ops() instead
> of resetting dev->dma_ops?
>
> ---
> Adding some debug code, and comparing before/after commit
> e8e683ae9a736407:
>
> sata_rcar ee30.sata: of_iommu_configure:227: err = -517
> -sata_rcar ee30.sata: of_iommu_configure:230: calling
> iommu_probe_device()
> -sata_rcar ee30.sata: iommu_probe_device:122: Calling ipmmu_add_device
> -sata_rcar ee30.sata: ipmmu_add_device:893
> -sata_rcar ee30.sata: of_iommu_configure:232: iommu_probe_device()
> returned -19
> -sata_rcar ee30.sata: dma_alloc_attrs:257: size 2048, ops =
> (null)
> -sata_rcar ee30.sata: __dma_direct_alloc_pages:104: size 4096
> -sata_rcar ee30.sata: __dma_direct_alloc_pages:115: trying
> dma_alloc_from_contiguous()
> -sata_rcar ee30.sata: dma_alloc_from_contiguous:202: cma_alloc(1)
> returned page ffbf00d20e00
> -sata_rcar ee30.sata: dma_alloc_attrs:271: allocated using
> dma_direct_alloc()
> -sata_rcar ee30.sata: dmam_alloc_attrs
Re: [PATCH 2/2] ACPI / scan: Refactor _CCA enforcement
On Fri, Dec 7, 2018 at 5:31 PM Robin Murphy wrote:
>
> Rather than checking the DMA attribute at each callsite, just pass it
> through for acpi_dma_configure() to handle directly. That can then deal
> with the relatively exceptional DEV_DMA_NOT_SUPPORTED case by explicitly
> installing dummy DMA ops instead of just skipping setup entirely. This
> will then free up the dev->dma_ops == NULL case for some valuable
> fastpath optimisations.
>
> CC: Rafael J. Wysocki
> CC: Len Brown
> CC: Greg Kroah-Hartman
> CC: Bjorn Helgaas
> Signed-off-by: Robin Murphy
The changes in this patch are fine by me:
Reviewed-by: Rafael J. Wysocki
> ---
> drivers/acpi/scan.c | 5 +
> drivers/base/platform.c | 3 +--
> drivers/pci/pci-driver.c | 3 +--
> 3 files changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index bd1c59fb0e17..b75ae34ed188 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1456,6 +1456,11 @@ int acpi_dma_configure(struct device *dev, enum
> dev_dma_attr attr)
> const struct iommu_ops *iommu;
> u64 dma_addr = 0, size = 0;
>
> + if (attr == DEV_DMA_NOT_SUPPORTED) {
> + set_dma_ops(dev, &dma_dummy_ops);
> + return 0;
> + }
> +
> iort_dma_setup(dev, &dma_addr, &size);
>
> iommu = iort_iommu_configure(dev);
> diff --git a/drivers/base/platform.c b/drivers/base/platform.c
> index 41b91af95afb..c6daca875c17 100644
> --- a/drivers/base/platform.c
> +++ b/drivers/base/platform.c
> @@ -1138,8 +1138,7 @@ int platform_dma_configure(struct device *dev)
> ret = of_dma_configure(dev, dev->of_node, true);
> } else if (has_acpi_companion(dev)) {
> attr = acpi_get_dma_attr(to_acpi_device_node(dev->fwnode));
> - if (attr != DEV_DMA_NOT_SUPPORTED)
> - ret = acpi_dma_configure(dev, attr);
> + ret = acpi_dma_configure(dev, attr);
> }
>
> return ret;
> diff --git a/drivers/pci/pci-driver.c b/drivers/pci/pci-driver.c
> index bef17c3fca67..f899a28b90f8 100644
> --- a/drivers/pci/pci-driver.c
> +++ b/drivers/pci/pci-driver.c
> @@ -1602,8 +1602,7 @@ static int pci_dma_configure(struct device *dev)
> struct acpi_device *adev =
> to_acpi_device_node(bridge->fwnode);
> enum dev_dma_attr attr = acpi_get_dma_attr(adev);
>
> - if (attr != DEV_DMA_NOT_SUPPORTED)
> - ret = acpi_dma_configure(dev, attr);
> + ret = acpi_dma_configure(dev, attr);
> }
>
> pci_put_host_bridge_device(bridge);
> --
> 2.19.1.dirty
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 1/4] PCI / ACPI: Identify untrusted PCI devices
On Thu, Nov 29, 2018 at 4:52 PM Mika Westerberg
wrote:
>
> A malicious PCI device may use DMA to attack the system. An external
> Thunderbolt port is a convenient point to attach such a device. The OS
> may use IOMMU to defend against DMA attacks.
>
> Recent BIOSes with Thunderbolt ports mark these externally facing root
> ports with this ACPI _DSD [1]:
>
> Name (_DSD, Package () {
> ToUUID ("efcc06cc-73ac-4bc3-bff0-76143807c389"),
> Package () {
> Package () {"ExternalFacingPort", 1},
> Package () {"UID", 0 }
> }
> })
>
> If we find such a root port, mark it and all its children as untrusted.
> The rest of the OS may use this information to enable DMA protection
> against malicious devices. For instance the device may be put behind an
> IOMMU to keep it from accessing memory outside of what the driver has
> allocated for it.
>
> While at it, add a comment on top of prp_guids array explaining the
> possible caveat resulting when these GUIDs are treated equivalent.
>
> [1]
> https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports
>
> Signed-off-by: Mika Westerberg
Acked-by: Rafael J. Wysocki
> ---
> drivers/acpi/property.c | 11 +++
> drivers/pci/pci-acpi.c | 19 +++
> drivers/pci/probe.c | 15 +++
> include/linux/pci.h | 8
> 4 files changed, 53 insertions(+)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 8c7c4583b52d..77abe0ec4043 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -24,6 +24,14 @@ static int acpi_data_get_property_array(const struct
> acpi_device_data *data,
> acpi_object_type type,
> const union acpi_object **obj);
>
> +/*
> + * The GUIDs here are made equivalent to each other in order to avoid extra
> + * complexity in the properties handling code, with the caveat that the
> + * kernel will accept certain combinations of GUID and properties that are
> + * not defined without a warning. For instance if any of the properties
> + * from different GUID appear in a property list of another, it will be
> + * accepted by the kernel. Firmware validation tools should catch these.
> + */
> static const guid_t prp_guids[] = {
> /* ACPI _DSD device properties GUID:
> daffd814-6eba-4d8c-8a91-bc9bbf4aa301 */
> GUID_INIT(0xdaffd814, 0x6eba, 0x4d8c,
> @@ -31,6 +39,9 @@ static const guid_t prp_guids[] = {
> /* Hotplug in D3 GUID: 6211e2c0-58a3-4af3-90e1-927a4e0c55a4 */
> GUID_INIT(0x6211e2c0, 0x58a3, 0x4af3,
> 0x90, 0xe1, 0x92, 0x7a, 0x4e, 0x0c, 0x55, 0xa4),
> + /* External facing port GUID: efcc06cc-73ac-4bc3-bff0-76143807c389 */
> + GUID_INIT(0xefcc06cc, 0x73ac, 0x4bc3,
> + 0xbf, 0xf0, 0x76, 0x14, 0x38, 0x07, 0xc3, 0x89),
> };
>
> static const guid_t ads_guid =
> diff --git a/drivers/pci/pci-acpi.c b/drivers/pci/pci-acpi.c
> index 921db6f80340..e1949f7efd9c 100644
> --- a/drivers/pci/pci-acpi.c
> +++ b/drivers/pci/pci-acpi.c
> @@ -789,6 +789,24 @@ static void pci_acpi_optimize_delay(struct pci_dev *pdev,
> ACPI_FREE(obj);
> }
>
> +static void pci_acpi_set_untrusted(struct pci_dev *dev)
> +{
> + u8 val;
> +
> + if (pci_pcie_type(dev) != PCI_EXP_TYPE_ROOT_PORT)
> + return;
> + if (device_property_read_u8(&dev->dev, "ExternalFacingPort", &val))
> + return;
> +
> + /*
> +* These root ports expose PCIe (including DMA) outside of the
> +* system so make sure we treat them and everything behind as
> +* untrusted.
> +*/
> + if (val)
> + dev->untrusted = 1;
> +}
> +
> static void pci_acpi_setup(struct device *dev)
> {
> struct pci_dev *pci_dev = to_pci_dev(dev);
> @@ -798,6 +816,7 @@ static void pci_acpi_setup(struct device *dev)
> return;
>
> pci_acpi_optimize_delay(pci_dev, adev->handle);
> + pci_acpi_set_untrusted(pci_dev);
>
> pci_acpi_add_pm_notifier(adev, pci_dev);
> if (!adev->wakeup.flags.valid)
> diff --git a/drivers/pci/probe.c b/drivers/pci/probe.c
> index b1c05b5054a0..257b9f6f2ebb 100644
> --- a/drivers/pci/probe.c
> +++ b/drivers/pci/probe.c
> @@ -1378,6 +1378,19 @@ static void set_pcie_thunderbolt(struct pci_dev *dev)
> }
> }
>
> +static void set_pcie_untrusted(struct pci_dev *dev)
> +{
> + struct pci_dev *parent;
>
Re: [PATCH v2 1/4] PCI / ACPI: Identify untrusted PCI devices
On Wed, Nov 28, 2018 at 11:54 AM Mika Westerberg
wrote:
>
> On Tue, Nov 27, 2018 at 06:14:43PM +0100, Rafael J. Wysocki wrote:
> > > diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> > > index 8c7c4583b52d..4bdad32f62c8 100644
> > > --- a/drivers/acpi/property.c
> > > +++ b/drivers/acpi/property.c
> > > @@ -31,6 +31,9 @@ static const guid_t prp_guids[] = {
> > > /* Hotplug in D3 GUID: 6211e2c0-58a3-4af3-90e1-927a4e0c55a4 */
> > > GUID_INIT(0x6211e2c0, 0x58a3, 0x4af3,
> > > 0x90, 0xe1, 0x92, 0x7a, 0x4e, 0x0c, 0x55, 0xa4),
> > > + /* External facing port GUID:
> > > efcc06cc-73ac-4bc3-bff0-76143807c389 */
> > > + GUID_INIT(0xefcc06cc, 0x73ac, 0x4bc3,
> > > + 0xbf, 0xf0, 0x76, 0x14, 0x38, 0x07, 0xc3, 0x89),
> > > };
> >
> > One observation here. Note that I really have no strong opinion on
> > that, so this is not an objection, but IMO it is fair to make things
> > clear for everybody.
> >
> > So this causes the External facing port GUID (which already is the
> > case with the Hotplug in D3 GUID for that matter) to be practically
> > equivalent to the ACPI _DSD device properties GUID. This means that
> > Linux will consider a combination of any of these GUIDs with any
> > properties defined for any of them as valid, which need not be the
> > case in Windows.
> >
> > For example, since the ExternalFacingPort property is defined along
> > with the External facing port GUID, Windows will likely ignore it if
> > it is used in a combination with the Hotplug in D3 GUID or the ACPI
> > _DSD device properties GUID. If the firmware combines the Hotplug in
> > D3 GUID or the ACPI _DSD device properties GUID with that property,
> > Windows will not regard it as valid, most likely, but Linux will use
> > it just fine. In the face of ASL bugs, which are inevitable (at least
> > just because ASL is code written by humans), that may become a real
> > problem, as systems successfully tested with Windows may not work with
> > Linux as expected and vice versa because of it.
>
> That's a fair point.
>
> > >From the ecosystem purity perspective this should be avoided, but then
> > I do realize that this allows code to be re-used and avoids quite a
> > bit of complexity. The likelihood of an ASL bug that will expose this
> > issue is arguably small, so maybe it is not a practical concern after
> > all.
>
> One option assuming we want to prevent the potential discrepancy you
> described above is to add ACPI specific property accessors that take
> GUID as parameter. Then it would only look properties inside that
> particular _DSD entry. I'm not sure if it is worth the added complexity,
> though.
I'm not sure if this is worth the extra complexity either, which is
why I have no strong opinion here. :-)
Maybe you can add a comment, next to the prp_guids[] definition, to
explain that the GUIDs are made equivalent to each other in order to
avoid extra complexity in the properties handling code, with the
caveat that the kernel will accept certain combinations of GUIDs and
properties that are not defined strictly speaking without warning, but
those combinations of GUIDs and properties are not expected to be used
by firmware and they should be caught by firmware validation tools and
reported as errors anyway.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/4] PCI / ACPI: Identify untrusted PCI devices
On Mon, Nov 26, 2018 at 12:15 PM Mika Westerberg
wrote:
>
> Recent systems with Thunderbolt ports may support IOMMU natively. This
> means that the platform utilizes IOMMU to prevent DMA attacks over
> externally exposed PCIe root ports (typically Thunderbolt ports)
>
> The system BIOS marks these PCIe root ports as being externally facing
> ports by implementing following ACPI _DSD [1] under the root port in
> question:
>
> Name (_DSD, Package () {
> ToUUID ("efcc06cc-73ac-4bc3-bff0-76143807c389"),
> Package () {
> Package () {"ExternalFacingPort", 1},
> Package () {"UID", 0 }
> }
> })
>
> To make it possible for IOMMU code to identify these devices, we look up
> for this property and mark all children devices (including the root port
> itself) with a new flag (->is_untrusted). This flag is meant to be used
> with all PCIe devices (not just Thunderbolt) that cannot be trusted in
> the same level than integrated devices and may need to put behind full
> IOMMU protection.
>
> [1]
> https://docs.microsoft.com/en-us/windows-hardware/drivers/pci/dsd-for-pcie-root-ports#identifying-externally-exposed-pcie-root-ports
>
> Signed-off-by: Mika Westerberg
> ---
> drivers/acpi/property.c | 3 +++
> drivers/pci/pci-acpi.c | 18 ++
> drivers/pci/probe.c | 22 ++
> include/linux/pci.h | 8
> 4 files changed, 51 insertions(+)
>
> diff --git a/drivers/acpi/property.c b/drivers/acpi/property.c
> index 8c7c4583b52d..4bdad32f62c8 100644
> --- a/drivers/acpi/property.c
> +++ b/drivers/acpi/property.c
> @@ -31,6 +31,9 @@ static const guid_t prp_guids[] = {
> /* Hotplug in D3 GUID: 6211e2c0-58a3-4af3-90e1-927a4e0c55a4 */
> GUID_INIT(0x6211e2c0, 0x58a3, 0x4af3,
> 0x90, 0xe1, 0x92, 0x7a, 0x4e, 0x0c, 0x55, 0xa4),
> + /* External facing port GUID: efcc06cc-73ac-4bc3-bff0-76143807c389 */
> + GUID_INIT(0xefcc06cc, 0x73ac, 0x4bc3,
> + 0xbf, 0xf0, 0x76, 0x14, 0x38, 0x07, 0xc3, 0x89),
> };
One observation here. Note that I really have no strong opinion on
that, so this is not an objection, but IMO it is fair to make things
clear for everybody.
So this causes the External facing port GUID (which already is the
case with the Hotplug in D3 GUID for that matter) to be practically
equivalent to the ACPI _DSD device properties GUID. This means that
Linux will consider a combination of any of these GUIDs with any
properties defined for any of them as valid, which need not be the
case in Windows.
For example, since the ExternalFacingPort property is defined along
with the External facing port GUID, Windows will likely ignore it if
it is used in a combination with the Hotplug in D3 GUID or the ACPI
_DSD device properties GUID. If the firmware combines the Hotplug in
D3 GUID or the ACPI _DSD device properties GUID with that property,
Windows will not regard it as valid, most likely, but Linux will use
it just fine. In the face of ASL bugs, which are inevitable (at least
just because ASL is code written by humans), that may become a real
problem, as systems successfully tested with Windows may not work with
Linux as expected and vice versa because of it.
>From the ecosystem purity perspective this should be avoided, but then
I do realize that this allows code to be re-used and avoids quite a
bit of complexity. The likelihood of an ASL bug that will expose this
issue is arguably small, so maybe it is not a practical concern after
all.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 1/4] PCI / ACPI: Identify untrusted PCI devices
On Tue, Nov 27, 2018 at 9:54 AM Mika Westerberg wrote: > > On Mon, Nov 26, 2018 at 06:17:11PM -0600, Bjorn Helgaas wrote: > > Hi Mika, > > Hi, > > > On Mon, Nov 26, 2018 at 02:15:23PM +0300, Mika Westerberg wrote: > > > Recent systems with Thunderbolt ports may support IOMMU natively. > > > > This sentence doesn't make sense to me. There's no logical connection > > between having an IOMMU and having a Thunderbolt port. > > > > > This means that the platform utilizes IOMMU to prevent DMA attacks > > > over externally exposed PCIe root ports (typically Thunderbolt > > > ports) > > > > Nor this one. The platform only uses the IOMMU to prevent DMA attacks > > if the OS chooses to do that. > > I guess I'm trying to say here that the recent changes add such support > to the platform BIOS that allows the OS to enable IOMMU without being > compromised by a malicious device that is already connected. The BIOS > sets the new ACPI DMAR bit in that case. > > > > The system BIOS marks these PCIe root ports as being externally facing > > > ports by implementing following ACPI _DSD [1] under the root port in > > > question: > > > > There's no standard that requires this, so the best we can say is that > > a system BIOS *may* mark externally facing ports with this mechanism. > > There is no standard but I'm quite sure this is something that will be > required to be implemented properly by the OEM by Microsoft hardware > compatibility suite. I think it would be fair to say that future versions of Windows will expect the firmware to identify the "externally facing" root PCIe ports as per the above which practically means that it is as good as a formal standard in the Windows world. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices
On Friday, November 16, 2018 11:57:38 AM CET Lorenzo Pieralisi wrote: > On Thu, Nov 15, 2018 at 07:33:54PM +, mario.limoncie...@dell.com wrote: > > > > > > > -Original Message- > > > From: Mika Westerberg > > > Sent: Thursday, November 15, 2018 1:01 PM > > > To: Lorenzo Pieralisi > > > Cc: Lukas Wunner; iommu@lists.linux-foundation.org; Joerg Roedel; David > > > Woodhouse; Lu Baolu; Ashok Raj; Bjorn Helgaas; Rafael J. Wysocki; Jacob > > > jun Pan; > > > Andreas Noever; Michael Jamet; Yehezkel Bernat; Christian Kellner; > > > Limonciello, > > > Mario; Anthony Wong; linux-a...@vger.kernel.org; > > > linux-...@vger.kernel.org; linux- > > > ker...@vger.kernel.org > > > Subject: Re: [PATCH 1/4] PCI / ACPI: Identify external PCI devices > > > > > > > > > [EXTERNAL EMAIL] > > > > > > On Thu, Nov 15, 2018 at 05:46:08PM +, Lorenzo Pieralisi wrote: > > > > Do you really need to parse it if the dev->is_thunderbolt check is > > > > enough ? > > > > > > Yes, we need to parse it one way or another. dev->is_thunderbolt is > > > based on heuristics which do not apply anymore when the thing gets > > > integrated in the SoC. > > > > > > The _DSD is there already (on existing systems) and is being used by > > > Windows so I don't understand why we cannot take advantage of it? Every > > > new system with Thunderbolt ports will have it. > > We have different opinions on this, there is no point in me reiterating > it over and over, I am against the approach taken to solve this problem > first in defining the bindings outside the ACPI specifications and > second by acquiescing to what has been done so that it will be done > over and over again. Arguably, however, we are on the receiving end of things here and even if we don't use this binding, that won't buy us anything (but it will introduce a fair amount of disappointment among both users and OEMs). If Windows uses it, then systems will ship with it regardless of what Linux does with it, so your "acquiescing to what has been done" argument leads to nowhere in this particular case. It's just a matter of whether or not Linux will provide the same level of functionality as Windows with respect to this and IMO it would be impractical to refuse to do that for purely formal reasons. > I will raise the point in the appropriate forum, it is up to Bjorn > and Rafael to decide on this patch. For the record, my opinion is that there's a little choice on whether or not to use this extra information that firmware is willing to give us. It could be defined in a better way and so on, but since it is in use anyway, we really have nothing to gain by refusing to use it. Now, where the handling of it belongs to is a separate matter that should be decided on its own. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 1/4] iommu/arm-smmu: Add pm_runtime/sleep ops
On Mon, Jul 23, 2018 at 7:59 AM, Marek Szyprowski wrote: > Hi Rafael, > > On 2018-07-11 22:36, Rafael J. Wysocki wrote: >> On Wed, Jul 11, 2018 at 3:40 PM, Marek Szyprowski >> wrote: [cut] >>> Frankly, if there are no other reasons I suggest to wire system >>> suspend/resume to pm_runtime_force_* helpers: >>> SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, >>> pm_runtime_force_resume). >> Not a good idea at all IMO. >> >> Use PM driver flags rather I'd say. > > Frankly, till now I wasn't aware of the DPM_FLAG_* in struct dev_pm_info > 'driver_flags'. They are a relatively recent addition. > I've briefly checked them but I don't see the equivalent > of using SET_SYSTEM_SLEEP_PM_OPS(pm_runtime_force_suspend, > pm_runtime_force_resume): keep device suspend if it was runtime suspended > AND really call pm_runtime_suspend if it was not runtime suspended on > system suspend. DPM_FLAG_SMART_SUSPEND is expected to cause that to happen. If you want the device to remain in suspend after the system-wide resume from sleep (if possible), you can set DPM_FLAG_LEAVE_SUSPENDED for it too. Currently a caveat is that genpd still doesn't support the flags. I have patches for that, but I haven't got to posting them yet. If you are interested, I can push this work forward relatively quickly, so please let me know. >>> This way you will have everything related to suspending and resuming in >>> one place and you would not need to bother about all possible cases (like >>> suspending from runtime pm active and suspending from runtime pm suspended >>> cases as well as restoring proper device state on resume). This is >>> especially important in recent kernel releases, where devices are >>> system-suspended regardless their runtime pm states (in older kernels >>> devices were first runtime resumed for system suspend, what made code >>> simpler, but wasn't best from power consumption perspective). >>> >>> If you go this way, You only need to ensure that runtime resume will also >>> restore proper device state besides enabling all the clocks. This will >>> also prepare your driver to properly operate inside power domain, where it >>> is possible for device to loose its internal state after runtime suspend >>> when respective power domain has been turned off. >> I'm not sure if you are aware of the pm_runtime_force_* limitations, though. > > What are those limitations? First off, they don't work with middle-layer code implementing its own PM callbacks that actually operate on devices (like the PCI bus type or the generic ACPI PM domain). This means that drivers using them may not work on systems with ACPI, for example. Second, pm_runtime_force_resume() will always try to leave the device in suspend during system-wide resume from sleep which may not be desirable. Finally, they expect the runtime PM status to be updated by system-wide PM callbacks of devices below the one using them (eg. its children and their children etc) which generally is not required and may not take place unless the drivers of those devices use pm_runtime_force_* themselves. So careful there. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 3/4] iommu/arm-smmu: Add the device_link between masters and smmu
On Mon, Jul 16, 2018 at 1:46 PM, Vivek Gautam wrote: > > > On 7/16/2018 2:25 PM, Rafael J. Wysocki wrote: >> >> On Thu, Jul 12, 2018 at 2:41 PM, Vivek Gautam >> wrote: >>> >>> Hi Rafael, >>> >>> >>> On Wed, Jul 11, 2018 at 4:06 PM, Vivek Gautam >>> wrote: >>>> >>>> Hi Rafael, >>>> >>>> >>>> >>>> On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote: >>>>> >>>>> On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote: >>>>>> >>>>>> From: Sricharan R >>>>>> >>>>>> Finally add the device link between the master device and >>>>>> smmu, so that the smmu gets runtime enabled/disabled only when the >>>>>> master needs it. This is done from add_device callback which gets >>>>>> called once when the master is added to the smmu. >>>>>> >>>>>> Signed-off-by: Sricharan R >>>>>> Signed-off-by: Vivek Gautam >>>>>> Reviewed-by: Tomasz Figa >>>>>> Cc: Rafael J. Wysocki >>>>>> Cc: Lukas Wunner >>>>>> --- >>>>>> >>>>>>- Change since v11 >>>>>> * Replaced DL_FLAG_AUTOREMOVE flag with >>>>>> DL_FLAG_AUTOREMOVE_SUPPLIER. >>>>>> >>>>>>drivers/iommu/arm-smmu.c | 12 >>>>>>1 file changed, 12 insertions(+) >>>>>> >>>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>>>> index 09265e206e2d..916cde4954d2 100644 >>>>>> --- a/drivers/iommu/arm-smmu.c >>>>>> +++ b/drivers/iommu/arm-smmu.c >>>>>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device >>>>>> *dev) >>>>>> iommu_device_link(&smmu->iommu, dev); >>>>>>+ if (pm_runtime_enabled(smmu->dev) && >>>>> >>>>> Why does the creation of the link depend on whether or not runtime PM >>>>> is enabled for the MMU device? >>>> >>>> >>>> The main purpose of this device link is to handle the runtime PM >>>> synchronization >>>> between the supplier (iommu) and consumer (client devices, such as >>>> GPU/display). >>>> Moreover, the runtime pm is conditionally enabled for smmu devices that >>>> support >>>> such [1]. >>> >>> Is there something you would like me to modify in this patch? >> >> Not really, as long as you are sure that it is correct. :-) >> >> You need to remember, however, that if you add system-wide PM >> callbacks to the driver, the ordering between them and the client >> device callbacks during system-wide suspend matters as well. Don't >> you need the link the ensure the correct system-wide suspend ordering >> too? > > > The fact that currently we handle clocks only through runtime pm callbacks, > would it be better to call runtime pm put/get in system-wide PM callbacks. > This would be same as i mentioned in the other thread. Well, my point is that there's no reason for system-wide suspend to depend directly on runtime PM (which can be effectively disabled by user space as mentioned for multiple times in this thread). It simply is not efficient to let the clock run while the system as a whole is sleeping (even if power/control has been set to "on" for this particular device) and it should not be too hard to prevent that from happening. You may need an additional flag in the driver for that or similar, but it definitely should be doable. Now, that's my advice and I'm not the maintainer of that code, so it is your call (as long as the maintainer agrees with it). Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 1/4] iommu/arm-smmu: Add pm_runtime/sleep ops
Hi,
On Mon, Jul 16, 2018 at 12:11 PM, Vivek Gautam
wrote:
> HI Rafael,
>
>
>
> On 7/16/2018 2:21 PM, Rafael J. Wysocki wrote:
>>
>> On Thu, Jul 12, 2018 at 12:57 PM, Vivek Gautam
>> wrote:
[cut]
>>>> Although, given the PM
>>>> subsystem internals, the suspend function wouldn't be called on SMMU
>>>> implementation needed power control (since they would have runtime PM
>>>> enabled) and on others, it would be called but do nothing (since no
>>>> clocks).
>>>>
>>>>> Honestly, I just don't know. :-)
>>>>>
>>>>> It just looks odd the way it is done. I think the clock should be
>>>>> gated during system-wide suspend too, because the system can spend
>>>>> much more time in a sleep state than in the working state, on average.
>>>>>
>>>>> And note that you cannot rely on runtime PM to always do it for you,
>>>>> because it may be disabled at a client device or even blocked by user
>>>>> space via power/control in sysfs and that shouldn't matter for
>>>>> system-wide PM.
>>>>
>>>> User space blocking runtime PM through sysfs is a good point. I'm not
>>>> 100% sure how the PM subsystem deals with that in case of system-wide
>>>> suspend. I guess for consistency and safety, we should have the
>>>> suspend callback.
>>>
>>> Will add the following suspend callback (same as
>>> arm_smmu_runtime_suspend):
>>>
>>> static int __maybe_unused arm_smmu_pm_suspend(struct device *dev)
>>> {
>>> struct arm_smmu_device *smmu = dev_get_drvdata(dev);
>>>
>>> clk_bulk_disable(smmu->num_clks, smmu->clks);
>>>
>>> return 0;
>>> }
>>
>> I think you also need to check if the clock has already been disabled
>> by runtime PM. Otherwise you may end up disabling it twice in a row.
>
>
> Should I rather call a pm_runtime_put() in suspend callback?
That wouldn't work as runtime PM may be effectively disabled by user
space via sysfs. That's one of the reasons why you need the extra
system-wide suspend callback in the first place. :-)
> Or an expanded form something similar to:
> https://elixir.bootlin.com/linux/v4.18-rc5/source/drivers/slimbus/qcom-ctrl.c#L695
Yes, you can do something like that, but be careful to make sure that
the state of the device after system-wide resume is consistent with
its runtime PM status in all cases.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 3/4] iommu/arm-smmu: Add the device_link between masters and smmu
On Thu, Jul 12, 2018 at 2:41 PM, Vivek Gautam wrote: > Hi Rafael, > > > On Wed, Jul 11, 2018 at 4:06 PM, Vivek Gautam > wrote: >> Hi Rafael, >> >> >> >> On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote: >>> >>> On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote: >>>> >>>> From: Sricharan R >>>> >>>> Finally add the device link between the master device and >>>> smmu, so that the smmu gets runtime enabled/disabled only when the >>>> master needs it. This is done from add_device callback which gets >>>> called once when the master is added to the smmu. >>>> >>>> Signed-off-by: Sricharan R >>>> Signed-off-by: Vivek Gautam >>>> Reviewed-by: Tomasz Figa >>>> Cc: Rafael J. Wysocki >>>> Cc: Lukas Wunner >>>> --- >>>> >>>> - Change since v11 >>>> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >>>> >>>> drivers/iommu/arm-smmu.c | 12 >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>> index 09265e206e2d..916cde4954d2 100644 >>>> --- a/drivers/iommu/arm-smmu.c >>>> +++ b/drivers/iommu/arm-smmu.c >>>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) >>>> iommu_device_link(&smmu->iommu, dev); >>>> + if (pm_runtime_enabled(smmu->dev) && >>> >>> Why does the creation of the link depend on whether or not runtime PM >>> is enabled for the MMU device? >> >> >> The main purpose of this device link is to handle the runtime PM >> synchronization >> between the supplier (iommu) and consumer (client devices, such as >> GPU/display). >> Moreover, the runtime pm is conditionally enabled for smmu devices that >> support >> such [1]. > > Is there something you would like me to modify in this patch? Not really, as long as you are sure that it is correct. :-) You need to remember, however, that if you add system-wide PM callbacks to the driver, the ordering between them and the client device callbacks during system-wide suspend matters as well. Don't you need the link the ensure the correct system-wide suspend ordering too? ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 1/4] iommu/arm-smmu: Add pm_runtime/sleep ops
On Thu, Jul 12, 2018 at 12:57 PM, Vivek Gautam
wrote:
> Hi,
>
>
> On Wed, Jul 11, 2018 at 6:21 PM, Tomasz Figa wrote:
>> On Wed, Jul 11, 2018 at 8:11 PM Rafael J. Wysocki wrote:
>>>
>>> On Wed, Jul 11, 2018 at 12:55 PM, Vivek Gautam
>>> wrote:
>>> > Hi Rafael,
>>> >
>>> >
>>> > On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki
>>> > wrote:
>>> >> On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote:
>>> >>> From: Sricharan R
>>> >>>
>>> >>> The smmu needs to be functional only when the respective
>>> >>> master's using it are active. The device_link feature
>>> >>> helps to track such functional dependencies, so that the
>>> >>> iommu gets powered when the master device enables itself
>>> >>> using pm_runtime. So by adapting the smmu driver for
>>> >>> runtime pm, above said dependency can be addressed.
>>> >>>
>>> >>> This patch adds the pm runtime/sleep callbacks to the
>>> >>> driver and also the functions to parse the smmu clocks
>>> >>> from DT and enable them in resume/suspend.
>>> >>>
>>> >>> Signed-off-by: Sricharan R
>>> >>> Signed-off-by: Archit Taneja
>>> >>> [vivek: Clock rework to request bulk of clocks]
>>> >>> Signed-off-by: Vivek Gautam
>>> >>> Reviewed-by: Tomasz Figa
>>> >>> ---
>>> >>>
>>> >>> - No change since v11.
>>> >>>
>>> >>> drivers/iommu/arm-smmu.c | 60
>>> >>> ++--
>>> >>> 1 file changed, 58 insertions(+), 2 deletions(-)
>>> >>>
>>> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> >>> index f7a96bcf94a6..a01d0dde21dd 100644
>>> >>> --- a/drivers/iommu/arm-smmu.c
>>> >>> +++ b/drivers/iommu/arm-smmu.c
>>> >>> @@ -48,6 +48,7 @@
>>> >>> #include
>>> >>> #include
>>> >>> #include
>>> >>> +#include
>>> >>> #include
>>> >>> #include
>>> >>>
>>> >>> @@ -205,6 +206,8 @@ struct arm_smmu_device {
>>> >>> u32 num_global_irqs;
>>> >>> u32 num_context_irqs;
>>> >>> unsigned int*irqs;
>>> >>> + struct clk_bulk_data*clks;
>>> >>> + int num_clks;
>>> >>>
>>> >>> u32 cavium_id_base; /* Specific to
>>> >>> Cavium */
>>> >>>
>>> >>> @@ -1897,10 +1900,12 @@ static int arm_smmu_device_cfg_probe(struct
>>> >>> arm_smmu_device *smmu)
>>> >>> struct arm_smmu_match_data {
>>> >>> enum arm_smmu_arch_version version;
>>> >>> enum arm_smmu_implementation model;
>>> >>> + const char * const *clks;
>>> >>> + int num_clks;
>>> >>> };
>>> >>>
>>> >>> #define ARM_SMMU_MATCH_DATA(name, ver, imp) \
>>> >>> -static struct arm_smmu_match_data name = { .version = ver, .model =
>>> >>> imp }
>>> >>> +static const struct arm_smmu_match_data name = { .version = ver,
>>> >>> .model = imp }
>>> >>>
>>> >>> ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
>>> >>> ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
>>> >>> @@ -1919,6 +1924,23 @@ static const struct of_device_id
>>> >>> arm_smmu_of_match[] = {
>>> >>> };
>>> >>> MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>>> >>>
>>> >>> +static void arm_smmu_fill_clk_data(struct arm_smmu_device *smmu,
>>> >>> +const char * const *clks)
>>> >>> +{
>>> >>> + int i;
>>> >>> +
>>> >>> + if (smmu->num_clks < 1)
>>> >>> + return;
>>> >>> +
>>> >>> + smmu
Re: [PATCH v12 1/4] iommu/arm-smmu: Add pm_runtime/sleep ops
On Wed, Jul 11, 2018 at 3:40 PM, Marek Szyprowski
wrote:
> Hi Tomasz,
>
> On 2018-07-11 14:51, Tomasz Figa wrote:
>> On Wed, Jul 11, 2018 at 8:11 PM Rafael J. Wysocki wrote:
>>> On Wed, Jul 11, 2018 at 12:55 PM, Vivek Gautam
>>> wrote:
>>>> On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki
>>>> wrote:
>>>>> On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote:
>>>>>> From: Sricharan R
>>>>>>
>>>>>> The smmu needs to be functional only when the respective
>>>>>> master's using it are active. The device_link feature
>>>>>> helps to track such functional dependencies, so that the
>>>>>> iommu gets powered when the master device enables itself
>>>>>> using pm_runtime. So by adapting the smmu driver for
>>>>>> runtime pm, above said dependency can be addressed.
>>>>>>
>>>>>> This patch adds the pm runtime/sleep callbacks to the
>>>>>> driver and also the functions to parse the smmu clocks
>>>>>> from DT and enable them in resume/suspend.
>>>>>>
>>>>>> Signed-off-by: Sricharan R
>>>>>> Signed-off-by: Archit Taneja
>>>>>> [vivek: Clock rework to request bulk of clocks]
>>>>>> Signed-off-by: Vivek Gautam
>>>>>> Reviewed-by: Tomasz Figa
>>>>>> ---
>>>>>>
>>>>>> - No change since v11.
>>>>>>
>>>>>> drivers/iommu/arm-smmu.c | 60
>>>>>> ++--
>>>>>> 1 file changed, 58 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>>>>> index f7a96bcf94a6..a01d0dde21dd 100644
>>>>>> --- a/drivers/iommu/arm-smmu.c
>>>>>> +++ b/drivers/iommu/arm-smmu.c
>>>>>> @@ -48,6 +48,7 @@
>>>>>> #include
>>>>>> #include
>>>>>> #include
>>>>>> +#include
>>>>>> #include
>>>>>> #include
>>>>>>
>>>>>> @@ -205,6 +206,8 @@ struct arm_smmu_device {
>>>>>>u32 num_global_irqs;
>>>>>>u32 num_context_irqs;
>>>>>>unsigned int*irqs;
>>>>>> + struct clk_bulk_data*clks;
>>>>>> + int num_clks;
>>>>>>
>>>>>>u32 cavium_id_base; /* Specific to
>>>>>> Cavium */
>>>>>>
>>>>>> @@ -1897,10 +1900,12 @@ static int arm_smmu_device_cfg_probe(struct
>>>>>> arm_smmu_device *smmu)
>>>>>> struct arm_smmu_match_data {
>>>>>>enum arm_smmu_arch_version version;
>>>>>>enum arm_smmu_implementation model;
>>>>>> + const char * const *clks;
>>>>>> + int num_clks;
>>>>>> };
>>>>>>
>>>>>> #define ARM_SMMU_MATCH_DATA(name, ver, imp) \
>>>>>> -static struct arm_smmu_match_data name = { .version = ver, .model = imp
>>>>>> }
>>>>>> +static const struct arm_smmu_match_data name = { .version = ver, .model
>>>>>> = imp }
>>>>>>
>>>>>> ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
>>>>>> ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
>>>>>> @@ -1919,6 +1924,23 @@ static const struct of_device_id
>>>>>> arm_smmu_of_match[] = {
>>>>>> };
>>>>>> MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>>>>>>
>>>>>> +static void arm_smmu_fill_clk_data(struct arm_smmu_device *smmu,
>>>>>> +const char * const *clks)
>>>>>> +{
>>>>>> + int i;
>>>>>> +
>>>>>> + if (smmu->num_clks < 1)
>>>>>> + return;
>>>>>> +
>>>>>> + smmu->clks = devm_kcalloc(smmu->dev, smmu->num_clks,
>>>>>> + sizeof(*smmu->clks), GFP_KERNEL);
>&g
Re: [PATCH v12 1/4] iommu/arm-smmu: Add pm_runtime/sleep ops
On Wed, Jul 11, 2018 at 12:55 PM, Vivek Gautam
wrote:
> Hi Rafael,
>
>
> On Wed, Jul 11, 2018 at 3:20 PM, Rafael J. Wysocki wrote:
>> On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote:
>>> From: Sricharan R
>>>
>>> The smmu needs to be functional only when the respective
>>> master's using it are active. The device_link feature
>>> helps to track such functional dependencies, so that the
>>> iommu gets powered when the master device enables itself
>>> using pm_runtime. So by adapting the smmu driver for
>>> runtime pm, above said dependency can be addressed.
>>>
>>> This patch adds the pm runtime/sleep callbacks to the
>>> driver and also the functions to parse the smmu clocks
>>> from DT and enable them in resume/suspend.
>>>
>>> Signed-off-by: Sricharan R
>>> Signed-off-by: Archit Taneja
>>> [vivek: Clock rework to request bulk of clocks]
>>> Signed-off-by: Vivek Gautam
>>> Reviewed-by: Tomasz Figa
>>> ---
>>>
>>> - No change since v11.
>>>
>>> drivers/iommu/arm-smmu.c | 60
>>> ++--
>>> 1 file changed, 58 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>>> index f7a96bcf94a6..a01d0dde21dd 100644
>>> --- a/drivers/iommu/arm-smmu.c
>>> +++ b/drivers/iommu/arm-smmu.c
>>> @@ -48,6 +48,7 @@
>>> #include
>>> #include
>>> #include
>>> +#include
>>> #include
>>> #include
>>>
>>> @@ -205,6 +206,8 @@ struct arm_smmu_device {
>>> u32 num_global_irqs;
>>> u32 num_context_irqs;
>>> unsigned int*irqs;
>>> + struct clk_bulk_data*clks;
>>> + int num_clks;
>>>
>>> u32 cavium_id_base; /* Specific to Cavium
>>> */
>>>
>>> @@ -1897,10 +1900,12 @@ static int arm_smmu_device_cfg_probe(struct
>>> arm_smmu_device *smmu)
>>> struct arm_smmu_match_data {
>>> enum arm_smmu_arch_version version;
>>> enum arm_smmu_implementation model;
>>> + const char * const *clks;
>>> + int num_clks;
>>> };
>>>
>>> #define ARM_SMMU_MATCH_DATA(name, ver, imp) \
>>> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
>>> +static const struct arm_smmu_match_data name = { .version = ver, .model =
>>> imp }
>>>
>>> ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
>>> ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
>>> @@ -1919,6 +1924,23 @@ static const struct of_device_id arm_smmu_of_match[]
>>> = {
>>> };
>>> MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>>>
>>> +static void arm_smmu_fill_clk_data(struct arm_smmu_device *smmu,
>>> +const char * const *clks)
>>> +{
>>> + int i;
>>> +
>>> + if (smmu->num_clks < 1)
>>> + return;
>>> +
>>> + smmu->clks = devm_kcalloc(smmu->dev, smmu->num_clks,
>>> + sizeof(*smmu->clks), GFP_KERNEL);
>>> + if (!smmu->clks)
>>> + return;
>>> +
>>> + for (i = 0; i < smmu->num_clks; i++)
>>> + smmu->clks[i].id = clks[i];
>>> +}
>>> +
>>> #ifdef CONFIG_ACPI
>>> static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
>>> {
>>> @@ -2001,6 +2023,9 @@ static int arm_smmu_device_dt_probe(struct
>>> platform_device *pdev,
>>> data = of_device_get_match_data(dev);
>>> smmu->version = data->version;
>>> smmu->model = data->model;
>>> + smmu->num_clks = data->num_clks;
>>> +
>>> + arm_smmu_fill_clk_data(smmu, data->clks);
>>>
>>> parse_driver_options(smmu);
>>>
>>> @@ -2099,6 +2124,14 @@ static int arm_smmu_device_probe(struct
>>> platform_device *pdev)
>>> smmu->irqs[i] = irq;
>>> }
>>>
>>> + err = devm_clk_bulk_get(smmu->dev, smmu->num_clks, smmu->clks);
>>> + if (err)
>>> + return err;
&
Re: [PATCH v12 2/4] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
On Wed, Jul 11, 2018 at 12:05 PM, Tomasz Figa wrote:
> Hi Rafael,
>
> Thanks for review.
>
> On Wed, Jul 11, 2018 at 6:53 PM Rafael J. Wysocki wrote:
>>
>> On Sunday, July 8, 2018 7:34:11 PM CEST Vivek Gautam wrote:
>> > From: Sricharan R
>> >
>> > The smmu device probe/remove and add/remove master device callbacks
>> > gets called when the smmu is not linked to its master, that is without
>> > the context of the master device. So calling runtime apis in those places
>> > separately.
>> >
>> > Signed-off-by: Sricharan R
>> > [vivek: Cleanup pm runtime calls]
>> > Signed-off-by: Vivek Gautam
>> > Reviewed-by: Tomasz Figa
>> > ---
>> >
>> > - Change since v11
>> >* Replaced pm_runtime_disable() with pm_runtime_force_suspend()
>> > to avoid warning about " Unpreparing enabled clock".
>> > Full warning text mentioned in cover patch.
>> >
>> > drivers/iommu/arm-smmu.c | 92
>> > +++-
>> > 1 file changed, 84 insertions(+), 8 deletions(-)
>> >
>> > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
>> > index a01d0dde21dd..09265e206e2d 100644
>> > --- a/drivers/iommu/arm-smmu.c
>> > +++ b/drivers/iommu/arm-smmu.c
>> > @@ -268,6 +268,20 @@ static struct arm_smmu_option_prop arm_smmu_options[]
>> > = {
>> > { 0, NULL},
>> > };
>> >
>> > +static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
>> > +{
>> > + if (pm_runtime_enabled(smmu->dev))
>>
>> Why do you need the pm_runtime_enabled() checks here and below?
>>
>> pm_runtime_get_sync() and pm_runtime_put() should work just fine if
>> runtime PM is not enabled.
>
> Because pm_runtime_get_sync() acquires a spin lock, even if only for
> the short time of checking if runtime PM is enabled and SMMU driver
> maintainers didn't want any spin locks in certain IOMMU API code paths
> on hardware implementations that don't need runtime PM, while we still
> need to be able to control runtime PM there on hardware
> implementations that need so.
OK, so it is an optimization. It would be good to put a comment in
there to that effect.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 3/4] iommu/arm-smmu: Add the device_link between masters and smmu
On Sunday, July 8, 2018 7:34:12 PM CEST Vivek Gautam wrote:
> From: Sricharan R
>
> Finally add the device link between the master device and
> smmu, so that the smmu gets runtime enabled/disabled only when the
> master needs it. This is done from add_device callback which gets
> called once when the master is added to the smmu.
>
> Signed-off-by: Sricharan R
> Signed-off-by: Vivek Gautam
> Reviewed-by: Tomasz Figa
> Cc: Rafael J. Wysocki
> Cc: Lukas Wunner
> ---
>
> - Change since v11
>* Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER.
>
> drivers/iommu/arm-smmu.c | 12
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index 09265e206e2d..916cde4954d2 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev)
>
> iommu_device_link(&smmu->iommu, dev);
>
> + if (pm_runtime_enabled(smmu->dev) &&
Why does the creation of the link depend on whether or not runtime PM
is enabled for the MMU device?
What about system-wide PM and system shutdown? Are they always guaranteed
to happen in the right order without the link?
> + !device_link_add(dev, smmu->dev,
> + DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER)) {
> + dev_err(smmu->dev, "Unable to add link to the consumer %s\n",
> + dev_name(dev));
> + ret = -ENODEV;
> + goto out_unlink;
> + }
> +
> return 0;
>
> +out_unlink:
> + iommu_device_unlink(&smmu->iommu, dev);
> + arm_smmu_master_free_smes(fwspec);
> out_cfg_free:
> kfree(cfg);
> out_free:
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v12 2/4] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
On Sunday, July 8, 2018 7:34:11 PM CEST Vivek Gautam wrote:
> From: Sricharan R
>
> The smmu device probe/remove and add/remove master device callbacks
> gets called when the smmu is not linked to its master, that is without
> the context of the master device. So calling runtime apis in those places
> separately.
>
> Signed-off-by: Sricharan R
> [vivek: Cleanup pm runtime calls]
> Signed-off-by: Vivek Gautam
> Reviewed-by: Tomasz Figa
> ---
>
> - Change since v11
>* Replaced pm_runtime_disable() with pm_runtime_force_suspend()
> to avoid warning about " Unpreparing enabled clock".
> Full warning text mentioned in cover patch.
>
> drivers/iommu/arm-smmu.c | 92
> +++-
> 1 file changed, 84 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index a01d0dde21dd..09265e206e2d 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -268,6 +268,20 @@ static struct arm_smmu_option_prop arm_smmu_options[] = {
> { 0, NULL},
> };
>
> +static inline int arm_smmu_rpm_get(struct arm_smmu_device *smmu)
> +{
> + if (pm_runtime_enabled(smmu->dev))
Why do you need the pm_runtime_enabled() checks here and below?
pm_runtime_get_sync() and pm_runtime_put() should work just fine if
runtime PM is not enabled.
> + return pm_runtime_get_sync(smmu->dev);
> +
> + return 0;
> +}
> +
> +static inline void arm_smmu_rpm_put(struct arm_smmu_device *smmu)
> +{
> + if (pm_runtime_enabled(smmu->dev))
> + pm_runtime_put(smmu->dev);
> +}
> +
> static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> {
> return container_of(dom, struct arm_smmu_domain, domain);
> @@ -913,11 +927,15 @@ static void arm_smmu_destroy_domain_context(struct
> iommu_domain *domain)
> struct arm_smmu_domain *smmu_domain = to_smmu_domain(domain);
> struct arm_smmu_device *smmu = smmu_domain->smmu;
> struct arm_smmu_cfg *cfg = &smmu_domain->cfg;
> - int irq;
> + int ret, irq;
>
> if (!smmu || domain->type == IOMMU_DOMAIN_IDENTITY)
> return;
>
> + ret = arm_smmu_rpm_get(smmu);
> + if (ret < 0)
> + return;
> +
> /*
>* Disable the context bank and free the page tables before freeing
>* it.
> @@ -932,6 +950,8 @@ static void arm_smmu_destroy_domain_context(struct
> iommu_domain *domain)
>
> free_io_pgtable_ops(smmu_domain->pgtbl_ops);
> __arm_smmu_free_bitmap(smmu->context_map, cfg->cbndx);
> +
> + arm_smmu_rpm_put(smmu);
> }
>
> static struct iommu_domain *arm_smmu_domain_alloc(unsigned type)
> @@ -1213,10 +1233,15 @@ static int arm_smmu_attach_dev(struct iommu_domain
> *domain, struct device *dev)
> return -ENODEV;
>
> smmu = fwspec_smmu(fwspec);
> +
> + ret = arm_smmu_rpm_get(smmu);
> + if (ret < 0)
> + return ret;
> +
> /* Ensure that the domain is finalised */
> ret = arm_smmu_init_domain_context(domain, smmu);
> if (ret < 0)
> - return ret;
> + goto rpm_put;
>
> /*
>* Sanity check the domain. We don't support domains across
> @@ -1226,33 +1251,50 @@ static int arm_smmu_attach_dev(struct iommu_domain
> *domain, struct device *dev)
> dev_err(dev,
> "cannot attach to SMMU %s whilst already attached to
> domain on SMMU %s\n",
> dev_name(smmu_domain->smmu->dev), dev_name(smmu->dev));
> - return -EINVAL;
> + ret = -EINVAL;
> + goto rpm_put;
> }
>
> /* Looks ok, so add the device to the domain */
> - return arm_smmu_domain_add_master(smmu_domain, fwspec);
> + ret = arm_smmu_domain_add_master(smmu_domain, fwspec);
> +
> +rpm_put:
> + arm_smmu_rpm_put(smmu);
> + return ret;
> }
>
> static int arm_smmu_map(struct iommu_domain *domain, unsigned long iova,
> phys_addr_t paddr, size_t size, int prot)
> {
> struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
> + struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu;
> + int ret;
>
> if (!ops)
> return -ENODEV;
>
> - return ops->map(ops, iova, paddr, size, prot);
> + arm_smmu_rpm_get(smmu);
> + ret = ops->map(ops, iova, paddr, size, prot);
> + arm_smmu_rpm_put(smmu);
> +
> + return ret;
> }
>
> static size_t arm_smmu_unmap(struct iommu_domain *domain, unsigned long iova,
>size_t size)
> {
> struct io_pgtable_ops *ops = to_smmu_domain(domain)->pgtbl_ops;
> + struct arm_smmu_device *smmu = to_smmu_domain(domain)->smmu;
> + size_t ret;
>
> if (!ops)
> return 0;
>
> - return ops->unmap(ops, iova, size);
> + arm_smmu_rpm_get(smmu);
> + ret = ops->unmap(ops, iova, size);
> + arm_smmu_rpm_put(smmu);
> +
> +
Re: [PATCH v12 1/4] iommu/arm-smmu: Add pm_runtime/sleep ops
On Sunday, July 8, 2018 7:34:10 PM CEST Vivek Gautam wrote:
> From: Sricharan R
>
> The smmu needs to be functional only when the respective
> master's using it are active. The device_link feature
> helps to track such functional dependencies, so that the
> iommu gets powered when the master device enables itself
> using pm_runtime. So by adapting the smmu driver for
> runtime pm, above said dependency can be addressed.
>
> This patch adds the pm runtime/sleep callbacks to the
> driver and also the functions to parse the smmu clocks
> from DT and enable them in resume/suspend.
>
> Signed-off-by: Sricharan R
> Signed-off-by: Archit Taneja
> [vivek: Clock rework to request bulk of clocks]
> Signed-off-by: Vivek Gautam
> Reviewed-by: Tomasz Figa
> ---
>
> - No change since v11.
>
> drivers/iommu/arm-smmu.c | 60
> ++--
> 1 file changed, 58 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c
> index f7a96bcf94a6..a01d0dde21dd 100644
> --- a/drivers/iommu/arm-smmu.c
> +++ b/drivers/iommu/arm-smmu.c
> @@ -48,6 +48,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
>
> @@ -205,6 +206,8 @@ struct arm_smmu_device {
> u32 num_global_irqs;
> u32 num_context_irqs;
> unsigned int*irqs;
> + struct clk_bulk_data*clks;
> + int num_clks;
>
> u32 cavium_id_base; /* Specific to Cavium */
>
> @@ -1897,10 +1900,12 @@ static int arm_smmu_device_cfg_probe(struct
> arm_smmu_device *smmu)
> struct arm_smmu_match_data {
> enum arm_smmu_arch_version version;
> enum arm_smmu_implementation model;
> + const char * const *clks;
> + int num_clks;
> };
>
> #define ARM_SMMU_MATCH_DATA(name, ver, imp) \
> -static struct arm_smmu_match_data name = { .version = ver, .model = imp }
> +static const struct arm_smmu_match_data name = { .version = ver, .model =
> imp }
>
> ARM_SMMU_MATCH_DATA(smmu_generic_v1, ARM_SMMU_V1, GENERIC_SMMU);
> ARM_SMMU_MATCH_DATA(smmu_generic_v2, ARM_SMMU_V2, GENERIC_SMMU);
> @@ -1919,6 +1924,23 @@ static const struct of_device_id arm_smmu_of_match[] =
> {
> };
> MODULE_DEVICE_TABLE(of, arm_smmu_of_match);
>
> +static void arm_smmu_fill_clk_data(struct arm_smmu_device *smmu,
> +const char * const *clks)
> +{
> + int i;
> +
> + if (smmu->num_clks < 1)
> + return;
> +
> + smmu->clks = devm_kcalloc(smmu->dev, smmu->num_clks,
> + sizeof(*smmu->clks), GFP_KERNEL);
> + if (!smmu->clks)
> + return;
> +
> + for (i = 0; i < smmu->num_clks; i++)
> + smmu->clks[i].id = clks[i];
> +}
> +
> #ifdef CONFIG_ACPI
> static int acpi_smmu_get_data(u32 model, struct arm_smmu_device *smmu)
> {
> @@ -2001,6 +2023,9 @@ static int arm_smmu_device_dt_probe(struct
> platform_device *pdev,
> data = of_device_get_match_data(dev);
> smmu->version = data->version;
> smmu->model = data->model;
> + smmu->num_clks = data->num_clks;
> +
> + arm_smmu_fill_clk_data(smmu, data->clks);
>
> parse_driver_options(smmu);
>
> @@ -2099,6 +2124,14 @@ static int arm_smmu_device_probe(struct
> platform_device *pdev)
> smmu->irqs[i] = irq;
> }
>
> + err = devm_clk_bulk_get(smmu->dev, smmu->num_clks, smmu->clks);
> + if (err)
> + return err;
> +
> + err = clk_bulk_prepare(smmu->num_clks, smmu->clks);
> + if (err)
> + return err;
> +
> err = arm_smmu_device_cfg_probe(smmu);
> if (err)
> return err;
> @@ -2181,6 +2214,9 @@ static int arm_smmu_device_remove(struct
> platform_device *pdev)
>
> /* Turn the thing off */
> writel(sCR0_CLIENTPD, ARM_SMMU_GR0_NS(smmu) + ARM_SMMU_GR0_sCR0);
> +
> + clk_bulk_unprepare(smmu->num_clks, smmu->clks);
> +
> return 0;
> }
>
> @@ -2197,7 +2233,27 @@ static int __maybe_unused arm_smmu_pm_resume(struct
> device *dev)
> return 0;
> }
>
> -static SIMPLE_DEV_PM_OPS(arm_smmu_pm_ops, NULL, arm_smmu_pm_resume);
> +static int __maybe_unused arm_smmu_runtime_resume(struct device *dev)
> +{
> + struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +
> + return clk_bulk_enable(smmu->num_clks, smmu->clks);
> +}
> +
> +static int __maybe_unused arm_smmu_runtime_suspend(struct device *dev)
> +{
> + struct arm_smmu_device *smmu = dev_get_drvdata(dev);
> +
> + clk_bulk_disable(smmu->num_clks, smmu->clks);
> +
> + return 0;
> +}
> +
> +static const struct dev_pm_ops arm_smmu_pm_ops = {
> + SET_SYSTEM_SLEEP_PM_OPS(NULL, arm_smmu_pm_resume)
This is suspicious.
If you need a runtime suspend method, why do you think that it is not necessary
to suspend the device during system-wide transitions?
> +
Re: [PATCH v9 1/5] driver core: Find an existing link between two devices
On Wednesday, March 14, 2018 12:50:54 PM CET Tomasz Figa wrote: > On Wed, Mar 14, 2018 at 8:12 PM, Rafael J. Wysocki wrote: > > On Tuesday, March 13, 2018 12:23:34 PM CET Tomasz Figa wrote: > >> On Tue, Mar 13, 2018 at 7:34 PM, Vivek Gautam > >> wrote: > >> > Hi Tomasz, > >> > > >> > On Tue, Mar 13, 2018 at 3:45 PM, Tomasz Figa wrote: > >> >> Hi Vivek, > >> >> > >> >> Thanks for the patch. > >> >> > >> >> On Tue, Mar 13, 2018 at 5:55 PM, Vivek Gautam > >> >> wrote: > >> >>> The lists managing the device-links can be traversed to > >> >>> find the link between two devices. The device_link_add() APIs > >> >>> does traverse these lists to check if there's already a link > >> >>> setup between the two devices. > >> >>> So, add a new APIs, device_link_find(), to find an existing > >> >>> device link between two devices - suppliers and consumers. > >> >> > >> >> I'm wondering if this API would be useful for anything else that the > >> >> problem we're trying to solve with deleting links without storing them > >> >> anywhere. Perhaps a device_link_del_dev(consumer, supplier) would be a > >> >> better alternative? > >> > > >> > Yea, that sounds simpler i think. Will add this API instead of > >> > find_link(). Thanks. > >> > >> Perhaps let's wait for a moment to see if there are other opinions. :) > >> > >> Rafael, Lucas, any thoughts? > > > > It is not clear to me what the device_link_del_dev(consumer, supplier) > > would do. > > It would delete a link between consumer and supplier. If there's one I suppose. I'm wondering if you are somehow trying to address the same problem as the device links reference counting patch from Lukas that has been queued up for 4.17 already. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 1/5] driver core: Find an existing link between two devices
On Tuesday, March 13, 2018 12:23:34 PM CET Tomasz Figa wrote: > On Tue, Mar 13, 2018 at 7:34 PM, Vivek Gautam > wrote: > > Hi Tomasz, > > > > On Tue, Mar 13, 2018 at 3:45 PM, Tomasz Figa wrote: > >> Hi Vivek, > >> > >> Thanks for the patch. > >> > >> On Tue, Mar 13, 2018 at 5:55 PM, Vivek Gautam > >> wrote: > >>> The lists managing the device-links can be traversed to > >>> find the link between two devices. The device_link_add() APIs > >>> does traverse these lists to check if there's already a link > >>> setup between the two devices. > >>> So, add a new APIs, device_link_find(), to find an existing > >>> device link between two devices - suppliers and consumers. > >> > >> I'm wondering if this API would be useful for anything else that the > >> problem we're trying to solve with deleting links without storing them > >> anywhere. Perhaps a device_link_del_dev(consumer, supplier) would be a > >> better alternative? > > > > Yea, that sounds simpler i think. Will add this API instead of > > find_link(). Thanks. > > Perhaps let's wait for a moment to see if there are other opinions. :) > > Rafael, Lucas, any thoughts? It is not clear to me what the device_link_del_dev(consumer, supplier) would do. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v9 1/5] driver core: Find an existing link between two devices
On Tuesday, March 13, 2018 9:55:30 AM CET Vivek Gautam wrote:
> The lists managing the device-links can be traversed to
> find the link between two devices. The device_link_add() APIs
> does traverse these lists to check if there's already a link
> setup between the two devices.
> So, add a new APIs, device_link_find(), to find an existing
> device link between two devices - suppliers and consumers.
>
> Signed-off-by: Vivek Gautam
> Cc: Rafael J. Wysocki
> Cc: Greg Kroah-Hartman
> ---
>
> * New patch added to this series.
>
> drivers/base/core.c| 30 +++---
> include/linux/device.h | 2 ++
> 2 files changed, 29 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 5847364f25d9..e8c9774e4ba2 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -144,6 +144,30 @@ static int device_reorder_to_tail(struct device *dev,
> void *not_used)
> return 0;
> }
>
> +/**
> + * device_link_find - find any existing link between two devices.
> + * @consumer: Consumer end of the link.
> + * @supplier: Supplier end of the link.
> + *
> + * Returns pointer to the existing link between a supplier and
> + * and consumer devices, or NULL if no link exists.
> + */
> +struct device_link *device_link_find(struct device *consumer,
> + struct device *supplier)
> +{
> + struct device_link *link = NULL;
> +
> + if (!consumer || !supplier)
> + return NULL;
> +
> + list_for_each_entry(link, &supplier->links.consumers, s_node)
> + if (link->consumer == consumer)
> + break;
> +
Any mutual exclusion?
Or is the caller expected to take care of it? And if so, then how?
> + return link;
> +}
> +EXPORT_SYMBOL_GPL(device_link_find);
> +
> /**
> * device_link_add - Create a link between two devices.
> * @consumer: Consumer end of the link.
> @@ -195,9 +219,9 @@ struct device_link *device_link_add(struct device
> *consumer,
> goto out;
> }
>
> - list_for_each_entry(link, &supplier->links.consumers, s_node)
> - if (link->consumer == consumer)
> - goto out;
> + link = device_link_find(consumer, supplier);
> + if (link)
> + goto out;
>
> link = kzalloc(sizeof(*link), GFP_KERNEL);
> if (!link)
> diff --git a/include/linux/device.h b/include/linux/device.h
> index b093405ed525..13bc1884c3eb 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -1278,6 +1278,8 @@ extern const char *dev_driver_string(const struct
> device *dev);
> struct device_link *device_link_add(struct device *consumer,
> struct device *supplier, u32 flags);
> void device_link_del(struct device_link *link);
> +struct device_link *device_link_find(struct device *consumer,
> + struct device *supplier);
>
> #ifdef CONFIG_PRINTK
>
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 1/6] base: power: runtime: Export pm_runtime_get/put_suppliers
On Tue, Jan 9, 2018 at 11:01 AM, Vivek Gautam
wrote:
> The device link allows the pm framework to tie the supplier and
> consumer. So, whenever the consumer is powered-on the supplier
> is powered-on first.
>
> There are however cases in which the consumer wants to power-on
> the supplier, but not itself.
> E.g., A Graphics or multimedia driver wants to power-on the SMMU
> to unmap a buffer and finish the TLB operations without powering
> on itself. Some of these unmap requests are coming from the
> user space when the controller itself is not powered-up, and it
> can be huge penalty in terms of power and latency to power-up
> the graphics/mm controllers.
> There can be an argument that the supplier should handle this case
> on its own and there should not be a need for the consumer to
> power-on the supplier. But as discussed on the thread [1] about
> ARM-SMMU runtime pm, we don't want to introduce runtime pm calls
> in atomic path in arm_smmu_unmap.
>
> [1] https://patchwork.kernel.org/patch/9827825/
>
> Signed-off-by: Vivek Gautam
Acked-by: Rafael J. Wysocki
Please feel free to route this along with the rest of the series.
Thanks!
> ---
>
> * This is v2 of the patch [1]. Adding it to this patch series.
>[1] https://patchwork.kernel.org/patch/10102447/
>
> drivers/base/power/runtime.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 6e89b51ea3d9..06a2a88fe866 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -1579,6 +1579,7 @@ void pm_runtime_get_suppliers(struct device *dev)
>
> device_links_read_unlock(idx);
> }
> +EXPORT_SYMBOL_GPL(pm_runtime_get_suppliers);
>
> /**
> * pm_runtime_put_suppliers - Drop references to supplier devices.
> @@ -1597,6 +1598,7 @@ void pm_runtime_put_suppliers(struct device *dev)
>
> device_links_read_unlock(idx);
> }
> +EXPORT_SYMBOL_GPL(pm_runtime_put_suppliers);
>
> void pm_runtime_new_link(struct device *dev)
> {
> --
> QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member
> of Code Aurora Forum, hosted by The Linux Foundation
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 0/6] iommu/arm-smmu: Add runtime pm/sleep support
On Tuesday, January 9, 2018 11:01:43 AM CET Vivek Gautam wrote:
> This series provides the support for turning on the arm-smmu's
> clocks/power domains using runtime pm. This is done using the
> recently introduced device links patches, which lets the smmu's
> runtime to follow the master's runtime pm, so the smmu remains
> powered only when the masters use it.
>
> It also adds support for Qcom's arm-smmu-v2 variant that
> has different clocks and power requirements.
>
> Took some reference from the exynos runtime patches [2].
>
> Previous version of the patchset [1].
>
> After much discussion [4] over the use of pm_runtime_get/put() in
> .unmap op path for the arm-smmu, and after disussing over more than
> a couple of approaches to address this, we are putting forward the
> changes *without* using pm_runtime APIs in 'unmap'. Rather, letting
> the client device take the control of powering on/off the connected
> iommu through pm_runtime_get(put)_suppliers() APIs for the scnerios
> when the iommu power can't be directly controlled by clients through
> device links.
> Rafael has agreed to export the suppliers APIs [5].
>
> [V5]
>* Dropped runtime pm calls from "arm_smmu_unmap" op as discussed over
> the list [4] for the last patch series.
>* Added a patch to export pm_runtime_get/put_suppliers() APIs to the
> series as agreed with Rafael [5].
>* Added the related patch for msm drm iommu layer to use
> pm_runtime_get/put_suppliers() APIs in msm_mmu_funcs.
>* Dropped arm-mmu500 clock patch since that would break existing
> platforms.
>* Changed compatible 'qcom,msm8996-smmu-v2' to 'qcom,smmu-v2' to reflect
> the IP version rather than the platform on which it is used.
> The same IP is used across multiple platforms including msm8996,
> and sdm845 etc.
>* Using clock bulk APIs to handle the clocks available to the IP as
> suggested by Stephen Boyd.
>* The first patch in v4 version of the patch-series:
> ("iommu/arm-smmu: Fix the error path in arm_smmu_add_device") has
> already made it to mainline.
>
> [V4]
>* Reworked the clock handling part. We now take clock names as data
> in the driver for supported compatible versions, and loop over them
> to get, enable, and disable the clocks.
>* Using qcom,msm8996 based compatibles for bindings instead of a generic
> qcom compatible.
>* Refactor MMU500 patch to just add the necessary clock names data and
> corresponding bindings.
>* Added the pm_runtime_get/put() calls in .unmap iommu op (fix added by
> Stanimir on top of previous patch version.
>* Added a patch to fix error path in arm_smmu_add_device()
>* Removed patch 3/5 of V3 patch series that added qcom,smmu-v2 bindings.
>
> [V3]
>* Reworked the patches to keep the clocks init/enabling function
> separately for each compatible.
>
>* Added clocks bindings for MMU40x/500.
>
>* Added a new compatible for qcom,smmu-v2 implementation and
> the clock bindings for the same.
>
>* Rebased on top of 4.11-rc1
>
> [V2]
>* Split the patches little differently.
>
>* Addressed comments.
>
>* Removed the patch #4 [3] from previous post
> for arm-smmu context save restore. Planning to
> post this separately after reworking/addressing Robin's
> feedback.
>
>* Reversed the sequence to disable clocks than enabling.
> This was required for those cases where the
> clocks are populated in a dependent order from DT.
>
> [1] https://www.spinics.net/lists/arm-kernel/msg567488.html
> [2] https://lkml.org/lkml/2016/10/20/70
> [3] https://patchwork.kernel.org/patch/9389717/
> [4] https://patchwork.kernel.org/patch/9827825/
> [5] https://patchwork.kernel.org/patch/10102445/
>
> Sricharan R (3):
> iommu/arm-smmu: Add pm_runtime/sleep ops
> iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device
> iommu/arm-smmu: Add the device_link between masters and smmu
>
> Vivek Gautam (3):
> base: power: runtime: Export pm_runtime_get/put_suppliers
> iommu/arm-smmu: Add support for qcom,smmu-v2 variant
> drm/msm: iommu: Replace runtime calls with runtime suppliers
>
> .../devicetree/bindings/iommu/arm,smmu.txt | 35 ++
> drivers/base/power/runtime.c | 2 +
> drivers/gpu/drm/msm/msm_iommu.c| 16 +--
> drivers/iommu/arm-smmu.c | 124
> -
> 4 files changed, 163 insertions(+), 14 deletions(-)
I need some time to review the series.
Thanks,
Rafael
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 0/3] Cavium ThunderX2 SMMUv3 errata workarounds
On Thu, Jun 8, 2017 at 6:32 PM, Lorenzo Pieralisi wrote: > On Tue, May 30, 2017 at 05:33:38PM +0530, Geetha sowjanya wrote: >> Cavium ThunderX2 SMMUv3 implementation has two Silicon Erratas. >> 1. Errata ID #74 >>SMMU register alias Page 1 is not implemented >> 2. Errata ID #126 >>SMMU doesnt support unique IRQ lines and also MSI for gerror, >>eventq and cmdq-sync >> >> The following patchset does software workaround for these two erratas. >> >> This series is based on patchset. >> https://www.spinics.net/lists/arm-kernel/msg578443.html > > Yes so it is not standalone. How are we going to merge these > ACPI IORT/ACPICA/SMMU patches - inclusive of: > > [1] https://www.spinics.net/lists/arm-kernel/msg586458.html > > Rafael, do ACPICA patches go upstream via the ACPI tree pull request ? Not as a rule. > To remove dependency on ACPICA changes this series needs updating > anyway and for [1] above I think the only solution is for all the > patches to go via the ACPI tree (if ACPICA updates go upstream with it). I think we may ask Lv to backport the header changes once they have been merged into Linux. Lv, would that work? Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 3/6] ACPI/IORT: Ignore all errors except EPROBE_DEFER
On Saturday, May 27, 2017 07:17:42 PM Sricharan R wrote:
> While deferring the probe of IOMMU masters, xlate and
> add_device callbacks called from iort_iommu_configure
> can pass back error values like -ENODEV, which means
> the IOMMU cannot be connected with that master for real
> reasons. Before the IOMMU probe deferral, all such errors
> were ignored. Now all those errors are propagated back,
> killing the master's probe for such errors. Instead ignore
> all the errors except EPROBE_DEFER, which is the only one
> of concern and let the master work without IOMMU, thus
> restoring the old behavior. Also make explicit that
> acpi_dma_configure handles only -EPROBE_DEFER from
> iort_iommu_configure.
>
> Fixes: 5a1bb638d567 ("drivers: acpi: Handle IOMMU lookup failure with
> deferred probing or error")
> Signed-off-by: Sricharan R
> ---
> drivers/acpi/arm64/iort.c | 6 ++
> drivers/acpi/scan.c | 4 ++--
> 2 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index c5fecf9..16e101f 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -782,6 +782,12 @@ const struct iommu_ops *iort_iommu_configure(struct
> device *dev)
> if (err)
> ops = ERR_PTR(err);
>
> + /* Ignore all other errors apart from EPROBE_DEFER */
> + if (IS_ERR(ops) && (PTR_ERR(ops) != -EPROBE_DEFER)) {
> + dev_dbg(dev, "Adding to IOMMU failed: %ld\n", PTR_ERR(ops));
> + ops = NULL;
> + }
> +
> return ops;
> }
>
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index e39ec7b..3a10d757 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1371,8 +1371,8 @@ int acpi_dma_configure(struct device *dev, enum
> dev_dma_attr attr)
> iort_set_dma_mask(dev);
>
> iommu = iort_iommu_configure(dev);
> - if (IS_ERR(iommu))
> - return PTR_ERR(iommu);
> + if (IS_ERR(iommu) && PTR_ERR(iommu) == -EPROBE_DEFER)
> + return -EPROBE_DEFER;
>
> size = max(dev->coherent_dma_mask, dev->coherent_dma_mask + 1);
> /*
>
ACK for the scan.c change and I'm assuming this to go in via ARM64.
Thanks,
Rafael
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [v5 1/4] ACPICA: IORT: Add Cavium ThunderX2 SMMUv3 model definition.
On Thursday, May 11, 2017 09:45:25 AM Will Deacon wrote: > On Thu, May 11, 2017 at 02:26:02AM +0200, Rafael J. Wysocki wrote: > > On Wednesday, May 10, 2017 05:01:55 PM Geetha sowjanya wrote: > > > From: Linu Cherian > > > > > > Add SMMUv3 model definition for ThunderX2. > > > > > > Signed-off-by: Linu Cherian > > > Signed-off-by: Geetha Sowjanya > > > > This is an ACPICA change, but you have not included the ACPICA maintainers > > into your original CC list (added now). > > > > Bob, Lv, how should this be routed? > > > > Do you want to apply this patch upstream first or can we make this change in > > Linux and upstream in parallel? That shouldn't be a big deal, right? > > I think we're still waiting for the updated IORT document to be published (I > think this should be in the next week or so), so I don't think we should > commit the new ID before that happens. OK, thanks for the heads-up. I think it's better to submit the actbl2.h update directly to ACPICA upstream when the doc is published and then work on top of that. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [v5 1/4] ACPICA: IORT: Add Cavium ThunderX2 SMMUv3 model definition.
On Wednesday, May 10, 2017 05:01:55 PM Geetha sowjanya wrote:
> From: Linu Cherian
>
> Add SMMUv3 model definition for ThunderX2.
>
> Signed-off-by: Linu Cherian
> Signed-off-by: Geetha Sowjanya
This is an ACPICA change, but you have not included the ACPICA maintainers
into your original CC list (added now).
Bob, Lv, how should this be routed?
Do you want to apply this patch upstream first or can we make this change in
Linux and upstream in parallel? That shouldn't be a big deal, right?
> ---
> include/acpi/actbl2.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/acpi/actbl2.h b/include/acpi/actbl2.h
> index faa9f2c..76a6f5d 100644
> --- a/include/acpi/actbl2.h
> +++ b/include/acpi/actbl2.h
> @@ -779,6 +779,8 @@ struct acpi_iort_smmu {
> #define ACPI_IORT_SMMU_CORELINK_MMU400 0x0002 /* ARM Corelink MMU-400
> */
> #define ACPI_IORT_SMMU_CORELINK_MMU500 0x0003 /* ARM Corelink MMU-500
> */
>
> +#define ACPI_IORT_SMMU_V3_CAVIUM_CN99XX 0x0002 /* Cavium ThunderX2
> SMMUv3 */
> +
> /* Masks for Flags field above */
>
> #define ACPI_IORT_SMMU_DVM_SUPPORTED(1)
>
Thanks,
Rafael
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V7 06/11] of/acpi: Configure dma operations at probe time for platform/amba/pci bus devices
On Mon, Jan 23, 2017 at 5:18 PM, Sricharan R wrote: > Configuring DMA ops at probe time will allow deferring device probe when > the IOMMU isn't available yet. The dma_configure for the device is > now called from the generic device_attach callback just before the > bus/driver probe is called. This way, configuring the DMA ops for the > device would be called at the same place for all bus_types, hence the > deferred probing mechanism should work for all buses as well. > > pci_bus_add_devices(platform/amba)(_device_create/driver_register) >| | > pci_bus_add_device (device_add/driver_register) >| | > device_attach device_initial_probe >| | > __device_attach_driver__device_attach_driver >| > driver_probe_device >| > really_probe >| > dma_configure > > Similarly on the device/driver_unregister path __device_release_driver is > called which inturn calls dma_deconfigure. > > This patch changes the dma ops configuration to probe time for > both OF and ACPI based platform/amba/pci bus devices. > > Signed-off-by: Sricharan R Acked-by: Rafael J. Wysocki ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] ACPI / DMAR: Avoid passing NULL to acpi_put_table()
From: Rafael J. Wysocki
Linus reported that commit 174cc7187e6f "ACPICA: Tables: Back port
acpi_get_table_with_size() and early_acpi_os_unmap_memory() from
Linux kernel" added a new warning on his desktop system:
ACPI Warning: Table 9fe6c0a0, Validation count is zero before decrement
which turns out to come from the acpi_put_table() in
detect_intel_iommu().
This happens if the DMAR table is not present in which case NULL is
passed to acpi_put_table() which doesn't check against that and
attempts to handle it regardless.
For this reason, check the pointer passed to acpi_put_table()
before invoking it.
Reported-by: Linus Torvalds
Fixes: 6b11d1d67713 ("ACPI / osl: Remove
acpi_get_table_with_size()/early_acpi_os_unmap_memory() users")
Signed-off-by: Rafael J. Wysocki
---
drivers/iommu/dmar.c |6 --
1 file changed, 4 insertions(+), 2 deletions(-)
Index: linux-pm/drivers/iommu/dmar.c
===
--- linux-pm.orig/drivers/iommu/dmar.c
+++ linux-pm/drivers/iommu/dmar.c
@@ -903,8 +903,10 @@ int __init detect_intel_iommu(void)
x86_init.iommu.iommu_init = intel_iommu_init;
#endif
- acpi_put_table(dmar_tbl);
- dmar_tbl = NULL;
+ if (dmar_tbl) {
+ acpi_put_table(dmar_tbl);
+ dmar_tbl = NULL;
+ }
up_write(&dmar_global_lock);
return ret ? 1 : -ENODEV;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 2/2] driver core: Silence device links sphinx warning
On Sun, Dec 4, 2016 at 1:10 PM, Lukas Wunner wrote:
> Silence this warning emitted by sphinx:
> include/linux/device.h:938: warning: No description found for parameter
> 'links'
>
> While at it, fix typos in comments of device links code.
>
> Cc: Rafael J. Wysocki
> Cc: Greg Kroah-Hartman
> Cc: Jonathan Corbet
> Cc: Silvio Fricke
> Signed-off-by: Lukas Wunner
Acked-by: Rafael J. Wysocki
and thanks for fixing these!
[Adds note to self to spellcheck patches next time.]
> ---
> drivers/base/core.c| 4 ++--
> include/linux/device.h | 1 +
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index d0c9df5..8c25e68 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -172,7 +172,7 @@ static int device_reorder_to_tail(struct device *dev,
> void *not_used)
> *
> * The supplier device is required to be registered when this function is
> called
> * and NULL will be returned if that is not the case. The consumer device
> need
> - * not be registerd, however.
> + * not be registered, however.
> */
> struct device_link *device_link_add(struct device *consumer,
> struct device *supplier, u32 flags)
> @@ -225,7 +225,7 @@ struct device_link *device_link_add(struct device
> *consumer,
> INIT_LIST_HEAD(&link->c_node);
> link->flags = flags;
>
> - /* Deterine the initial link state. */
> + /* Determine the initial link state. */
> if (flags & DL_FLAG_STATELESS) {
> link->status = DL_STATE_NONE;
> } else {
> diff --git a/include/linux/device.h b/include/linux/device.h
> index 3896af4..87edfdf 100644
> --- a/include/linux/device.h
> +++ b/include/linux/device.h
> @@ -813,6 +813,7 @@ struct dev_links_info {
> * on. This shrinks the "Board Support Packages" (BSPs) and
> * minimizes board-specific #ifdefs in drivers.
> * @driver_data: Private pointer for driver specific info.
> + * @links: Links to suppliers and consumers of this device.
> * @power: For device power management.
> * See Documentation/power/admin-guide/devices.rst for details.
> * @pm_domain: Provide callbacks that are executed during system suspend,
> --
> 2.10.2
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] ACPI/IORT: Make dma masks set-up IORT specific
On Mon, Dec 5, 2016 at 1:26 PM, Lorenzo Pieralisi
wrote:
> The introduction of acpi_dma_configure() allows to configure DMA
> and related IOMMU for any device that is DMA capable. To achieve
> that goal it ensures DMA masks are set-up to sane default values
> before proceeding with IOMMU and DMA ops configuration.
>
> On x86/ia64 systems, through acpi_bind_one(), acpi_dma_configure() is
> called for every device that has an ACPI companion, in that every device
> is considered DMA capable on x86/ia64 systems (ie acpi_get_dma_attr() API),
> which has the side effect of initializing dma masks also for
> pseudo-devices (eg CPUs and memory nodes) and potentially for devices
> whose dma masks were not set-up before the acpi_dma_configure() API was
> introduced, which may have noxious side effects.
>
> Therefore, in preparation for IORT firmware specific DMA masks set-up,
> wrap the default DMA masks set-up in acpi_dma_configure() inside an IORT
> specific wrapper that reverts to a NOP on x86/ia64 systems, restoring the
> default expected behaviour on x86/ia64 systems and keeping DMA default
> masks set-up on IORT based (ie ARM) arch configurations.
>
> Signed-off-by: Lorenzo Pieralisi
> Cc: Will Deacon
> Cc: Hanjun Guo
> Cc: Bjorn Helgaas
> Cc: Robin Murphy
> Cc: Tomasz Nowicki
> Cc: Joerg Roedel
> Cc: "Rafael J. Wysocki"
> Cc: Sricharan R
Acked -by: Rafael J. Wysocki
> ---
> Joerg,
>
> pending Rafael's ACK on it, given the 4.10 release timing and that the
> series is queued via the IOMMU tree please consider applying this patch to
> your arm/smmu branch for 4.10, it is not fixing a bug but it is modifying
> the x86/ia64 code path; I prefer preventing any issue related to default
> dma masks on x86/ia64 so I hope it can get merged along with the rest of
> the ACPI IORT SMMU series.
>
> Thanks a lot and apologies,
> Lorenzo
>
> drivers/acpi/arm64/iort.c | 22 ++
> drivers/acpi/scan.c | 14 +-
> include/linux/acpi_iort.h | 2 ++
> 3 files changed, 25 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/acpi/arm64/iort.c b/drivers/acpi/arm64/iort.c
> index 47bace8..e0d2e6e 100644
> --- a/drivers/acpi/arm64/iort.c
> +++ b/drivers/acpi/arm64/iort.c
> @@ -547,6 +547,28 @@ static const struct iommu_ops *iort_iommu_xlate(struct
> device *dev,
> }
>
> /**
> + * iort_set_dma_mask - Set-up dma mask for a device.
> + *
> + * @dev: device to configure
> + */
> +void iort_set_dma_mask(struct device *dev)
> +{
> + /*
> +* Set default coherent_dma_mask to 32 bit. Drivers are expected to
> +* setup the correct supported mask.
> +*/
> + if (!dev->coherent_dma_mask)
> + dev->coherent_dma_mask = DMA_BIT_MASK(32);
> +
> + /*
> +* Set it to coherent_dma_mask by default if the architecture
> +* code has not set it.
> +*/
> + if (!dev->dma_mask)
> + dev->dma_mask = &dev->coherent_dma_mask;
> +}
> +
> +/**
> * iort_iommu_configure - Set-up IOMMU configuration for a device.
> *
> * @dev: device to configure
> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
> index 80698d3..93b00cf 100644
> --- a/drivers/acpi/scan.c
> +++ b/drivers/acpi/scan.c
> @@ -1380,19 +1380,7 @@ void acpi_dma_configure(struct device *dev, enum
> dev_dma_attr attr)
> {
> const struct iommu_ops *iommu;
>
> - /*
> -* Set default coherent_dma_mask to 32 bit. Drivers are expected to
> -* setup the correct supported mask.
> -*/
> - if (!dev->coherent_dma_mask)
> - dev->coherent_dma_mask = DMA_BIT_MASK(32);
> -
> - /*
> -* Set it to coherent_dma_mask by default if the architecture
> -* code has not set it.
> -*/
> - if (!dev->dma_mask)
> - dev->dma_mask = &dev->coherent_dma_mask;
> + iort_set_dma_mask(dev);
>
> iommu = iort_iommu_configure(dev);
>
> diff --git a/include/linux/acpi_iort.h b/include/linux/acpi_iort.h
> index dcb2b60..77e0809 100644
> --- a/include/linux/acpi_iort.h
> +++ b/include/linux/acpi_iort.h
> @@ -35,6 +35,7 @@ bool iort_node_match(u8 type);
> u32 iort_msi_map_rid(struct device *dev, u32 req_id);
> struct irq_domain *iort_get_device_domain(struct device *dev, u32 req_id);
> /* IOMMU interface */
> +void iort_set_dma_mask(struct device *dev);
> const struct iommu_ops *iort_iommu_configure(struct device *dev);
> #else
> static inline void acpi_iort_init(void) { }
> @@ -45,6 +46,7 @@ static inline struct irq_domain
> *iort_get_device_domain(st
Re: [PATCH v9 07/16] drivers: acpi: implement acpi_dma_configure
On Sat, Dec 3, 2016 at 11:39 AM, Lorenzo Pieralisi
wrote:
> On Sat, Dec 03, 2016 at 03:11:09AM +0100, Rafael J. Wysocki wrote:
>> On Fri, Dec 2, 2016 at 4:38 PM, Lorenzo Pieralisi
>> wrote:
>> > Rafael, Mark, Suravee,
>> >
>> > On Mon, Nov 21, 2016 at 10:01:39AM +, Lorenzo Pieralisi wrote:
>> >> On DT based systems, the of_dma_configure() API implements DMA
>> >> configuration for a given device. On ACPI systems an API equivalent to
>> >> of_dma_configure() is missing which implies that it is currently not
>> >> possible to set-up DMA operations for devices through the ACPI generic
>> >> kernel layer.
>> >>
>> >> This patch fills the gap by introducing acpi_dma_configure/deconfigure()
>> >> calls that for now are just wrappers around arch_setup_dma_ops() and
>> >> arch_teardown_dma_ops() and also updates ACPI and PCI core code to use
>> >> the newly introduced acpi_dma_configure/acpi_dma_deconfigure functions.
>> >>
>> >> Since acpi_dma_configure() is used to configure DMA operations, the
>> >> function initializes the dma/coherent_dma masks to sane default values
>> >> if the current masks are uninitialized (also to keep the default values
>> >> consistent with DT systems) to make sure the device has a complete
>> >> default DMA set-up.
>> >
>> > I spotted a niggle that unfortunately was hard to spot (and should not
>> > be a problem per se but better safe than sorry) and I am not comfortable
>> > with it.
>> >
>> > Following commit d0562674838c ("ACPI / scan: Parse _CCA and setup
>> > device coherency") in acpi_bind_one() we check if the acpi_device
>> > associated with a device just added supports DMA, first it was
>> > done with acpi_check_dma() and then commit 1831eff876bd ("device
>> > property: ACPI: Make use of the new DMA Attribute APIs") changed
>> > it to acpi_get_dma_attr().
>> >
>> > The subsequent check (attr != DEV_DMA_NOT_SUPPORTED) is always true
>> > on _any_ acpi device we pass to acpi_bind_one() on x86, which was
>> > fine because we used it to call arch_setup_dma_ops(), which is a nop
>> > on x86. On ARM64 a _CCA method is required to define if a device
>> > supports DMA so (attr != DEV_DMA_NOT_SUPPORTED) may well be false.
>> >
>> > Now, acpi_bind_one() is used to bind an acpi_device to its physical
>> > node also for pseudo-devices like cpus and memory nodes. For those
>> > objects, on x86, attr will always be != DEV_DMA_NOT_SUPPORTED.
>> >
>> > So far so good, because on x86 arch_setup_dma_ops() is empty code.
>> >
>> > With this patch, I use the (attr != DEV_DMA_NOT_SUPPORTED) check
>> > to call acpi_dma_configure() which is basically a nop on x86 except
>> > that it sets up the dma_mask/coherent_dma_mask to a sane default value
>> > (after all we are setting up DMA for the device so it makes sense to
>> > initialize the masks there if they were unset since we are configuring
>> > DMA for the device in question) for the given device.
>> >
>> > Problem is, as per the explanation above, we are also setting the
>> > default dma masks for pseudo-devices (eg CPUs) that were previously
>> > untouched, it should not be a problem per-se but I am not comfortable
>> > with that, honestly it does not make much sense.
>> >
>> > An easy "fix" would be to move the default dma masks initialization out
>> > of acpi_dma_configure() (as it was in previous patch versions of this
>> > series - I moved it to acpi_dma_configure() just a consolidation point
>> > for initializing the masks instead of scattering them in every
>> > acpi_dma_configure caller) I can send this as a fix-up patch to Joerg if
>> > we think that's the right thing to do (or I can send it to Rafael later
>> > when the code is in the merged depending on the timing) just let me
>> > know please.
>>
>> Why can't arch_setup_dma_ops() set those masks too?
>
> Because the dma masks set-up is done by the caller (see
> of_dma_configure()) according to firmware configuration or
> platform data knowledge. I wanted to replicate the of_dma_configure()
> interface on ACPI for obvious reasons (on ARM systems), I stopped
> short of adding ACPI code to mirror of_dma_get_range() equivalent
> (through the _DMA object) but I am really really nervous about changing
> the code path on x86 because in theory all is fine, in practice even
> just settin
Re: [PATCH v9 07/16] drivers: acpi: implement acpi_dma_configure
On Fri, Dec 2, 2016 at 4:38 PM, Lorenzo Pieralisi
wrote:
> Rafael, Mark, Suravee,
>
> On Mon, Nov 21, 2016 at 10:01:39AM +, Lorenzo Pieralisi wrote:
>> On DT based systems, the of_dma_configure() API implements DMA
>> configuration for a given device. On ACPI systems an API equivalent to
>> of_dma_configure() is missing which implies that it is currently not
>> possible to set-up DMA operations for devices through the ACPI generic
>> kernel layer.
>>
>> This patch fills the gap by introducing acpi_dma_configure/deconfigure()
>> calls that for now are just wrappers around arch_setup_dma_ops() and
>> arch_teardown_dma_ops() and also updates ACPI and PCI core code to use
>> the newly introduced acpi_dma_configure/acpi_dma_deconfigure functions.
>>
>> Since acpi_dma_configure() is used to configure DMA operations, the
>> function initializes the dma/coherent_dma masks to sane default values
>> if the current masks are uninitialized (also to keep the default values
>> consistent with DT systems) to make sure the device has a complete
>> default DMA set-up.
>
> I spotted a niggle that unfortunately was hard to spot (and should not
> be a problem per se but better safe than sorry) and I am not comfortable
> with it.
>
> Following commit d0562674838c ("ACPI / scan: Parse _CCA and setup
> device coherency") in acpi_bind_one() we check if the acpi_device
> associated with a device just added supports DMA, first it was
> done with acpi_check_dma() and then commit 1831eff876bd ("device
> property: ACPI: Make use of the new DMA Attribute APIs") changed
> it to acpi_get_dma_attr().
>
> The subsequent check (attr != DEV_DMA_NOT_SUPPORTED) is always true
> on _any_ acpi device we pass to acpi_bind_one() on x86, which was
> fine because we used it to call arch_setup_dma_ops(), which is a nop
> on x86. On ARM64 a _CCA method is required to define if a device
> supports DMA so (attr != DEV_DMA_NOT_SUPPORTED) may well be false.
>
> Now, acpi_bind_one() is used to bind an acpi_device to its physical
> node also for pseudo-devices like cpus and memory nodes. For those
> objects, on x86, attr will always be != DEV_DMA_NOT_SUPPORTED.
>
> So far so good, because on x86 arch_setup_dma_ops() is empty code.
>
> With this patch, I use the (attr != DEV_DMA_NOT_SUPPORTED) check
> to call acpi_dma_configure() which is basically a nop on x86 except
> that it sets up the dma_mask/coherent_dma_mask to a sane default value
> (after all we are setting up DMA for the device so it makes sense to
> initialize the masks there if they were unset since we are configuring
> DMA for the device in question) for the given device.
>
> Problem is, as per the explanation above, we are also setting the
> default dma masks for pseudo-devices (eg CPUs) that were previously
> untouched, it should not be a problem per-se but I am not comfortable
> with that, honestly it does not make much sense.
>
> An easy "fix" would be to move the default dma masks initialization out
> of acpi_dma_configure() (as it was in previous patch versions of this
> series - I moved it to acpi_dma_configure() just a consolidation point
> for initializing the masks instead of scattering them in every
> acpi_dma_configure caller) I can send this as a fix-up patch to Joerg if
> we think that's the right thing to do (or I can send it to Rafael later
> when the code is in the merged depending on the timing) just let me
> know please.
Why can't arch_setup_dma_ops() set those masks too?
Thanks,
Rafael
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 16/16] drivers: acpi: iort: introduce iort_iommu_configure
On Wed, Nov 9, 2016 at 3:19 PM, Lorenzo Pieralisi wrote: > DT based systems have a generic kernel API to configure IOMMUs > for devices (ie of_iommu_configure()). > > On ARM based ACPI systems, the of_iommu_configure() equivalent can > be implemented atop ACPI IORT kernel API, with the corresponding > functions to map device identifiers to IOMMUs and retrieve the > corresponding IOMMU operations necessary for DMA operations set-up. > > By relying on the iommu_fwspec generic kernel infrastructure, > implement the IORT based IOMMU configuration for ARM ACPI systems > and hook it up in the ACPI kernel layer that implements DMA > configuration for a device. > > Signed-off-by: Lorenzo Pieralisi > Tested-by: Hanjun Guo > Tested-by: Tomasz Nowicki > Cc: Hanjun Guo > Cc: Tomasz Nowicki > Cc: "Rafael J. Wysocki" For the ACPI core part: Acked-by: Rafael J. Wysocki ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 07/16] drivers: acpi: implement acpi_dma_configure
On Wed, Nov 9, 2016 at 3:19 PM, Lorenzo Pieralisi wrote: > On DT based systems, the of_dma_configure() API implements DMA > configuration for a given device. On ACPI systems an API equivalent to > of_dma_configure() is missing which implies that it is currently not > possible to set-up DMA operations for devices through the ACPI generic > kernel layer. > > This patch fills the gap by introducing acpi_dma_configure/deconfigure() > calls that for now are just wrappers around arch_setup_dma_ops() and > arch_teardown_dma_ops() and also updates ACPI and PCI core code to use > the newly introduced acpi_dma_configure/acpi_dma_deconfigure functions. > > Since acpi_dma_configure() is used to configure DMA operations, the > function initializes the dma/coherent_dma masks to sane default values > if the current masks are uninitialized (also to keep the default values > consistent with DT systems) to make sure the device has a complete > default DMA set-up. > > The DMA range size passed to arch_setup_dma_ops() is sized according > to the device coherent_dma_mask (starting at address 0x0), mirroring the > DT probing path behaviour when a dma-ranges property is not provided > for the device being probed; this changes the current arch_setup_dma_ops() > call parameters in the ACPI probing case, but since arch_setup_dma_ops() > is a NOP on all architectures but ARM/ARM64 this patch does not change > the current kernel behaviour on them. > > Signed-off-by: Lorenzo Pieralisi > Acked-by: Bjorn Helgaas [pci] > Tested-by: Hanjun Guo > Tested-by: Tomasz Nowicki > Cc: Bjorn Helgaas > Cc: Robin Murphy > Cc: Tomasz Nowicki > Cc: Joerg Roedel > Cc: "Rafael J. Wysocki" LGTM Acked-by: Rafael J. Wysocki ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 00/16] ACPI IORT ARM SMMU support
On Tue, Nov 15, 2016 at 11:12 AM, Lorenzo Pieralisi wrote: > Hi Rafael, > > On Thu, Nov 10, 2016 at 12:36:12AM +0100, Rafael J. Wysocki wrote: >> Hi Lorenzo, >> >> On Wed, Nov 9, 2016 at 3:19 PM, Lorenzo Pieralisi >> wrote: >> > This patch series is v7 of a previous posting: >> > >> > https://lkml.org/lkml/2016/10/18/506 >> >> I don't see anything objectionable in this series. >> >> Please let me know which patches in particular to look at in detail. > > Are patches 7 and consequently 16 (that builds on top of 7) ok with > you ? They should be equivalent to nop on anything other than ARM64 > but they touch ACPI core so they require an ACK if they are ok please. Yes, they are. Please feel free to add my ACKs to those or I will send them separately later today. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 2/2] iommu/exynos: Add proper runtime pm support
On Wed, Nov 9, 2016 at 4:07 PM, Marek Szyprowski wrote: > Hi Luis, > > > On 2016-11-08 23:14, Luis R. Rodriguez wrote: >> >> On Mon, Oct 10, 2016 at 03:32:06PM +0200, Marek Szyprowski wrote: >>> >>> Hi Luis >>> >>> >>> On 2016-10-06 19:37, Luis R. Rodriguez wrote: On Thu, Sep 29, 2016 at 10:12:31AM +0200, Marek Szyprowski wrote: > > This patch uses recently introduced device links to track the runtime > pm > state of the master's device. This way each SYSMMU controller is > runtime > activated when its master's device is active instead of? >>> >>> instead of keeping SYSMMU controller runtime active all the time. >> >> I thought Rafael's work was for suspend/resume, not for runtime suspend. >> Is it for both ? > > > Yes, it solves both problems, although the suspend/resume was easy to > workaround just by using LATE_SLEEP_OPS. Right, but that's just in this particular case, because the dependency chain is of length 2. :-) If you had a longer chain, you might in theory use the _noirq() stage somehow, but that has limitations. >> Because as far as I can tell this was painted to help >> with suspend/resume ? It helps with three things, (async) suspend/resume, runtime PM and shutdown (that last part is the hardest to figure out). The ordering in which all of these are carried out is analogous and cannot be determined correctly by the device registration ordering itself in general (which has been a known fact for years, but some localized workarounds were put in some places to work around that). Moreover, even if the list ordering (of dpm_list, for instance) is correct, it still doesn't guarantee the right ordering of actions that are carried out asynchronously. They are all started in the list order, but they may be running in parallel with each other and complete at different times. For this reason, there needs to be a way to ensure that, say, the suspend operations for consumer devices complete before their suppliers will become unavailable and so on. Both runtime PM and system suspend/resume have this problem. It is not present in the system shutdown case, but it still helps to get a correct list ordering (ie. such that won't cause supplier devices to be shut down before their consumers) in this case too. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm
On Thu, Nov 10, 2016 at 1:12 AM, Luis R. Rodriguez wrote: > On Thu, Nov 10, 2016 at 01:05:42AM +0100, Rafael J. Wysocki wrote: >> On Thu, Nov 10, 2016 at 12:55 AM, Luis R. Rodriguez >> wrote: >> > On Tue, Nov 08, 2016 at 04:30:44PM +0100, Lukas Wunner wrote: >> >> On Tue, Nov 08, 2016 at 08:27:12AM +0100, Marek Szyprowski wrote: >> >> > On 2016-11-07 22:47, Luis R. Rodriguez wrote: >> >> > > Has there been any review of the existing similar solutions out there >> >> > > such as the DRM / audio component framework? Would that help ? >> >> > >> >> > Nope, none of that solution deals with runtime pm. >> >> >> >> Well, they do. Hybrid graphics laptops often have an HDA controller >> >> on the discrete GPU and I assume that's what Luis meant. There's code >> >> in drivers/gpu/vga/vga_switcheroo.c to make this (only sort of) work: >> >> >> >> * When the GPU is powered up/down, the HDA controller's driver is >> >> instructed to pm_runtime_get/put the HDA device (see call to >> >> set_audio_state() in vga_switcheroo_set_dynamic_switch()). >> >> >> >> * When a runtime PM ref is acquired on the HDA device, the >> >> GPU is powered up (see vga_switcheroo_runtime_resume_hdmi_audio()). >> >> >> >> >> >> Unfortunately this is all fairly broken, e.g.: >> >> >> >> * If a runtime PM ref on the HDA device is held for more than 5 sec >> >> (autosuspend delay of the GPU), the GPU will be powered down and >> >> the HDA device will become inaccessible, regardless of the runtime >> >> PM ref still being held (because vga_switcheroo_set_dynamic_switch() >> >> doesn't check the refcount of the HDA device). >> >> >> >> * The DRM device is afforded direct-complete but the HDA device is not. >> >> If the GPU is handled earlier by dpm_suspend(), then runtime PM will >> >> have been disabled on the GPU and thus the HDA device will fail to >> >> runtime resume before system sleep. >> >> >> >> Rafael's series allows representation of such inter-device dependencies >> >> in the PM core and can thus replace kludgy and broken "solutions" like >> >> the one above. >> >> >> >> There's one thing that I haven't understood myself though: In an e-mail >> >> exchange in September Rafael has argued that the above-mentioned hybrid >> >> graphics use case "isn't a good [example] IMO. That clearly is a case >> >> when two (or more) devices share power resources controlled by a single >> >> on/off switch. Which is a clear use case for a PM domain." >> >> >> >> The same seems to apply to Marek's SYSMMU use case. When applying device >> >> links to SYSMMU or hybrid graphics, we select one of the devices in the >> >> PM domain as master and have the other one depend on it as slave, i.e. >> >> a synthetic hierarchical relationship is established. >> >> >> >> I've responded to Rafael on September 18 that this can't be solved with >> >> a struct dev_pm_domain, but haven't received a reply since: >> >> https://lkml.org/lkml/2016/9/18/103 >> > >> > Rafael note: >> > >> > The one he asked here. >> >> OK, so please see the message I've just sent. :-) >> >> The bottom line is that there may be multiple ways to approach a >> problem like this and which of them works best really depends on the >> particular case. >> >> You seem to be thinking that the device links infrastructure may not >> be necessary after all if there are other ways to address the problems >> it is intended for, but those other ways may still be less viable >> practically due to the complexity involved and similar. Also they may >> lead to code duplication in different places that try to address those >> problems in a similar fashion, but slightly differently. > > I was not arguing that, I have been suggesting that pre-existing solutions > should at least be reviewed and considered, if they are sub-par and > the device links infrastructure is much simpler and provides the same > solution folks should be alert and consider embracing it. If not and > its the other way around and we could generalize the others, it should > mean we could learn from them. > > From what I gather from Plumbers its not clear to me any
Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm
On Thu, Nov 10, 2016 at 12:55 AM, Luis R. Rodriguez wrote: > On Tue, Nov 08, 2016 at 04:30:44PM +0100, Lukas Wunner wrote: >> On Tue, Nov 08, 2016 at 08:27:12AM +0100, Marek Szyprowski wrote: >> > On 2016-11-07 22:47, Luis R. Rodriguez wrote: >> > > Has there been any review of the existing similar solutions out there >> > > such as the DRM / audio component framework? Would that help ? >> > >> > Nope, none of that solution deals with runtime pm. >> >> Well, they do. Hybrid graphics laptops often have an HDA controller >> on the discrete GPU and I assume that's what Luis meant. There's code >> in drivers/gpu/vga/vga_switcheroo.c to make this (only sort of) work: >> >> * When the GPU is powered up/down, the HDA controller's driver is >> instructed to pm_runtime_get/put the HDA device (see call to >> set_audio_state() in vga_switcheroo_set_dynamic_switch()). >> >> * When a runtime PM ref is acquired on the HDA device, the >> GPU is powered up (see vga_switcheroo_runtime_resume_hdmi_audio()). >> >> >> Unfortunately this is all fairly broken, e.g.: >> >> * If a runtime PM ref on the HDA device is held for more than 5 sec >> (autosuspend delay of the GPU), the GPU will be powered down and >> the HDA device will become inaccessible, regardless of the runtime >> PM ref still being held (because vga_switcheroo_set_dynamic_switch() >> doesn't check the refcount of the HDA device). >> >> * The DRM device is afforded direct-complete but the HDA device is not. >> If the GPU is handled earlier by dpm_suspend(), then runtime PM will >> have been disabled on the GPU and thus the HDA device will fail to >> runtime resume before system sleep. >> >> Rafael's series allows representation of such inter-device dependencies >> in the PM core and can thus replace kludgy and broken "solutions" like >> the one above. >> >> There's one thing that I haven't understood myself though: In an e-mail >> exchange in September Rafael has argued that the above-mentioned hybrid >> graphics use case "isn't a good [example] IMO. That clearly is a case >> when two (or more) devices share power resources controlled by a single >> on/off switch. Which is a clear use case for a PM domain." >> >> The same seems to apply to Marek's SYSMMU use case. When applying device >> links to SYSMMU or hybrid graphics, we select one of the devices in the >> PM domain as master and have the other one depend on it as slave, i.e. >> a synthetic hierarchical relationship is established. >> >> I've responded to Rafael on September 18 that this can't be solved with >> a struct dev_pm_domain, but haven't received a reply since: >> https://lkml.org/lkml/2016/9/18/103 > > Rafael note: > > The one he asked here. OK, so please see the message I've just sent. :-) The bottom line is that there may be multiple ways to approach a problem like this and which of them works best really depends on the particular case. You seem to be thinking that the device links infrastructure may not be necessary after all if there are other ways to address the problems it is intended for, but those other ways may still be less viable practically due to the complexity involved and similar. Also they may lead to code duplication in different places that try to address those problems in a similar fashion, but slightly differently. All this is about providing people with reasonably straightforward and common ways to deal with certain class of problems showing up in multiple places. It is not about getting the driver probe ordering right magically in one go. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 7/7] iommu/exynos: Use device dependency links to control runtime pm
On Tue, Nov 8, 2016 at 4:30 PM, Lukas Wunner wrote: > On Tue, Nov 08, 2016 at 08:27:12AM +0100, Marek Szyprowski wrote: >> On 2016-11-07 22:47, Luis R. Rodriguez wrote: >> > Has there been any review of the existing similar solutions out there >> > such as the DRM / audio component framework? Would that help ? >> >> Nope, none of that solution deals with runtime pm. > > Well, they do. Hybrid graphics laptops often have an HDA controller > on the discrete GPU and I assume that's what Luis meant. There's code > in drivers/gpu/vga/vga_switcheroo.c to make this (only sort of) work: > > * When the GPU is powered up/down, the HDA controller's driver is > instructed to pm_runtime_get/put the HDA device (see call to > set_audio_state() in vga_switcheroo_set_dynamic_switch()). > > * When a runtime PM ref is acquired on the HDA device, the > GPU is powered up (see vga_switcheroo_runtime_resume_hdmi_audio()). > > > Unfortunately this is all fairly broken, e.g.: > > * If a runtime PM ref on the HDA device is held for more than 5 sec > (autosuspend delay of the GPU), the GPU will be powered down and > the HDA device will become inaccessible, regardless of the runtime > PM ref still being held (because vga_switcheroo_set_dynamic_switch() > doesn't check the refcount of the HDA device). > > * The DRM device is afforded direct-complete but the HDA device is not. > If the GPU is handled earlier by dpm_suspend(), then runtime PM will > have been disabled on the GPU and thus the HDA device will fail to > runtime resume before system sleep. > > Rafael's series allows representation of such inter-device dependencies > in the PM core and can thus replace kludgy and broken "solutions" like > the one above. > > There's one thing that I haven't understood myself though: In an e-mail > exchange in September Rafael has argued that the above-mentioned hybrid > graphics use case "isn't a good [example] IMO. That clearly is a case > when two (or more) devices share power resources controlled by a single > on/off switch. Which is a clear use case for a PM domain." > > The same seems to apply to Marek's SYSMMU use case. When applying device > links to SYSMMU or hybrid graphics, we select one of the devices in the > PM domain as master and have the other one depend on it as slave, i.e. > a synthetic hierarchical relationship is established. > > I've responded to Rafael on September 18 that this can't be solved with > a struct dev_pm_domain, but haven't received a reply since: > https://lkml.org/lkml/2016/9/18/103 Well, that clearly fell off my radar, sorry about that. The idea, roughly, is that if there is a single on/off switch acting on multiple devices, you can (a) set up a PM domain tracking all of those device's runtime PM invocations and (b) maintaining a reference counter of devices still not suspended. This way it would only turn the switch off when all of the devices in question had been suspended. Analogously, it would turn the switch on before resuming the first device in the domain. Of course, that code isn't available as a library, you would need to implement it (or use genpd, but chances are it is too heavy weight for the job). In theory, it shouldn't really matter where the switch is and how it is operated as long as the PM domain "driver" knows how to access it. However, for that to work something would need to bind that "driver" to the domain and something would need to know which devices needed to be added to the PM domains during enumeration and the ordering of things bringup matters a lot here, which is a problem. So in short, you are right that for the GPU/HDA thing device links would likely to be a simpler to use and still get the job done. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 7/7] iommu/exynos: Use device dependency links to control runtime pm
On Thu, Nov 10, 2016 at 12:31 AM, Luis R. Rodriguez wrote: > On Tue, Nov 08, 2016 at 02:29:24PM +0100, Marek Szyprowski wrote: >> This patch uses recently introduced device dependency links to track the >> runtime pm state of the master's device. The goal is to let SYSMMU >> controller device's runtime PM to follow the runtime PM state of the >> respective master's device. This way each SYSMMU controller is active >> only when its master's device is active and can properly restore or save >> its state instead on runtime PM transition of master's device. >> This approach replaces old behavior, when SYSMMU controller was set to >> runtime active once after attaching to the master device. In the new >> approach SYSMMU controllers no longer prevents respective power domains >> to be turned off when master's device is not being used. >> >> The dependency links also enforces proper order of suspending/restoring >> devices during system sleep transition, so there is no more need to use >> LATE_SYSTEM_SLEEP_PM_OPS-based workaround for ensuring that SYSMMUs are >> suspended after their master devices. > > Patches 1-6 seems reasonable to me, however in so far as this patch is > concerned I'd appreaciate if you and Rafael can reply to Lukas Wunner's > questions. Which questions exactly do you mean? Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 01/16] drivers: acpi: add FWNODE_ACPI_STATIC fwnode type
On Wed, Nov 9, 2016 at 3:19 PM, Lorenzo Pieralisi wrote: > On systems booting with a device tree, every struct device is associated > with a struct device_node, that provides its DT firmware representation. > The device node can be used in generic kernel contexts (eg IRQ > translation, IOMMU streamid mapping), to retrieve the properties > associated with the device and carry out kernel operations accordingly. > Owing to the 1:1 relationship between the device and its device_node, > the device_node can also be used as a look-up token for the device (eg > looking up a device through its device_node), to retrieve the device in > kernel paths where the device_node is available. > > On systems booting with ACPI, the same abstraction provided by > the device_node is required to provide look-up functionality. > > The struct acpi_device, that represents firmware objects in the > ACPI namespace already includes a struct fwnode_handle of > type FWNODE_ACPI as their member; the same abstraction is missing > though for devices that are instantiated out of static ACPI tables > entries (eg ARM SMMU devices). > > Add a new fwnode_handle type to associate devices created out > of static ACPI table entries to the respective firmware components > and create a simple ACPI core layer interface to dynamically allocate > and free the corresponding firmware nodes so that kernel subsystems > can use it to instantiate the nodes and associate them with the > respective devices. > > Signed-off-by: Lorenzo Pieralisi > Reviewed-by: Hanjun Guo > Tested-by: Hanjun Guo > Tested-by: Tomasz Nowicki > Cc: "Rafael J. Wysocki" Acked-by: Rafael J. Wysocki Thanks! ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v7 00/16] ACPI IORT ARM SMMU support
Hi Lorenzo, On Wed, Nov 9, 2016 at 3:19 PM, Lorenzo Pieralisi wrote: > This patch series is v7 of a previous posting: > > https://lkml.org/lkml/2016/10/18/506 I don't see anything objectionable in this series. Please let me know which patches in particular to look at in detail. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 00/16] ACPI IORT ARM SMMU support
On Wed, Oct 26, 2016 at 1:04 PM, Lorenzo Pieralisi
wrote:
> Rafael, Joerg (and anyone else CC'ed),
>
> On Tue, Oct 18, 2016 at 05:03:58PM +0100, Lorenzo Pieralisi wrote:
>> This patch series is v6 of a previous posting:
>>
>> https://lkml.org/lkml/2016/9/9/418
>>
>> v5 -> v6
>> - Rebased against v4.9-rc1
>> - Changed FWNODE_IOMMU to FWNODE_ACPI_STATIC
>> - Moved platform devices creation into IORT code
>> - Updated fwnode handling
>> - Added default dma masks initialization
>
> Any comments on v6 ? Patches touching generic ACPI code
> are {1, 2, 7}, patch 4 updates the IOMMU of_iommu_{set/get}_ops()
> API to make it work on ACPI systems too, by replacing the
> device_node with a fwnode_handle pointer as look-up token;
> the remainder of patches are ARM specific and creates the
> infrastructure to probe ARM SMMU devices through ACPI,
> ARM IORT table in particular. Given the generic bits changes
> above I would not leave it to late -rc to reach an agreement
> please, thank you.
I'll do my best to look at these in the next few days, but please also
note what I wrote before:
http://marc.info/?l=linux-acpi&m=147744344531599&w=2
Thanks,
Rafael
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 01/14] drivers: iommu: add FWNODE_IOMMU fwnode type
Hi,
On Thu, Oct 13, 2016 at 6:32 PM, Lorenzo Pieralisi
wrote:
> Hi Rafael,
>
> On Fri, Sep 30, 2016 at 05:48:01PM +0200, Rafael J. Wysocki wrote:
>> On Fri, Sep 30, 2016 at 11:07 AM, Lorenzo Pieralisi
>> wrote:
>> > On Thu, Sep 29, 2016 at 10:59:40PM +0200, Rafael J. Wysocki wrote:
>> >> On Thursday, September 29, 2016 03:15:20 PM Lorenzo Pieralisi wrote:
>> >> > Hi Rafael,
>> >> >
>> >> > On Fri, Sep 09, 2016 at 03:23:30PM +0100, Lorenzo Pieralisi wrote:
>> >> > > On systems booting with a device tree, every struct device is
>> >> > > associated with a struct device_node, that represents its DT
>> >> > > representation. The device node can be used in generic kernel
>> >> > > contexts (eg IRQ translation, IOMMU streamid mapping), to
>> >> > > retrieve the properties associated with the device and carry
>> >> > > out kernel operation accordingly. Owing to the 1:1 relationship
>> >> > > between the device and its device_node, the device_node can also
>> >> > > be used as a look-up token for the device (eg looking up a device
>> >> > > through its device_node), to retrieve the device in kernel paths
>> >> > > where the device_node is available.
>> >> > >
>> >> > > On systems booting with ACPI, the same abstraction provided by
>> >> > > the device_node is required to provide look-up functionality.
>> >> > >
>> >> > > Therefore, mirroring the approach implemented in the IRQ domain
>> >> > > kernel layer, this patch adds an additional fwnode type FWNODE_IOMMU.
>> >> > >
>> >> > > This patch also implements a glue kernel layer that allows to
>> >> > > allocate/free FWNODE_IOMMU fwnode_handle structures and associate
>> >> > > them with IOMMU devices.
>> >> > >
>> >> > > Signed-off-by: Lorenzo Pieralisi
>> >> > > Reviewed-by: Hanjun Guo
>> >> > > Cc: Joerg Roedel
>> >> > > Cc: "Rafael J. Wysocki"
>> >> > > ---
>> >> > > include/linux/fwnode.h | 1 +
>> >> > > include/linux/iommu.h | 25 +
>> >> > > 2 files changed, 26 insertions(+)
>> >> > >
>> >> > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
>> >> > > index 8516717..6e10050 100644
>> >> > > --- a/include/linux/fwnode.h
>> >> > > +++ b/include/linux/fwnode.h
>> >> > > @@ -19,6 +19,7 @@ enum fwnode_type {
>> >> > > FWNODE_ACPI_DATA,
>> >> > > FWNODE_PDATA,
>> >> > > FWNODE_IRQCHIP,
>> >> > > + FWNODE_IOMMU,
>> >> >
>> >> > This patch provides groundwork for this series and it is key for
>> >> > the rest of it, basically the point here is that we need a fwnode
>> >> > to differentiate platform devices created out of static ACPI tables
>> >> > entries (ie IORT), that represent IOMMU components.
>> >> >
>> >> > The corresponding device is not an ACPI device (I could fabricate one as
>> >> > it is done for other static tables entries eg FADT power button, but I
>> >> > do not necessarily see the reason for doing that given that all we need
>> >> > the fwnode for is a token identifier), so FWNODE_ACPI does not apply
>> >> > here.
>> >> >
>> >> > Please let me know if it is reasonable how I sorted this out (it
>> >> > is basically identical to IRQCHIP, just another enum entry), the
>> >> > remainder of the code depends on this.
>> >>
>> >> I'm not familiar with the use case, so I don't see anything unreasonable
>> >> in it.
>> >
>> > The use case is pretty simple: on ARM SMMU devices are platform devices.
>> > When booting with DT they are identified through an of_node and related
>> > FWNODE_OF type. When booting with ACPI, the ARM SMMU platform devices,
>> > to be equivalent to DT booting path, should be created out of static
>> > IORT table entries (that's how we describe SMMUs); we need to create
>> > a fwnode "token" to associate with those platform devices and that's
>> > not a FWNODE_ACPI (that is for an ACPI device firmware object, here we
>> > reall
Re: [PATCH v5 01/14] drivers: iommu: add FWNODE_IOMMU fwnode type
On Fri, Sep 30, 2016 at 11:07 AM, Lorenzo Pieralisi
wrote:
> On Thu, Sep 29, 2016 at 10:59:40PM +0200, Rafael J. Wysocki wrote:
>> On Thursday, September 29, 2016 03:15:20 PM Lorenzo Pieralisi wrote:
>> > Hi Rafael,
>> >
>> > On Fri, Sep 09, 2016 at 03:23:30PM +0100, Lorenzo Pieralisi wrote:
>> > > On systems booting with a device tree, every struct device is
>> > > associated with a struct device_node, that represents its DT
>> > > representation. The device node can be used in generic kernel
>> > > contexts (eg IRQ translation, IOMMU streamid mapping), to
>> > > retrieve the properties associated with the device and carry
>> > > out kernel operation accordingly. Owing to the 1:1 relationship
>> > > between the device and its device_node, the device_node can also
>> > > be used as a look-up token for the device (eg looking up a device
>> > > through its device_node), to retrieve the device in kernel paths
>> > > where the device_node is available.
>> > >
>> > > On systems booting with ACPI, the same abstraction provided by
>> > > the device_node is required to provide look-up functionality.
>> > >
>> > > Therefore, mirroring the approach implemented in the IRQ domain
>> > > kernel layer, this patch adds an additional fwnode type FWNODE_IOMMU.
>> > >
>> > > This patch also implements a glue kernel layer that allows to
>> > > allocate/free FWNODE_IOMMU fwnode_handle structures and associate
>> > > them with IOMMU devices.
>> > >
>> > > Signed-off-by: Lorenzo Pieralisi
>> > > Reviewed-by: Hanjun Guo
>> > > Cc: Joerg Roedel
>> > > Cc: "Rafael J. Wysocki"
>> > > ---
>> > > include/linux/fwnode.h | 1 +
>> > > include/linux/iommu.h | 25 +
>> > > 2 files changed, 26 insertions(+)
>> > >
>> > > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
>> > > index 8516717..6e10050 100644
>> > > --- a/include/linux/fwnode.h
>> > > +++ b/include/linux/fwnode.h
>> > > @@ -19,6 +19,7 @@ enum fwnode_type {
>> > > FWNODE_ACPI_DATA,
>> > > FWNODE_PDATA,
>> > > FWNODE_IRQCHIP,
>> > > + FWNODE_IOMMU,
>> >
>> > This patch provides groundwork for this series and it is key for
>> > the rest of it, basically the point here is that we need a fwnode
>> > to differentiate platform devices created out of static ACPI tables
>> > entries (ie IORT), that represent IOMMU components.
>> >
>> > The corresponding device is not an ACPI device (I could fabricate one as
>> > it is done for other static tables entries eg FADT power button, but I
>> > do not necessarily see the reason for doing that given that all we need
>> > the fwnode for is a token identifier), so FWNODE_ACPI does not apply
>> > here.
>> >
>> > Please let me know if it is reasonable how I sorted this out (it
>> > is basically identical to IRQCHIP, just another enum entry), the
>> > remainder of the code depends on this.
>>
>> I'm not familiar with the use case, so I don't see anything unreasonable
>> in it.
>
> The use case is pretty simple: on ARM SMMU devices are platform devices.
> When booting with DT they are identified through an of_node and related
> FWNODE_OF type. When booting with ACPI, the ARM SMMU platform devices,
> to be equivalent to DT booting path, should be created out of static
> IORT table entries (that's how we describe SMMUs); we need to create
> a fwnode "token" to associate with those platform devices and that's
> not a FWNODE_ACPI (that is for an ACPI device firmware object, here we
> really do not need one), so this patch.
>
>> If you're asking about whether or not I mind adding more fwnode types in
>> principle, then no, I don't. :-)
>
> Yes, that's what I was asking, the only point that bugs me is that for
> both FWNODE_IRQCHIP and FWNODE_IOMMU the fwnode is just a "token" (ie a
> valid pointer) used for look-up and the type in the fwnode_handle is
> mostly there for error checking, I was wondering if we could create a
> specific fwnode_type for this specific usage (eg FWNODE_TAG and then add
> a type to it as part of its container struct) instead of adding an enum
> value per subsystem - it seems there are other fwnode types in the
> pipeline :), so I am asking:
>
> lkml.kernel.org/r/3d1468514043-21081-3-git-send-email-miny...@acm.org
OK, I see your concern now, so thanks for presenting it so clearly.
While I don't see anything wrong with having per-subsystem fwnode
types in principle, I agree that if the only purpose of them is to
mean "this comes from ACPI, but from a static table, not from the
namespace", it would be better to have a single fwnode type for that,
like FWNODE_ACPI_STATIC or similar.
Thanks,
Rafael
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v5 01/14] drivers: iommu: add FWNODE_IOMMU fwnode type
On Thursday, September 29, 2016 03:15:20 PM Lorenzo Pieralisi wrote:
> Hi Rafael,
>
> On Fri, Sep 09, 2016 at 03:23:30PM +0100, Lorenzo Pieralisi wrote:
> > On systems booting with a device tree, every struct device is
> > associated with a struct device_node, that represents its DT
> > representation. The device node can be used in generic kernel
> > contexts (eg IRQ translation, IOMMU streamid mapping), to
> > retrieve the properties associated with the device and carry
> > out kernel operation accordingly. Owing to the 1:1 relationship
> > between the device and its device_node, the device_node can also
> > be used as a look-up token for the device (eg looking up a device
> > through its device_node), to retrieve the device in kernel paths
> > where the device_node is available.
> >
> > On systems booting with ACPI, the same abstraction provided by
> > the device_node is required to provide look-up functionality.
> >
> > Therefore, mirroring the approach implemented in the IRQ domain
> > kernel layer, this patch adds an additional fwnode type FWNODE_IOMMU.
> >
> > This patch also implements a glue kernel layer that allows to
> > allocate/free FWNODE_IOMMU fwnode_handle structures and associate
> > them with IOMMU devices.
> >
> > Signed-off-by: Lorenzo Pieralisi
> > Reviewed-by: Hanjun Guo
> > Cc: Joerg Roedel
> > Cc: "Rafael J. Wysocki"
> > ---
> > include/linux/fwnode.h | 1 +
> > include/linux/iommu.h | 25 +
> > 2 files changed, 26 insertions(+)
> >
> > diff --git a/include/linux/fwnode.h b/include/linux/fwnode.h
> > index 8516717..6e10050 100644
> > --- a/include/linux/fwnode.h
> > +++ b/include/linux/fwnode.h
> > @@ -19,6 +19,7 @@ enum fwnode_type {
> > FWNODE_ACPI_DATA,
> > FWNODE_PDATA,
> > FWNODE_IRQCHIP,
> > + FWNODE_IOMMU,
>
> This patch provides groundwork for this series and it is key for
> the rest of it, basically the point here is that we need a fwnode
> to differentiate platform devices created out of static ACPI tables
> entries (ie IORT), that represent IOMMU components.
>
> The corresponding device is not an ACPI device (I could fabricate one as
> it is done for other static tables entries eg FADT power button, but I
> do not necessarily see the reason for doing that given that all we need
> the fwnode for is a token identifier), so FWNODE_ACPI does not apply
> here.
>
> Please let me know if it is reasonable how I sorted this out (it
> is basically identical to IRQCHIP, just another enum entry), the
> remainder of the code depends on this.
I'm not familiar with the use case, so I don't see anything unreasonable
in it.
If you're asking about whether or not I mind adding more fwnode types in
principle, then no, I don't. :-)
Thanks,
Rafael
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)
On Monday, September 26, 2016 10:15:24 AM Marek Szyprowski wrote:
> Hi Rafael,
>
>
> On 2016-09-24 03:25, Rafael J. Wysocki wrote:
> > On Friday, September 23, 2016 03:50:02 PM Lukas Wunner wrote:
> >> On Fri, Sep 23, 2016 at 02:49:20PM +0200, Rafael J. Wysocki wrote:
> >>> On Tuesday, September 20, 2016 10:51:13 AM Marek Szyprowski wrote:
> >>>> On 2016-09-19 23:45, Tobias Jakobi wrote:
> >>>>> I did some tests with the new version today. Sadly the reboot/shutdown
> >>>>> issues are still present.
> >>>> Thanks for the report. I've managed to reproduce this issue and it is
> >>>> again
> >>>> caused by modifying device on devices_kset list before it will be finally
> >>>> added by device_add(). I thought that the new patchset allows creating
> >>>> links to a device, which has not been yet added to system device list.
> >> Hm, Marek, why isn't it possible to set up the links from the consumer's
> >> ->probe hook in this case?
>
> Because consumers are unaware of the IOMMU presence, so they are also
> unaware
> of the links that have to be created.
>
> >>>> Should it be allowed to create a link to device, which
> >>>> has not yet been added to system device list by device_add()?
> >>> While it would be easy to require both the consumer and producer devices
> >>> to
> >>> be registered for creating a link between them, that would just make it
> >>> harder to use links in the first place.
> >>>
> >>> So ideally, it should be possible to create links between devices before
> >>> registering them, but since I didn't take that into account in the current
> >>> patch series, some quite substantial changes are needed to cover that.
> >>>
> >>> Additional link states come to mind, but then the "stateless" links are
> >>> affected by this problem too.
> >> device_link_add() could be changed to call device_reorder_to_tail()
> >> only if device_is_registered(consumer) returns true.
> >>
> >> That's an inline function defined in which returns
> >> dev->kobj.state_in_sysfs, a flag which is set in kobject_add().
> > I know what that function is, but using it alone is not sufficient,
> > because dev->kobj.state_in_sysfs is set before the device is added to
> > dpm_list.
>
> I found that checking for dev->p was enough to check if device has been
> added to system or not, but this seems to be some kind of ugly workaround:
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index 4542ba9f60d4..780495918b53 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -180,9 +180,11 @@ struct device_link *device_link_add(struct device
> *consumer,
> * It is necessary to hold dpm_list locked throughout all that
> or else
> * we may end up suspending with a wrong ordering of it.
> */
> - device_pm_lock();
> - device_reorder_to_tail(consumer, NULL);
> - device_pm_unlock();
> + if (consumer->p) {
> + device_pm_lock();
> + device_reorder_to_tail(consumer, NULL);
> + device_pm_unlock();
> + }
This still is somewhat racy, because the device may not be in dpm_list yet
even if consumer->p is set.
There needs to be something checked and set under device_pm_lock() to avoid
that race.
Let me add it to the patchset and we'll see.
>
> list_add_tail_rcu(&link->s_node, &supplier->links_to_consumers);
> list_add_tail_rcu(&link->c_node, &consumer->links_to_suppliers);
>
>
> >
> >> Then device_add() would have to check if any links are already
> >> set up and reorder the consumer behind the suppliers.
> >>
> >> Doesn't seem to be *that* complex, but probably I'm missing something,
> >> this is just off the cuff...
> > There are some cases to consider and some races to avoid AFAICS.
> >
> > It all gets a lot simpler if device_link_add() is allowed to return NULL
> > when
> > the supplier device passed to it has not been registered yet. That looks
> > like
> > a reasonable thing to do to me, but I wonder if someone has a use case in
> > which
> > it would be a substantial limitation.
>
> Hmmm, you are talking here about the supplier, but my case is that
> supplier is
> already registered and probed, but the consumer is about to be created.
Right.
> If you
> think that supporting such case makes no sense,
I think that it does make sense.
I only was wondering if that was going to be sufficient. :-)
Thanks,
Rafael
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)
On Friday, September 23, 2016 03:50:02 PM Lukas Wunner wrote: > On Fri, Sep 23, 2016 at 02:49:20PM +0200, Rafael J. Wysocki wrote: > > On Tuesday, September 20, 2016 10:51:13 AM Marek Szyprowski wrote: > > > On 2016-09-19 23:45, Tobias Jakobi wrote: > > > > I did some tests with the new version today. Sadly the reboot/shutdown > > > > issues are still present. > > > > > > Thanks for the report. I've managed to reproduce this issue and it is > > > again > > > caused by modifying device on devices_kset list before it will be finally > > > added by device_add(). I thought that the new patchset allows creating > > > links to a device, which has not been yet added to system device list. > > Hm, Marek, why isn't it possible to set up the links from the consumer's > ->probe hook in this case? > > > > > Should it be allowed to create a link to device, which > > > has not yet been added to system device list by device_add()? > > > > While it would be easy to require both the consumer and producer devices to > > be registered for creating a link between them, that would just make it > > harder to use links in the first place. > > > > So ideally, it should be possible to create links between devices before > > registering them, but since I didn't take that into account in the current > > patch series, some quite substantial changes are needed to cover that. > > > > Additional link states come to mind, but then the "stateless" links are > > affected by this problem too. > > device_link_add() could be changed to call device_reorder_to_tail() > only if device_is_registered(consumer) returns true. > > That's an inline function defined in which returns > dev->kobj.state_in_sysfs, a flag which is set in kobject_add(). I know what that function is, but using it alone is not sufficient, because dev->kobj.state_in_sysfs is set before the device is added to dpm_list. > Then device_add() would have to check if any links are already > set up and reorder the consumer behind the suppliers. > > Doesn't seem to be *that* complex, but probably I'm missing something, > this is just off the cuff... There are some cases to consider and some races to avoid AFAICS. It all gets a lot simpler if device_link_add() is allowed to return NULL when the supplier device passed to it has not been registered yet. That looks like a reasonable thing to do to me, but I wonder if someone has a use case in which it would be a substantial limitation. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 0/2] Exynos IOMMU: proper runtime PM support (use device dependencies)
On Tuesday, September 20, 2016 10:51:13 AM Marek Szyprowski wrote:
> Hi All,
>
> On 2016-09-19 23:45, Tobias Jakobi wrote:
> > I did some tests with the new version today. Sadly the reboot/shutdown
> > issues are still present.
>
> Thanks for the report. I've managed to reproduce this issue and it is again
> caused by modifying device on devices_kset list before it will be finally
> added by device_add(). I thought that the new patchset allows creating links
> to a device, which has not been yet added to system device list.
>
> Rafael:
> What is your opinion?
Well, this is a problem. :-)
> Should it be allowed to create a link to device, which
> has not yet been added to system device list by device_add()?
While it would be easy to require both the consumer and producer devices to
be registered for creating a link between them, that would just make it harder
to use links in the first place.
So ideally, it should be possible to create links between devices before
registering them, but since I didn't take that into account in the current
patch series, some quite substantial changes are needed to cover that.
Additional link states come to mind, but then the "stateless" links are
affected by this problem too.
> My code used to do that, because IOMMUs are configured from
> of_platform_device_create_pdata()
> of_dma_configure() of_iommu_configure(), which happens before device_add().
>
> To solve the reported corruption of devices_kset list, following change is
> needed:
>
> diff --git a/drivers/base/core.c b/drivers/base/core.c
> index aa8196508db9..4542ba9f60d4 100644
> --- a/drivers/base/core.c
> +++ b/drivers/base/core.c
> @@ -1039,11 +1039,18 @@ static void devices_kset_move_after(struct
> device *deva, struct device *devb)
>*/
> void devices_kset_move_last(struct device *dev)
> {
> + struct device *d;
> +
> if (!devices_kset)
> return;
> pr_debug("devices_kset: Moving %s to end of list\n",
> dev_name(dev));
> spin_lock(&devices_kset->list_lock);
> - list_move_tail(&dev->kobj.entry, &devices_kset->list);
> + list_for_each_entry(d, &devices_kset->list, kobj.entry) {
> + if (d == dev) {
> + list_move_tail(&dev->kobj.entry,
> &devices_kset->list);
> + break;
> + }
> + }
> spin_unlock(&devices_kset->list_lock);
> }
>
>
> If you think that links can be created only to a device, which has been
> fully
> added to the system, I will register a bus notifier and create a link on
> consumers
> device probe then.
That would be a workaround for a coverage gap, so not particularly attractive.
Thanks,
Rafael
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
On Thursday, July 28, 2016 05:28:31 PM Lukas Wunner wrote: > On Thu, Jul 28, 2016 at 02:30:31AM +0200, Rafael J. Wysocki wrote: > > On Monday, July 25, 2016 12:48:32 AM Lukas Wunner wrote: > > > On Thu, Jul 21, 2016 at 02:25:15AM +0200, Rafael J. Wysocki wrote: > > > > On Thursday, July 21, 2016 01:25:53 AM Lukas Wunner wrote: > > > > > I guess I could amend portdrv to return -EPROBE_DEFER on Macs if > > > > > no driver is bound to the NHI. Doesn't feel pretty to me though. > > > > > > > > > > Ultimately this seems to be the same issue as with calling > > > > > dev_pm_domain_set() for a bound device. Perhaps device_link_add() > > > > > can likewise be allowed if a runtime PM ref is held for the devices > > > > > and the call happens under lock_system_sleep()? > > > > > > > > No, the whole synchronization scheme in the links code would have had > > > > to be > > > > changed for that to really work. > > > > > > > > And it really is about what is needed (at least in principle) to run > > > > your > > > > device. If you think you need device X with a driver to handle device Y > > > > correctly, then either you need it all the time, from probe to remove, > > > > or > > > > you just don't really need it at all. > > > > > > Real life isn't as simple as that. > > > > > > In this case, we have consumers (hotplug ports) which are doing fine > > > if the driver for the supplier (NHI) is not loaded. But once it loads, > > > the links must be in place. > > > > Hmm. > > > > What if it is not loaded and the system suspends. Will everything work > > as expected after the subsequent resume? > > The short answer is yes. OK I think it's possible to add a link flag to address this case. Namely, if that flag is passed to device_link_add(), the link will be added in the DEVICE_LINK_ACTIVE state right away, but that will need to be synchronized against all possible transitions of the consumer device (at least). It's better to do that in a separate patch for this reason IMO. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 08/10] PM core: Fix restoring devices with links during system PM transition
On Friday, June 17, 2016 08:26:58 AM Marek Szyprowski wrote:
> When devices are being runtime resumed during the system PM transition to
> suspend state, the link suppliers might be already resumed and
> have runtime pm disabled. This is normal case. This patch adds special
> support for such case. Simple call to pm_runtime_get_sync returns error
> when device has runtime PM disabled, what results in incorrect runtime PM
> state during system wide PM transition.
The problem is real, but the proposed solution is questionable.
>
> Signed-off-by: Marek Szyprowski
> ---
> drivers/base/power/runtime.c | 22 +-
> 1 file changed, 21 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index 0ea00d442e0f..d00b079fad88 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -261,6 +261,26 @@ static int rpm_check_suspend_allowed(struct device *dev)
> }
>
> /**
> + * __pm_runtime_force_resume - force resume of the device
> + * @dev: Device to resume
> + *
> + * This function works like __pm_runtime_resume(dev, RPM_GET_PUT), but
> + * also handles devices with runtime PM disabled. This allows to properly
> + * control all devices during preparation for system PM transition.
> + */
> +static int __pm_runtime_force_resume(struct device *dev)
> +{
> + if (!dev->power.disable_depth)
> + return pm_runtime_get_sync(dev);
> +
> + if (dev->power.runtime_status != RPM_ACTIVE ||
> + atomic_inc_not_zero(&dev->power.usage_count) == 0)
> + return -EINVAL;
> +
> + return 0;
> +}
> +
> +/**
> * __rpm_callback - Run a given runtime PM callback for a given device.
> * @cb: Runtime PM callback to run.
> * @dev: Device to run the callback for.
> @@ -291,7 +311,7 @@ static int __rpm_callback(int (*cb)(struct device *),
> struct device *dev)
> if ((link->flags & DEVICE_LINK_PM_RUNTIME)
> && link->status !=
> DEVICE_LINK_SUPPLIER_UNBIND
> && !link->rpm_active) {
> - retval =
> pm_runtime_get_sync(link->supplier);
> + retval =
> __pm_runtime_force_resume(link->supplier);
> if (retval < 0) {
>
> pm_runtime_put_noidle(link->supplier);
> goto fail;
The reason why power.disable_depth is different from 0 may not be system
suspend,
in which case it should fail even if the status happens to be RPM_ACTIVE.
Thanks,
Rafael
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 07/10] driver core: Add support for links to already probed drivers
On Friday, June 17, 2016 08:26:57 AM Marek Szyprowski wrote: > Set proper link state if link is created between already probed supplier > device and to be probed consumer device. > > Signed-off-by: Marek Szyprowski > --- > drivers/base/core.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 4e778539b750..d9c5c5542a6b 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -125,7 +125,9 @@ struct devlink *device_link_add(struct device *consumer, > > link->flags = flags; > link->status = (flags & DEVICE_LINK_PROBE_TIME) ? > - DEVICE_LINK_CONSUMER_PROBE : DEVICE_LINK_DORMANT; > + DEVICE_LINK_CONSUMER_PROBE : > + (supplier->driver ? DEVICE_LINK_AVAILABLE : > + DEVICE_LINK_DORMANT); > spin_lock_init(&link->lock); > > /* > The supplier->driver check is insufficient and racy. It is insufficient, because supplier->driver is also set during supplier probe and the probe may still not be successful. It is racy, because supplier->driver may be modified right after this check. The only way to address the issue at hand I can see is to add a flag to indicate to device_link_add() that the supplier has already been probed successfully. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 06/10] driver core: Avoid endless recursion if device has more than one link
On Friday, June 17, 2016 08:26:56 AM Marek Szyprowski wrote: > This patch fixes endless recursion, which happends when device has > more than one link. > > Signed-off-by: Marek Szyprowski > --- > drivers/base/core.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/base/core.c b/drivers/base/core.c > index 215cd44de761..4e778539b750 100644 > --- a/drivers/base/core.c > +++ b/drivers/base/core.c > @@ -57,7 +57,8 @@ static int device_reorder_to_tail(struct device *dev, void > *not_used) > device_pm_move_last(dev); > device_for_each_child(dev, NULL, device_reorder_to_tail); > list_for_each_entry(link, &dev->consumer_links, c_node) > - device_reorder_to_tail(link->consumer, NULL); > + if (link->consumer != dev) > + device_reorder_to_tail(link->consumer, NULL); > > return 0; > } > If I'm not mistaken, this should not be necessary unless dev has a link pointing to itself as a consumer. That would be a bug, though. I can add a WARN_ON() to catch this case, but then if there's a link from a consumer of dev pointing back to dev as a consumer, that still will loop forever. I guess we need to detect circular dependencies and fail link creation in such cases. Oh well. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
On Monday, July 25, 2016 12:48:32 AM Lukas Wunner wrote: > On Thu, Jul 21, 2016 at 02:25:15AM +0200, Rafael J. Wysocki wrote: > > On Thursday, July 21, 2016 01:25:53 AM Lukas Wunner wrote: > > > On Thu, Jul 21, 2016 at 12:51:31AM +0200, Rafael J. Wysocki wrote: > > > > On Wednesday, July 20, 2016 05:23:40 PM Lukas Wunner wrote: > > > > > On Wed, Jul 20, 2016 at 02:52:42PM +0200, Rafael J. Wysocki wrote: > > > > > > On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote: > > > > > > > On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote: > > > > > > > > On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote: > > > > > > > > > On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki > > > > > > > > > wrote: > > > > > > > > > > On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner > > > > > > > > > > wrote: > > > > > > > > > > > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek > > > > > > > > > > > Szyprowski wrote: > > > > > > > > > > > > From: "Rafael J. Wysocki" > > > > > > > > > > > We also have such a functional dependency for Thunderbolt > > > > > > > > > > > on Macs: > > > > > > > > > > > On resume from system sleep, the PCIe hotplug ports may > > > > > > > > > > > not resume > > > > > > > > > > > before the thunderbolt driver has reestablished the PCI > > > > > > > > > > > tunnels. > > > > > > > > > > > Currently this is enforced by > > > > > > > > > > > quirk_apple_wait_for_thunderbolt() > > > > > > > > > > > in drivers/pci/quirks.c. It would be good if we could > > > > > > > > > > > represent > > > > > > > > > > > this dependency using something like Rafael's approach > > > > > > > > > > > instead of > > > > > > > > > > > open coding it, however one detail in Rafael's patches is > > > > > > > > > > > problematic: > > > > > > > > > > > > > > > > > > > > > > > New links are added by calling device_link_add() which > > > > > > > > > > > > may happen > > > > > > > > > > > > either before the consumer device is probed or when > > > > > > > > > > > > probing it, in > > > > > > > > > > > > which case the caller needs to ensure that the driver > > > > > > > > > > > > of the > > > > > > > > > > > > supplier device is present and functional and the > > > > > > > > > > > > DEVICE_LINK_PROBE_TIME > > > > > > > > > > > > flag should be passed to device_link_add() to reflect > > > > > > > > > > > > that. > > > > > > > > > > > > > > > > > > > > > > The thunderbolt driver cannot call device_link_add() > > > > > > > > > > > before the > > > > > > > > > > > PCIe hotplug ports are bound to a driver unless we amend > > > > > > > > > > > portdrv > > > > > > > > > > > to return -EPROBE_DEFER for Thunderbolt hotplug ports on > > > > > > > > > > > Macs > > > > > > > > > > > if the thunderbolt driver isn't loaded. > > > > > > > > > > > > > > > > > > > > > > It would therefore be beneficial if device_link_add() can > > > > > > > > > > > be > > > > > > > > > > > called even *after* the consumer is bound. > > > > > > > > > > > > > > > > > > > > I don't quite follow. > > > > > > > > > > > > > > > > > > > > Who's the provider and who's the consumer here? > > > > > > > > > > > > > > > > > > thunderbolt.ko is the supplier. > > >
Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
On Thursday, July 21, 2016 01:25:53 AM Lukas Wunner wrote: > On Thu, Jul 21, 2016 at 12:51:31AM +0200, Rafael J. Wysocki wrote: > > On Wednesday, July 20, 2016 05:23:40 PM Lukas Wunner wrote: > > > On Wed, Jul 20, 2016 at 02:52:42PM +0200, Rafael J. Wysocki wrote: > > > > On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote: > > > > > On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote: > > > > > > On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote: > > > > > > > On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote: > > > > > > > > On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner > > > > > > > > wrote: > > > > > > > > > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski > > > > > > > > > wrote: > > > > > > > > > > From: "Rafael J. Wysocki" > > > > > > > > > We also have such a functional dependency for Thunderbolt on > > > > > > > > > Macs: > > > > > > > > > On resume from system sleep, the PCIe hotplug ports may not > > > > > > > > > resume > > > > > > > > > before the thunderbolt driver has reestablished the PCI > > > > > > > > > tunnels. > > > > > > > > > Currently this is enforced by > > > > > > > > > quirk_apple_wait_for_thunderbolt() > > > > > > > > > in drivers/pci/quirks.c. It would be good if we could > > > > > > > > > represent > > > > > > > > > this dependency using something like Rafael's approach > > > > > > > > > instead of > > > > > > > > > open coding it, however one detail in Rafael's patches is > > > > > > > > > problematic: > > > > > > > > > > > > > > > > > > > New links are added by calling device_link_add() which may > > > > > > > > > > happen > > > > > > > > > > either before the consumer device is probed or when probing > > > > > > > > > > it, in > > > > > > > > > > which case the caller needs to ensure that the driver of the > > > > > > > > > > supplier device is present and functional and the > > > > > > > > > > DEVICE_LINK_PROBE_TIME > > > > > > > > > > flag should be passed to device_link_add() to reflect that. > > > > > > > > > > > > > > > > > > The thunderbolt driver cannot call device_link_add() before > > > > > > > > > the > > > > > > > > > PCIe hotplug ports are bound to a driver unless we amend > > > > > > > > > portdrv > > > > > > > > > to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs > > > > > > > > > if the thunderbolt driver isn't loaded. > > > > > > > > > > > > > > > > > > It would therefore be beneficial if device_link_add() can be > > > > > > > > > called even *after* the consumer is bound. > > > > > > > > > > > > > > > > I don't quite follow. > > > > > > > > > > > > > > > > Who's the provider and who's the consumer here? > > > > > > > > > > > > > > thunderbolt.ko is the supplier. > > > > > > > > > > > > But it binds to the children of the ports that are supposed to be > > > > > > its > > > > > > consumers? > > > > > > > > > > > > Why is that even expected to work? > > > > > > > > > > No, the consumers are aunts (or uncles) of the supplier, if you will. > > > > > :-) > > > > > > > > > > The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" > > > > > in > > > > > the drawing below). The supplier is the NHI: > > > > > > > > > > (Root Port) Upstream Bridge --+-- Downstream Bridge 0 > > > > > NHI > > > > > +-- Downstream Bridge 1 -- > > > &
Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
On Wednesday, July 20, 2016 05:23:40 PM Lukas Wunner wrote: > On Wed, Jul 20, 2016 at 02:52:42PM +0200, Rafael J. Wysocki wrote: > > On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote: > > > On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote: > > > > On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote: > > > > > On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote: > > > > > > On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner > > > > > > wrote: > > > > > > > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote: > > > > > > > > From: "Rafael J. Wysocki" > > > > > > > We also have such a functional dependency for Thunderbolt on Macs: > > > > > > > On resume from system sleep, the PCIe hotplug ports may not resume > > > > > > > before the thunderbolt driver has reestablished the PCI tunnels. > > > > > > > Currently this is enforced by quirk_apple_wait_for_thunderbolt() > > > > > > > in drivers/pci/quirks.c. It would be good if we could represent > > > > > > > this dependency using something like Rafael's approach instead of > > > > > > > open coding it, however one detail in Rafael's patches is > > > > > > > problematic: > > > > > > > > > > > > > > > New links are added by calling device_link_add() which may > > > > > > > > happen > > > > > > > > either before the consumer device is probed or when probing it, > > > > > > > > in > > > > > > > > which case the caller needs to ensure that the driver of the > > > > > > > > supplier device is present and functional and the > > > > > > > > DEVICE_LINK_PROBE_TIME > > > > > > > > flag should be passed to device_link_add() to reflect that. > > > > > > > > > > > > > > The thunderbolt driver cannot call device_link_add() before the > > > > > > > PCIe hotplug ports are bound to a driver unless we amend portdrv > > > > > > > to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs > > > > > > > if the thunderbolt driver isn't loaded. > > > > > > > > > > > > > > It would therefore be beneficial if device_link_add() can be > > > > > > > called even *after* the consumer is bound. > > > > > > > > > > > > I don't quite follow. > > > > > > > > > > > > Who's the provider and who's the consumer here? > > > > > > > > > > thunderbolt.ko is the supplier. > > > > > > > > But it binds to the children of the ports that are supposed to be its > > > > consumers? > > > > > > > > Why is that even expected to work? > > > > > > No, the consumers are aunts (or uncles) of the supplier, if you will. :-) > > > > > > The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in > > > the drawing below). The supplier is the NHI: > > > > > > (Root Port) Upstream Bridge --+-- Downstream Bridge 0 NHI > > > +-- Downstream Bridge 1 -- > > > +-- Downstream Bridge 2 -- > > > ... > > > > > > We're calling pci_power_up() and pci_restore_state() from > > > pci_pm_resume_noirq(). And that will fail for devices below > > > the hotplug ports if the PCI tunnels haven't been re-established > > > yet by the NHI. > > > > So the NHI is a PCIe device, right? > > > > Does the Thunderbolt driver bind to that device? > > The NHI is a PCI device but not a bridge. It has class 0x88000. > Yes, thunderbolt.ko binds to the NHI. > > And portdrv binds to the upstream bridge and downstream bridges. > Those have class 0x60400. OK, so why would there be a problem with creating links from the NHI (producer) to the ports (consumers) before binding portdrv to them? Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
On Wednesday, July 20, 2016 08:24:50 AM Lukas Wunner wrote: > On Wed, Jul 20, 2016 at 02:33:18AM +0200, Rafael J. Wysocki wrote: > > On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote: > > > On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote: > > > > On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner wrote: > > > > > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote: > > > > > > From: "Rafael J. Wysocki" > > > > > We also have such a functional dependency for Thunderbolt on Macs: > > > > > On resume from system sleep, the PCIe hotplug ports may not resume > > > > > before the thunderbolt driver has reestablished the PCI tunnels. > > > > > Currently this is enforced by quirk_apple_wait_for_thunderbolt() > > > > > in drivers/pci/quirks.c. It would be good if we could represent > > > > > this dependency using something like Rafael's approach instead of > > > > > open coding it, however one detail in Rafael's patches is problematic: > > > > > > > > > > > New links are added by calling device_link_add() which may happen > > > > > > either before the consumer device is probed or when probing it, in > > > > > > which case the caller needs to ensure that the driver of the > > > > > > supplier device is present and functional and the > > > > > > DEVICE_LINK_PROBE_TIME > > > > > > flag should be passed to device_link_add() to reflect that. > > > > > > > > > > The thunderbolt driver cannot call device_link_add() before the > > > > > PCIe hotplug ports are bound to a driver unless we amend portdrv > > > > > to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs > > > > > if the thunderbolt driver isn't loaded. > > > > > > > > > > It would therefore be beneficial if device_link_add() can be > > > > > called even *after* the consumer is bound. > > > > > > > > I don't quite follow. > > > > > > > > Who's the provider and who's the consumer here? > > > > > > thunderbolt.ko is the supplier. > > > > But it binds to the children of the ports that are supposed to be its > > consumers? > > > > Why is that even expected to work? > > No, the consumers are aunts (or uncles) of the supplier, if you will. :-) > > The consumers are the hotplug ports (named "Downstream Bridge 1 / 2" in > the drawing below). The supplier is the NHI: > > (Root Port) Upstream Bridge --+-- Downstream Bridge 0 NHI > +-- Downstream Bridge 1 -- > +-- Downstream Bridge 2 -- > ... > > We're calling pci_power_up() and pci_restore_state() from > pci_pm_resume_noirq(). And that will fail for devices below > the hotplug ports if the PCI tunnels haven't been re-established > yet by the NHI. So the NHI is a PCIe device, right? Does the Thunderbolt driver bind to that device? > Currently we achieve that via quirk_apple_wait_for_thunderbolt() in > drivers/pci/quirks.c. It would be more elegant if we could make this > relationship explicit with "device links" and let the core handle it. > > Or am I mistaken and this particular use case is not what "device links" > are intended for? I'm not sure yet. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
On Friday, June 17, 2016 04:07:38 PM Lukas Wunner wrote: > On Fri, Jun 17, 2016 at 02:54:56PM +0200, Rafael J. Wysocki wrote: > > On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner wrote: > > > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote: > > > > From: "Rafael J. Wysocki" > > > We also have such a functional dependency for Thunderbolt on Macs: > > > On resume from system sleep, the PCIe hotplug ports may not resume > > > before the thunderbolt driver has reestablished the PCI tunnels. > > > Currently this is enforced by quirk_apple_wait_for_thunderbolt() > > > in drivers/pci/quirks.c. It would be good if we could represent > > > this dependency using something like Rafael's approach instead of > > > open coding it, however one detail in Rafael's patches is problematic: > > > > > > > New links are added by calling device_link_add() which may happen > > > > either before the consumer device is probed or when probing it, in > > > > which case the caller needs to ensure that the driver of the > > > > supplier device is present and functional and the DEVICE_LINK_PROBE_TIME > > > > flag should be passed to device_link_add() to reflect that. > > > > > > The thunderbolt driver cannot call device_link_add() before the > > > PCIe hotplug ports are bound to a driver unless we amend portdrv > > > to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs > > > if the thunderbolt driver isn't loaded. > > > > > > It would therefore be beneficial if device_link_add() can be > > > called even *after* the consumer is bound. > > > > I don't quite follow. > > > > Who's the provider and who's the consumer here? > > thunderbolt.ko is the supplier. But it binds to the children of the ports that are supposed to be its consumers? Why is that even expected to work? Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 02/10] driver core: Functional dependencies tracking support
On Fri, Jun 17, 2016 at 12:36 PM, Lukas Wunner wrote: > Hi Marek, > > On Fri, Jun 17, 2016 at 08:26:52AM +0200, Marek Szyprowski wrote: >> From: "Rafael J. Wysocki" >> >> Currently, there is a problem with handling cases where functional >> dependencies between devices are involved. >> >> What I mean by a "functional dependency" is when the driver of device >> B needs both device A and its driver to be present and functional to >> be able to work. This implies that the driver of A needs to be >> working for B to be probed successfully and it cannot be unbound from >> the device before the B's driver. This also has certain consequences >> for power management of these devices (suspend/resume and runtime PM >> ordering). >> >> Add support for representing those functional dependencies between >> devices to allow the driver core to track them and act on them in >> certain cases where they matter. > > Rafael has indicated that he intends to respin this series: > https://lkml.org/lkml/2016/6/8/1061 That's OK. > We also have such a functional dependency for Thunderbolt on Macs: > On resume from system sleep, the PCIe hotplug ports may not resume > before the thunderbolt driver has reestablished the PCI tunnels. > Currently this is enforced by quirk_apple_wait_for_thunderbolt() > in drivers/pci/quirks.c. It would be good if we could represent > this dependency using something like Rafael's approach instead of > open coding it, however one detail in Rafael's patches is problematic: > >> New links are added by calling device_link_add() which may happen >> either before the consumer device is probed or when probing it, in >> which case the caller needs to ensure that the driver of the >> supplier device is present and functional and the DEVICE_LINK_PROBE_TIME >> flag should be passed to device_link_add() to reflect that. > > The thunderbolt driver cannot call device_link_add() before the > PCIe hotplug ports are bound to a driver unless we amend portdrv > to return -EPROBE_DEFER for Thunderbolt hotplug ports on Macs > if the thunderbolt driver isn't loaded. > > It would therefore be beneficial if device_link_add() can be > called even *after* the consumer is bound. I don't quite follow. Who's the provider and who's the consumer here? Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH 1/3] PM / Runtime: Add notifiers for device runtime PM events
On Wed, Jun 8, 2016 at 12:25 PM, Marek Szyprowski
wrote:
> From: Krzysztof Kozlowski
>
> Allow drivers registering for certain runtime PM events of other
> devices. Some drivers in power domain are more or less coupled. When one
> driver is suspending (thus leading to power domain being also turned
> off) the other might have to perform some necessary steps. For example
> Exynos IOMMU has to save its context.
>
> Based on previous work of Sylwester Nawrocki .
>
> Signed-off-by: Krzysztof Kozlowski
> Signed-off-by: Marek Szyprowski
No, this is not the right way to address this and using notifiers for
that is just wrong (because of the potential ordering issues).
Also, the problem is not limited to runtime PM, but also to system
suspend/resume and initialization/shutdown.
I posted a series of device dependencies patches a few months ago that
might help to address this problem, but there was almost no interest
in it at that time.
Thanks,
Rafael
> ---
> drivers/base/power/generic_ops.c | 9 +++
> drivers/base/power/power.h | 6 +
> drivers/base/power/runtime.c | 34 +--
> include/linux/pm.h | 2 ++
> include/linux/pm_runtime.h | 51
>
> 5 files changed, 100 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/base/power/generic_ops.c
> b/drivers/base/power/generic_ops.c
> index 07c3c4a9522d..f0838229b781 100644
> --- a/drivers/base/power/generic_ops.c
> +++ b/drivers/base/power/generic_ops.c
> @@ -10,6 +10,7 @@
> #include
> #include
> #include
> +#include "power.h"
>
> #ifdef CONFIG_PM
> /**
> @@ -25,8 +26,12 @@ int pm_generic_runtime_suspend(struct device *dev)
> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> int ret;
>
> + pm_runtime_notifier_call(dev, RPM_EVENT_SUSPEND_PRE);
> +
> ret = pm && pm->runtime_suspend ? pm->runtime_suspend(dev) : 0;
>
> + pm_runtime_notifier_call(dev, RPM_EVENT_SUSPEND_POST);
> +
> return ret;
> }
> EXPORT_SYMBOL_GPL(pm_generic_runtime_suspend);
> @@ -44,8 +49,12 @@ int pm_generic_runtime_resume(struct device *dev)
> const struct dev_pm_ops *pm = dev->driver ? dev->driver->pm : NULL;
> int ret;
>
> + pm_runtime_notifier_call(dev, RPM_EVENT_RESUME_PRE);
> +
> ret = pm && pm->runtime_resume ? pm->runtime_resume(dev) : 0;
>
> + pm_runtime_notifier_call(dev, RPM_EVENT_RESUME_POST);
> +
> return ret;
> }
> EXPORT_SYMBOL_GPL(pm_generic_runtime_resume);
> diff --git a/drivers/base/power/power.h b/drivers/base/power/power.h
> index 50e30e7b059d..30b6319ce96c 100644
> --- a/drivers/base/power/power.h
> +++ b/drivers/base/power/power.h
> @@ -1,4 +1,5 @@
> #include
> +#include
>
> static inline void device_pm_init_common(struct device *dev)
> {
> @@ -20,6 +21,7 @@ static inline void pm_runtime_early_init(struct device *dev)
> extern void pm_runtime_init(struct device *dev);
> extern void pm_runtime_reinit(struct device *dev);
> extern void pm_runtime_remove(struct device *dev);
> +extern int pm_runtime_notifier_call(struct device *dev, enum rpm_event
> event);
>
> struct wake_irq {
> struct device *dev;
> @@ -87,6 +89,10 @@ static inline void pm_runtime_early_init(struct device
> *dev)
> static inline void pm_runtime_init(struct device *dev) {}
> static inline void pm_runtime_reinit(struct device *dev) {}
> static inline void pm_runtime_remove(struct device *dev) {}
> +static inline pm_runtime_notifier_call(struct device *dev, enum rpm_event
> event)
> +{
> + return 0;
> +}
>
> static inline int dpm_sysfs_add(struct device *dev) { return 0; }
> static inline void dpm_sysfs_remove(struct device *dev) {}
> diff --git a/drivers/base/power/runtime.c b/drivers/base/power/runtime.c
> index b74690418504..3a5637ca8400 100644
> --- a/drivers/base/power/runtime.c
> +++ b/drivers/base/power/runtime.c
> @@ -227,6 +227,27 @@ void pm_runtime_set_memalloc_noio(struct device *dev,
> bool enable)
> }
> EXPORT_SYMBOL_GPL(pm_runtime_set_memalloc_noio);
>
> +int pm_runtime_notifier_call(struct device *dev, enum rpm_event event)
> +{
> + return atomic_notifier_call_chain(&dev->power.runtime_notifier,
> + event, dev);
> +}
> +
> +int pm_runtime_register_notifier(struct device *dev, struct notifier_block
> *nb)
> +{
> + return atomic_notifier_chain_register(&dev->power.runtime_notifier,
> + nb);
> +}
> +EXPORT_SYMBOL_GPL(pm_runtime_register_notifier);
> +
> +int pm_runtime_unregister_notifier(struct device *dev,
> + struct notifier_block *nb)
> +{
> + return atomic_notifier_chain_unregister(&dev->power.runtime_notifier,
> + nb);
> +}
> +EXPORT_SYMBOL_GPL(pm_runtime_unregister_notifier);
> +
> /**
> * rpm_check_suspend_allowed - Test whether a device may be s
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Monday, September 28, 2015 10:24:58 AM Arnd Bergmann wrote: > On Sunday 27 September 2015 16:10:48 Rafael J. Wysocki wrote: > > On Saturday, September 26, 2015 09:33:56 PM Arnd Bergmann wrote: > > > On Saturday 26 September 2015 11:40:00 Viresh Kumar wrote: > > > > On 25 September 2015 at 15:19, Rafael J. Wysocki > > > > wrote: > > > > > So if you allow something like debugfs to update your structure, how > > > > > do you make sure there is the proper locking? > > > > > > > > Not really sure at all.. Isn't there some debugfs locking that will > > > > jump in, to avoid updation of fields to the same device? > > > > > > No, if you need any locking to access variable, you cannot use the > > > simple debugfs helpers but have to provide your own functions. > > > > > > > >> Anyway, that problem isn't here for sure as its between two > > > > >> unsigned-longs. So, should I just move it to bool and resend ? > > > > > > > > > > I guess it might be more convenient to fold this into the other patch, > > > > > because we seem to be splitting hairs here. > > > > > > > > I can and that's what I did. But then Arnd asked me to separate it > > > > out. I can fold it back if that's what you want. > > > > > > It still makes sense to keep it separate I think, the patch is clearly > > > different from the other parts. > > > > I just don't see much point in going from unsigned long to u32 and then > > from 32 to bool if we can go directly to bool in one go. > > It's only important to keep the 34-file multi-subsystem trivial cleanup > that doesn't change any functionality separate from the bugfix. Which isn't a bugfix really, because the EC code is not run on any big-endian systems to my knowledge. And it won't matter after the [2/2] anyway. And the changelog of it doesn't really makes sense, because it talks about future systems, but after the [2/2] no future systems will run that code in the first place. > If you like to avoid patching one of the files twice, the alternative would > be to first change the API for all other instances from u32 to bool > and leave ACPI alone, and then do the second patch that changes ACPI > from long to bool. My point is that this patch doesn't matter. It doesn't really fix anything and the result of it goes away after the second patch. The only marginal value of having it as a separate commit is in case if (a) we need to revert the [2/2] for some reason and (b) ACPI-based ARM systems (the big-endian ones) become full-hardware at one point. You know what the chances of that are, though. :-) That said I've ACKed the patch, because I don't care that much. I'm not exactly sure why you care either. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V5 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Sun, Sep 27, 2015 at 12:04 AM, Viresh Kumar wrote:
> global_lock is defined as an unsigned long and accessing only its lower
> 32 bits from sysfs is incorrect, as we need to consider other 32 bits
> for big endian 64-bit systems. There are no such platforms yet, but the
> code needs to be robust for such a case.
>
> Fix that by changing type of 'global_lock' to u32.
>
> Signed-off-by: Viresh Kumar
Acked-by: Rafael J. Wysocki
Greg, please take this one along with the [2/2] if that one looks good to you.
> ---
> BCC'd a lot of people (rather than cc'ing them) to make sure
> - the series reaches them
> - mailing lists do not block the patchset due to long cc list
> - and we don't spam the BCC'd people for every reply
>
> V4->V5:
> - Switch back to the original solution of making global_lock u32.
> ---
> drivers/acpi/ec_sys.c | 2 +-
> drivers/acpi/internal.h | 2 +-
> 2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c
> index b4c216bab22b..bea8e425a8de 100644
> --- a/drivers/acpi/ec_sys.c
> +++ b/drivers/acpi/ec_sys.c
> @@ -128,7 +128,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec,
> unsigned int ec_device_count)
> if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe))
> goto error;
> if (!debugfs_create_bool("use_global_lock", 0444, dev_dir,
> -(u32 *)&first_ec->global_lock))
> +&first_ec->global_lock))
> goto error;
>
> if (write_support)
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index 9e426210c2a8..9db196de003c 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -138,7 +138,7 @@ struct acpi_ec {
> unsigned long gpe;
> unsigned long command_addr;
> unsigned long data_addr;
> - unsigned long global_lock;
> + u32 global_lock;
> unsigned long flags;
> unsigned long reference_count;
> struct mutex mutex;
> --
> 2.4.0
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-acpi" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Saturday, September 26, 2015 09:33:56 PM Arnd Bergmann wrote: > On Saturday 26 September 2015 11:40:00 Viresh Kumar wrote: > > On 25 September 2015 at 15:19, Rafael J. Wysocki wrote: > > > So if you allow something like debugfs to update your structure, how > > > do you make sure there is the proper locking? > > > > Not really sure at all.. Isn't there some debugfs locking that will > > jump in, to avoid updation of fields to the same device? > > No, if you need any locking to access variable, you cannot use the > simple debugfs helpers but have to provide your own functions. > > > >> Anyway, that problem isn't here for sure as its between two > > >> unsigned-longs. So, should I just move it to bool and resend ? > > > > > > I guess it might be more convenient to fold this into the other patch, > > > because we seem to be splitting hairs here. > > > > I can and that's what I did. But then Arnd asked me to separate it > > out. I can fold it back if that's what you want. > > It still makes sense to keep it separate I think, the patch is clearly > different from the other parts. I just don't see much point in going from unsigned long to u32 and then from 32 to bool if we can go directly to bool in one go. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Saturday, September 26, 2015 12:52:08 PM James Bottomley wrote: > On Fri, 2015-09-25 at 22:58 +0200, Rafael J. Wysocki wrote: > > On Friday, September 25, 2015 01:25:49 PM Viresh Kumar wrote: > > > On 25 September 2015 at 13:33, Rafael J. Wysocki > > > wrote: > > > > You're going to change that into bool in the next patch, right? > > > > > > Yeah. > > > > > > > So what if bool is a byte and the field is not word-aligned > > > > > > Its between two 'unsigned long' variables today, and the struct isn't > > > packed. > > > So, it will be aligned, isn't it? > > > > > > > and changing > > > > that byte requires a read-modify-write. How do we ensure that things > > > > remain > > > > consistent in that case? > > > > > > I didn't understood why a read-modify-write is special here? That's > > > what will happen > > > to most of the non-word-sized fields anyway? > > > > > > Probably I didn't understood what you meant.. > > > > Say you have three adjacent fields in a structure, x, y, z, each one byte > > long. > > Initially, all of them are equal to 0. > > > > CPU A writes 1 to x and CPU B writes 2 to y at the same time. > > > > What's the result? > > I think every CPU's cache architecure guarantees adjacent store > integrity, even in the face of SMP, so it's x==1 and y==2. If you're > thinking of old alpha SMP system where the lowest store width is 32 bits > and thus you have to do RMW to update a byte, this was usually fixed by > padding (assuming the structure is not packed). However, it was such a > problem that even the later alpha chips had byte extensions. OK, thanks! Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Fri, Sep 25, 2015 at 11:44 PM, Viresh Kumar wrote: > On 25-09-15, 22:58, Rafael J. Wysocki wrote: >> Say you have three adjacent fields in a structure, x, y, z, each one byte >> long. >> Initially, all of them are equal to 0. >> >> CPU A writes 1 to x and CPU B writes 2 to y at the same time. >> >> What's the result? > > But then two CPUs can update the same variable as well, and we must > have proper locking in place to fix that. Right. So if you allow something like debugfs to update your structure, how do you make sure there is the proper locking? Is that not a problem in all of the places modified by the [2/2]? How can the future users of the API know what to do to avoid potential problems with it? > > Anyway, that problem isn't here for sure as its between two > unsigned-longs. So, should I just move it to bool and resend ? I guess it might be more convenient to fold this into the other patch, because we seem to be splitting hairs here. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Friday, September 25, 2015 01:25:49 PM Viresh Kumar wrote: > On 25 September 2015 at 13:33, Rafael J. Wysocki wrote: > > You're going to change that into bool in the next patch, right? > > Yeah. > > > So what if bool is a byte and the field is not word-aligned > > Its between two 'unsigned long' variables today, and the struct isn't packed. > So, it will be aligned, isn't it? > > > and changing > > that byte requires a read-modify-write. How do we ensure that things remain > > consistent in that case? > > I didn't understood why a read-modify-write is special here? That's > what will happen > to most of the non-word-sized fields anyway? > > Probably I didn't understood what you meant.. Say you have three adjacent fields in a structure, x, y, z, each one byte long. Initially, all of them are equal to 0. CPU A writes 1 to x and CPU B writes 2 to y at the same time. What's the result? Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Friday, September 25, 2015 10:26:22 PM Rafael J. Wysocki wrote: > On Friday, September 25, 2015 11:52:56 AM Viresh Kumar wrote: > > On 25-09-15, 20:49, Johannes Berg wrote: > > > Ok, then, but that means Rafael is completely wrong ... > > > debugfs_create_bool() takes a *pointer* and it needs to be long-lived, > > > it can't be on the stack. You also don't get a call when it changes. > > > > Ahh, ofcourse. My bad as well... > > Well, sorry about the wrong suggestion. > > > I think we can change structure definition but will wait for Rafael's > > comment before that. > > OK, change the structure then. But here's a question. You're going to change that into bool in the next patch, right? So what if bool is a byte and the field is not word-aligned and changing that byte requires a read-modify-write. How do we ensure that things remain consistent in that case? Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Friday, September 25, 2015 11:52:56 AM Viresh Kumar wrote: > On 25-09-15, 20:49, Johannes Berg wrote: > > Ok, then, but that means Rafael is completely wrong ... > > debugfs_create_bool() takes a *pointer* and it needs to be long-lived, > > it can't be on the stack. You also don't get a call when it changes. > > Ahh, ofcourse. My bad as well... Well, sorry about the wrong suggestion. > I think we can change structure definition but will wait for Rafael's > comment before that. OK, change the structure then. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Friday, September 25, 2015 10:18:13 PM Rafael J. Wysocki wrote: > On Friday, September 25, 2015 09:41:37 AM Viresh Kumar wrote: > > global_lock is defined as an unsigned long and accessing only its lower > > 32 bits from sysfs is incorrect, as we need to consider other 32 bits > > for big endian 64 bit systems. There are no such platforms yet, but the > > code needs to be robust for such a case. > > > > Fix that by passing a local variable to debugfs_create_bool() and > > assigning its value to global_lock later. > > > > Signed-off-by: Viresh Kumar > > Acked-by: Rafael J. Wysocki > > Greg, please take this one if the [2/2] looks good to you. Ouch, turns out it was a bad idea. Please scratch that. Thanks, Rafael ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH V4 1/2] ACPI / EC: Fix broken 64bit big-endian users of 'global_lock'
On Friday, September 25, 2015 09:41:37 AM Viresh Kumar wrote:
> global_lock is defined as an unsigned long and accessing only its lower
> 32 bits from sysfs is incorrect, as we need to consider other 32 bits
> for big endian 64 bit systems. There are no such platforms yet, but the
> code needs to be robust for such a case.
>
> Fix that by passing a local variable to debugfs_create_bool() and
> assigning its value to global_lock later.
>
> Signed-off-by: Viresh Kumar
Acked-by: Rafael J. Wysocki
Greg, please take this one if the [2/2] looks good to you.
> ---
> V3->V4:
> - Create a local variable instead of changing type of global_lock
> (Rafael)
> - Drop the stable tag
> - BCC'd a lot of people (rather than cc'ing them) to make sure
> - the series reaches them
> - mailing lists do not block the patchset due to long cc list
> - and we don't spam the BCC'd people for every reply
> ---
> drivers/acpi/ec_sys.c | 6 --
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/ec_sys.c b/drivers/acpi/ec_sys.c
> index b4c216bab22b..b44b91331a56 100644
> --- a/drivers/acpi/ec_sys.c
> +++ b/drivers/acpi/ec_sys.c
> @@ -110,6 +110,7 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec,
> unsigned int ec_device_count)
> struct dentry *dev_dir;
> char name[64];
> umode_t mode = 0400;
> + u32 val;
>
> if (ec_device_count == 0) {
> acpi_ec_debugfs_dir = debugfs_create_dir("ec", NULL);
> @@ -127,10 +128,11 @@ static int acpi_ec_add_debugfs(struct acpi_ec *ec,
> unsigned int ec_device_count)
>
> if (!debugfs_create_x32("gpe", 0444, dev_dir, (u32 *)&first_ec->gpe))
> goto error;
> - if (!debugfs_create_bool("use_global_lock", 0444, dev_dir,
> - (u32 *)&first_ec->global_lock))
> + if (!debugfs_create_bool("use_global_lock", 0444, dev_dir, &val))
> goto error;
>
> + first_ec->global_lock = val;
> +
> if (write_support)
> mode = 0600;
> if (!debugfs_create_file("io", mode, dev_dir, ec, &acpi_ec_io_ops))
>
Thanks,
Rafael
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 4/7] DMA-API: Add dma_(un)map_resource() documentation
On 7/8/2015 5:11 PM, Bjorn Helgaas wrote: [+cc Rafael] On Tue, Jul 07, 2015 at 01:14:27PM -0400, Mark Hounschell wrote: On 07/07/2015 11:15 AM, Bjorn Helgaas wrote: On Wed, May 20, 2015 at 08:11:17AM -0400, Mark Hounschell wrote: Most currently available hardware doesn't allow reads but will allow writes on PCIe peer-to-peer transfers. All current AMD chipsets are this way. I'm pretty sure all Intel chipsets are this way also. What happens with reads is they are just dropped with no indication of error other than the data will not be as expected. Supposedly the PCIe spec does not even require any peer-to-peer support. Regular PCI there is no problem and this API could be useful. However I doubt seriously you will find a pure PCI motherboard that has an IOMMU. I don't understand the chipset manufactures reasoning for disabling PCIe peer-to-peer reads. We would like to make PCIe versions of our cards but their application requires peer-to-peer reads and writes. So we cannot develop PCIe versions of the cards. I'd like to understand this better. Peer-to-peer between two devices below the same Root Port should work as long as ACS doesn't prevent it. If we find an Intel or AMD IOMMU, I think we configure ACS to prevent direct peer-to-peer (see "pci_acs_enable"), but maybe it could still be done with the appropriate IOMMU support. And if you boot with "iommu=off", we don't do that ACS configuration, so peer-to-peer should work. I suppose the problem is that peer-to-peer doesn't work between devices under different Root Ports or even devices under different Root Complexes? PCIe r3.0, sec 6.12.1.1, says Root Ports that support peer-to-peer traffic are required to implement ACS P2P Request Redirect, so if a Root Port doesn't implement RR, we can assume it doesn't support peer-to-peer. But unfortunately the converse is not true: if a Root Port implements RR, that does *not* imply that it supports peer-to-peer traffic. So I don't know how to discover whether peer-to-peer between Root Ports or Root Complexes is supported. Maybe there's some clue in the IOMMU? The Intel VT-d spec mentions it, but "peer" doesn't even appear in the AMD spec. And I'm curious about why writes sometimes work when reads do not. That sounds like maybe the hardware support is there, but we don't understand how to configure everything correctly. Can you give us the specifics of the topology you'd like to use, e.g., lspci -vv of the path between the two devices? First off, writes always work for me. Not just sometimes. Only reads NEVER do. Reading the AMD-990FX-990X-970-Register-Programming-Requirements-48693.pdf in section 2.5 "Enabling/Disabling Peer-to-Peer Traffic Access", it states specifically that only P2P memory writes are supported. This has been the case with older AMD chipset also. In one of the older chipset documents I read (I think the 770 series) , it said this was a security feature. Makes no sense to me. As for the topology I'd like to be able to use. This particular configuration (MB) has a single regular pci slot and the rest are pci-e. In two of those pci-e slots is a pci-e to pci expansion chassis interface card connected to a regular pci expansion rack. I am trying to to peer to peer between a regular pci card in one of those chassis to another regular pci card in the other chassis. In turn through the pci-e subsystem. Attached is the lcpci -vv output from this particular box. The cards that initiate the P2P are these: 04:04.0 Intelligent controller [0e80]: PLX Technology, Inc. Device 0480 (rev 55) 04:05.0 Intelligent controller [0e80]: PLX Technology, Inc. Device 0480 (rev 55) 04:06.0 Intelligent controller [0e80]: PLX Technology, Inc. Device 0480 (rev 55) 04:07.0 Intelligent controller [0e80]: PLX Technology, Inc. Device 0480 (rev 55) The card they need to P2P to and from is this one. 0a:05.0 Network controller: VMIC GE-IP PCI5565,PMC5565 Reflective Memory Node (rev 01) Peer-to-peer traffic initiated by 04:04.0 and targeted at 0a:05.0 has to be routed up to Root Port 00:04.0, over to Root Port 00:0b.0, and back down to 0a:05.0: 00:04.0: Root Port to [bus 02-05] Slot #4 ACS ReqRedir+ 02:00.0: PCIe-to-PCI bridge to [bus 03-05] 03:04.0: PCI-to-PCI bridge to [bus 04-05] 04:04.0: PLX intelligent controller 00:0b.0: Root Port to [bus 08-0e] Slot #11 ACS ReqRedir+ 00:0b.0: bridge window [mem 0xd000-0xd84f] 08:00.0: PCIe-to-PCI bridge to [bus 09-0e] 08:00.0: bridge window [mem 0xd000-0xd84f] 09:04.0: PCI-to-PCI bridge to [bus 0a-0e] 09:04.0: bridge window [mem 0xd000-0xd84f] 0a:05.0: VMIC GE-IP reflective memory node 0a:05.0: BAR 3 [mem 0xd000-0xd7ff] Both Root Ports do support ACS, including P2P RR, but that doesn't tell us anything about whether the Root Complex actually supports peer-to-peer traffic between the Root Ports. Per the AMD 990FX/990X/970 spec, your hardware supports it for writes but not reads. So your hardware
Re: [RFC PATCH 2/2] iommu: rockchip: Handle system-wide and runtime PM
On Thursday, December 18, 2014 11:28:58 PM Laurent Pinchart wrote: > Hi Kevin, > [cut] > > >> > > >> It would be better to be able to reference count the DMA engine from the > > >> bus master IMO and arguably you can use the runtime PM framework for > > >> that. Namely, give bus masters someting like > > >> > > >> pm_runtime_get_my_DMA_engine(bus_master_device) > > >> pm_runtime_put_my_DMA_engine(bus_master_device) > > >> > > >> and let them call these as they see fit. > > > > > > Please note that we're not talking about DMA engines here, but about > > > IOMMUs. DMA is involved through the DMA mapping API which hides the IOMMU > > > completely from the bus master drivers, not the DMA engine API. > > > > > > Exposing the IOMMU is something we want to avoid, but DMA mapping > > > start/stop operations could certainly be implemented. > > > > The problem with that is it only solves the IOMMU problem. We have a > > more generic PM dependency problem of which this IOMMU example is only a > > subset, so I think we need a more generic solution. > > I agree that a more generic solution is needed at least to support ACPI _DEP, > but that might not be optimal in the IOMMU use case as explained above. Well, since we need it anyway, why don't we implement it and then figure out if anything more specific needs to be done for the IOMMU case? -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [RFC PATCH 2/2] iommu: rockchip: Handle system-wide and runtime PM
On Wednesday, December 17, 2014 02:15:31 AM Laurent Pinchart wrote: > Hi Tomasz, > > On Tuesday 16 December 2014 11:18:33 Tomasz Figa wrote: > > On Tue, Dec 16, 2014 at 4:53 AM, Laurent Pinchart wrote: > > > On Monday 15 December 2014 11:39:01 Tomasz Figa wrote: > > >> On Sat, Dec 13, 2014 at 5:47 AM, Laurent Pinchart wrote: > > >>> On Friday 12 December 2014 13:15:51 Tomasz Figa wrote: > > >>>> On Fri, Dec 12, 2014 at 5:48 AM, Rafael J. Wysocki wrote: > > >>>>> On Thursday, December 11, 2014 04:51:37 PM Ulf Hansson wrote: > > >>>>>> On 11 December 2014 at 16:31, Kevin Hilman wrote: > > >>>>>>> [+ Laurent Pinchart] > > >>>>>>> > > >>>>>>> Tomasz Figa writes: > > >>>>>>>> On Thu, Dec 11, 2014 at 8:58 PM, Ulf Hansson wrote: > > >>>>>>> [...] > > >>>>>>> > > >>>>>>>>>> @@ -988,11 +1107,28 @@ static int rk_iommu_probe(struct > > >>>>>>>>>> platform_device *pdev) > > >>>>>>>>>> return -ENXIO; > > >>>>>>>>>> } > > >>>>>>>>>> > > >>>>>>>>>> + pm_runtime_no_callbacks(dev); > > >>>>>>>>>> + pm_runtime_enable(dev); > > >>>>>>>>>> + > > >>>>>>>>>> + /* Synchronize state of the domain with driver data. */ > > >>>>>>>>>> + pm_runtime_get_sync(dev); > > >>>>>>>>>> + iommu->is_powered = true; > > >>>>>>>>> > > >>>>>>>>> Doesn't the runtime PM status reflect the value of "is_powered", > > >>>>>>>>> thus why do you need to have a copy of it? Could it perpahps be > > >>>>>>>>> that you try to cope with the case when CONFIG_PM is unset? > > >>>>>>>> > > >>>>>>>> It's worth noting that this driver fully relies on status of other > > >>>>>>>> devices in the power domain the IOMMU is in and does not enforce > > >>>>>>>> the status on its own. So in general, as far as my understanding > > >>>>>>>> of PM runtime subsystem, the status of the IOMMU device will be > > >>>>>>>> always suspended, because nobody will call pm_runtime_get() on it > > >>>>>>>> (except the get and put pair in probe). So is_powered is here to > > >>>>>>>> track status of the domain, not the device. Feel free to suggest a > > >>>>>>>> better way, though. > > >>>>>>> > > >>>>>>> I still don't like these notifiers. I think they add ways to > > >>>>>>> bypass having proper runtime PM implemented for devices/subsystems. > > >>>>>> > > >>>>>> I do agree, but I haven't found another good solution to the > > >>>>>> problem. > > >>>>> > > >>>>> For the record, I'm not liking this mostly because it "fixes" a > > >>>>> generic problem in a way that's hidden in the genpd code and very > > >>>>> indirect. > > >>>> > > >>>> Well, that's true. This is indeed a generic problem of PM dependencies > > >>>> between devices (other than those represented by parent-child > > >>>> relation), which in fact doesn't have much to do with genpd, but > > >>>> rather with those devices directly. It is just that genpd is the most > > >>>> convenient location to solve this in current code and in a simple way. > > >>>> In other words, I see this solution as a reasonable way to get the > > >>>> problem solved quickly for now, so that we can start thinking about a > > >>>> more elegant solution. > > >>>> > > >>>>>>> From a high-level, the IOMMU is just another device inside the PM > > >>>>>>> domain, so ideally it should be doing it's own _get() and _put() > > >>>>>>> calls so the PM domain code
Re: [RFC PATCH 2/2] iommu: rockchip: Handle system-wide and runtime PM
On Thursday, December 11, 2014 04:51:37 PM Ulf Hansson wrote: > On 11 December 2014 at 16:31, Kevin Hilman wrote: > > [+ Laurent Pinchart] > > > > Tomasz Figa writes: > > > >> On Thu, Dec 11, 2014 at 8:58 PM, Ulf Hansson > >> wrote: > > > > [...] > > > >>>> @@ -988,11 +1107,28 @@ static int rk_iommu_probe(struct platform_device > >>>> *pdev) > >>>> return -ENXIO; > >>>> } > >>>> > >>>> + pm_runtime_no_callbacks(dev); > >>>> + pm_runtime_enable(dev); > >>>> + > >>>> + /* Synchronize state of the domain with driver data. */ > >>>> + pm_runtime_get_sync(dev); > >>>> + iommu->is_powered = true; > >>> > >>> Doesn't the runtime PM status reflect the value of "is_powered", thus > >>> why do you need to have a copy of it? Could it perpahps be that you > >>> try to cope with the case when CONFIG_PM is unset? > >>> > >> > >> It's worth noting that this driver fully relies on status of other > >> devices in the power domain the IOMMU is in and does not enforce the > >> status on its own. So in general, as far as my understanding of PM > >> runtime subsystem, the status of the IOMMU device will be always > >> suspended, because nobody will call pm_runtime_get() on it (except the > >> get and put pair in probe). So is_powered is here to track status of > >> the domain, not the device. Feel free to suggest a better way, though. > > > > I still don't like these notifiers. I think they add ways to bypass > > having proper runtime PM implemented for devices/subsystems. > > I do agree, but I haven't found another good solution to the problem. For the record, I'm not liking this mostly because it "fixes" a generic problem in a way that's hidden in the genpd code and very indirect. > > From a high-level, the IOMMU is just another device inside the PM > > domain, so ideally it should be doing it's own _get() and _put() calls > > so the PM domain code would just do the right thing without the need for > > notifiers. > > As I understand it, the IOMMU (or for other similar cases) shouldn't > be doing any get() and put() at all because there are no IO API to > serve request from. > > In principle we could consider these kind devices as "parent" devices > to those other devices that needs them. Then runtime PM core would > take care of things for us, right!? > > Now, I am not so sure using the "parent" approach is actually viable, > since it will likely have other complications, but I haven't > thoroughly thought it though yet. That actually need not be a "parent". What's needed in this case is to do a pm_runtime_get_sync() on a device depended on every time a dependent device is runtime-resumed (and analogously for suspending). The core doesn't have a way to do that, but it looks like we'll need to add it anyway for various reasons (ACPI _DEP is one of them as I mentioned some time ago, but people dismissed it basically as not their problem). -- I speak only for myself. Rafael J. Wysocki, Intel Open Source Technology Center. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [Patch Part3 V1 17/22] pci, ACPI, iommu: enhance pci_root to support DMAR device hotplug
On Tuesday, April 22, 2014 03:07:28 PM Jiang Liu wrote:
> Finally enhance pci_root driver to support DMAR device hotplug when
> hot-plugging PCI host bridges.
>
> Signed-off-by: Jiang Liu
> ---
> drivers/acpi/pci_root.c | 16 ++--
> 1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/acpi/pci_root.c b/drivers/acpi/pci_root.c
> index d388f13d48b4..aa8f549869f3 100644
> --- a/drivers/acpi/pci_root.c
> +++ b/drivers/acpi/pci_root.c
> @@ -33,6 +33,7 @@
> #include
> #include
> #include
> +#include
> #include
> #include
> #include/* for acpi_hest_init() */
> @@ -511,6 +512,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
> struct acpi_pci_root *root;
> acpi_handle handle = device->handle;
> int no_aspm = 0, clear_aspm = 0;
> + bool hotadd = (system_state != SYSTEM_BOOTING);
The parens are not necessary in the above instruction.
> root = kzalloc(sizeof(struct acpi_pci_root), GFP_KERNEL);
> if (!root)
> @@ -557,6 +559,11 @@ static int acpi_pci_root_add(struct acpi_device *device,
> strcpy(acpi_device_class(device), ACPI_PCI_ROOT_CLASS);
> device->driver_data = root;
>
> + if (hotadd && dmar_device_hotplug(handle, true)) {
> + result = -ENXIO;
> + goto end;
> + }
> +
> pr_info(PREFIX "%s [%s] (domain %04x %pR)\n",
> acpi_device_name(device), acpi_device_bid(device),
> root->segment, &root->secondary);
> @@ -583,7 +590,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
> root->segment, (unsigned int)root->secondary.start);
> device->driver_data = NULL;
> result = -ENODEV;
> - goto end;
> + goto remove_dmar;
> }
>
> if (clear_aspm) {
> @@ -597,7 +604,7 @@ static int acpi_pci_root_add(struct acpi_device *device,
> if (device->wakeup.flags.run_wake)
> device_set_run_wake(root->bus->bridge, true);
>
> - if (system_state != SYSTEM_BOOTING) {
> + if (hotadd) {
> pcibios_resource_survey_bus(root->bus);
> pci_assign_unassigned_root_bus_resources(root->bus);
> }
> @@ -607,6 +614,9 @@ static int acpi_pci_root_add(struct acpi_device *device,
> pci_unlock_rescan_remove();
> return 1;
>
> +remove_dmar:
> + if (hotadd)
> + dmar_device_hotplug(handle, false);
I suppose that works if dmar_device_hotplug() returned false before?
> end:
> kfree(root);
> return result;
> @@ -625,6 +635,8 @@ static void acpi_pci_root_remove(struct acpi_device
> *device)
>
> pci_remove_root_bus(root->bus);
>
> + dmar_device_hotplug(device->handle, false);
> +
> pci_unlock_rescan_remove();
>
> kfree(root);
>
--
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
