[PATCH v2 1/3] input: touchscreen: imagis: use FIELD_GET where applicable
Instead of manually extracting certain bits from registers with binary
ANDs and shifts, the FIELD_GET macro can be used. With this in mind, the
*_SHIFT macros can be dropped.
Signed-off-by: Duje Mihanović
---
drivers/input/touchscreen/imagis.c | 17 ++---
1 file changed, 6 insertions(+), 11 deletions(-)
diff --git a/drivers/input/touchscreen/imagis.c
b/drivers/input/touchscreen/imagis.c
index e1fafa561ee3..4eae98771bd2 100644
--- a/drivers/input/touchscreen/imagis.c
+++ b/drivers/input/touchscreen/imagis.c
@@ -1,5 +1,6 @@
// SPDX-License-Identifier: GPL-2.0-only
+#include
#include
#include
#include
@@ -29,12 +30,9 @@
#define IST3038C_I2C_RETRY_COUNT 3
#define IST3038C_MAX_FINGER_NUM10
#define IST3038C_X_MASKGENMASK(23, 12)
-#define IST3038C_X_SHIFT 12
#define IST3038C_Y_MASKGENMASK(11, 0)
#define IST3038C_AREA_MASK GENMASK(27, 24)
-#define IST3038C_AREA_SHIFT24
#define IST3038C_FINGER_COUNT_MASK GENMASK(15, 12)
-#define IST3038C_FINGER_COUNT_SHIFT12
#define IST3038C_FINGER_STATUS_MASKGENMASK(9, 0)
struct imagis_properties {
@@ -106,8 +104,7 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
goto out;
}
- finger_count = (intr_message & IST3038C_FINGER_COUNT_MASK) >>
- IST3038C_FINGER_COUNT_SHIFT;
+ finger_count = FIELD_GET(IST3038C_FINGER_COUNT_MASK, intr_message);
if (finger_count > IST3038C_MAX_FINGER_NUM) {
dev_err(&ts->client->dev,
"finger count %d is more than maximum supported\n",
@@ -115,7 +112,7 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
goto out;
}
- finger_pressed = intr_message & IST3038C_FINGER_STATUS_MASK;
+ finger_pressed = FIELD_GET(IST3038C_FINGER_STATUS_MASK, intr_message);
for (i = 0; i < finger_count; i++) {
if (ts->tdata->protocol_b)
@@ -136,12 +133,10 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER,
finger_pressed & BIT(i));
touchscreen_report_pos(ts->input_dev, &ts->prop,
- (finger_status & IST3038C_X_MASK) >>
- IST3038C_X_SHIFT,
- finger_status & IST3038C_Y_MASK, 1);
+ FIELD_GET(IST3038C_X_MASK,
finger_status),
+ FIELD_GET(IST3038C_Y_MASK,
finger_status), 1);
input_report_abs(ts->input_dev, ABS_MT_TOUCH_MAJOR,
-(finger_status & IST3038C_AREA_MASK) >>
- IST3038C_AREA_SHIFT);
+ FIELD_GET(IST3038C_AREA_MASK,
finger_status));
}
input_mt_sync_frame(ts->input_dev);
--
2.43.0
[PATCH v2 0/3] Imagis touch keys and FIELD_GET cleanup
Tiny series to clean up the field extraction and add touch key support. Depends on the IST3032C series (at least the touch key patches, the other one could be split up): https://lore.kernel.org/20240120191940.3631-1-kar...@gimli.ms.mff.cuni.cz Signed-off-by: Duje Mihanović --- Changes in v2: - Fix compile error - Add FIELD_GET patch - Allow specifying custom keycodes - Link to v1: https://lore.kernel.org/20231112194124.24916-1-duje.mihano...@skole.hr --- Duje Mihanović (3): input: touchscreen: imagis: use FIELD_GET where applicable dt-bindings: input: imagis: Document touch keys input: touchscreen: imagis: Add touch key support .../input/touchscreen/imagis,ist3038c.yaml | 11 ++ drivers/input/touchscreen/imagis.c | 42 +++--- 2 files changed, 41 insertions(+), 12 deletions(-) --- base-commit: 553f2a83ae97246be401ca9b313063f5fa879f12 change-id: 20240120-b4-imagis-keys-a0a9f2b31740 Best regards, -- Duje Mihanović
[PATCH v2 3/3] input: touchscreen: imagis: Add touch key support
IST3032C (and possibly some other models) has touch keys. Add support
for them to the imagis driver.
Signed-off-by: Duje Mihanović
---
drivers/input/touchscreen/imagis.c | 25 -
1 file changed, 24 insertions(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/imagis.c
b/drivers/input/touchscreen/imagis.c
index 4eae98771bd2..6dcb82313c32 100644
--- a/drivers/input/touchscreen/imagis.c
+++ b/drivers/input/touchscreen/imagis.c
@@ -34,6 +34,7 @@
#define IST3038C_AREA_MASK GENMASK(27, 24)
#define IST3038C_FINGER_COUNT_MASK GENMASK(15, 12)
#define IST3038C_FINGER_STATUS_MASKGENMASK(9, 0)
+#define IST3032C_KEY_STATUS_MASK GENMASK(20, 16)
struct imagis_properties {
unsigned int interrupt_msg_cmd;
@@ -41,6 +42,7 @@ struct imagis_properties {
unsigned int whoami_cmd;
unsigned int whoami_val;
bool protocol_b;
+ bool touch_keys_supported;
};
struct imagis_ts {
@@ -49,6 +51,8 @@ struct imagis_ts {
struct input_dev *input_dev;
struct touchscreen_properties prop;
struct regulator_bulk_data supplies[2];
+ u32 keycodes[2];
+ int num_keycodes;
};
static int imagis_i2c_read_reg(struct imagis_ts *ts,
@@ -93,7 +97,7 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
{
struct imagis_ts *ts = dev_id;
u32 intr_message, finger_status;
- unsigned int finger_count, finger_pressed;
+ unsigned int finger_count, finger_pressed, key_pressed;
int i;
int error;
@@ -139,6 +143,11 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
FIELD_GET(IST3038C_AREA_MASK,
finger_status));
}
+ key_pressed = FIELD_GET(IST3032C_KEY_STATUS_MASK, intr_message);
+
+ for (int i = 0; i < ts->num_keycodes; i++)
+ input_report_key(ts->input_dev, ts->keycodes[i], (key_pressed &
BIT(i)));
+
input_mt_sync_frame(ts->input_dev);
input_sync(ts->input_dev);
@@ -224,6 +233,19 @@ static int imagis_init_input_dev(struct imagis_ts *ts)
input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 16, 0, 0);
+ if (ts->tdata->touch_keys_supported) {
+ ts->num_keycodes = of_property_read_variable_u32_array(
+ ts->client->dev.of_node, "linux,keycodes",
+ ts->keycodes, 0, ARRAY_SIZE(ts->keycodes));
+ if (ts->num_keycodes <= 0) {
+ ts->keycodes[0] = KEY_APPSELECT;
+ ts->keycodes[1] = KEY_BACK;
+ ts->num_keycodes = 2;
+ }
+ }
+
+ for (int i = 0; i < ts->num_keycodes; i++)
+ input_set_capability(input_dev, EV_KEY, ts->keycodes[i]);
touchscreen_parse_properties(input_dev, true, &ts->prop);
if (!ts->prop.max_x || !ts->prop.max_y) {
@@ -365,6 +387,7 @@ static const struct imagis_properties imagis_3032c_data = {
.touch_coord_cmd = IST3038C_REG_TOUCH_COORD,
.whoami_cmd = IST3038C_REG_CHIPID,
.whoami_val = IST3032C_WHOAMI,
+ .touch_keys_supported = true,
};
static const struct imagis_properties imagis_3038b_data = {
--
2.43.0
[PATCH v2 2/3] dt-bindings: input: imagis: Document touch keys
IST3032C (and possibly some other models) has touch keys. Document this. Signed-off-by: Duje Mihanović --- .../bindings/input/touchscreen/imagis,ist3038c.yaml | 11 +++ 1 file changed, 11 insertions(+) diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml index 2af71cbcc97d..960e5436642f 100644 --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml @@ -42,6 +42,17 @@ properties: touchscreen-inverted-y: true touchscreen-swapped-x-y: true +if: + properties: +compatible: + contains: +const: imagis,ist3032c +then: + properties: +linux,keycodes: + description: Keycodes for the touch keys + maxItems: 2 + additionalProperties: false required: -- 2.43.0
Re: [PATCH v12 for-next 0/4] riscv: ftrace: Miscellaneous ftrace improvements
Hello: This series was applied to riscv/linux.git (fixes) by Palmer Dabbelt : On Thu, 30 Nov 2023 13:15:27 +0100 you wrote: > From: Björn Töpel > > NB! Song told me that he would not have the time work on this series, > so I picked it up. > > This series includes a three ftrace improvements for RISC-V: > > [...] Here is the summary with links: - [v12,for-next,1/4] riscv: select FTRACE_MCOUNT_USE_PATCHABLE_FUNCTION_ENTRY https://git.kernel.org/riscv/c/b546d6363af4 - [v12,for-next,2/4] riscv: ftrace: Make function graph use ftrace directly https://git.kernel.org/riscv/c/35e61e8827ee - [v12,for-next,3/4] riscv: ftrace: Add DYNAMIC_FTRACE_WITH_DIRECT_CALLS support https://git.kernel.org/riscv/c/196c79f19a92 - [v12,for-next,4/4] samples: ftrace: Add RISC-V support for SAMPLE_FTRACE_DIRECT[_MULTI] https://git.kernel.org/riscv/c/629291dd8499 You are awesome, thank you! -- Deet-doot-dot, I am a bot. https://korg.docs.kernel.org/patchwork/pwbot.html
[PATCH v4 5/5] input/touchscreen: imagis: add support for IST3032C
From: Karel Balej
IST3032C is a touchscreen chip used for instance in the
samsung,coreprimevelte smartphone, with which this was tested. Add the
chip specific information to the driver.
Reviewed-by: Markuss Broks
Signed-off-by: Karel Balej
---
Notes:
v4:
* Change the WHOAMI definition position to preserve alphanumerical order
of the definitions.
* Add Markuss' Reviewed-by trailer.
drivers/input/touchscreen/imagis.c | 10 ++
1 file changed, 10 insertions(+)
diff --git a/drivers/input/touchscreen/imagis.c
b/drivers/input/touchscreen/imagis.c
index 9af8a6332ae6..e1fafa561ee3 100644
--- a/drivers/input/touchscreen/imagis.c
+++ b/drivers/input/touchscreen/imagis.c
@@ -11,6 +11,8 @@
#include
#include
+#define IST3032C_WHOAMI0x32c
+
#define IST3038B_REG_STATUS0x20
#define IST3038B_REG_CHIPID0x30
#define IST3038B_WHOAMI0x30380b
@@ -363,6 +365,13 @@ static int imagis_resume(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume);
+static const struct imagis_properties imagis_3032c_data = {
+ .interrupt_msg_cmd = IST3038C_REG_INTR_MESSAGE,
+ .touch_coord_cmd = IST3038C_REG_TOUCH_COORD,
+ .whoami_cmd = IST3038C_REG_CHIPID,
+ .whoami_val = IST3032C_WHOAMI,
+};
+
static const struct imagis_properties imagis_3038b_data = {
.interrupt_msg_cmd = IST3038B_REG_STATUS,
.touch_coord_cmd = IST3038B_REG_STATUS,
@@ -380,6 +389,7 @@ static const struct imagis_properties imagis_3038c_data = {
#ifdef CONFIG_OF
static const struct of_device_id imagis_of_match[] = {
+ { .compatible = "imagis,ist3032c", .data = &imagis_3032c_data },
{ .compatible = "imagis,ist3038b", .data = &imagis_3038b_data },
{ .compatible = "imagis,ist3038c", .data = &imagis_3038c_data },
{ },
--
2.43.0
[PATCH v4 4/5] dt-bindings: input/touchscreen: imagis: add compatible for IST3032C
From: Karel Balej IST3032C is a touchscreen IC which seems mostly compatible with IST3038C except that it reports a different chip ID value. Signed-off-by: Karel Balej --- Notes: v4: * Reword commit description to mention how this IC differs from the already supported. .../devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml index b5372c4eae56..2af71cbcc97d 100644 --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml @@ -18,6 +18,7 @@ properties: compatible: enum: + - imagis,ist3032c - imagis,ist3038b - imagis,ist3038c -- 2.43.0
[PATCH v4 3/5] input/touchscreen: imagis: Add support for Imagis IST3038B
From: Markuss Broks
Imagis IST3038B is another variant of Imagis IST3038 IC, which has
a different register interface from IST3038C (possibly firmware defined).
This should also work for IST3044B (though untested), however other
variants using this interface/protocol(IST3026, IST3032, IST3026B,
IST3032B) have a different format for coordinates, and they'd need
additional effort to be supported by this driver.
Signed-off-by: Markuss Broks
Signed-off-by: Karel Balej
---
Notes:
v4:
* Sort the definitions in alphanumerical order.
drivers/input/touchscreen/imagis.c | 58 --
1 file changed, 47 insertions(+), 11 deletions(-)
diff --git a/drivers/input/touchscreen/imagis.c
b/drivers/input/touchscreen/imagis.c
index e67fd3011027..9af8a6332ae6 100644
--- a/drivers/input/touchscreen/imagis.c
+++ b/drivers/input/touchscreen/imagis.c
@@ -11,9 +11,13 @@
#include
#include
+#define IST3038B_REG_STATUS0x20
+#define IST3038B_REG_CHIPID0x30
+#define IST3038B_WHOAMI0x30380b
+
#define IST3038C_HIB_ACCESS(0x800B << 16)
#define IST3038C_DIRECT_ACCESS BIT(31)
-#define IST3038C_REG_CHIPID0x40001000
+#define IST3038C_REG_CHIPID(0x40001000 | IST3038C_DIRECT_ACCESS)
#define IST3038C_REG_HIB_BASE 0x3100
#define IST3038C_REG_TOUCH_STATUS (IST3038C_REG_HIB_BASE |
IST3038C_HIB_ACCESS)
#define IST3038C_REG_TOUCH_COORD (IST3038C_REG_HIB_BASE |
IST3038C_HIB_ACCESS | 0x8)
@@ -31,8 +35,17 @@
#define IST3038C_FINGER_COUNT_SHIFT12
#define IST3038C_FINGER_STATUS_MASKGENMASK(9, 0)
+struct imagis_properties {
+ unsigned int interrupt_msg_cmd;
+ unsigned int touch_coord_cmd;
+ unsigned int whoami_cmd;
+ unsigned int whoami_val;
+ bool protocol_b;
+};
+
struct imagis_ts {
struct i2c_client *client;
+ const struct imagis_properties *tdata;
struct input_dev *input_dev;
struct touchscreen_properties prop;
struct regulator_bulk_data supplies[2];
@@ -84,8 +97,7 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
int i;
int error;
- error = imagis_i2c_read_reg(ts, IST3038C_REG_INTR_MESSAGE,
- &intr_message);
+ error = imagis_i2c_read_reg(ts, ts->tdata->interrupt_msg_cmd,
&intr_message);
if (error) {
dev_err(&ts->client->dev,
"failed to read the interrupt message: %d\n", error);
@@ -104,9 +116,13 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
finger_pressed = intr_message & IST3038C_FINGER_STATUS_MASK;
for (i = 0; i < finger_count; i++) {
- error = imagis_i2c_read_reg(ts,
- IST3038C_REG_TOUCH_COORD + (i * 4),
- &finger_status);
+ if (ts->tdata->protocol_b)
+ error = imagis_i2c_read_reg(ts,
+ ts->tdata->touch_coord_cmd,
&finger_status);
+ else
+ error = imagis_i2c_read_reg(ts,
+ ts->tdata->touch_coord_cmd
+ (i * 4),
+ &finger_status);
if (error) {
dev_err(&ts->client->dev,
"failed to read coordinates for finger %d:
%d\n",
@@ -261,6 +277,12 @@ static int imagis_probe(struct i2c_client *i2c)
ts->client = i2c;
+ ts->tdata = device_get_match_data(dev);
+ if (!ts->tdata) {
+ dev_err(dev, "missing chip data\n");
+ return -EINVAL;
+ }
+
error = imagis_init_regulators(ts);
if (error) {
dev_err(dev, "regulator init error: %d\n", error);
@@ -279,15 +301,13 @@ static int imagis_probe(struct i2c_client *i2c)
return error;
}
- error = imagis_i2c_read_reg(ts,
- IST3038C_REG_CHIPID | IST3038C_DIRECT_ACCESS,
- &chip_id);
+ error = imagis_i2c_read_reg(ts, ts->tdata->whoami_cmd, &chip_id);
if (error) {
dev_err(dev, "chip ID read failure: %d\n", error);
return error;
}
- if (chip_id != IST3038C_WHOAMI) {
+ if (chip_id != ts->tdata->whoami_val) {
dev_err(dev, "unknown chip ID: 0x%x\n", chip_id);
return -EINVAL;
}
@@ -343,9 +363,25 @@ static int imagis_resume(struct device *dev)
static DEFINE_SIMPLE_DEV_PM_OPS(imagis_pm_ops, imagis_suspend, imagis_resume);
+static const struct imagis_properties imagis_3038b_data = {
+ .interrupt_msg_cmd = IST3038B_REG_STATUS,
+ .touch_coord_cmd = IST3038B_REG_STATUS,
+ .whoami_cmd = IST3038B_REG_CHIPID,
+ .whoami_val = IST3038B_WHOAMI,
+ .protocol_b
[PATCH v4 2/5] dt-bindings: input/touchscreen: Add compatible for IST3038B
From: Markuss Broks Imagis IST3038B is a variant (firmware?) of Imagis IST3038 IC differing from IST3038C in its register interface. Add the compatible for it to the IST3038C bindings. Signed-off-by: Markuss Broks Acked-by: Conor Dooley [bal...@matfyz.cz: elaborate chip differences in the commit message] Signed-off-by: Karel Balej --- Notes: v4: * Mention how the chip is different in terms of the programming model in the commit message. * Add Conor's trailer. .../devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml index 0d6b033fd5fb..b5372c4eae56 100644 --- a/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml +++ b/Documentation/devicetree/bindings/input/touchscreen/imagis,ist3038c.yaml @@ -18,6 +18,7 @@ properties: compatible: enum: + - imagis,ist3038b - imagis,ist3038c reg: -- 2.43.0
[PATCH v4 1/5] input/touchscreen: imagis: Correct the maximum touch area value
From: Markuss Broks
As specified in downstream IST3038B driver and proved by testing,
the correct maximum reported value of touch area is 16.
Signed-off-by: Markuss Broks
Signed-off-by: Karel Balej
---
drivers/input/touchscreen/imagis.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/input/touchscreen/imagis.c
b/drivers/input/touchscreen/imagis.c
index 07111ca24455..e67fd3011027 100644
--- a/drivers/input/touchscreen/imagis.c
+++ b/drivers/input/touchscreen/imagis.c
@@ -210,7 +210,7 @@ static int imagis_init_input_dev(struct imagis_ts *ts)
input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_X);
input_set_capability(input_dev, EV_ABS, ABS_MT_POSITION_Y);
- input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 255, 0, 0);
+ input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0, 16, 0, 0);
touchscreen_parse_properties(input_dev, true, &ts->prop);
if (!ts->prop.max_x || !ts->prop.max_y) {
--
2.43.0
[PATCH v4 0/5] input/touchscreen: imagis: add support for IST3032C
From: Karel Balej Hello, this patch series generalizes the Imagis touchscreen driver to support other Imagis chips, namely IST3038B and IST3032C, which use a slightly different protocol. It also adds necessary information to the driver so that the IST3032C touchscreen can be used with it. The motivation for this is the samsung,coreprimevelte smartphone with which this series has been tested. However, the support for this device is not yet in-tree, the effort is happening at [1]. Preliminary version of the regulator driver needed to use the touchscreen on this phone can be found here [2]. Note that this is a prerequisite for this patch [3] which implements support for touch keys for Imagis touchscreens that have it. [1] https://lore.kernel.org/all/20240110-pxa1908-lkml-v8-0-fea768a59...@skole.hr/ [2] https://lore.kernel.org/all/20231228100208.2932-1-kar...@gimli.ms.mff.cuni.cz/ [3] https://lore.kernel.org/all/20231112194124.24916-1-duje.mihano...@skole.hr/ --- v4: - Rebase to v6.7. - v3: https://lore.kernel.org/all/20231202125948.10345-1-kar...@gimli.ms.mff.cuni.cz/ - Address feedback and add trailers. v3: - Rebase to v6.7-rc3. - v2: https://lore.kernel.org/all/20231003133440.4696-1-kar...@gimli.ms.mff.cuni.cz/ v2: - Do not rename the driver. - Do not hardcode voltage required by the IST3032C. - Use Markuss' series which generalizes the driver. Link to the original series: https://lore.kernel.org/all/20220504152406.8730-1-markuss.br...@gmail.com/ - Separate bindings into separate patch. - v1: https://lore.kernel.org/all/20230926173531.18715-1-bal...@matfyz.cz/ Karel Balej (2): dt-bindings: input/touchscreen: imagis: add compatible for IST3032C input/touchscreen: imagis: add support for IST3032C Markuss Broks (3): input/touchscreen: imagis: Correct the maximum touch area value dt-bindings: input/touchscreen: Add compatible for IST3038B input/touchscreen: imagis: Add support for Imagis IST3038B .../input/touchscreen/imagis,ist3038c.yaml| 2 + drivers/input/touchscreen/imagis.c| 70 +++ 2 files changed, 60 insertions(+), 12 deletions(-) -- 2.43.0
Re: [PATCH v2] kprobes: Use synchronize_rcu_tasks_rude in kprobe_optimizer
On Fri, Jan 19, 2024 at 06:37:26AM -0800, Paul E. McKenney wrote:
> On Thu, Jan 18, 2024 at 02:18:42AM +, Chen Zhongjin wrote:
> > There is a deadlock scenario in kprobe_optimizer():
> >
> > pid A pid B pid C
> > kprobe_optimizer() do_exit() perf_kprobe_init()
> > mutex_lock(&kprobe_mutex) exit_tasks_rcu_start()
> > mutex_lock(&kprobe_mutex)
> > synchronize_rcu_tasks() zap_pid_ns_processes() // waiting
> > kprobe_mutex
> > // waiting tasks_rcu_exit_srcu kernel_wait4()
> > // waiting pid C exit
> >
> > To avoid this deadlock loop, use synchronize_rcu_tasks_rude() in
> > kprobe_optimizer()
> > rather than synchronize_rcu_tasks(). synchronize_rcu_tasks_rude() can also
> > promise
> > that all preempted tasks have scheduled, but it will not wait
> > tasks_rcu_exit_srcu.
> >
> > Fixes: a30b85df7d59 ("kprobes: Use synchronize_rcu_tasks() for optprobe
> > with CONFIG_PREEMPT=y")
> > Signed-off-by: Chen Zhongjin
>
> Just so you know, your email ends up in gmail's spam folder. :-/
>
> > ---
> > v1 -> v2: Add Fixes tag
> > ---
> > arch/Kconfig | 2 +-
> > kernel/kprobes.c | 2 +-
> > 2 files changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/Kconfig b/arch/Kconfig
> > index f4b210ab0612..dc6a18854017 100644
> > --- a/arch/Kconfig
> > +++ b/arch/Kconfig
> > @@ -104,7 +104,7 @@ config STATIC_CALL_SELFTEST
> > config OPTPROBES
> > def_bool y
> > depends on KPROBES && HAVE_OPTPROBES
> > - select TASKS_RCU if PREEMPTION
> > + select TASKS_RUDE_RCU
> >
> > config KPROBES_ON_FTRACE
> > def_bool y
> > diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> > index d5a0ee40bf66..09056ae50c58 100644
> > --- a/kernel/kprobes.c
> > +++ b/kernel/kprobes.c
> > @@ -623,7 +623,7 @@ static void kprobe_optimizer(struct work_struct *work)
> > * Note that on non-preemptive kernel, this is transparently converted
> > * to synchronoze_sched() to wait for all interrupts to have completed.
> > */
> > - synchronize_rcu_tasks();
> > + synchronize_rcu_tasks_rude();
>
> Again, that comment reads in full as follows:
>
> /*
>* Step 2: Wait for quiesence period to ensure all potentially
>* preempted tasks to have normally scheduled. Because optprobe
>* may modify multiple instructions, there is a chance that Nth
>* instruction is preempted. In that case, such tasks can return
>* to 2nd-Nth byte of jump instruction. This wait is for avoiding it.
>* Note that on non-preemptive kernel, this is transparently converted
>* to synchronoze_sched() to wait for all interrupts to have completed.
>*/
>
> Please note well that first sentence.
>
> Unless that first sentence no longer holds, this patch cannot work
> because synchronize_rcu_tasks_rude() will not (repeat, NOT) wait for
> preempted tasks.
>
> So how to safely break this deadlock? Reproducing Chen Zhongjin's
> diagram:
>
> pid A pid B pid C
> kprobe_optimizer()do_exit() perf_kprobe_init()
> mutex_lock(&kprobe_mutex) exit_tasks_rcu_start()
> mutex_lock(&kprobe_mutex)
> synchronize_rcu_tasks() zap_pid_ns_processes() // waiting
> kprobe_mutex
> // waiting tasks_rcu_exit_srcukernel_wait4()
> // waiting pid C exit
>
> We need to stop synchronize_rcu_tasks() from waiting on tasks like
> pid B that are voluntarily blocked. One way to do that is to replace
> SRCU with a set of per-CPU lists. Then exit_tasks_rcu_start() adds the
> current task to this list and does ...
>
> OK, this is getting a bit involved. If you would like to follow along,
> please feel free to look here:
>
> https://docs.google.com/document/d/1MEHHs5qbbZBzhN8dGP17pt-d87WptFJ2ZQcqS221d9I/edit?usp=sharing
And please see below for a prototype patch, which passes moderate
rcutorture testing.
Chen Zhongjin, does this prevent the deadlock you have been seeing?
Thanx, Paul
commit 113fe0eeabe7a83e87d638d44b9e1d0f9691b146
Author: Paul E. McKenney
Date: Sat Jan 20 07:07:08 2024 -0800
rcu-tasks: Eliminate deadlocks involving do_exit() and RCU tasks
Holding a mutex across synchronize_rcu_tasks() and acquiring
that same mutex in code called from do_exit() after its call to
exit_tasks_rcu_start() but before its call to exit_tasks_rcu_stop()
results in deadlock. This is by design, because tasks that are far
enough into do_exit() are no longer present on the tasks list, making
it a bit difficult for RCU Tasks to find them, let alone wait on them
to do a voluntary context switch. However, such deadlocks are becoming
more frequent. In addition, lockdep currently does not detect s
Re: [PATCH] ring-buffer: Simplify reservation with try_cmpxchg() loop
On Sat, 20 Jan 2024 08:47:13 -0500
Steven Rostedt wrote:
> > I would also consider reducing code complexity as a worthwhile metric
> > in addition to speed. It makes it easier to extend in the future,
> > easier to understand for reviewers, and subtle bugs are less likely
> > to creep in.
>
> Really, it wouldn't make it that much simpler. The addition of the
> cmpxchg() of this patch removed the nasty part of the code.
Now let's look at the difference of the two. You still need to save the
current timestamp in one variable. I have to do it in two, so your
algorithm does have that advantage. I currently have (eliminating the
"always add absolute timestamp" switch):
w = write;
before = before_stamp;
again:
after = write_stamp; (equivalent to your last_tsc)
ts = rdtsc();
if (!w)
delta = 0; // event has same ts as subbuf
else if (before == after)
delta = ts - after;
else {
delta = 0;
inject_absolute = true;
}
before_stamp = ts;
if (!try_cmpxchg(&write, w, w + length))
goto again;
write_stamp = ts;
Now if I were to updated it to use a delta from the last injected
timestamp, where injecting a timestamp only happens on overflow.
#define TS_BITS 27
#define MAX_DELTA ((1 << TS_BITS) - 1)
#define BITMASK (~MAX_DELTA)
w = write;
again:
ts = rdtsc();
delta = ts & MAX_DELTA;
if (ts - (write_stamp & BITMASK) > MAX_DELTA)
inject_absolute = true;
if (!try_cmpxchg(&write, w, w + length))
goto again;
write_stamp = ts;
I admit that it does simplify the code a little, but does it do it
enough to be worth the process of deprecating an ABI with a new one?
-- Steve
Re: [PATCH] ring-buffer: Simplify reservation with try_cmpxchg() loop
On Fri, 19 Jan 2024 20:49:36 -0500 Mathieu Desnoyers wrote: > >> Let's say we have the following ktime_get() values (monotonic timestamp > >> value) for > >> a sequence of events: > >> > >> Timestamp (Hex)Encoding in the > >> trace > >> > >> Packet header timestamp begin 0x12345678 64-bit: 0x12345678 > >> > >> Event 1 0x12345678 16-bit: 0x5678 > >> (When decoded, same value as previous timestamp, no overflow) > >> Event 2 0x1234 16-bit: 0x > >> (When decoded, going from "0x5678" to "0x" does not overflow > >> 16-bit) > >> Event 3 0x1235 16-bit: 0x > >> (When decoded, going from "0x" to "0x" overflow 16-bit > >> exactly once > >> which allows the trace reader to reconstruct timestamp 0x1235 > >> from the > >> previous timestamp and the 16-bit timestamp encoding.) > >> Event 4 0x1237 64-bit: 0x1237 > >> (Encoding over 16-bit not possible because going from 0x1235 to > >> 0x1237 would overflow 16-bit twice, which cannot be detected > >> by a trace reader. Therefore use the full 64-bit timestamp in the > >> "large" event header representation.) > > > > I think that's basically what I said, but you are just looking at it > > differently ;-) Or should I say, you are using bits for optimization. > > Based on your explanation below, we are really talking about different things > here. Let me try to reply to your explanation to try to show where what I am > doing completely differs from what you have in mind. This will help explain > how I handle 16-bit overflow as well. > > > The events are based off of the last injected timestamp. > > Incorrect. There is no "injected timestamp". There is only a concept > of the "current timestamp" as we either write to or read from the > event stream. I will take the point of view of the trace reader for > the rest of the discussion. > > > The above example, > > starts with an timestamp injection into the packet header: 0x12345678, with > > the lsb 16bits ignore. > > Wrong again. The 16 least significant bits are not ignored. The "current > timestamp" > is really 0x12345678 when the packet header is read. In the packet header you have 0x12345678 in the first event you have 0x5678 how does that get you the timestamp? If that event had 0x, when the reader reads this packet, it would take the header 0x12345678 chop off (ignore) the 5678, and add the , right? > > > So in actuality, it inserts 0x1234, plus adds a > > 5678 that represents the creation of the header (like we discussed about in > > the last tracing meeting). > > There is no "0x1234" reference time. There is only the actual 0x12345678 > current time at packet begin, including those 16 low order bits. It could be 0x1234 for all I care. I know you use that as the start time for when tracing starts. But for the events in the packet (which is what I care about) that bottom 16 bits in the packet header isn't necessary. > > > > > The first event has: 0x5678 which is based on the previous injected > > timestamp of 0x1234. > > It is not "based" on the previous injected timestamp. It just represents > the low-order 16-bits of the timestamp at event 1. The trace readers takes > two informations to compute the complete current timestamp for event 1: > > 1) The current timestamp just before event 1 is read (0x12345678), And where does that reader get that information from? Either the packet header, or you need to inject a timestamp somewhere, don't you? > 2) The low-order 16 bits of event 1 (0x5678). > > Given that there is no 16-bit overflow when comparing: > > 0x12345678 & 0x and 0x5678 > > We know that the resulting current timestamp for event 1 is: > > (0x12345678 & ~0x) + 0x5678 = 0x12345678 And above you ignore the least 16 bits ;-) > > Or basically that time did not move between the packet header and event 1. > > > > > the events go on just using a delta from that until you see it overflow (or > > too big of a delta to fit in the 16 bits), and you insert a new full > > timestamps that everything will be based on: > > > >0x1237 > > No. The following events use the same algorithm I just described: They use Right, you mask off the bits of the timestamp. > this notion of "current timestamp" and the information provided by the new > timestamp field in the event header to figure out the updated current > timestamp. > > It is _never_ based on some kind of arbitrary reference, it is always > absolute, > and is always computed based on the current timestamp and the timestamp field > encountered. And that current timestamp was created by some injected timestamp in the writer. The reader has to get it from somewhere! > > > > > Now events following that are ju
