On Wed, Jul 17, 2024 at 07:46:14PM +0900, Damien Le Moal wrote: > On 7/17/24 18:00, Johan Hovold wrote: > > 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 > > Yes, so we can suppress the "Starting disk" message for runtime resume, to match > the runtime suspend not having the message. No, the point is that the stopping disk message is also suppressed when sd_do_start_stop() returns false (i.e. when sd_start_stop_device() is never called). See sd_suspend_common(). > >> 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. > > The patch is not *that* broken, because sd_do_start_stop() returning false mean > only that the disk will *not* be started using a START STOP UNIT command. But > the underlying LLD must start the drive. So the message is not wrong, even > though it is probably best to suppress it for the runtime case.
