From: Yong Wu <yong.wu@mediatek.com>
To: Will Deacon <will@kernel.org>
Cc: Matthias Brugger <matthias.bgg@gmail.com>,
Joerg Roedel <joro@8bytes.org>, Will Deacon <will.deacon@arm.com>,
Evan Green <evgreen@chromium.org>,
Robin Murphy <robin.murphy@arm.com>,
Tomasz Figa <tfiga@google.com>,
<linux-mediatek@lists.infradead.org>,
<srv_heupstream@mediatek.com>, <linux-kernel@vger.kernel.org>,
<linux-arm-kernel@lists.infradead.org>,
<iommu@lists.linux-foundation.org>, <youlin.pei@mediatek.com>,
Nicolas Boichat <drinkcat@chromium.org>, <anan.sun@mediatek.com>,
<cui.zhang@mediatek.com>, <chao.hao@mediatek.com>,
<edison.hsieh@mediatek.com>
Subject: Re: [PATCH v4 2/7] iommu/mediatek: Add a new tlb_lock for tlb_flush
Date: Thu, 24 Oct 2019 15:22:26 +0800 [thread overview]
Message-ID: <1571901746.19130.134.camel@mhfsdcap03> (raw)
In-Reply-To: <20191023165252.GA27471@willie-the-truck>
On Wed, 2019-10-23 at 17:52 +0100, Will Deacon wrote:
> On Wed, Oct 16, 2019 at 11:33:07AM +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 lock, 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
> >
> > The HW requires tlb_flush/tlb_sync in pairs strictly, this patch adds
> > a new tlb_lock for tlb operations to fix this issue.
> >
> > Fixes: 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API TLB
> > sync")
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> > drivers/iommu/mtk_iommu.c | 23 ++++++++++++++++++++++-
> > drivers/iommu/mtk_iommu.h | 1 +
> > 2 files changed, 23 insertions(+), 1 deletion(-)
>
> [...]
>
> > static void mtk_iommu_tlb_flush_page_nosync(struct iommu_iotlb_gather *gather,
> > unsigned long iova, size_t granule,
> > void *cookie)
> > {
> > + struct mtk_iommu_data *data = cookie;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&data->tlb_lock, flags);
> > mtk_iommu_tlb_add_flush_nosync(iova, granule, granule, true, cookie);
> > + spin_unlock_irqrestore(&data->tlb_lock, flags);
>
> Given that you release the lock here, what prevents another nosync()
> operation being issued before you've managed to do the sync()?
This patch can not guarantee each flush_nosync() always followed by a
sync().
The current mainline don't guarantee this too(Current v7s call
flush_nosync 16 times, then call tlb_sync 1 time in the
supersection/largepage case.). At this situation, it don't guarantee the
previous tlb_flushes are finished, But we never got the timeout
issue(Fortunately our HW always work well at that time. Maybe the HW
finish the previous tlb flush so quickly even though we don't polling
its finish flag).
We got the timeout issue only after this commit 4d689b619445. This patch
only fixes this issue via adding a new lock in iotlb_sync.(It don't
adjust the sequence of flush_nosync() and sync()).
>
> Will
WARNING: multiple messages have this Message-ID (diff)
From: Yong Wu <yong.wu@mediatek.com>
To: Will Deacon <will@kernel.org>
Cc: youlin.pei@mediatek.com, anan.sun@mediatek.com,
Nicolas Boichat <drinkcat@chromium.org>,
cui.zhang@mediatek.com, srv_heupstream@mediatek.com,
chao.hao@mediatek.com, edison.hsieh@mediatek.com,
Will Deacon <will.deacon@arm.com>,
linux-kernel@vger.kernel.org, Evan Green <evgreen@chromium.org>,
Tomasz Figa <tfiga@google.com>,
iommu@lists.linux-foundation.org,
linux-mediatek@lists.infradead.org,
Matthias Brugger <matthias.bgg@gmail.com>,
Robin Murphy <robin.murphy@arm.com>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4 2/7] iommu/mediatek: Add a new tlb_lock for tlb_flush
Date: Thu, 24 Oct 2019 15:22:26 +0800 [thread overview]
Message-ID: <1571901746.19130.134.camel@mhfsdcap03> (raw)
In-Reply-To: <20191023165252.GA27471@willie-the-truck>
On Wed, 2019-10-23 at 17:52 +0100, Will Deacon wrote:
> On Wed, Oct 16, 2019 at 11:33:07AM +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 lock, 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
> >
> > The HW requires tlb_flush/tlb_sync in pairs strictly, this patch adds
> > a new tlb_lock for tlb operations to fix this issue.
> >
> > Fixes: 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API TLB
> > sync")
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> > drivers/iommu/mtk_iommu.c | 23 ++++++++++++++++++++++-
> > drivers/iommu/mtk_iommu.h | 1 +
> > 2 files changed, 23 insertions(+), 1 deletion(-)
>
> [...]
>
> > static void mtk_iommu_tlb_flush_page_nosync(struct iommu_iotlb_gather *gather,
> > unsigned long iova, size_t granule,
> > void *cookie)
> > {
> > + struct mtk_iommu_data *data = cookie;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&data->tlb_lock, flags);
> > mtk_iommu_tlb_add_flush_nosync(iova, granule, granule, true, cookie);
> > + spin_unlock_irqrestore(&data->tlb_lock, flags);
>
> Given that you release the lock here, what prevents another nosync()
> operation being issued before you've managed to do the sync()?
This patch can not guarantee each flush_nosync() always followed by a
sync().
The current mainline don't guarantee this too(Current v7s call
flush_nosync 16 times, then call tlb_sync 1 time in the
supersection/largepage case.). At this situation, it don't guarantee the
previous tlb_flushes are finished, But we never got the timeout
issue(Fortunately our HW always work well at that time. Maybe the HW
finish the previous tlb flush so quickly even though we don't polling
its finish flag).
We got the timeout issue only after this commit 4d689b619445. This patch
only fixes this issue via adding a new lock in iotlb_sync.(It don't
adjust the sequence of flush_nosync() and sync()).
>
> Will
_______________________________________________
iommu mailing list
iommu@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/iommu
WARNING: multiple messages have this Message-ID (diff)
From: Yong Wu <yong.wu@mediatek.com>
To: Will Deacon <will@kernel.org>
Cc: youlin.pei@mediatek.com, anan.sun@mediatek.com,
Nicolas Boichat <drinkcat@chromium.org>,
cui.zhang@mediatek.com, srv_heupstream@mediatek.com,
chao.hao@mediatek.com, Joerg Roedel <joro@8bytes.org>,
edison.hsieh@mediatek.com, Will Deacon <will.deacon@arm.com>,
linux-kernel@vger.kernel.org, Evan Green <evgreen@chromium.org>,
Tomasz Figa <tfiga@google.com>,
iommu@lists.linux-foundation.org,
linux-mediatek@lists.infradead.org,
Matthias Brugger <matthias.bgg@gmail.com>,
Robin Murphy <robin.murphy@arm.com>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4 2/7] iommu/mediatek: Add a new tlb_lock for tlb_flush
Date: Thu, 24 Oct 2019 15:22:26 +0800 [thread overview]
Message-ID: <1571901746.19130.134.camel@mhfsdcap03> (raw)
In-Reply-To: <20191023165252.GA27471@willie-the-truck>
On Wed, 2019-10-23 at 17:52 +0100, Will Deacon wrote:
> On Wed, Oct 16, 2019 at 11:33:07AM +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 lock, 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
> >
> > The HW requires tlb_flush/tlb_sync in pairs strictly, this patch adds
> > a new tlb_lock for tlb operations to fix this issue.
> >
> > Fixes: 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API TLB
> > sync")
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> > drivers/iommu/mtk_iommu.c | 23 ++++++++++++++++++++++-
> > drivers/iommu/mtk_iommu.h | 1 +
> > 2 files changed, 23 insertions(+), 1 deletion(-)
>
> [...]
>
> > static void mtk_iommu_tlb_flush_page_nosync(struct iommu_iotlb_gather *gather,
> > unsigned long iova, size_t granule,
> > void *cookie)
> > {
> > + struct mtk_iommu_data *data = cookie;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&data->tlb_lock, flags);
> > mtk_iommu_tlb_add_flush_nosync(iova, granule, granule, true, cookie);
> > + spin_unlock_irqrestore(&data->tlb_lock, flags);
>
> Given that you release the lock here, what prevents another nosync()
> operation being issued before you've managed to do the sync()?
This patch can not guarantee each flush_nosync() always followed by a
sync().
The current mainline don't guarantee this too(Current v7s call
flush_nosync 16 times, then call tlb_sync 1 time in the
supersection/largepage case.). At this situation, it don't guarantee the
previous tlb_flushes are finished, But we never got the timeout
issue(Fortunately our HW always work well at that time. Maybe the HW
finish the previous tlb flush so quickly even though we don't polling
its finish flag).
We got the timeout issue only after this commit 4d689b619445. This patch
only fixes this issue via adding a new lock in iotlb_sync.(It don't
adjust the sequence of flush_nosync() and sync()).
>
> Will
_______________________________________________
Linux-mediatek mailing list
Linux-mediatek@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-mediatek
WARNING: multiple messages have this Message-ID (diff)
From: Yong Wu <yong.wu@mediatek.com>
To: Will Deacon <will@kernel.org>
Cc: youlin.pei@mediatek.com, anan.sun@mediatek.com,
Nicolas Boichat <drinkcat@chromium.org>,
cui.zhang@mediatek.com, srv_heupstream@mediatek.com,
chao.hao@mediatek.com, Joerg Roedel <joro@8bytes.org>,
edison.hsieh@mediatek.com, Will Deacon <will.deacon@arm.com>,
linux-kernel@vger.kernel.org, Evan Green <evgreen@chromium.org>,
Tomasz Figa <tfiga@google.com>,
iommu@lists.linux-foundation.org,
linux-mediatek@lists.infradead.org,
Matthias Brugger <matthias.bgg@gmail.com>,
Robin Murphy <robin.murphy@arm.com>,
linux-arm-kernel@lists.infradead.org
Subject: Re: [PATCH v4 2/7] iommu/mediatek: Add a new tlb_lock for tlb_flush
Date: Thu, 24 Oct 2019 15:22:26 +0800 [thread overview]
Message-ID: <1571901746.19130.134.camel@mhfsdcap03> (raw)
In-Reply-To: <20191023165252.GA27471@willie-the-truck>
On Wed, 2019-10-23 at 17:52 +0100, Will Deacon wrote:
> On Wed, Oct 16, 2019 at 11:33:07AM +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 lock, 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
> >
> > The HW requires tlb_flush/tlb_sync in pairs strictly, this patch adds
> > a new tlb_lock for tlb operations to fix this issue.
> >
> > Fixes: 4d689b619445 ("iommu/io-pgtable-arm-v7s: Convert to IOMMU API TLB
> > sync")
> > Signed-off-by: Yong Wu <yong.wu@mediatek.com>
> > ---
> > drivers/iommu/mtk_iommu.c | 23 ++++++++++++++++++++++-
> > drivers/iommu/mtk_iommu.h | 1 +
> > 2 files changed, 23 insertions(+), 1 deletion(-)
>
> [...]
>
> > static void mtk_iommu_tlb_flush_page_nosync(struct iommu_iotlb_gather *gather,
> > unsigned long iova, size_t granule,
> > void *cookie)
> > {
> > + struct mtk_iommu_data *data = cookie;
> > + unsigned long flags;
> > +
> > + spin_lock_irqsave(&data->tlb_lock, flags);
> > mtk_iommu_tlb_add_flush_nosync(iova, granule, granule, true, cookie);
> > + spin_unlock_irqrestore(&data->tlb_lock, flags);
>
> Given that you release the lock here, what prevents another nosync()
> operation being issued before you've managed to do the sync()?
This patch can not guarantee each flush_nosync() always followed by a
sync().
The current mainline don't guarantee this too(Current v7s call
flush_nosync 16 times, then call tlb_sync 1 time in the
supersection/largepage case.). At this situation, it don't guarantee the
previous tlb_flushes are finished, But we never got the timeout
issue(Fortunately our HW always work well at that time. Maybe the HW
finish the previous tlb flush so quickly even though we don't polling
its finish flag).
We got the timeout issue only after this commit 4d689b619445. This patch
only fixes this issue via adding a new lock in iotlb_sync.(It don't
adjust the sequence of flush_nosync() and sync()).
>
> Will
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2019-10-24 7:22 UTC|newest]
Thread overview: 48+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-16 3:33 [PATCH v4 0/7] Improve tlb range flush Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` [PATCH v4 1/7] iommu/mediatek: Correct the flush_iotlb_all callback Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` [PATCH v4 2/7] iommu/mediatek: Add a new tlb_lock for tlb_flush Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-23 16:52 ` Will Deacon
2019-10-23 16:52 ` Will Deacon
2019-10-23 16:52 ` Will Deacon
2019-10-23 16:52 ` Will Deacon
2019-10-24 7:22 ` Yong Wu [this message]
2019-10-24 7:22 ` Yong Wu
2019-10-24 7:22 ` Yong Wu
2019-10-24 7:22 ` Yong Wu
2019-10-16 3:33 ` [PATCH v4 3/7] iommu/mediatek: Use gather to achieve the tlb range flush Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-23 16:55 ` Will Deacon
2019-10-23 16:55 ` Will Deacon
2019-10-23 16:55 ` Will Deacon
2019-10-23 16:55 ` Will Deacon
2019-10-24 7:22 ` Yong Wu
2019-10-24 7:22 ` Yong Wu
2019-10-24 7:22 ` Yong Wu
2019-10-24 7:22 ` Yong Wu
2019-10-16 3:33 ` [PATCH v4 4/7] iommu/mediatek: Delete the leaf in the tlb_flush Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` [PATCH v4 5/7] iommu/mediatek: Move the tlb_sync into tlb_flush Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` [PATCH v4 6/7] iommu/mediatek: Get rid of the pgtlock Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` [PATCH v4 7/7] iommu/mediatek: Reduce the tlb flush timeout value Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
2019-10-16 3:33 ` Yong Wu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1571901746.19130.134.camel@mhfsdcap03 \
--to=yong.wu@mediatek.com \
--cc=anan.sun@mediatek.com \
--cc=chao.hao@mediatek.com \
--cc=cui.zhang@mediatek.com \
--cc=drinkcat@chromium.org \
--cc=edison.hsieh@mediatek.com \
--cc=evgreen@chromium.org \
--cc=iommu@lists.linux-foundation.org \
--cc=joro@8bytes.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=robin.murphy@arm.com \
--cc=srv_heupstream@mediatek.com \
--cc=tfiga@google.com \
--cc=will.deacon@arm.com \
--cc=will@kernel.org \
--cc=youlin.pei@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.