[03/13] iommu/mediatek: Add probe_defer for smi-larb
diff mbox series

Message ID 1546318276-18993-4-git-send-email-yong.wu@mediatek.com
State New
Headers show
Series
  • Clean up "mediatek,larb" after adding device_link
Related show

Commit Message

Yong Wu Jan. 1, 2019, 4:51 a.m. UTC
The iommu consumer should use device_link to connect with the
smi-larb(supplier). then the smi-larb should run before the iommu
consumer. Here we delay the iommu driver until the smi driver is
ready, then all the iommu consumer always is after the smi driver.

When there is no this patch, if some consumer drivers run before
smi-larb, the supplier link_status is DL_DEV_NO_DRIVER(0) in the
device_link_add, then device_links_driver_bound will use WARN_ON
to complain that the link_status of supplier is not right.

This is a preparing patch for adding device_link.

Signed-off-by: Yong Wu <yong.wu@mediatek.com>
---
 drivers/iommu/mtk_iommu.c    | 2 +-
 drivers/iommu/mtk_iommu_v1.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

Comments

Evan Green Feb. 25, 2019, 11:54 p.m. UTC | #1
On Mon, Dec 31, 2018 at 8:52 PM Yong Wu <yong.wu@mediatek.com> wrote:
>
> The iommu consumer should use device_link to connect with the
> smi-larb(supplier). then the smi-larb should run before the iommu
> consumer. Here we delay the iommu driver until the smi driver is
> ready, then all the iommu consumer always is after the smi driver.
>
> When there is no this patch, if some consumer drivers run before
> smi-larb, the supplier link_status is DL_DEV_NO_DRIVER(0) in the
> device_link_add, then device_links_driver_bound will use WARN_ON
> to complain that the link_status of supplier is not right.
>

I don't quite have enough knowledge of device links to figure out if
this is a) the right fix, b) a deficiency in the device_links code, or
c) neither.

Perhaps someone else could chime in on this one.
-Evan
Yong Wu Feb. 27, 2019, 2:33 p.m. UTC | #2
On Mon, 2019-02-25 at 15:54 -0800, Evan Green wrote:
> On Mon, Dec 31, 2018 at 8:52 PM Yong Wu <yong.wu@mediatek.com> wrote:
> >
> > The iommu consumer should use device_link to connect with the
> > smi-larb(supplier). then the smi-larb should run before the iommu
> > consumer. Here we delay the iommu driver until the smi driver is
> > ready, then all the iommu consumer always is after the smi driver.
> >
> > When there is no this patch, if some consumer drivers run before
> > smi-larb, the supplier link_status is DL_DEV_NO_DRIVER(0) in the
> > device_link_add, then device_links_driver_bound will use WARN_ON
> > to complain that the link_status of supplier is not right.
> >
> 
> I don't quite have enough knowledge of device links to figure out if
> this is a) the right fix, b) a deficiency in the device_links code, or
> c) neither.

I can not say more about the device link, But from the MTK iommu point
of view, it looks a right fix. As the comment above, we should make sure
the probe sequence, SMI-larb should be before MTK IOMMU which need be
before iommu-consumer. this patch help SMI-larb always is before
MTK_IOMMU.

Then if there is no this patchset, what the MTK_IOMMU will happen?.

The MTK_IOMMU is subsys_initcall, all the consume only need after the
MTK_IOMMU and don't need wait for SMI-larb driver. If some consume run
before SMI-larb driver, It also is OK while it don't call
mtk_smi_larb_get in this probe.(Of course it will abort if it call this
while smi-driver don't probe at that time). the consumer and smi-larb
normally are module_init, we can not guarantee the the sequence. it is a
potential issue.

Some consume know it should wait for SMI-larb like [1], but It don't
work.

[1]
https://elixir.bootlin.com/linux/v5.0-rc1/source/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c#L145

> 
> Perhaps someone else could chime in on this one.
> -Evan
> 
> _______________________________________________
> Linux-mediatek mailing list
> Linux-mediatek@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-mediatek
Evan Green March 5, 2019, 7:02 p.m. UTC | #3
On Wed, Feb 27, 2019 at 6:34 AM Yong Wu <yong.wu@mediatek.com> wrote:
>
> On Mon, 2019-02-25 at 15:54 -0800, Evan Green wrote:
> > On Mon, Dec 31, 2018 at 8:52 PM Yong Wu <yong.wu@mediatek.com> wrote:
> > >
> > > The iommu consumer should use device_link to connect with the
> > > smi-larb(supplier). then the smi-larb should run before the iommu
> > > consumer. Here we delay the iommu driver until the smi driver is
> > > ready, then all the iommu consumer always is after the smi driver.
> > >
> > > When there is no this patch, if some consumer drivers run before
> > > smi-larb, the supplier link_status is DL_DEV_NO_DRIVER(0) in the
> > > device_link_add, then device_links_driver_bound will use WARN_ON
> > > to complain that the link_status of supplier is not right.
> > >
> >
> > I don't quite have enough knowledge of device links to figure out if
> > this is a) the right fix, b) a deficiency in the device_links code, or
> > c) neither.
>
> I can not say more about the device link, But from the MTK iommu point
> of view, it looks a right fix. As the comment above, we should make sure
> the probe sequence, SMI-larb should be before MTK IOMMU which need be
> before iommu-consumer. this patch help SMI-larb always is before
> MTK_IOMMU.
>
> Then if there is no this patchset, what the MTK_IOMMU will happen?.
>
> The MTK_IOMMU is subsys_initcall, all the consume only need after the
> MTK_IOMMU and don't need wait for SMI-larb driver. If some consume run
> before SMI-larb driver, It also is OK while it don't call
> mtk_smi_larb_get in this probe.(Of course it will abort if it call this
> while smi-driver don't probe at that time). the consumer and smi-larb
> normally are module_init, we can not guarantee the the sequence. it is a
> potential issue.
>
> Some consume know it should wait for SMI-larb like [1], but It don't
> work.
>
> [1]
> https://elixir.bootlin.com/linux/v5.0-rc1/source/drivers/media/platform/mtk-mdp/mtk_mdp_comp.c#L145

Ok, I think I get it now. The device_links that get created are
enforcing a probe order dependency, but in cases where this driver
probes before those device links are even created, then this extra
check defers so that the probe order stays correct. Then the
device_links were correct to WARN because they're basically saying,
"hey, you asked us to enforce a particular dependency, but I can
already see probe is happening in an order that violates that
dependency".

Is that right?

-Evan

Patch
diff mbox series

diff --git a/drivers/iommu/mtk_iommu.c b/drivers/iommu/mtk_iommu.c
index 36526c9..202e41b 100644
--- a/drivers/iommu/mtk_iommu.c
+++ b/drivers/iommu/mtk_iommu.c
@@ -645,7 +645,7 @@  static int mtk_iommu_probe(struct platform_device *pdev)
 			id = i;
 
 		plarbdev = of_find_device_by_node(larbnode);
-		if (!plarbdev)
+		if (!plarbdev || !plarbdev->dev.driver)
 			return -EPROBE_DEFER;
 		data->smi_imu.larb_imu[id].dev = &plarbdev->dev;
 
diff --git a/drivers/iommu/mtk_iommu_v1.c b/drivers/iommu/mtk_iommu_v1.c
index f8b8275..9386aee 100644
--- a/drivers/iommu/mtk_iommu_v1.c
+++ b/drivers/iommu/mtk_iommu_v1.c
@@ -601,7 +601,7 @@  static int mtk_iommu_probe(struct platform_device *pdev)
 			plarbdev = of_platform_device_create(
 						larb_spec.np, NULL,
 						platform_bus_type.dev_root);
-			if (!plarbdev) {
+			if (!plarbdev || !plarbdev->dev.driver) {
 				of_node_put(larb_spec.np);
 				return -EPROBE_DEFER;
 			}