From: Joerg Roedel <joro@8bytes.org> To: Robin Murphy <robin.murphy@arm.com> Cc: Will Deacon <will.deacon@arm.com>, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, Alex Williamson <alex.williamson@redhat.com>, David Woodhouse <dwmw2@infradead.org>, iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, Joerg Roedel <jroedel@suse.de> Subject: Re: [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface Date: Fri, 10 Feb 2017 16:33:40 +0100 [thread overview] Message-ID: <20170210153339.GK7339@8bytes.org> (raw) In-Reply-To: <5bda5842-45b4-fc3c-7eea-351e4c2261b1@arm.com> On Fri, Feb 10, 2017 at 02:35:39PM +0000, Robin Murphy wrote: > On 09/02/17 11:32, Joerg Roedel wrote: > > + ret = iommu_device_sysfs_add(&iommu->iommu, iommu->dev, NULL, > > + "msm-smmu.%pa", &ioaddr); > > + if (ret) { > > + pr_err("Could not add msm-smmu at %pa to sysfs\n", &ioaddr); > > + goto fail; > > + } > > Nit: there's a bit of inconsistency with printing errors between the > various drivers (for both _sysfs_add and _register). I reckon if we want > error messages we may as well just fold them into the helper functions. Yeah, this could be unified too. For now I looked how verbose the driver was that I was going to change and added messages to be consistent inside the drivers. > > > + > > + iommu_device_set_ops(&iommu->iommu, &msm_iommu_ops); > > + iommu_device_set_fwnode(&iommu->iommu, &pdev->dev.of_node->fwnode); > > + > > + ret = iommu_device_register(&iommu->iommu); > > + if (ret) { > > + pr_err("Could not register msm-smmu at %pa\n", &ioaddr); > > + goto fail; > > + } > > I think there's a corresponding unregister missing for > msm_iommu_remove() here (and similarly in the ARM SMMU drivers, looking > back). I know it's not strictly a problem at the moment, but I do now > have IOMMU-drivers-as-modules working on top of the probe deferral > series... ;) Well, that there was an iommu_register_instance() without any unregistration interface at all makes me believe that unregistering iommus is not really implemented yet. And in fact, the remove functions for msm and arm-smmu seem to only disable the hardware, but are not removing the corresponding data structures. So I think we are fine from that side. Joerg
WARNING: multiple messages have this Message-ID (diff)
From: Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> To: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> Cc: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>, iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Joerg Roedel <jroedel-l3A5Bk7waGM@public.gmane.org>, David Woodhouse <dwmw2-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org> Subject: Re: [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface Date: Fri, 10 Feb 2017 16:33:40 +0100 [thread overview] Message-ID: <20170210153339.GK7339@8bytes.org> (raw) In-Reply-To: <5bda5842-45b4-fc3c-7eea-351e4c2261b1-5wv7dgnIgG8@public.gmane.org> On Fri, Feb 10, 2017 at 02:35:39PM +0000, Robin Murphy wrote: > On 09/02/17 11:32, Joerg Roedel wrote: > > + ret = iommu_device_sysfs_add(&iommu->iommu, iommu->dev, NULL, > > + "msm-smmu.%pa", &ioaddr); > > + if (ret) { > > + pr_err("Could not add msm-smmu at %pa to sysfs\n", &ioaddr); > > + goto fail; > > + } > > Nit: there's a bit of inconsistency with printing errors between the > various drivers (for both _sysfs_add and _register). I reckon if we want > error messages we may as well just fold them into the helper functions. Yeah, this could be unified too. For now I looked how verbose the driver was that I was going to change and added messages to be consistent inside the drivers. > > > + > > + iommu_device_set_ops(&iommu->iommu, &msm_iommu_ops); > > + iommu_device_set_fwnode(&iommu->iommu, &pdev->dev.of_node->fwnode); > > + > > + ret = iommu_device_register(&iommu->iommu); > > + if (ret) { > > + pr_err("Could not register msm-smmu at %pa\n", &ioaddr); > > + goto fail; > > + } > > I think there's a corresponding unregister missing for > msm_iommu_remove() here (and similarly in the ARM SMMU drivers, looking > back). I know it's not strictly a problem at the moment, but I do now > have IOMMU-drivers-as-modules working on top of the probe deferral > series... ;) Well, that there was an iommu_register_instance() without any unregistration interface at all makes me believe that unregistering iommus is not really implemented yet. And in fact, the remove functions for msm and arm-smmu seem to only disable the hardware, but are not removing the corresponding data structures. So I think we are fine from that side. Joerg
next prev parent reply other threads:[~2017-02-10 15:58 UTC|newest] Thread overview: 61+ messages / expand[flat|nested] mbox.gz Atom feed top 2017-02-09 11:32 [PATCH 00/11 v3] Let IOMMU core know about individual IOMMUs Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 11:32 ` [PATCH 01/11] iommu: Rename iommu_get_instance() Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-10 14:12 ` Robin Murphy 2017-02-10 14:12 ` Robin Murphy 2017-02-10 15:36 ` Joerg Roedel 2017-02-10 15:36 ` Joerg Roedel 2017-02-09 11:32 ` [PATCH 02/11] iommu: Rename struct iommu_device Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 11:32 ` [PATCH 03/11] iommu: Introduce new 'struct iommu_device' Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 20:42 ` kbuild test robot 2017-02-09 20:42 ` kbuild test robot 2017-02-09 11:32 ` [PATCH 04/11] iommu: Add sysfs bindings for struct iommu_device Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 11:32 ` [PATCH 05/11] iommu: Make iommu_device_link/unlink take a " Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 11:32 ` [PATCH 06/11] iommu: Add iommu_device_set_fwnode() interface Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-10 14:16 ` Robin Murphy 2017-02-10 14:16 ` Robin Murphy 2017-02-10 15:22 ` Joerg Roedel 2017-02-10 15:22 ` Joerg Roedel 2017-02-10 16:03 ` Robin Murphy 2017-02-10 16:03 ` Robin Murphy 2017-02-10 16:11 ` Joerg Roedel 2017-02-10 16:11 ` Joerg Roedel 2017-02-10 16:59 ` Robin Murphy 2017-02-10 16:59 ` Robin Murphy 2017-02-09 11:32 ` [PATCH 07/11] iommu/arm-smmu: Make use of the iommu_register interface Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-10 14:20 ` Robin Murphy 2017-02-10 14:20 ` Robin Murphy 2017-02-10 15:25 ` Joerg Roedel 2017-02-10 15:25 ` Joerg Roedel 2017-02-10 17:07 ` Robin Murphy 2017-02-10 17:07 ` Robin Murphy 2017-02-09 11:32 ` [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-10 14:35 ` Robin Murphy 2017-02-10 14:35 ` Robin Murphy 2017-02-10 15:33 ` Joerg Roedel [this message] 2017-02-10 15:33 ` Joerg Roedel 2017-02-10 17:36 ` Robin Murphy 2017-02-10 17:36 ` Robin Murphy 2017-02-09 11:32 ` [PATCH 09/11] iommu/mediatek: " Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 11:32 ` [PATCH 10/11] iommu/exynos: " Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-09 11:32 ` Joerg Roedel 2017-02-10 13:46 ` Marek Szyprowski 2017-02-10 13:46 ` Marek Szyprowski 2017-02-10 13:46 ` Marek Szyprowski 2017-02-10 13:59 ` Joerg Roedel 2017-02-10 13:59 ` Joerg Roedel 2017-02-10 13:59 ` Joerg Roedel 2017-02-09 11:33 ` [PATCH 11/11] iommu: Remove iommu_register_instance interface Joerg Roedel 2017-02-09 11:33 ` Joerg Roedel 2017-02-09 11:33 ` Joerg Roedel
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=20170210153339.GK7339@8bytes.org \ --to=joro@8bytes.org \ --cc=alex.williamson@redhat.com \ --cc=dwmw2@infradead.org \ --cc=iommu@lists.linux-foundation.org \ --cc=jroedel@suse.de \ --cc=linux-kernel@vger.kernel.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=robin.murphy@arm.com \ --cc=will.deacon@arm.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: linkBe 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.