[PATCH -next] iommu/amd: fix variable "iommu" set but not used
The commit dce8d6964ebd ("iommu/amd: Convert to probe/release_device()
call-backs") introduced an unused variable,
drivers/iommu/amd_iommu.c: In function 'amd_iommu_uninit_device':
drivers/iommu/amd_iommu.c:422:20: warning: variable 'iommu' set but not
used [-Wunused-but-set-variable]
struct amd_iommu *iommu;
^
Signed-off-by: Qian Cai
---
drivers/iommu/amd_iommu.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index fef3689ee535..2b8eb58d2bea 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -419,15 +419,12 @@ static void iommu_ignore_device(struct device *dev)
static void amd_iommu_uninit_device(struct device *dev)
{
struct iommu_dev_data *dev_data;
- struct amd_iommu *iommu;
int devid;
devid = get_device_id(dev);
if (devid < 0)
return;
- iommu = amd_iommu_rlookup_table[devid];
-
dev_data = search_dev_data(devid);
if (!dev_data)
return;
--
2.21.0 (Apple Git-122.2)
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH] iommu/renesas: fix unused-function warning
gcc warns because the only reference to ipmmu_find_group
is inside of an #ifdef:
drivers/iommu/ipmmu-vmsa.c:878:28: error: 'ipmmu_find_group' defined but not
used [-Werror=unused-function]
Change the #ifdef to an equivalent IS_ENABLED().
Fixes: 6580c8a78424 ("iommu/renesas: Convert to probe/release_device()
call-backs")
Signed-off-by: Arnd Bergmann
---
drivers/iommu/ipmmu-vmsa.c | 7 ++-
1 file changed, 2 insertions(+), 5 deletions(-)
diff --git a/drivers/iommu/ipmmu-vmsa.c b/drivers/iommu/ipmmu-vmsa.c
index fb7e702dee23..4c2972f3153b 100644
--- a/drivers/iommu/ipmmu-vmsa.c
+++ b/drivers/iommu/ipmmu-vmsa.c
@@ -903,11 +903,8 @@ static const struct iommu_ops ipmmu_ops = {
.probe_device = ipmmu_probe_device,
.release_device = ipmmu_release_device,
.probe_finalize = ipmmu_probe_finalize,
-#if defined(CONFIG_ARM) && !defined(CONFIG_IOMMU_DMA)
- .device_group = generic_device_group,
-#else
- .device_group = ipmmu_find_group,
-#endif
+ .device_group = IS_ENABLED(CONFIG_ARM) && !IS_ENABLED(CONFIG_IOMMU_DMA)
+ ? generic_device_group : ipmmu_find_group,
.pgsize_bitmap = SZ_1G | SZ_2M | SZ_4K,
.of_xlate = ipmmu_of_xlate,
};
--
2.26.0
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/arm-smmu-v3: Don't reserve implementation defined register space
> On May 6, 2020, at 10:46 AM, Jean-Philippe Brucker
> wrote:
>
> Some SMMUv3 implementation embed the Perf Monitor Group Registers (PMCG)
> inside the first 64kB region of the SMMU. Since PMCG are managed by a
> separate driver, this layout causes resource reservation conflicts
> during boot.
>
> To avoid this conflict, only reserve the MMIO region we actually use:
> the first 0xe0 bytes of page 0 and the first 0xd0 bytes of page 1.
> Although devm_ioremap() still works on full pages under the hood, this
> way we benefit from resource conflict checks.
>
> Signed-off-by: Jean-Philippe Brucker
> ---
> A nicer (and hopefully working) solution to the problem dicussed here:
> https://lore.kernel.org/linux-iommu/20200421155745.19815-1-jean-phili...@linaro.org/
> ---
> drivers/iommu/arm-smmu-v3.c | 50 +
> 1 file changed, 45 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
> index 82508730feb7a1..fc85cdd5b62cca 100644
> --- a/drivers/iommu/arm-smmu-v3.c
> +++ b/drivers/iommu/arm-smmu-v3.c
> @@ -171,6 +171,9 @@
> #define ARM_SMMU_PRIQ_IRQ_CFG10xd8
> #define ARM_SMMU_PRIQ_IRQ_CFG20xdc
>
> +#define ARM_SMMU_PAGE0_REG_SZ0xe0
> +#define ARM_SMMU_PAGE1_REG_SZ0xd0
> +
> /* Common MSI config fields */
> #define MSI_CFG0_ADDR_MASKGENMASK_ULL(51, 2)
> #define MSI_CFG2_SH GENMASK(5, 4)
> @@ -628,6 +631,7 @@ struct arm_smmu_strtab_cfg {
> struct arm_smmu_device {
> struct device *dev;
> void __iomem*base;
> + void __iomem*page1;
>
> #define ARM_SMMU_FEAT_2_LVL_STRTAB(1 << 0)
> #define ARM_SMMU_FEAT_2_LVL_CDTAB (1 << 1)
> @@ -733,11 +737,14 @@ static struct arm_smmu_option_prop arm_smmu_options[] =
> {
> static inline void __iomem *arm_smmu_page1_fixup(unsigned long offset,
>struct arm_smmu_device *smmu)
> {
> - if ((offset > SZ_64K) &&
> - (smmu->options & ARM_SMMU_OPT_PAGE0_REGS_ONLY))
> - offset -= SZ_64K;
> + void __iomem *base = smmu->base;
>
> - return smmu->base + offset;
> + if (offset > SZ_64K) {
> + offset -= SZ_64K;
> + if (smmu->page1)
> + base = smmu->page1;
> + }
> + return base + offset;
> }
>
> static struct arm_smmu_domain *to_smmu_domain(struct iommu_domain *dom)
> @@ -4021,6 +4028,28 @@ err_reset_pci_ops: __maybe_unused;
> return err;
> }
>
> +static void __iomem *arm_smmu_ioremap(struct device *dev,
> + resource_size_t start,
> + resource_size_t size)
> +{
> + void __iomem *dest_ptr;
> + struct resource *res;
> +
> + res = devm_request_mem_region(dev, start, size, dev_name(dev));
> + if (!res) {
> + dev_err(dev, "can't request SMMU region %pa\n", );
> + return IOMEM_ERR_PTR(-EINVAL);
> + }
> +
> + dest_ptr = devm_ioremap(dev, start, size);
> + if (!dest_ptr) {
> + dev_err(dev, "ioremap failed for SMMU region %pR\n", res);
> + devm_release_mem_region(dev, start, size);
> + dest_ptr = IOMEM_ERR_PTR(-ENOMEM);
> + }
> + return dest_ptr;
> +}
> +
> static int arm_smmu_device_probe(struct platform_device *pdev)
> {
> int irq, ret;
> @@ -4056,10 +4085,21 @@ static int arm_smmu_device_probe(struct
> platform_device *pdev)
> }
> ioaddr = res->start;
>
> - smmu->base = devm_ioremap_resource(dev, res);
> + /*
> + * Only map what we need, because the IMPLEMENTATION DEFINED registers
> + * may be used for the PMCGs, which are reserved by the PMU driver.
> + */
> + smmu->base = arm_smmu_ioremap(dev, ioaddr, ARM_SMMU_PAGE0_REG_SZ);
> if (IS_ERR(smmu->base))
> return PTR_ERR(smmu->base);
>
> + if (arm_smmu_resource_size(smmu) > SZ_64K) {
> + smmu->page1 = arm_smmu_ioremap(dev, ioaddr + SZ_64K,
> +ARM_SMMU_PAGE1_REG_SZ);
> + if (IS_ERR(smmu->page1))
> + return PTR_ERR(smmu->page1);
> + }
> +
> /* Interrupt lines */
>
> irq = platform_get_irq_byname_optional(pdev, "combined");
> —
> 2.26.2
>
Tested-by: Tuan Phan mailto:tuanp...@os.amperecomputing.com>>
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/iova: Retry from last rb tree node if iova search fails
On 5/7/2020 6:54 PM, Robin Murphy wrote:
> On 2020-05-06 9:01 pm, vji...@codeaurora.org wrote:
>> From: Vijayanand Jitta
>>
>> When ever a new iova alloc request comes iova is always searched
>> from the cached node and the nodes which are previous to cached
>> node. So, even if there is free iova space available in the nodes
>> which are next to the cached node iova allocation can still fail
>> because of this approach.
>>
>> Consider the following sequence of iova alloc and frees on
>> 1GB of iova space
>>
>> 1) alloc - 500MB
>> 2) alloc - 12MB
>> 3) alloc - 499MB
>> 4) free - 12MB which was allocated in step 2
>> 5) alloc - 13MB
>>
>> After the above sequence we will have 12MB of free iova space and
>> cached node will be pointing to the iova pfn of last alloc of 13MB
>> which will be the lowest iova pfn of that iova space. Now if we get an
>> alloc request of 2MB we just search from cached node and then look
>> for lower iova pfn's for free iova and as they aren't any, iova alloc
>> fails though there is 12MB of free iova space.
>
> Yup, this could definitely do with improving. Unfortunately I think this
> particular implementation is slightly flawed...
>
>> To avoid such iova search failures do a retry from the last rb tree node
>> when iova search fails, this will search the entire tree and get an iova
>> if its available
>>
>> Signed-off-by: Vijayanand Jitta
>> ---
>> drivers/iommu/iova.c | 11 +++
>> 1 file changed, 11 insertions(+)
>>
>> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
>> index 0e6a953..2985222 100644
>> --- a/drivers/iommu/iova.c
>> +++ b/drivers/iommu/iova.c
>> @@ -186,6 +186,7 @@ static int __alloc_and_insert_iova_range(struct
>> iova_domain *iovad,
>> unsigned long flags;
>> unsigned long new_pfn;
>> unsigned long align_mask = ~0UL;
>> + bool retry = false;
>> if (size_aligned)
>> align_mask <<= fls_long(size - 1);
>> @@ -198,6 +199,8 @@ static int __alloc_and_insert_iova_range(struct
>> iova_domain *iovad,
>> curr = __get_cached_rbnode(iovad, limit_pfn);
>> curr_iova = rb_entry(curr, struct iova, node);
>> +
>> +retry_search:
>> do {
>> limit_pfn = min(limit_pfn, curr_iova->pfn_lo);
>> new_pfn = (limit_pfn - size) & align_mask;
>> @@ -207,6 +210,14 @@ static int __alloc_and_insert_iova_range(struct
>> iova_domain *iovad,
>> } while (curr && new_pfn <= curr_iova->pfn_hi);
>> if (limit_pfn < size || new_pfn < iovad->start_pfn) {
>> + if (!retry) {
>> + curr = rb_last(>rbroot);
>
> Why walk when there's an anchor node there already? However...
>
>> + curr_iova = rb_entry(curr, struct iova, node);
>> + limit_pfn = curr_iova->pfn_lo;
>
> ...this doesn't look right, as by now we've lost the original limit_pfn
> supplied by the caller, so are highly likely to allocate beyond the
> range our caller asked for. In fact AFAICS we'd start allocating from
> directly directly below the anchor node, beyond the end of the entire
> address space.
>
> The logic I was imagining we want here was something like the rapidly
> hacked up (and untested) diff below.
>
> Thanks,
> Robin.
>
Thanks for your comments ,I have gone through below logic and I see some
issue with retry check as there could be case where alloc_lo is set to
some pfn other than start_pfn in that case we don't retry and there can
still be iova available. I understand its a hacked up version, I can
work on this.
But how about we just store limit_pfn and get the node using that and
retry for once from that node, it would be similar to my patch just
correcting the curr node and limit_pfn update in retry check. do you see
any issue with this approach ?
Thanks,
Vijay.
> ->8-
> diff --git a/drivers/iommu/iova.c b/drivers/iommu/iova.c
> index 0e6a9536eca6..3574c19272d6 100644
> --- a/drivers/iommu/iova.c
> +++ b/drivers/iommu/iova.c
> @@ -186,6 +186,7 @@ static int __alloc_and_insert_iova_range(struct
> iova_domain *iovad,
> unsigned long flags;
> unsigned long new_pfn;
> unsigned long align_mask = ~0UL;
> + unsigned long alloc_hi, alloc_lo;
>
> if (size_aligned)
> align_mask <<= fls_long(size - 1);
> @@ -196,17 +197,27 @@ static int __alloc_and_insert_iova_range(struct
> iova_domain *iovad,
> size >= iovad->max32_alloc_size)
> goto iova32_full;
>
> + alloc_hi = IOVA_ANCHOR;
> + alloc_lo = iovad->start_pfn;
> +retry:
> curr = __get_cached_rbnode(iovad, limit_pfn);
> curr_iova = rb_entry(curr, struct iova, node);
> + if (alloc_hi < curr_iova->pfn_hi) {
> + alloc_lo = curr_iova->pfn_hi;
> + alloc_hi = limit_pfn;
> + }
> +
> do {
> - limit_pfn = min(limit_pfn, curr_iova->pfn_lo);
> - new_pfn = (limit_pfn - size) & align_mask;
> + alloc_hi = min(alloc_hi,
Re: [PATCH v5] iommu/arm-smmu-qcom: Request direct mapping for modem device
Hey Stephen, Thanks for taking time to review the patch. On 5/8/20 2:44 AM, Stephen Boyd wrote: Quoting Sibi Sankar (2020-05-07 12:21:57) The modem remote processor has two modes of access to the DDR, a direct mode and through a SMMU which requires direct mapping. The configuration of the modem SIDs is handled in TrustZone. Is it "The configuration of the modem SIDs is typically handled by code running in the ARM CPU's secure mode, i.e. secure EL1"? And is that even true? I though it was programmed by EL2. What I meant to say was qcom implementation of TZ or qcom proprietary bootloaders. I should have been more specific and mentioned that the configuration is done at EL2 by QHEE (Qualcomm's Hypervisor Execution Environment) before we enter the kernel. On platforms where TrustZone TrustZone is always there. is absent this needs to be explicitly done from kernel. Add compatibles for modem to opt in for direct mapping on such platforms. Signed-off-by: Sai Prakash Ranjan Is Sai the author? Or does this need a co-developed-by tag? I decided to include Sai's S-b just to show I took back ownership of the patch from his patch series. I'll drop it in the next re-spin. Signed-off-by: Sibi Sankar --- V5 * Reword commit message and drop unnecessary details I don't see any improvement! Probably because I don't understand _why_ the modem needs a direct mapping. The commit text basically says "we need to do it because it isn't done in secure world sometimes". This is probably wrong what I wrote below, but I'd like to clarify to confirm my understanding. Maybe the commit text should say: Thanks for taking time to reword the commit message will use the same with a few corrections. The modem remote processor has two access paths to DDR. One path is directly connected to DDR and another path goes through an SMMU. The SMMU path is configured to be a direct mapping because it's used by various peripherals in the modem subsystem. I'll use ^^ as is. Typically this direct mapping is configured by programming modem SIDs into the SMMU when EL2 responds to a hyp call from the code that loads the modem binary in the secure world. Typically this direct mapping is configured statically at EL2 by QHEE (Qualcomm's Hypervisor Execution Environment) before the kernel is entered. In certain firmware configurations, especially when the kernel is entered at EL2, we don't want secure mode to make hyp calls to program the SMMU because the kernel is in full control of the SMMU. Let's add compatibles here so that we can have the kernel program the SIDs for the modem in these cases. In certain firmware configuration, especially when the kernel is already in full control of the SMMU, defer programming the modem SIDs to the kernel. Let's add compatibles here so that we can have the kernel program the SIDs for the modem in these cases. Will/Stephen, Let me know if the above changes are okay? I'll re-spin the patch based on your feedback. -- Qualcomm Innovation Center, Inc. Qualcomm Innovation Center, Inc, is a member of Code Aurora Forum, a Linux Foundation Collaborative Project ___ iommu mailing list iommu@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iomm/arm-smmu: Add stall implementation hook
On Fri, May 8, 2020 at 8:32 AM Rob Clark wrote:
>
> On Thu, May 7, 2020 at 5:54 AM Will Deacon wrote:
> >
> > On Thu, May 07, 2020 at 11:55:54AM +0100, Robin Murphy wrote:
> > > On 2020-05-07 11:14 am, Sai Prakash Ranjan wrote:
> > > > On 2020-04-22 01:50, Sai Prakash Ranjan wrote:
> > > > > Add stall implementation hook to enable stalling
> > > > > faults on QCOM platforms which supports it without
> > > > > causing any kind of hardware mishaps. Without this
> > > > > on QCOM platforms, GPU faults can cause unrelated
> > > > > GPU memory accesses to return zeroes. This has the
> > > > > unfortunate result of command-stream reads from CP
> > > > > getting invalid data, causing a cascade of fail.
> > >
> > > I think this came up before, but something about this rationale doesn't
> > > add
> > > up - we're not *using* stalls at all, we're still terminating faulting
> > > transactions unconditionally; we're just using CFCFG to terminate them
> > > with
> > > a slight delay, rather than immediately. It's really not clear how or why
> > > that makes a difference. Is it a GPU bug? Or an SMMU bug? Is this reliable
> > > (or even a documented workaround for something), or might things start
> > > blowing up again if any other behaviour subtly changes? I'm not dead set
> > > against adding this, but I'd *really* like to have a lot more confidence
> > > in
> > > it.
> >
> > Rob mentioned something about the "bus returning zeroes" before, but I agree
> > that we need more information so that we can reason about this and maintain
> > the code as the driver continues to change. That needs to be a comment in
> > the driver, and I don't think "but android seems to work" is a good enough
> > justification. There was some interaction with HUPCF as well.
>
> The issue is that there are multiple parallel memory accesses
> happening at the same time, for example CP (the cmdstream processor)
> will be reading ahead and setting things up for the next draw or
> compute grid, in parallel with some memory accesses from the shader
> which could trigger a fault. (And with faults triggered by something
> in the shader, there are *many* shader threads running in parallel so
> those tend to generate a big number of faults at the same time.)
>
> We need either CFCFG or HUPCF, otherwise what I have observed is that
> while the fault happens, CP's memory access will start returning
> zero's instead of valid cmdstream data, which triggers a GPU hang. I
> can't say whether this is something unique to qcom's implementation of
> the smmu spec or not.
>
> *Often* a fault is the result of the usermode gl/vk/cl driver bug,
> although I don't think that is an argument against fixing this in the
> smmu driver.. I've been carrying around a local patch to set HUPCF for
> *years* because debugging usermode driver issues is so much harder
> without. But there are some APIs where faults can be caused by the
> user's app on top of the usermode driver.
>
Also, I'll add to that, a big wish of mine is to have stall with the
ability to resume later from a wq context. That would enable me to
hook in the gpu crash dump handling for faults, which would make
debugging these sorts of issues much easier. I think I posted a
prototype of this quite some time back, which would schedule a worker
on the first fault (since there are cases where you see 1000's of
faults at once), which grabbed some information about the currently
executing submit and some gpu registers to indicate *where* in the
submit (a single submit could have 100's or 1000's of draws), and then
resumed the iommu cb.
(This would ofc eventually be useful for svm type things.. I expect
we'll eventually care about that too.)
BR,
-R
>
> BR,
> -R
>
> >
> > As a template, I'd suggest:
> >
> > > > > diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> > > > > index 8d1cd54d82a6..d5134e0d5cce 100644
> > > > > --- a/drivers/iommu/arm-smmu.h
> > > > > +++ b/drivers/iommu/arm-smmu.h
> > > > > @@ -386,6 +386,7 @@ struct arm_smmu_impl {
> > > > > int (*init_context)(struct arm_smmu_domain *smmu_domain);
> > > > > void (*tlb_sync)(struct arm_smmu_device *smmu, int page, int
> > > > > sync,
> > > > > int status);
> >
> > /*
> > * Stall transactions on a context fault, where they will be terminated
> > * in response to the resulting IRQ rather than immediately. This should
> > * pretty much always be set to "false" as stalling can introduce the
> > * potential for deadlock in most SoCs, however it is needed on Qualcomm
> > * because .
> > */
> >
> > > > > +bool stall;
> >
> > Hmm, the more I think about this, the more I think this is an erratum
> > workaround in disguise, in which case this could be better named...
> >
> > Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iomm/arm-smmu: Add stall implementation hook
On Thu, May 7, 2020 at 5:54 AM Will Deacon wrote:
>
> On Thu, May 07, 2020 at 11:55:54AM +0100, Robin Murphy wrote:
> > On 2020-05-07 11:14 am, Sai Prakash Ranjan wrote:
> > > On 2020-04-22 01:50, Sai Prakash Ranjan wrote:
> > > > Add stall implementation hook to enable stalling
> > > > faults on QCOM platforms which supports it without
> > > > causing any kind of hardware mishaps. Without this
> > > > on QCOM platforms, GPU faults can cause unrelated
> > > > GPU memory accesses to return zeroes. This has the
> > > > unfortunate result of command-stream reads from CP
> > > > getting invalid data, causing a cascade of fail.
> >
> > I think this came up before, but something about this rationale doesn't add
> > up - we're not *using* stalls at all, we're still terminating faulting
> > transactions unconditionally; we're just using CFCFG to terminate them with
> > a slight delay, rather than immediately. It's really not clear how or why
> > that makes a difference. Is it a GPU bug? Or an SMMU bug? Is this reliable
> > (or even a documented workaround for something), or might things start
> > blowing up again if any other behaviour subtly changes? I'm not dead set
> > against adding this, but I'd *really* like to have a lot more confidence in
> > it.
>
> Rob mentioned something about the "bus returning zeroes" before, but I agree
> that we need more information so that we can reason about this and maintain
> the code as the driver continues to change. That needs to be a comment in
> the driver, and I don't think "but android seems to work" is a good enough
> justification. There was some interaction with HUPCF as well.
The issue is that there are multiple parallel memory accesses
happening at the same time, for example CP (the cmdstream processor)
will be reading ahead and setting things up for the next draw or
compute grid, in parallel with some memory accesses from the shader
which could trigger a fault. (And with faults triggered by something
in the shader, there are *many* shader threads running in parallel so
those tend to generate a big number of faults at the same time.)
We need either CFCFG or HUPCF, otherwise what I have observed is that
while the fault happens, CP's memory access will start returning
zero's instead of valid cmdstream data, which triggers a GPU hang. I
can't say whether this is something unique to qcom's implementation of
the smmu spec or not.
*Often* a fault is the result of the usermode gl/vk/cl driver bug,
although I don't think that is an argument against fixing this in the
smmu driver.. I've been carrying around a local patch to set HUPCF for
*years* because debugging usermode driver issues is so much harder
without. But there are some APIs where faults can be caused by the
user's app on top of the usermode driver.
BR,
-R
>
> As a template, I'd suggest:
>
> > > > diff --git a/drivers/iommu/arm-smmu.h b/drivers/iommu/arm-smmu.h
> > > > index 8d1cd54d82a6..d5134e0d5cce 100644
> > > > --- a/drivers/iommu/arm-smmu.h
> > > > +++ b/drivers/iommu/arm-smmu.h
> > > > @@ -386,6 +386,7 @@ struct arm_smmu_impl {
> > > > int (*init_context)(struct arm_smmu_domain *smmu_domain);
> > > > void (*tlb_sync)(struct arm_smmu_device *smmu, int page, int sync,
> > > > int status);
>
> /*
> * Stall transactions on a context fault, where they will be terminated
> * in response to the resulting IRQ rather than immediately. This should
> * pretty much always be set to "false" as stalling can introduce the
> * potential for deadlock in most SoCs, however it is needed on Qualcomm
> * because .
> */
>
> > > > +bool stall;
>
> Hmm, the more I think about this, the more I think this is an erratum
> workaround in disguise, in which case this could be better named...
>
> Will
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH] iommu/virtio: reverse arguments to list_add
On Tue, May 05, 2020 at 08:47:47PM +0200, Julia Lawall wrote:
> Elsewhere in the file, there is a list_for_each_entry with
> >resv_regions as the second argument, suggesting that
> >resv_regions is the list head. So exchange the
> arguments on the list_add call to put the list head in the
> second argument.
>
> Fixes: 2a5a31487445 ("iommu/virtio: Add probe request")
> Signed-off-by: Julia Lawall
Applied for v5.7, thanks.
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH -next] iommu/amd: Remove set but not used variable 'iommu'
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/iommu/amd_iommu.c: In function 'amd_iommu_uninit_device':
drivers/iommu/amd_iommu.c:422:20: warning:
variable 'iommu' set but not used [-Wunused-but-set-variable]
commit dce8d6964ebd ("iommu/amd: Convert to probe/release_device() call-backs")
involved this, remove it.
Signed-off-by: YueHaibing
---
drivers/iommu/amd_iommu.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/iommu/amd_iommu.c b/drivers/iommu/amd_iommu.c
index fef3689ee535..2b8eb58d2bea 100644
--- a/drivers/iommu/amd_iommu.c
+++ b/drivers/iommu/amd_iommu.c
@@ -419,15 +419,12 @@ static void iommu_ignore_device(struct device *dev)
static void amd_iommu_uninit_device(struct device *dev)
{
struct iommu_dev_data *dev_data;
- struct amd_iommu *iommu;
int devid;
devid = get_device_id(dev);
if (devid < 0)
return;
- iommu = amd_iommu_rlookup_table[devid];
-
dev_data = search_dev_data(devid);
if (!dev_data)
return;
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH -next] iommu: remove set but not used variable 'data'
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/iommu/mtk_iommu_v1.c:467:25:
warning: variable ‘data’ set but not used [-Wunused-but-set-variable]
struct mtk_iommu_data *data;
Reported-by: Hulk Robot
Signed-off-by: Chen Zhou
---
drivers/iommu/mtk_iommu_v1.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index 7bdd74c7cb9f..36cc1d9667a2 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -464,12 +464,11 @@ static void mtk_iommu_probe_finalize(struct device *dev)
static void mtk_iommu_release_device(struct device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
- struct mtk_iommu_data *data;
if (!fwspec || fwspec->ops != _iommu_ops)
return;
- data = dev_iommu_priv_get(dev);
+ dev_iommu_priv_get(dev);
iommu_fwspec_free(dev);
}
--
2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
[PATCH -next] iommu/arm-smmu-v3: remove set but not used variable 'smmu'
Fixes gcc '-Wunused-but-set-variable' warning:
drivers/iommu/arm-smmu-v3.c:2989:26:
warning: variable ‘smmu’ set but not used [-Wunused-but-set-variable]
struct arm_smmu_device *smmu;
Reported-by: Hulk Robot
Signed-off-by: Chen Zhou
---
drivers/iommu/arm-smmu-v3.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/drivers/iommu/arm-smmu-v3.c b/drivers/iommu/arm-smmu-v3.c
index 42e1ee7e5197..89ee9c5d8b88 100644
--- a/drivers/iommu/arm-smmu-v3.c
+++ b/drivers/iommu/arm-smmu-v3.c
@@ -2986,13 +2986,11 @@ static void arm_smmu_release_device(struct device *dev)
{
struct iommu_fwspec *fwspec = dev_iommu_fwspec_get(dev);
struct arm_smmu_master *master;
- struct arm_smmu_device *smmu;
if (!fwspec || fwspec->ops != _smmu_ops)
return;
master = dev_iommu_priv_get(dev);
- smmu = master->smmu;
arm_smmu_detach_dev(master);
arm_smmu_disable_pasid(master);
kfree(master);
--
2.20.1
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/25] drm: core: fix common struct sg_table related issues
On Fri, May 08, 2020 at 09:12:13AM +0200, Marek Szyprowski wrote:
> Then we would just need one more helper to construct scatterlist, as the
> above two are read-only don't allow to modify scatterlist:
>
> #define for_each_sgtable_sg(sgt, sg, i) \
> for_each_sg(sgt->sgl, sg, sgt->orig_nents, i)
>
> With the above 3 helpers we can probably get rid of all instances of
> sg_table->{nents,orig_nents} from the DRM code. I will prepare patches soon.
Sounds great, thanks!
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
Re: [PATCH v3 02/25] drm: core: fix common struct sg_table related issues
Hi Christoph,
On 05.05.2020 13:09, Christoph Hellwig wrote:
> On Tue, May 05, 2020 at 12:51:58PM +0200, Marek Szyprowski wrote:
>> On 05.05.2020 12:15, Christoph Hellwig wrote:
- for_each_sg_page(st->sgl, _iter, st->nents, 0)
+ for_each_sg_page(st->sgl, _iter, st->orig_nents, 0)
>>> Would it make sense to also add a for_each_sgtable_page helper that
>>> hides the use of orig_nents? To be used like:
>>>
>>> for_each_sgtable_page(st, _iter, 0) {
>> We would need two helpers:
>>
>> for_each_sgtable_cpu_page() and for_each_sgtable_dma_page().
>>
>> I considered them, but then I found that there are already
>> for_each_sg_page(), for_each_sg_dma_page() and various special iterators
>> like sg_page_iter, sg_dma_page_iter and sg_mapping_iter. Too bad that
>> they are almost not used, at least in the DRM subsystem. I wonder if it
>> make sense to apply them or simply provide the two above mentioned
>> wrappers?
> None of the helpers helps with passing the right parameters from the
> sg_table. So in doube we'd need wrappers for all of the above, or
> none..
I've played a bit with the code and the existing scatterlist iterators -
for_each_sg_page() and for_each_sg_dma_page(). I've found them quite handy!
The biggest advantage of them is that they always iterate over
scatterlist in PAGE_SIZE units, what should make the code much easier to
understand. Here is example of their application to the function that
started this thread:
int drm_prime_sg_to_page_addr_arrays(struct sg_table *sgt, struct page
**pages,
dma_addr_t *addrs, int max_entries)
{
struct sg_dma_page_iter dma_iter;
struct sg_page_iter page_iter;
if (addrs)
for_each_sgtable_dma_page(sgt, _iter, 0)
*addrs++ = sg_page_iter_dma_address(_iter);
if (pages)
for_each_sgtable_page(sgt, _iter, 0)
*pages++ = sg_page_iter_page(_iter);
return 0;
}
After applying those iterators where possible (they can be used only for
reading the scatterlist), we would just need to add 2 trivial wrappers
to use them with sg_table objects:
#define for_each_sgtable_page(sgt, piter, pgoffset) \
for_each_sg_page(sgt->sgl, piter, sgt->orig_nents, pgoffset)
#define for_each_sgtable_dma_page(sgt, dma_iter, pgoffset) \
for_each_sg_dma_page(sgt->sgl, dma_iter, sgt->nents, pgoffset)
Then we would just need one more helper to construct scatterlist, as the
above two are read-only don't allow to modify scatterlist:
#define for_each_sgtable_sg(sgt, sg, i) \
for_each_sg(sgt->sgl, sg, sgt->orig_nents, i)
With the above 3 helpers we can probably get rid of all instances of
sg_table->{nents,orig_nents} from the DRM code. I will prepare patches soon.
Best regards
--
Marek Szyprowski, PhD
Samsung R Institute Poland
___
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
