22.01.2020 14:52, Jon Hunter пишет:
On 22/01/2020 07:16, Sameer Pujar wrote:
...
>> +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.
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);
Now for Tegra210 (or actually 64-bit Tegra) RPM is always enabled and so we don't need to worry about the !RPM case. However, I still don't see the harm in this.
There is no real harm today, but:
1. I'd prefer to be very careful with RPM in general, based on previous experience.
2. It should be a bug if device isn't RPM-suspended during of driver's removal. Thus the real problem needs to be fixed rather than worked around.