* [PATCH] [v3] HID: add support for Apple Magic Trackpad 2
From: Sean O'Brien @ 2018-09-20 23:13 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Sean O'Brien, Marek Wyborski, linux-kernel, Dmitry Torokhov,
linux-input, Henrik Rydberg, Jiri Kosina, Claudio Mettler
USB device
Vendor 05ac (Apple)
Device 0265 (Magic Trackpad 2)
Bluetooth device
Vendor 004c (Apple)
Device 0265 (Magic Trackpad 2)
Add support for Apple Magic Trackpad 2 over USB and bluetooth, putting
the device in multi-touch mode.
Signed-off-by: Claudio Mettler <claudio@ponyfleisch.ch>
Signed-off-by: Marek Wyborski <marek.wyborski@emwesoft.com>
Signed-off-by: Sean O'Brien <seobrien@chromium.org>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-magicmouse.c | 149 +++++++++++++++++++++++++++++++----
2 files changed, 134 insertions(+), 16 deletions(-)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index 5146ee029db4..bb0cd212c7cc 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -92,6 +92,7 @@
#define USB_DEVICE_ID_APPLE_MIGHTYMOUSE 0x0304
#define USB_DEVICE_ID_APPLE_MAGICMOUSE 0x030d
#define USB_DEVICE_ID_APPLE_MAGICTRACKPAD 0x030e
+#define USB_DEVICE_ID_APPLE_MAGICTRACKPAD2 0x0265
#define USB_DEVICE_ID_APPLE_FOUNTAIN_ANSI 0x020e
#define USB_DEVICE_ID_APPLE_FOUNTAIN_ISO 0x020f
#define USB_DEVICE_ID_APPLE_GEYSER_ANSI 0x0214
diff --git a/drivers/hid/hid-magicmouse.c b/drivers/hid/hid-magicmouse.c
index b454c4386157..6a3a6c83e509 100644
--- a/drivers/hid/hid-magicmouse.c
+++ b/drivers/hid/hid-magicmouse.c
@@ -54,6 +54,8 @@ module_param(report_undeciphered, bool, 0644);
MODULE_PARM_DESC(report_undeciphered, "Report undeciphered multi-touch state field using a MSC_RAW event");
#define TRACKPAD_REPORT_ID 0x28
+#define TRACKPAD2_USB_REPORT_ID 0x02
+#define TRACKPAD2_BT_REPORT_ID 0x31
#define MOUSE_REPORT_ID 0x29
#define DOUBLE_REPORT_ID 0xf7
/* These definitions are not precise, but they're close enough. (Bits
@@ -91,6 +93,17 @@ MODULE_PARM_DESC(report_undeciphered, "Report undeciphered multi-touch state fie
#define TRACKPAD_RES_Y \
((TRACKPAD_MAX_Y - TRACKPAD_MIN_Y) / (TRACKPAD_DIMENSION_Y / 100))
+#define TRACKPAD2_DIMENSION_X (float)16000
+#define TRACKPAD2_MIN_X -3678
+#define TRACKPAD2_MAX_X 3934
+#define TRACKPAD2_RES_X \
+ ((TRACKPAD2_MAX_X - TRACKPAD2_MIN_X) / (TRACKPAD2_DIMENSION_X / 100))
+#define TRACKPAD2_DIMENSION_Y (float)11490
+#define TRACKPAD2_MIN_Y -2478
+#define TRACKPAD2_MAX_Y 2587
+#define TRACKPAD2_RES_Y \
+ ((TRACKPAD2_MAX_Y - TRACKPAD2_MIN_Y) / (TRACKPAD2_DIMENSION_Y / 100))
+
/**
* struct magicmouse_sc - Tracks Magic Mouse-specific data.
* @input: Input device through which we report events.
@@ -183,6 +196,7 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
{
struct input_dev *input = msc->input;
int id, x, y, size, orientation, touch_major, touch_minor, state, down;
+ int pressure = 0;
if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
id = (tdata[6] << 2 | tdata[5] >> 6) & 0xf;
@@ -194,6 +208,20 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
touch_minor = tdata[4];
state = tdata[7] & TOUCH_STATE_MASK;
down = state != TOUCH_STATE_NONE;
+ } else if (input->id.product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) {
+ id = tdata[8] & 0xf;
+ x = (tdata[1] << 27 | tdata[0] << 19) >> 19;
+ y = -((tdata[3] << 30 | tdata[2] << 22 | tdata[1] << 14) >> 19);
+ size = tdata[6];
+ orientation = (tdata[8] >> 5) - 4;
+ touch_major = tdata[4];
+ touch_minor = tdata[5];
+ /* Add to pressure to prevent libraries such as libinput
+ * from ignoring low pressure touches
+ */
+ pressure = tdata[7] + 30;
+ state = tdata[3] & 0xC0;
+ down = state == 0x80;
} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
id = (tdata[7] << 2 | tdata[6] >> 6) & 0xf;
x = (tdata[1] << 27 | tdata[0] << 19) >> 19;
@@ -215,7 +243,8 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
/* If requested, emulate a scroll wheel by detecting small
* vertical touch motions.
*/
- if (emulate_scroll_wheel) {
+ if (emulate_scroll_wheel && (input->id.product !=
+ USB_DEVICE_ID_APPLE_MAGICTRACKPAD2)) {
unsigned long now = jiffies;
int step_x = msc->touches[id].scroll_x - x;
int step_y = msc->touches[id].scroll_y - y;
@@ -269,10 +298,14 @@ static void magicmouse_emit_touch(struct magicmouse_sc *msc, int raw_id, u8 *tda
input_report_abs(input, ABS_MT_POSITION_X, x);
input_report_abs(input, ABS_MT_POSITION_Y, y);
+ if (input->id.product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD2)
+ input_report_abs(input, ABS_MT_PRESSURE, pressure);
+
if (report_undeciphered) {
if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE)
input_event(input, EV_MSC, MSC_RAW, tdata[7]);
- else /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
+ else if (input->id.product !=
+ USB_DEVICE_ID_APPLE_MAGICTRACKPAD2)
input_event(input, EV_MSC, MSC_RAW, tdata[8]);
}
}
@@ -287,6 +320,7 @@ static int magicmouse_raw_event(struct hid_device *hdev,
switch (data[0]) {
case TRACKPAD_REPORT_ID:
+ case TRACKPAD2_BT_REPORT_ID:
/* Expect four bytes of prefix, and N*9 bytes of touch data. */
if (size < 4 || ((size - 4) % 9) != 0)
return 0;
@@ -301,12 +335,22 @@ static int magicmouse_raw_event(struct hid_device *hdev,
magicmouse_emit_touch(msc, ii, data + ii * 9 + 4);
clicks = data[1];
+ break;
+ case TRACKPAD2_USB_REPORT_ID:
+ /* Expect twelve bytes of prefix and N*9 bytes of touch data. */
+ if (size < 12 || ((size - 12) % 9) != 0)
+ return 0;
+ npoints = (size - 12) / 9;
+ if (npoints > 15) {
+ hid_warn(hdev, "invalid size value (%d) for TRACKPAD2_USB_REPORT_ID\n",
+ size);
+ return 0;
+ }
+ msc->ntouches = 0;
+ for (ii = 0; ii < npoints; ii++)
+ magicmouse_emit_touch(msc, ii, data + ii * 9 + 12);
- /* The following bits provide a device specific timestamp. They
- * are unused here.
- *
- * ts = data[1] >> 6 | data[2] << 2 | data[3] << 10;
- */
+ clicks = data[1];
break;
case MOUSE_REPORT_ID:
/* Expect six bytes of prefix, and N*8 bytes of touch data. */
@@ -352,6 +396,9 @@ static int magicmouse_raw_event(struct hid_device *hdev,
magicmouse_emit_buttons(msc, clicks & 3);
input_report_rel(input, REL_X, x);
input_report_rel(input, REL_Y, y);
+ } else if (input->id.product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) {
+ input_mt_sync_frame(input);
+ input_report_key(input, BTN_MOUSE, clicks & 1);
} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
input_report_key(input, BTN_MOUSE, clicks & 1);
input_mt_report_pointer_emulation(input, true);
@@ -364,6 +411,7 @@ static int magicmouse_raw_event(struct hid_device *hdev,
static int magicmouse_setup_input(struct input_dev *input, struct hid_device *hdev)
{
int error;
+ int mt_flags = 0;
__set_bit(EV_KEY, input->evbit);
@@ -380,6 +428,22 @@ static int magicmouse_setup_input(struct input_dev *input, struct hid_device *hd
__set_bit(REL_WHEEL, input->relbit);
__set_bit(REL_HWHEEL, input->relbit);
}
+ } else if (input->id.product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) {
+ /* setting the device name to ensure the same driver settings
+ * get loaded, whether connected through bluetooth or USB
+ */
+ input->name = "Apple Inc. Magic Trackpad 2";
+
+ __clear_bit(EV_MSC, input->evbit);
+ __clear_bit(BTN_0, input->keybit);
+ __clear_bit(BTN_RIGHT, input->keybit);
+ __clear_bit(BTN_MIDDLE, input->keybit);
+ __set_bit(BTN_MOUSE, input->keybit);
+ __set_bit(INPUT_PROP_BUTTONPAD, input->propbit);
+ __set_bit(BTN_TOOL_FINGER, input->keybit);
+
+ mt_flags = INPUT_MT_POINTER | INPUT_MT_DROP_UNUSED |
+ INPUT_MT_TRACK;
} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
/* input->keybit is initialized with incorrect button info
* for Magic Trackpad. There really is only one physical
@@ -402,14 +466,13 @@ static int magicmouse_setup_input(struct input_dev *input, struct hid_device *hd
__set_bit(EV_ABS, input->evbit);
- error = input_mt_init_slots(input, 16, 0);
+ error = input_mt_init_slots(input, 16, mt_flags);
if (error)
return error;
input_set_abs_params(input, ABS_MT_TOUCH_MAJOR, 0, 255 << 2,
4, 0);
input_set_abs_params(input, ABS_MT_TOUCH_MINOR, 0, 255 << 2,
4, 0);
- input_set_abs_params(input, ABS_MT_ORIENTATION, -31, 32, 1, 0);
/* Note: Touch Y position from the device is inverted relative
* to how pointer motion is reported (and relative to how USB
@@ -418,6 +481,7 @@ static int magicmouse_setup_input(struct input_dev *input, struct hid_device *hd
* inverse of the reported Y.
*/
if (input->id.product == USB_DEVICE_ID_APPLE_MAGICMOUSE) {
+ input_set_abs_params(input, ABS_MT_ORIENTATION, -31, 32, 1, 0);
input_set_abs_params(input, ABS_MT_POSITION_X,
MOUSE_MIN_X, MOUSE_MAX_X, 4, 0);
input_set_abs_params(input, ABS_MT_POSITION_Y,
@@ -427,7 +491,25 @@ static int magicmouse_setup_input(struct input_dev *input, struct hid_device *hd
MOUSE_RES_X);
input_abs_set_res(input, ABS_MT_POSITION_Y,
MOUSE_RES_Y);
+ } else if (input->id.product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) {
+ input_set_abs_params(input, ABS_MT_PRESSURE, 0, 283, 0, 0);
+ input_set_abs_params(input, ABS_PRESSURE, 0, 283, 0, 0);
+ input_set_abs_params(input, ABS_MT_ORIENTATION, -3, 4, 0, 0);
+ input_set_abs_params(input, ABS_X, TRACKPAD2_MIN_X,
+ TRACKPAD2_MAX_X, 0, 0);
+ input_set_abs_params(input, ABS_Y, TRACKPAD2_MIN_Y,
+ TRACKPAD2_MAX_Y, 0, 0);
+ input_set_abs_params(input, ABS_MT_POSITION_X,
+ TRACKPAD2_MIN_X, TRACKPAD2_MAX_X, 0, 0);
+ input_set_abs_params(input, ABS_MT_POSITION_Y,
+ TRACKPAD2_MIN_Y, TRACKPAD2_MAX_Y, 0, 0);
+
+ input_abs_set_res(input, ABS_X, TRACKPAD2_RES_X);
+ input_abs_set_res(input, ABS_Y, TRACKPAD2_RES_Y);
+ input_abs_set_res(input, ABS_MT_POSITION_X, TRACKPAD2_RES_X);
+ input_abs_set_res(input, ABS_MT_POSITION_Y, TRACKPAD2_RES_Y);
} else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
+ input_set_abs_params(input, ABS_MT_ORIENTATION, -31, 32, 1, 0);
input_set_abs_params(input, ABS_X, TRACKPAD_MIN_X,
TRACKPAD_MAX_X, 4, 0);
input_set_abs_params(input, ABS_Y, TRACKPAD_MIN_Y,
@@ -447,7 +529,8 @@ static int magicmouse_setup_input(struct input_dev *input, struct hid_device *hd
input_set_events_per_packet(input, 60);
- if (report_undeciphered) {
+ if (report_undeciphered &&
+ input->id.product != USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) {
__set_bit(EV_MSC, input->evbit);
__set_bit(MSC_RAW, input->mscbit);
}
@@ -465,7 +548,8 @@ static int magicmouse_input_mapping(struct hid_device *hdev,
msc->input = hi->input;
/* Magic Trackpad does not give relative data after switching to MT */
- if (hi->input->id.product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD &&
+ if ((hi->input->id.product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD ||
+ hi->input->id.product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) &&
field->flags & HID_MAIN_ITEM_RELATIVE)
return -1;
@@ -494,11 +578,20 @@ static int magicmouse_input_configured(struct hid_device *hdev,
static int magicmouse_probe(struct hid_device *hdev,
const struct hid_device_id *id)
{
- const u8 feature[] = { 0xd7, 0x01 };
+ const u8 *feature;
+ const u8 feature_mt[] = { 0xD7, 0x01 };
+ const u8 feature_mt_trackpad2_usb[] = { 0x02, 0x01 };
+ const u8 feature_mt_trackpad2_bt[] = { 0xF1, 0x02, 0x01 };
u8 *buf;
struct magicmouse_sc *msc;
struct hid_report *report;
int ret;
+ int feature_size;
+
+ if (id->vendor == USB_VENDOR_ID_APPLE &&
+ id->product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD2 &&
+ hdev->type != HID_TYPE_USBMOUSE)
+ return 0;
msc = devm_kzalloc(&hdev->dev, sizeof(*msc), GFP_KERNEL);
if (msc == NULL) {
@@ -532,7 +625,14 @@ static int magicmouse_probe(struct hid_device *hdev,
if (id->product == USB_DEVICE_ID_APPLE_MAGICMOUSE)
report = hid_register_report(hdev, HID_INPUT_REPORT,
MOUSE_REPORT_ID, 0);
- else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
+ else if (id->product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) {
+ if (id->vendor == BT_VENDOR_ID_APPLE)
+ report = hid_register_report(hdev, HID_INPUT_REPORT,
+ TRACKPAD2_BT_REPORT_ID, 0);
+ else /* USB_VENDOR_ID_APPLE */
+ report = hid_register_report(hdev, HID_INPUT_REPORT,
+ TRACKPAD2_USB_REPORT_ID, 0);
+ } else { /* USB_DEVICE_ID_APPLE_MAGICTRACKPAD */
report = hid_register_report(hdev, HID_INPUT_REPORT,
TRACKPAD_REPORT_ID, 0);
report = hid_register_report(hdev, HID_INPUT_REPORT,
@@ -546,7 +646,20 @@ static int magicmouse_probe(struct hid_device *hdev,
}
report->size = 6;
- buf = kmemdup(feature, sizeof(feature), GFP_KERNEL);
+ if (id->product == USB_DEVICE_ID_APPLE_MAGICTRACKPAD2) {
+ if (id->vendor == BT_VENDOR_ID_APPLE) {
+ feature_size = sizeof(feature_mt_trackpad2_bt);
+ feature = feature_mt_trackpad2_bt;
+ } else { /* USB_VENDOR_ID_APPLE */
+ feature_size = sizeof(feature_mt_trackpad2_usb);
+ feature = feature_mt_trackpad2_usb;
+ }
+ } else {
+ feature_size = sizeof(feature_mt);
+ feature = feature_mt;
+ }
+
+ buf = kmemdup(feature, feature_size, GFP_KERNEL);
if (!buf) {
ret = -ENOMEM;
goto err_stop_hw;
@@ -560,10 +673,10 @@ static int magicmouse_probe(struct hid_device *hdev,
* but there seems to be no other way of switching the mode.
* Thus the super-ugly hacky success check below.
*/
- ret = hid_hw_raw_request(hdev, buf[0], buf, sizeof(feature),
+ ret = hid_hw_raw_request(hdev, buf[0], buf, feature_size,
HID_FEATURE_REPORT, HID_REQ_SET_REPORT);
kfree(buf);
- if (ret != -EIO && ret != sizeof(feature)) {
+ if (ret != -EIO && ret != feature_size) {
hid_err(hdev, "unable to request touch data (%d)\n", ret);
goto err_stop_hw;
}
@@ -579,6 +692,10 @@ static const struct hid_device_id magic_mice[] = {
USB_DEVICE_ID_APPLE_MAGICMOUSE), .driver_data = 0 },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_APPLE,
USB_DEVICE_ID_APPLE_MAGICTRACKPAD), .driver_data = 0 },
+ { HID_BLUETOOTH_DEVICE(BT_VENDOR_ID_APPLE,
+ USB_DEVICE_ID_APPLE_MAGICTRACKPAD2), .driver_data = 0 },
+ { HID_USB_DEVICE(USB_VENDOR_ID_APPLE,
+ USB_DEVICE_ID_APPLE_MAGICTRACKPAD2), .driver_data = 0 },
{ }
};
MODULE_DEVICE_TABLE(hid, magic_mice);
--
2.19.0.444.g18242da7ef-goog
^ permalink raw reply related
* Re: [PATCH v2] HID: logitech: fix a used uninitialized GCC warning
From: zhong jiang @ 2018-09-21 1:46 UTC (permalink / raw)
To: jikos, benjamin.tissoires; +Cc: linux-input, linux-kernel
In-Reply-To: <1536824470-36659-1-git-send-email-zhongjiang@huawei.com>
Hi Jiri
Can you pick up the patch?
Thanks
zhong jiang
On 2018/9/13 15:41, zhong jiang wrote:
> Fix the following compile warning:
>
> drivers/hid/hid-logitech-hidpp.c: In function 'hi_res_scroll_enable':
> drivers/hid/hid-logitech-hidpp.c:2714:54: warning: 'multiplier' may be used uninitialized in this function [-Wmaybe-uninitialized]
> hidpp->vertical_wheel_counter.resolution_multiplier = multiplier;
>
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
> v1->v2:
> According to Benjamin's suggestion, To initialize the value
> and remove the duplicated assignement.
>
> drivers/hid/hid-logitech-hidpp.c | 8 +++-----
> 1 file changed, 3 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> index 5f0c080..f012808 100644
> --- a/drivers/hid/hid-logitech-hidpp.c
> +++ b/drivers/hid/hid-logitech-hidpp.c
> @@ -1231,7 +1231,6 @@ static int hidpp_hrw_get_wheel_capability(struct hidpp_device *hidpp,
> *multiplier = response.fap.params[0];
> return 0;
> return_default:
> - *multiplier = 8;
> hid_warn(hidpp->hid_dev,
> "Couldn't get wheel multiplier (error %d), assuming %d.\n",
> ret, *multiplier);
> @@ -2696,7 +2695,7 @@ static int hi_res_scroll_look_up_microns(__u32 product_id)
> static int hi_res_scroll_enable(struct hidpp_device *hidpp)
> {
> int ret;
> - u8 multiplier;
> + u8 multiplier = 8;
>
> if (hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL_X2121) {
> ret = hidpp_hrw_set_wheel_mode(hidpp, false, true, false);
> @@ -2704,10 +2703,9 @@ static int hi_res_scroll_enable(struct hidpp_device *hidpp)
> } else if (hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL_X2120) {
> ret = hidpp_hrs_set_highres_scrolling_mode(hidpp, true,
> &multiplier);
> - } else /* if (hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL_1P0) */ {
> + } else /* if (hidpp->quirks & HIDPP_QUIRK_HI_RES_SCROLL_1P0) */
> ret = hidpp10_enable_scrolling_acceleration(hidpp);
> - multiplier = 8;
> - }
> +
> if (ret)
> return ret;
>
^ permalink raw reply
* Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards
From: Linus Walleij @ 2018-09-21 15:36 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Dmitry Torokhov, Rafael J. Wysocki, Linux Input,
open list:GPIO SUBSYSTEM, linux-kernel@vger.kernel.org,
Andy Shevchenko
In-Reply-To: <20180920135348.GF11965@kuha.fi.intel.com>
On Thu, Sep 20, 2018 at 6:53 AM Heikki Krogerus
<heikki.krogerus@linux.intel.com> wrote:
> The child nodes will change the purpose of the build-in property
> support. Originally the goal was just to support adding of build-in
> device properties to real firmware nodes, but things have changed
> quite a bit from that. These child nodes are purely tied to the
> build-in device property support, so we should be talking about adding
> pset type child nodes to pset type parent nodes in the API:
> fwnode_pset_add_child_node(), or something like that.
>
> I'm sorry to be a bit of pain in the butt with this. The build-in
> property support is a hack, it always was. A useful hack, but hack
> nevertheless. That means we need to be extra careful when expanding
> it, like here.
I dunno how we got to here, what I tried to solve and Dmitry tries
to make more general is converting old boardfiles to use fwnode, because
I initially tried to just support gpio descriptors from board files.
If boardfiles is what you mean with "build-in property support" I don't
know, it predates both device tree and ACPI and any other hardware
descriptions on the ARM architecture, from the perspective of these
systems things are fine and the hardware description languages
are the novelty...
But I guess you know because you worked with OMAP :)
So there is something I don't understand here.
Yours,
Linus Walleij
^ permalink raw reply
* WARNING: kmalloc bug in input_mt_init_slots
From: syzbot @ 2018-09-21 17:24 UTC (permalink / raw)
To: dmitry.torokhov, linux-input, linux-kernel, rydberg,
syzkaller-bugs
Hello,
syzbot found the following crash on:
HEAD commit: 234b69e3e089 ocfs2: fix ocfs2 read block panic
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=131f761a400000
kernel config: https://syzkaller.appspot.com/x/.config?x=5fa12be50bca08d8
dashboard link: https://syzkaller.appspot.com/bug?extid=87829a10073277282ad1
compiler: gcc (GCC) 8.0.1 20180413 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=126ca61a400000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=119d6511400000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+87829a10073277282ad1@syzkaller.appspotmail.com
input: syz0 as /devices/virtual/input/input25382
WARNING: CPU: 0 PID: 11238 at mm/slab_common.c:1031 kmalloc_slab+0x56/0x70
mm/slab_common.c:1031
Kernel panic - not syncing: panic_on_warn set ...
CPU: 0 PID: 11238 Comm: syz-executor124 Not tainted 4.19.0-rc4+ #25
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x1c4/0x2b4 lib/dump_stack.c:113
panic+0x238/0x4e7 kernel/panic.c:184
__warn.cold.8+0x163/0x1ba kernel/panic.c:536
report_bug+0x254/0x2d0 lib/bug.c:186
fixup_bug arch/x86/kernel/traps.c:178 [inline]
do_error_trap+0x1fc/0x4d0 arch/x86/kernel/traps.c:296
do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:316
invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:993
RIP: 0010:kmalloc_slab+0x56/0x70 mm/slab_common.c:1031
kobject: 'input25395' (00000000663cc863): kobject_cleanup, parent
(null)
Code: c5 40 2b 17 89 5d c3 48 85 ff b8 10 00 00 00 74 f4 83 ef 01 c1 ef 03
0f b6 87 60 2a 17 89 eb d8 31 c0 81 e6 00 02 00 00 75 db <0f> 0b 5d c3 48
8b 04 c5 80 2a 17 89 5d c3 66 90 66 2e 0f 1f 84 00
kobject: 'input25395' (00000000663cc863): calling ktype release
RSP: 0018:ffff8801c477f978 EFLAGS: 00010246
RAX: 0000000000000000 RBX: 00000000fffffffd RCX: ffffffff8534b947
RDX: 0000000000000000 RSI: 0000000000000000 RDI: 0000003fffffff60
RBP: ffff8801c477f978 R08: ffff8801d2eee000 R09: ffffed003731ee41
R10: ffff8801c477fa48 R11: ffff8801b98f720f R12: 0000000000000000
R13: 0000000000000000 R14: ffff8801b92ac9c0 R15: 00000000006080c0
__do_kmalloc mm/slab.c:3713 [inline]
__kmalloc+0x25/0x760 mm/slab.c:3727
kobject: 'input25395': free name
kmalloc include/linux/slab.h:518 [inline]
kzalloc include/linux/slab.h:707 [inline]
input_mt_init_slots+0xe5/0x4a0 drivers/input/input-mt.c:52
uinput_create_device drivers/input/misc/uinput.c:335 [inline]
uinput_ioctl_handler.isra.10+0x2049/0x2540 drivers/input/misc/uinput.c:876
uinput_ioctl+0x4c/0x60 drivers/input/misc/uinput.c:1047
vfs_ioctl fs/ioctl.c:46 [inline]
file_ioctl fs/ioctl.c:501 [inline]
do_vfs_ioctl+0x1de/0x1720 fs/ioctl.c:685
ksys_ioctl+0xa9/0xd0 fs/ioctl.c:702
__do_sys_ioctl fs/ioctl.c:709 [inline]
__se_sys_ioctl fs/ioctl.c:707 [inline]
__x64_sys_ioctl+0x73/0xb0 fs/ioctl.c:707
do_syscall_64+0x1b9/0x820 arch/x86/entry/common.c:290
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x446ec9
Code: e8 2c b3 02 00 48 83 c4 18 c3 0f 1f 80 00 00 00 00 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 2b 09 fc ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fa83f4b1da8 EFLAGS: 00000246 ORIG_RAX: 0000000000000010
RAX: ffffffffffffffda RBX: 00000000006dbc28 RCX: 0000000000446ec9
RDX: 0000000000446ec9 RSI: 0000000000005501 RDI: 0000000000000004
RBP: 00000000006dbc20 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000006dbc2c
R13: 6e69752f7665642f R14: 00007fa83f4b29c0 R15: 00000000006dbd2c
Kernel Offset: disabled
Rebooting in 86400 seconds..
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#bug-status-tracking for how to communicate with
syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: WARNING: kmalloc bug in input_mt_init_slots
From: Dmitry Torokhov @ 2018-09-21 17:52 UTC (permalink / raw)
To: syzbot+87829a10073277282ad1, Christoph Lameter, Pekka Enberg
Cc: linux-input@vger.kernel.org, lkml, Henrik Rydberg, syzkaller-bugs
In-Reply-To: <000000000000e5f76c057664e73d@google.com>
On Fri, Sep 21, 2018 at 10:24 AM syzbot
<syzbot+87829a10073277282ad1@syzkaller.appspotmail.com> wrote:
>
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: 234b69e3e089 ocfs2: fix ocfs2 read block panic
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=131f761a400000
> kernel config: https://syzkaller.appspot.com/x/.config?x=5fa12be50bca08d8
> dashboard link: https://syzkaller.appspot.com/bug?extid=87829a10073277282ad1
> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=126ca61a400000
> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=119d6511400000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+87829a10073277282ad1@syzkaller.appspotmail.com
>
> input: syz0 as /devices/virtual/input/input25382
> WARNING: CPU: 0 PID: 11238 at mm/slab_common.c:1031 kmalloc_slab+0x56/0x70
> mm/slab_common.c:1031
> Kernel panic - not syncing: panic_on_warn set ...
This is coming from:
commit 6286ae97d10ea2b5cd90532163797ab217bfdbdf
Author: Christoph Lameter <cl@linux.com>
Date: Fri May 3 15:43:18 2013 +0000
slab: Return NULL for oversized allocations
The inline path seems to have changed the SLAB behavior for very large
kmalloc allocations with commit e3366016 ("slab: Use common
kmalloc_index/kmalloc_size functions"). This patch restores the old
behavior but also adds diagnostics so that we can figure where in the
code these large allocations occur.
Reported-and-tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
Signed-off-by: Christoph Lameter <cl@linux.com>
Link: http://lkml.kernel.org/r/201305040348.CIF81716.OStQOHFJMFLOVF@I-love.SAKURA.ne.jp
[ penberg@kernel.org: use WARN_ON_ONCE ]
Signed-off-by: Pekka Enberg <penberg@kernel.org>
You'll have to convince Cristoph that WARN_ON_ONCE() there is evil and
has to be eradicated so that KASAN can run (but then we'd not know
easily that some allocation failed because it was too big and never
had a chance of succeeding vs. ordinary memory failure).
Can I recommend that maybe you introduce infrastructure for
panic_on_warn to ignore certain "well known" warnings?
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards
From: Dmitry Torokhov @ 2018-09-21 23:31 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Linus Walleij, Rafael J . Wysocki, linux-input, linux-gpio,
linux-kernel, Andy Shevchenko
In-Reply-To: <20180920135348.GF11965@kuha.fi.intel.com>
Hi Heikki,
On Thu, Sep 20, 2018 at 04:53:48PM +0300, Heikki Krogerus wrote:
> Hi Dmitry,
>
> On Mon, Sep 17, 2018 at 11:16:00AM -0700, Dmitry Torokhov wrote:
> > +/**
> > + * device_add_child_properties - Add a collection of properties to a device object.
> > + * @dev: Device to add properties to.
>
> In case you didn't notice my comment for this, you are missing @parent
> here.
>
> But why do you need both the parent and the dev?
I could go by parent only and fetch dev from parent.
>
> > + * @properties: Collection of properties to add.
> > + *
> > + * Associate a collection of device properties represented by @properties as a
> > + * child of given @parent firmware node. The function takes a copy of
> > + * @properties.
> > + */
> > +struct fwnode_handle *
> > +device_add_child_properties(struct device *dev,
> > + struct fwnode_handle *parent,
> > + const struct property_entry *properties)
> > +{
> > + struct property_set *p;
> > + struct property_set *parent_pset;
> > +
> > + if (!properties)
> > + return ERR_PTR(-EINVAL);
> > +
> > + parent_pset = to_pset_node(parent);
>
> For this function, the parent will in practice have to be
> dev_fwnode(dev), so I don't think you need @parent at all, no?
>
> There is something wrong here..
Yes, I expect majority of the calls will use dev_fwnode(dev) as parent,
but nobody stops you from doing:
device_add_properties(dev, props);
c1 = device_add_child_properties(dev, dev_fwnode(dev), cp1);
c2 = device_add_child_properties(dev, c1, cp2);
c3 = device_add_child_properties(dev, c2, cp3);
...
>
> > + if (!parent_pset)
> > + return ERR_PTR(-EINVAL);
> > +
> > + p = pset_create_set(properties);
> > + if (IS_ERR(p))
> > + return ERR_CAST(p);
> > +
> > + p->dev = dev;
>
> That looks wrong.
>
> I'm guessing the assumption here is that the child nodes will never be
> assigned to their own devices, but you can't do that. It will limit
> the use of the child nodes to a very small number of cases, possibly
> only to gpios.
If I need to assign a node to a device I'll use device_add_properties()
API. device_add_child_properties() is for nodes living "below" the
device.
All nodes (the primary/secondary and children) would point to the owning
device, just for convenience.
>
> I think that has to be fixed. It should not be a big deal. Just expect
> the child nodes to be removed separately, and add ref counting to the
> struct property_set handling.
Why do we need to remove them separately and what do we need refcounting
for?
>
> > + p->parent = parent_pset;
> > + list_add_tail(&p->child_node, &parent_pset->children);
> > +
> > + return &p->fwnode;
> > +}
> > +EXPORT_SYMBOL_GPL(device_add_child_properties);
>
> The child nodes will change the purpose of the build-in property
> support. Originally the goal was just to support adding of build-in
> device properties to real firmware nodes, but things have changed
> quite a bit from that. These child nodes are purely tied to the
> build-in device property support, so we should be talking about adding
> pset type child nodes to pset type parent nodes in the API:
> fwnode_pset_add_child_node(), or something like that.
OK, I can change device_add_child_properties() to
fwnode_pset_add_child_node() if Rafael would prefer this name.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards
From: Dmitry Torokhov @ 2018-09-21 23:33 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Linus Walleij, Rafael J . Wysocki, linux-input, linux-gpio,
linux-kernel, Andy Shevchenko
In-Reply-To: <20180920101648.GC11965@kuha.fi.intel.com>
On Thu, Sep 20, 2018 at 01:16:48PM +0300, Heikki Krogerus wrote:
> On Wed, Sep 19, 2018 at 10:13:26AM -0700, Dmitry Torokhov wrote:
> > > > diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
> > > > index 08ecc13080ae..63f2377aefe8 100644
> > > > --- a/drivers/base/pset_property.c
> > > > +++ b/drivers/base/pset_property.c
> > > > @@ -18,6 +18,11 @@ struct property_set {
> > > > struct device *dev;
> > > > struct fwnode_handle fwnode;
> > > > const struct property_entry *properties;
> > > > +
> > > > + struct property_set *parent;
> > > > + /* Entry in parent->children list */
> > > > + struct list_head child_node;
> > > > + struct list_head children;
> > >
> > > Add
> > >
> > > const char *name;
> > >
> > > and you can implement also pset_get_named_child_node().
> >
> > Or
> > char name[];
> >
> > to avoid separate allocation.
>
> Let's not do that, especially if you are planning on exporting this
> structure.
Can you please elaborate why? Not using pointer saves us 4/8 bytes +
however much memory we need for bookkeeping for the extra chunk. Given
that majority of pset nodes are unnamed this seems wasteful.
> If the name is coming from .rodata, there is no need to
> allocate anything for the name. Check kstrdup_const().
The data is most likely coming as __initconst so we do need to copy it.
>
> > Alternatively, we can add it later when we need it, and add
> > device_add_named_child_properties().
> >
> > I'll leave it up to Rafael to decide.
>
> Fair enough.
>
>
> Thanks,
>
> --
> heikki
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH V2 1/8] dt-bindings: mfd: document stpmic1
From: Lee Jones @ 2018-09-23 13:19 UTC (permalink / raw)
To: Pascal PAILLET-LME
Cc: dmitry.torokhov@gmail.com, robh+dt@kernel.org,
mark.rutland@arm.com, lgirdwood@gmail.com, broonie@kernel.org,
wim@linux-watchdog.org, linux@roeck-us.net,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org,
benjamin.gaignard@linaro.org, eballetbo@gmail.com
In-Reply-To: <1536325173-16617-2-git-send-email-p.paillet@st.com>
On Fri, 07 Sep 2018, Pascal PAILLET-LME wrote:
> From: pascal paillet <p.paillet@st.com>
>
> stpmic1 is a pmic from STMicroelectronics. The stpmic1 integrates 10
> regulators and 3 switches with various capabilities.
>
> Signed-off-by: pascal paillet <p.paillet@st.com>
> ---
> changes in v2:
> * the hardware component has been renamed from stpmu1 to stpmic1 !
> * replace _ with - in properties name
> * fix node names in example
> * remove regulator compatibles in example
> * add st,stpmic1.h to the patch
>
> Rob, I did not change the usage of the properties because it would lead to a lot
> of st properties; for example st,main-control-register would be replaced by:
> st,power_cycling_on_turn_off
> st,pwrctrl_enabled
> st,pwrctrl_active_high
> should I go this way ?
>
> Rob, I did not found the standard property for st,onkey-press-seconds = <10>;
>
> .../devicetree/bindings/mfd/st,stpmic1.txt | 137 +++++++++++++++++++++
> include/dt-bindings/mfd/st,stpmic1.h | 46 +++++++
> 2 files changed, 183 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/mfd/st,stpmic1.txt
> create mode 100644 include/dt-bindings/mfd/st,stpmic1.h
>
> diff --git a/Documentation/devicetree/bindings/mfd/st,stpmic1.txt b/Documentation/devicetree/bindings/mfd/st,stpmic1.txt
> new file mode 100644
> index 0000000..9f2c516
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/mfd/st,stpmic1.txt
> @@ -0,0 +1,137 @@
> +* STMicroelectronics STPMIC1 Power Management IC
> +
> +Required parent device properties:
> +- compatible: "st,stpmic1"
> +- reg: the I2C slave address for the stpmic1 chip
> +- interrupts-extended: interrupt lines to use: second irq is for wakeup.
> +- #interrupt-cells: should be 2.
> +- interrupt-controller: describes the STPMIC1 as an interrupt
I always find these are easier to read when they are tab aligned.
While we're at it, let's throw some capital letters in there and
remove the '.', as it's only used on one of the lines:
- compatible: "st,stpmic1"
- reg: The I2C slave address for the STPIC1 chip
- interrupts-extended: Interrupt lines to use: second IRQ is for wakeup.
- #interrupt-cells: Should be 2
- interrupt-controller: Describes the STPMIC1 as an interrupt
Better, no?
> + controller (has its own domain). interrupt number are the following:
Capital letters usually follow a '.'
> + /* Interrupt Register 1 (0x50 for latch) */
> + IT_SWOUT_R=0
> + IT_SWOUT_F=1
> + IT_VBUS_OTG_R=2
> + IT_VBUS_OTG_F=3
> + IT_WAKEUP_R=4
> + IT_WAKEUP_F=5
> + IT_PONKEY_R=6
> + IT_PONKEY_F=7
> + /* Interrupt Register 2 (0x51 for latch) */
> + IT_OVP_BOOST=8
> + IT_OCP_BOOST=9
> + IT_OCP_SWOUT=10
> + IT_OCP_OTG=11
> + IT_CURLIM_BUCK4=12
> + IT_CURLIM_BUCK3=13
> + IT_CURLIM_BUCK2=14
> + IT_CURLIM_BUCK1=15
> + /* Interrupt Register 3 (0x52 for latch) */
> + IT_SHORT_SWOUT=16
> + IT_SHORT_SWOTG=17
> + IT_CURLIM_LDO6=18
> + IT_CURLIM_LDO5=19
> + IT_CURLIM_LDO4=20
> + IT_CURLIM_LDO3=21
> + IT_CURLIM_LDO2=22
> + IT_CURLIM_LDO1=23
> + /* Interrupt Register 3 (0x52 for latch) */
> + IT_SWIN_R=24
> + IT_SWIN_F=25
> + IT_RESERVED_1=26
> + IT_RESERVED_2=27
> + IT_VINLOW_R=28
> + IT_VINLOW_F=29
> + IT_TWARN_R=30
> + IT_TWARN_F=31
> +
> +Optional parent device properties:
> +- st,main-control-register:
> + -bit 1: Power cycling will be performed on turn OFF condition
> + -bit 2: PWRCTRL is functional
> + -bit 3: PWRCTRL active high
> +- st,pads-pull-register:
> + -bit 1: WAKEUP pull down is not active
> + -bit 2: PWRCTRL pull up is active
> + -bit 3: PWRCTRL pull down is active
> + -bit 4: WAKEUP detector is disabled
> +- st,vin-control-register:
> + -bit 0: VINLOW monitoring is enabled
> + -bit [1...3]: VINLOW rising threshold
> + 000 VINOK_f + 50mV
> + 001 VINOK_f + 100mV
> + 010 VINOK_f + 150mV
> + 011 VINOK_f + 200mV
> + 100 VINOK_f + 250mV
> + 101 VINOK_f + 300mV
> + 110 VINOK_f + 350mV
> + 111 VINOK_f + 400mV
> + -bit [4...5]: VINLOW hyst
> + 00 100mV
> + 01 200mV
> + 10 300mV
> + 11 400mV
> + -bit 6: SW_OUT detector is disabled
> + -bit 7: SW_IN detector is enabled.
> +- st,usb-control-register:
> + -bit 3: SW_OUT current limit
> + 0: 600mA
> + 1: 1.1A
> + -bit 4: VBUS_OTG discharge is enabled
> + -bit 5: SW_OUT discharge is enabled
> + -bit 6: VBUS_OTG detection is enabled
> + -bit 7: BOOST_OVP is disabled
> +
> +
Did you mean to add a double line space here?
> +stpmic1 consists is a varied group of sub-devices:
Should be STPMIC1, no?
> +Device Description
> +------ ------------
> +st,stpmic1-onkey : On key
Please describe this a "Power on key"
> +st,stpmic1-regulators : Regulators
> +st,stpmic1-wdt : Watchdog
> +
> +each sub-device bindings is be described in associated driver
"Each sub-device binding is described in own documentation file"
> +documentation section.
Please list them using relative paths e.g:
../../bindings/watchdog/st,stm32-iwdg.txt
Or
../watchdog/st,stm32-iwdg.txt
> +Example:
> +
> +pmic: stpmic1@33 {
> + compatible = "st,stpmic1";
> + reg = <0x33>;
> + interrupts = <0 2>;
> + interrupts-extended = <&intc GIC_SPI 149 IRQ_TYPE_NONE>,
> + <&exti 55 1>;
> + st,version_status = <0x10>;
> + st,main-control-register=<0x0c>;
> + interrupt-controller;
> + #interrupt-cells = <2>;
'\n' here.
> + onkey {
> + compatible = "st,stpmic1-onkey";
> + interrupt-parent = <&pmic>;
> + interrupts = <IT_PONKEY_F 0>,<IT_PONKEY_R 1>;
> + interrupt-names = "onkey-falling", "onkey-rising";
> + st,onkey-pwroff-enabled;
> + st,onkey-long-press-seconds = <10>;
> + };
> +
> + watchdog {
> + compatible = "st,stpmic1-wdt";
> + };
> +
> + regulators {
> + compatible = "st,stpmic1-regulators";
> +
> + vdd_core: buck1 {
> + regulator-name = "vdd_core";
> + regulator-boot-on;
> + regulator-min-microvolt = <700000>;
> + regulator-max-microvolt = <1200000>;
> + };
> + vdd: buck3 {
> + regulator-name = "vdd";
> + regulator-min-microvolt = <3300000>;
> + regulator-max-microvolt = <3300000>;
> + regulator-boot-on;
> + regulator-pull-down;
> + };
> + };
> diff --git a/include/dt-bindings/mfd/st,stpmic1.h b/include/dt-bindings/mfd/st,stpmic1.h
> new file mode 100644
> index 0000000..e32ac8f
> --- /dev/null
> +++ b/include/dt-bindings/mfd/st,stpmic1.h
> @@ -0,0 +1,46 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) STMicroelectronics 2018 - All Rights Reserved
> + * Author: Philippe Peurichard <philippe.peurichard@st.com>,
> + * Pascal Paillet <p.paillet@st.com> for STMicroelectronics.
* Author(s): Philippe Peurichard <philippe.peurichard@st.com>,
* Pascal Paillet <p.paillet@st.com> for STMicroelectronics.
> + */
> +
> +#ifndef __DT_BINDINGS_STPMIC1_H__
> +#define __DT_BINDINGS_STPMIC1_H__
> +
> +/* IRQ definitions */
> +#define IT_PONKEY_F 0
> +#define IT_PONKEY_R 1
> +#define IT_WAKEUP_F 2
> +#define IT_WAKEUP_R 3
> +#define IT_VBUS_OTG_F 4
> +#define IT_VBUS_OTG_R 5
> +#define IT_SWOUT_F 6
> +#define IT_SWOUT_R 7
Please ensure these are all tab aligned for readability.
> +#define IT_CURLIM_BUCK1 8
> +#define IT_CURLIM_BUCK2 9
> +#define IT_CURLIM_BUCK3 10
> +#define IT_CURLIM_BUCK4 11
> +#define IT_OCP_OTG 12
> +#define IT_OCP_SWOUT 13
> +#define IT_OCP_BOOST 14
> +#define IT_OVP_BOOST 15
> +
> +#define IT_CURLIM_LDO1 16
> +#define IT_CURLIM_LDO2 17
> +#define IT_CURLIM_LDO3 18
> +#define IT_CURLIM_LDO4 19
> +#define IT_CURLIM_LDO5 20
> +#define IT_CURLIM_LDO6 21
> +#define IT_SHORT_SWOTG 22
> +#define IT_SHORT_SWOUT 23
> +
> +#define IT_TWARN_F 24
> +#define IT_TWARN_R 25
> +#define IT_VINLOW_F 26
> +#define IT_VINLOW_R 27
> +#define IT_SWIN_F 30
> +#define IT_SWIN_R 31
> +
> +#endif /* __DT_BINDINGS_STPMIC1_H__ */
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: [PATCH V2 2/8] mfd: stpmic1: add stpmic1 driver
From: Lee Jones @ 2018-09-23 14:41 UTC (permalink / raw)
To: Pascal PAILLET-LME
Cc: dmitry.torokhov@gmail.com, robh+dt@kernel.org,
mark.rutland@arm.com, lgirdwood@gmail.com, broonie@kernel.org,
wim@linux-watchdog.org, linux@roeck-us.net,
linux-input@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-watchdog@vger.kernel.org,
benjamin.gaignard@linaro.org, eballetbo@gmail.com
In-Reply-To: <1536325173-16617-3-git-send-email-p.paillet@st.com>
On Fri, 07 Sep 2018, Pascal PAILLET-LME wrote:
> From: pascal paillet <p.paillet@st.com>
This is odd. What is your reason for not using `git send-email`?
Also your name should really be capitalised.
Pascal Paillet
> stpmic1 is a pmic from STMicroelectronics. The stpmic1 integrates 10
"STPMIC1" and "PMIC"
> regulators and 3 switches with various capabilities.
What about the Watchdog that I saw in the bindings?
> Signed-off-by: pascal paillet <p.paillet@st.com>
> ---
> changes in v2:
> * the hardware component has been renamed from stpmu1 to stpmic1 !
> * Handle remarks from Enric
> * change headers
> * split binding description on another patch
Please use proper English grammar.
Capital letters at the start of sentences and for names of things, etc.
> On other mfd upstreamed by us (ST) we always get the remark to use
MFD
> devm_of_platform_populate and not mfd_add_devices().
> MFD maintainers could you please clarify wich API I should here, thanks ?
You can use either depending on the use-case.
Here I think the former (first - devm_of_platform_populate) is better.
> drivers/mfd/Kconfig | 13 ++
> drivers/mfd/Makefile | 1 +
> drivers/mfd/stpmic1.c | 457 ++++++++++++++++++++++++++++++++++++++++++++
> include/linux/mfd/stpmic1.h | 220 +++++++++++++++++++++
> 4 files changed, 691 insertions(+)
> create mode 100644 drivers/mfd/stpmic1.c
> create mode 100644 include/linux/mfd/stpmic1.h
>
> diff --git a/drivers/mfd/Kconfig b/drivers/mfd/Kconfig
> index b860eb5..7984803 100644
> --- a/drivers/mfd/Kconfig
> +++ b/drivers/mfd/Kconfig
> @@ -1812,6 +1812,19 @@ config MFD_STM32_TIMERS
> for PWM and IIO Timer. This driver allow to share the
> registers between the others drivers.
>
> +config MFD_STPMIC1
> + tristate "Support for STPMIC1 PMIC"
> + depends on (I2C=y && OF)
> + select REGMAP_I2C
> + select REGMAP_IRQ
> + select MFD_CORE
> + help
> + Support for STMicroelectronics STPMIC1 PMIC. Stpmic1 mfd driver is
"STPMIC MFD"
> + the core driver for stpmic1 component that mainly handles interrupts.
Same here.
> + To compile this driver as a module, choose M here: the
> + module will be called stpmic1.
> +
> menu "Multimedia Capabilities Port drivers"
> depends on ARCH_SA1100
>
> diff --git a/drivers/mfd/Makefile b/drivers/mfd/Makefile
> index e9fd20d..b194929 100644
> --- a/drivers/mfd/Makefile
> +++ b/drivers/mfd/Makefile
> @@ -220,6 +220,7 @@ obj-$(CONFIG_INTEL_SOC_PMIC_CHTDC_TI) += intel_soc_pmic_chtdc_ti.o
> obj-$(CONFIG_MFD_MT6397) += mt6397-core.o
>
> obj-$(CONFIG_MFD_ALTERA_A10SR) += altera-a10sr.o
> +obj-$(CONFIG_MFD_STPMIC1) += stpmic1.o
> obj-$(CONFIG_MFD_SUN4I_GPADC) += sun4i-gpadc.o
>
> obj-$(CONFIG_MFD_STM32_LPTIMER) += stm32-lptimer.o
> diff --git a/drivers/mfd/stpmic1.c b/drivers/mfd/stpmic1.c
> new file mode 100644
> index 0000000..ea0bff2
> --- /dev/null
> +++ b/drivers/mfd/stpmic1.c
> @@ -0,0 +1,457 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) STMicroelectronics 2018
> +// Author: Pascal Paillet <p.paillet@st.com> for STMicroelectronics.
You don't need to put "for STMicroelectronics", since you are an ST
employee. This is something we agreed to use when upstreaming ST code
with our Linaro addresses.
> +#include <linux/i2c.h>
> +#include <linux/interrupt.h>
> +#include <linux/mfd/core.h>
> +#include <linux/mfd/stpmic1.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_irq.h>
> +#include <linux/of_platform.h>
> +#include <linux/pm_wakeirq.h>
> +#include <linux/regmap.h>
'\n' here.
> +#include <dt-bindings/mfd/st,stpmic1.h>
> +
> +static bool stpmic1_reg_readable(struct device *dev, unsigned int reg);
> +static bool stpmic1_reg_writeable(struct device *dev, unsigned int reg);
> +static bool stpmic1_reg_volatile(struct device *dev, unsigned int reg);
This is a bad sign. Why you need these forward declarations?
Best to reorder the functions instead.
> +const struct regmap_config stpmic1_regmap_config = {
> + .reg_bits = 8,
> + .val_bits = 8,
> + .cache_type = REGCACHE_RBTREE,
> + .max_register = PMIC_MAX_REGISTER_ADDRESS,
> + .readable_reg = stpmic1_reg_readable,
> + .writeable_reg = stpmic1_reg_writeable,
> + .volatile_reg = stpmic1_reg_volatile,
> +};
> +
> +#define FILL_IRQS(_index) \
> + [(_index)] = { \
> + .reg_offset = ((_index) >> 3), \
> + .mask = (1 << (_index % 8)), \
> + }
If you need this, then it is likely that others need it too. Instead
of hand-rolling your own MACROs, please push it into the Regmap
subsystem.
> +static const struct regmap_irq stpmic1_irqs[] = {
> + FILL_IRQS(IT_PONKEY_F),
> + FILL_IRQS(IT_PONKEY_R),
> + FILL_IRQS(IT_WAKEUP_F),
> + FILL_IRQS(IT_WAKEUP_R),
> + FILL_IRQS(IT_VBUS_OTG_F),
> + FILL_IRQS(IT_VBUS_OTG_R),
> + FILL_IRQS(IT_SWOUT_F),
> + FILL_IRQS(IT_SWOUT_R),
> +
> + FILL_IRQS(IT_CURLIM_BUCK1),
> + FILL_IRQS(IT_CURLIM_BUCK2),
> + FILL_IRQS(IT_CURLIM_BUCK3),
> + FILL_IRQS(IT_CURLIM_BUCK4),
> + FILL_IRQS(IT_OCP_OTG),
> + FILL_IRQS(IT_OCP_SWOUT),
> + FILL_IRQS(IT_OCP_BOOST),
> + FILL_IRQS(IT_OVP_BOOST),
> +
> + FILL_IRQS(IT_CURLIM_LDO1),
> + FILL_IRQS(IT_CURLIM_LDO2),
> + FILL_IRQS(IT_CURLIM_LDO3),
> + FILL_IRQS(IT_CURLIM_LDO4),
> + FILL_IRQS(IT_CURLIM_LDO5),
> + FILL_IRQS(IT_CURLIM_LDO6),
> + FILL_IRQS(IT_SHORT_SWOTG),
> + FILL_IRQS(IT_SHORT_SWOUT),
> +
> + FILL_IRQS(IT_TWARN_F),
> + FILL_IRQS(IT_TWARN_R),
> + FILL_IRQS(IT_VINLOW_F),
> + FILL_IRQS(IT_VINLOW_R),
> + FILL_IRQS(IT_SWIN_F),
> + FILL_IRQS(IT_SWIN_R),
> +};
> +
> +static const struct regmap_irq_chip stpmic1_regmap_irq_chip = {
> + .name = "pmic_irq",
> + .status_base = INT_PENDING_R1,
> + .mask_base = INT_CLEAR_MASK_R1,
> + .unmask_base = INT_SET_MASK_R1,
> + .ack_base = INT_CLEAR_R1,
> + .num_regs = STPMIC1_PMIC_NUM_IRQ_REGS,
> + .irqs = stpmic1_irqs,
> + .num_irqs = ARRAY_SIZE(stpmic1_irqs),
> +};
> +
> +static bool stpmic1_reg_readable(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case TURN_ON_SR:
> + case TURN_OFF_SR:
> + case ICC_LDO_TURN_OFF_SR:
> + case ICC_BUCK_TURN_OFF_SR:
> + case RREQ_STATE_SR:
> + case VERSION_SR:
> + case SWOFF_PWRCTRL_CR:
> + case PADS_PULL_CR:
> + case BUCKS_PD_CR:
> + case LDO14_PD_CR:
> + case LDO56_VREF_PD_CR:
> + case VBUS_DET_VIN_CR:
> + case PKEY_TURNOFF_CR:
> + case BUCKS_MASK_RANK_CR:
> + case BUCKS_MASK_RESET_CR:
> + case LDOS_MASK_RANK_CR:
> + case LDOS_MASK_RESET_CR:
> + case WCHDG_CR:
> + case WCHDG_TIMER_CR:
> + case BUCKS_ICCTO_CR:
> + case LDOS_ICCTO_CR:
> + case BUCK1_ACTIVE_CR:
> + case BUCK2_ACTIVE_CR:
> + case BUCK3_ACTIVE_CR:
> + case BUCK4_ACTIVE_CR:
> + case VREF_DDR_ACTIVE_CR:
> + case LDO1_ACTIVE_CR:
> + case LDO2_ACTIVE_CR:
> + case LDO3_ACTIVE_CR:
> + case LDO4_ACTIVE_CR:
> + case LDO5_ACTIVE_CR:
> + case LDO6_ACTIVE_CR:
> + case BUCK1_STDBY_CR:
> + case BUCK2_STDBY_CR:
> + case BUCK3_STDBY_CR:
> + case BUCK4_STDBY_CR:
> + case VREF_DDR_STDBY_CR:
> + case LDO1_STDBY_CR:
> + case LDO2_STDBY_CR:
> + case LDO3_STDBY_CR:
> + case LDO4_STDBY_CR:
> + case LDO5_STDBY_CR:
> + case LDO6_STDBY_CR:
> + case BST_SW_CR:
> + case INT_PENDING_R1:
> + case INT_PENDING_R2:
> + case INT_PENDING_R3:
> + case INT_PENDING_R4:
> + case INT_DBG_LATCH_R1:
> + case INT_DBG_LATCH_R2:
> + case INT_DBG_LATCH_R3:
> + case INT_DBG_LATCH_R4:
> + case INT_CLEAR_R1:
> + case INT_CLEAR_R2:
> + case INT_CLEAR_R3:
> + case INT_CLEAR_R4:
> + case INT_MASK_R1:
> + case INT_MASK_R2:
> + case INT_MASK_R3:
> + case INT_MASK_R4:
> + case INT_SET_MASK_R1:
> + case INT_SET_MASK_R2:
> + case INT_SET_MASK_R3:
> + case INT_SET_MASK_R4:
> + case INT_CLEAR_MASK_R1:
> + case INT_CLEAR_MASK_R2:
> + case INT_CLEAR_MASK_R3:
> + case INT_CLEAR_MASK_R4:
> + case INT_SRC_R1:
> + case INT_SRC_R2:
> + case INT_SRC_R3:
> + case INT_SRC_R4:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static bool stpmic1_reg_writeable(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case SWOFF_PWRCTRL_CR:
> + case PADS_PULL_CR:
> + case BUCKS_PD_CR:
> + case LDO14_PD_CR:
> + case LDO56_VREF_PD_CR:
> + case VBUS_DET_VIN_CR:
> + case PKEY_TURNOFF_CR:
> + case BUCKS_MASK_RANK_CR:
> + case BUCKS_MASK_RESET_CR:
> + case LDOS_MASK_RANK_CR:
> + case LDOS_MASK_RESET_CR:
> + case WCHDG_CR:
> + case WCHDG_TIMER_CR:
> + case BUCKS_ICCTO_CR:
> + case LDOS_ICCTO_CR:
> + case BUCK1_ACTIVE_CR:
> + case BUCK2_ACTIVE_CR:
> + case BUCK3_ACTIVE_CR:
> + case BUCK4_ACTIVE_CR:
> + case VREF_DDR_ACTIVE_CR:
> + case LDO1_ACTIVE_CR:
> + case LDO2_ACTIVE_CR:
> + case LDO3_ACTIVE_CR:
> + case LDO4_ACTIVE_CR:
> + case LDO5_ACTIVE_CR:
> + case LDO6_ACTIVE_CR:
> + case BUCK1_STDBY_CR:
> + case BUCK2_STDBY_CR:
> + case BUCK3_STDBY_CR:
> + case BUCK4_STDBY_CR:
> + case VREF_DDR_STDBY_CR:
> + case LDO1_STDBY_CR:
> + case LDO2_STDBY_CR:
> + case LDO3_STDBY_CR:
> + case LDO4_STDBY_CR:
> + case LDO5_STDBY_CR:
> + case LDO6_STDBY_CR:
> + case BST_SW_CR:
> + case INT_DBG_LATCH_R1:
> + case INT_DBG_LATCH_R2:
> + case INT_DBG_LATCH_R3:
> + case INT_DBG_LATCH_R4:
> + case INT_CLEAR_R1:
> + case INT_CLEAR_R2:
> + case INT_CLEAR_R3:
> + case INT_CLEAR_R4:
> + case INT_SET_MASK_R1:
> + case INT_SET_MASK_R2:
> + case INT_SET_MASK_R3:
> + case INT_SET_MASK_R4:
> + case INT_CLEAR_MASK_R1:
> + case INT_CLEAR_MASK_R2:
> + case INT_CLEAR_MASK_R3:
> + case INT_CLEAR_MASK_R4:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static bool stpmic1_reg_volatile(struct device *dev, unsigned int reg)
> +{
> + switch (reg) {
> + case TURN_ON_SR:
> + case TURN_OFF_SR:
> + case ICC_LDO_TURN_OFF_SR:
> + case ICC_BUCK_TURN_OFF_SR:
> + case RREQ_STATE_SR:
> + case INT_PENDING_R1:
> + case INT_PENDING_R2:
> + case INT_PENDING_R3:
> + case INT_PENDING_R4:
> + case INT_SRC_R1:
> + case INT_SRC_R2:
> + case INT_SRC_R3:
> + case INT_SRC_R4:
> + case WCHDG_CR:
> + return true;
> + default:
> + return false;
> + }
> +}
> +
> +static int stpmic1_configure_from_dt(struct stpmic1_dev *pmic_dev)
> +{
> + struct device_node *np = pmic_dev->np;
> + u32 reg;
> + int ret, irq;
> +
> + irq = of_irq_get(np, 0);
> + if (irq <= 0) {
> + dev_err(pmic_dev->dev,
If you do:
struct device *dev = pmic_dev->dev
... above you can save yourself a line break.
> + "Failed to get irq config: %d\n", irq);
Failed to get the config, or the IRQ?
Also should be "IRQ".
> + return irq ?: -ENODEV;
Is 0 not a valid IRQ? If not, what does it mean in this use-case?
> + }
> + pmic_dev->irq = irq;
> +
> + irq = of_irq_get(np, 1);
Please define what '0' and '1' mean here.
> + if (irq > 0)
> + pmic_dev->irq_wake = irq;
> + else
> + pmic_dev->irq_wake = pmic_dev->irq;
> +
> + device_init_wakeup(pmic_dev->dev, true);
'\n' here.
> + ret = dev_pm_set_dedicated_wake_irq(pmic_dev->dev, pmic_dev->irq_wake);
> + if (ret)
> + dev_warn(pmic_dev->dev, "failed to set up wakeup irq");
"IRQ"
> + if (!of_property_read_u32(np, "st,main-control-register", ®)) {
> + ret = regmap_update_bits(pmic_dev->regmap,
> + SWOFF_PWRCTRL_CR,
> + PWRCTRL_POLARITY_HIGH |
> + PWRCTRL_PIN_VALID |
> + RESTART_REQUEST_ENABLED,
> + reg);
> + if (ret) {
> + dev_err(pmic_dev->dev,
> + "Failed to update main control register: %d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + if (!of_property_read_u32(np, "st,pads-pull-register", ®)) {
> + ret = regmap_update_bits(pmic_dev->regmap,
> + PADS_PULL_CR,
> + WAKEUP_DETECTOR_DISABLED |
> + PWRCTRL_PD_ACTIVE |
> + PWRCTRL_PU_ACTIVE |
> + WAKEUP_PD_ACTIVE,
> + reg);
> + if (ret) {
> + dev_err(pmic_dev->dev,
> + "Failed to update pads control register: %d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + if (!of_property_read_u32(np, "st,vin-control-register", ®)) {
> + ret = regmap_update_bits(pmic_dev->regmap,
> + VBUS_DET_VIN_CR,
> + VINLOW_CTRL_REG_MASK,
> + reg);
> + if (ret) {
> + dev_err(pmic_dev->dev,
> + "Failed to update vin control register: %d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + if (!of_property_read_u32(np, "st,usb-control-register", ®)) {
> + ret = regmap_update_bits(pmic_dev->regmap, BST_SW_CR,
> + BOOST_OVP_DISABLED |
> + VBUS_OTG_DETECTION_DISABLED |
> + SW_OUT_DISCHARGE |
> + VBUS_OTG_DISCHARGE |
> + OCP_LIMIT_HIGH,
> + reg);
> + if (ret) {
> + dev_err(pmic_dev->dev,
> + "Failed to update usb control register: %d\n",
> + ret);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
> +int stpmic1_device_init(struct stpmic1_dev *pmic_dev)
> +{
> + int ret;
> + unsigned int val;
> +
> + pmic_dev->regmap =
> + devm_regmap_init_i2c(pmic_dev->i2c, &stpmic1_regmap_config);
> +
Remove this line.
> + if (IS_ERR(pmic_dev->regmap))
> + return PTR_ERR(pmic_dev->regmap);
> +
> + ret = stpmic1_configure_from_dt(pmic_dev);
You don't need all of these separate probe/init/setup functions if you;
a) always call them and b) call them only once from a single call-site.
If you *really* want to break-up probe() just pull out the DT stuff,
but in all honesty, I wouldn't worry about it.
> + if (ret) {
> + dev_err(pmic_dev->dev,
> + "Unable to configure PMIC from Device Tree: %d\n", ret);
> + return ret;
> + }
> +
> + /* Read Version ID */
> + ret = regmap_read(pmic_dev->regmap, VERSION_SR, &val);
> + if (ret) {
> + dev_err(pmic_dev->dev, "Unable to read pmic version\n");
> + return ret;
> + }
> + dev_info(pmic_dev->dev, "PMIC Chip Version: 0x%x\n", val);
> +
> + /* Initialize PMIC IRQ Chip & IRQ domains associated */
> + ret = devm_regmap_add_irq_chip(pmic_dev->dev, pmic_dev->regmap,
> + pmic_dev->irq,
> + IRQF_ONESHOT | IRQF_SHARED,
> + 0, &stpmic1_regmap_irq_chip,
> + &pmic_dev->irq_data);
> + if (ret) {
> + dev_err(pmic_dev->dev, "IRQ Chip registration failed: %d\n",
> + ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int stpmic1_probe(struct i2c_client *i2c,
> + const struct i2c_device_id *id)
> +{
> + struct stpmic1_dev *pmic;
Remove the "_dev" and instead of 'pmic' please use 'ddata'.
Although I don't see the point in having device data at all. What do
you use it for besides passing to functions called from probe()?
> + struct device *dev = &i2c->dev;
> + int ret;
> +
> + pmic = devm_kzalloc(dev, sizeof(struct stpmic1_dev), GFP_KERNEL);
> + if (!pmic)
> + return -ENOMEM;
> +
> + pmic->np = dev->of_node;
> +
> + dev_set_drvdata(dev, pmic);
Looks like either this or the i2c_get_clientdata() call is not
correct. Have you tested suspend/resume? I suggest you do not use
i2c_get_clientdata().
> + pmic->dev = dev;
> + pmic->i2c = i2c;
> +
> + ret = stpmic1_device_init(pmic);
> + if (ret)
> + return ret;
> +
> + return devm_of_platform_populate(pmic->dev);
> +}
> +
> +static const struct i2c_device_id stpmic1_id[] = {
> + { "stpmic1", 0 },
Why 0?
> + {}
> +};
> +
> +MODULE_DEVICE_TABLE(i2c, stpmic1_id);
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int stpmic1_suspend(struct device *dev)
> +{
> + struct i2c_client *i2c = container_of(dev, struct i2c_client, dev);
Should use to_i2c_client().
> + struct stpmic1_dev *pmic_dev = i2c_get_clientdata(i2c);
How does this 'struct stpmic1_dev *' get into there?
Also, if you put it into dev->driver_data instead, it will save a few
lines.
> + if (device_may_wakeup(dev))
> + enable_irq_wake(pmic_dev->irq_wake);
> +
> + disable_irq(pmic_dev->irq);
'\n' here.
> + return 0;
> +}
> +
> +static int stpmic1_resume(struct device *dev)
> +{
> + struct i2c_client *i2c = container_of(dev, struct i2c_client, dev);
> + struct stpmic1_dev *pmic_dev = i2c_get_clientdata(i2c);
As above.
> + int ret;
> +
> + ret = regcache_sync(pmic_dev->regmap);
> + if (ret)
> + return ret;
> +
> + if (device_may_wakeup(dev))
> + disable_irq_wake(pmic_dev->irq_wake);
> +
> + enable_irq(pmic_dev->irq);
'\n' here.
> + return 0;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(stpmic1_pm, stpmic1_suspend, stpmic1_resume);
> +
> +static struct i2c_driver stpmic1_driver = {
> + .driver = {
> + .name = "stpmic1",
> + .pm = &stpmic1_pm,
> + },
Odd tabbing.
> + .probe = stpmic1_probe,
> + .id_table = stpmic1_id,
> +};
> +
> +module_i2c_driver(stpmic1_driver);
> +
> +MODULE_DESCRIPTION("STPMIC1 PMIC Driver");
> +MODULE_AUTHOR("Pascal Paillet");
This normally contains an email address.
> +MODULE_LICENSE("GPL v2");
> diff --git a/include/linux/mfd/stpmic1.h b/include/linux/mfd/stpmic1.h
> new file mode 100644
> index 0000000..024769d
> --- /dev/null
> +++ b/include/linux/mfd/stpmic1.h
> @@ -0,0 +1,220 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) STMicroelectronics 2018 - All Rights Reserved
> + * Author: Philippe Peurichard <philippe.peurichard@st.com>,
> + * Pascal Paillet <p.paillet@st.com> for STMicroelectronics.
> + */
> +
> +#ifndef __LINUX_MFD_STPMIC1_H
> +#define __LINUX_MFD_STPMIC1_H
> +
> +#define TURN_ON_SR 0x1
> +#define TURN_OFF_SR 0x2
> +#define ICC_LDO_TURN_OFF_SR 0x3
> +#define ICC_BUCK_TURN_OFF_SR 0x4
> +#define RREQ_STATE_SR 0x5
> +#define VERSION_SR 0x6
> +
> +#define SWOFF_PWRCTRL_CR 0x10
> +#define PADS_PULL_CR 0x11
> +#define BUCKS_PD_CR 0x12
> +#define LDO14_PD_CR 0x13
> +#define LDO56_VREF_PD_CR 0x14
> +#define VBUS_DET_VIN_CR 0x15
> +#define PKEY_TURNOFF_CR 0x16
> +#define BUCKS_MASK_RANK_CR 0x17
> +#define BUCKS_MASK_RESET_CR 0x18
> +#define LDOS_MASK_RANK_CR 0x19
> +#define LDOS_MASK_RESET_CR 0x1A
> +#define WCHDG_CR 0x1B
> +#define WCHDG_TIMER_CR 0x1C
> +#define BUCKS_ICCTO_CR 0x1D
> +#define LDOS_ICCTO_CR 0x1E
> +
> +#define BUCK1_ACTIVE_CR 0x20
> +#define BUCK2_ACTIVE_CR 0x21
> +#define BUCK3_ACTIVE_CR 0x22
> +#define BUCK4_ACTIVE_CR 0x23
> +#define VREF_DDR_ACTIVE_CR 0x24
> +#define LDO1_ACTIVE_CR 0x25
> +#define LDO2_ACTIVE_CR 0x26
> +#define LDO3_ACTIVE_CR 0x27
> +#define LDO4_ACTIVE_CR 0x28
> +#define LDO5_ACTIVE_CR 0x29
> +#define LDO6_ACTIVE_CR 0x2A
> +
> +#define BUCK1_STDBY_CR 0x30
> +#define BUCK2_STDBY_CR 0x31
> +#define BUCK3_STDBY_CR 0x32
> +#define BUCK4_STDBY_CR 0x33
> +#define VREF_DDR_STDBY_CR 0x34
> +#define LDO1_STDBY_CR 0x35
> +#define LDO2_STDBY_CR 0x36
> +#define LDO3_STDBY_CR 0x37
> +#define LDO4_STDBY_CR 0x38
> +#define LDO5_STDBY_CR 0x39
> +#define LDO6_STDBY_CR 0x3A
> +
> +#define BST_SW_CR 0x40
> +
> +#define INT_PENDING_R1 0x50
> +#define INT_PENDING_R2 0x51
> +#define INT_PENDING_R3 0x52
> +#define INT_PENDING_R4 0x53
> +
> +#define INT_DBG_LATCH_R1 0x60
> +#define INT_DBG_LATCH_R2 0x61
> +#define INT_DBG_LATCH_R3 0x62
> +#define INT_DBG_LATCH_R4 0x63
> +
> +#define INT_CLEAR_R1 0x70
> +#define INT_CLEAR_R2 0x71
> +#define INT_CLEAR_R3 0x72
> +#define INT_CLEAR_R4 0x73
> +
> +#define INT_MASK_R1 0x80
> +#define INT_MASK_R2 0x81
> +#define INT_MASK_R3 0x82
> +#define INT_MASK_R4 0x83
> +
> +#define INT_SET_MASK_R1 0x90
> +#define INT_SET_MASK_R2 0x91
> +#define INT_SET_MASK_R3 0x92
> +#define INT_SET_MASK_R4 0x93
> +
> +#define INT_CLEAR_MASK_R1 0xA0
> +#define INT_CLEAR_MASK_R2 0xA1
> +#define INT_CLEAR_MASK_R3 0xA2
> +#define INT_CLEAR_MASK_R4 0xA3
> +
> +#define INT_SRC_R1 0xB0
> +#define INT_SRC_R2 0xB1
> +#define INT_SRC_R3 0xB2
> +#define INT_SRC_R4 0xB3
> +
> +#define PMIC_MAX_REGISTER_ADDRESS INT_SRC_R4
> +
> +#define STPMIC1_PMIC_NUM_IRQ_REGS 4
> +
> +#define TURN_OFF_SR_ICC_EVENT 0x08
> +
> +#define LDO_VOLTAGE_MASK GENMASK(6, 2)
> +#define BUCK_VOLTAGE_MASK GENMASK(7, 2)
> +#define LDO_BUCK_VOLTAGE_SHIFT 2
> +
> +#define LDO_ENABLE_MASK BIT(0)
> +#define BUCK_ENABLE_MASK BIT(0)
> +
> +#define BUCK_HPLP_ENABLE_MASK BIT(1)
> +#define BUCK_HPLP_SHIFT 1
> +
> +#define STDBY_ENABLE_MASK BIT(0)
> +
> +#define BUCKS_PD_CR_REG_MASK GENMASK(7, 0)
> +#define BUCK_MASK_RANK_REGISTER_MASK GENMASK(3, 0)
> +#define BUCK_MASK_RESET_REGISTER_MASK GENMASK(3, 0)
> +#define LDO1234_PULL_DOWN_REGISTER_MASK GENMASK(7, 0)
> +#define LDO56_VREF_PD_CR_REG_MASK GENMASK(5, 0)
> +#define LDO_MASK_RANK_REGISTER_MASK GENMASK(5, 0)
> +#define LDO_MASK_RESET_REGISTER_MASK GENMASK(5, 0)
> +
> +#define BUCK1_PULL_DOWN_REG BUCKS_PD_CR
> +#define BUCK1_PULL_DOWN_MASK BIT(0)
> +#define BUCK2_PULL_DOWN_REG BUCKS_PD_CR
> +#define BUCK2_PULL_DOWN_MASK BIT(2)
> +#define BUCK3_PULL_DOWN_REG BUCKS_PD_CR
> +#define BUCK3_PULL_DOWN_MASK BIT(4)
> +#define BUCK4_PULL_DOWN_REG BUCKS_PD_CR
> +#define BUCK4_PULL_DOWN_MASK BIT(6)
> +
> +#define LDO1_PULL_DOWN_REG LDO14_PD_CR
> +#define LDO1_PULL_DOWN_MASK BIT(0)
> +#define LDO2_PULL_DOWN_REG LDO14_PD_CR
> +#define LDO2_PULL_DOWN_MASK BIT(2)
> +#define LDO3_PULL_DOWN_REG LDO14_PD_CR
> +#define LDO3_PULL_DOWN_MASK BIT(4)
> +#define LDO4_PULL_DOWN_REG LDO14_PD_CR
> +#define LDO4_PULL_DOWN_MASK BIT(6)
> +#define LDO5_PULL_DOWN_REG LDO56_VREF_PD_CR
> +#define LDO5_PULL_DOWN_MASK BIT(0)
> +#define LDO6_PULL_DOWN_REG LDO56_VREF_PD_CR
> +#define LDO6_PULL_DOWN_MASK BIT(2)
> +#define VREF_DDR_PULL_DOWN_REG LDO56_VREF_PD_CR
> +#define VREF_DDR_PULL_DOWN_MASK BIT(4)
> +
> +#define BUCKS_ICCTO_CR_REG_MASK GENMASK(6, 0)
> +#define LDOS_ICCTO_CR_REG_MASK GENMASK(5, 0)
> +
> +#define LDO_BYPASS_MASK BIT(7)
> +
> +/* Main PMIC Control Register
> + * SWOFF_PWRCTRL_CR
> + * Address : 0x10
> + */
> +#define ICC_EVENT_ENABLED BIT(4)
> +#define PWRCTRL_POLARITY_HIGH BIT(3)
> +#define PWRCTRL_PIN_VALID BIT(2)
> +#define RESTART_REQUEST_ENABLED BIT(1)
> +#define SOFTWARE_SWITCH_OFF_ENABLED BIT(0)
> +
> +/* Main PMIC PADS Control Register
> + * PADS_PULL_CR
> + * Address : 0x11
> + */
> +#define WAKEUP_DETECTOR_DISABLED BIT(4)
> +#define PWRCTRL_PD_ACTIVE BIT(3)
> +#define PWRCTRL_PU_ACTIVE BIT(2)
> +#define WAKEUP_PD_ACTIVE BIT(1)
> +#define PONKEY_PU_ACTIVE BIT(0)
> +
> +/* Main PMIC VINLOW Control Register
> + * VBUS_DET_VIN_CRC DMSC
> + * Address : 0x15
> + */
> +#define SWIN_DETECTOR_ENABLED BIT(7)
> +#define SWOUT_DETECTOR_ENABLED BIT(6)
> +#define VINLOW_ENABLED BIT(0)
> +#define VINLOW_CTRL_REG_MASK GENMASK(7, 0)
> +
> +/* USB Control Register
> + * Address : 0x40
> + */
> +#define BOOST_OVP_DISABLED BIT(7)
> +#define VBUS_OTG_DETECTION_DISABLED BIT(6)
> +#define SW_OUT_DISCHARGE BIT(5)
> +#define VBUS_OTG_DISCHARGE BIT(4)
> +#define OCP_LIMIT_HIGH BIT(3)
> +#define SWIN_SWOUT_ENABLED BIT(2)
> +#define USBSW_OTG_SWITCH_ENABLED BIT(1)
> +#define BOOST_ENABLED BIT(0)
> +
> +/* PKEY_TURNOFF_CR
> + * Address : 0x16
> + */
> +#define PONKEY_PWR_OFF BIT(7)
> +#define PONKEY_CC_FLAG_CLEAR BIT(6)
> +#define PONKEY_TURNOFF_TIMER_MASK GENMASK(3, 0)
> +#define PONKEY_TURNOFF_MASK GENMASK(7, 0)
> +
> +/*
> + * struct stpmic1_dev - stpmic1 master device for sub-drivers
> + * @dev: master device of the chip (can be used to access platform data)
> + * @i2c: i2c client private data for regulator
> + * @np: device DT node pointer
> + * @irq_base: base IRQ numbers
> + * @irq: generic IRQ number
> + * @irq_wake: wakeup IRQ number
> + * @regmap_irq_chip_data: irq chip data
> + */
What do you do with this?
I'm fairly sure not all of these attributes are required.
> +struct stpmic1_dev {
> + struct device *dev;
> + struct i2c_client *i2c;
> + struct regmap *regmap;
> + struct device_node *np;
> + unsigned int irq_base;
> + int irq;
> + int irq_wake;
> + struct regmap_irq_chip_data *irq_data;
> +};
> +
> +#endif /* __LINUX_MFD_STPMIC1_H */
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* Re: WARNING: kmalloc bug in input_mt_init_slots
From: Dmitry Vyukov @ 2018-09-23 16:33 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: syzbot+87829a10073277282ad1, Christoph Lameter, Pekka Enberg,
linux-input@vger.kernel.org, lkml, Henrik Rydberg, syzkaller-bugs,
Linux-MM
In-Reply-To: <CAKdAkRS7PSXv65MTnvKOewqESxt0_FtKohd86ioOuYR3R0z9dw@mail.gmail.com>
On Fri, Sep 21, 2018 at 7:52 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Fri, Sep 21, 2018 at 10:24 AM syzbot
> <syzbot+87829a10073277282ad1@syzkaller.appspotmail.com> wrote:
>>
>> Hello,
>>
>> syzbot found the following crash on:
>>
>> HEAD commit: 234b69e3e089 ocfs2: fix ocfs2 read block panic
>> git tree: upstream
>> console output: https://syzkaller.appspot.com/x/log.txt?x=131f761a400000
>> kernel config: https://syzkaller.appspot.com/x/.config?x=5fa12be50bca08d8
>> dashboard link: https://syzkaller.appspot.com/bug?extid=87829a10073277282ad1
>> compiler: gcc (GCC) 8.0.1 20180413 (experimental)
>> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=126ca61a400000
>> C reproducer: https://syzkaller.appspot.com/x/repro.c?x=119d6511400000
>>
>> IMPORTANT: if you fix the bug, please add the following tag to the commit:
>> Reported-by: syzbot+87829a10073277282ad1@syzkaller.appspotmail.com
>>
>> input: syz0 as /devices/virtual/input/input25382
>> WARNING: CPU: 0 PID: 11238 at mm/slab_common.c:1031 kmalloc_slab+0x56/0x70
>> mm/slab_common.c:1031
>> Kernel panic - not syncing: panic_on_warn set ...
>
> This is coming from:
>
> commit 6286ae97d10ea2b5cd90532163797ab217bfdbdf
> Author: Christoph Lameter <cl@linux.com>
> Date: Fri May 3 15:43:18 2013 +0000
>
> slab: Return NULL for oversized allocations
>
> The inline path seems to have changed the SLAB behavior for very large
> kmalloc allocations with commit e3366016 ("slab: Use common
> kmalloc_index/kmalloc_size functions"). This patch restores the old
> behavior but also adds diagnostics so that we can figure where in the
> code these large allocations occur.
>
> Reported-and-tested-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
> Signed-off-by: Christoph Lameter <cl@linux.com>
> Link: http://lkml.kernel.org/r/201305040348.CIF81716.OStQOHFJMFLOVF@I-love.SAKURA.ne.jp
> [ penberg@kernel.org: use WARN_ON_ONCE ]
> Signed-off-by: Pekka Enberg <penberg@kernel.org>
>
> You'll have to convince Cristoph that WARN_ON_ONCE() there is evil and
> has to be eradicated so that KASAN can run (but then we'd not know
> easily that some allocation failed because it was too big and never
> had a chance of succeeding vs. ordinary memory failure).
>
> Can I recommend that maybe you introduce infrastructure for
> panic_on_warn to ignore certain "well known" warnings?
Hi Christoph,
What was the motivation behind that WARNING about large allocations in
kmalloc? Why do we want to know about them? Is the general policy that
kmalloc calls with potentially large size requests need to use NOWARN?
If this WARNING still considered useful? Or we should change it to
pr_err?
^ permalink raw reply
* Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards
From: Heikki Krogerus @ 2018-09-24 7:29 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linus Walleij, Rafael J . Wysocki, linux-input, linux-gpio,
linux-kernel, Andy Shevchenko
In-Reply-To: <20180921233336.GB44099@dtor-ws>
On Fri, Sep 21, 2018 at 04:33:36PM -0700, Dmitry Torokhov wrote:
> On Thu, Sep 20, 2018 at 01:16:48PM +0300, Heikki Krogerus wrote:
> > On Wed, Sep 19, 2018 at 10:13:26AM -0700, Dmitry Torokhov wrote:
> > > > > diff --git a/drivers/base/pset_property.c b/drivers/base/pset_property.c
> > > > > index 08ecc13080ae..63f2377aefe8 100644
> > > > > --- a/drivers/base/pset_property.c
> > > > > +++ b/drivers/base/pset_property.c
> > > > > @@ -18,6 +18,11 @@ struct property_set {
> > > > > struct device *dev;
> > > > > struct fwnode_handle fwnode;
> > > > > const struct property_entry *properties;
> > > > > +
> > > > > + struct property_set *parent;
> > > > > + /* Entry in parent->children list */
> > > > > + struct list_head child_node;
> > > > > + struct list_head children;
> > > >
> > > > Add
> > > >
> > > > const char *name;
> > > >
> > > > and you can implement also pset_get_named_child_node().
> > >
> > > Or
> > > char name[];
> > >
> > > to avoid separate allocation.
> >
> > Let's not do that, especially if you are planning on exporting this
> > structure.
>
> Can you please elaborate why? Not using pointer saves us 4/8 bytes +
> however much memory we need for bookkeeping for the extra chunk. Given
> that majority of pset nodes are unnamed this seems wasteful.
>
> > If the name is coming from .rodata, there is no need to
> > allocate anything for the name. Check kstrdup_const().
>
> The data is most likely coming as __initconst so we do need to copy it.
OK, I did not consider that. Yes, it makes sense to always copy.
Thanks,
--
heikki
^ permalink raw reply
* Re: [PATCH 0/9] HID: intel ISH: Cleanup patches
From: Jiri Kosina @ 2018-09-24 9:21 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <20180911234421.10691-1-srinivas.pandruvada@linux.intel.com>
On Tue, 11 Sep 2018, Srinivas Pandruvada wrote:
> This series is a cleanup series only and help to abstract client API.
>
> There are no functional changes.
>
> Even Xu (6):
> hid: intel-ish-hid: ishtp: add helper function for driver data get/set
> hid: intel-ish-hid: use helper function for private driver data
> set/get
> hid: intel-ish-hid: ishtp: add helper functions for client buffer
> operation
> hid: intel-ish-hid: use helper function to access client buffer
> hid: intel-ish-hid: ishtp: add helper function for client search
> hid: intel-ish-hid: use helper function to search client id
>
> Hong Liu (2):
> HID: intel-ish-hid: use resource-managed api
> HID: intel-ish-hid: using list_head for ipc write queue
>
> Srinivas Pandruvada (1):
> HID: intel_ish-hid: Enhance API to get ring buffer sizes
Applied to for-4.20/ish.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] hyper-v: Fix wakeup from suspend-to-idle
From: Jiri Kosina @ 2018-09-24 9:24 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: linux-pm, Rafael J. Wysocki, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Dmitry Torokhov, linux-input, linux-kernel
In-Reply-To: <20180912161101.2634-1-vkuznets@redhat.com>
On Wed, 12 Sep 2018, Vitaly Kuznetsov wrote:
> It makes little sense but still possible to put Hyper-V guests into
> suspend-to-idle state. To wake them up two wakeup sources were registered
> in the past: hyperv-keyboard and hid-hyperv. However, since
> commit eed4d47efe95 ("ACPI / sleep: Ignore spurious SCI wakeups from
> suspend-to-idle") pm_wakeup_event() from these devices is ignored. Switch
> to pm_wakeup_hard_event() API as these devices are actually the only
> possible way to wakeup Hyper-V guests.
>
> Fixes: eed4d47efe95 (ACPI / sleep: Ignore spurious SCI wakeups from suspend-to-idle)
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> drivers/hid/hid-hyperv.c | 2 +-
Acked-by: Jiri Kosina <jkosina@suse.cz>
for the above. I guess this'd better go through ACPI tree?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH V2] hid: hid-core: Fix a sleep-in-atomic-context bug in __hid_request()
From: Jiri Kosina @ 2018-09-24 9:26 UTC (permalink / raw)
To: Jia-Ju Bai; +Cc: benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <20180913033432.16336-1-baijiaju1990@gmail.com>
On Thu, 13 Sep 2018, Jia-Ju Bai wrote:
> hid_alloc_report_buf() has to be called with GFP_ATOMIC in
> __hid_request(), because there are the following callchains
> leading to __hid_request() being an atomic context:
>
> picolcd_send_and_wait (acquire a spinlock)
> hid_hw_request
> __hid_request
> hid_alloc_report_buf(GFP_KERNEL)
>
> picolcd_reset (acquire a spinlock)
> hid_hw_request
> __hid_request
> hid_alloc_report_buf(GFP_KERNEL)
>
> lg4ff_play (acquire a spinlock)
> hid_hw_request
> __hid_request
> hid_alloc_report_buf(GFP_KERNEL)
>
> lg4ff_set_autocenter_ffex (acquire a spinlock)
> hid_hw_request
> __hid_request
> hid_alloc_report_buf(GFP_KERNEL)
Hm, so it's always drivers calling out into core in atomic context. So
either we take this, and put our bets on being able to allocate the buffer
without sleeping, or actually fix the few drivers (it's just lg4ff and
picolcd at the end of the day) not to do that, and explicitly anotate
__hid_request() with might_sleep().
Hmm?
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH v2] HID: logitech: fix a used uninitialized GCC warning
From: Jiri Kosina @ 2018-09-24 9:31 UTC (permalink / raw)
To: zhong jiang; +Cc: benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <1536824470-36659-1-git-send-email-zhongjiang@huawei.com>
On Thu, 13 Sep 2018, zhong jiang wrote:
> Fix the following compile warning:
>
> drivers/hid/hid-logitech-hidpp.c: In function 'hi_res_scroll_enable':
> drivers/hid/hid-logitech-hidpp.c:2714:54: warning: 'multiplier' may be used uninitialized in this function [-Wmaybe-uninitialized]
> hidpp->vertical_wheel_counter.resolution_multiplier = multiplier;
>
> Signed-off-by: zhong jiang <zhongjiang@huawei.com>
> ---
> v1->v2:
> According to Benjamin's suggestion, To initialize the value
> and remove the duplicated assignement.
Applied to for-4.20/logitech-highres. Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH] HID: intel-ish-hid: Enable Ice Lake mobile
From: Jiri Kosina @ 2018-09-24 9:46 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: benjamin.tissoires, linux-input, linux-kernel
In-Reply-To: <20180911234543.10847-1-srinivas.pandruvada@linux.intel.com>
On Tue, 11 Sep 2018, Srinivas Pandruvada wrote:
> Added PCI ID for Ice Lake mobile platform.
>
> Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
> ---
> drivers/hid/intel-ish-hid/ipc/hw-ish.h | 1 +
> drivers/hid/intel-ish-hid/ipc/pci-ish.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/drivers/hid/intel-ish-hid/ipc/hw-ish.h b/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> index 97869b7410eb..1b4e93e19e6b 100644
> --- a/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> +++ b/drivers/hid/intel-ish-hid/ipc/hw-ish.h
> @@ -29,6 +29,7 @@
> #define CNL_Ax_DEVICE_ID 0x9DFC
> #define GLK_Ax_DEVICE_ID 0x31A2
> #define CNL_H_DEVICE_ID 0xA37C
> +#define ICL_MOBILE_DEVICE_ID 0x34FC
>
> #define REVISION_ID_CHT_A0 0x6
> #define REVISION_ID_CHT_Ax_SI 0x0
> diff --git a/drivers/hid/intel-ish-hid/ipc/pci-ish.c b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> index 09d085946db3..4d78f85021a5 100644
> --- a/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> +++ b/drivers/hid/intel-ish-hid/ipc/pci-ish.c
> @@ -38,6 +38,7 @@ static const struct pci_device_id ish_pci_tbl[] = {
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, CNL_Ax_DEVICE_ID)},
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, GLK_Ax_DEVICE_ID)},
> {PCI_DEVICE(PCI_VENDOR_ID_INTEL, CNL_H_DEVICE_ID)},
> + {PCI_DEVICE(PCI_VENDOR_ID_INTEL, ICL_MOBILE_DEVICE_ID)},
> {0, }
Applied to for-4.19/fixes.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards
From: Heikki Krogerus @ 2018-09-24 10:20 UTC (permalink / raw)
To: Linus Walleij
Cc: Dmitry Torokhov, Rafael J. Wysocki, Linux Input,
open list:GPIO SUBSYSTEM, linux-kernel@vger.kernel.org,
Andy Shevchenko
In-Reply-To: <CACRpkdZtM4G=4r3m6vLTms9MJFkdiOdPFucebO-Z0Kh-3ws3+g@mail.gmail.com>
Hi Linus,
On Fri, Sep 21, 2018 at 08:36:53AM -0700, Linus Walleij wrote:
> On Thu, Sep 20, 2018 at 6:53 AM Heikki Krogerus
> <heikki.krogerus@linux.intel.com> wrote:
>
> > The child nodes will change the purpose of the build-in property
> > support. Originally the goal was just to support adding of build-in
> > device properties to real firmware nodes, but things have changed
> > quite a bit from that. These child nodes are purely tied to the
> > build-in device property support, so we should be talking about adding
> > pset type child nodes to pset type parent nodes in the API:
> > fwnode_pset_add_child_node(), or something like that.
> >
> > I'm sorry to be a bit of pain in the butt with this. The build-in
> > property support is a hack, it always was. A useful hack, but hack
> > nevertheless. That means we need to be extra careful when expanding
> > it, like here.
>
> I dunno how we got to here, what I tried to solve and Dmitry tries
> to make more general is converting old boardfiles to use fwnode, because
> I initially tried to just support gpio descriptors from board files.
I understand that, but what I'm trying to say is that the solution for
the child nodes is not generic enough. I'll try to explain below.
> If boardfiles is what you mean with "build-in property support" I don't
> know, it predates both device tree and ACPI and any other hardware
> descriptions on the ARM architecture, from the perspective of these
> systems things are fine and the hardware description languages
> are the novelty...
Rafael talked about "build-in properties" at one point, and I just
started using that expression after that, but it's probable not the
best term to describe this thing.
But please note that we are not talking about only static information
with these property sets. They can be populated dynamically as well,
so this kind of extensions must consider and support that. On top of
board files we need to consider things like glue and probing drivers
and even buses in some cases.
> But I guess you know because you worked with OMAP :)
>
> So there is something I don't understand here.
Sorry for the poor choice of words on my behalf. I guess I'm not
explaining my point properly. Let me try again.
So I'm seeing a case that this solution does not seem to be able to
support, but ultimately it will need to do so: with USB ports we are
going to need to be able to assign a device to the child nodes that
represent those ports.
To be honest, normally I would not care about that, and I would simply
wait for this to go into mainline, and then propose a modification to
it so it can also support that other case.
However, this time I feel that we should not do that. The solution for
the child nodes should be implemented so that it can support all the
known cases from the beginning. The reason for that is because I fear
that we'll end up having a pile of ad-hoc style solutions, each
solving an individual problem, and a whole lot of duplicated stuff for
these property sets. In the end we will have spaghetti with meatballs
that nobody is able fully understand nor handle. And I really see a
strong possibility for that to happen here.
Thanks,
--
heikki
^ permalink raw reply
* Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards
From: Heikki Krogerus @ 2018-09-24 13:20 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Linus Walleij, Rafael J . Wysocki, linux-input, linux-gpio,
linux-kernel, Andy Shevchenko
In-Reply-To: <20180921233119.GA44099@dtor-ws>
Hi Dmitry,
On Fri, Sep 21, 2018 at 04:31:19PM -0700, Dmitry Torokhov wrote:
> > > + if (!parent_pset)
> > > + return ERR_PTR(-EINVAL);
> > > +
> > > + p = pset_create_set(properties);
> > > + if (IS_ERR(p))
> > > + return ERR_CAST(p);
> > > +
> > > + p->dev = dev;
> >
> > That looks wrong.
> >
> > I'm guessing the assumption here is that the child nodes will never be
> > assigned to their own devices, but you can't do that. It will limit
> > the use of the child nodes to a very small number of cases, possibly
> > only to gpios.
>
> If I need to assign a node to a device I'll use device_add_properties()
> API. device_add_child_properties() is for nodes living "below" the
> device.
device_add_properties() is not available to us before we have the
actual struct device meant for the properties. If the child device is
populated outside of the "boardfiles" then we have to be able to link
it to the child node afterwards.
But I took a closer look at the code and realized that you are not
using that p->dev with the child nodes for anything, so I may be wrong
about this. You are not actually linking the child nodes to that
device here.
> All nodes (the primary/secondary and children) would point to the owning
> device, just for convenience.
Since you don't use that for anything, then drop that line. It's only
confusing.
And besides, since we will have separate child devices for those child
nodes, there is a small change that somebody needs to call
device_remove_properties(child_dev). At least then that operation
becomes safe, as it's a nop with the child pset nodes.
> > I think that has to be fixed. It should not be a big deal. Just expect
> > the child nodes to be removed separately, and add ref counting to the
> > struct property_set handling.
>
> Why do we need to remove them separately and what do we need refcounting
> for?
If you could remove the nodes independently, then obviously the parent
can not be removed before the children. The ref count would be there
to prevent that.
Though my original comment is not valid, I still think that the child
nodes should be created and destroyed separately. Actually, I don't
think we should be talking about child nodes in the API at all. Check
below..
> > > + p->parent = parent_pset;
> > > + list_add_tail(&p->child_node, &parent_pset->children);
> > > +
> > > + return &p->fwnode;
> > > +}
> > > +EXPORT_SYMBOL_GPL(device_add_child_properties);
> >
> > The child nodes will change the purpose of the build-in property
> > support. Originally the goal was just to support adding of build-in
> > device properties to real firmware nodes, but things have changed
> > quite a bit from that. These child nodes are purely tied to the
> > build-in device property support, so we should be talking about adding
> > pset type child nodes to pset type parent nodes in the API:
> > fwnode_pset_add_child_node(), or something like that.
>
> OK, I can change device_add_child_properties() to
> fwnode_pset_add_child_node() if Rafael would prefer this name.
So not even that. We should have API like this:
struct fwnode_handle *
fwnode_pset_create(struct fwnode_handle *parent, const char *name,
const struct property_entry *props);
void fwnode_pset_destroy(struct fwnode_handle *fwnode);
int device_add_properties(struct device *dev,
const struct fwnode_handle *pset_node);
void device_remove_properties(struct device *dev);
So basically we separate creation of the nodes from linking them to the
devices completely. That would give this API much better structure. It
would scale better, for example, it would then be possible to add
child nodes from different locations if needed.
Thanks,
--
heikki
^ permalink raw reply
* Re: WARNING: kmalloc bug in input_mt_init_slots
From: Christopher Lameter @ 2018-09-24 15:08 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Dmitry Torokhov, syzbot+87829a10073277282ad1, Pekka Enberg,
linux-input@vger.kernel.org, lkml, Henrik Rydberg, syzkaller-bugs,
Linux-MM
In-Reply-To: <CACT4Y+YOb6M=xuPG64PAvd=0bcteicGtwQO60CevN_V67SJ=MQ@mail.gmail.com>
On Sun, 23 Sep 2018, Dmitry Vyukov wrote:
> What was the motivation behind that WARNING about large allocations in
> kmalloc? Why do we want to know about them? Is the general policy that
> kmalloc calls with potentially large size requests need to use NOWARN?
> If this WARNING still considered useful? Or we should change it to
> pr_err?
In general large allocs should be satisfied by the page allocator. The
slab allocators are used for allocating and managing small objects. The
page allocator has mechanisms to deal with large objects (compound pages,
multiple page sized allocs etc).
^ permalink raw reply
* Re: WARNING: kmalloc bug in input_mt_init_slots
From: Dmitry Vyukov @ 2018-09-24 15:18 UTC (permalink / raw)
To: Christopher Lameter
Cc: Dmitry Torokhov, syzbot+87829a10073277282ad1, Pekka Enberg,
linux-input@vger.kernel.org, lkml, Henrik Rydberg, syzkaller-bugs,
Linux-MM
In-Reply-To: <010001660c1fafb2-6d0dc7e1-d898-4589-874c-1be1af94e22d-000000@email.amazonses.com>
On Mon, Sep 24, 2018 at 5:08 PM, Christopher Lameter <cl@linux.com> wrote:
> On Sun, 23 Sep 2018, Dmitry Vyukov wrote:
>
>> What was the motivation behind that WARNING about large allocations in
>> kmalloc? Why do we want to know about them? Is the general policy that
>> kmalloc calls with potentially large size requests need to use NOWARN?
>> If this WARNING still considered useful? Or we should change it to
>> pr_err?
>
> In general large allocs should be satisfied by the page allocator. The
> slab allocators are used for allocating and managing small objects. The
> page allocator has mechanisms to deal with large objects (compound pages,
> multiple page sized allocs etc).
I am asking more about the status of this warning. If it fires in
input_mt_init_slots(), does it mean that input_mt_init_slots() needs
to be fixed? If not, then we need to change this warning to something
else.
^ permalink raw reply
* Re: WARNING: kmalloc bug in input_mt_init_slots
From: Christopher Lameter @ 2018-09-24 15:55 UTC (permalink / raw)
To: Dmitry Vyukov
Cc: Dmitry Torokhov, syzbot+87829a10073277282ad1, Pekka Enberg,
linux-input@vger.kernel.org, lkml, Henrik Rydberg, syzkaller-bugs,
Linux-MM
In-Reply-To: <CACT4Y+ayX8vzd2JPrLeFhf3K_Quf4x6SDtmtkNJuwNLyOh67tQ@mail.gmail.com>
On Mon, 24 Sep 2018, Dmitry Vyukov wrote:
> On Mon, Sep 24, 2018 at 5:08 PM, Christopher Lameter <cl@linux.com> wrote:
> > On Sun, 23 Sep 2018, Dmitry Vyukov wrote:
> >
> >> What was the motivation behind that WARNING about large allocations in
> >> kmalloc? Why do we want to know about them? Is the general policy that
> >> kmalloc calls with potentially large size requests need to use NOWARN?
> >> If this WARNING still considered useful? Or we should change it to
> >> pr_err?
> >
> > In general large allocs should be satisfied by the page allocator. The
> > slab allocators are used for allocating and managing small objects. The
> > page allocator has mechanisms to deal with large objects (compound pages,
> > multiple page sized allocs etc).
>
> I am asking more about the status of this warning. If it fires in
> input_mt_init_slots(), does it mean that input_mt_init_slots() needs
> to be fixed? If not, then we need to change this warning to something
> else.
Hmmm.. kmalloc falls back to the page allocator already?
See
static __always_inline void *kmalloc(size_t size, gfp_t flags)
{
if (__builtin_constant_p(size)) {
if (size > KMALLOC_MAX_CACHE_SIZE)
return kmalloc_large(size, flags);
Note that this uses KMALLOC_MAX_CACHE_SIZE which should be smaller than
KMALLOC_MAX_SIZE.
How large is the allocation? AFACIT nRequests larger than KMALLOC_MAX_SIZE
are larger than the maximum allowed by the page allocator. Thus the warning
and the NULL return.
^ permalink raw reply
* Re: WARNING: kmalloc bug in input_mt_init_slots
From: Dmitry Torokhov @ 2018-09-24 18:41 UTC (permalink / raw)
To: Christopher Lameter
Cc: Dmitry Vyukov, syzbot+87829a10073277282ad1, Pekka Enberg,
linux-input@vger.kernel.org, lkml, Henrik Rydberg, syzkaller-bugs,
Linux-MM
In-Reply-To: <010001660c4a8bbe-91200766-00df-48bd-bc60-a03da2ccdb7d-000000@email.amazonses.com>
On Mon, Sep 24, 2018 at 03:55:04PM +0000, Christopher Lameter wrote:
> On Mon, 24 Sep 2018, Dmitry Vyukov wrote:
>
> > On Mon, Sep 24, 2018 at 5:08 PM, Christopher Lameter <cl@linux.com> wrote:
> > > On Sun, 23 Sep 2018, Dmitry Vyukov wrote:
> > >
> > >> What was the motivation behind that WARNING about large allocations in
> > >> kmalloc? Why do we want to know about them? Is the general policy that
> > >> kmalloc calls with potentially large size requests need to use NOWARN?
> > >> If this WARNING still considered useful? Or we should change it to
> > >> pr_err?
> > >
> > > In general large allocs should be satisfied by the page allocator. The
> > > slab allocators are used for allocating and managing small objects. The
> > > page allocator has mechanisms to deal with large objects (compound pages,
> > > multiple page sized allocs etc).
> >
> > I am asking more about the status of this warning. If it fires in
> > input_mt_init_slots(), does it mean that input_mt_init_slots() needs
> > to be fixed? If not, then we need to change this warning to something
> > else.
>
> Hmmm.. kmalloc falls back to the page allocator already?
>
> See
>
> static __always_inline void *kmalloc(size_t size, gfp_t flags)
> {
> if (__builtin_constant_p(size)) {
It would not be a constant here though.
> if (size > KMALLOC_MAX_CACHE_SIZE)
> return kmalloc_large(size, flags);
>
>
> Note that this uses KMALLOC_MAX_CACHE_SIZE which should be smaller than
> KMALLOC_MAX_SIZE.
>
>
> How large is the allocation? AFACIT nRequests larger than KMALLOC_MAX_SIZE
> are larger than the maximum allowed by the page allocator. Thus the warning
> and the NULL return.
The size in this particular case is being derived from a value passed
from userspace. Input core does not care about any limits on size of
memory kmalloc() can support and is perfectly happy with getting NULL
and telling userspace to go away with their silly requests by returning
-ENOMEM.
For the record: I definitely do not want to pre-sanitize size neither in
uinput nor in input core.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [RFC/PATCH 2/5] device property: introduce notion of subnodes for legacy boards
From: Dmitry Torokhov @ 2018-09-24 18:45 UTC (permalink / raw)
To: Heikki Krogerus
Cc: Linus Walleij, Rafael J . Wysocki, linux-input, linux-gpio,
linux-kernel, Andy Shevchenko
In-Reply-To: <20180924132050.GK11965@kuha.fi.intel.com>
On Mon, Sep 24, 2018 at 04:20:50PM +0300, Heikki Krogerus wrote:
> Hi Dmitry,
>
> On Fri, Sep 21, 2018 at 04:31:19PM -0700, Dmitry Torokhov wrote:
> > > > + if (!parent_pset)
> > > > + return ERR_PTR(-EINVAL);
> > > > +
> > > > + p = pset_create_set(properties);
> > > > + if (IS_ERR(p))
> > > > + return ERR_CAST(p);
> > > > +
> > > > + p->dev = dev;
> > >
> > > That looks wrong.
> > >
> > > I'm guessing the assumption here is that the child nodes will never be
> > > assigned to their own devices, but you can't do that. It will limit
> > > the use of the child nodes to a very small number of cases, possibly
> > > only to gpios.
> >
> > If I need to assign a node to a device I'll use device_add_properties()
> > API. device_add_child_properties() is for nodes living "below" the
> > device.
>
> device_add_properties() is not available to us before we have the
> actual struct device meant for the properties. If the child device is
> populated outside of the "boardfiles" then we have to be able to link
> it to the child node afterwards.
I think we are talking about totally different use cases and that is why
we are having hard time coming to a mutually agreeable solution. Could
you please describe in more detail what you would like to achieve,
and preferably show how it is described now with DT and/or ACPI, so that
I have a better frame of reference.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH v2 05/17] compat_ioctl: move more drivers to generic_compat_ioctl_ptrarg
From: Arnd Bergmann @ 2018-09-24 20:18 UTC (permalink / raw)
To: Jason Gunthorpe
Cc: linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
linux-iio-u79uwXL29TY76Z2rM5mHXA, linux-pci,
linux-remoteproc-u79uwXL29TY76Z2rM5mHXA,
linux-nvme-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r, Platform Driver,
sparclinux, driverdevel, linux-scsi,
linux-nvdimm-hn68Rpc1hR1g9hUCZPvPmw, linux-rdma,
qat-linux-ral2JQCrhuEAvxtiuMwx3w,
amd-gfx-PD4FTy7X32lNgt0PjOBp9y5qC8QIuHrW,
open list:HID CORE LAYER, Darren Hart, Linux Media Mailing List,
linaro-mm-sig-cunTk1MwBs8s++Sfvej+rw, dri-devel, ceph-devel,
gregkh, USB list, linux-wireless
In-Reply-To: <20180918175952.GJ11367-uk2M96/98Pc@public.gmane.org>
On Tue, Sep 18, 2018 at 7:59 PM Jason Gunthorpe <jgg-uk2M96/98Pc@public.gmane.org> wrote:
>
> On Tue, Sep 18, 2018 at 10:51:08AM -0700, Darren Hart wrote:
> > On Fri, Sep 14, 2018 at 09:57:48PM +0100, Al Viro wrote:
> > > On Fri, Sep 14, 2018 at 01:35:06PM -0700, Darren Hart wrote:
> > >
> > > > Acked-by: Darren Hart (VMware) <dvhart-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org>
> > > >
> > > > As for a longer term solution, would it be possible to init fops in such
> > > > a way that the compat_ioctl call defaults to generic_compat_ioctl_ptrarg
> > > > so we don't have to duplicate this boilerplate for every ioctl fops
> > > > structure?
> > >
> > > Bad idea, that... Because several years down the road somebody will add
> > > an ioctl that takes an unsigned int for argument. Without so much as looking
> > > at your magical mystery macro being used to initialize file_operations.
> >
> > Fair, being explicit in the declaration as it is currently may be
> > preferable then.
>
> It would be much cleaner and safer if you could arrange things to add
> something like this to struct file_operations:
>
> long (*ptr_ioctl) (struct file *, unsigned int, void __user *);
>
> Where the core code automatically converts the unsigned long to the
> void __user * as appropriate.
>
> Then it just works right always and the compiler will help address
> Al's concern down the road.
I think if we wanted to do this with a new file operation, the best
way would be to do the copy_from_user()/copy_to_user() in the caller
as well.
We already do this inside of some subsystems, notably drivers/media/,
and it simplifies the implementation of the ioctl handler function
significantly. We obviously cannot do this in general, both because of
traditional drivers that have 16-bit command codes (drivers/tty and others)
and also because of drivers that by accident defined the commands
incorrectly and use the wrong type or the wrong direction in the
definition.
Arnd
^ permalink raw reply
* Re: [PATCH v2 05/17] compat_ioctl: move more drivers to generic_compat_ioctl_ptrarg
From: Jason Gunthorpe @ 2018-09-24 20:35 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Darren Hart, Al Viro, Linux FS-devel Mailing List, gregkh,
David Miller, driverdevel, Linux Kernel Mailing List, qat-linux,
open list:HARDWARE RANDOM NUMBER GENERATOR CORE,
Linux Media Mailing List, dri-devel, linaro-mm-sig, amd-gfx,
open list:HID CORE LAYER, linux-iio, linux-rdma, linux-nvdimm,
linux-nvme, linux-pci
In-Reply-To: <CAK8P3a17GY89in7PeLk1F2T-0Xq=sCrwwntM+Y4BCpXheUC+qQ@mail.gmail.com>
On Mon, Sep 24, 2018 at 10:18:52PM +0200, Arnd Bergmann wrote:
> On Tue, Sep 18, 2018 at 7:59 PM Jason Gunthorpe <jgg@ziepe.ca> wrote:
> >
> > On Tue, Sep 18, 2018 at 10:51:08AM -0700, Darren Hart wrote:
> > > On Fri, Sep 14, 2018 at 09:57:48PM +0100, Al Viro wrote:
> > > > On Fri, Sep 14, 2018 at 01:35:06PM -0700, Darren Hart wrote:
> > > >
> > > > > Acked-by: Darren Hart (VMware) <dvhart@infradead.org>
> > > > >
> > > > > As for a longer term solution, would it be possible to init fops in such
> > > > > a way that the compat_ioctl call defaults to generic_compat_ioctl_ptrarg
> > > > > so we don't have to duplicate this boilerplate for every ioctl fops
> > > > > structure?
> > > >
> > > > Bad idea, that... Because several years down the road somebody will add
> > > > an ioctl that takes an unsigned int for argument. Without so much as looking
> > > > at your magical mystery macro being used to initialize file_operations.
> > >
> > > Fair, being explicit in the declaration as it is currently may be
> > > preferable then.
> >
> > It would be much cleaner and safer if you could arrange things to add
> > something like this to struct file_operations:
> >
> > long (*ptr_ioctl) (struct file *, unsigned int, void __user *);
> >
> > Where the core code automatically converts the unsigned long to the
> > void __user * as appropriate.
> >
> > Then it just works right always and the compiler will help address
> > Al's concern down the road.
>
> I think if we wanted to do this with a new file operation, the best
> way would be to do the copy_from_user()/copy_to_user() in the caller
> as well.
>
> We already do this inside of some subsystems, notably drivers/media/,
> and it simplifies the implementation of the ioctl handler function
> significantly. We obviously cannot do this in general, both because of
> traditional drivers that have 16-bit command codes (drivers/tty and others)
> and also because of drivers that by accident defined the commands
> incorrectly and use the wrong type or the wrong direction in the
> definition.
That could work well, but the first idea could be done globally and
mechanically, while this would require very careful per-driver
investigation.
Particularly if the core code has worse performance.. ie due to
kmalloc calls or something.
I think it would make more sense to start by having the core do the
case to __user and then add another entry point to have the core do
the copy_from_user, and so on.
Jason
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox