| Message ID | 20180708173413.1965-4-vivek.gautam@codeaurora.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Sunday, July 8, 2018 7:34:12 PM CEST 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> > Reviewed-by: Tomasz Figa <tfiga@chromium.org> > Cc: Rafael J. Wysocki <rjw@rjwysocki.net> > Cc: Lukas Wunner <lukas@wunner.de> > --- > > - Change since v11 > * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. > > drivers/iommu/arm-smmu.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c > index 09265e206e2d..916cde4954d2 100644 > --- a/drivers/iommu/arm-smmu.c > +++ b/drivers/iommu/arm-smmu.c > @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) > > iommu_device_link(&smmu->iommu, dev); > > + if (pm_runtime_enabled(smmu->dev) && Why does the creation of the link depend on whether or not runtime PM is enabled for the MMU device? What about system-wide PM and system shutdown? Are they always guaranteed to happen in the right order without the link? > + !device_link_add(dev, smmu->dev, > + DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER)) { > + dev_err(smmu->dev, "Unable to add link to the consumer %s\n", > + dev_name(dev)); > + ret = -ENODEV; > + goto out_unlink; > + } > + > return 0; > > +out_unlink: > + iommu_device_unlink(&smmu->iommu, dev); > + arm_smmu_master_free_smes(fwspec); > out_cfg_free: > kfree(cfg); > out_free: >
Hi Rafael, On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote: > On Sunday, July 8, 2018 7:34:12 PM CEST 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> >> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >> Cc: Lukas Wunner <lukas@wunner.de> >> --- >> >> - Change since v11 >> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >> >> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> index 09265e206e2d..916cde4954d2 100644 >> --- a/drivers/iommu/arm-smmu.c >> +++ b/drivers/iommu/arm-smmu.c >> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) >> >> iommu_device_link(&smmu->iommu, dev); >> >> + if (pm_runtime_enabled(smmu->dev) && > Why does the creation of the link depend on whether or not runtime PM > is enabled for the MMU device? The main purpose of this device link is to handle the runtime PM synchronization between the supplier (iommu) and consumer (client devices, such as GPU/display). Moreover, the runtime pm is conditionally enabled for smmu devices that support such [1]. > > What about system-wide PM and system shutdown? Are they always guaranteed > to happen in the right order without the link? When there's no runtime PM, there's no clocks, and other resources to be handled. So, we don't need device link for system-wide PM and system shutdown to work correctly. That's the case with current arm-smmu driver. Is it something that i am missing here? [1] https://lkml.org/lkml/2018/3/8/775 Thanks Vivek >> + !device_link_add(dev, smmu->dev, >> + DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER)) { >> + dev_err(smmu->dev, "Unable to add link to the consumer %s\n", >> + dev_name(dev)); >> + ret = -ENODEV; >> + goto out_unlink; >> + } >> + >> return 0; >> >> +out_unlink: >> + iommu_device_unlink(&smmu->iommu, dev); >> + arm_smmu_master_free_smes(fwspec); >> out_cfg_free: >> kfree(cfg); >> out_free: >> >
Hi Rafael, On Wed, Jul 11, 2018 at 4:06 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > Hi Rafael, > > > > On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote: >> >> On Sunday, July 8, 2018 7:34:12 PM CEST 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> >>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>> Cc: Lukas Wunner <lukas@wunner.de> >>> --- >>> >>> - Change since v11 >>> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >>> >>> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>> index 09265e206e2d..916cde4954d2 100644 >>> --- a/drivers/iommu/arm-smmu.c >>> +++ b/drivers/iommu/arm-smmu.c >>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) >>> iommu_device_link(&smmu->iommu, dev); >>> + if (pm_runtime_enabled(smmu->dev) && >> >> Why does the creation of the link depend on whether or not runtime PM >> is enabled for the MMU device? > > > The main purpose of this device link is to handle the runtime PM > synchronization > between the supplier (iommu) and consumer (client devices, such as > GPU/display). > Moreover, the runtime pm is conditionally enabled for smmu devices that > support > such [1]. Is there something you would like me to modify in this patch? Best regards Vivek >> >> >> What about system-wide PM and system shutdown? Are they always guaranteed >> to happen in the right order without the link? > > > When there's no runtime PM, there's no clocks, and other resources to be > handled. > So, we don't need device link for system-wide PM and system shutdown to work > correctly. > That's the case with current arm-smmu driver. > Is it something that i am missing here? > > [1] https://lkml.org/lkml/2018/3/8/775 > > Thanks > Vivek >>> >>> + !device_link_add(dev, smmu->dev, >>> + DL_FLAG_PM_RUNTIME | >>> DL_FLAG_AUTOREMOVE_SUPPLIER)) { >>> + dev_err(smmu->dev, "Unable to add link to the consumer >>> %s\n", >>> + dev_name(dev)); >>> + ret = -ENODEV; >>> + goto out_unlink; >>> + } >>> + >>> return 0; >>> +out_unlink: >>> + iommu_device_unlink(&smmu->iommu, dev); >>> + arm_smmu_master_free_smes(fwspec); >>> out_cfg_free: >>> kfree(cfg); >>> out_free: >>> >> > > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jul 12, 2018 at 2:41 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > Hi Rafael, > > > On Wed, Jul 11, 2018 at 4:06 PM, Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: >> Hi Rafael, >> >> >> >> On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote: >>> >>> On Sunday, July 8, 2018 7:34:12 PM CEST 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> >>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>>> Cc: Lukas Wunner <lukas@wunner.de> >>>> --- >>>> >>>> - Change since v11 >>>> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >>>> >>>> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>> index 09265e206e2d..916cde4954d2 100644 >>>> --- a/drivers/iommu/arm-smmu.c >>>> +++ b/drivers/iommu/arm-smmu.c >>>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) >>>> iommu_device_link(&smmu->iommu, dev); >>>> + if (pm_runtime_enabled(smmu->dev) && >>> >>> Why does the creation of the link depend on whether or not runtime PM >>> is enabled for the MMU device? >> >> >> The main purpose of this device link is to handle the runtime PM >> synchronization >> between the supplier (iommu) and consumer (client devices, such as >> GPU/display). >> Moreover, the runtime pm is conditionally enabled for smmu devices that >> support >> such [1]. > > Is there something you would like me to modify in this patch? Not really, as long as you are sure that it is correct. :-) You need to remember, however, that if you add system-wide PM callbacks to the driver, the ordering between them and the client device callbacks during system-wide suspend matters as well. Don't you need the link the ensure the correct system-wide suspend ordering too?
On 7/16/2018 2:25 PM, Rafael J. Wysocki wrote: > On Thu, Jul 12, 2018 at 2:41 PM, Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: >> Hi Rafael, >> >> >> On Wed, Jul 11, 2018 at 4:06 PM, Vivek Gautam >> <vivek.gautam@codeaurora.org> wrote: >>> Hi Rafael, >>> >>> >>> >>> On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote: >>>> On Sunday, July 8, 2018 7:34:12 PM CEST 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> >>>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>>>> Cc: Lukas Wunner <lukas@wunner.de> >>>>> --- >>>>> >>>>> - Change since v11 >>>>> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >>>>> >>>>> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >>>>> 1 file changed, 12 insertions(+) >>>>> >>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>>> index 09265e206e2d..916cde4954d2 100644 >>>>> --- a/drivers/iommu/arm-smmu.c >>>>> +++ b/drivers/iommu/arm-smmu.c >>>>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) >>>>> iommu_device_link(&smmu->iommu, dev); >>>>> + if (pm_runtime_enabled(smmu->dev) && >>>> Why does the creation of the link depend on whether or not runtime PM >>>> is enabled for the MMU device? >>> >>> The main purpose of this device link is to handle the runtime PM >>> synchronization >>> between the supplier (iommu) and consumer (client devices, such as >>> GPU/display). >>> Moreover, the runtime pm is conditionally enabled for smmu devices that >>> support >>> such [1]. >> Is there something you would like me to modify in this patch? > Not really, as long as you are sure that it is correct. :-) > > You need to remember, however, that if you add system-wide PM > callbacks to the driver, the ordering between them and the client > device callbacks during system-wide suspend matters as well. Don't > you need the link the ensure the correct system-wide suspend ordering > too? The fact that currently we handle clocks only through runtime pm callbacks, would it be better to call runtime pm put/get in system-wide PM callbacks. This would be same as i mentioned in the other thread. Best regards Vivek > -- > To unsubscribe from this list: send the line "unsubscribe linux-arm-msm" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jul 16, 2018 at 1:46 PM, Vivek Gautam <vivek.gautam@codeaurora.org> wrote: > > > On 7/16/2018 2:25 PM, Rafael J. Wysocki wrote: >> >> On Thu, Jul 12, 2018 at 2:41 PM, Vivek Gautam >> <vivek.gautam@codeaurora.org> wrote: >>> >>> Hi Rafael, >>> >>> >>> On Wed, Jul 11, 2018 at 4:06 PM, Vivek Gautam >>> <vivek.gautam@codeaurora.org> wrote: >>>> >>>> Hi Rafael, >>>> >>>> >>>> >>>> On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote: >>>>> >>>>> On Sunday, July 8, 2018 7:34:12 PM CEST 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> >>>>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>>>>> Cc: Lukas Wunner <lukas@wunner.de> >>>>>> --- >>>>>> >>>>>> - Change since v11 >>>>>> * Replaced DL_FLAG_AUTOREMOVE flag with >>>>>> DL_FLAG_AUTOREMOVE_SUPPLIER. >>>>>> >>>>>> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >>>>>> 1 file changed, 12 insertions(+) >>>>>> >>>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>>>> index 09265e206e2d..916cde4954d2 100644 >>>>>> --- a/drivers/iommu/arm-smmu.c >>>>>> +++ b/drivers/iommu/arm-smmu.c >>>>>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device >>>>>> *dev) >>>>>> iommu_device_link(&smmu->iommu, dev); >>>>>> + if (pm_runtime_enabled(smmu->dev) && >>>>> >>>>> Why does the creation of the link depend on whether or not runtime PM >>>>> is enabled for the MMU device? >>>> >>>> >>>> The main purpose of this device link is to handle the runtime PM >>>> synchronization >>>> between the supplier (iommu) and consumer (client devices, such as >>>> GPU/display). >>>> Moreover, the runtime pm is conditionally enabled for smmu devices that >>>> support >>>> such [1]. >>> >>> Is there something you would like me to modify in this patch? >> >> Not really, as long as you are sure that it is correct. :-) >> >> You need to remember, however, that if you add system-wide PM >> callbacks to the driver, the ordering between them and the client >> device callbacks during system-wide suspend matters as well. Don't >> you need the link the ensure the correct system-wide suspend ordering >> too? > > > The fact that currently we handle clocks only through runtime pm callbacks, > would it be better to call runtime pm put/get in system-wide PM callbacks. > This would be same as i mentioned in the other thread. Well, my point is that there's no reason for system-wide suspend to depend directly on runtime PM (which can be effectively disabled by user space as mentioned for multiple times in this thread). It simply is not efficient to let the clock run while the system as a whole is sleeping (even if power/control has been set to "on" for this particular device) and it should not be too hard to prevent that from happening. You may need an additional flag in the driver for that or similar, but it definitely should be doable. Now, that's my advice and I'm not the maintainer of that code, so it is your call (as long as the maintainer agrees with it). Thanks, Rafael
On 7/17/2018 1:16 PM, Rafael J. Wysocki wrote: > On Mon, Jul 16, 2018 at 1:46 PM, Vivek Gautam > <vivek.gautam@codeaurora.org> wrote: >> >> On 7/16/2018 2:25 PM, Rafael J. Wysocki wrote: >>> On Thu, Jul 12, 2018 at 2:41 PM, Vivek Gautam >>> <vivek.gautam@codeaurora.org> wrote: >>>> Hi Rafael, >>>> >>>> >>>> On Wed, Jul 11, 2018 at 4:06 PM, Vivek Gautam >>>> <vivek.gautam@codeaurora.org> wrote: >>>>> Hi Rafael, >>>>> >>>>> >>>>> >>>>> On 7/11/2018 3:23 PM, Rafael J. Wysocki wrote: >>>>>> On Sunday, July 8, 2018 7:34:12 PM CEST 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> >>>>>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>>>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>>>>>> Cc: Lukas Wunner <lukas@wunner.de> >>>>>>> --- >>>>>>> >>>>>>> - Change since v11 >>>>>>> * Replaced DL_FLAG_AUTOREMOVE flag with >>>>>>> DL_FLAG_AUTOREMOVE_SUPPLIER. >>>>>>> >>>>>>> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >>>>>>> 1 file changed, 12 insertions(+) >>>>>>> >>>>>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>>>>> index 09265e206e2d..916cde4954d2 100644 >>>>>>> --- a/drivers/iommu/arm-smmu.c >>>>>>> +++ b/drivers/iommu/arm-smmu.c >>>>>>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device >>>>>>> *dev) >>>>>>> iommu_device_link(&smmu->iommu, dev); >>>>>>> + if (pm_runtime_enabled(smmu->dev) && >>>>>> Why does the creation of the link depend on whether or not runtime PM >>>>>> is enabled for the MMU device? >>>>> >>>>> The main purpose of this device link is to handle the runtime PM >>>>> synchronization >>>>> between the supplier (iommu) and consumer (client devices, such as >>>>> GPU/display). >>>>> Moreover, the runtime pm is conditionally enabled for smmu devices that >>>>> support >>>>> such [1]. >>>> Is there something you would like me to modify in this patch? >>> Not really, as long as you are sure that it is correct. :-) >>> >>> You need to remember, however, that if you add system-wide PM >>> callbacks to the driver, the ordering between them and the client >>> device callbacks during system-wide suspend matters as well. Don't >>> you need the link the ensure the correct system-wide suspend ordering >>> too? >> >> The fact that currently we handle clocks only through runtime pm callbacks, >> would it be better to call runtime pm put/get in system-wide PM callbacks. >> This would be same as i mentioned in the other thread. > Well, my point is that there's no reason for system-wide suspend to > depend directly on runtime PM (which can be effectively disabled by > user space as mentioned for multiple times in this thread). > > It simply is not efficient to let the clock run while the system as a > whole is sleeping (even if power/control has been set to "on" for this > particular device) and it should not be too hard to prevent that from > happening. You may need an additional flag in the driver for that or > similar, but it definitely should be doable. Right, I will modify the things are required. Thanks again for pointing this out. Best regards Vivek > > Now, that's my advice and I'm not the maintainer of that code, so it > is your call (as long as the maintainer agrees with it). > > Thanks, > Rafael
On Wed, Jul 11, 2018 at 3:23 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: > On Sunday, July 8, 2018 7:34:12 PM CEST 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> >> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >> Cc: Lukas Wunner <lukas@wunner.de> >> --- >> >> - Change since v11 >> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >> >> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >> index 09265e206e2d..916cde4954d2 100644 >> --- a/drivers/iommu/arm-smmu.c >> +++ b/drivers/iommu/arm-smmu.c >> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) >> >> iommu_device_link(&smmu->iommu, dev); >> >> + if (pm_runtime_enabled(smmu->dev) && > > Why does the creation of the link depend on whether or not runtime PM > is enabled for the MMU device? > > What about system-wide PM and system shutdown? Are they always guaranteed > to happen in the right order without the link? Hi Robin, As Rafael pointed, we should the device link creation should not depend on runtime PM being enabled or not, as we would also want to guarantee that system wide PM callbacks are called in the right order for smmu and clients. Does this change of removing the check for pm_runtime_enabled() from here looks okay to you? FYI, as discussed in the first patch [1] of this series, I will add a system wide suspend callback - arm_smmu_pm_suspend, that would do clock disable, and will add corresponding clock enable calls in arm_smmu_pm_resume(). [1] https://lore.kernel.org/patchwork/patch/960460/ Best regards Vivek > >> + !device_link_add(dev, smmu->dev, >> + DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER)) { >> + dev_err(smmu->dev, "Unable to add link to the consumer %s\n", >> + dev_name(dev)); >> + ret = -ENODEV; >> + goto out_unlink; >> + } >> + >> return 0; >> >> +out_unlink: >> + iommu_device_unlink(&smmu->iommu, dev); >> + arm_smmu_master_free_smes(fwspec); >> out_cfg_free: >> kfree(cfg); >> out_free: >> > >
On 18/07/18 10:30, Vivek Gautam wrote: > On Wed, Jul 11, 2018 at 3:23 PM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote: >> On Sunday, July 8, 2018 7:34:12 PM CEST 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> >>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>> Cc: Lukas Wunner <lukas@wunner.de> >>> --- >>> >>> - Change since v11 >>> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >>> >>> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >>> 1 file changed, 12 insertions(+) >>> >>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>> index 09265e206e2d..916cde4954d2 100644 >>> --- a/drivers/iommu/arm-smmu.c >>> +++ b/drivers/iommu/arm-smmu.c >>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) >>> >>> iommu_device_link(&smmu->iommu, dev); >>> >>> + if (pm_runtime_enabled(smmu->dev) && >> >> Why does the creation of the link depend on whether or not runtime PM >> is enabled for the MMU device? >> >> What about system-wide PM and system shutdown? Are they always guaranteed >> to happen in the right order without the link? > > Hi Robin, > > As Rafael pointed, we should the device link creation should not depend on > runtime PM being enabled or not, as we would also want to guarantee > that system wide PM callbacks are called in the right order for smmu > and clients. > > Does this change of removing the check for pm_runtime_enabled() from here > looks okay to you? FWIW the existing system PM ops make no claim to be perfect, and I wouldn't be at all surprised if it was only by coincidence that my devices happened to put on the relevant lists in the right order to start with. If we no longer need to worry about explicit device_link housekeeping in the SMMU driver, then creating them unconditionally sounds like the sensible thing to do. I'd be inclined to treat failure as non-fatal like we do for the sysfs link, though, since it's another thing that correct SMMU operation doesn't actually depend on (at this point we don't necessarily know if this consumer even has a driver at all). > FYI, as discussed in the first patch [1] of this series, I will add a > system wide > suspend callback - arm_smmu_pm_suspend, that would do clock disable, and will > add corresponding clock enable calls in arm_smmu_pm_resume(). OK, I still don't really understand the finer points of how system PM and runtime PM interact, but if making it robust is just a case of calling the runtime suspend/resume hooks as appropriate from the system ones, that sounds reasonable. Robin. > > [1] https://lore.kernel.org/patchwork/patch/960460/ > > > Best regards > Vivek > >> >>> + !device_link_add(dev, smmu->dev, >>> + DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER)) { >>> + dev_err(smmu->dev, "Unable to add link to the consumer %s\n", >>> + dev_name(dev)); >>> + ret = -ENODEV; >>> + goto out_unlink; >>> + } >>> + >>> return 0; >>> >>> +out_unlink: >>> + iommu_device_unlink(&smmu->iommu, dev); >>> + arm_smmu_master_free_smes(fwspec); >>> out_cfg_free: >>> kfree(cfg); >>> out_free: >>> >> >> > > >
On Wed, Jul 18, 2018 at 6:13 PM, Robin Murphy <robin.murphy@arm.com> wrote: > On 18/07/18 10:30, Vivek Gautam wrote: >> >> On Wed, Jul 11, 2018 at 3:23 PM, Rafael J. Wysocki <rjw@rjwysocki.net> >> wrote: >>> >>> On Sunday, July 8, 2018 7:34:12 PM CEST 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> >>>> Reviewed-by: Tomasz Figa <tfiga@chromium.org> >>>> Cc: Rafael J. Wysocki <rjw@rjwysocki.net> >>>> Cc: Lukas Wunner <lukas@wunner.de> >>>> --- >>>> >>>> - Change since v11 >>>> * Replaced DL_FLAG_AUTOREMOVE flag with DL_FLAG_AUTOREMOVE_SUPPLIER. >>>> >>>> drivers/iommu/arm-smmu.c | 12 ++++++++++++ >>>> 1 file changed, 12 insertions(+) >>>> >>>> diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c >>>> index 09265e206e2d..916cde4954d2 100644 >>>> --- a/drivers/iommu/arm-smmu.c >>>> +++ b/drivers/iommu/arm-smmu.c >>>> @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device >>>> *dev) >>>> >>>> iommu_device_link(&smmu->iommu, dev); >>>> >>>> + if (pm_runtime_enabled(smmu->dev) && >>> >>> >>> Why does the creation of the link depend on whether or not runtime PM >>> is enabled for the MMU device? >>> >>> What about system-wide PM and system shutdown? Are they always >>> guaranteed >>> to happen in the right order without the link? >> >> >> Hi Robin, >> >> As Rafael pointed, we should the device link creation should not depend on >> runtime PM being enabled or not, as we would also want to guarantee >> that system wide PM callbacks are called in the right order for smmu >> and clients. >> >> Does this change of removing the check for pm_runtime_enabled() from here >> looks okay to you? > > > FWIW the existing system PM ops make no claim to be perfect, and I wouldn't > be at all surprised if it was only by coincidence that my devices happened > to put on the relevant lists in the right order to start with. If we no > longer need to worry about explicit device_link housekeeping in the SMMU > driver, then creating them unconditionally sounds like the sensible thing to > do. I'd be inclined to treat failure as non-fatal like we do for the sysfs > link, though, since it's another thing that correct SMMU operation doesn't > actually depend on (at this point we don't necessarily know if this consumer > even has a driver at all). Thanks. I will then respin the patch taking care of treating failure as non-fatal. >> FYI, as discussed in the first patch [1] of this series, I will add a >> system wide >> suspend callback - arm_smmu_pm_suspend, that would do clock disable, and >> will >> add corresponding clock enable calls in arm_smmu_pm_resume(). > > > OK, I still don't really understand the finer points of how system PM and > runtime PM interact, but if making it robust is just a case of calling the > runtime suspend/resume hooks as appropriate from the system ones, that > sounds reasonable. Sure. Thanks. Best regards Vivek > > Robin. > >> >> [1] https://lore.kernel.org/patchwork/patch/960460/ >> >> >> Best regards >> Vivek >> >>> >>>> + !device_link_add(dev, smmu->dev, >>>> + DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER)) >>>> { >>>> + dev_err(smmu->dev, "Unable to add link to the consumer >>>> %s\n", >>>> + dev_name(dev)); >>>> + ret = -ENODEV; >>>> + goto out_unlink; >>>> + } >>>> + >>>> return 0; >>>> >>>> +out_unlink: >>>> + iommu_device_unlink(&smmu->iommu, dev); >>>> + arm_smmu_master_free_smes(fwspec); >>>> out_cfg_free: >>>> kfree(cfg); >>>> out_free: >>>> >>> >>> >> >> >> > _______________________________________________ > iommu mailing list > iommu@lists.linux-foundation.org > https://lists.linuxfoundation.org/mailman/listinfo/iommu
diff --git a/drivers/iommu/arm-smmu.c b/drivers/iommu/arm-smmu.c index 09265e206e2d..916cde4954d2 100644 --- a/drivers/iommu/arm-smmu.c +++ b/drivers/iommu/arm-smmu.c @@ -1461,8 +1461,20 @@ static int arm_smmu_add_device(struct device *dev) iommu_device_link(&smmu->iommu, dev); + if (pm_runtime_enabled(smmu->dev) && + !device_link_add(dev, smmu->dev, + DL_FLAG_PM_RUNTIME | DL_FLAG_AUTOREMOVE_SUPPLIER)) { + dev_err(smmu->dev, "Unable to add link to the consumer %s\n", + dev_name(dev)); + ret = -ENODEV; + goto out_unlink; + } + return 0; +out_unlink: + iommu_device_unlink(&smmu->iommu, dev); + arm_smmu_master_free_smes(fwspec); out_cfg_free: kfree(cfg); out_free: