date:20200930

01.10.2020 05:48, Nicolin Chen пишет:
> On Thu, Oct 01, 2020 at 05:06:19AM +0300, Dmitry Osipenko wrote:
>> 01.10.2020 04:26, Nicolin Chen пишет:
>>> On Thu, Oct 01, 2020 at 12:56:46AM +0300, Dmitry Osipenko wrote:
 01.10.2020 00:32, Nicolin Chen пишет:
> On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote:
>> ...
 It looks to me like the only reason why you need this new global API is
 because PCI devices may not have a device tree node with a phandle to
 the IOMMU. However, SMMU support for PCI will only be enabled if the
 root complex has an iommus property, right? In that case, can't we
 simply do something like this:

if (dev_is_pci(dev))
np = find_host_bridge(dev)->of_node;
else
np = dev->of_node;

 ? I'm not sure exactly what find_host_bridge() is called, but I'm 
 pretty
 sure that exists.

 Once we have that we can still iterate over the iommus property and do
 not need to rely on this global variable.
>>>
>>> I agree that it'd work. But I was hoping to simplify the code
>>> here if it's possible. Looks like we have an argument on this
>>> so I will choose to go with your suggestion above for now.
>>
>> This patch removed more lines than were added. If this will be opposite
>> for the Thierry's suggestion, then it's probably not a great suggestion.
>
> Sorry, I don't quite understand this comments. Would you please
> elaborate what's this "it" being "not a great suggestion"?
>

 I meant that you should try to implement Thierry's solution, but if the
 end result will be worse than the current patch, then you shouldn't make
 a v4, but get back to this discussion in order to choose the best option
 and make everyone agree on it.
>>>
>>> I see. Thanks for the reply. And here is a sample implementation:
>>
>> That's what I supposed to happen :) The new variant adds code and
>> complexity, while old did the opposite. Hence the old variant is clearly
>> more attractive, IMO.
> 
> I personally am not a fan of adding a path for PCI device either,
> since PCI/IOMMU cores could have taken care of it while the same
> path can't be used for other buses.
> 
> If we can't come to an agreement on globalizing mc pointer, would
> it be possible to pass tegra_mc_driver through tegra_smmu_probe()
> so we can continue to use driver_find_device_by_fwnode() as v1?
> 
> v1: https://lkml.org/lkml/2020/9/26/68
> 

I think we already agreed that it will be good to have a common helper.
So far Thierry only objected that the implementation of the helper could
be improved.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

On Thu, Oct 01, 2020 at 05:06:19AM +0300, Dmitry Osipenko wrote:
> 01.10.2020 04:26, Nicolin Chen пишет:
> > On Thu, Oct 01, 2020 at 12:56:46AM +0300, Dmitry Osipenko wrote:
> >> 01.10.2020 00:32, Nicolin Chen пишет:
> >>> On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote:
>  ...
> >> It looks to me like the only reason why you need this new global API is
> >> because PCI devices may not have a device tree node with a phandle to
> >> the IOMMU. However, SMMU support for PCI will only be enabled if the
> >> root complex has an iommus property, right? In that case, can't we
> >> simply do something like this:
> >>
> >>if (dev_is_pci(dev))
> >>np = find_host_bridge(dev)->of_node;
> >>else
> >>np = dev->of_node;
> >>
> >> ? I'm not sure exactly what find_host_bridge() is called, but I'm 
> >> pretty
> >> sure that exists.
> >>
> >> Once we have that we can still iterate over the iommus property and do
> >> not need to rely on this global variable.
> >
> > I agree that it'd work. But I was hoping to simplify the code
> > here if it's possible. Looks like we have an argument on this
> > so I will choose to go with your suggestion above for now.
> 
>  This patch removed more lines than were added. If this will be opposite
>  for the Thierry's suggestion, then it's probably not a great suggestion.
> >>>
> >>> Sorry, I don't quite understand this comments. Would you please
> >>> elaborate what's this "it" being "not a great suggestion"?
> >>>
> >>
> >> I meant that you should try to implement Thierry's solution, but if the
> >> end result will be worse than the current patch, then you shouldn't make
> >> a v4, but get back to this discussion in order to choose the best option
> >> and make everyone agree on it.
> > 
> > I see. Thanks for the reply. And here is a sample implementation:
> 
> That's what I supposed to happen :) The new variant adds code and
> complexity, while old did the opposite. Hence the old variant is clearly
> more attractive, IMO.

I personally am not a fan of adding a path for PCI device either,
since PCI/IOMMU cores could have taken care of it while the same
path can't be used for other buses.

If we can't come to an agreement on globalizing mc pointer, would
it be possible to pass tegra_mc_driver through tegra_smmu_probe()
so we can continue to use driver_find_device_by_fwnode() as v1?

v1: https://lkml.org/lkml/2020/9/26/68
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

30.09.2020 19:47, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 07:25:41PM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 19:06, Thierry Reding пишет:
>>> On Wed, Sep 30, 2020 at 06:36:52PM +0300, Dmitry Osipenko wrote:
  I'...
>> +struct tegra_mc *mc = devm_tegra_get_memory_controller(dev);
>> +struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>
> It looks to me like the only reason why you need this new global API is
> because PCI devices may not have a device tree node with a phandle to
> the IOMMU. However, SMMU support for PCI will only be enabled if the
> root complex has an iommus property, right? In that case, can't we
> simply do something like this:
>
>   if (dev_is_pci(dev))
>   np = find_host_bridge(dev)->of_node;
>   else
>   np = dev->of_node;
>
> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
> sure that exists.
>
> Once we have that we can still iterate over the iommus property and do
> not need to rely on this global variable.

 This sounds more complicated than the current variant.

 Secondly, I'm already about to use the new tegra_get_memory_controller()
 API for all the T20/30/124/210 EMC and devfreq drivers.
>>>
>>> Why do we need it there? They seem to work fine without it right now.
>>
>> All the Tegra30/124/210 EMC drivers are already duplicating that MC
>> lookup code and only the recent T210 driver does it properly.
>>
>>> If it is required for new functionality, we can always make the dependent
>>> on a DT reference via phandle without breaking any existing code.
>>
>> That's correct, it will be also needed for the new functionality as
>> well, hence even more drivers will need to perform the MC lookup.
> 
> I don't have any issues with adding a helper if we need it from several
> different locations. But the helper should be working off of a given
> device and look up the device via the device tree node referenced by
> phandle. We already have those phandles in place for the EMC devices,
> and any other device that needs to interoperate with the MC should also
> get such a reference.
> 
>> I don't quite understand why you're asking for the phandle reference,
>> it's absolutely not needed for the MC lookup and won't work for the
> 
> We need that phandle in order to establish a link between the devices.
> Yes, you can probably do it without the phandle and just match by
> compatible string. But we don't do that for other types of devices
> either, right? For a display driver we reference the attached panel via
> phandle, but we could also just look it up via name or absolute path or
> some other heuristic. But a phandle is just a much more explicit way of
> linking the devices, so why not use it?

There are dozens variants of the panels and system could easily have
more than one panel, hence a direct lookup by phandle is a natural
choice for the panels.

While all Tegra SoCs have a single fixed MC in the system, and thus,
there is no real need to use phandle because we can't mix up MC with
anything else.

>> older DTs if DT change will be needed. Please give a detailed explanation.
> 
> New functionality doesn't have to work with older DTs.

This is fine in general, but I'm afraid that in this particular case we
will need to have a fall back anyways because otherwise it should break
the old functionality.

So I don't think that using phandle for the MC device finding is really
warrant.

Phandle is kinda more applicable for the cases where only the DT node
lookup is needed (not the lookup of the MC device driver), but even then
it is also not mandatory.

I hope you agree.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

01.10.2020 04:26, Nicolin Chen пишет:
> On Thu, Oct 01, 2020 at 12:56:46AM +0300, Dmitry Osipenko wrote:
>> 01.10.2020 00:32, Nicolin Chen пишет:
>>> On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote:
 ...
>> It looks to me like the only reason why you need this new global API is
>> because PCI devices may not have a device tree node with a phandle to
>> the IOMMU. However, SMMU support for PCI will only be enabled if the
>> root complex has an iommus property, right? In that case, can't we
>> simply do something like this:
>>
>>  if (dev_is_pci(dev))
>>  np = find_host_bridge(dev)->of_node;
>>  else
>>  np = dev->of_node;
>>
>> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
>> sure that exists.
>>
>> Once we have that we can still iterate over the iommus property and do
>> not need to rely on this global variable.
>
> I agree that it'd work. But I was hoping to simplify the code
> here if it's possible. Looks like we have an argument on this
> so I will choose to go with your suggestion above for now.

 This patch removed more lines than were added. If this will be opposite
 for the Thierry's suggestion, then it's probably not a great suggestion.
>>>
>>> Sorry, I don't quite understand this comments. Would you please
>>> elaborate what's this "it" being "not a great suggestion"?
>>>
>>
>> I meant that you should try to implement Thierry's solution, but if the
>> end result will be worse than the current patch, then you shouldn't make
>> a v4, but get back to this discussion in order to choose the best option
>> and make everyone agree on it.
> 
> I see. Thanks for the reply. And here is a sample implementation:

That's what I supposed to happen :) The new variant adds code and
complexity, while old did the opposite. Hence the old variant is clearly
more attractive, IMO.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

On Thu, Oct 01, 2020 at 12:56:46AM +0300, Dmitry Osipenko wrote:
> 01.10.2020 00:32, Nicolin Chen пишет:
> > On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote:
> >> ...
>  It looks to me like the only reason why you need this new global API is
>  because PCI devices may not have a device tree node with a phandle to
>  the IOMMU. However, SMMU support for PCI will only be enabled if the
>  root complex has an iommus property, right? In that case, can't we
>  simply do something like this:
> 
>   if (dev_is_pci(dev))
>   np = find_host_bridge(dev)->of_node;
>   else
>   np = dev->of_node;
> 
>  ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
>  sure that exists.
> 
>  Once we have that we can still iterate over the iommus property and do
>  not need to rely on this global variable.
> >>>
> >>> I agree that it'd work. But I was hoping to simplify the code
> >>> here if it's possible. Looks like we have an argument on this
> >>> so I will choose to go with your suggestion above for now.
> >>
> >> This patch removed more lines than were added. If this will be opposite
> >> for the Thierry's suggestion, then it's probably not a great suggestion.
> > 
> > Sorry, I don't quite understand this comments. Would you please
> > elaborate what's this "it" being "not a great suggestion"?
> > 
> 
> I meant that you should try to implement Thierry's solution, but if the
> end result will be worse than the current patch, then you shouldn't make
> a v4, but get back to this discussion in order to choose the best option
> and make everyone agree on it.

I see. Thanks for the reply. And here is a sample implementation:

@@ -814,12 +815,15 @@ static struct tegra_smmu *tegra_smmu_find(struct 
device_node *np)
 }
 
 static int tegra_smmu_configure(struct tegra_smmu *smmu, struct device *dev,
-   struct of_phandle_args *args)
+   struct of_phandle_args *args, struct 
fwnode_handle *fwnode)
 {
const struct iommu_ops *ops = smmu->iommu.ops;
int err;
 
-   err = iommu_fwspec_init(dev, >of_node->fwnode, ops);
+   if (!fwnode)
+   return -ENOENT;
+
+   err = iommu_fwspec_init(dev, fwnode, ops);
if (err < 0) {
dev_err(dev, "failed to initialize fwspec: %d\n", err);
return err;
@@ -835,6 +839,19 @@ static int tegra_smmu_configure(struct tegra_smmu *smmu, 
struct device *dev,
return 0;
 }
 
+static struct device_node *tegra_smmu_find_pci_np(struct pci_dev *pci_dev)
+{
+   struct pci_bus *bus = pci_dev->bus;
+   struct device *dev = >dev;
+
+   while (!of_property_read_bool(dev->of_node, "iommus") && bus->parent) {
+   dev = >parent->dev;
+   bus = bus->parent;
+   }
+
+   return dev->of_node;
+}
+
 static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
 {
struct device_node *np = dev->of_node;
@@ -843,11 +860,14 @@ static struct iommu_device 
*tegra_smmu_probe_device(struct device *dev)
unsigned int index = 0;
int err;
 
+   if (dev_is_pci(dev))
+   np = tegra_smmu_find_pci_np(to_pci_dev(dev));
+
while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
  ) == 0) {
smmu = tegra_smmu_find(args.np);
if (smmu) {
-   err = tegra_smmu_configure(smmu, dev, );
+   err = tegra_smmu_configure(smmu, dev, , 
>fwnode);
of_node_put(args.np);
 
if (err < 0)

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[PATCH] cma: decrease CMA_ALIGNMENT lower limit to 2

2020-09-30 Thread Paul Cercueil

On an embedded system with a tiny (1 MiB) CMA area for video memory, and
a simple enough video pipeline, we can decrease the CMA_ALIGNMENT by a
factor of 2 to avoid wasting memory, as all the allocations for video
buffers will be of the exact same size (dictated by the size of the
screen).

Signed-off-by: Paul Cercueil 
---
 kernel/dma/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/dma/Kconfig b/kernel/dma/Kconfig
index 847a9d1fa634..f15e782e19ca 100644
--- a/kernel/dma/Kconfig
+++ b/kernel/dma/Kconfig
@@ -162,7 +162,7 @@ endchoice
 
 config CMA_ALIGNMENT
int "Maximum PAGE_SIZE order of alignment for contiguous buffers"
-   range 4 12
+   range 2 12
default 8
help
  DMA mapping framework by default aligns all buffers to the smallest
-- 
2.28.0

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RESEND PATCH 1/4] iommu/vt-d: Disable SVM in the platform when IOMMUs have inconsistencies

Some IOMMU Capabilities must be consistent for Shared Virtual Memory (SVM).
Audit IOMMU Capability/Extended Capabilities and check if IOMMUs have
the consistent value for features as below. When the features are not
matched among IOMMUs, disable SVMs in the platform during DMAR
initialization. Audit IOMMUs again when a device is hot plugged.

Disable Shared Virtual Memory when below features are mistmatched:
  - First Level Translation Support (FLTS)
  - Process Address Space ID Support (PASID)
  - Extended Accessed Flag Support (EAFS)
  - Supervisor Support (SRS)
  - Execute Request Support (ERS)
  - Page Request Support (PRS)

Signed-off-by: Kyung Min Park 
---
 drivers/iommu/intel/Makefile |  2 +-
 drivers/iommu/intel/audit.c  | 95 
 drivers/iommu/intel/audit.h  | 29 +++
 drivers/iommu/intel/iommu.c  | 12 -
 4 files changed, 136 insertions(+), 2 deletions(-)
 create mode 100644 drivers/iommu/intel/audit.c
 create mode 100644 drivers/iommu/intel/audit.h

diff --git a/drivers/iommu/intel/Makefile b/drivers/iommu/intel/Makefile
index fb8e1e8c8029..02c26acb479f 100644
--- a/drivers/iommu/intel/Makefile
+++ b/drivers/iommu/intel/Makefile
@@ -1,6 +1,6 @@
 # SPDX-License-Identifier: GPL-2.0
 obj-$(CONFIG_DMAR_TABLE) += dmar.o
-obj-$(CONFIG_INTEL_IOMMU) += iommu.o pasid.o
+obj-$(CONFIG_INTEL_IOMMU) += iommu.o pasid.o audit.o
 obj-$(CONFIG_INTEL_IOMMU) += trace.o
 obj-$(CONFIG_INTEL_IOMMU_DEBUGFS) += debugfs.o
 obj-$(CONFIG_INTEL_IOMMU_SVM) += svm.o
diff --git a/drivers/iommu/intel/audit.c b/drivers/iommu/intel/audit.c
new file mode 100644
index ..2893170f5b6c
--- /dev/null
+++ b/drivers/iommu/intel/audit.c
@@ -0,0 +1,95 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * audit.c - audit iommu capabilities for boot time and hot plug
+ *
+ * Copyright (C) 2020 Intel Corporation
+ *
+ * Author: Kyung Min Park 
+ */
+
+#define pr_fmt(fmt)"DMAR: " fmt
+
+#include 
+#include "audit.h"
+
+static bool svm_sanity_check = true;
+static u64 intel_iommu_ecap_sanity = ~0ULL;
+
+static void set_cap_audit_svm_sanity(bool svm_sanity)
+{
+   svm_sanity_check = svm_sanity;
+}
+
+bool get_cap_audit_svm_sanity(void)
+{
+   return svm_sanity_check;
+}
+
+static inline void check_dmar_capabilities(struct intel_iommu *a,
+  struct intel_iommu *b)
+{
+   if (MINIMAL_SVM_ECAP & (a->ecap ^ b->ecap))
+   set_cap_audit_svm_sanity(false);
+}
+
+static int audit_iommu_capabilities_hotplug(struct intel_iommu *hot_iommu)
+{
+   bool mismatch = false;
+
+   if (!IS_ENABLED(CONFIG_INTEL_IOMMU_SVM))
+   goto out;
+
+   if (!get_cap_audit_svm_sanity() && (hot_iommu->flags & 
VTD_FLAG_SVM_CAPABLE)) {
+   pr_warn("Disable SVM in the IOMMU: SVM disabled at boot 
time.\n");
+   hot_iommu->flags = hot_iommu->flags & ~VTD_FLAG_SVM_CAPABLE;
+   } else if (get_cap_audit_svm_sanity() && (MINIMAL_SVM_ECAP &
+  (hot_iommu->ecap ^ intel_iommu_ecap_sanity))) {
+   pr_warn("Abort Hot Plug IOMMU: SVM inconsistent.\n");
+   mismatch = true;
+   }
+
+out:
+   if (mismatch)
+   return -EFAULT;
+
+   return 0;
+}
+
+static int audit_iommu_capabilities(void)
+{
+   struct dmar_drhd_unit *first_drhd, *drhd;
+   struct intel_iommu *iommu;
+   int ret = -EFAULT;
+
+   rcu_read_lock();
+   first_drhd = list_first_or_null_rcu(_drhd_units, typeof(*drhd),
+   list);
+   if (!first_drhd) {
+   ret = 0;
+   goto out;
+   }
+
+   for_each_active_iommu(iommu, drhd)
+   check_dmar_capabilities(first_drhd->iommu, iommu);
+
+   if (get_cap_audit_svm_sanity())
+   intel_iommu_ecap_sanity = (intel_iommu_ecap_sanity & 
~MINIMAL_SVM_ECAP) |
+  (first_drhd->iommu->ecap & 
MINIMAL_SVM_ECAP);
+
+   ret = 0;
+out:
+   rcu_read_unlock();
+   return ret;
+}
+
+int intel_iommu_audit_capabilities(enum cap_audit_type type, struct 
intel_iommu *iommu)
+{
+   switch (type) {
+   case CAP_AUDIT_STATIC_DMAR:
+   return audit_iommu_capabilities();
+   case CAP_AUDIT_HOTPLUG_DMAR:
+   return audit_iommu_capabilities_hotplug(iommu);
+   default:
+   return -EFAULT;
+   }
+}
diff --git a/drivers/iommu/intel/audit.h b/drivers/iommu/intel/audit.h
new file mode 100644
index ..887900d9517d
--- /dev/null
+++ b/drivers/iommu/intel/audit.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * audit.h - audit iommu capabilities header
+ *
+ * Copyright (C) 2020 Intel Corporation
+ *
+ * Author: Kyung Min Park 
+ */
+
+/*
+ * Extended Capability Register Mask
+ */
+#define ECAP_FLTS_MASK BIT(47)
+#define ECAP_PASID_MASKBIT(40)
+#define ECAP_EAFS_MASK BIT(34)
+#define ECAP_SRS_MASK

[RESEND PATCH 3/4] iommu/vt-d: Audit IOMMUs for Interrupt Remapping features

Audit IOMMU Capability/Extended Capabilities for Interrupt Remapping.
Check if the IOMMUs have the consistent value for the features as below.
When the features are not matched among IOMMUs, report out the IOMMU
features during irq remapping initialization. Audit IOMMUs again
when a device is hot plugged.

Report out features when below features are mismatched:
  - Posted Interrupts (PI)
  - Extended Interrupt Mode (EIM)

Signed-off-by: Kyung Min Park 
---
 drivers/iommu/intel/Makefile|  2 +-
 drivers/iommu/intel/audit.c | 39 -
 drivers/iommu/intel/audit.h |  4 +++
 drivers/iommu/intel/irq_remapping.c |  8 ++
 4 files changed, 46 insertions(+), 7 deletions(-)

diff --git a/drivers/iommu/intel/Makefile b/drivers/iommu/intel/Makefile
index 02c26acb479f..b5760c4abc54 100644
--- a/drivers/iommu/intel/Makefile
+++ b/drivers/iommu/intel/Makefile
@@ -4,4 +4,4 @@ obj-$(CONFIG_INTEL_IOMMU) += iommu.o pasid.o audit.o
 obj-$(CONFIG_INTEL_IOMMU) += trace.o
 obj-$(CONFIG_INTEL_IOMMU_DEBUGFS) += debugfs.o
 obj-$(CONFIG_INTEL_IOMMU_SVM) += svm.o
-obj-$(CONFIG_IRQ_REMAP) += irq_remapping.o
+obj-$(CONFIG_IRQ_REMAP) += irq_remapping.o audit.o
diff --git a/drivers/iommu/intel/audit.c b/drivers/iommu/intel/audit.c
index f783acabb402..e005bc61770a 100644
--- a/drivers/iommu/intel/audit.c
+++ b/drivers/iommu/intel/audit.c
@@ -27,6 +27,13 @@ bool get_cap_audit_svm_sanity(void)
return svm_sanity_check;
 }
 
+static inline void check_irq_capabilities(struct intel_iommu *a,
+ struct intel_iommu *b)
+{
+   CHECK_FEATURE_MISMATCH(a, b, cap, pi_support, CAP_PI_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, ecap, eim_support, ECAP_EIM_MASK);
+}
+
 static inline void check_dmar_capabilities(struct intel_iommu *a,
   struct intel_iommu *b)
 {
@@ -57,10 +64,17 @@ static inline void check_dmar_capabilities(struct 
intel_iommu *a,
CHECK_FEATURE_MISMATCH(a, b, ecap, coherent, ECAP_C_MASK);
 }
 
-static int audit_iommu_capabilities_hotplug(struct intel_iommu *hot_iommu)
+static int audit_iommu_capabilities_hotplug(struct intel_iommu *hot_iommu,
+   bool audit_irq)
 {
bool mismatch = false;
 
+   if (audit_irq) {
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, cap, pi_support, 
CAP_PI_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, ecap, eim_support, 
ECAP_EIM_MASK);
+   goto out;
+   }
+
CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, cap, 5lp_support, 
CAP_FL5LP_MASK);
CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, cap, fl1gp_support, 
CAP_FL1GP_MASK);
CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, cap, read_drain, CAP_RD_MASK);
@@ -103,7 +117,7 @@ static int audit_iommu_capabilities_hotplug(struct 
intel_iommu *hot_iommu)
return 0;
 }
 
-static int audit_iommu_capabilities(void)
+static int audit_iommu_capabilities(bool audit_irq)
 {
struct dmar_drhd_unit *first_drhd, *drhd;
struct intel_iommu *iommu;
@@ -117,8 +131,17 @@ static int audit_iommu_capabilities(void)
goto out;
}
 
-   for_each_active_iommu(iommu, drhd)
-   check_dmar_capabilities(first_drhd->iommu, iommu);
+   for_each_active_iommu(iommu, drhd) {
+   if (audit_irq)
+   check_irq_capabilities(first_drhd->iommu, iommu);
+   else
+   check_dmar_capabilities(first_drhd->iommu, iommu);
+   }
+
+   if (audit_irq) {
+   ret = 0;
+   goto out;
+   }
 
if (get_cap_audit_svm_sanity())
intel_iommu_ecap_sanity = (intel_iommu_ecap_sanity & 
~MINIMAL_SVM_ECAP) |
@@ -134,9 +157,13 @@ int intel_iommu_audit_capabilities(enum cap_audit_type 
type, struct intel_iommu
 {
switch (type) {
case CAP_AUDIT_STATIC_DMAR:
-   return audit_iommu_capabilities();
+   return audit_iommu_capabilities(false);
+   case CAP_AUDIT_STATIC_IRQR:
+   return audit_iommu_capabilities(true);
case CAP_AUDIT_HOTPLUG_DMAR:
-   return audit_iommu_capabilities_hotplug(iommu);
+   return audit_iommu_capabilities_hotplug(iommu, false);
+   case CAP_AUDIT_HOTPLUG_IRQR:
+   return audit_iommu_capabilities_hotplug(iommu, true);
default:
return -EFAULT;
}
diff --git a/drivers/iommu/intel/audit.h b/drivers/iommu/intel/audit.h
index e3a370405f82..6dfebe8e8fbe 100644
--- a/drivers/iommu/intel/audit.h
+++ b/drivers/iommu/intel/audit.h
@@ -11,6 +11,7 @@
  * Capability Register Mask
  */
 #define CAP_FL5LP_MASK BIT(60)
+#define CAP_PI_MASKBIT(59)
 #define CAP_FL1GP_MASK BIT(56)
 #define CAP_RD_MASKBIT(55)
 #define CAP_WD_MASKBIT(54)
@@ -38,6 +39,7 @@
 #define ECAP_NEST_MASK BIT(26)
 #define ECAP_SC_MASK

[RESEND PATCH 4/4] iommu/vt-d: Scale capability to the lowest supported between the IOMMUs

Audit IOMMU Capability/Extended Capabilities and check if the IOMMUs
have the consistent value for features as below. Find common denominator
for the features and set to the lowest supported value for each IOMMU.

Abort hot plug when the hot plugged IOMMU does not meet the aforementioned
common denominator.

Set capability to the lowest supported when below features are mismatched:
  - Maximum Address Mask Value (MAMV)
  - Second Level Large Page Support (SLLPS)
  - Maximum Guest Address Width (MGAW)
  - Supported Adjusted Guest Address Width (SAGAW)
  - Number of Domains supported (ND)
  - Pasid Size Supported (PSS)

Signed-off-by: Kyung Min Park 
---
 drivers/iommu/intel/audit.c | 23 +++
 drivers/iommu/intel/audit.h | 27 +++
 include/linux/intel-iommu.h |  1 +
 3 files changed, 51 insertions(+)

diff --git a/drivers/iommu/intel/audit.c b/drivers/iommu/intel/audit.c
index e005bc61770a..7e12c963c2b7 100644
--- a/drivers/iommu/intel/audit.c
+++ b/drivers/iommu/intel/audit.c
@@ -40,6 +40,13 @@ static inline void check_dmar_capabilities(struct 
intel_iommu *a,
if (MINIMAL_SVM_ECAP & (a->ecap ^ b->ecap))
set_cap_audit_svm_sanity(false);
 
+   MINIMAL_FEATURE_IOMMU(b, cap, CAP_MAMV_MASK);
+   MINIMAL_FEATURE_IOMMU(b, cap, CAP_SLLPS_MASK);
+   MINIMAL_FEATURE_IOMMU(b, cap, CAP_MGAW_MASK);
+   MINIMAL_FEATURE_IOMMU(b, cap, CAP_SAGAW_MASK);
+   MINIMAL_FEATURE_IOMMU(b, cap, CAP_NDOMS_MASK);
+   MINIMAL_FEATURE_IOMMU(b, ecap, ECAP_PSS_MASK);
+
CHECK_FEATURE_MISMATCH(a, b, cap, 5lp_support, CAP_FL5LP_MASK);
CHECK_FEATURE_MISMATCH(a, b, cap, fl1gp_support, CAP_FL1GP_MASK);
CHECK_FEATURE_MISMATCH(a, b, cap, read_drain, CAP_RD_MASK);
@@ -98,6 +105,14 @@ static int audit_iommu_capabilities_hotplug(struct 
intel_iommu *hot_iommu,
CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, ecap, qis, ECAP_QI_MASK);
CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, ecap, coherent, ECAP_C_MASK);
 
+   /* Abort hot plug if the hot plug iommu feature is smaller than global 
*/
+   MINIMAL_FEATURE_HOTPLUG(hot_iommu, cap, mamv, CAP_MAMV_MASK, mismatch);
+   MINIMAL_FEATURE_HOTPLUG(hot_iommu, cap, super_page_val, CAP_SLLPS_MASK, 
mismatch);
+   MINIMAL_FEATURE_HOTPLUG(hot_iommu, cap, mgaw, CAP_MGAW_MASK, mismatch);
+   MINIMAL_FEATURE_HOTPLUG(hot_iommu, cap, sagaw, CAP_SAGAW_MASK, 
mismatch);
+   MINIMAL_FEATURE_HOTPLUG(hot_iommu, cap, ndoms, CAP_NDOMS_MASK, 
mismatch);
+   MINIMAL_FEATURE_HOTPLUG(hot_iommu, ecap, pss, ECAP_PSS_MASK, mismatch);
+
if (!IS_ENABLED(CONFIG_INTEL_IOMMU_SVM))
goto out;
 
@@ -147,6 +162,14 @@ static int audit_iommu_capabilities(bool audit_irq)
intel_iommu_ecap_sanity = (intel_iommu_ecap_sanity & 
~MINIMAL_SVM_ECAP) |
   (first_drhd->iommu->ecap & 
MINIMAL_SVM_ECAP);
 
+   /* scale capability to the lowest supported value */
+   for_each_active_iommu(iommu, drhd) {
+   iommu->cap = (intel_iommu_cap_sanity & MINIMAL_FEATURE_CAP) |
+(~MINIMAL_FEATURE_CAP & iommu->cap);
+   iommu->ecap = (intel_iommu_ecap_sanity & ECAP_PSS_MASK) |
+ (~ECAP_PSS_MASK & iommu->ecap);
+   }
+
ret = 0;
 out:
rcu_read_unlock();
diff --git a/drivers/iommu/intel/audit.h b/drivers/iommu/intel/audit.h
index 6dfebe8e8fbe..a293e71ce9ab 100644
--- a/drivers/iommu/intel/audit.h
+++ b/drivers/iommu/intel/audit.h
@@ -13,9 +13,14 @@
 #define CAP_FL5LP_MASK BIT(60)
 #define CAP_PI_MASKBIT(59)
 #define CAP_FL1GP_MASK BIT(56)
+#define CAP_MAMV_MASK  GENMASK_ULL(53, 48)
 #define CAP_RD_MASKBIT(55)
 #define CAP_WD_MASKBIT(54)
 #define CAP_PSI_MASK   BIT(39)
+#define CAP_SLLPS_MASK GENMASK_ULL(37, 34)
+#define CAP_MGAW_MASK  GENMASK_ULL(21, 16)
+#define CAP_SAGAW_MASK GENMASK_ULL(12, 8)
+#define CAP_NDOMS_MASK GENMASK_ULL(2, 0)
 #define CAP_CM_MASKBIT(7)
 #define CAP_PHMR_MASK  BIT(6)
 #define CAP_PLMR_MASK  BIT(5)
@@ -32,6 +37,7 @@
 #define ECAP_PDS_MASK  BIT(42)
 #define ECAP_DIT_MASK  BIT(41)
 #define ECAP_PASID_MASKBIT(40)
+#define ECAP_PSS_MASK  GENMASK_ULL(39, 35)
 #define ECAP_EAFS_MASK BIT(34)
 #define ECAP_SRS_MASK  BIT(31)
 #define ECAP_ERS_MASK  BIT(30)
@@ -47,6 +53,9 @@
 #define MINIMAL_SVM_ECAP (ECAP_FLTS_MASK | ECAP_PASID_MASK | ECAP_EAFS_MASK | \
  ECAP_SRS_MASK | ECAP_ERS_MASK | ECAP_PRS_MASK)
 
+#define MINIMAL_FEATURE_CAP (CAP_MAMV_MASK | CAP_SLLPS_MASK | CAP_MGAW_MASK | \
+CAP_SAGAW_MASK | CAP_NDOMS_MASK)
+
 #define DO_CHECK_FEATURE_MISMATCH(a, b, cap, feature, MASK) \
 do { \
if (cap##_##feature(a) != cap##_##feature(b)) { \
@@ -65,6 +74,24 @@ do { \

[RESEND PATCH 0/4] Audit Capability and Extended Capability among IOMMUs

Modern platforms have more than one IOMMU. Each IOMMU has its own
feature set. Some of these features must be consistent among IOMMUs.
Otherwise, these differences can lead to improper behavior in the system.
On the other hand, for some features, each IOMMU can have different
capacity values. So, different actions are required to deal with the
inconsistencies depending on the IOMMU features.

Currently, some inconsistencies are ignored by the IOMMU driver.
This patchset checks IOMMU capabilities and extended capabilities
centralizedly during boot and take different actions according to
the impacts caused by the mismatches.

For example:
 1. Disable Shared Virtual Memory.
 2. Use common capacity values (normally the lowest capacity value) for
all IOMMUs.
 3. Report feature mismatches.

Detailed information on the IOMMU Capability / Extended Capability can
be found in Intel VT-d Specification.

Link: 
https://software.intel.com/sites/default/files/managed/c5/15/vt-directed-io-spec.pdf

Kyung Min Park (4):
  iommu/vt-d: Disable SVM in the platform when IOMMUs have
inconsistencies
  iommu/vt-d: Report out when IOMMU features have inconsistencies
  iommu/vt-d: Audit IOMMUs for Interrupt Remapping features
  iommu/vt-d: Scale capability to the lowest supported between the
IOMMUs

 drivers/iommu/intel/Makefile|   4 +-
 drivers/iommu/intel/audit.c | 193 
 drivers/iommu/intel/audit.h | 103 +++
 drivers/iommu/intel/iommu.c |  12 +-
 drivers/iommu/intel/irq_remapping.c |   8 ++
 include/linux/intel-iommu.h |   3 +
 6 files changed, 320 insertions(+), 3 deletions(-)
 create mode 100644 drivers/iommu/intel/audit.c
 create mode 100644 drivers/iommu/intel/audit.h

-- 
2.17.1

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

[RESEND PATCH 2/4] iommu/vt-d: Report out when IOMMU features have inconsistencies

IOMMU features as below can have incompatibilities among IOMMUs.
Audit IOMMU Capability/Extended Capability and check if the IOMMUs have
the consistent value for features as below. Report out when features as
below have incompatibility among IOMMUs.

Report out features when below features are mismatched:
  - First Level 5 Level Paging Support (FL5LP)
  - First Level 1 GByte Page Support (FL1GP)
  - Read Draining (DRD)
  - Write Draining (DWD)
  - Page Selective Invalidation (PSI)
  - Caching Mode (CM)
  - Protected High/Low-Memory Region (PHMR/PLMR)
  - Scalable Mode Page Walk Coherency (SMPWC)
  - First Level Translation Support (FLTS)
  - Second Level Translation Support (SLTS)
  - Second Level Accessed/Dirty Support (SLADS)
  - Virtual Command Support (VCS)
  - Scalable Mode Translation Support (SMTS)
  - Device TLB Invalidation Throttle (DIT)
  - Page Drain Support (PDS)
  - Nested Translation Support (NEST)
  - Snoop Control (SC)
  - Pass Through (PT)
  - Device TLB Support (DT)
  - Queued Invalidation (QI)
  - Page walk Coherency (C)

Signed-off-by: Kyung Min Park 
---
 drivers/iommu/intel/audit.c | 48 +
 drivers/iommu/intel/audit.h | 43 +
 include/linux/intel-iommu.h |  2 ++
 3 files changed, 93 insertions(+)

diff --git a/drivers/iommu/intel/audit.c b/drivers/iommu/intel/audit.c
index 2893170f5b6c..f783acabb402 100644
--- a/drivers/iommu/intel/audit.c
+++ b/drivers/iommu/intel/audit.c
@@ -13,6 +13,8 @@
 #include "audit.h"
 
 static bool svm_sanity_check = true;
+/* global variables that hold feature consistency and minimum features */
+static u64 intel_iommu_cap_sanity = ~0ULL;
 static u64 intel_iommu_ecap_sanity = ~0ULL;
 
 static void set_cap_audit_svm_sanity(bool svm_sanity)
@@ -30,12 +32,58 @@ static inline void check_dmar_capabilities(struct 
intel_iommu *a,
 {
if (MINIMAL_SVM_ECAP & (a->ecap ^ b->ecap))
set_cap_audit_svm_sanity(false);
+
+   CHECK_FEATURE_MISMATCH(a, b, cap, 5lp_support, CAP_FL5LP_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, cap, fl1gp_support, CAP_FL1GP_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, cap, read_drain, CAP_RD_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, cap, write_drain, CAP_WD_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, cap, pgsel_inv, CAP_PSI_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, cap, caching_mode, CAP_CM_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, cap, phmr, CAP_PHMR_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, cap, plmr, CAP_PLMR_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, ecap, smpwc, ECAP_SMPWC_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, ecap, flts, ECAP_FLTS_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, ecap, slts, ECAP_SLTS_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, ecap, slads, ECAP_SLADS_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, ecap, vcs, ECAP_VCS_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, ecap, smts, ECAP_SMTS_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, ecap, pds, ECAP_PDS_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, ecap, dit, ECAP_DIT_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, ecap, nest, ECAP_NEST_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, ecap, sc_support, ECAP_SC_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, ecap, pass_through, ECAP_PT_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, ecap, dev_iotlb_support, ECAP_DT_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, ecap, qis, ECAP_QI_MASK);
+   CHECK_FEATURE_MISMATCH(a, b, ecap, coherent, ECAP_C_MASK);
 }
 
 static int audit_iommu_capabilities_hotplug(struct intel_iommu *hot_iommu)
 {
bool mismatch = false;
 
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, cap, 5lp_support, 
CAP_FL5LP_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, cap, fl1gp_support, 
CAP_FL1GP_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, cap, read_drain, CAP_RD_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, cap, write_drain, 
CAP_WD_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, cap, pgsel_inv, CAP_PSI_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, cap, caching_mode, 
CAP_CM_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, cap, phmr, CAP_PHMR_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, cap, plmr, CAP_PLMR_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, ecap, smpwc, ECAP_SMPWC_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, ecap, flts, ECAP_FLTS_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, ecap, slts, ECAP_SLTS_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, ecap, slads, ECAP_SLADS_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, ecap, vcs, ECAP_VCS_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, ecap, smts, ECAP_SMTS_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, ecap, pds, ECAP_PDS_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, ecap, dit, ECAP_DIT_MASK);
+   CHECK_FEATURE_MISMATCH_HOTPLUG(hot_iommu, ecap, nest, ECAP_NEST_MASK);
+

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

01.10.2020 00:32, Nicolin Chen пишет:
> On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote:
>> ...
 It looks to me like the only reason why you need this new global API is
 because PCI devices may not have a device tree node with a phandle to
 the IOMMU. However, SMMU support for PCI will only be enabled if the
 root complex has an iommus property, right? In that case, can't we
 simply do something like this:

if (dev_is_pci(dev))
np = find_host_bridge(dev)->of_node;
else
np = dev->of_node;

 ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
 sure that exists.

 Once we have that we can still iterate over the iommus property and do
 not need to rely on this global variable.
>>>
>>> I agree that it'd work. But I was hoping to simplify the code
>>> here if it's possible. Looks like we have an argument on this
>>> so I will choose to go with your suggestion above for now.
>>
>> This patch removed more lines than were added. If this will be opposite
>> for the Thierry's suggestion, then it's probably not a great suggestion.
> 
> Sorry, I don't quite understand this comments. Would you please
> elaborate what's this "it" being "not a great suggestion"?
> 

I meant that you should try to implement Thierry's solution, but if the
end result will be worse than the current patch, then you shouldn't make
a v4, but get back to this discussion in order to choose the best option
and make everyone agree on it.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

On Thu, Oct 01, 2020 at 12:24:25AM +0300, Dmitry Osipenko wrote:
> ...
> >> It looks to me like the only reason why you need this new global API is
> >> because PCI devices may not have a device tree node with a phandle to
> >> the IOMMU. However, SMMU support for PCI will only be enabled if the
> >> root complex has an iommus property, right? In that case, can't we
> >> simply do something like this:
> >>
> >>if (dev_is_pci(dev))
> >>np = find_host_bridge(dev)->of_node;
> >>else
> >>np = dev->of_node;
> >>
> >> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
> >> sure that exists.
> >>
> >> Once we have that we can still iterate over the iommus property and do
> >> not need to rely on this global variable.
> > 
> > I agree that it'd work. But I was hoping to simplify the code
> > here if it's possible. Looks like we have an argument on this
> > so I will choose to go with your suggestion above for now.
> 
> This patch removed more lines than were added. If this will be opposite
> for the Thierry's suggestion, then it's probably not a great suggestion.

Sorry, I don't quite understand this comments. Would you please
elaborate what's this "it" being "not a great suggestion"?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

...
>> It looks to me like the only reason why you need this new global API is
>> because PCI devices may not have a device tree node with a phandle to
>> the IOMMU. However, SMMU support for PCI will only be enabled if the
>> root complex has an iommus property, right? In that case, can't we
>> simply do something like this:
>>
>>  if (dev_is_pci(dev))
>>  np = find_host_bridge(dev)->of_node;
>>  else
>>  np = dev->of_node;
>>
>> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
>> sure that exists.
>>
>> Once we have that we can still iterate over the iommus property and do
>> not need to rely on this global variable.
> 
> I agree that it'd work. But I was hoping to simplify the code
> here if it's possible. Looks like we have an argument on this
> so I will choose to go with your suggestion above for now.

This patch removed more lines than were added. If this will be opposite
for the Thierry's suggestion, then it's probably not a great suggestion.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

On Wed, Sep 30, 2020 at 06:09:43PM +0300, Dmitry Osipenko wrote:
> ...
> >  static int tegra_smmu_attach_dev(struct iommu_domain *domain,
> >  struct device *dev)
> >  {
> > +   struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> > struct tegra_smmu *smmu = dev_iommu_priv_get(dev);
> > struct tegra_smmu_as *as = to_smmu_as(domain);
> > -   struct device_node *np = dev->of_node;
> > -   struct of_phandle_args args;
> > unsigned int index = 0;
> > int err = 0;
> >  
> > -   while (!of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> > -  )) {
> > -   unsigned int swgroup = args.args[0];
> > -
> > -   if (args.np != smmu->dev->of_node) {
> > -   of_node_put(args.np);
> > -   continue;
> > -   }
> > -
> > -   of_node_put(args.np);
> > +   if (!fwspec || fwspec->ops != _smmu_ops)
> > +   return -ENOENT;
> 
> In previous reply you said that these fwspec checks are borrowed from
> the arm-smmu driver, but I'm now looking at what other drivers do and I
> don't see them having this check.
> 
> Hence I'm objecting the need to have this check here. You either should
> give a rational to having the check or it should be removed.
> 
> Please never blindly copy foreign code, you should always double-check it.

I will give a test and remove it upon positive result.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

On Wed, Sep 30, 2020 at 05:31:31PM +0200, Thierry Reding wrote:
> On Wed, Sep 30, 2020 at 01:42:57AM -0700, Nicolin Chen wrote:
> > Previously the driver relies on bus_set_iommu() in .probe() to call
> > in .probe_device() function so each client can poll iommus property
> > in DTB to configure fwspec via tegra_smmu_configure(). According to
> > the comments in .probe(), this is a bit of a hack. And this doesn't
> > work for a client that doesn't exist in DTB, PCI device for example.
> > 
> > Actually when a device/client gets probed, the of_iommu_configure()
> > will call in .probe_device() function again, with a prepared fwspec
> > from of_iommu_configure() that reads the SWGROUP id in DTB as we do
> > in tegra-smmu driver.
> > 
> > Additionally, as a new helper devm_tegra_get_memory_controller() is
> > introduced, there's no need to poll the iommus property in order to
> > get mc->smmu pointers or SWGROUP id.
> > 
> > This patch reworks .probe_device() and .attach_dev() by doing:
> > 1) Using fwspec to get swgroup id in .attach_dev/.dettach_dev()
> > 2) Removing DT polling code, tegra_smmu_find/tegra_smmu_configure()
> > 3) Calling devm_tegra_get_memory_controller() in .probe_device()
> > 4) Also dropping the hack in .probe() that's no longer needed.
> > 
> > Signed-off-by: Nicolin Chen 
[...]
> >  static struct iommu_device *tegra_smmu_probe_device(struct device *dev)
> >  {
> > -   struct device_node *np = dev->of_node;
> > -   struct tegra_smmu *smmu = NULL;
> > -   struct of_phandle_args args;
> > -   unsigned int index = 0;
> > -   int err;
> > -
> > -   while (of_parse_phandle_with_args(np, "iommus", "#iommu-cells", index,
> > - ) == 0) {
> > -   smmu = tegra_smmu_find(args.np);
> > -   if (smmu) {
> > -   err = tegra_smmu_configure(smmu, dev, );
> > -   of_node_put(args.np);
> > -
> > -   if (err < 0)
> > -   return ERR_PTR(err);
> > -
> > -   /*
> > -* Only a single IOMMU master interface is currently
> > -* supported by the Linux kernel, so abort after the
> > -* first match.
> > -*/
> > -   dev_iommu_priv_set(dev, smmu);
> > -
> > -   break;
> > -   }
> > +   struct tegra_mc *mc = devm_tegra_get_memory_controller(dev);
> > +   struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> 
> It looks to me like the only reason why you need this new global API is
> because PCI devices may not have a device tree node with a phandle to
> the IOMMU. However, SMMU support for PCI will only be enabled if the
> root complex has an iommus property, right? In that case, can't we
> simply do something like this:
> 
>   if (dev_is_pci(dev))
>   np = find_host_bridge(dev)->of_node;
>   else
>   np = dev->of_node;
> 
> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
> sure that exists.
> 
> Once we have that we can still iterate over the iommus property and do
> not need to rely on this global variable.

I agree that it'd work. But I was hoping to simplify the code
here if it's possible. Looks like we have an argument on this
so I will choose to go with your suggestion above for now.

> > -   of_node_put(args.np);
> > -   index++;
> > -   }
> > +   /* An invalid mc pointer means mc and smmu drivers are not ready */
> > +   if (IS_ERR(mc))
> > +   return ERR_PTR(-EPROBE_DEFER);
> >  
> > -   if (!smmu)
> > +   /*
> > +* IOMMU core allows -ENODEV return to carry on. So bypass any call
> > +* from bus_set_iommu() during tegra_smmu_probe(), as a device will
> > +* call in again via of_iommu_configure when fwspec is prepared.
> > +*/
> > +   if (!mc->smmu || !fwspec || fwspec->ops != _smmu_ops)
> > return ERR_PTR(-ENODEV);
> >  
> > -   return >iommu;
> > +   dev_iommu_priv_set(dev, mc->smmu);
> > +
> > +   return >smmu->iommu;
> >  }
> >  
> >  static void tegra_smmu_release_device(struct device *dev)
> > @@ -1089,16 +1027,6 @@ struct tegra_smmu *tegra_smmu_probe(struct device 
> > *dev,
> > if (!smmu)
> > return ERR_PTR(-ENOMEM);
> >  
> > -   /*
> > -* This is a bit of a hack. Ideally we'd want to simply return this
> > -* value. However the IOMMU registration process will attempt to add
> > -* all devices to the IOMMU when bus_set_iommu() is called. In order
> > -* not to rely on global variables to track the IOMMU instance, we
> > -* set it here so that it can be looked up from the .probe_device()
> > -* callback via the IOMMU device's .drvdata field.
> > -*/
> > -   mc->smmu = smmu;
> 
> I don't think this is going to work. I distinctly remember putting this
> here because we needed access to this before ->probe_device() had been
> called for any of the devices.

Do you remember which exact part of code

Re: [PATCH v3 3/3] iommu/tegra-smmu: Add PCI support

On Wed, Sep 30, 2020 at 05:53:20PM +0300, Dmitry Osipenko wrote:
> ...
> > +#ifdef CONFIG_PCI
> > +   if (!iommu_present(_bus_type)) {
> 
> 
> In the previous reply you said that you're borrowing this check from the
> arm-smmu driver, but arm-smmu also has a similar check for
> platform_bus_type, while Tegra SMMU driver doesn't have it. Hence I'm
> objecting the necessity of having this check.
> 
> Please give a rationale for having this check in the code. And please
> note that cargo cult isn't a rationale to me.

Okay, okayI will remove it upon testing.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [patch V2 24/46] PCI: vmd: Mark VMD irqdomain with DOMAIN_BUS_VMD_MSI

2020-09-30 Thread Jason Gunthorpe

On Wed, Sep 30, 2020 at 01:08:27PM +, Derrick, Jonathan wrote:
> +Megha
> 
> On Wed, 2020-09-30 at 09:57 -0300, Jason Gunthorpe wrote:
> > On Wed, Sep 30, 2020 at 12:45:30PM +, Derrick, Jonathan wrote:
> > > Hi Jason
> > > 
> > > On Mon, 2020-08-31 at 11:39 -0300, Jason Gunthorpe wrote:
> > > > On Wed, Aug 26, 2020 at 01:16:52PM +0200, Thomas Gleixner wrote:
> > > > > From: Thomas Gleixner 
> > > > > 
> > > > > Devices on the VMD bus use their own MSI irq domain, but it is not
> > > > > distinguishable from regular PCI/MSI irq domains. This is required
> > > > > to exclude VMD devices from getting the irq domain pointer set by
> > > > > interrupt remapping.
> > > > > 
> > > > > Override the default bus token.
> > > > > 
> > > > > Signed-off-by: Thomas Gleixner 
> > > > > Acked-by: Bjorn Helgaas 
> > > > >  drivers/pci/controller/vmd.c |6 ++
> > > > >  1 file changed, 6 insertions(+)
> > > > > 
> > > > > +++ b/drivers/pci/controller/vmd.c
> > > > > @@ -579,6 +579,12 @@ static int vmd_enable_domain(struct vmd_
> > > > >   return -ENODEV;
> > > > >   }
> > > > >  
> > > > > + /*
> > > > > +  * Override the irq domain bus token so the domain can be 
> > > > > distinguished
> > > > > +  * from a regular PCI/MSI domain.
> > > > > +  */
> > > > > + irq_domain_update_bus_token(vmd->irq_domain, 
> > > > > DOMAIN_BUS_VMD_MSI);
> > > > > +
> > > > 
> > > > Having the non-transparent-bridge hold a MSI table and
> > > > multiplex/de-multiplex IRQs looks like another good use case for
> > > > something close to pci_subdevice_msi_create_irq_domain()?
> > > > 
> > > > If each device could have its own internal MSI-X table programmed
> > > > properly things would work alot better. Disable capture/remap of the
> > > > MSI range in the NTB.
> > > We can disable the capture and remap in newer devices so we don't even
> > > need the irq domain.
> > 
> > You'd still need an irq domain, it just comes from
> > pci_subdevice_msi_create_irq_domain() instead of internal to this
> > driver.
> I have this set which disables remapping and bypasses the creation of
> the IRQ domain:
> https://patchwork.ozlabs.org/project/linux-pci/list/?series=192936

After Thomas's series VMD needs to supply a hierarchical IRQ domain to
get the correct PCI originator. This instead of the x86 patch in that
series. I think that domain should be created by
pci_subdevice_msi_create_irq_domain(), at least I'd start there.

Jason
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [patch V2 00/46] x86, PCI, XEN, genirq ...: Prepare for device MSI

2020-09-30 Thread Thomas Gleixner

Megha,

On Wed, Sep 30 2020 at 10:25, Megha Dey wrote:
> On 9/30/2020 8:20 AM, Thomas Gleixner wrote:
 Your IMS patches? Why do you need something special again?
>
> By IMS patches, I meant your IMS driver patch that was updated (as it 
> was untested, it had some compile errors and we removed the IMS_QUEUE
> parts) :

Ok.

> The whole patchset can be found here:
>
> https://lore.kernel.org/lkml/f4a085f1-f6de-2539-12fe-c7308d243...@intel.com/
>
> It would be great if you could review the IMS patches :)

It somehow slipped through the cracks. I'll have a look.

> We were hoping to get IMS in the 5.10 merge window :)

Hope dies last, right?

>>> We might be able to put together a mockup just to prove it
>> If that makes Megha's stuff going that would of course be appreciated,
>> but we can defer the IMS_QUEUE part for later. It's orthogonal to the
>> IMS_ARRAY stuff.
>
> In our patch series, we have removed the IMS_QUEUE stuff and retained 
> only the IMS_ARRAY parts > as that was sufficient for us.

That works. We can add that back when Jason has his puzzle pieces
sorted.

Thanks,

tglx
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()

...
>> I don't think that the MC driver will stay built-in forever, although
>> its modularization is complicated right now. Hence something shall keep
>> the reference to the MC device resources while they are in use and this
>> patch takes care of doing that.
> 
> It looks to me like all the other places where we get a reference to the
> MC also keep a reference to the device. That's obviously not going to be
> enough once the code is turned into a module. At that point we need to
> make sure to also grab a reference to the module. But that's orthogonal
> to this discussion.
> 
>> Secondly, the Nicolin's patch doesn't pretend on anything, but rather
> 
> Yes, the patch does pretend to "look up" the memory controller device,
> but in reality it will always return a singleton object, which can just
> as easily be achieved by using a global variable.
> 
>> brings the already existing duplicated code to a single common place.
> 
> Where exactly is that duplicated code? The only places I see where we
> get a reference to the memory controller are from the EMC drivers and
> they properly look up the MC via the nvidia,memory-controller device
> tree property.

Yours observation is correct, all the drivers *do the lookup*. My point
is that the nvidia,memory-controller usage isn't strictly necessary.

The tegra20-devfreq driver doesn't use the phandle lookup because
Tegra20 DTs don't have it, instead it uses the compatible lookup. Hence
this patch doesn't really change the already existing behaviour for the
drivers. The phandle usage is optional.

This patch adds a common API that is usable by *all* the already
existing drivers, starting from the Tegra20 drivers.

> But that's not what this new helper does. This code will use the OF
> lookup table to find any match and then returns that, completely
> ignoring any links established by the device tree.

As I already said in the other reply, it should be fine to add lookup by
the phandle and then fall back to the compatible matching. On the other
hand, this is not strictly necessary because we always have only a
single MC device at a time.

Please note that I don't have any objections to improving this patch. In
the end either way will work, so we just need to choose the best option.
I was merely explaining the rationale behind this patch and not trying
to defend it.

Yours suggestion about using static mc variable is also good to me since
currently MC driver is built-in and at least that won't be a
globally-visible kernel variable, which doesn't feel great to have.

I think we can follow approach of the static mc variable for the starter
and improve it once there will be a real need. This should be the
simplest option right now.

But again, we may slightly future-proof the API by adding the
resource-managed variant. Either way will be good, IMO :) Currently I
don't have a strong preference.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [patch V2 00/46] x86, PCI, XEN, genirq ...: Prepare for device MSI

2020-09-30 Thread Dey, Megha

Hi Thomas/Jason,

On 9/30/2020 8:20 AM, Thomas Gleixner wrote:

On Wed, Sep 30 2020 at 08:43, Jason Gunthorpe wrote:

On Wed, Sep 30, 2020 at 08:41:48AM +0200, Thomas Gleixner wrote:

On Tue, Sep 29 2020 at 16:03, Megha Dey wrote:

On 8/26/2020 4:16 AM, Thomas Gleixner wrote:

#9 is obviously just for the folks interested in IMS

I see that the tip tree (as of 9/29) has most of these patches but
notice that the DEV_MSI related patches

haven't made it. I have tested the tip tree(x86/irq branch) with your
DEV_MSI infra patches and our IMS patches with the IDXD driver and was

Your IMS patches? Why do you need something special again?

By IMS patches, I meant your IMS driver patch that was updated (as it
was untested, it had some compile

errors and we removed the IMS_QUEUE parts) :

https://lore.kernel.org/lkml/160021246221.67751.16280230469654363209.st...@djiang5-desk3.ch.intel.com/

and some iommu related changes required by IMS.

https://lore.kernel.org/lkml/160021246905.67751.1674517279122764758.st...@djiang5-desk3.ch.intel.com/

The whole patchset can be found here:

https://lore.kernel.org/lkml/f4a085f1-f6de-2539-12fe-c7308d243...@intel.com/

It would be great if you could review the IMS patches :)

wondering if we should push out those patches as part of our patchset?

As I don't have any hardware to test that, I was waiting for you and
Jason to confirm that this actually works for the two different IMS
implementations.

How urgently do you need this? The code looked good from what I
understood. It will be a while before we have all the parts to send an
actual patch though.

I personally do not need it at all :) Megha might have different
thoughts...

I have tested these patches and it works fine (I had to add a couple of
EXPORT_SYMBOLS).

We were hoping to get IMS in the 5.10 merge window :)

We might be able to put together a mockup just to prove it

If that makes Megha's stuff going that would of course be appreciated,
but we can defer the IMS_QUEUE part for later. It's orthogonal to the
IMS_ARRAY stuff.

In our patch series, we have removed the IMS_QUEUE stuff and retained
only the IMS_ARRAY parts

as that was sufficient for us.

Thanks,

tglx

___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

On Wed, Sep 30, 2020 at 07:25:41PM +0300, Dmitry Osipenko wrote:
> 30.09.2020 19:06, Thierry Reding пишет:
> > On Wed, Sep 30, 2020 at 06:36:52PM +0300, Dmitry Osipenko wrote:
> >>  I'...
>  +struct tegra_mc *mc = devm_tegra_get_memory_controller(dev);
>  +struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
> >>>
> >>> It looks to me like the only reason why you need this new global API is
> >>> because PCI devices may not have a device tree node with a phandle to
> >>> the IOMMU. However, SMMU support for PCI will only be enabled if the
> >>> root complex has an iommus property, right? In that case, can't we
> >>> simply do something like this:
> >>>
> >>>   if (dev_is_pci(dev))
> >>>   np = find_host_bridge(dev)->of_node;
> >>>   else
> >>>   np = dev->of_node;
> >>>
> >>> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
> >>> sure that exists.
> >>>
> >>> Once we have that we can still iterate over the iommus property and do
> >>> not need to rely on this global variable.
> >>
> >> This sounds more complicated than the current variant.
> >>
> >> Secondly, I'm already about to use the new tegra_get_memory_controller()
> >> API for all the T20/30/124/210 EMC and devfreq drivers.
> > 
> > Why do we need it there? They seem to work fine without it right now.
> 
> All the Tegra30/124/210 EMC drivers are already duplicating that MC
> lookup code and only the recent T210 driver does it properly.
> 
> > If it is required for new functionality, we can always make the dependent
> > on a DT reference via phandle without breaking any existing code.
> 
> That's correct, it will be also needed for the new functionality as
> well, hence even more drivers will need to perform the MC lookup.

