On Wed, Jul 17, 2024 at 07:48:26AM +0900, Damien Le Moal wrote:
On 7/17/24 01:11, Johan Hovold wrote:
This reverts commit 7a6bbc2829d4ab592c7e440a6f6f5deb3cd95db4.
The offending commit tried to suppress a double "Starting disk" message for some drivers, but instead started spamming the log with bogus messages every five seconds:
[ 311.798956] sd 0:0:0:0: [sda] Starting disk [ 316.919103] sd 0:0:0:0: [sda] Starting disk [ 322.040775] sd 0:0:0:0: [sda] Starting disk [ 327.161140] sd 0:0:0:0: [sda] Starting disk [ 332.281352] sd 0:0:0:0: [sda] Starting disk [ 337.401878] sd 0:0:0:0: [sda] Starting disk [ 342.521527] sd 0:0:0:0: [sda] Starting disk [ 345.850401] sd 0:0:0:0: [sda] Starting disk [ 350.967132] sd 0:0:0:0: [sda] Starting disk [ 356.090454] sd 0:0:0:0: [sda] Starting disk ...
on machines that do not actually stop the disk on runtime suspend (e.g. the Qualcomm sc8280xp CRD with UFS).
This is odd. If the disk is not being being suspended, why does the platform even enable runtime PM for it ?
This is clearly intended to be supported as sd_do_start_stop() returns false and that prevents sd_start_stop_device() from being called on resume (and similarly on suspend which is why there are no matching stopping disk messages above):
[ 32.822189] sd 0:0:0:0: sd_resume_common - runtime = 1, sd_do_start_stop = 0, manage_runtime_start_stop = 0
Are you sure about this ? Or is it simply that the runtime pm timer is set to a very low interval ?
I haven't tried to determine why runtime pm is used this way, but your patch is clearly broken as it prints a message about starting the disk even when sd_do_start_stop() returns false.
It almost sound like what we need to do here is suppress this message for the runtime resume case, so something like:
No, that would only make things worse as I assume you'd have a stopped disk message without a matching start message for driver that do end up stopping the disk here.
However, I would like to make sure that this platform is not calling sd_resume_runtime() for nothing every 5s. If that is the case, then there is a more fundamental problem here and reverting this patch is only hiding that.
This is with the Qualcomm UFS driver, but it seems it just relies on the generic ufshcd_pltfrm_init() implementation.
Also not sure why anyone would want to see these messages on every runtime suspend (for drivers that end up taking this path), but that's a separate discussion.
Johan