Re: Allowing devices to set a minimum wait time at power down/reboot

2017-07-10 Thread Stefan Fritsch
On Sun, 9 Jul 2017, Mark Kettenis wrote:
> > there are some open issues that usb devices do not flush their caches fast 
> > enough (at least on some boards). And my mail about shortening the wait 
> > times in reboot(8) brought up that issue again.
> > 
> > My suggestion would be to have a global variable that takes the minimum 
> > delay for the next shutdown/reboot and affected devices could set that 
> > value in their DVACT_SUSPEND hook. That way, if a device is removed before 
> > the shudown, the delay won't trigger.
> > 
> > The diff below is only compile tested by I think it demonstrates the idea.
> > 
> > One could also do something more fancy like using different delays for 
> > reboot and powerdown, or making umass only set a new SDEV_ flag and then 
> > have the scsi layer only call set_powerdown_delay() if the device has been 
> > written to.
> > 
> > Opinions?
> 
> Such devices should make sure they flush their caches and properly
> wait for that operation to complete.

That would be preferable, but there is some evidence that not all usb 
thumbdrives implement cache flush correctly:

http://marc.info/?l=openbsd-misc&m=144237305322074&w=2
http://marc.info/?l=openbsd-tech&m=147688940628911&w=2

The fact that it only happens on a few mainboards could be explained if it 
depends on how quickly the mainboard powers off the usb bus on shutdown 
(and reset).



Re: Allowing devices to set a minimum wait time at power down/reboot

2017-07-09 Thread Mark Kettenis
> Date: Sun, 9 Jul 2017 14:12:34 +0200 (CEST)
> From: Stefan Fritsch 
> 
> Hi,
> 
> there are some open issues that usb devices do not flush their caches fast 
> enough (at least on some boards). And my mail about shortening the wait 
> times in reboot(8) brought up that issue again.
> 
> My suggestion would be to have a global variable that takes the minimum 
> delay for the next shutdown/reboot and affected devices could set that 
> value in their DVACT_SUSPEND hook. That way, if a device is removed before 
> the shudown, the delay won't trigger.
> 
> The diff below is only compile tested by I think it demonstrates the idea.
> 
> One could also do something more fancy like using different delays for 
> reboot and powerdown, or making umass only set a new SDEV_ flag and then 
> have the scsi layer only call set_powerdown_delay() if the device has been 
> written to.
> 
> Opinions?

Such devices should make sure they flush their caches and properly
wait for that operation to complete.



Allowing devices to set a minimum wait time at power down/reboot

2017-07-09 Thread Stefan Fritsch
Hi,

there are some open issues that usb devices do not flush their caches fast 
enough (at least on some boards). And my mail about shortening the wait 
times in reboot(8) brought up that issue again.

My suggestion would be to have a global variable that takes the minimum 
delay for the next shutdown/reboot and affected devices could set that 
value in their DVACT_SUSPEND hook. That way, if a device is removed before 
the shudown, the delay won't trigger.

The diff below is only compile tested by I think it demonstrates the idea.

One could also do something more fancy like using different delays for 
reboot and powerdown, or making umass only set a new SDEV_ flag and then 
have the scsi layer only call set_powerdown_delay() if the device has been 
written to.

Opinions?

Cheers,
Stefan

diff --git a/sys/arch/amd64/amd64/machdep.c b/sys/arch/amd64/amd64/machdep.c
index 9653de19924..ed4bb545ec7 100644
--- a/sys/arch/amd64/amd64/machdep.c
+++ b/sys/arch/amd64/amd64/machdep.c
@@ -86,6 +86,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #include 
@@ -715,6 +716,7 @@ struct pcb dumppcb;
 __dead void
 boot(int howto)
 {
+   unsigned delay_ms;
if ((howto & RB_POWERDOWN) != 0)
lid_action = 0;
 
@@ -756,7 +758,8 @@ haltsys:
extern int acpi_enabled;
 
if (acpi_enabled) {
-   delay(50);
+   delay_ms = MAX(500, powerdown_delay);
+   delay(delay_ms * 1000);
if ((howto & RB_POWERDOWN) != 0)
acpi_powerdown();
}
@@ -770,8 +773,9 @@ haltsys:
}
 
printf("rebooting...\n");
-   if (cpureset_delay > 0)
-   delay(cpureset_delay * 1000);
+   delay_ms = MAX(cpureset_delay, powerdown_delay);
+   if (delay_ms > 0)
+   delay(delay_ms * 1000);
cpu_reset();
for (;;)
continue;
diff --git a/sys/dev/acpi/acpi.c b/sys/dev/acpi/acpi.c
index 09495657aad..d2380d6d78d 100644
--- a/sys/dev/acpi/acpi.c
+++ b/sys/dev/acpi/acpi.c
@@ -2444,6 +2444,10 @@ acpi_sleep_state(struct acpi_softc *sc, int sleepmode)
acpi_disable_allgpes(sc);
acpi_enable_wakegpes(sc, state);
 
+   if (powerdown_delay > 0) {
+   delay(1000 * powerdown_delay);
+   powerdown_delay = 0;
+   }
/* Sleep */
sc->sc_state = state;
error = acpi_sleep_cpu(sc, state);
diff --git a/sys/dev/usb/umass.c b/sys/dev/usb/umass.c
index 0fc1782ac21..8ff61b29c76 100644
--- a/sys/dev/usb/umass.c
+++ b/sys/dev/usb/umass.c
@@ -131,6 +131,7 @@
 #include 
 #include 
 #include 
+#include 
 #undef KASSERT
 #define KASSERT(cond, msg)
 #include 
@@ -178,13 +179,15 @@ char *states[TSTATE_STATES+1] = {
 int umass_match(struct device *, void *, void *); 
 void umass_attach(struct device *, struct device *, void *); 
 int umass_detach(struct device *, int); 
+int umass_activate(struct device *, int);
 
 struct cfdriver umass_cd = { 
NULL, "umass", DV_DULL 
 }; 
 
 const struct cfattach umass_ca = {
-   sizeof(struct umass_softc), umass_match, umass_attach, umass_detach
+   sizeof(struct umass_softc), umass_match, umass_attach, umass_detach,
+   umass_activate
 };
 
 void umass_disco(struct umass_softc *sc);
@@ -651,6 +654,20 @@ umass_detach(struct device *self, int flags)
return (rv);
 }
 
+int umass_activate(struct device *self, int act)
+{
+   int rv = 0;
+
+   switch (act) {
+   case DVACT_SUSPEND:
+   set_powerdown_delay(2000);
+   break;
+   }
+   rv = config_activate_children(self, act);
+
+   return (rv);
+}
+
 void
 umass_disco(struct umass_softc *sc)
 {
diff --git a/sys/kern/kern_xxx.c b/sys/kern/kern_xxx.c
index 975c6bc34d8..a3eeb48bc7f 100644
--- a/sys/kern/kern_xxx.c
+++ b/sys/kern/kern_xxx.c
@@ -40,6 +40,19 @@
 #include 
 #include 
 
+/*
+ * Minimum delay in ms before actual powerdown/reboot, in order
+ * to give time to devices for flushing caches.
+ */
+unsigned int powerdown_delay;
+
+void
+set_powerdown_delay(unsigned int ms)
+{
+   if (powerdown_delay < ms)
+   powerdown_delay = ms;
+}
+
 int
 sys_reboot(struct proc *p, void *v, register_t *retval)
 {
diff --git a/sys/sys/reboot.h b/sys/sys/reboot.h
index f82d87a8e9d..a58e772ad6f 100644
--- a/sys/sys/reboot.h
+++ b/sys/sys/reboot.h
@@ -96,8 +96,11 @@
 #ifdefined(_KERNEL) && !defined(_STANDALONE) && !defined(_LOCORE)
 
 __BEGIN_DECLS
+extern unsigned int powerdown_delay;
+
 __dead voidreboot(int);
 __dead voidboot(int);
+void set_powerdown_delay(unsigned int);
 __END_DECLS
 
 #endif /* _KERNEL */