I don't have any issues with adding a helper if we need it from several
different locations. But the helper should be working off of a given
device and look up the device via the device tree node referenced by
phandle. We already have those phandles in place for the EMC devices,
and any other device that needs to interoperate with the MC should also
get such a reference.

> I don't quite understand why you're asking for the phandle reference,
> it's absolutely not needed for the MC lookup and won't work for the

We need that phandle in order to establish a link between the devices.
Yes, you can probably do it without the phandle and just match by
compatible string. But we don't do that for other types of devices
either, right? For a display driver we reference the attached panel via
phandle, but we could also just look it up via name or absolute path or
some other heuristic. But a phandle is just a much more explicit way of
linking the devices, so why not use it?

> older DTs if DT change will be needed. Please give a detailed explanation.

New functionality doesn't have to work with older DTs.

Thierry


signature.asc
Description: PGP signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()

On Wed, Sep 30, 2020 at 07:26:00PM +0300, Dmitry Osipenko wrote:
> 30.09.2020 19:15, Thierry Reding пишет:
> > On Wed, Sep 30, 2020 at 07:06:27PM +0300, Dmitry Osipenko wrote:
> >> 30.09.2020 19:03, Thierry Reding пишет:
> >>> On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote:
>  30.09.2020 18:23, Thierry Reding пишет:
> > On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
> >> From: Dmitry Osipenko 
> >>
> >> Multiple Tegra drivers need to retrieve Memory Controller and hence 
> >> there
> >> is quite some duplication of the retrieval code among the drivers. 
> >> Let's
> >> add a new common helper for the retrieval of the MC.
> >>
> >> Signed-off-by: Dmitry Osipenko 
> >> Signed-off-by: Nicolin Chen 
> >> ---
> >>
> >> Changelog
> >> v2->v3:
> >>  * Replaced with Dimtry's devm_tegra_get_memory_controller()
> >> v1->v2:
> >>  * N/A
> >>
> >>  drivers/memory/tegra/mc.c | 39 +++
> >>  include/soc/tegra/mc.h| 17 +
> >>  2 files changed, 56 insertions(+)
> >
> > Let's not add this helper, please. If a device needs a reference to the
> > memory controller, it should have a phandle to the memory controller in
> > device tree so that it can be looked up explicitly.
> >
> > Adding this helper is officially sanctioning that it's okay not to have
> > that reference and that's a bad idea.
> 
>  And please explain why it's a bad idea, I don't see anything bad here at
>  all.
> >>>
> >>> Well, you said yourself in a recent comment that we should avoid global
> >>> variables. devm_tegra_get_memory_controller() is nothing but a glorified
> >>> global variable.
> >>
> >> This is not a variable, but a common helper function which will remove
> >> the duplicated code and will help to avoid common mistakes like a missed
> >> put_device().
> > 
> > Yeah, you're right: this is actually much worse than a global variable.
> > It's a helper function that needs 50+ lines in order to effectively
> > access a global variable.
> > 
> > You could write this much simpler by doing something like this:
> > 
> > static struct tegra_mc *global_mc;
> > 
> > int tegra_mc_probe(...)
> > {
> > ...
> > 
> > global_mc = mc;
> > 
> > ...
> > }
> > 
> > struct tegra_mc *tegra_get_memory_controller(void)
> > {
> > return global_mc;
> > }
> > 
> > The result is *exactly* the same, except that this is actually more
> > honest. Nicolin's patch *pretends* that it isn't using a global variable
> > by wrapping a lot of complicated code around it.
> > 
> > But that doesn't change the fact that this accesses a singleton object
> > without actually being able to tie it to the device in the first place.
> 
> I don't think that the MC driver will stay built-in forever, although
> its modularization is complicated right now. Hence something shall keep
> the reference to the MC device resources while they are in use and this
> patch takes care of doing that.

It looks to me like all the other places where we get a reference to the
MC also keep a reference to the device. That's obviously not going to be
enough once the code is turned into a module. At that point we need to
make sure to also grab a reference to the module. But that's orthogonal
to this discussion.

> Secondly, the Nicolin's patch doesn't pretend on anything, but rather

Yes, the patch does pretend to "look up" the memory controller device,
but in reality it will always return a singleton object, which can just
as easily be achieved by using a global variable.

> brings the already existing duplicated code to a single common place.

Where exactly is that duplicated code? The only places I see where we
get a reference to the memory controller are from the EMC drivers and
they properly look up the MC via the nvidia,memory-controller device
tree property.

But that's not what this new helper does. This code will use the OF
lookup table to find any match and then returns that, completely
ignoring any links established by the device tree.

Thierry


signature.asc
Description: PGP signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

...
>> Secondly, I'm already about to use the new tegra_get_memory_controller()
>> API for all the T20/30/124/210 EMC and devfreq drivers.
> 
> Also, this really proves the point I was trying to make about how this
> is going to proliferate...

Sorry, I'm probably totally missing yours point.. "what" exactly will
proliferate?
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev

30.09.2020 19:06, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 06:36:52PM +0300, Dmitry Osipenko wrote:
>>  I'...
 +  struct tegra_mc *mc = devm_tegra_get_memory_controller(dev);
 +  struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
>>>
>>> It looks to me like the only reason why you need this new global API is
>>> because PCI devices may not have a device tree node with a phandle to
>>> the IOMMU. However, SMMU support for PCI will only be enabled if the
>>> root complex has an iommus property, right? In that case, can't we
>>> simply do something like this:
>>>
>>> if (dev_is_pci(dev))
>>> np = find_host_bridge(dev)->of_node;
>>> else
>>> np = dev->of_node;
>>>
>>> ? I'm not sure exactly what find_host_bridge() is called, but I'm pretty
>>> sure that exists.
>>>
>>> Once we have that we can still iterate over the iommus property and do
>>> not need to rely on this global variable.
>>
>> This sounds more complicated than the current variant.
>>
>> Secondly, I'm already about to use the new tegra_get_memory_controller()
>> API for all the T20/30/124/210 EMC and devfreq drivers.
> 
> Why do we need it there? They seem to work fine without it right now.

All the Tegra30/124/210 EMC drivers are already duplicating that MC
lookup code and only the recent T210 driver does it properly.

> If it is required for new functionality, we can always make the dependent
> on a DT reference via phandle without breaking any existing code.

That's correct, it will be also needed for the new functionality as
well, hence even more drivers will need to perform the MC lookup.

I don't quite understand why you're asking for the phandle reference,
it's absolutely not needed for the MC lookup and won't work for the
older DTs if DT change will be needed. Please give a detailed explanation.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()

30.09.2020 19:15, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 07:06:27PM +0300, Dmitry Osipenko wrote:
>> 30.09.2020 19:03, Thierry Reding пишет:
>>> On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote:
 30.09.2020 18:23, Thierry Reding пишет:
> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
>> From: Dmitry Osipenko 
>>
>> Multiple Tegra drivers need to retrieve Memory Controller and hence there
>> is quite some duplication of the retrieval code among the drivers. Let's
>> add a new common helper for the retrieval of the MC.
>>
>> Signed-off-by: Dmitry Osipenko 
>> Signed-off-by: Nicolin Chen 
>> ---
>>
>> Changelog
>> v2->v3:
>>  * Replaced with Dimtry's devm_tegra_get_memory_controller()
>> v1->v2:
>>  * N/A
>>
>>  drivers/memory/tegra/mc.c | 39 +++
>>  include/soc/tegra/mc.h| 17 +
>>  2 files changed, 56 insertions(+)
>
> Let's not add this helper, please. If a device needs a reference to the
> memory controller, it should have a phandle to the memory controller in
> device tree so that it can be looked up explicitly.
>
> Adding this helper is officially sanctioning that it's okay not to have
> that reference and that's a bad idea.

 And please explain why it's a bad idea, I don't see anything bad here at
 all.
>>>
>>> Well, you said yourself in a recent comment that we should avoid global
>>> variables. devm_tegra_get_memory_controller() is nothing but a glorified
>>> global variable.
>>
>> This is not a variable, but a common helper function which will remove
>> the duplicated code and will help to avoid common mistakes like a missed
>> put_device().
> 
> Yeah, you're right: this is actually much worse than a global variable.
> It's a helper function that needs 50+ lines in order to effectively
> access a global variable.
> 
> You could write this much simpler by doing something like this:
> 
>   static struct tegra_mc *global_mc;
> 
>   int tegra_mc_probe(...)
>   {
>   ...
> 
>   global_mc = mc;
> 
>   ...
>   }
> 
>   struct tegra_mc *tegra_get_memory_controller(void)
>   {
>   return global_mc;
>   }
> 
> The result is *exactly* the same, except that this is actually more
> honest. Nicolin's patch *pretends* that it isn't using a global variable
> by wrapping a lot of complicated code around it.
> 
> But that doesn't change the fact that this accesses a singleton object
> without actually being able to tie it to the device in the first place.

I don't think that the MC driver will stay built-in forever, although
its modularization is complicated right now. Hence something shall keep
the reference to the MC device resources while they are in use and this
patch takes care of doing that.

Secondly, the Nicolin's patch doesn't pretend on anything, but rather
brings the already existing duplicated code to a single common place.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 1/3] memory: tegra: Add devm_tegra_get_memory_controller()

On Wed, Sep 30, 2020 at 07:06:27PM +0300, Dmitry Osipenko wrote:
> 30.09.2020 19:03, Thierry Reding пишет:
> > On Wed, Sep 30, 2020 at 06:53:06PM +0300, Dmitry Osipenko wrote:
> >> 30.09.2020 18:23, Thierry Reding пишет:
> >>> On Wed, Sep 30, 2020 at 01:42:56AM -0700, Nicolin Chen wrote:
>  From: Dmitry Osipenko 
> 
>  Multiple Tegra drivers need to retrieve Memory Controller and hence there
>  is quite some duplication of the retrieval code among the drivers. Let's
>  add a new common helper for the retrieval of the MC.
> 
>  Signed-off-by: Dmitry Osipenko 
>  Signed-off-by: Nicolin Chen 
>  ---
> 
>  Changelog
>  v2->v3:
>   * Replaced with Dimtry's devm_tegra_get_memory_controller()
>  v1->v2:
>   * N/A
> 
>   drivers/memory/tegra/mc.c | 39 +++
>   include/soc/tegra/mc.h| 17 +
>   2 files changed, 56 insertions(+)
> >>>
> >>> Let's not add this helper, please. If a device needs a reference to the
> >>> memory controller, it should have a phandle to the memory controller in
> >>> device tree so that it can be looked up explicitly.
> >>>
> >>> Adding this helper is officially sanctioning that it's okay not to have
> >>> that reference and that's a bad idea.
> >>
> >> And please explain why it's a bad idea, I don't see anything bad here at
> >> all.
> > 
> > Well, you said yourself in a recent comment that we should avoid global
> > variables. devm_tegra_get_memory_controller() is nothing but a glorified
> > global variable.
> 
> This is not a variable, but a common helper function which will remove
> the duplicated code and will help to avoid common mistakes like a missed
> put_device().

Yeah, you're right: this is actually much worse than a global variable.
It's a helper function that needs 50+ lines in order to effectively
access a global variable.

You could write this much simpler by doing something like this:

static struct tegra_mc *global_mc;

int tegra_mc_probe(...)
{
...

global_mc = mc;

...
}

struct tegra_mc *tegra_get_memory_controller(void)
{
return global_mc;
}

The result is *exactly* the same, except that this is actually more
honest. Nicolin's patch *pretends* that it isn't using a global variable
by wrapping a lot of complicated code around it.

But that doesn't change the fact that this accesses a singleton object
without actually being able to tie it to the device in the first place.

Thierry


signature.asc
Description: PGP signature
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu

Re: [PATCH v3 2/3] iommu/tegra-smmu: Rework .probe_device and .attach_dev