grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Patch that fixes an 'at_keyboard' module issue (unreliable key press


From: Daniel Kiper
Subject: Re: Patch that fixes an 'at_keyboard' module issue (unreliable key presses)
Date: Wed, 2 Oct 2019 15:12:08 +0200
User-agent: NeoMutt/20170113 (1.7.2)

Folks,

I am happy to take this patch but I need it in proper form. Michael,
"git format-patch" and "git send-email" are your friends.

Daniel

On Wed, Sep 11, 2019 at 12:29:42AM +0200, Michael Bideau wrote:
> Hi Paul and everyone,
>
> My turn to be sorry for the late reply.
> I was on my first sailing cruise last week, no computer on board. Scary
> (45 knots), beautiful and a real team effort. Like grub dev' ahah.
>
> My answer inline below...
>
> Best regards,
>
> Michael
>
> Le mardi 03 septembre 2019 à 13:48 +0200, Paul Menzel a écrit :
> > Dear Michael,
> >
> >
> > I am sorry for the late reply.
> >
> >
> > On 2019-08-29 00:24, Michael Bideau wrote:
> >
> > > Le mardi 27 août 2019 à 11:57 +0200, Paul Menzel a écrit :
> > > > On 8/24/19 9:09 PM, Michael Bideau wrote:
> > > >
> > > > > This patch fixes an issue that prevented the 'at_keyboard'
> > > > > module
> > > > > to work (for me).
> > > > >
> > > > > The cause is a bad/wrong return value in the function
> > > > > 'grub_at_keyboard_getkey()' in file
> > > > > 'grub-core/term/at_keyboard.c' at line 234.
> > > > >
> > > > > ///////// patch /////////
> > > > > diff --git a/grub-core/term/at_keyboard.c b/grub-
> > > > > core/term/at_keyboard.c
> > > > > index f0a986eb1..597111077 100644
> > > > > --- a/grub-core/term/at_keyboard.c
> > > > > +++ b/grub-core/term/at_keyboard.c
> > > > > @@ -234,7 +234,7 @@ grub_at_keyboard_getkey (struct
> > > > > grub_term_input
> > > > > *term __attribute__ ((unused)))
> > > > >      return GRUB_TERM_NO_KEY;
> > > > >
> > > > >    if (! KEYBOARD_ISREADY (grub_inb (KEYBOARD_REG_STATUS)))
> > > > > -    return -1;
> > > > > +    return GRUB_TERM_NO_KEY;
> > > > >    at_key = grub_inb (KEYBOARD_REG_DATA);
> > > > >    old_led = ps2_state.led_status;
> > > > > ///////// end of patch /////////
> > > > >
> > > > >
> > > > > My symptoms were to have an unresponsive keyboard: keys needed
> > > > > to be pressed 10x and more to effectively be printed, sometimes
> > > > > generating multiple key presses (after 1 or 2 sec of no
> > > > > printing). Very problematic for typing passphrase in early
> > > > > stage
> > > > > (with GRUB_ENABLE_CRYPTODISK). When switching to 'console'
> > > > > terminal input, keyboard works perfectly. It also worked great
> > > > > with grub 2.02 packaged by Debian (2.02+dfsg1- 20). It was not
> > > > > an
> > > > > output issue, but an input one.
> > > >
> > > > […]
> > > > I think I had a similar issue and tried to fix it in commit
> > > > d3a3543a
> > > > (normal/menu: Do not treat error values as key presses) [1],
> > > > present
> > > > in GRUB 2.04. Do you have that commit in your tree?
> > >
> > > Yes I have this/your commit in my tree.
> > >
> > > ///////// your patch d3a3543a /////////
> > > diff --git a/grub-core/normal/menu.c b/grub-core/normal/menu.c
> > > index e7a83c2d6..d5e0c79a7 100644
> > > --- a/grub-core/normal/menu.c
> > > +++ b/grub-core/normal/menu.c
> > > @@ -698,7 +698,8 @@ run_menu (grub_menu_t menu, int nested, int
> > > *auto_boot)
> > >
> > >        c = grub_getkey_noblock ();
> > >
> > > -      if (c != GRUB_TERM_NO_KEY)
> > > +      /* Negative values are returned on error. */
> > > +      if ((c != GRUB_TERM_NO_KEY) && (c > 0))
> > >         {
> > >           if (timeout >= 0)
> > >             {
> > > ///////// end of your patch d3a3543a /////////
> > >
> > > For what I understand, your patch seems to "only" address
> > > normal/menu
> > > timeout issue (but I might be wrong, I'm still very new to grub2
> > > code),
> > > but mine fixes the keyboard (if input is 'at_keyboard') wherever it
> > > is
> > > used in the normal/menu or in the grub terminal/shell or even with
> > > the
> > > cryptomount utility (typing passphrase was my issue at the
> > > beginning).
> > >
> > > To be clear, with the last checkout (commit 4e75b2ae3), that
> > > includes
> > > your commit d3a3543a, I have my keyboard not working reliably (as
> > > described previously), and with my patch it works perfectly
> > > (meaning
> > > in the grub terminal/shell I can type commands seamlessly and even
> > > use different keyboard layouts).
> >
> > You are correct. Thank you for the analysis. Could you please create
> > a
> > “proper” commit with a commit message (most can be taken from your
> > mail), and send that to this list?
>
> The "proper" commit :
>
> ///////// show 261dd4318 /////////
> at_keyboard: fix unreliable key presses
>
> This patch fixes an issue that prevented the 'at_keyboard' module to work 
> (for me).
>
> The cause is a bad/wrong return value in the function 
> 'grub_at_keyboard_getkey()' in file 'grub-core/term/at_keyboard.c' at line 
> 237.
>
> My symptoms were to have an unresponsive keyboard: keys needed to be pressed 
> 10x and more to
> effectively be printed, sometimes generating multiple key presses (after 1 or 
> 2 sec of no printing).
> Very problematic for typing passphrase in early stage (with 
> GRUB_ENABLE_CRYPTODISK).
> When switching to 'console' terminal input, keyboard works perfectly.
> It also worked great with grub 2.02 packaged by Debian (2.02+dfsg1-20).
> It was not an output issue, but an input one.
>
> I've managed to analyse the issue and found where it came from: the following 
> commit:
> ---------- commit ----------
> Commit: 216950a4eea1a1ead1c28eaca94e34ea2ef2ad19
> Author: Vladimir Serbinenko <address@hidden>
> Date: Mon May 8 21:41:22 2017 +0200
>
> at_keyboard: Split protocol from controller code.
>
> On vexpress controller is different but protocol is the same, so reuse the 
> code.
> ---------- end of commit ----------
>
> 3 lines where moved from the function 'fetch_key()' in file 
> 'grub-core/term/at_keyboard.c', to the begining of function 
> 'grub_at_keyboard_getkey()' (same file).
> But returning '-1' made sense when in function 'fetch_key()' but not anymore 
> in function 'grub_at_keyboard_getkey()', which should return 
> 'GRUB_TERM_NO_KEY'.
>
> I think it was just an incomplete cut-paste, missing a small manual 
> correction.
> Better fix it.
> ///////// end of commit /////////
>
> Please, feel free to edit/cut the commit message as you wish.
>
> > By the way, do you only have this problem on one machine or all? Can
> > it
> > be reproduced in QEMU?
>
> Actually I only have that issue with my libvirt-qemu-kvm virtual
> machines (where I do grub tweaking and theming).
> I've never used qemu directly, only through libvirt and with a GUI
> (virt-manager).
>
> But I can provide you the log of libvirt containing the qemu command,
> and the dump of my VM configuration (and some host informations). See
> below.
>
> ///////// log of libvirt: /var/log/libvirt/qemu/debian9.log /////////
> 019-09-10 21:27:15.157+0000: starting up libvirt version: 4.10.0, package: 2 
> (Guido Günther <address@hidden> Tue, 18 Dec 2018 12:55:10 +0100), qemu 
> version: 2.12.0Debian 1:2.12+dfsg-3+b1, kernel: 4.9.0-8-amd64, hostname: 
> debian-host
> LC_ALL=C PATH=/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin 
> QEMU_AUDIO_DRV=spice /usr/bin/kvm -name guest=debian9,debug-threads=on -S 
> -object 
> secret,id=masterKey0,format=raw,file=/var/lib/libvirt/qemu/domain-1-debian9/master-key.aes
>  -machine pc-i440fx-2.8,accel=kvm,usb=off,vmport=off,dump-guest-core=off -cpu 
> Nehalem -m 1024 -realtime mlock=off -smp 1,sockets=1,cores=1,threads=1 -uuid 
> 360d8792-c34f-43d6-bc80-149de68ff11b -no-user-config -nodefaults -chardev 
> socket,id=charmonitor,fd=26,server,nowait -mon 
> chardev=charmonitor,id=monitor,mode=control -rtc base=utc,driftfix=slew 
> -global kvm-pit.lost_tick_policy=delay -no-hpet -no-shutdown -global 
> PIIX4_PM.disable_s3=1 -global PIIX4_PM.disable_s4=1 -boot menu=on,strict=on 
> -device ich9-usb-ehci1,id=usb,bus=pci.0,addr=0x4.0x7 -device 
> ich9-usb-uhci1,masterbus=usb.0,firstport=0,bus=pci.0,multifunction=on,addr=0x4
>  -device ich9-usb-uhci2,masterbus=usb.0,firstport=2,bus=pci.0,addr=0x4.0x1 
> -device ich9-usb-uhci3,masterbus=usb.0,firstport=4,bus=pci.0,addr=0x4.0x2 
> -device lsi,id=scsi0,bus=pci.0,addr=0x6 -device 
> ahci,id=sata0,bus=pci.0,addr=0x7 -device 
> virtio-serial-pci,id=virtio-serial0,bus=pci.0,addr=0x5 -drive 
> file=/var/lib/libvirt/images/debian-9.5.0-amd64-netinst.iso,format=raw,if=none,id=drive-ide0-0-0,readonly=on
>  -device ide-cd,bus=ide.0,unit=0,drive=drive-ide0-0-0,id=ide0-0-0,bootindex=2 
> -drive 
> file=/home/michael/VMs/test-btrfs1.qcow2,format=qcow2,if=none,id=drive-sata0-0-0
>  -device ide-hd,bus=sata0.0,drive=drive-sata0-0-0,id=sata0-0-0,bootindex=1 
> -drive 
> file=/home/michael/VMs/test-btrfs2.qcow2,format=qcow2,if=none,id=drive-sata0-0-1
>  -device ide-hd,bus=sata0.1,drive=drive-sata0-0-1,id=sata0-0-1 -netdev 
> tap,fd=28,id=hostnet0,vhost=on,vhostfd=29 -device 
> virtio-net-pci,netdev=hostnet0,id=net0,mac=52:54:00:68:f1:50,bus=pci.0,addr=0x3
>  -chardev pty,id=charserial0 -device 
> isa-serial,chardev=charserial0,id=serial0 -chardev 
> socket,id=charchannel0,fd=30,server,nowait -device 
> virtserialport,bus=virtio-serial0.0,nr=1,chardev=charchannel0,id=channel0,name=org.qemu.guest_agent.0
>  -chardev spicevmc,id=charchannel1,name=vdagent -device 
> virtserialport,bus=virtio-serial0.0,nr=2,chardev=charchannel1,id=channel1,name=com.redhat.spice.0
>  -device usb-tablet,id=input0,bus=usb.0,port=1 -spice 
> port=5900,addr=127.0.0.1,disable-ticketing,image-compression=off,seamless-migration=on
>  -device 
> qxl-vga,id=video0,ram_size=67108864,vram_size=67108864,vram64_size_mb=0,vgamem_mb=16,max_outputs=1,bus=pci.0,addr=0x2
>  -chardev spicevmc,id=charredir0,name=usbredir -device 
> usb-redir,chardev=charredir0,id=redir0,bus=usb.0,port=2 -chardev 
> spicevmc,id=charredir1,name=usbredir -device 
> usb-redir,chardev=charredir1,id=redir1,bus=usb.0,port=3 -device 
> virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x8 -sandbox 
> on,obsolete=deny,elevateprivileges=deny,spawn=deny,resourcecontrol=deny -msg 
> timestamp=on
>
> 2019-09-10T21:27:15.760474Z qemu-system-x86_64: -chardev pty,id=charserial0: 
> char device redirected to /dev/pts/0 (label charserial0)
> ///////// end of log /////////
>
>
> ///////// XML output of command: sudo virsh dumpxml debian9 /////////
> <domain type='kvm' id='1'>
>   <name>debian9</name>
>   <uuid>360d8792-c34f-43d6-bc80-149de68ff11b</uuid>
>   <memory unit='KiB'>1048576</memory>
>   <currentMemory unit='KiB'>1048576</currentMemory>
>   <vcpu placement='static'>1</vcpu>
>   <resource>
>     <partition>/machine</partition>
>   </resource>
>   <os>
>     <type arch='x86_64' machine='pc-i440fx-2.8'>hvm</type>
>     <bootmenu enable='yes'/>
>   </os>
>   <features>
>     <acpi/>
>     <apic/>
>     <vmport state='off'/>
>   </features>
>   <cpu mode='custom' match='exact' check='full'>
>     <model fallback='forbid'>Nehalem</model>
>     <feature policy='require' name='vme'/>
>     <feature policy='require' name='x2apic'/>
>     <feature policy='require' name='hypervisor'/>
>   </cpu>
>   <clock offset='utc'>
>     <timer name='rtc' tickpolicy='catchup'/>
>     <timer name='pit' tickpolicy='delay'/>
>     <timer name='hpet' present='no'/>
>   </clock>
>   <on_poweroff>destroy</on_poweroff>
>   <on_reboot>restart</on_reboot>
>   <on_crash>restart</on_crash>
>   <pm>
>     <suspend-to-mem enabled='no'/>
>     <suspend-to-disk enabled='no'/>
>   </pm>
>   <devices>
>     <emulator>/usr/bin/kvm</emulator>
>     <disk type='file' device='cdrom'>
>       <driver name='qemu' type='raw'/>
>       <source file='/var/lib/libvirt/images/debian-9.5.0-amd64-
> netinst.iso'/>
>       <backingStore/>
>       <target dev='hda' bus='ide'/>
>       <readonly/>
>       <boot order='2'/>
>       <alias name='ide0-0-0'/>
>       <address type='drive' controller='0' bus='0' target='0'
> unit='0'/>
>     </disk>
>     <disk type='file' device='disk'>
>       <driver name='qemu' type='qcow2'/>
>       <source file='/home/michael/VMs/test-btrfs1.qcow2'/>
>       <backingStore/>
>       <target dev='sda' bus='sata'/>
>       <boot order='1'/>
>       <alias name='sata0-0-0'/>
>       <address type='drive' controller='0' bus='0' target='0'
> unit='0'/>
>     </disk>
>     <disk type='file' device='disk'>
>       <driver name='qemu' type='qcow2'/>
>       <source file='/home/michael/VMs/test-btrfs2.qcow2'/>
>       <backingStore/>
>       <target dev='sdb' bus='sata'/>
>       <alias name='sata0-0-1'/>
>       <address type='drive' controller='0' bus='0' target='0'
> unit='1'/>
>     </disk>
>     <controller type='usb' index='0' model='ich9-ehci1'>
>       <alias name='usb'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
> function='0x7'/>
>     </controller>
>     <controller type='usb' index='0' model='ich9-uhci1'>
>       <alias name='usb'/>
>       <master startport='0'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
> function='0x0' multifunction='on'/>
>     </controller>
>     <controller type='usb' index='0' model='ich9-uhci2'>
>       <alias name='usb'/>
>       <master startport='2'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
> function='0x1'/>
>     </controller>
>     <controller type='usb' index='0' model='ich9-uhci3'>
>       <alias name='usb'/>
>       <master startport='4'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x04'
> function='0x2'/>
>     </controller>
>     <controller type='pci' index='0' model='pci-root'>
>       <alias name='pci.0'/>
>     </controller>
>     <controller type='ide' index='0'>
>       <alias name='ide'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x01'
> function='0x1'/>
>     </controller>
>     <controller type='virtio-serial' index='0'>
>       <alias name='virtio-serial0'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x05'
> function='0x0'/>
>     </controller>
>     <controller type='sata' index='0'>
>       <alias name='sata0'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x07'
> function='0x0'/>
>     </controller>
>     <controller type='scsi' index='0' model='lsilogic'>
>       <alias name='scsi0'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x06'
> function='0x0'/>
>     </controller>
>     <interface type='network'>
>       <mac address='52:54:00:68:f1:50'/>
>       <source network='default' bridge='virbr0'/>
>       <target dev='vnet0'/>
>       <model type='virtio'/>
>       <alias name='net0'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x03'
> function='0x0'/>
>     </interface>
>     <serial type='pty'>
>       <source path='/dev/pts/0'/>
>       <target type='isa-serial' port='0'>
>         <model name='isa-serial'/>
>       </target>
>       <alias name='serial0'/>
>     </serial>
>     <console type='pty' tty='/dev/pts/0'>
>       <source path='/dev/pts/0'/>
>       <target type='serial' port='0'/>
>       <alias name='serial0'/>
>     </console>
>     <channel type='unix'>
>       <source mode='bind'
> path='/var/lib/libvirt/qemu/channel/target/domain-1-
> debian9/org.qemu.guest_agent.0'/>
>       <target type='virtio' name='org.qemu.guest_agent.0'
> state='disconnected'/>
>       <alias name='channel0'/>
>       <address type='virtio-serial' controller='0' bus='0' port='1'/>
>     </channel>
>     <channel type='spicevmc'>
>       <target type='virtio' name='com.redhat.spice.0'
> state='disconnected'/>
>       <alias name='channel1'/>
>       <address type='virtio-serial' controller='0' bus='0' port='2'/>
>     </channel>
>     <input type='tablet' bus='usb'>
>       <alias name='input0'/>
>       <address type='usb' bus='0' port='1'/>
>     </input>
>     <input type='mouse' bus='ps2'>
>       <alias name='input1'/>
>     </input>
>     <input type='keyboard' bus='ps2'>
>       <alias name='input2'/>
>     </input>
>     <graphics type='spice' port='5900' autoport='yes'
> listen='127.0.0.1'>
>       <listen type='address' address='127.0.0.1'/>
>       <image compression='off'/>
>     </graphics>
>     <video>
>       <model type='qxl' ram='65536' vram='65536' vgamem='16384'
> heads='1' primary='yes'/>
>       <alias name='video0'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x02'
> function='0x0'/>
>     </video>
>     <redirdev bus='usb' type='spicevmc'>
>       <alias name='redir0'/>
>       <address type='usb' bus='0' port='2'/>
>     </redirdev>
>     <redirdev bus='usb' type='spicevmc'>
>       <alias name='redir1'/>
>       <address type='usb' bus='0' port='3'/>
>     </redirdev>
>     <memballoon model='virtio'>
>       <stats period='5'/>
>       <alias name='balloon0'/>
>       <address type='pci' domain='0x0000' bus='0x00' slot='0x08'
> function='0x0'/>
>     </memballoon>
>   </devices>
>   <seclabel type='dynamic' model='dac' relabel='yes'>
>     <label>+64055:+64055</label>
>     <imagelabel>+64055:+64055</imagelabel>
>   </seclabel>
> </domain>
> ///////// XML end of output /////////
>
> Hope this helps.
> If you need more informations, I'll be glad to dig.
>
> > > PS: there is a typo in my introduction of my patch: it is at the
> > > line
> > > 237 and not 234 as mentioned. Do you think I should answer to my
> > > first
> > > email to say it (so people not reading this sub-thread will know)?
> >
> > You can fix that in the commit message.
> >
> > Please ask if you need further help.
>
> Thank you very much for your help and kind support.
>
> > Kind regards,
> >
> > Paul
> >
> >
> > > > [1]:
> > > > https://git.savannah.gnu.org/cgit/grub.git/commit/?id=d3a3543a5666c1dd180ae6027948ca753dcffc18



reply via email to

[Prev in Thread] Current Thread [Next in Thread]