Re: [PATCH 1/3] iommu/amd: Implement dma_[un]map_resource()
On Tue, Oct 08, 2019 at 04:18:35PM -0600, Logan Gunthorpe wrote:
> From: Kit Chow
>
> Currently the Intel IOMMU uses the default dma_[un]map_resource()
s/Intel/AMD/ ?
> +static dma_addr_t map_resource(struct device *dev, phys_addr_t paddr,
> + size_t size, enum dma_data_direction dir, unsigned long attrs)
> +{
> + struct protection_domain *domain;
> + struct dma_ops_domain *dma_dom;
> +
> + domain = get_domain(dev);
> + if (PTR_ERR(domain) == -EINVAL)
> + return (dma_addr_t)paddr;
I thought that case can't happen anymore?
Also note that Joerg just applied the patch to convert the AMD iommu
driver to use the dma-iommu ops. Can you test that series and check
it does the right thing for your use case? From looking at the code
I think it should.
Re: [V2, 2/2] media: i2c: Add DW9768 VCM driver
Hi Dongchun,
On Thu, Sep 5, 2019 at 4:22 PM wrote:
>
> From: Dongchun Zhu
>
> This patch adds a V4L2 sub-device driver for DW9768 lens voice coil,
> and provides control to set the desired focus.
>
> The DW9768 is a 10 bit DAC with 100mA output current sink capability
> from Dongwoon, designed for linear control of voice coil motor,
> and controlled via I2C serial interface.
>
> Signed-off-by: Dongchun Zhu
> ---
> MAINTAINERS| 1 +
> drivers/media/i2c/Kconfig | 10 ++
> drivers/media/i2c/Makefile | 1 +
> drivers/media/i2c/dw9768.c | 349
> +
> 4 files changed, 361 insertions(+)
> create mode 100644 drivers/media/i2c/dw9768.c
>
Please see my further comments inline.
[snip]
> +struct regval_list {
> + unsigned char reg_num;
> + unsigned char value;
nit: Since we have strictly sized values here, should we use u8 for
both fields instead?
> +};
> +
> +static struct regval_list dw9768_init_regs[] = {
> + {0x02, 0x02},
> + {DW9768_CMD_DELAY, DW9768_CMD_DELAY},
> + {0x06, 0x41},
> + {0x07, 0x39},
> + {DW9768_CMD_DELAY, DW9768_CMD_DELAY},
> +};
> +
> +static struct regval_list dw9768_release_regs[] = {
> + {0x02, 0x00},
> + {DW9768_CMD_DELAY, DW9768_CMD_DELAY},
> + {0x01, 0x00},
> + {DW9768_CMD_DELAY, DW9768_CMD_DELAY},
> +};
> +
> +static int dw9768_write_smbus(struct dw9768 *dw9768, unsigned char reg,
> + unsigned char value)
Should we use u8 for the last two arguments here as well?
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> + int ret;
> +
> + if (reg == DW9768_CMD_DELAY && value == DW9768_CMD_DELAY)
> + usleep_range(DW9768_CTRL_DELAY_US,
> +DW9768_CTRL_DELAY_US + 100);
ret will be uninitialized if we go this path.
> + else
> + ret = i2c_smbus_write_byte_data(client, reg, value);
> + return ret;
> +}
> +
> +static int dw9768_write_array(struct dw9768 *dw9768, struct regval_list
> *vals,
> + u32 len)
Since len is an array size, should we use size_t instead?
> +{
> + unsigned int i;
size_t?
> + int ret;
> +
> + for (i = 0; i < len; i++) {
> + ret = dw9768_write_smbus(dw9768, vals->reg_num, vals->value);
This should refer to vals[i] instead.
> + if (ret < 0)
> + return ret;
> + }
> + return 0;
> +}
> +
> +static int dw9768_set_position(struct dw9768 *dw9768, u16 val)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> + u8 addr[2];
> +
> + addr[0] = (val >> DW9768_DAC_SHIFT) & DW9768_REG_MASK_MSB;
> + addr[1] = val & DW9768_REG_MASK_LSB;
> +
> + return i2c_smbus_write_block_data(client, DW9768_SET_POSITION_ADDR,
> + ARRAY_SIZE(addr), addr);
As we discovered earlier, i2c_smbus_write_block_data() uses a
different protocol from what we expected. Please change to
i2c_smbus_write_word_data(), as per our downstream changes.
> +}
> +
> +static int dw9768_release(struct dw9768 *dw9768)
> +{
> + return dw9768_write_array(dw9768, dw9768_release_regs,
> + ARRAY_SIZE(dw9768_release_regs));
> +}
> +
> +static int dw9768_init(struct dw9768 *dw9768)
> +{
> + return dw9768_write_array(dw9768, dw9768_init_regs,
> + ARRAY_SIZE(dw9768_init_regs));
> +}
> +
> +/* Power handling */
> +static int dw9768_power_off(struct dw9768 *dw9768)
> +{
> + struct i2c_client *client = v4l2_get_subdevdata(&dw9768->sd);
> + int ret;
> +
> + ret = dw9768_release(dw9768);
> + if (ret)
> + dev_err(&client->dev, "dw9768 release failed!\n");
> +
> + ret = regulator_disable(dw9768->vin);
> + if (ret)
> + return ret;
> +
> + return regulator_disable(dw9768->vdd);
> +}
> +
> +static int dw9768_power_on(struct dw9768 *dw9768)
> +{
> + int ret;
> +
> + ret = regulator_enable(dw9768->vin);
> + if (ret < 0)
> + return ret;
> +
> + ret = regulator_enable(dw9768->vdd);
> + if (ret < 0)
> + return ret;
There is at least T_opr = 200 us of delay needed here. Would you be
able to add a comment and a corresponding usleep_range() call? I guess
the range of (300, 400) would be enough on the safe side.
Best regards,
Tomasz
[PATCH 0/3] AMD IOMMU Changes for NTB
Hi,
Please find the following patches which help support
Non-Transparent-Bridge (NTB) devices on AMD platforms with the IOMMU
enabled.
The first patch implements dma_map_resource() correctly with the AMD
IOMMU. This is required for correct functioning of ntb_transport which
uses that interface.
The second two patches add support for multiple PCI aliases. NTB
hardware will normally send TLPs from a range of requestor IDs to
facilitate routing the responses back to the correct requestor on the
other side of the bridge. To support this, NTB hardware registers a
number of PCI aliases. Currently the AMD IOMMU only allows for one
PCI alias so TLPs from the other aliases get rejected.
See commit ad281ecf1c7d ("PCI: Add DMA alias quirk for Microsemi
Switchtec NTB") for more information on this.
Similar patches were upstreamed for Intel hardware earlier this year:
commit 21d5d27c042d ("iommu/vt-d: Implement dma_[un]map_resource()")
commit 3f0c625c6ae7 ("iommu/vt-d: Allow interrupts from the entire bus
for aliased devices")
Thanks,
Logan
--
Kit Chow (1):
iommu/amd: Implement dma_[un]map_resource()
Logan Gunthorpe (2):
iommu/amd: Support multiple PCI DMA aliases in device table
iommu/amd: Support multiple PCI DMA aliases in IRQ Remapping
drivers/iommu/amd_iommu.c | 198 +++-
drivers/iommu/amd_iommu_types.h | 2 +-
2 files changed, 120 insertions(+), 80 deletions(-)
--
2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH 1/3] iommu/amd: Implement dma_[un]map_resource()
From: Kit Chow
Currently the Intel IOMMU uses the default dma_[un]map_resource()
implementations does nothing and simply returns the physical address
unmodified.
However, this doesn't create the IOVA entries necessary for addresses
mapped this way to work when the IOMMU is enabled. Thus, when the
IOMMU is enabled, drivers relying on dma_map_resource() will not get the
propper mapping. We see this when running ntb_transport with the IOMMU
enabled, DMA, and switchtec hardware.
The implementation for the amd version of map_resource() is nearly
identical to map_page(), just with a phys_addr passed instead of a page.
dma_unmap_resource() uses unmap_page() directly as the functions are
identical.
Signed-off-by: Kit Chow
[log...@deltatee.com: Cleaned up into a propper commit and wrote the
commit message]
Signed-off-by: Logan Gunthorpe
---
drivers/iommu/amd_iommu.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 2369b8af81f3..aa3d9e705a45 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -2553,6 +2553,23 @@ static void unmap_page(struct device *dev, dma_addr_t
dma_addr, size_t size,
__unmap_single(dma_dom, dma_addr, size, dir);
}
+static dma_addr_t map_resource(struct device *dev, phys_addr_t paddr,
+ size_t size, enum dma_data_direction dir, unsigned long attrs)
+{
+ struct protection_domain *domain;
+ struct dma_ops_domain *dma_dom;
+
+ domain = get_domain(dev);
+ if (PTR_ERR(domain) == -EINVAL)
+ return (dma_addr_t)paddr;
+ else if (IS_ERR(domain))
+ return DMA_MAPPING_ERROR;
+
+ dma_dom = to_dma_ops_domain(domain);
+
+ return __map_single(dev, dma_dom, paddr, size, dir, *dev->dma_mask);
+}
+
static int sg_num_pages(struct device *dev,
struct scatterlist *sglist,
int nelems)
@@ -2795,6 +2812,8 @@ static const struct dma_map_ops amd_iommu_dma_ops = {
.unmap_page = unmap_page,
.map_sg = map_sg,
.unmap_sg = unmap_sg,
+ .map_resource = map_resource,
+ .unmap_resource = unmap_page,
.dma_supported = amd_iommu_dma_supported,
.mmap = dma_common_mmap,
.get_sgtable= dma_common_get_sgtable,
--
2.20.1
[PATCH 3/3] iommu/amd: Support multiple PCI DMA aliases in IRQ Remapping
Non-Transparent Bridge (NTB) devices (among others) may have many DMA
aliases seeing the hardware will send requests with different device ids
depending on their origin across the bridged hardware.
See commit ad281ecf1c7d ("PCI: Add DMA alias quirk for Microsemi Switchtec
NTB") for more information on this.
The AMD IOMMU IRQ remapping functionality ignores all PCI aliases for
IRQs so if devices send an interrupt from one of their aliases they
will be blocked on AMD hardware with the IOMMU enabled.
To fix this, ensure IRQ remapping is enabled for all aliases with
MSI interrupts.
This is analogous to the functionality added to the Intel IRQ remapping
code in commit 3f0c625c6ae7 ("iommu/vt-d: Allow interrupts from the entire
bus for aliased devices")
Signed-off-by: Logan Gunthorpe
---
drivers/iommu/amd_iommu.c | 46 +--
1 file changed, 39 insertions(+), 7 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 1f15f0bae5b3..f8a6fa8d639b 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -3758,7 +3758,27 @@ static void set_remap_table_entry(struct amd_iommu
*iommu, u16 devid,
iommu_flush_dte(iommu, devid);
}
-static struct irq_remap_table *alloc_irq_table(u16 devid)
+static int set_remap_table_entry_alias(struct pci_dev *pdev, u16 alias,
+ void *data)
+{
+ struct irq_remap_table *table = data;
+
+ irq_lookup_table[alias] = table;
+ set_dte_irq_entry(alias, table);
+
+ return 0;
+}
+
+static int iommu_flush_dte_alias(struct pci_dev *pdev, u16 alias, void *data)
+{
+ struct amd_iommu *iommu = data;
+
+ iommu_flush_dte(iommu, alias);
+
+ return 0;
+}
+
+static struct irq_remap_table *alloc_irq_table(u16 devid, struct pci_dev *pdev)
{
struct irq_remap_table *table = NULL;
struct irq_remap_table *new_table = NULL;
@@ -3804,7 +3824,14 @@ static struct irq_remap_table *alloc_irq_table(u16 devid)
table = new_table;
new_table = NULL;
- set_remap_table_entry(iommu, devid, table);
+ if (pdev) {
+ pci_for_each_dma_alias(pdev, set_remap_table_entry_alias,
+ table);
+ pci_for_each_dma_alias(pdev, iommu_flush_dte_alias, iommu);
+ } else {
+ set_remap_table_entry(iommu, devid, table);
+ }
+
if (devid != alias)
set_remap_table_entry(iommu, alias, table);
@@ -3821,7 +3848,8 @@ static struct irq_remap_table *alloc_irq_table(u16 devid)
return table;
}
-static int alloc_irq_index(u16 devid, int count, bool align)
+static int alloc_irq_index(u16 devid, int count, bool align,
+ struct pci_dev *pdev)
{
struct irq_remap_table *table;
int index, c, alignment = 1;
@@ -3831,7 +3859,7 @@ static int alloc_irq_index(u16 devid, int count, bool
align)
if (!iommu)
return -ENODEV;
- table = alloc_irq_table(devid);
+ table = alloc_irq_table(devid, pdev);
if (!table)
return -ENODEV;
@@ -4264,7 +4292,7 @@ static int irq_remapping_alloc(struct irq_domain *domain,
unsigned int virq,
struct irq_remap_table *table;
struct amd_iommu *iommu;
- table = alloc_irq_table(devid);
+ table = alloc_irq_table(devid, NULL);
if (table) {
if (!table->min_index) {
/*
@@ -4281,11 +4309,15 @@ static int irq_remapping_alloc(struct irq_domain
*domain, unsigned int virq,
} else {
index = -ENOMEM;
}
- } else {
+ } else if (info->type == X86_IRQ_ALLOC_TYPE_MSI ||
+ info->type == X86_IRQ_ALLOC_TYPE_MSIX) {
bool align = (info->type == X86_IRQ_ALLOC_TYPE_MSI);
- index = alloc_irq_index(devid, nr_irqs, align);
+ index = alloc_irq_index(devid, nr_irqs, align, info->msi_dev);
+ } else {
+ index = alloc_irq_index(devid, nr_irqs, false, NULL);
}
+
if (index < 0) {
pr_warn("Failed to allocate IRTE\n");
ret = index;
--
2.20.1
[PATCH 2/3] iommu/amd: Support multiple PCI DMA aliases in device table
Non-Transparent Bridge (NTB) devices (among others) may have many DMA
aliases seeing the hardware will send requests with different device ids
depending on their origin across the bridged hardware.
See commit ad281ecf1c7d ("PCI: Add DMA alias quirk for Microsemi
Switchtec NTB") for more information on this.
The AMD IOMMU ignores all the PCI aliases except the last one so DMA
transfers from these aliases will be blocked on AMD hardware with the
IOMMU enabled.
To fix this, ensure the DTEs are cloned for every PCI alias. This is
done by copying the DTE data for each alias as well as the IVRS alias
every time it is changed.
Signed-off-by: Logan Gunthorpe
---
drivers/iommu/amd_iommu.c | 133 +++-
drivers/iommu/amd_iommu_types.h | 2 +-
2 files changed, 62 insertions(+), 73 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index aa3d9e705a45..1f15f0bae5b3 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -226,71 +226,61 @@ static struct iommu_dev_data *search_dev_data(u16 devid)
return NULL;
}
-static int __last_alias(struct pci_dev *pdev, u16 alias, void *data)
+static int clone_alias(struct pci_dev *pdev, u16 alias, void *data)
{
- *(u16 *)data = alias;
- return 0;
-}
-
-static u16 get_alias(struct device *dev)
-{
- struct pci_dev *pdev = to_pci_dev(dev);
- u16 devid, ivrs_alias, pci_alias;
+ u16 devid = pci_dev_id(pdev);
- /* The callers make sure that get_device_id() does not fail here */
- devid = get_device_id(dev);
-
- /* For ACPI HID devices, we simply return the devid as such */
- if (!dev_is_pci(dev))
- return devid;
+ if (devid == alias)
+ return 0;
- ivrs_alias = amd_iommu_alias_table[devid];
+ amd_iommu_rlookup_table[alias] =
+ amd_iommu_rlookup_table[devid];
+ memcpy(amd_iommu_dev_table[alias].data,
+ amd_iommu_dev_table[devid].data,
+ sizeof(amd_iommu_dev_table[alias].data));
- pci_for_each_dma_alias(pdev, __last_alias, &pci_alias);
+ return 0;
+}
- if (ivrs_alias == pci_alias)
- return ivrs_alias;
+static void clone_aliases(struct pci_dev *pdev)
+{
+ if (!pdev)
+ return;
/*
-* DMA alias showdown
-*
-* The IVRS is fairly reliable in telling us about aliases, but it
-* can't know about every screwy device. If we don't have an IVRS
-* reported alias, use the PCI reported alias. In that case we may
-* still need to initialize the rlookup and dev_table entries if the
-* alias is to a non-existent device.
+* The IVRS alias stored in the alias table may not be
+* part of the PCI DMA aliases if it's bus differs
+* from the original device.
*/
- if (ivrs_alias == devid) {
- if (!amd_iommu_rlookup_table[pci_alias]) {
- amd_iommu_rlookup_table[pci_alias] =
- amd_iommu_rlookup_table[devid];
- memcpy(amd_iommu_dev_table[pci_alias].data,
- amd_iommu_dev_table[devid].data,
- sizeof(amd_iommu_dev_table[pci_alias].data));
- }
+ clone_alias(pdev, amd_iommu_alias_table[pci_dev_id(pdev)], NULL);
- return pci_alias;
- }
+ pci_for_each_dma_alias(pdev, clone_alias, NULL);
+}
- pci_info(pdev, "Using IVRS reported alias %02x:%02x.%d "
- "for device [%04x:%04x], kernel reported alias "
- "%02x:%02x.%d\n", PCI_BUS_NUM(ivrs_alias), PCI_SLOT(ivrs_alias),
- PCI_FUNC(ivrs_alias), pdev->vendor, pdev->device,
- PCI_BUS_NUM(pci_alias), PCI_SLOT(pci_alias),
- PCI_FUNC(pci_alias));
+static struct pci_dev *setup_aliases(struct device *dev)
+{
+ struct pci_dev *pdev = to_pci_dev(dev);
+ u16 ivrs_alias;
+
+ /* For ACPI HID devices, there are no aliases */
+ if (!dev_is_pci(dev))
+ return NULL;
/*
-* If we don't have a PCI DMA alias and the IVRS alias is on the same
-* bus, then the IVRS table may know about a quirk that we don't.
+* Add the IVRS alias to the pci aliases if it is on the same
+* bus. The IVRS table may know about a quirk that we don't.
*/
- if (pci_alias == devid &&
+ ivrs_alias = amd_iommu_alias_table[pci_dev_id(pdev)];
+ if (ivrs_alias != pci_dev_id(pdev) &&
PCI_BUS_NUM(ivrs_alias) == pdev->bus->number) {
pci_add_dma_alias(pdev, ivrs_alias & 0xff);
pci_info(pdev, "Added PCI DMA alias %02x.%d\n",
PCI_SLOT(ivrs_alias), PCI_FUNC(ivrs_alias));
}
- return ivrs_alias;
+ clone_aliases(pdev);
+
+ return pdev;
}
static struct iommu_dev_data *f
Re: [PATCH v3 6/6] iommu/amd: Switch to use acpi_dev_hid_uid_match()
On Tue Oct 01 19, Andy Shevchenko wrote:
Since we have a generic helper, drop custom implementation in the driver.
Signed-off-by: Andy Shevchenko
Reviewed-by: Mika Westerberg
---
drivers/iommu/amd_iommu.c | 30 +-
1 file changed, 5 insertions(+), 25 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index 2369b8af81f3..40f3cf44aa98 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -124,30 +124,6 @@ static struct lock_class_key reserved_rbtree_key;
*
/
-static inline int match_hid_uid(struct device *dev,
- struct acpihid_map_entry *entry)
-{
- struct acpi_device *adev = ACPI_COMPANION(dev);
- const char *hid, *uid;
-
- if (!adev)
- return -ENODEV;
-
- hid = acpi_device_hid(adev);
- uid = acpi_device_uid(adev);
-
- if (!hid || !(*hid))
- return -ENODEV;
-
- if (!uid || !(*uid))
- return strcmp(hid, entry->hid);
-
- if (!(*entry->uid))
- return strcmp(hid, entry->hid);
-
- return (strcmp(hid, entry->hid) || strcmp(uid, entry->uid));
-}
-
static inline u16 get_pci_device_id(struct device *dev)
{
struct pci_dev *pdev = to_pci_dev(dev);
@@ -158,10 +134,14 @@ static inline u16 get_pci_device_id(struct device *dev)
static inline int get_acpihid_device_id(struct device *dev,
struct acpihid_map_entry **entry)
{
+ struct acpi_device *adev = ACPI_COMPANION(dev);
struct acpihid_map_entry *p;
+ if (!adev)
+ return -ENODEV;
+
list_for_each_entry(p, &acpihid_map, list) {
- if (!match_hid_uid(dev, p)) {
+ if (acpi_dev_hid_uid_match(adev, p->hid, p->uid)) {
if (entry)
*entry = p;
return p->devid;
--
2.23.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Reviewed-by: Jerry Snitselaar
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/rockchip: Don't loop until failure to count interrupts
Hi Heiko,
Missatge de Heiko Stübner del dia dt., 8 d’oct. 2019
a les 19:58:
>
> Hi Enric,
>
> Am Dienstag, 8. Oktober 2019, 15:58:43 CEST schrieb Enric Balletbo i Serra:
> > As platform_get_irq() now prints an error when the interrupt does not
> > exist, counting interrupts by looping until failure causes the printing
> > of scary messages like:
> >
> > rk_iommu ff924000.iommu: IRQ index 1 not found
> > rk_iommu ff914000.iommu: IRQ index 1 not found
> > rk_iommu ff903f00.iommu: IRQ index 1 not found
> > rk_iommu ff8f3f00.iommu: IRQ index 1 not found
> > rk_iommu ff650800.iommu: IRQ index 1 not found
> >
> > Fix this by using the platform_irq_count() helper to avoid touching
> > non-existent interrupts.
>
> It looks like we did the same fix ... see my patch from september 25:
> https://patchwork.kernel.org/patch/11161221/
>
How in the hell I didn't catch this patch! Anyway, forget about this
patch then, sorry for the noise.
Thanks,
Enric
>
> > Fixes: 7723f4c5ecdb8d83 ("driver core: platform: Add an error message to
> > platform_get_irq*()")
> > Signed-off-by: Enric Balletbo i Serra
> > ---
> >
> > drivers/iommu/rockchip-iommu.c | 35 +++---
> > 1 file changed, 24 insertions(+), 11 deletions(-)
> >
> > diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> > index 26290f310f90..8c6318bd1b37 100644
> > --- a/drivers/iommu/rockchip-iommu.c
> > +++ b/drivers/iommu/rockchip-iommu.c
> > @@ -1136,7 +1136,7 @@ static int rk_iommu_probe(struct platform_device
> > *pdev)
> > struct rk_iommu *iommu;
> > struct resource *res;
> > int num_res = pdev->num_resources;
> > - int err, i, irq;
> > + int err, i, irq, num_irqs;
> >
> > iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
> > if (!iommu)
> > @@ -1219,20 +1219,28 @@ static int rk_iommu_probe(struct platform_device
> > *pdev)
> >
> > pm_runtime_enable(dev);
> >
> > - i = 0;
> > - while ((irq = platform_get_irq(pdev, i++)) != -ENXIO) {
> > - if (irq < 0)
> > - return irq;
> > + num_irqs = platform_irq_count(pdev);
> > + if (num_irqs < 0) {
> > + err = num_irqs;
> > + goto err_disable_pm_runtime;
> > + }
>
> By moving the basic irq count above the pm_runtime_enable
> you save some lines and the whole goto logic ... see my patch
> for reference :-D
>
> Heiko
>
> > +
> > + for (i = 0; i < num_irqs; i++) {
> > + irq = platform_get_irq(pdev, i);
> > + if (irq < 0) {
> > + err = irq;
> > + goto err_disable_pm_runtime;
> > + }
> >
> > err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
> > IRQF_SHARED, dev_name(dev), iommu);
> > - if (err) {
> > - pm_runtime_disable(dev);
> > - goto err_remove_sysfs;
> > - }
> > + if (err)
> > + goto err_disable_pm_runtime;
> > }
> >
> > return 0;
> > +err_disable_pm_runtime:
> > + pm_runtime_disable(dev);
> > err_remove_sysfs:
> > iommu_device_sysfs_remove(&iommu->iommu);
> > err_put_group:
> > @@ -1245,10 +1253,15 @@ static int rk_iommu_probe(struct platform_device
> > *pdev)
> > static void rk_iommu_shutdown(struct platform_device *pdev)
> > {
> > struct rk_iommu *iommu = platform_get_drvdata(pdev);
> > - int i = 0, irq;
> > + int i, irq, num_irqs;
> >
> > - while ((irq = platform_get_irq(pdev, i++)) != -ENXIO)
> > + num_irqs = platform_irq_count(pdev);
> > + for (i = 0; i < num_irqs; i++) {
> > + irq = platform_get_irq(pdev, i);
> > + if (irq < 0)
> > + continue;
> > devm_free_irq(iommu->dev, irq, iommu);
> > + }
> >
> > pm_runtime_force_suspend(&pdev->dev);
> > }
> >
>
>
>
>
>
> ___
> Linux-rockchip mailing list
> linux-rockc...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-rockchip
Re: [PATCH] iommu/rockchip: Don't loop until failure to count interrupts
Hi Enric,
Am Dienstag, 8. Oktober 2019, 15:58:43 CEST schrieb Enric Balletbo i Serra:
> As platform_get_irq() now prints an error when the interrupt does not
> exist, counting interrupts by looping until failure causes the printing
> of scary messages like:
>
> rk_iommu ff924000.iommu: IRQ index 1 not found
> rk_iommu ff914000.iommu: IRQ index 1 not found
> rk_iommu ff903f00.iommu: IRQ index 1 not found
> rk_iommu ff8f3f00.iommu: IRQ index 1 not found
> rk_iommu ff650800.iommu: IRQ index 1 not found
>
> Fix this by using the platform_irq_count() helper to avoid touching
> non-existent interrupts.
It looks like we did the same fix ... see my patch from september 25:
https://patchwork.kernel.org/patch/11161221/
> Fixes: 7723f4c5ecdb8d83 ("driver core: platform: Add an error message to
> platform_get_irq*()")
> Signed-off-by: Enric Balletbo i Serra
> ---
>
> drivers/iommu/rockchip-iommu.c | 35 +++---
> 1 file changed, 24 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 26290f310f90..8c6318bd1b37 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -1136,7 +1136,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
> struct rk_iommu *iommu;
> struct resource *res;
> int num_res = pdev->num_resources;
> - int err, i, irq;
> + int err, i, irq, num_irqs;
>
> iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
> if (!iommu)
> @@ -1219,20 +1219,28 @@ static int rk_iommu_probe(struct platform_device
> *pdev)
>
> pm_runtime_enable(dev);
>
> - i = 0;
> - while ((irq = platform_get_irq(pdev, i++)) != -ENXIO) {
> - if (irq < 0)
> - return irq;
> + num_irqs = platform_irq_count(pdev);
> + if (num_irqs < 0) {
> + err = num_irqs;
> + goto err_disable_pm_runtime;
> + }
By moving the basic irq count above the pm_runtime_enable
you save some lines and the whole goto logic ... see my patch
for reference :-D
Heiko
> +
> + for (i = 0; i < num_irqs; i++) {
> + irq = platform_get_irq(pdev, i);
> + if (irq < 0) {
> + err = irq;
> + goto err_disable_pm_runtime;
> + }
>
> err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
> IRQF_SHARED, dev_name(dev), iommu);
> - if (err) {
> - pm_runtime_disable(dev);
> - goto err_remove_sysfs;
> - }
> + if (err)
> + goto err_disable_pm_runtime;
> }
>
> return 0;
> +err_disable_pm_runtime:
> + pm_runtime_disable(dev);
> err_remove_sysfs:
> iommu_device_sysfs_remove(&iommu->iommu);
> err_put_group:
> @@ -1245,10 +1253,15 @@ static int rk_iommu_probe(struct platform_device
> *pdev)
> static void rk_iommu_shutdown(struct platform_device *pdev)
> {
> struct rk_iommu *iommu = platform_get_drvdata(pdev);
> - int i = 0, irq;
> + int i, irq, num_irqs;
>
> - while ((irq = platform_get_irq(pdev, i++)) != -ENXIO)
> + num_irqs = platform_irq_count(pdev);
> + for (i = 0; i < num_irqs; i++) {
> + irq = platform_get_irq(pdev, i);
> + if (irq < 0)
> + continue;
> devm_free_irq(iommu->dev, irq, iommu);
> + }
>
> pm_runtime_force_suspend(&pdev->dev);
> }
>
[PATCH] iommu/rockchip: Don't loop until failure to count interrupts
As platform_get_irq() now prints an error when the interrupt does not
exist, counting interrupts by looping until failure causes the printing
of scary messages like:
rk_iommu ff924000.iommu: IRQ index 1 not found
rk_iommu ff914000.iommu: IRQ index 1 not found
rk_iommu ff903f00.iommu: IRQ index 1 not found
rk_iommu ff8f3f00.iommu: IRQ index 1 not found
rk_iommu ff650800.iommu: IRQ index 1 not found
Fix this by using the platform_irq_count() helper to avoid touching
non-existent interrupts.
Fixes: 7723f4c5ecdb8d83 ("driver core: platform: Add an error message to
platform_get_irq*()")
Signed-off-by: Enric Balletbo i Serra
---
drivers/iommu/rockchip-iommu.c | 35 +++---
1 file changed, 24 insertions(+), 11 deletions(-)
diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
index 26290f310f90..8c6318bd1b37 100644
--- a/drivers/iommu/rockchip-iommu.c
+++ b/drivers/iommu/rockchip-iommu.c
@@ -1136,7 +1136,7 @@ static int rk_iommu_probe(struct platform_device *pdev)
struct rk_iommu *iommu;
struct resource *res;
int num_res = pdev->num_resources;
- int err, i, irq;
+ int err, i, irq, num_irqs;
iommu = devm_kzalloc(dev, sizeof(*iommu), GFP_KERNEL);
if (!iommu)
@@ -1219,20 +1219,28 @@ static int rk_iommu_probe(struct platform_device *pdev)
pm_runtime_enable(dev);
- i = 0;
- while ((irq = platform_get_irq(pdev, i++)) != -ENXIO) {
- if (irq < 0)
- return irq;
+ num_irqs = platform_irq_count(pdev);
+ if (num_irqs < 0) {
+ err = num_irqs;
+ goto err_disable_pm_runtime;
+ }
+
+ for (i = 0; i < num_irqs; i++) {
+ irq = platform_get_irq(pdev, i);
+ if (irq < 0) {
+ err = irq;
+ goto err_disable_pm_runtime;
+ }
err = devm_request_irq(iommu->dev, irq, rk_iommu_irq,
IRQF_SHARED, dev_name(dev), iommu);
- if (err) {
- pm_runtime_disable(dev);
- goto err_remove_sysfs;
- }
+ if (err)
+ goto err_disable_pm_runtime;
}
return 0;
+err_disable_pm_runtime:
+ pm_runtime_disable(dev);
err_remove_sysfs:
iommu_device_sysfs_remove(&iommu->iommu);
err_put_group:
@@ -1245,10 +1253,15 @@ static int rk_iommu_probe(struct platform_device *pdev)
static void rk_iommu_shutdown(struct platform_device *pdev)
{
struct rk_iommu *iommu = platform_get_drvdata(pdev);
- int i = 0, irq;
+ int i, irq, num_irqs;
- while ((irq = platform_get_irq(pdev, i++)) != -ENXIO)
+ num_irqs = platform_irq_count(pdev);
+ for (i = 0; i < num_irqs; i++) {
+ irq = platform_get_irq(pdev, i);
+ if (irq < 0)
+ continue;
devm_free_irq(iommu->dev, irq, iommu);
+ }
pm_runtime_force_suspend(&pdev->dev);
}
--
2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v2 6/6] iommu/amd: Switch to use acpi_dev_hid_uid_match()
On Mon, Oct 07, 2019 at 05:28:48PM +0200, Joerg Roedel wrote: > On Tue, Sep 24, 2019 at 10:37:39PM +0300, Andy Shevchenko wrote: > > Since we have a generic helper, drop custom implementation in the driver. > > > > Signed-off-by: Andy Shevchenko > > --- > > drivers/iommu/amd_iommu.c | 30 +- > > 1 file changed, 5 insertions(+), 25 deletions(-) > > Acked-by: Joerg Roedel Thanks! There is v3 available, does it apply to it? -- With Best Regards, Andy Shevchenko ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v4 3/4] vfio: zpci: defining the VFIO headers
On Fri, 20 Sep 2019 11:14:28 -0400
Matthew Rosato wrote:
> On 9/20/19 10:02 AM, Cornelia Huck wrote:
> > On Thu, 19 Sep 2019 16:55:57 -0400
> > Matthew Rosato wrote:
> >
> >> On 9/19/19 11:20 AM, Cornelia Huck wrote:
> >>> On Fri, 6 Sep 2019 20:13:50 -0400
> >>> Matthew Rosato wrote:
> >>>
> From: Pierre Morel
>
> We define a new device region in vfio.h to be able to
> get the ZPCI CLP information by reading this region from
> userland.
>
> We create a new file, vfio_zdev.h to define the structure
> of the new region we defined in vfio.h
>
> Signed-off-by: Pierre Morel
> Signed-off-by: Matthew Rosato
> ---
> include/uapi/linux/vfio.h | 1 +
> include/uapi/linux/vfio_zdev.h | 35 +++
> 2 files changed, 36 insertions(+)
> create mode 100644 include/uapi/linux/vfio_zdev.h
> >
> diff --git a/include/uapi/linux/vfio_zdev.h
> b/include/uapi/linux/vfio_zdev.h
> new file mode 100644
> index 000..55e0d6d
> --- /dev/null
> +++ b/include/uapi/linux/vfio_zdev.h
> @@ -0,0 +1,35 @@
> +/* SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note */
> +/*
> + * Region definition for ZPCI devices
> + *
> + * Copyright IBM Corp. 2019
> + *
> + * Author(s): Pierre Morel
> + */
> +
> +#ifndef _VFIO_ZDEV_H_
> +#define _VFIO_ZDEV_H_
> +
> +#include
> +
> +/**
> + * struct vfio_region_zpci_info - ZPCI information.
> >>>
> >>> Hm... probably should also get some more explanation. E.g. is that
> >>> derived from a hardware structure?
> >>>
> >>
> >> The structure itself is not mapped 1:1 to a hardware structure, but it
> >> does serve as a collection of information that was derived from other
> >> hardware structures.
> >>
> >> "Used for passing hardware feature information about a zpci device
> >> between the host and guest" ?
> >
> > "zPCI specific hardware feature information for a device"?
> >
> > Are we reasonably sure that this is complete for now? I'm not sure if
> > expanding this structure would work; adding another should always be
> > possible, though (if a bit annoying).
> >
>
> I think trying to make the structure expandable would be best... If we
> allow arbitrary-sized reads of the info, and only add new fields onto
> the end it should be OK, no? (older qemu doesn't get the info it doesn't
> ask for / understand) But I guess that's not compatible with having
> util_str[] size being defined dynamically. Another caveat would be if
> CLP_UTIL_STR_LEN were to grow in size in the future, and assuming
> util_str[] was no longer at the end of the structure, I guess the
> additional data would have to end up in a
> util_str2[CLP_UTIL_STR_LEN_NEW-CLP_UTIL_STR_LEN_OLD]... To explain what
> I mean, something like:
>
> struct vfio_region_zpci_info {
> <..>
> __u8 util_str[CLP_UTIL_STR_LEN_OLD];
> /* END OF V1 */
> __u8 foo;
> /* END OF V2 */
> __u8 util_str2[CLP_UTIL_STR_LEN_NEW-CLP_UTIL_STR_LEN_OLD];
> /* END OF V3 */
> } __packed;
[Sorry about the late response -- was on PTO]
That sounds a bit too complicated to me, and I'd prefer the "add
another region if we missed something" approach. If we put anything
looking potentially useful in here now, that "add another region" event
is hopefully far in the future.
>
>
> >>
> + *
> + */
> +struct vfio_region_zpci_info {
> +__u64 dasm;
> +__u64 start_dma;
> +__u64 end_dma;
> +__u64 msi_addr;
> +__u64 flags;
> +__u16 pchid;
> +__u16 mui;
> +__u16 noi;
> +__u16 maxstbl;
> +__u8 version;
> +__u8 gid;
> +#define VFIO_PCI_ZDEV_FLAGS_REFRESH 1
> +__u8 util_str[];
> +} __packed;
> +
> +#endif
> >>>
> >>>
> >>
> >
>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v6 0/5] iommu/amd: Convert the AMD iommu driver to the dma-iommu api
On Sun, Sep 08, 2019 at 09:56:36AM -0700, Tom Murphy wrote: > Convert the AMD iommu driver to the dma-iommu api. Remove the iova > handling and reserve region code from the AMD iommu driver. Applied, thanks. Note that it will not show up in linux-next before -rc3. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [GIT PULL] iommu/arm-smmu: Fixes for 5.4-rc2
On Wed, Oct 02, 2019 at 02:12:59PM +0100, Will Deacon wrote: > The following changes since commit 54ecb8f7028c5eb3d740bb82b0f1d90f2df63c5c: > > Linux 5.4-rc1 (2019-09-30 10:35:40 -0700) > > are available in the Git repository at: > > git://git.kernel.org/pub/scm/linux/kernel/git/will/linux.git > for-joerg/arm-smmu/fixes Pulled, thanks Will. ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/mediatek: Move the tlb_sync into tlb_flush
On Mon, 2019-09-30 at 13:09 +0100, Will Deacon wrote:
> On Mon, Sep 30, 2019 at 01:42:22PM +0800, Yong Wu wrote:
> > The commit 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API
> > TLB sync") help move the tlb_sync of unmap from v7s into the iommu
> > framework. It helps add a new function "mtk_iommu_iotlb_sync", But it
> > lacked the dom->pgtlock, then it will cause the variable
> > "tlb_flush_active" may be changed unexpectedly, we could see this warning
> > log randomly:
> >
> > mtk-iommu 10205000.iommu: Partial TLB flush timed out, falling back to
> > full flush
> >
> > To fix this issue, we can add dom->pgtlock in the "mtk_iommu_iotlb_sync".
> > And when checking this issue, we find that __arm_v7s_unmap call
> > io_pgtable_tlb_add_flush consecutively when it is supersection/largepage,
> > this also is potential unsafe for us. There is no tlb flush queue in the
> > MediaTek M4U HW. The HW always expect the tlb_flush/tlb_sync one by one.
> > If v7s don't always gurarantee the sequence, Thus, In this patch I move
> > the tlb_sync into tlb_flush(also rename the function deleting "_nosync").
> > and we don't care if it is leaf, rearrange the callback functions. Also,
> > the tlb flush/sync was already finished in v7s, then iotlb_sync and
> > iotlb_sync_all is unnecessary.
> >
> > Besides, there are two minor changes:
> > a) Use writel for the register F_MMU_INV_RANGE which is for triggering the
> > HW work. We expect all the setting(iova_start/iova_end...) have already
> > been finished before F_MMU_INV_RANGE.
> > b) Reduce the tlb timeout value from 10us to 1000us. the original value
> > is so long that affect the multimedia performance.
> >
> > Fixes: 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API TLB
> > sync")
> > Signed-off-by: Chao Hao
> > Signed-off-by: Yong Wu
> > ---
> > This patch looks break the logic for tlb_flush and tlb_sync. I'm not
> > sure if it
> > is reasonable. If someone has concern, I could change:
> > a) Add dom->pgtlock in the mtk_iommu_iotlb_sync
> > b) Add a io_pgtable_tlb_sync in [1].
>
> The patch looks ok to me, but please could you split it up so that the
> timeout and writel are done separately?
Thanks for the quick review, I will separate them.
>
> Thanks,
>
> Will
Re: [PATCH] iommu/mediatek: Move the tlb_sync into tlb_flush
Hi Tomasz,
Sorry for reply late.
On Wed, 2019-10-02 at 14:18 +0900, Tomasz Figa wrote:
> Hi Yong,
>
> On Mon, Sep 30, 2019 at 2:42 PM Yong Wu wrote:
> >
> > The commit 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API
> > TLB sync") help move the tlb_sync of unmap from v7s into the iommu
> > framework. It helps add a new function "mtk_iommu_iotlb_sync", But it
> > lacked the dom->pgtlock, then it will cause the variable
> > "tlb_flush_active" may be changed unexpectedly, we could see this warning
> > log randomly:
> >
>
> Thanks for the patch! Please see my comments inline.
>
> > mtk-iommu 10205000.iommu: Partial TLB flush timed out, falling back to
> > full flush
> >
> > To fix this issue, we can add dom->pgtlock in the "mtk_iommu_iotlb_sync".
> > And when checking this issue, we find that __arm_v7s_unmap call
> > io_pgtable_tlb_add_flush consecutively when it is supersection/largepage,
> > this also is potential unsafe for us. There is no tlb flush queue in the
> > MediaTek M4U HW. The HW always expect the tlb_flush/tlb_sync one by one.
> > If v7s don't always gurarantee the sequence, Thus, In this patch I move
> > the tlb_sync into tlb_flush(also rename the function deleting "_nosync").
> > and we don't care if it is leaf, rearrange the callback functions. Also,
> > the tlb flush/sync was already finished in v7s, then iotlb_sync and
> > iotlb_sync_all is unnecessary.
>
> Performance-wise, we could do much better. Instead of synchronously
> syncing at the end of mtk_iommu_tlb_add_flush(), we could sync at the
> beginning, if there was any previous flush still pending. We would
> also have to keep the .iotlb_sync() callback, to take care of waiting
> for the last flush. That would allow better pipelining with CPU in
> cases like this:
>
> for (all pages in range) {
>change page table();
>flush();
> }
>
> "change page table()" could execute while the IOMMU is flushing the
> previous change.
Do you mean adding a new tlb_sync before tlb_flush_no_sync, like below:
mtk_iommu_tlb_add_flush_nosync {
+ mtk_iommu_tlb_sync();
tlb_flush_no_sync();
data->tlb_flush_active = true;
}
mtk_iommu_tlb_sync {
if (!data->tlb_flush_active)
return;
tlb_sync();
data->tlb_flush_active = false;
}
This way look improve the flow, But adjusting the flow is not the root
cause of this issue. the problem is "data->tlb_flush_active" may be
changed from mtk_iommu_iotlb_sync which don't have a dom->pglock.
Currently the synchronisation of the tlb_flush/tlb_sync flow are
controlled by the variable "data->tlb_flush_active".
In this patch putting the tlb_flush/tlb_sync together looks make
the flow simpler:
a) Don't need the sensitive variable "tlb_flush_active".
b) Remove mtk_iommu_iotlb_sync, Don't need add lock in it.
c) Simplify the tlb_flush_walk/tlb_flush_leaf.
is it ok?
>
> >
> > Besides, there are two minor changes:
> > a) Use writel for the register F_MMU_INV_RANGE which is for triggering the
> > HW work. We expect all the setting(iova_start/iova_end...) have already
> > been finished before F_MMU_INV_RANGE.
> > b) Reduce the tlb timeout value from 10us to 1000us. the original value
> > is so long that affect the multimedia performance.
>
> By definition, timeout is something that should not normally happen.
> Too long timeout affecting multimedia performance would suggest that
> the timeout was actually happening, which is the core problem, not the
> length of the timeout. Could you provide more details on this?
As description above, this issue is because there is no dom->pgtlock in
the mtk_iommu_iotlb_sync. I have tried that the issue will disappear
after adding lock in it.
Although the issue is fixed after this patch, I still would like to
reduce the timeout value for somehow error happen in the future. 100ms
is unnecessary for us. It looks a minor improvement rather than fixing
the issue. I will use a new patch for it.
>
> >
> > Fixes: 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API TLB
> > sync")
> > Signed-off-by: Chao Hao
> > Signed-off-by: Yong Wu
> > ---
> > This patch looks break the logic for tlb_flush and tlb_sync. I'm not
> > sure if it
> > is reasonable. If someone has concern, I could change:
> > a) Add dom->pgtlock in the mtk_iommu_iotlb_sync
> > b) Add a io_pgtable_tlb_sync in [1].
> >
> > [1]
> > https://elixir.bootlin.com/linux/v5.3-rc1/source/drivers/iommu/io-pgtable-arm-v7s.c#L655
> >
> > This patch rebase on Joerg's mediatek-smmu-merge branch which has mt8183
> > and Will's "Rework IOMMU API to allow for batching of invalidation".
> > ---
> > drivers/iommu/mtk_iommu.c | 74
> > ---
> > drivers/iommu/mtk_iommu.h | 1 -
> > 2 files changed, 19 insertions(+), 56 deletions(-)
> >
> > diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
> > index 6066272..e13cc56 100644
> > --- a/drivers/iommu/mtk_iommu.c
> > +++ b/drivers/iommu/mtk_iommu.c
> >
Re: [PATCH] iommu/mediatek: Move the tlb_sync into tlb_flush
On Wed, 2019-10-02 at 11:35 +0100, Robin Murphy wrote:
> On 02/10/2019 06:18, Tomasz Figa wrote:
> > Hi Yong,
> >
> > On Mon, Sep 30, 2019 at 2:42 PM Yong Wu wrote:
> >>
> >> The commit 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API
> >> TLB sync") help move the tlb_sync of unmap from v7s into the iommu
> >> framework. It helps add a new function "mtk_iommu_iotlb_sync", But it
> >> lacked the dom->pgtlock, then it will cause the variable
> >> "tlb_flush_active" may be changed unexpectedly, we could see this warning
> >> log randomly:
> >>
> >
> > Thanks for the patch! Please see my comments inline.
> >
> >> mtk-iommu 10205000.iommu: Partial TLB flush timed out, falling back to
> >> full flush
> >>
> >> To fix this issue, we can add dom->pgtlock in the "mtk_iommu_iotlb_sync".
> >> And when checking this issue, we find that __arm_v7s_unmap call
> >> io_pgtable_tlb_add_flush consecutively when it is supersection/largepage,
> >> this also is potential unsafe for us. There is no tlb flush queue in the
> >> MediaTek M4U HW. The HW always expect the tlb_flush/tlb_sync one by one.
> >> If v7s don't always gurarantee the sequence, Thus, In this patch I move
> >> the tlb_sync into tlb_flush(also rename the function deleting "_nosync").
> >> and we don't care if it is leaf, rearrange the callback functions. Also,
> >> the tlb flush/sync was already finished in v7s, then iotlb_sync and
> >> iotlb_sync_all is unnecessary.
> >
> > Performance-wise, we could do much better. Instead of synchronously
> > syncing at the end of mtk_iommu_tlb_add_flush(), we could sync at the
> > beginning, if there was any previous flush still pending. We would
> > also have to keep the .iotlb_sync() callback, to take care of waiting
> > for the last flush. That would allow better pipelining with CPU in
> > cases like this:
> >
> > for (all pages in range) {
> > change page table();
> > flush();
> > }
> >
> > "change page table()" could execute while the IOMMU is flushing the
> > previous change.
>
> FWIW, given that the underlying invalidation mechanism is range-based,
> this driver would be an ideal candidate for making use of the new
> iommu_gather mechanism. As a fix for stable, though, simply ensuring
> that add_flush syncs any pending invalidation before issuing a new one
> sounds like a good idea (and probably a simpler patch too).
Thanks very much for the confirmation.
>
> [...]
> >> @@ -574,8 +539,7 @@ static int mtk_iommu_of_xlate(struct device *dev,
> >> struct of_phandle_args *args)
> >> .detach_dev = mtk_iommu_detach_device,
> >> .map= mtk_iommu_map,
> >> .unmap = mtk_iommu_unmap,
> >> - .flush_iotlb_all = mtk_iommu_flush_iotlb_all,
> >
> > Don't we still want .flush_iotlb_all()? I think it should be more
> > efficient in some cases than doing a big number of single flushes.
> > (That said, the previous implementation didn't do any flush at all. It
> > just waited for previously queued flushes to happen. Was that
> > expected?)
>
> Commit 07fdef34d2be ("iommu/arm-smmu-v3: Implement flush_iotlb_all
> hook") has an explanation of what the deal was there - similarly, it's
> probably worth this driver implementing it properly as well now (but
> that's really a separate patch).
Thanks the hint, At the beginning, I noticed that we don't have
DOMAIN_ATTR_DMA_USE_FLUSH_QUEUE, thus I thought flush_iotlb_all is
unnecessary.
After grep, iommu_flush_tlb_all also is called in the x_direct_mapping,
then we still need this.
If putting it in a new patch(switch flush_iotlb_all to
mtk_iommu_tlb_flush_all), then the Fixes tag(commit id: 4d689b619445) is
needed?
>
> Robin.
>
> ___
> Linux-mediatek mailing list
> linux-media...@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
