Message-ID: <CAGS+omDXb8M7RswgmQ33+PAh5VEnt9q00+Xn6KL1ZD_g-=q2cw@mail.gmail.com>
Date:	Mon, 27 Oct 2014 18:08:10 +0800
From:	Daniel Kurtz <djkurtz@...omium.org>
To:	Heiko Stübner <heiko@...ech.de>
Cc:	Joerg Roedel <joro@...tes.org>, Simon Xue <xxm@...k-chips.com>,
	Grant Likely <grant.likely@...aro.org>,
	Rob Herring <robh+dt@...nel.org>,
	open list <linux-kernel@...r.kernel.org>,
	"open list:IOMMU DRIVERS" <iommu@...ts.linux-foundation.org>,
	"moderated list:ARM/Rockchip SoC..." 
	<linux-arm-kernel@...ts.infradead.org>,
	"open list:OPEN FIRMWARE AND..." <devicetree@...r.kernel.org>
Subject: Re: [PATCH v5 1/3] iommu/rockchip: rk3288 iommu driver

On Mon, Oct 27, 2014 at 4:32 AM, Heiko Stübner <heiko@...ech.de> wrote:
> Hi Daniel,
>
> Am Freitag, 24. Oktober 2014, 15:33:47 schrieb Daniel Kurtz:
>
> [...]
>
>> +static int rk_iommu_attach_device(struct iommu_domain *domain,
>> +                               struct device *dev)
>> +{
>> +     struct rk_iommu *iommu = dev_get_drvdata(dev->archdata.iommu);
>
> Here I get a null-ptr dereference [0] when using the iommu driver with the
> pending drm changes.

That's what I get for testing against a heavily modified v3.14-based kernel...

In v3.14, dev_get_drvdata() would happily return NULL if dev=NULL.
This "feature" was removed in v3.15 by this patch:

commit d4332013919aa87dbdede67d677e4cf2cd32e898
Author: Jean Delvare <jdelvare@...e.de>
Date:   Mon Apr 14 12:57:43 2014 +0200
driver core: dev_get_drvdata: Don't check for NULL dev

>
>> +     struct rk_iommu_domain *rk_domain = domain->priv;
>> +     unsigned long flags;
>> +     int ret;
>> +     phys_addr_t dte_addr;
>> +
>> +     /*
>> +      * Allow 'virtual devices' (e.g., drm) to attach to domain.
>> +      * Such a device has a NULL archdata.iommu.
>> +      */
>> +     if (!iommu)
>
> When the comment is correct, the code should probably do something like
> the following?
>
> if (!dev->archdata.iommu)
>         return 0;
>
> iommu = dev_get_drvdata(dev->archdata.iommu);
>

Yes, that looks reasonable.

>
>> +             return 0;
>> +
>> +     ret = rk_iommu_enable_stall(iommu);
>> +     if (ret)
>> +             return ret;
>> +
>> +     ret = rk_iommu_force_reset(iommu);
>> +     if (ret)
>> +             return ret;
>> +
>> +     iommu->domain = domain;
>> +
>> +     ret = devm_request_irq(dev, iommu->irq, rk_iommu_irq,
>> +                            IRQF_SHARED, dev_name(dev), iommu);
>> +     if (ret)
>> +             return ret;
>> +
>> +     dte_addr = virt_to_phys(rk_domain->dt);
>> +     rk_iommu_write(iommu, RK_MMU_DTE_ADDR, dte_addr);
>> +     rk_iommu_command(iommu, RK_MMU_CMD_ZAP_CACHE);
>> +     rk_iommu_write(iommu, RK_MMU_INT_MASK, RK_MMU_IRQ_MASK);
>> +
>> +     ret = rk_iommu_enable_paging(iommu);
>> +     if (ret)
>> +             return ret;
>> +
>> +     spin_lock_irqsave(&rk_domain->iommus_lock, flags);
>> +     list_add_tail(&iommu->node, &rk_domain->iommus);
>> +     spin_unlock_irqrestore(&rk_domain->iommus_lock, flags);
>> +
>> +     dev_info(dev, "Attached to iommu domain\n");
>> +
>> +     rk_iommu_disable_stall(iommu);
>> +
>> +     return 0;
>> +}
>
> [...]
>
>> +
>> +static struct platform_driver rk_iommu_driver = {
>> +     .probe = rk_iommu_probe,
>> +     .remove = rk_iommu_remove,
>> +     .driver = {
>> +                .name = "rk_iommu",
>> +                .owner = THIS_MODULE,
>> +                .of_match_table = of_match_ptr(rk_iommu_dt_ids),
>> +     },
>> +};
>> +
>> +static int __init rk_iommu_init(void)
>> +{
>> +     int ret;
>> +
>> +     ret = bus_set_iommu(&platform_bus_type, &rk_iommu_ops);
>
> on 3.18-rc1 this fails with -ENODEV, as add_iommu_group() is missing the
> add_device callback in rk_iommu_ops, so the iommu driver actually never
> gets registered.

v3.18-rc1 has patch [0] which changes
bus_set_iommu()->iommu_bus_init() to propagate the return value of
add_iommu_group(), whereas it was ignored in v3.17.

[0] commit fb3e306515ba6a012364b698b8ca71c337424ed3
Author: Mark Salter <msalter@...hat.com>
Date:   Sun Sep 21 13:58:24 2014 -0400

    iommu: Fix bus notifier breakage


This patch made it mandatory that iommu drivers provide an add_group
callback.   I'm not exactly sure why.  Iommu groups do not seem to be
a good fit for the rockchip iommus, since the iommus are all 1:1 with
their master device.

The exynos add_group() is a possibility, however, it causes an
iommu_group to be allocated for every single platform_device, even if
they do not use an iommu.  This seems very wasteful.  Instead we can
check the device's dt node for an iommus field to a phandle with a
"#iommu-cells" field.

Also, perhaps the add_device() is a good place to stick other generic
device initialization code, which we are currently sprinkling in the
drivers of rockchip iommu masters (drm/codec).  Other drivers do this:
 * shmobile: sets up the iommu mapping with arm_iommu_create_mapping()
/ arm_iommu_attach_device()
 * omap: use of_parse_phandle()/of_find_device_by_node() to set a
master device's dev->archdata.iommu.

Or, perhaps we can just ignore iommu groups entirely and use dummy functions:
 static int rk_iommu_add_device(struct device *dev) { return 0; }
 static void rk_iommu_remove_device(struct device *dev) { }

I'll investigate more.

-Dan

>
> I've stolen the generic add_device and remove_device callbacks from the
> exynos iommu driver which makes the rk one at least probe.
>
> Can't say how far it goes, as I'm still struggling with the floating display
> subsystem parts. My current diff against this version can be found in [1].
>
> Maybe the issue I had in attach_device also simply resulted from this one,
> not sure right now.
>
>
> Heiko
>
>> +     if (ret)
>> +             return ret;
>> +
>> +     return platform_driver_register(&rk_iommu_driver);
>> +}
>> +static void __exit rk_iommu_exit(void)
>> +{
>> +     platform_driver_unregister(&rk_iommu_driver);
>> +}
>> +
>> +subsys_initcall(rk_iommu_init);
>> +module_exit(rk_iommu_exit);
>> +
>> +MODULE_DESCRIPTION("IOMMU API for Rockchip");
>> +MODULE_AUTHOR("Simon Xue <xxm@...k-chips.com> and Daniel Kurtz
>> <djkurtz@...omium.org>"); +MODULE_ALIAS("platform:rockchip-iommu");
>> +MODULE_LICENSE("GPL v2");
>
>
>
> [0]
>
> [drm] Initialized drm 1.1.0 20060810
> Unable to handle kernel NULL pointer dereference at virtual address 00000058
> pgd = c0004000
> [00000058] *pgd=00000000
> Internal error: Oops: 5 [#1] SMP ARM
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper/0 Not tainted 3.18.0-rc1+ #1274
> task: ee067b40 ti: ee068000 task.ti: ee068000
> PC is at rk_iommu_attach_device+0x3c/0x29c
> LR is at rk_iommu_attach_device+0x30/0x29c
> pc : [<c03686d4>]    lr : [<c03686c8>]    psr: 60000153
> sp : ee069db8  ip : 00000000  fp : 00000000
> r10: ee276f00  r9 : 00000000  r8 : ee27cc00
> r7 : ee27cf80  r6 : ee11b610  r5 : ee11b610  r4 : ee27cf80
> r3 : 00000000  r2 : c07bb588  r1 : c045b6d0  r0 : c054a27f
> Flags: nZCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
> Control: 10c5387d  Table: 0000406a  DAC: 00000015
> Process swapper/0 (pid: 1, stack limit = 0xee068240)
> Stack: (0xee069db8 to 0xee06a000)
> 9da0:                                                       c07c75a8 c0367fec
> 9dc0: c0367fc4 ee276f00 c07c75a8 ee27cf80 ee11b610 00000000 ee27cf80 ee27cc00
> 9de0: 00000000 c0366c48 ee27c250 c001a470 ee27c250 ee11b610 ee2cf400 00000000
> 9e00: ee27cf80 c0225388 c0225298 ee2cf400 00000000 00000000 00000000 c0213cb0
> 9e20: ee11ca40 ee11b610 ee2cf400 00000000 ee2db130 c022522c ee2dbf80 00000004
> 9e40: ee2db110 c022a9e4 ee27cc00 c07c73f8 ee2dbf80 c07da7c0 c082633c c022abe4
> 9e60: c0446710 ee127010 ffffffed c07c7354 c07da7c0 c0230174 ee127010 c07c7354
> 9e80: 00000000 c022ebc0 c07c7354 ee127010 ee069ea8 ee127010 ee127044 c07c7354
> 9ea0: 00000000 c05be4a0 ee068000 c022ee38 00000000 c07c7354 c022edd0 c022d28c
> 9ec0: ee04675c ee1226b4 c07c7354 ee2d6a80 c07c75a8 c022e2b4 c0512e6d c0512e6d
> 9ee0: 00000072 c07c7354 c07b6e18 c07dfac0 c05dd274 c022f4b0 00000000 ee27cd00
> 9f00: c07b6e18 c00089d0 ee0e9300 c0100018 ee0e9300 ee0e9080 ee0e9000 c0420e38
> 9f20: c0802444 00000000 c057d980 c01001a4 c05a7594 ef7fcc05 00000000 c0036b68
> 9f40: 00000000 00000000 c057d980 c057cd90 000000bf 00000006 c07ba5f0 00000006
> 9f60: c05d1568 c07dfac0 c05dd274 000000bf 00000000 00000000 00000000 c05a7d20
> 9f80: 00000006 00000006 c05a7594 ee068000 00000000 c04171ac 00000000 00000000
> 9fa0: 00000000 c04171b4 00000000 c000e878 00000000 00000000 00000000 00000000
> 9fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
> 9fe0: 00000000 00000000 00000000 00000000 00000013 00000000 00000000 00000000
> [<c03686d4>] (rk_iommu_attach_device) from [<c0366c48>] (iommu_attach_device+0x18/0x24)
> [<c0366c48>] (iommu_attach_device) from [<c001a470>] (arm_iommu_attach_device+0x18/0xd0)
> [<c001a470>] (arm_iommu_attach_device) from [<c0225388>] (rockchip_drm_load+0xf0/0x198)
> [<c0225388>] (rockchip_drm_load) from [<c0213cb0>] (drm_dev_register+0x80/0x100)
> [<c0213cb0>] (drm_dev_register) from [<c022522c>] (rockchip_drm_bind+0x48/0x74)
> [<c022522c>] (rockchip_drm_bind) from [<c022a9e4>] (try_to_bring_up_master.part.2+0xa4/0xf4)
> [<c022a9e4>] (try_to_bring_up_master.part.2) from [<c022abe4>] (component_add+0x9c/0x104)
> [<c022abe4>] (component_add) from [<c0230174>] (platform_drv_probe+0x48/0x90)
> [<c0230174>] (platform_drv_probe) from [<c022ebc0>] (driver_probe_device+0x130/0x340)
> [<c022ebc0>] (driver_probe_device) from [<c022ee38>] (__driver_attach+0x68/0x8c)
> [<c022ee38>] (__driver_attach) from [<c022d28c>] (bus_for_each_dev+0x6c/0x80)
> [<c022d28c>] (bus_for_each_dev) from [<c022e2b4>] (bus_add_driver+0xfc/0x1f0)
> [<c022e2b4>] (bus_add_driver) from [<c022f4b0>] (driver_register+0x9c/0xe0)
> [<c022f4b0>] (driver_register) from [<c00089d0>] (do_one_initcall+0x110/0x1bc)
> [<c00089d0>] (do_one_initcall) from [<c05a7d20>] (kernel_init_freeable+0xfc/0x1c8)
> [<c05a7d20>] (kernel_init_freeable) from [<c04171b4>] (kernel_init+0x8/0xe4)
> [<c04171b4>] (kernel_init) from [<c000e878>] (ret_from_fork+0x14/0x3c)
> Code: eb02c07d e5963140 e59f1234 e59f0234 (e5934058)
> ---[ end trace 41e4f8e55e7119af ]---
> Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
>
> ---[ end Kernel panic - not syncing: Attempted to kill init! exitcode=0x0000000b
>
>
>
> [1]
>
> diff --git a/drivers/iommu/rockchip-iommu.c b/drivers/iommu/rockchip-iommu.c
> index 56ffb76..959348f 100644
> --- a/drivers/iommu/rockchip-iommu.c
> +++ b/drivers/iommu/rockchip-iommu.c
> @@ -708,8 +708,8 @@ static size_t rk_iommu_unmap(struct iommu_domain *domain, unsigned long _iova,
>  static int rk_iommu_attach_device(struct iommu_domain *domain,
>                                   struct device *dev)
>  {
> -       struct rk_iommu *iommu = dev_get_drvdata(dev->archdata.iommu);
>         struct rk_iommu_domain *rk_domain = domain->priv;
> +       struct rk_iommu *iommu;
>         unsigned long flags;
>         int ret;
>         phys_addr_t dte_addr;
> @@ -718,9 +718,11 @@ static int rk_iommu_attach_device(struct iommu_domain *domain,
>          * Allow 'virtual devices' (e.g., drm) to attach to domain.
>          * Such a device has a NULL archdata.iommu.
>          */
> -       if (!iommu)
> +       if (!dev->archdata.iommu)
>                 return 0;
>
> +       iommu = dev_get_drvdata(dev->archdata.iommu);
> +
>         ret = rk_iommu_enable_stall(iommu);
>         if (ret)
>                 return ret;
> @@ -837,6 +839,32 @@ static void rk_iommu_domain_destroy(struct iommu_domain *domain)
>         domain->priv = NULL;
>  }
>
> +static int rk_iommu_add_device(struct device *dev)
> +{
> +       struct iommu_group *group;
> +       int ret;
> +
> +       group = iommu_group_get(dev);
> +
> +       if (!group) {
> +               group = iommu_group_alloc();
> +               if (IS_ERR(group)) {
> +                       dev_err(dev, "Failed to allocate IOMMU group\n");
> +                       return PTR_ERR(group);
> +               }
> +       }
> +
> +       ret = iommu_group_add_device(group, dev);
> +       iommu_group_put(group);
> +
> +       return ret;
> +}
> +
> +static void rk_iommu_remove_device(struct device *dev)
> +{
> +       iommu_group_remove_device(dev);
> +}
> +
>  static const struct iommu_ops rk_iommu_ops = {
>         .domain_init = rk_iommu_domain_init,
>         .domain_destroy = rk_iommu_domain_destroy,
> @@ -845,6 +873,8 @@ static const struct iommu_ops rk_iommu_ops = {
>         .map = rk_iommu_map,
>         .unmap = rk_iommu_unmap,
>         .iova_to_phys = rk_iommu_iova_to_phys,
> +       .add_device = rk_iommu_add_device,
> +       .remove_device = rk_iommu_remove_device,
>         .pgsize_bitmap = RK_IOMMU_PGSIZE_BITMAP,
>  };
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists