From: Vivek Gautam <vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> To: Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org>, "Rafael J. Wysocki" <rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org> Cc: Mark Rutland <mark.rutland-5wv7dgnIgG8@public.gmane.org>, devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, Stephen Boyd <sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>, Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>, "list-Y9sIeH5OGRo@public.gmane.org:IOMMU DRIVERS <iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>, Joerg Roedel <joro-zLv9SwRftAIdnm+yROfE0A@public.gmane.org>, " <iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org>, open list <linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org>, robh+dt <robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>, linux-arm-msm <linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org> Subject: Re: [PATCH v8 4/5] iommu/arm-smmu: Add the device_link between masters and smmu Date: Fri, 9 Mar 2018 12:41:46 +0530 [thread overview] Message-ID: <CAFp+6iFzGYWZMLkNrN1ZJJ2xH4CxQsiU6oYboHDzL0jDwm+4VQ@mail.gmail.com> (raw) In-Reply-To: <CAFp+6iF1oM=fmRCqSG-SxcUVvvOLet_Y0p7pmGn+=B-LdMNiww-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> On Thu, Mar 8, 2018 at 10:29 AM, Vivek Gautam <vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> wrote: > On Wed, Mar 7, 2018 at 6:17 PM, Robin Murphy <robin.murphy-5wv7dgnIgG8@public.gmane.org> wrote: >> On 02/03/18 10:10, Vivek Gautam wrote: >>> >>> From: Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> >>> >>> Finally add the device link between the master device and >>> smmu, so that the smmu gets runtime enabled/disabled only when the >>> master needs it. This is done from add_device callback which gets >>> called once when the master is added to the smmu. >>> >>> Signed-off-by: Sricharan R <sricharan-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> >>> Signed-off-by: Vivek Gautam <vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> >>> --- >>> drivers/iommu/arm-smmu.c | 21 +++++++++++++++++++++ >>> 1 file changed, 21 insertions(+) >>> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>> index 3d6a1875431f..bb1ea82c1003 100644 >>> --- a/drivers/iommu/arm-smmu.c >>> +++ b/drivers/iommu/arm-smmu.c >>> @@ -217,6 +217,9 @@ struct arm_smmu_device { >>> /* IOMMU core code handle */ >>> struct iommu_device iommu; >>> + >>> + /* runtime PM link to master */ >>> + struct device_link *link; >> >> >> Just the one? we will either have to count all the devices that are present on the iommu bus, or maintain a list to which all the links can be added. But to add the list, we will have to initialize a LIST_HEAD in struct device_link as well. Or, I think we don't even need to maintain a pointer to link with smmu. In arm_smmu_remove_device(), we can find out the correct link, and delete it. list_for_each_entry(link, &dev->links.suppliers, c_node) if (link->supplier == smmu->dev); device_link_del(link); Should that be fine? Rafael, does the above snippet looks right to you? Context: smmu->dev is the supplier, and dev is the consumer. We want to find the link, and delete it. regards Vivek >> >>> }; >>> enum arm_smmu_context_fmt { >>> @@ -1470,10 +1473,26 @@ static int arm_smmu_add_device(struct device *dev) >>> iommu_device_link(&smmu->iommu, dev); >>> + /* >>> + * Establish the link between smmu and master, so that the >>> + * smmu gets runtime enabled/disabled as per the master's >>> + * needs. >>> + */ >>> + smmu->link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME); >> >> >> Maybe I've misunderstood how the API works, but AFAICS the second and >> subsequent devices are all just going to overwrite (and leak) the link of >> the previous one... > > Sorry, my bad. Will take care of this. > > regards > Vivek > >> >>> + if (!smmu->link) { >>> + dev_warn(smmu->dev, "Unable to create device link between >>> %s and %s\n", >>> + dev_name(smmu->dev), dev_name(dev)); >>> + ret = -ENODEV; >>> + goto out_unlink; >>> + } >>> + >>> arm_smmu_rpm_put(smmu); >>> return 0; >>> +out_unlink: >>> + iommu_device_unlink(&smmu->iommu, dev); >>> + arm_smmu_master_free_smes(fwspec); >>> out_rpm_put: >>> arm_smmu_rpm_put(smmu); >>> out_cfg_free: >>> @@ -1496,6 +1515,8 @@ static void arm_smmu_remove_device(struct device >>> *dev) >>> cfg = fwspec->iommu_priv; >>> smmu = cfg->smmu; >>> + device_link_del(smmu->link); >> >> >> ...and equivalently you end up with a double-free (or more) here of a link >> which may not have belonged to dev anyway. >> >> Robin. >> >> >>> + >>> ret = arm_smmu_rpm_get(smmu); >>> if (ret < 0) >>> return; >>> >> > > > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
WARNING: multiple messages have this Message-ID (diff)
From: Vivek Gautam <vivek.gautam@codeaurora.org> To: Robin Murphy <robin.murphy@arm.com>, "Rafael J. Wysocki" <rjw@rjwysocki.net> Cc: "list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>," <joro@8bytes.org>, "robh+dt" <robh+dt@kernel.org>, Mark Rutland <mark.rutland@arm.com>, Will Deacon <will.deacon@arm.com>, Rob Clark <robdclark@gmail.com>, "list@263.net:IOMMU DRIVERS <iommu@lists.linux-foundation.org>, Joerg Roedel <joro@8bytes.org>," <iommu@lists.linux-foundation.org>, devicetree@vger.kernel.org, open list <linux-kernel@vger.kernel.org>, Tomasz Figa <tfiga@chromium.org>, jcrouse@codeaurora.org, Stephen Boyd <sboyd@codeaurora.org>, Sricharan R <sricharan@codeaurora.org>, Marek Szyprowski <m.szyprowski@samsung.com>, Archit Taneja <architt@codeaurora.org>, linux-arm-msm <linux-arm-msm@vger.kernel.org> Subject: Re: [PATCH v8 4/5] iommu/arm-smmu: Add the device_link between masters and smmu Date: Fri, 9 Mar 2018 12:41:46 +0530 [thread overview] Message-ID: <CAFp+6iFzGYWZMLkNrN1ZJJ2xH4CxQsiU6oYboHDzL0jDwm+4VQ@mail.gmail.com> (raw) In-Reply-To: <CAFp+6iF1oM=fmRCqSG-SxcUVvvOLet_Y0p7pmGn+=B-LdMNiww@mail.gmail.com> On Thu, Mar 8, 2018 at 10:29 AM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > On Wed, Mar 7, 2018 at 6:17 PM, Robin Murphy <robin.murphy@arm.com> wrote: >> On 02/03/18 10:10, Vivek Gautam wrote: >>> >>> From: Sricharan R <sricharan@codeaurora.org> >>> >>> Finally add the device link between the master device and >>> smmu, so that the smmu gets runtime enabled/disabled only when the >>> master needs it. This is done from add_device callback which gets >>> called once when the master is added to the smmu. >>> >>> Signed-off-by: Sricharan R <sricharan@codeaurora.org> >>> Signed-off-by: Vivek Gautam <vivek.gautam@codeaurora.org> >>> --- >>> drivers/iommu/arm-smmu.c | 21 +++++++++++++++++++++ >>> 1 file changed, 21 insertions(+) >>> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>> index 3d6a1875431f..bb1ea82c1003 100644 >>> --- a/drivers/iommu/arm-smmu.c >>> +++ b/drivers/iommu/arm-smmu.c >>> @@ -217,6 +217,9 @@ struct arm_smmu_device { >>> /* IOMMU core code handle */ >>> struct iommu_device iommu; >>> + >>> + /* runtime PM link to master */ >>> + struct device_link *link; >> >> >> Just the one? we will either have to count all the devices that are present on the iommu bus, or maintain a list to which all the links can be added. But to add the list, we will have to initialize a LIST_HEAD in struct device_link as well. Or, I think we don't even need to maintain a pointer to link with smmu. In arm_smmu_remove_device(), we can find out the correct link, and delete it. list_for_each_entry(link, &dev->links.suppliers, c_node) if (link->supplier == smmu->dev); device_link_del(link); Should that be fine? Rafael, does the above snippet looks right to you? Context: smmu->dev is the supplier, and dev is the consumer. We want to find the link, and delete it. regards Vivek >> >>> }; >>> enum arm_smmu_context_fmt { >>> @@ -1470,10 +1473,26 @@ static int arm_smmu_add_device(struct device *dev) >>> iommu_device_link(&smmu->iommu, dev); >>> + /* >>> + * Establish the link between smmu and master, so that the >>> + * smmu gets runtime enabled/disabled as per the master's >>> + * needs. >>> + */ >>> + smmu->link = device_link_add(dev, smmu->dev, DL_FLAG_PM_RUNTIME); >> >> >> Maybe I've misunderstood how the API works, but AFAICS the second and >> subsequent devices are all just going to overwrite (and leak) the link of >> the previous one... > > Sorry, my bad. Will take care of this. > > regards > Vivek > >> >>> + if (!smmu->link) { >>> + dev_warn(smmu->dev, "Unable to create device link between >>> %s and %s\n", >>> + dev_name(smmu->dev), dev_name(dev)); >>> + ret = -ENODEV; >>> + goto out_unlink; >>> + } >>> + >>> arm_smmu_rpm_put(smmu); >>> return 0; >>> +out_unlink: >>> + iommu_device_unlink(&smmu->iommu, dev); >>> + arm_smmu_master_free_smes(fwspec); >>> out_rpm_put: >>> arm_smmu_rpm_put(smmu); >>> out_cfg_free: >>> @@ -1496,6 +1515,8 @@ static void arm_smmu_remove_device(struct device >>> *dev) >>> cfg = fwspec->iommu_priv; >>> smmu = cfg->smmu; >>> + device_link_del(smmu->link); >> >> >> ...and equivalently you end up with a double-free (or more) here of a link >> which may not have belonged to dev anyway. >> >> Robin. >> >> >>> + >>> ret = arm_smmu_rpm_get(smmu); >>> if (ret < 0) >>> return; >>> >> > > > > -- > QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member > of Code Aurora Forum, hosted by The Linux Foundation -- QUALCOMM INDIA, on behalf of Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, hosted by The Linux Foundation
next prev parent reply other threads:[~2018-03-09 7:11 UTC|newest] Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top 2018-03-02 10:10 [PATCH v8 0/5] iommu/arm-smmu: Add runtime pm/sleep support Vivek Gautam 2018-03-02 10:10 ` Vivek Gautam [not found] ` <20180302101050.6191-1-vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> 2018-03-02 10:10 ` [PATCH v8 1/5] iommu/arm-smmu: Destroy domain context in failure path Vivek Gautam 2018-03-02 10:10 ` Vivek Gautam [not found] ` <20180302101050.6191-2-vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> 2018-03-07 12:20 ` Robin Murphy 2018-03-07 12:20 ` Robin Murphy [not found] ` <d3500b33-8ce6-d767-7e9b-2fd75fea6cbb-5wv7dgnIgG8@public.gmane.org> 2018-03-08 5:32 ` Vivek Gautam 2018-03-08 5:32 ` Vivek Gautam 2018-03-02 10:10 ` [PATCH v8 2/5] iommu/arm-smmu: Add pm_runtime/sleep ops Vivek Gautam 2018-03-02 10:10 ` Vivek Gautam 2018-03-02 10:10 ` [PATCH v8 3/5] iommu/arm-smmu: Invoke pm_runtime during probe, add/remove device Vivek Gautam 2018-03-02 10:10 ` Vivek Gautam 2018-03-07 12:38 ` Robin Murphy [not found] ` <d0f2b3ed-136d-7704-2aef-1173a342a89c-5wv7dgnIgG8@public.gmane.org> 2018-03-07 13:52 ` Tomasz Figa 2018-03-07 13:52 ` Tomasz Figa [not found] ` <CAAFQd5BTdF176x5ycatCmCHrMyVX2OZqdXd-JGq06RyDUb0N3g-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-03-07 16:58 ` Robin Murphy 2018-03-07 16:58 ` Robin Murphy [not found] ` <30a2fa5e-3d8e-acb6-ab31-bec652f1be99-5wv7dgnIgG8@public.gmane.org> 2018-03-08 4:33 ` Tomasz Figa 2018-03-08 4:33 ` Tomasz Figa 2018-03-08 12:12 ` Robin Murphy [not found] ` <6fe36177-a8a5-5f17-bf65-1a53538221a4-5wv7dgnIgG8@public.gmane.org> 2018-03-09 4:50 ` Tomasz Figa 2018-03-09 4:50 ` Tomasz Figa [not found] ` <CAAFQd5BU+hU8aPyq6Rcaiwzu1sf7vcRNwnzt5LZZ+L01DnjqhA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-03-09 17:36 ` Robin Murphy 2018-03-09 17:36 ` Robin Murphy 2018-03-02 10:10 ` [PATCH v8 4/5] iommu/arm-smmu: Add the device_link between masters and smmu Vivek Gautam 2018-03-02 10:10 ` Vivek Gautam [not found] ` <20180302101050.6191-5-vivek.gautam-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org> 2018-03-07 12:47 ` Robin Murphy 2018-03-07 12:47 ` Robin Murphy 2018-03-08 4:59 ` Vivek Gautam 2018-03-08 4:59 ` Vivek Gautam [not found] ` <CAFp+6iF1oM=fmRCqSG-SxcUVvvOLet_Y0p7pmGn+=B-LdMNiww-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-03-09 7:11 ` Vivek Gautam [this message] 2018-03-09 7:11 ` Vivek Gautam [not found] ` <CAFp+6iFzGYWZMLkNrN1ZJJ2xH4CxQsiU6oYboHDzL0jDwm+4VQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-03-09 12:34 ` Robin Murphy 2018-03-09 12:34 ` Robin Murphy [not found] ` <f6822915-8921-c9cc-218a-f094ac5ed032-5wv7dgnIgG8@public.gmane.org> 2018-03-12 10:21 ` Vivek Gautam 2018-03-12 10:21 ` Vivek Gautam [not found] ` <f3011ef1-7ffe-8c2b-b9d6-3fb094789656-5wv7dgnIgG8@public.gmane.org> 2018-03-09 10:40 ` Vivek Gautam 2018-03-09 10:40 ` Vivek Gautam 2018-03-02 10:10 ` [PATCH v8 5/5] iommu/arm-smmu: Add support for qcom,smmu-v2 variant Vivek Gautam 2018-03-02 10:10 ` Vivek Gautam 2018-03-05 13:25 ` [PATCH v8 0/5] iommu/arm-smmu: Add runtime pm/sleep support Tomasz Figa 2018-03-05 13:25 ` Tomasz Figa [not found] ` <CAAFQd5AZoCgVts=DOET7js5VPi4ONM2m9R-WM6pWHud26XDVfA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org> 2018-03-05 17:19 ` Vivek Gautam 2018-03-05 17:19 ` Vivek Gautam
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=CAFp+6iFzGYWZMLkNrN1ZJJ2xH4CxQsiU6oYboHDzL0jDwm+4VQ@mail.gmail.com \ --to=vivek.gautam-sgv2jx0feol9jmxxk+q4oq@public.gmane.org \ --cc=devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org \ --cc=linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org \ --cc=mark.rutland-5wv7dgnIgG8@public.gmane.org \ --cc=rjw-LthD3rsA81gm4RdzfppkhA@public.gmane.org \ --cc=robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org \ --cc=robin.murphy-5wv7dgnIgG8@public.gmane.org \ --cc=sboyd-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org \ --cc=will.deacon-5wv7dgnIgG8@public.gmane.org \ /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.