iommu/vt-d: Move intel_iommu_ops to header file

PATCH Superseded Andy Shevchenko 2 participants 11 messages Jul 29, 2021 – Oct 8, 2021

Subsystems (2)

IOMMU Intel IOMMU (VT-d)

lore

Share & tools

lore

Andy Shevchenko

author

2021-07-29 16:35 UTC #0 lore raw

PATCH 1/2 iommu/vt-d: Move intel_iommu_ops to header file

Compiler is not happy about hidden declaration of intel_iommu_ops.

.../drivers/iommu/intel/iommu.c:414:24: warning: symbol 'intel_iommu_ops' was not declared. Should it be static?

Move declaration to header file to make compiler happy.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/iommu/intel/dmar.c  | 2 --
 include/linux/intel-iommu.h | 2 ++
 2 files changed, 2 insertions(+), 2 deletions(-)

drivers/iommu/intel/dmar.c +0 -2

index d66f79acd14d..d2d974cf8322 100644
@@ -66,8 +66,6 @@ static unsigned long dmar_seq_ids[BITS_TO_LONGS(DMAR_UNITS_SUPPORTED)];
 static int alloc_iommu(struct dmar_drhd_unit *drhd);
 static void free_iommu(struct intel_iommu *iommu);
 
-extern const struct iommu_ops intel_iommu_ops;
-
 static void dmar_register_drhd_unit(struct dmar_drhd_unit *drhd)
 {
 	/*

include/linux/intel-iommu.h +2 -0

index d0fa0b31994d..309c1e13183a 100644
@@ -811,6 +811,8 @@ struct context_entry *iommu_context_addr(struct intel_iommu *iommu, u8 bus,
 					 u8 devfn, int alloc);
 
 #ifdef CONFIG_INTEL_IOMMU
+extern const struct iommu_ops intel_iommu_ops;
+
 extern int iommu_calculate_agaw(struct intel_iommu *iommu);
 extern int iommu_calculate_max_sagaw(struct intel_iommu *iommu);
 extern int dmar_disabled;

Signature · 6 lines

2.30.2

_______________________________________________
iommu mailing list
[email protected]
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Andy Shevchenko

author

2021-07-29 16:35 UTC #1 lore raw

PATCH 2/2 iommu/vt-d: Drop the kernel doc annotation

Kernel doc validator is unhappy with the following

.../perf.c:16: warning: Function parameter or member 'latency_lock' not described in 'DEFINE_SPINLOCK' .../perf.c:16: warning: expecting prototype for perf.c(). Prototype was for DEFINE_SPINLOCK() instead

Drop kernel doc annotation since the top comment is not in the required format.

Signed-off-by Andy Shevchenko <[email protected]>

---
 drivers/iommu/intel/perf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/iommu/intel/perf.c +1 -1

index 73b7ec705552..0e8e03252d92 100644
@@ -1,5 +1,5 @@
 // SPDX-License-Identifier: GPL-2.0
-/**
+/*
  * perf.c - performance monitor
  *
  * Copyright (C) 2021 Intel Corporation

Signature · 1 line

2.30.2

Lu Baolu

maintainer

2021-07-30 02:22 UTC #2 lore raw

In reply to Andy Shevchenko

Hi Andy,

Quoted 5 lines · Andy Shevchenko

Compiler is not happy about hidden declaration of intel_iommu_ops.
.../drivers/iommu/intel/iommu.c:414:24: warning: symbol 'intel_iommu_ops' was not declared. Should it be static?
Move declaration to header file to make compiler happy.

Thanks for the cleanup. Sharing data structures between different files doesn't seem to be a good design. How about adding a helper so that the intel_iommu_ops could be a static one?

include/linux/intel-iommu.h +0 -0

index 05a65eb155f7..2258e69a93a3 100644
@@ -757,6 +757,7 @@ int intel_iommu_enable_pasid(struct intel_iommu *iommu, struct device *dev);
 struct dmar_domain *find_domain(struct device *dev);
 struct device_domain_info *get_domain_info(struct device *dev);
 struct intel_iommu *device_to_iommu(struct device *dev, u8 *bus, u8

*devfn);
+int intel_iommu_register_device(struct intel_iommu *iommu);

#ifdef CONFIG_INTEL_IOMMU_SVM
extern void intel_svm_check(struct intel_iommu *iommu);
@@ -805,7 +806,6 @@ void intel_iommu_debugfs_init(void);
static inline void intel_iommu_debugfs_init(void) {} #endif /* CONFIG_INTEL_IOMMU_DEBUGFS */

-extern const struct attribute_group *intel_iommu_groups[];
bool context_present(struct context_entry *context);
struct context_entry *iommu_context_addr(struct intel_iommu *iommu, u8
bus,

                                         u8 devfn, int alloc);

drivers/iommu/intel/dmar.c +1 ~~-11~~

index 0ec5514c9980..deef4e77de73 100644
@@ -66,8 +66,6 @@ static unsigned long dmar_seq_ids[BITS_TO_LONGS(DMAR_UNITS_SUPPORTED)];
 static int alloc_iommu(struct dmar_drhd_unit *drhd);
 static void free_iommu(struct intel_iommu *iommu);
 
-extern const struct iommu_ops intel_iommu_ops;
-
 static void dmar_register_drhd_unit(struct dmar_drhd_unit *drhd)
 {
        /*

@@ -1133,15 +1131,9 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
         * present at boot time, then sets intel_iommu_enabled.
         */
        if (intel_iommu_enabled && !drhd->ignored) {
-               err = iommu_device_sysfs_add(&iommu->iommu, NULL,
-                                            intel_iommu_groups,
-                                            "%s", iommu->name);
+               err = intel_iommu_register_device(iommu);
                if (err)
                        goto err_unmap;
-
-               err = iommu_device_register(&iommu->iommu, &intel_iommu_ops, NULL);
-               if (err)
-                       goto err_sysfs;
        }
 
        drhd->iommu = iommu;

@@ -1149,8 +1141,6 @@ static int alloc_iommu(struct dmar_drhd_unit *drhd)
 
        return 0;
 
-err_sysfs:
-       iommu_device_sysfs_remove(&iommu->iommu);
 err_unmap:
        unmap_iommu(iommu);
 error_free_seq_id:

drivers/iommu/intel/iommu.c +19 -2

index 6df5fc5063e1..27ba3062bcac 100644
@@ -388,7 +388,7 @@ int for_each_device_domain(int (*fn)(struct device_domain_info *info,
        return 0;
 }
 
-const struct iommu_ops intel_iommu_ops;
+static const struct iommu_ops intel_iommu_ops;
 
 static bool translation_pre_enabled(struct intel_iommu *iommu)
 {

@@ -4221,7 +4221,7 @@ static struct attribute_group intel_iommu_group = {
        .attrs = intel_iommu_attrs,
 };
 
-const struct attribute_group *intel_iommu_groups[] = {
+static const struct attribute_group *intel_iommu_groups[] = {
        &intel_iommu_group,
        NULL,
 };

@@ -4301,6 +4301,23 @@ static int __init probe_acpi_namespace_devices(void)
        return 0;
 }
 
+int intel_iommu_register_device(struct intel_iommu *iommu)
+{
+       int ret;
+
+       ret = iommu_device_sysfs_add(&iommu->iommu, NULL,
+                                    intel_iommu_groups,
+                                    "%s", iommu->name);
+       if (ret)
+               return ret;
+
+       ret = iommu_device_register(&iommu->iommu, &intel_iommu_ops, NULL);
+       if (ret)
+               iommu_device_sysfs_remove(&iommu->iommu);
+
+       return ret;
+}
+
 int __init intel_iommu_init(void)
 {
        int ret = -ENODEV;

@@ -4407,10 +4424,7 @@ int __init intel_iommu_init(void)
                        pr_info_once("IOMMU batching disallowed due to

virtualization\n");
                        iommu_set_dma_strict();
                }

- iommu_device_sysfs_add(&iommu->iommu, NULL,
- intel_iommu_groups,
- "%s", iommu->name);
- iommu_device_register(&iommu->iommu, &intel_iommu_ops, NULL);

+               intel_iommu_register_device(iommu);
        }
        up_read(&dmar_global_lock);

@@ -5589,7 +5603,7 @@ static void intel_iommu_iotlb_sync_map(struct iommu_domain *domain,

}

-const struct iommu_ops intel_iommu_ops = { +static const struct iommu_ops intel_iommu_ops = {

        .capable                = intel_iommu_capable,
        .domain_alloc           = intel_iommu_domain_alloc,
        .domain_free            = intel_iommu_domain_free,

Best regards,
baolu

Quoted 34 lines

Signed-off-by Andy Shevchenko <[email protected]>

---
  drivers/iommu/intel/dmar.c  | 2 --
  include/linux/intel-iommu.h | 2 ++
  2 files changed, 2 insertions(+), 2 deletions(-)

drivers/iommu/intel/dmar.c +0 -2

index d66f79acd14d..d2d974cf8322 100644
@@ -66,8 +66,6 @@ static unsigned long dmar_seq_ids[BITS_TO_LONGS(DMAR_UNITS_SUPPORTED)];
  static int alloc_iommu(struct dmar_drhd_unit *drhd);
  static void free_iommu(struct intel_iommu *iommu);
 
-extern const struct iommu_ops intel_iommu_ops;
-
  static void dmar_register_drhd_unit(struct dmar_drhd_unit *drhd)
  {
  	/*

include/linux/intel-iommu.h +2 -0

index d0fa0b31994d..309c1e13183a 100644
@@ -811,6 +811,8 @@ struct context_entry *iommu_context_addr(struct intel_iommu *iommu, u8 bus,
  					 u8 devfn, int alloc);
 
  #ifdef CONFIG_INTEL_IOMMU
+extern const struct iommu_ops intel_iommu_ops;
+
  extern int iommu_calculate_agaw(struct intel_iommu *iommu);
  extern int iommu_calculate_max_sagaw(struct intel_iommu *iommu);
  extern int dmar_disabled;

Andy Shevchenko

author

2021-07-30 08:05 UTC #3 lore raw

Quoted 10 lines · Lu Baolu

Quoted 5 lines · Andy Shevchenko
Compiler is not happy about hidden declaration of intel_iommu_ops.
.../drivers/iommu/intel/iommu.c:414:24: warning: symbol 'intel_iommu_ops' was not declared. Should it be static?
Move declaration to header file to make compiler happy.
Thanks for the cleanup. Sharing data structures between different files doesn't seem to be a good design. How about adding a helper so that the intel_iommu_ops could be a static one?

Whatever suits the purpose.
Can you apply patch 2 of this series, please?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Lu Baolu

maintainer

2021-07-30 13:01 UTC #4 lore raw

Quoted 6 lines · Andy Shevchenko

Quoted 5 lines · Lu Baolu
Quoted 4 lines · Andy Shevchenko
Compiler is not happy about hidden declaration of intel_iommu_ops.
.../drivers/iommu/intel/iommu.c:414:24: warning: symbol 'intel_iommu_ops' was not declared. Should it be static?

Quoted 9 lines

Quoted 5 lines
Quoted 1 line
Move declaration to header file to make compiler happy.
Thanks for the cleanup. Sharing data structures between different files doesn't seem to be a good design. How about adding a helper so that the intel_iommu_ops could be a static one?
Whatever suits the purpose.
Can you apply patch 2 of this series, please?

Yes, I will. Thanks!

Best regards,
baolu

Andy Shevchenko

author

2021-08-11 13:50 UTC #5 lore raw

Quoted 13 lines · Lu Baolu

Quoted 12 lines · Andy Shevchenko
Quoted 10 lines · Lu Baolu
Quoted 5 lines · Andy Shevchenko
Compiler is not happy about hidden declaration of intel_iommu_ops.
.../drivers/iommu/intel/iommu.c:414:24: warning: symbol 'intel_iommu_ops' was not declared. Should it be static?
Move declaration to header file to make compiler happy.
Thanks for the cleanup. Sharing data structures between different files doesn't seem to be a good design. How about adding a helper so that the intel_iommu_ops could be a static one?

Quoted 5 lines

Quoted 3 lines
Whatever suits the purpose.
Can you apply patch 2 of this series, please?
Yes, I will. Thanks!

Gentle reminder.

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Lu Baolu

maintainer

2021-08-11 14:02 UTC #6 lore raw

Quoted 14 lines · Andy Shevchenko

Quoted 13 lines · Lu Baolu
Quoted 12 lines · Andy Shevchenko
Quoted 10 lines · Lu Baolu
Quoted 5 lines · Andy Shevchenko
Compiler is not happy about hidden declaration of intel_iommu_ops.
.../drivers/iommu/intel/iommu.c:414:24: warning: symbol 'intel_iommu_ops' was not declared. Should it be static?
Move declaration to header file to make compiler happy.
Thanks for the cleanup. Sharing data structures between different files doesn't seem to be a good design. How about adding a helper so that the intel_iommu_ops could be a static one?

Quoted 8 lines

Quoted 5 lines
Quoted 3 lines
Whatever suits the purpose.
Can you apply patch 2 of this series, please?
Yes, I will. Thanks!
Gentle reminder.

Thanks. Normally I will queue the vt-d patches to Joerg in the rc6 week.

Best regards,
baolu

Andy Shevchenko

author

2021-08-11 14:23 UTC #7 lore raw

Quoted 15 lines · Lu Baolu

Quoted 14 lines · Andy Shevchenko
Quoted 13 lines · Lu Baolu
Quoted 12 lines · Andy Shevchenko
Quoted 10 lines · Lu Baolu
Quoted 5 lines · Andy Shevchenko
Compiler is not happy about hidden declaration of intel_iommu_ops.
.../drivers/iommu/intel/iommu.c:414:24: warning: symbol 'intel_iommu_ops' was not declared. Should it be static?
Move declaration to header file to make compiler happy.
Thanks for the cleanup. Sharing data structures between different files doesn't seem to be a good design. How about adding a helper so that the intel_iommu_ops could be a static one?

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Quoted 2 lines
Whatever suits the purpose.
Can you apply patch 2 of this series, please?
Yes, I will. Thanks!
Gentle reminder.
Thanks. Normally I will queue the vt-d patches to Joerg in the rc6 week.

I see, but don't we need to have them in Linux Next for a few weeks for testing? Perhaps you need to add your tree to be integrated in the Linux Next?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Andy Shevchenko

author

2021-10-06 16:14 UTC #8 lore raw

In reply to Lu Baolu

Quoted 5 lines · Lu Baolu

Hi Andy,
Quoted 2 lines · Andy Shevchenko
Compiler is not happy about hidden declaration of intel_iommu_ops.

Quoted 7 lines

Quoted 3 lines
.../drivers/iommu/intel/iommu.c:414:24: warning: symbol 'intel_iommu_ops' was not declared. Should it be static?
Move declaration to header file to make compiler happy.
Thanks for the cleanup. Sharing data structures between different files doesn't seem to be a good design. How about adding a helper so that the intel_iommu_ops could be a static one?

I don't see any change in the upstream. What's the plan? Can we take my patch as a quick fix?

Signature · 2 lines

With Best Regards,
Andy Shevchenko

Lu Baolu

maintainer

2021-10-08 04:38 UTC #9 lore raw

Hi Andy,

Quoted 8 lines · Andy Shevchenko

Quoted 7 lines · Lu Baolu
Hi Andy,
Quoted 4 lines · Andy Shevchenko
Compiler is not happy about hidden declaration of intel_iommu_ops.
.../drivers/iommu/intel/iommu.c:414:24: warning: symbol 'intel_iommu_ops' was not declared. Should it be static?

Quoted 9 lines

Quoted 5 lines
Quoted 1 line
Move declaration to header file to make compiler happy.
Thanks for the cleanup. Sharing data structures between different files doesn't seem to be a good design. How about adding a helper so that the intel_iommu_ops could be a static one?
I don't see any change in the upstream. What's the plan? Can we take my patch as a quick fix?

Thanks for the reminding.

Can you please tell in which kernel configuration could above warning be triggered?

Best regards,
baolu

Lu Baolu

maintainer

2021-10-08 11:59 UTC #10 lore raw

In reply to Andy Shevchenko

Hi Andy,

Quoted 8 lines · Andy Shevchenko

Quoted 7 lines · Lu Baolu
Hi Andy,
Quoted 4 lines · Andy Shevchenko
Compiler is not happy about hidden declaration of intel_iommu_ops.
.../drivers/iommu/intel/iommu.c:414:24: warning: symbol 'intel_iommu_ops' was not declared. Should it be static?

Quoted 9 lines

Quoted 5 lines
Quoted 1 line
Move declaration to header file to make compiler happy.
Thanks for the cleanup. Sharing data structures between different files doesn't seem to be a good design. How about adding a helper so that the intel_iommu_ops could be a static one?
I don't see any change in the upstream. What's the plan? Can we take my patch as a quick fix?

This patch will cause below build error:

drivers/iommu/intel/dmar.c: In function ‘alloc_iommu’:
drivers/iommu/intel/dmar.c:1140:47: error: ‘intel_iommu_ops’ undeclared (first use in this function); did you mean ‘intel_iommu_groups’?
1140 | err = iommu_device_register(&iommu->iommu, &intel_iommu_ops,

NULL);
      |                                               ^~~~~~~~~~~~~~~
      |                                               intel_iommu_groups

drivers/iommu/intel/dmar.c:1140:47: note: each undeclared identifier is reported only once for each function it appears in make[3]: *** [scripts/Makefile.build:277: drivers/iommu/intel/dmar.o] Error

# CONFIG_INTEL_IOMMU is not set
CONFIG_IRQ_REMAP=y

Best regards,
baolu