-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HID based IPTS and ITHC support, 6.1 edition #133
Conversation
This reverts commit 903e7de. Signed-off-by: Dorian Stoll <[email protected]>
drivers/hid/ipts/thread.h
Outdated
bool should_stop; | ||
|
||
struct mutex lock; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could replace those with set_bit() and test_bit() (https://www.kernel.org/doc/html/latest/core-api/kernel-api.html?highlight=set_bit#bit-operations) as full mutexes are a bit overkill for that task. Alternatives are atomics (which is probably better for explicit acquire/release semantics without needing rmb/wmb) or I guess you could even use READ_ONCE()
and WRITE_ONCE()
with memory barriers if you want to keep the bool.
So with bitops you'd do something like:
int ipts_thread_stop(struct ipts_thread *thread)
{
int ret = 0;
if (!thread)
return -EFAULT;
if (!thread->thread)
return 0;
set_bit(IPTS_THREAD_STOP_BIT, &thread->flags);
wmb(); // ensure bit is set before we start waiting
wait_for_completion(&thread->done);
ret = kthread_stop(thread->thread);
thread->thread = NULL;
thread->data = NULL;
thread->threadfn = NULL;
return ret;
}
and
bool ipts_thread_should_stop(struct ipts_thread *thread)
{
bool ret = false;
if (!thread)
return false;
// optional: add rmb(); here to have "acquire" semantics, but that's not really necessary
return test_bit(IPTS_THREAD_STOP_BIT, thread->flags);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would smp_load_acquire
/ smp_store_release
work too? That could be the easiest solution, because it already seems to combine READ_ONCE
and WRITE_ONCE
with a barrier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I forgot about those. Yeah I think those should work too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although... After checking, those have the barriers exactly the opposite way (which is what you more often want), but I think we only need the wmb()
after setting the flag. If at all. I'm not entirely sure there how the semantics are for the wait_for_completion()
call, but the point is that we want to make sure that the bit is set before waiting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me (I haven't extensively checked it though). I've added one comment for a potential optimization, but that's something that we can work on later.
5ae67d2
to
dcfd07a
Compare
Based on linux-surface/intel-precise-touch@8abe268 Signed-off-by: Dorian Stoll <[email protected]> Patchset: ipts
Signed-off-by: Dorian Stoll <[email protected]> Patchset: ithc
Based on quo/ithc-linux@55803a2 Signed-off-by: Dorian Stoll <[email protected]> Patchset: ithc
[ Upstream commit 7f74563 ] LE Create CIS command shall not be sent before all CIS Established events from its previous invocation have been processed. Currently it is sent via hci_sync but that only waits for the first event, but there can be multiple. Make it wait for all events, and simplify the CIS creation as follows: Add new flag HCI_CONN_CREATE_CIS, which is set if Create CIS has been sent for the connection but it is not yet completed. Make BT_CONNECT state to mean the connection wants Create CIS. On events after which new Create CIS may need to be sent, send it if possible and some connections need it. These events are: hci_connect_cis, iso_connect_cfm, hci_cs_le_create_cis, hci_le_cis_estabilished_evt. The Create CIS status/completion events shall queue new Create CIS only if at least one of the connections transitions away from BT_CONNECT, so that we don't loop if controller is sending bogus events. This fixes sending multiple CIS Create for the same CIS in the "ISO AC 6(i) - Success" BlueZ test case: < HCI Command: LE Create Co.. (0x08|0x0064) plen 9 #129 [hci0] Number of CIS: 2 CIS Handle: 257 ACL Handle: 42 CIS Handle: 258 ACL Handle: 42 > HCI Event: Command Status (0x0f) plen 4 #130 [hci0] LE Create Connected Isochronous Stream (0x08|0x0064) ncmd 1 Status: Success (0x00) > HCI Event: LE Meta Event (0x3e) plen 29 #131 [hci0] LE Connected Isochronous Stream Established (0x19) Status: Success (0x00) Connection Handle: 257 ... < HCI Command: LE Setup Is.. (0x08|0x006e) plen 13 #132 [hci0] ... > HCI Event: Command Complete (0x0e) plen 6 #133 [hci0] LE Setup Isochronous Data Path (0x08|0x006e) ncmd 1 ... < HCI Command: LE Create Co.. (0x08|0x0064) plen 5 #134 [hci0] Number of CIS: 1 CIS Handle: 258 ACL Handle: 42 > HCI Event: Command Status (0x0f) plen 4 #135 [hci0] LE Create Connected Isochronous Stream (0x08|0x0064) ncmd 1 Status: ACL Connection Already Exists (0x0b) > HCI Event: LE Meta Event (0x3e) plen 29 #136 [hci0] LE Connected Isochronous Stream Established (0x19) Status: Success (0x00) Connection Handle: 258 ... Fixes: c09b80b ("Bluetooth: hci_conn: Fix not waiting for HCI_EVT_LE_CIS_ESTABLISHED") Signed-off-by: Pauli Virtanen <[email protected]> Signed-off-by: Luiz Augusto von Dentz <[email protected]> Signed-off-by: Sasha Levin <[email protected]>
Creating sysfs files for all Cells caused a boot failure for linux-6.8-rc1 on Apple M1, which (in downstream dts files) has multiple nvmem cells that use the same byte address. This causes the device probe to fail with [ 0.605336] sysfs: cannot create duplicate filename '/devices/platform/soc@200000000/2922bc000.efuse/apple_efuses_nvmem0/cells/efuse@a10' [ 0.605347] CPU: 7 PID: 1 Comm: swapper/0 Tainted: G S 6.8.0-rc1-arnd-5+ linux-surface#133 [ 0.605355] Hardware name: Apple Mac Studio (M1 Ultra, 2022) (DT) [ 0.605362] Call trace: [ 0.605365] show_stack+0x18/0x2c [ 0.605374] dump_stack_lvl+0x60/0x80 [ 0.605383] dump_stack+0x18/0x24 [ 0.605388] sysfs_warn_dup+0x64/0x80 [ 0.605395] sysfs_add_bin_file_mode_ns+0xb0/0xd4 [ 0.605402] internal_create_group+0x268/0x404 [ 0.605409] sysfs_create_groups+0x38/0x94 [ 0.605415] devm_device_add_groups+0x50/0x94 [ 0.605572] nvmem_populate_sysfs_cells+0x180/0x1b0 [ 0.605682] nvmem_register+0x38c/0x470 [ 0.605789] devm_nvmem_register+0x1c/0x6c [ 0.605895] apple_efuses_probe+0xe4/0x120 [ 0.606000] platform_probe+0xa8/0xd0 As far as I can tell, this is a problem for any device with multiple cells on different bits of the same address. Avoid the issue by changing the file name to include the first bit number. Fixes: 0331c61 ("nvmem: core: Expose cells through sysfs") Link: https://github.com/AsahiLinux/linux/blob/bd0a1a7d4/arch/arm64/boot/dts/apple/t600x-dieX.dtsi#L156 Cc: <[email protected]> Cc: Miquel Raynal <[email protected]> Cc: Rafał Miłecki <[email protected]> Cc: Chen-Yu Tsai <[email protected]> Cc: Srinivas Kandagatla <[email protected]> Cc: Greg Kroah-Hartman <[email protected]> Cc: <[email protected]> Cc: Sven Peter <[email protected]> Signed-off-by: Arnd Bergmann <[email protected]> Signed-off-by: Srinivas Kandagatla <[email protected]> Reviewed-by: Eric Curtin <[email protected]> Reviewed-by: Miquel Raynal <[email protected]> Link: https://lore.kernel.org/r/[email protected] Signed-off-by: Greg Kroah-Hartman <[email protected]>
iptsd feels pretty much ready and will work better than what is currently in the repo, so lets start doing this ...
Things that need to be done once this is merged:
CONFIG_MISC_IPTS=m
toCONFIG_HID_IPTS=m
in our kernel configsCONFIG_HID_ITHC=m
to the kernel configsChanges since #132: