Re: [PATCH] iommu/vt-d: Don't register bus-notifier under
On 2017-10-10 09:21, Joerg Roedel wrote: > On Mon, Oct 09, 2017 at 06:58:13PM +0200, Jan Kiszka wrote: >>> extern int dmar_table_init(void); >>> extern int dmar_dev_scope_init(void); >>> +extern void dmar_register_bus_notifier(void); >>> extern int dmar_parse_dev_scope(void *start, void *end, int *cnt, >>> struct dmar_dev_scope **devices, u16 segment); >>> extern void *dmar_alloc_dev_scope(void *start, void *end, int *cnt); >>> >> >> Silences the warning, but locking in the init paths still smells fishy >> to me. > > Yes, its certainly not optimal, but the code that runs in there also > runs at iommu hotplug time, so we can't just remove the locking there > entirely. It may not just be "sub-optimal" but rather borken: "One example: dmar_table_init is not consistently protected by dmar_global_lock." Jan > > On the other side the warning you reported is a false-positive, it can > never dead-lock because the reverse lock-order happens only at > initialization time, but I don't know how to silence it otherwise. > > > Regards, > > Joerg > -- Siemens AG, Corporate Technology, CT RDA ITP SES-DE Corporate Competence Center Embedded Linux ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/vt-d: Don't register bus-notifier under
On Mon, Oct 09, 2017 at 06:58:13PM +0200, Jan Kiszka wrote: > > extern int dmar_table_init(void); > > extern int dmar_dev_scope_init(void); > > +extern void dmar_register_bus_notifier(void); > > extern int dmar_parse_dev_scope(void *start, void *end, int *cnt, > > struct dmar_dev_scope **devices, u16 segment); > > extern void *dmar_alloc_dev_scope(void *start, void *end, int *cnt); > > > > Silences the warning, but locking in the init paths still smells fishy > to me. Yes, its certainly not optimal, but the code that runs in there also runs at iommu hotplug time, so we can't just remove the locking there entirely. On the other side the warning you reported is a false-positive, it can never dead-lock because the reverse lock-order happens only at initialization time, but I don't know how to silence it otherwise. Regards, Joerg ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/vt-d: Don't register bus-notifier under
On 2017-10-06 15:08, Joerg Roedel wrote:
> Hey Jan,
>
> On Wed, Sep 27, 2017 at 04:19:15PM +0200, Jan Kiszka wrote:
>> If we could drop the dmar_global_lock around bus_register_notifier in
>> dmar_dev_scope_init, the issue above would likely be resolved.
>
> That is true. Can you please try this patch?
>
>>From dd7685f84d3954f9361bfb4290fb8a0dd033097a Mon Sep 17 00:00:00 2001
> From: Joerg Roedel <jroe...@suse.de>
> Date: Fri, 6 Oct 2017 15:00:53 +0200
> Subject: [PATCH] iommu/vt-d: Don't register bus-notifier under
> dmar_global_lock
>
> The notifier function will take the dmar_global_lock too, so
> lockdep complains about inverse locking order when the
> notifier is registered under the dmar_global_lock.
>
> Reported-by: Jan Kiszka <jan.kis...@siemens.com>
> Fixes: 59ce0515cdaf ('iommu/vt-d: Update DRHD/RMRR/ATSR device scope caches
> when PCI hotplug happens')
> Signed-off-by: Joerg Roedel <jroe...@suse.de>
> ---
> drivers/iommu/dmar.c| 7 +--
> drivers/iommu/intel-iommu.c | 10 ++
> include/linux/dmar.h| 1 +
> 3 files changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
> index 57c920c1372d..1ea7cd537873 100644
> --- a/drivers/iommu/dmar.c
> +++ b/drivers/iommu/dmar.c
> @@ -801,13 +801,16 @@ int __init dmar_dev_scope_init(void)
> dmar_free_pci_notify_info(info);
> }
> }
> -
> - bus_register_notifier(_bus_type, _pci_bus_nb);
> }
>
> return dmar_dev_scope_status;
> }
>
> +void dmar_register_bus_notifier(void)
> +{
> + bus_register_notifier(_bus_type, _pci_bus_nb);
> +}
> +
>
> int __init dmar_table_init(void)
> {
> diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
> index 6784a05dd6b2..934cef924461 100644
> --- a/drivers/iommu/intel-iommu.c
> +++ b/drivers/iommu/intel-iommu.c
> @@ -4752,6 +4752,16 @@ int __init intel_iommu_init(void)
> goto out_free_dmar;
> }
>
> + up_write(_global_lock);
> +
> + /*
> + * The bus notifier takes the dmar_global_lock, so lockdep will
> + * complain later when we register it under the lock.
> + */
> + dmar_register_bus_notifier();
> +
> + down_write(_global_lock);
> +
> if (no_iommu || dmar_disabled) {
> /*
>* We exit the function here to ensure IOMMU's remapping and
> diff --git a/include/linux/dmar.h b/include/linux/dmar.h
> index e8ffba1052d3..e2433bc50210 100644
> --- a/include/linux/dmar.h
> +++ b/include/linux/dmar.h
> @@ -112,6 +112,7 @@ static inline bool dmar_rcu_check(void)
>
> extern int dmar_table_init(void);
> extern int dmar_dev_scope_init(void);
> +extern void dmar_register_bus_notifier(void);
> extern int dmar_parse_dev_scope(void *start, void *end, int *cnt,
> struct dmar_dev_scope **devices, u16 segment);
> extern void *dmar_alloc_dev_scope(void *start, void *end, int *cnt);
>
Silences the warning, but locking in the init paths still smells fishy
to me.
Thanks,
Jan
--
Siemens AG, Corporate Technology, CT RDA ITP SES-DE
Corporate Competence Center Embedded Linux
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/vt-d: Don't register bus-notifier under
Hey Jan,
On Wed, Sep 27, 2017 at 04:19:15PM +0200, Jan Kiszka wrote:
> If we could drop the dmar_global_lock around bus_register_notifier in
> dmar_dev_scope_init, the issue above would likely be resolved.
That is true. Can you please try this patch?
>From dd7685f84d3954f9361bfb4290fb8a0dd033097a Mon Sep 17 00:00:00 2001
From: Joerg Roedel <jroe...@suse.de>
Date: Fri, 6 Oct 2017 15:00:53 +0200
Subject: [PATCH] iommu/vt-d: Don't register bus-notifier under
dmar_global_lock
The notifier function will take the dmar_global_lock too, so
lockdep complains about inverse locking order when the
notifier is registered under the dmar_global_lock.
Reported-by: Jan Kiszka <jan.kis...@siemens.com>
Fixes: 59ce0515cdaf ('iommu/vt-d: Update DRHD/RMRR/ATSR device scope caches
when PCI hotplug happens')
Signed-off-by: Joerg Roedel <jroe...@suse.de>
---
drivers/iommu/dmar.c| 7 +--
drivers/iommu/intel-iommu.c | 10 ++
include/linux/dmar.h| 1 +
3 files changed, 16 insertions(+), 2 deletions(-)
diff --git a/drivers/iommu/dmar.c b/drivers/iommu/dmar.c
index 57c920c1372d..1ea7cd537873 100644
--- a/drivers/iommu/dmar.c
+++ b/drivers/iommu/dmar.c
@@ -801,13 +801,16 @@ int __init dmar_dev_scope_init(void)
dmar_free_pci_notify_info(info);
}
}
-
- bus_register_notifier(_bus_type, _pci_bus_nb);
}
return dmar_dev_scope_status;
}
+void dmar_register_bus_notifier(void)
+{
+ bus_register_notifier(_bus_type, _pci_bus_nb);
+}
+
int __init dmar_table_init(void)
{
diff --git a/drivers/iommu/intel-iommu.c b/drivers/iommu/intel-iommu.c
index 6784a05dd6b2..934cef924461 100644
--- a/drivers/iommu/intel-iommu.c
+++ b/drivers/iommu/intel-iommu.c
@@ -4752,6 +4752,16 @@ int __init intel_iommu_init(void)
goto out_free_dmar;
}
+ up_write(_global_lock);
+
+ /*
+* The bus notifier takes the dmar_global_lock, so lockdep will
+* complain later when we register it under the lock.
+*/
+ dmar_register_bus_notifier();
+
+ down_write(_global_lock);
+
if (no_iommu || dmar_disabled) {
/*
* We exit the function here to ensure IOMMU's remapping and
diff --git a/include/linux/dmar.h b/include/linux/dmar.h
index e8ffba1052d3..e2433bc50210 100644
--- a/include/linux/dmar.h
+++ b/include/linux/dmar.h
@@ -112,6 +112,7 @@ static inline bool dmar_rcu_check(void)
extern int dmar_table_init(void);
extern int dmar_dev_scope_init(void);
+extern void dmar_register_bus_notifier(void);
extern int dmar_parse_dev_scope(void *start, void *end, int *cnt,
struct dmar_dev_scope **devices, u16 segment);
extern void *dmar_alloc_dev_scope(void *start, void *end, int *cnt);
--
2.13.6
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
