Adjusted commit message:

The KDSKBSTATE ioctl brings the LED up. However, some keyboards (like qemu
keyboard) may not have LED. Therefore, removing the error check as ukbd(4)
does allow the keyboard works correctly with kbdcontrol(4).

MFC planned?

My second thought is that it seems hkbd_set_leds() has a check if there is no leds and return 0, why it doesn't work with qemu?

Also, I think we can even remove the HID_DEBUG check around hkbd_no_leds. (This seems not a very descriptive name, though...)

BTW for the commit message should can mention https://bugs.freebsd.org/288968

The previous comment is incorrect. I check the HID document. HID has a usage page to locate the LED stuff. And FreeBSD kernel does check if such usage page exists. The problem maybe comes from qemu.

In D52101#1189875, @aokblast wrote:

The previous comment is incorrect. I check the HID document. HID has a usage page to locate the LED stuff. And FreeBSD kernel does check if such usage page exists. The problem maybe comes from qemu.

Ok, yes, I was wondering why the /* figure out leds on keyboard */ part in hkbd_parse_hid() doesn't work. If it works then hkbd_set_leds() should work as our expectation.

I mentioned perhaps making hkbd_no_leds a normal sysctl is that it can be used as a workaround for people in ticket 288968.

In D52101#1189879, @lwhsu wrote:

In D52101#1189875, @aokblast wrote:

The previous comment is incorrect. I check the HID document. HID has a usage page to locate the LED stuff. And FreeBSD kernel does check if such usage page exists. The problem maybe comes from qemu.

Ok, yes, I was wondering why the /* figure out leds on keyboard */ part in`hkbd_parse_hid() doesn't work. If it works then hkbd_set_leds()` should work as our expectation.

I mentioned perhaps making hkbd_no_leds a normal sysctl is that it can be used as a workaround for people in ticket 288968.

I locate the problem inside qemu and do some modification then it works. I will submit a patch to qemu later.

In D52101#1189895, @aokblast wrote:

In D52101#1189879, @lwhsu wrote:

I mentioned perhaps making hkbd_no_leds a normal sysctl is that it can be used as a workaround for people in ticket 288968.

I locate the problem inside qemu and do some modification then it works. I will submit a patch to qemu later.

That's great. I feel then we have one more reason to have that sysctl to let people to workaround it before qemu having a new release and being deployed to the production environments.

In D52101#1189896, @lwhsu wrote:

In D52101#1189895, @aokblast wrote:

In D52101#1189879, @lwhsu wrote:

I mentioned perhaps making hkbd_no_leds a normal sysctl is that it can be used as a workaround for people in ticket 288968.

I locate the problem inside qemu and do some modification then it works. I will submit a patch to qemu later.

That's great. I feel then we have one more reason to have that sysctl to let people to workaround it before qemu having a new release and being deployed to the production environments.

Wouldn't it means I need to create another patch for that? Why not just use the fix in here?

In D52101#1189908, @aokblast wrote:

In D52101#1189896, @lwhsu wrote:

In D52101#1189895, @aokblast wrote:

In D52101#1189879, @lwhsu wrote:

I mentioned perhaps making hkbd_no_leds a normal sysctl is that it can be used as a workaround for people in ticket 288968.

I locate the problem inside qemu and do some modification then it works. I will submit a patch to qemu later.

That's great. I feel then we have one more reason to have that sysctl to let people to workaround it before qemu having a new release and being deployed to the production environments.

Wouldn't it means I need to create another patch for that? Why not just use the fix in here?

In most of case we want to avoid ignoring the return error entirely, as this implicitly hides the error in other places, which might cause other issues in the future.

Hmm, I am on the fence now, indeed it's also not good to ask people to manually set a workaround, even it's just a simple sysctl.

I guess I should accept this, as we can fix qemu but probably harder for Parallel.

After all, can we add some DPRINTF() in KDSKBSTATE or hkbd_set_leds() or KDSETLED to indicate that an error occurs?

In D52101#1189939, @lwhsu wrote:

In D52101#1189908, @aokblast wrote:

In D52101#1189896, @lwhsu wrote:

In D52101#1189895, @aokblast wrote:

In D52101#1189879, @lwhsu wrote:

I mentioned perhaps making hkbd_no_leds a normal sysctl is that it can be used as a workaround for people in ticket 288968.

I locate the problem inside qemu and do some modification then it works. I will submit a patch to qemu later.

That's great. I feel then we have one more reason to have that sysctl to let people to workaround it before qemu having a new release and being deployed to the production environments.

Wouldn't it means I need to create another patch for that? Why not just use the fix in here?

In most of case we want to avoid ignoring the return error entirely, as this implicitly hides the error in other places, which might cause other issues in the future.

Hmm, I am on the fence now, indeed it's also not good to ask people to manually set a workaround, even it's just a simple sysctl.

I guess I should accept this, as we can fix qemu but probably harder for Parallel.

After all, can we add some DPRINTF() in KDSKBSTATE or hkbd_set_leds() or KDSETLED to indicate that an error occurs?

I don't think it is necessary since we ignore the return value of this ioctl in all other place of kernel and kernel module but I still do it. It also fixes the Parallel according to the PR reply. I think we should let it go ASAP. It affect part of the qemu user (even VPS who use qemu) and all of the Parallel user.