On 23/01/2020 15:16, Dmitry Osipenko wrote:

23.01.2020 12:22, Sameer Pujar пишет:

quoted

On 1/22/2020 9:57 PM, Dmitry Osipenko wrote:

quoted

External email: Use caution opening links or attachments


22.01.2020 14:52, Jon Hunter пишет:

quoted

On 22/01/2020 07:16, Sameer Pujar wrote:

...

quoted

quoted

quoted

quoted

quoted

quoted

quoted

+static int tegra210_i2s_remove(struct platform_device *pdev)
+{
+     pm_runtime_disable(&pdev->dev);
+     if (!pm_runtime_status_suspended(&pdev->dev))
+             tegra210_i2s_runtime_suspend(&pdev->dev);

This breaks device's RPM refcounting if it was disabled in the
active
state. This code should be removed. At most you could warn
about the
unxpected RPM state here, but it shouldn't be necessary.

I guess this was added for safety and explicit suspend keeps clock
disabled.
Not sure if ref-counting of the device matters when runtime PM is
disabled and device is removed.
I see few drivers using this way.

It should matter (if I'm not missing something) because RPM should
be in
a wrecked state once you'll try to re-load the driver's module.
Likely
that those few other drivers are wrong.

[snip]

Once the driver is re-loaded and RPM is enabled, I don't think it
would use
the same 'dev' and the corresponding ref count. Doesn't it use the
new
counters?
If RPM is not working for some reason, most likely it would be the
case
for other
devices. What best driver can do is probably do a force suspend
during
removal if
already not done. I would prefer to keep, since multiple drivers
still
have it,
unless there is a real harm in doing so.

I took a closer look and looks like the counter actually should be
reset. Still I don't think that it's a good practice to make changes
underneath of RPM, it may strike back.

If RPM is broken, it probably would have been caught during device
usage.
I will remove explicit suspend here if no any concerns from other
folks.
Thanks.

I recall that this was the preferred way of doing this from the RPM
folks. Tegra30 I2S driver does the same and Stephen had pointed me to
this as a reference.
I believe that this is meant to ensure that the
device is always powered-off regardless of it RPM is enabled or not and
what the current state is.

Yes, it was kinda actual for the case of unavailable RPM.

quoted

Anyways, /I think/ variant like this should have been more preferred:

if (!pm_runtime_enabled(&pdev->dev))
         tegra210_i2s_runtime_suspend(&pdev->dev);
else
         pm_runtime_disable(&pdev->dev);

I think it looks to be similar to what is there already.

pm_runtime_disable(&pdev->dev); // it would turn out to be a dummy call
if !RPM
if (!pm_runtime_status_suspended(&pdev->dev)) // it is true always if !RPM
        tegra210_i2s_runtime_suspend(&pdev->dev);

Maybe this is fine for !RPM, but not really fine in a case of enabled
RPM. Device could be in resumed state after pm_runtime_disable() if it
wasn't suspended before the disabling.

I don't see any problem with this for the !RPM case.

Jon

-- 
nvpublic