Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752086AbaJ0KIf (ORCPT ); Mon, 27 Oct 2014 06:08:35 -0400 Received: from mail-ie0-f171.google.com ([209.85.223.171]:59231 "EHLO mail-ie0-f171.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751699AbaJ0KIb convert rfc822-to-8bit (ORCPT ); Mon, 27 Oct 2014 06:08:31 -0400 MIME-Version: 1.0 In-Reply-To: <9308178.MnIriKUXEi@diego> References: <1414136029-22695-1-git-send-email-djkurtz@chromium.org> <1414136029-22695-2-git-send-email-djkurtz@chromium.org> <9308178.MnIriKUXEi@diego> From: Daniel Kurtz Date: Mon, 27 Oct 2014 18:08:10 +0800 X-Google-Sender-Auth: wt2ksNAahWcVDgS7KRzlmwn-vKA Message-ID: Subject: Re: [PATCH v5 1/3] iommu/rockchip: rk3288 iommu driver To: =?UTF-8?Q?Heiko_St=C3=BCbner?= Cc: Joerg Roedel , Simon Xue , Grant Likely , Rob Herring , open list , "open list:IOMMU DRIVERS" , "moderated list:ARM/Rockchip SoC..." , "open list:OPEN FIRMWARE AND..." Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Mon, Oct 27, 2014 at 4:32 AM, Heiko Stübner 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 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 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 and Daniel Kurtz >> "); +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 : [] lr : [] 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 > [] (rk_iommu_attach_device) from [] (iommu_attach_device+0x18/0x24) > [] (iommu_attach_device) from [] (arm_iommu_attach_device+0x18/0xd0) > [] (arm_iommu_attach_device) from [] (rockchip_drm_load+0xf0/0x198) > [] (rockchip_drm_load) from [] (drm_dev_register+0x80/0x100) > [] (drm_dev_register) from [] (rockchip_drm_bind+0x48/0x74) > [] (rockchip_drm_bind) from [] (try_to_bring_up_master.part.2+0xa4/0xf4) > [] (try_to_bring_up_master.part.2) from [] (component_add+0x9c/0x104) > [] (component_add) from [] (platform_drv_probe+0x48/0x90) > [] (platform_drv_probe) from [] (driver_probe_device+0x130/0x340) > [] (driver_probe_device) from [] (__driver_attach+0x68/0x8c) > [] (__driver_attach) from [] (bus_for_each_dev+0x6c/0x80) > [] (bus_for_each_dev) from [] (bus_add_driver+0xfc/0x1f0) > [] (bus_add_driver) from [] (driver_register+0x9c/0xe0) > [] (driver_register) from [] (do_one_initcall+0x110/0x1bc) > [] (do_one_initcall) from [] (kernel_init_freeable+0xfc/0x1c8) > [] (kernel_init_freeable) from [] (kernel_init+0x8/0xe4) > [] (kernel_init) from [] (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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/