| Message ID | 1517999482-17317-5-git-send-email-vivek.gautam@codeaurora.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Vivek, Thanks for the patch. Please see my comments inline. On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam <vivek.gautam@codeaurora.org> 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 | 16 ++++++++++++++++ > 1 file changed, 16 insertions(+) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index c024f69c1682..c7e924d553bd 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -215,6 +215,9 @@ struct arm_smmu_device { > > /* IOMMU core code handle */ > struct iommu_device iommu; > + > + /* runtime PM link to master */ > + struct device_link *link; > }; > > enum arm_smmu_context_fmt { > @@ -1425,6 +1428,17 @@ static int arm_smmu_add_device(struct device *dev) > > pm_runtime_put_sync(smmu->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); > + if (!smmu->link) > + dev_warn(smmu->dev, > + "Unable to create device link between %s and %s\n", > + dev_name(smmu->dev), dev_name(dev)); How likely it is that the master can work normally even if the link add fails? Perhaps we should just return an error here? > + > return 0; > > out_rpm_put: > @@ -1449,6 +1463,8 @@ static void arm_smmu_remove_device(struct device *dev) > cfg = fwspec->iommu_priv; > smmu = cfg->smmu; > > + device_link_del(smmu->link); We allowed smmu->link in arm_smmu_add_device(), but here we don't check it. Looking at the code, device_link_del() doesn't seem to check either. Note that this problem would go away if we fail add_device on device_link_add() failure, as I suggested above, so no change would be necessary. Best regards, Tomasz
Hi Tomasz, On Tue, Feb 13, 2018 at 2:01 PM, Tomasz Figa <tfiga@chromium.org> wrote: > Hi Vivek, > > Thanks for the patch. Please see my comments inline. Thanks for reviewing the patch series. > > On Wed, Feb 7, 2018 at 7:31 PM, Vivek Gautam > <vivek.gautam@codeaurora.org> 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 | 16 ++++++++++++++++ >> 1 file changed, 16 insertions(+) >> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> index c024f69c1682..c7e924d553bd 100644 >> --- a/drivers/iommu/arm-smmu.c >> +++ b/drivers/iommu/arm-smmu.c >> @@ -215,6 +215,9 @@ struct arm_smmu_device { >> >> /* IOMMU core code handle */ >> struct iommu_device iommu; >> + >> + /* runtime PM link to master */ >> + struct device_link *link; >> }; >> >> enum arm_smmu_context_fmt { >> @@ -1425,6 +1428,17 @@ static int arm_smmu_add_device(struct device *dev) >> >> pm_runtime_put_sync(smmu->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); >> + if (!smmu->link) >> + dev_warn(smmu->dev, >> + "Unable to create device link between %s and %s\n", >> + dev_name(smmu->dev), dev_name(dev)); > > How likely it is that the master can work normally even if the link > add fails? Perhaps we should just return an error here? Right. We are assuming that the power is handled for most of the smmu operations, after we add the master with smmu, based on the fact that the device link is successful. We should return error code here. Will make the necessary change. > >> + >> return 0; >> >> out_rpm_put: >> @@ -1449,6 +1463,8 @@ static void arm_smmu_remove_device(struct device *dev) >> cfg = fwspec->iommu_priv; >> smmu = cfg->smmu; >> >> + device_link_del(smmu->link); > > We allowed smmu->link in arm_smmu_add_device(), but here we don't > check it. Looking at the code, device_link_del() doesn't seem to check > either. > > Note that this problem would go away if we fail add_device on > device_link_add() failure, as I suggested above, so no change would be > necessary. Sure. After making the above change, this should also be handled. Best regards Vivek > > Best regards, > Tomasz
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index c024f69c1682..c7e924d553bd 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -215,6 +215,9 @@ struct arm_smmu_device { /* IOMMU core code handle */ struct iommu_device iommu; + + /* runtime PM link to master */ + struct device_link *link; }; enum arm_smmu_context_fmt { @@ -1425,6 +1428,17 @@ static int arm_smmu_add_device(struct device *dev) pm_runtime_put_sync(smmu->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); + if (!smmu->link) + dev_warn(smmu->dev, + "Unable to create device link between %s and %s\n", + dev_name(smmu->dev), dev_name(dev)); + return 0; out_rpm_put: @@ -1449,6 +1463,8 @@ static void arm_smmu_remove_device(struct device *dev) cfg = fwspec->iommu_priv; smmu = cfg->smmu; + device_link_del(smmu->link); + ret = pm_runtime_get_sync(smmu->dev); if (ret) return;