From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752164AbdBJRhn (ORCPT ); Fri, 10 Feb 2017 12:37:43 -0500 Received: from foss.arm.com ([217.140.101.70]:39814 "EHLO foss.arm.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751060AbdBJRhm (ORCPT ); Fri, 10 Feb 2017 12:37:42 -0500 Subject: Re: [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface To: Joerg Roedel References: <1486639981-32368-1-git-send-email-joro@8bytes.org> <1486639981-32368-9-git-send-email-joro@8bytes.org> <5bda5842-45b4-fc3c-7eea-351e4c2261b1@arm.com> <20170210153339.GK7339@8bytes.org> Cc: Will Deacon , Lorenzo Pieralisi , Alex Williamson , David Woodhouse , iommu@lists.linux-foundation.org, linux-kernel@vger.kernel.org, Joerg Roedel From: Robin Murphy Message-ID: <045e481d-92bb-ae3d-5e01-c06e31f6d426@arm.com> Date: Fri, 10 Feb 2017 17:36:53 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <20170210153339.GK7339@8bytes.org> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/02/17 15:33, Joerg Roedel wrote: > 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. For the ARM SMMUs at least, the SMMU-specific data is (well, should be) all devm_* managed, thus freed automatically by the driver core after remove() returns. It is true that there's an implicit expectation that the SMMU won't be removed until all domains, groups and masters have been explicitly torn down by the relevant detach()/remove()/free() calls, although I guess the sysfs links might actually help enforce that. > So I think we are fine from that side. Sure, I mostly just wanted not to lose sight of the future possibility of unloadable IOMMU drivers (admittedly I've not even tried that yet, only loading them post-boot). Robin. > > > Joerg > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robin Murphy Subject: Re: [PATCH 08/11] iommu/msm: Make use of iommu_device_register interface Date: Fri, 10 Feb 2017 17:36:53 +0000 Message-ID: <045e481d-92bb-ae3d-5e01-c06e31f6d426@arm.com> References: <1486639981-32368-1-git-send-email-joro@8bytes.org> <1486639981-32368-9-git-send-email-joro@8bytes.org> <5bda5842-45b4-fc3c-7eea-351e4c2261b1@arm.com> <20170210153339.GK7339@8bytes.org> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20170210153339.GK7339-zLv9SwRftAIdnm+yROfE0A@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Joerg Roedel Cc: Will Deacon , iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Joerg Roedel , David Woodhouse List-Id: iommu@lists.linux-foundation.org On 10/02/17 15:33, Joerg Roedel wrote: > 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. For the ARM SMMUs at least, the SMMU-specific data is (well, should be) all devm_* managed, thus freed automatically by the driver core after remove() returns. It is true that there's an implicit expectation that the SMMU won't be removed until all domains, groups and masters have been explicitly torn down by the relevant detach()/remove()/free() calls, although I guess the sysfs links might actually help enforce that. > So I think we are fine from that side. Sure, I mostly just wanted not to lose sight of the future possibility of unloadable IOMMU drivers (admittedly I've not even tried that yet, only loading them post-boot). Robin. > > > Joerg >