| Message ID | 158379379177.1643521.631344932643277192.stgit@bahia.lan |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Fix virtio-serial | expand |
On Mon, Mar 09, 2020 at 11:43:12PM +0100, Greg Kurz wrote: > --- a/board-qemu/slof/virtio-serial.fs > +++ b/board-qemu/slof/virtio-serial.fs > @@ -19,13 +19,8 @@ virtio-setup-vd VALUE virtiodev > \ Quiescence the virtqueue of this device so that no more background > \ transactions can be pending. > : shutdown ( -- ) > - initialized? IF > - my-phandle node>path open-dev ?dup IF > - virtiodev virtio-serial-shutdown > - close-dev > - THEN > - FALSE to initialized? > - THEN > + virtiodev virtio-serial-shutdown > + FALSE to initialized? > ; It now also calls virtio-serial-shutdown if it never was initialised? (Rest looks perfectly reasonable). Segher
On 10/03/2020 11:35, Segher Boessenkool wrote: > On Mon, Mar 09, 2020 at 11:43:12PM +0100, Greg Kurz wrote: >> --- a/board-qemu/slof/virtio-serial.fs >> +++ b/board-qemu/slof/virtio-serial.fs >> @@ -19,13 +19,8 @@ virtio-setup-vd VALUE virtiodev >> \ Quiescence the virtqueue of this device so that no more background >> \ transactions can be pending. >> : shutdown ( -- ) >> - initialized? IF >> - my-phandle node>path open-dev ?dup IF >> - virtiodev virtio-serial-shutdown >> - close-dev >> - THEN >> - FALSE to initialized? >> - THEN >> + virtiodev virtio-serial-shutdown >> + FALSE to initialized? >> ; > > It now also calls virtio-serial-shutdown if it never was initialised? No. This also removes "['] shutdown add-quiesce-xt" so the only way to end up in "shutdown" is having open-count incremented == open virtio-serial. > > (Rest looks perfectly reasonable). > > > Segher > _______________________________________________ > SLOF mailing list > SLOF@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/slof >
Hi! On Tue, Mar 10, 2020 at 02:38:22PM +1100, Alexey Kardashevskiy wrote: > On 10/03/2020 11:35, Segher Boessenkool wrote: > > On Mon, Mar 09, 2020 at 11:43:12PM +0100, Greg Kurz wrote: > >> --- a/board-qemu/slof/virtio-serial.fs > >> +++ b/board-qemu/slof/virtio-serial.fs > >> @@ -19,13 +19,8 @@ virtio-setup-vd VALUE virtiodev > >> \ Quiescence the virtqueue of this device so that no more background > >> \ transactions can be pending. > >> : shutdown ( -- ) > >> - initialized? IF > >> - my-phandle node>path open-dev ?dup IF > >> - virtiodev virtio-serial-shutdown > >> - close-dev > >> - THEN > >> - FALSE to initialized? > >> - THEN > >> + virtiodev virtio-serial-shutdown > >> + FALSE to initialized? > >> ; > > > > It now also calls virtio-serial-shutdown if it never was initialised? > > > No. This also removes "['] shutdown add-quiesce-xt" so the only way to > end up in "shutdown" is having open-count incremented == open virtio-serial. Anything can call shutdown, the nice simple name encourages that even. It's not a good idea to make this word more fragile, imho. Segher
On Thu, 12 Mar 2020 11:49:35 -0500 Segher Boessenkool <segher@kernel.crashing.org> wrote: > Hi! > > On Tue, Mar 10, 2020 at 02:38:22PM +1100, Alexey Kardashevskiy wrote: > > On 10/03/2020 11:35, Segher Boessenkool wrote: > > > On Mon, Mar 09, 2020 at 11:43:12PM +0100, Greg Kurz wrote: > > >> --- a/board-qemu/slof/virtio-serial.fs > > >> +++ b/board-qemu/slof/virtio-serial.fs > > >> @@ -19,13 +19,8 @@ virtio-setup-vd VALUE virtiodev > > >> \ Quiescence the virtqueue of this device so that no more background > > >> \ transactions can be pending. > > >> : shutdown ( -- ) > > >> - initialized? IF > > >> - my-phandle node>path open-dev ?dup IF > > >> - virtiodev virtio-serial-shutdown > > >> - close-dev > > >> - THEN > > >> - FALSE to initialized? > > >> - THEN > > >> + virtiodev virtio-serial-shutdown > > >> + FALSE to initialized? > > >> ; > > > > > > It now also calls virtio-serial-shutdown if it never was initialised? > > > > > > No. This also removes "['] shutdown add-quiesce-xt" so the only way to > > end up in "shutdown" is having open-count incremented == open virtio-serial. > > Anything can call shutdown, the nice simple name encourages that even. > It's not a good idea to make this word more fragile, imho. > Makes sense. I'll fix that in a follow-up patch though because Alexey already merged this. > > Segher
On 13/03/2020 03:49, Segher Boessenkool wrote: > Hi! > > On Tue, Mar 10, 2020 at 02:38:22PM +1100, Alexey Kardashevskiy wrote: >> On 10/03/2020 11:35, Segher Boessenkool wrote: >>> On Mon, Mar 09, 2020 at 11:43:12PM +0100, Greg Kurz wrote: >>>> --- a/board-qemu/slof/virtio-serial.fs >>>> +++ b/board-qemu/slof/virtio-serial.fs >>>> @@ -19,13 +19,8 @@ virtio-setup-vd VALUE virtiodev >>>> \ Quiescence the virtqueue of this device so that no more background >>>> \ transactions can be pending. >>>> : shutdown ( -- ) >>>> - initialized? IF >>>> - my-phandle node>path open-dev ?dup IF >>>> - virtiodev virtio-serial-shutdown >>>> - close-dev >>>> - THEN >>>> - FALSE to initialized? >>>> - THEN >>>> + virtiodev virtio-serial-shutdown >>>> + FALSE to initialized? >>>> ; >>> >>> It now also calls virtio-serial-shutdown if it never was initialised? >> >> >> No. This also removes "['] shutdown add-quiesce-xt" so the only way to >> end up in "shutdown" is having open-count incremented == open virtio-serial. > > Anything can call shutdown, the nice simple name encourages that even. > It's not a good idea to make this word more fragile, imho. In practice, one would need to run a guest with virtio-serial but not make it default stdout to avoid having it initialized, then select virtio-serial device in the SLOF prompt and call its "shutdown" method. This nice simple name is not that easily accessible ;) I mean I'll merge Greg's fix when he posts one but there is no rush or did I miss anything? Thanks,
Hi! On Fri, Mar 13, 2020 at 11:31:46AM +1100, Alexey Kardashevskiy wrote: > >>> It now also calls virtio-serial-shutdown if it never was initialised? > >> > >> No. This also removes "['] shutdown add-quiesce-xt" so the only way to > >> end up in "shutdown" is having open-count incremented == open virtio-serial. > > > > Anything can call shutdown, the nice simple name encourages that even. > > It's not a good idea to make this word more fragile, imho. > > In practice, one would need to run a guest with virtio-serial but not > make it default stdout to avoid having it initialized, then select > virtio-serial device in the SLOF prompt and call its "shutdown" method. > This nice simple name is not that easily accessible ;) Well, that depends on the user ;-) My point is that this was bracketed with initialized? IF ... THEN before, which was more robust. That is all. Thanks, Segher
On Thu, Mar 12, 2020 at 08:20:34PM -0500, Segher Boessenkool wrote: > Hi! > > On Fri, Mar 13, 2020 at 11:31:46AM +1100, Alexey Kardashevskiy wrote: > > >>> It now also calls virtio-serial-shutdown if it never was initialised? > > >> > > >> No. This also removes "['] shutdown add-quiesce-xt" so the only way to > > >> end up in "shutdown" is having open-count incremented == open virtio-serial. > > > > > > Anything can call shutdown, the nice simple name encourages that even. > > > It's not a good idea to make this word more fragile, imho. > > > > In practice, one would need to run a guest with virtio-serial but not > > make it default stdout to avoid having it initialized, then select > > virtio-serial device in the SLOF prompt and call its "shutdown" method. > > This nice simple name is not that easily accessible ;) > > Well, that depends on the user ;-) (I mean, it can be called as just apply shutdown disk0 or similar -- with "disk0" a device alias -- this is slightly harder to do on an _open_ device, i.e. on an instance, but that isn't really hard either.) Segher
diff --git a/board-qemu/slof/virtio-serial.fs b/board-qemu/slof/virtio-serial.fs index a99293f6ef77..e307231ed0dc 100644 --- a/board-qemu/slof/virtio-serial.fs +++ b/board-qemu/slof/virtio-serial.fs @@ -19,13 +19,8 @@ virtio-setup-vd VALUE virtiodev \ Quiescence the virtqueue of this device so that no more background \ transactions can be pending. : shutdown ( -- ) - initialized? IF - my-phandle node>path open-dev ?dup IF - virtiodev virtio-serial-shutdown - close-dev - THEN - FALSE to initialized? - THEN + virtiodev virtio-serial-shutdown + FALSE to initialized? ; : virtio-serial-term-emit @@ -39,7 +34,6 @@ virtio-setup-vd VALUE virtiodev : init ( -- ) virtiodev virtio-serial-init drop TRUE to initialized? - ['] shutdown add-quiesce-xt ; 0 VALUE open-count @@ -58,7 +52,7 @@ virtiodev virtio-serial-init drop : close open-count 0> IF open-count 1 - dup to open-count - 0= IF close THEN + 0= IF shutdown THEN THEN close ; diff --git a/slof/fs/client.fs b/slof/fs/client.fs index db7a1925792c..76231f9aef75 100644 --- a/slof/fs/client.fs +++ b/slof/fs/client.fs @@ -203,6 +203,11 @@ ALSO client-voc DEFINITIONS \ End of life of SLOF now, call platform quiesce as quiesce \ is an undocumented extension and not everybody supports it close-dev + \ Some device, eg. virtio-serial, need all instances to be + \ closed in order to be reset properly + s" stdout" get-chosen IF + decode-int nip nip close-dev + THEN quiesce ELSE close-dev
The "io" word of term-io.fs opens two separate instances of the device for stdin and stdout. The prom_init() function in Linux closes stdin at some point, which internally calls quiesce and shuts the device down through a quiesce hook. When the "open-count" variable in virtio-serial.fs reaches 0, ie. when closing the last instance, we call "close" two times, which is clearly wrong. This never hits however because the stdout instance is never closed which prevents "open-count" to reach 0. It would make more sense to shutdown the device when closing the last instance, for symmetry with the first open that initializes the device. Change the shutdown sequence to do that rather than relying on a quiesce hook. Have quiesce to explicitly close stdout, which is supposedly the last instance, and shutdown the device. Signed-off-by: Greg Kurz <groug@kaod.org> --- board-qemu/slof/virtio-serial.fs | 12 +++--------- slof/fs/client.fs | 5 +++++ 2 files changed, 8 insertions(+), 9 deletions(-)