* Re: regression fixes sitting in subsystem git trees for a week or longer
From: Linux regression tracking (Thorsten Leemhuis) @ 2024-04-25 9:33 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Linus Torvalds, Jiri Kosina, Douglas Anderson, Hans de Goede,
linux-input, linux-kernel, Kenny Levinsen, Benjamin Tissoires,
Linux regressions mailing list
In-Reply-To: <qcd5klmhyx23rowpbm4egshm6hemhh4stq7r6soblnuul55524@yyktdlowepw7>
On 25.04.24 10:44, Benjamin Tissoires wrote:
> On Apr 25 2024, Thorsten Leemhuis wrote:
>> On 24.04.24 20:53, Linus Torvalds wrote:
>>> On Wed, 24 Apr 2024 at 09:56, Thorsten Leemhuis
>>> <regressions@leemhuis.info> wrote:
>> [...]
>> And the arch linux wiki even documents a workaround:
>> https://wiki.archlinux.org/title/Lenovo_ThinkPad_Z16_Gen_2#Initialization_failure
>>
>> Those are just the reports and discussions I found. And you know how
>> it is: many people that struggle will never report a problem.
>
> short FYI, (I've Cc-ed you on the PR), but I just sent the PR for HID,
> which includes this fix.
Great, many thx. Saw it right after sending my mail... :-/
>> Is cherry picking from -next as easy for you? Maintainers sometimes
>> improve small details when merging a fix, so it might be better to
>> take fixes from there instead of pulling them from lore.
>
> Maybe one suggestion that might help to reduce these kind of situations
> in the future: can you configure your bot to notify the maintainers
> after a couple of days that the patch has been merged that it would be
> nice if they could send the PR to Linus?
Yes, that is an idea in the long run, but I'm not sure if it's wise now
or later. People easily get annoyed by these mails (which I totally
understand!) and then will start hating the bot or regression tracking
in general. That's why I'm really careful here.
There are also the subsystems that regularly flush their fixes shortly
before a new -rc, so they likely never want to see such reminders. And
sending them right after a new -rc is better than nothing, but not ideal
either.
IOW: it's complicated. :-/
> In this case I bet Jiri forgot to send it because he was overloaded and
> so was I.
Understood and no worries. But this became a good opportunity to raise
the general problem, as that is something that bugs me. Sorry. Hope you
don't mind to much that I used that chance.
> So a friendly reminder could make things go faster.
I'll already did this occasionally manually, but that of course does not
scale. Sometimes I wonder if it would be more efficient for nearly all
of us if subsystems just flushed their -fixes branch shortly before each
new -rc, as Linus apparently is not bothered by PRs that contain just a
change or two. But that of course creates work for each of the subsystem
maintainers, unless they creates scripts to handle that work nearly for
free (it seems to me the x86 folks have something like that).
Of course that would mean...
> And maybe, before sending the reminder, if you could also check that the
> target branch hasn't been touched in 2 days that would prevent annoyances
> when we just added a commit and want to let it stay in for-next for 24h
> before sending the full branch.
...that nothing big or slightly dangerous should be merged to -fixes
branches on Fridays.
>> P.S: Wondering if I should team up with the kernel package maintainers
>> of Arch Linux, Fedora, and openSUSE and start a git tree based on the
>> latest stable tree with additional fixes and reverts for regressions
>> not yet fixed upstream...[1] But that feels kinda wrong: it IMHO
>> would be better to resolve those problems quickly in the proper
>> upstream trees.
>
> I would also say that this is wrong. Unless all regressions go through
> your tree and you then send PR to Linus, [...]
Ohh, sorry, I was not clear here, as that would be totally wrong --
fixes definitely should go through the subsystems trees, as they have
the knowledge and the infra to check them (hmm, maybe a dedicated tree
might make sense for the smaller subsystems, but let's ignore that).
What I meant was just a tree those distros could merge into their
kernels to quickly resolve issues that upstream is slow to fix. But that
obviously has downsides, too. And is yet more work.
> However, do you have some kind of dashboard that you could share with
> the package maintainers? This way they could easily compare the not-yet
> applied fixes with their bugs and decide to backport them themselves.
I have for the kernel overall, but nothing subsystem specific. But that
is pretty high on my todo list, as...
> In other words: let others do the hard work, you are doing a lot already
...I'm very well aware of this. :-/
Ciao, Thorsten
^ permalink raw reply
* Re: regression fixes sitting in subsystem git trees for a week or longer
From: Benjamin Tissoires @ 2024-04-25 8:44 UTC (permalink / raw)
To: Thorsten Leemhuis
Cc: Linus Torvalds, Jiri Kosina, Douglas Anderson, Hans de Goede,
linux-input, linux-kernel, Kenny Levinsen, Benjamin Tissoires,
Linux regressions mailing list
In-Reply-To: <87698732-5439-42bd-b2b2-864bb4f3b3ec@leemhuis.info>
On Apr 25 2024, Thorsten Leemhuis wrote:
> On 24.04.24 20:53, Linus Torvalds wrote:
> > On Wed, 24 Apr 2024 at 09:56, Thorsten Leemhuis
> > <regressions@leemhuis.info> wrote:
> >>
> >> out of interest: what's your stance on regression fixes sitting in
> >> subsystem git trees for a week or longer before being mainlined?
> >
> > Annoying, but probably depends on circumstances. The fact that it took
> > a while to even be noticed presumably means it's not common or holding
> > anything up.
>
> Well, I searched and found quite a few users that reported the problem:
>
> https://bbs.archlinux.org/viewtopic.php?id=293971 (at least 4 people)
> https://bbs.archlinux.org/viewtopic.php?id=293978 (2 people)
> https://bugzilla.redhat.com/show_bug.cgi?id=2271136 (1)
> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2061040 (1)
> https://forums.opensuse.org/t/no-touchpad-found-el-touchpad-a-veces-es-reconocido-por-el-sistema/174100 (1)
> https://oldos.me/@jay/112294956758222518 (1)
>
> There are also these two I mentioned earlier already:
> https://social.lol/@major/112294920993272987 (1)
> https://lore.kernel.org/all/9a880b2b-2a28-4647-9f0f-223f9976fdee@manjaro.org/ (1)
>
> Side note: there were more discussions about it here:
> https://forums.lenovo.com/t5/Fedora/PSA-Z16-Gen-2-touchpad-not-working-on-kernel-6-8/m-p/5299530
> https://www.reddit.com/r/thinkpad/comments/1bwxwnr/review_thinkpad_z16_gen_2_with_arch_linux/
> https://www.reddit.com/r/linuxhardware/comments/1bwxhwa/review_thinkpad_z16_gen_2_arch_linux/
>
> And the arch linux wiki even documents a workaround:
> https://wiki.archlinux.org/title/Lenovo_ThinkPad_Z16_Gen_2#Initialization_failure
>
> Those are just the reports and discussions I found. And you know how
> it is: many people that struggle will never report a problem.
>
short FYI, (I've Cc-ed you on the PR), but I just sent the PR for HID,
which includes this fix.
>
> IMHO this all casts a bad light on our "no regression" rule, as the
> fix is ready, just not mainlined and backported. And as I mentioned:
> I see similar situations all the time. That's why I made noise here.
>
>
> > That said, th4e last HID pull I have is from March 14. If the issue is
> > just that there's nothing else happening, I think people should just
> > point me to the patch and say "can you apply this single fix?"
>
> Then I'll likely do so in my regression reports more often.
>
> Is cherry picking from -next as easy for you? Maintainers sometimes
> improve small details when merging a fix, so it might be better to
> take fixes from there instead of pulling them from lore.
Maybe one suggestion that might help to reduce these kind of situations
in the future: can you configure your bot to notify the maintainers
after a couple of days that the patch has been merged that it would be
nice if they could send the PR to Linus?
In this case I bet Jiri forgot to send it because he was overloaded and
so was I. So a friendly reminder could make things go faster.
And maybe, before sending the reminder, if you could also check that the
target branch hasn't been touched in 2 days that would prevent annoyances
when we just added a commit and want to let it stay in for-next for 24h
before sending the full branch.
>
> Ciao, Thorsten
>
> P.S: Wondering if I should team up with the kernel package maintainers
> of Arch Linux, Fedora, and openSUSE and start a git tree based on the
> latest stable tree with additional fixes and reverts for regressions
> not yet fixed upstream...[1] But that feels kinda wrong: it IMHO
> would be better to resolve those problems quickly in the proper
> upstream trees.
I would also say that this is wrong. Unless all regressions go through
your tree and you then send PR to Linus, you might quickly get
overloaded because sometimes the fix can not be cherry-picked if there
is one other change just before.
However, do you have some kind of dashboard that you could share with
the package maintainers? This way they could easily compare the not-yet
applied fixes with their bugs and decide to backport them themselves.
In other words: let others do the hard work, you are doing a lot already
:)
Anyway, I really think a friendly reminder would help makes things go
faster. Something like "Hey, it seems that you applied a regression fix
that I am currently tracking and that you haven't sent the PR to Linus
yet. Could you please send it ASAP as we already have several users
reporting the issue?".
Cheers,
Benjamin
>
> [1] yes, I'm fully aware that such a tree can only address some of the
> issues; but from what I see that already would make quite a difference.
^ permalink raw reply
* Re: regression fixes sitting in subsystem git trees for a week or longer
From: Thorsten Leemhuis @ 2024-04-25 8:25 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jiri Kosina, Douglas Anderson, Hans de Goede, linux-input,
linux-kernel, Kenny Levinsen, Benjamin Tissoires,
Linux regressions mailing list
In-Reply-To: <CAHk-=wjy_ph9URuFt-pq+2AJ__p7gFDx=yzVSCsx16xAYvNw9g@mail.gmail.com>
On 24.04.24 20:53, Linus Torvalds wrote:
> On Wed, 24 Apr 2024 at 09:56, Thorsten Leemhuis
> <regressions@leemhuis.info> wrote:
>>
>> out of interest: what's your stance on regression fixes sitting in
>> subsystem git trees for a week or longer before being mainlined?
>
> Annoying, but probably depends on circumstances. The fact that it took
> a while to even be noticed presumably means it's not common or holding
> anything up.
Well, I searched and found quite a few users that reported the problem:
https://bbs.archlinux.org/viewtopic.php?id=293971 (at least 4 people)
https://bbs.archlinux.org/viewtopic.php?id=293978 (2 people)
https://bugzilla.redhat.com/show_bug.cgi?id=2271136 (1)
https://bugs.launchpad.net/ubuntu/+source/linux/+bug/2061040 (1)
https://forums.opensuse.org/t/no-touchpad-found-el-touchpad-a-veces-es-reconocido-por-el-sistema/174100 (1)
https://oldos.me/@jay/112294956758222518 (1)
There are also these two I mentioned earlier already:
https://social.lol/@major/112294920993272987 (1)
https://lore.kernel.org/all/9a880b2b-2a28-4647-9f0f-223f9976fdee@manjaro.org/ (1)
Side note: there were more discussions about it here:
https://forums.lenovo.com/t5/Fedora/PSA-Z16-Gen-2-touchpad-not-working-on-kernel-6-8/m-p/5299530
https://www.reddit.com/r/thinkpad/comments/1bwxwnr/review_thinkpad_z16_gen_2_with_arch_linux/
https://www.reddit.com/r/linuxhardware/comments/1bwxhwa/review_thinkpad_z16_gen_2_arch_linux/
And the arch linux wiki even documents a workaround:
https://wiki.archlinux.org/title/Lenovo_ThinkPad_Z16_Gen_2#Initialization_failure
Those are just the reports and discussions I found. And you know how
it is: many people that struggle will never report a problem.
IMHO this all casts a bad light on our "no regression" rule, as the
fix is ready, just not mainlined and backported. And as I mentioned:
I see similar situations all the time. That's why I made noise here.
> That said, th4e last HID pull I have is from March 14. If the issue is
> just that there's nothing else happening, I think people should just
> point me to the patch and say "can you apply this single fix?"
Then I'll likely do so in my regression reports more often.
Is cherry picking from -next as easy for you? Maintainers sometimes
improve small details when merging a fix, so it might be better to
take fixes from there instead of pulling them from lore.
Ciao, Thorsten
P.S: Wondering if I should team up with the kernel package maintainers
of Arch Linux, Fedora, and openSUSE and start a git tree based on the
latest stable tree with additional fixes and reverts for regressions
not yet fixed upstream...[1] But that feels kinda wrong: it IMHO
would be better to resolve those problems quickly in the proper
upstream trees.
[1] yes, I'm fully aware that such a tree can only address some of the
issues; but from what I see that already would make quite a difference.
^ permalink raw reply
* Re: [PATCH v4 2/4] HID: Add Himax HX83102J touchscreen driver
From: Felix Kaechele @ 2024-04-25 3:26 UTC (permalink / raw)
To: Allen_Lin, dmitry.torokhov, robh, krzysztof.kozlowski+dt, conor,
jikos, benjamin.tissoires, linux-input, devicetree, linux-kernel
In-Reply-To: <TY0PR06MB561105A3386E9D76F429110D9E0F2@TY0PR06MB5611.apcprd06.prod.outlook.com>
Hey there,
I've recently been working on adding HX83100A support to the already
existing himax_hx83112b driver. So I thought I'd take a look at this, too.
First of all, thank you for providing documented code. The vendor
released code is much harder to parse and understand, especially given
that there is almost no commentary.
With the chip family having strong similarities between chips this is
helpful for my HX83100A additions to the himax_hx83112b driver.
As far as I understand (and given that I have no access to data sheets
and register maps) HX83112B and HX83100A are I2C, not SPI, and also
don't emit HID packets internally, but a custom event data structure.
So there are certainly some differences with that.
But registers and sequences are identical, even with the different
busses involved.
I'm sure there could be some value in combining efforts for both
drivers. At the same time I see that this is supposed to go in as a hid
driver, not into input/touchscreen. So I don't know how practical
combining things would be. So my comments below may possibly be
irrelevant if the HX83102J is the only HID over SPI chip in the family.
My comments inline are based on what I learned from studying the vendor
driver at https://github.com/HimaxSoftware/HX83112_Android_Driver and
https://github.com/HimaxSoftware/HX83100_Android_Driver
On 2024-04-17 03:41, Allen_Lin wrote:
> Add a new driver for Himax HX83102J touchscreen controllers.
> This driver supports Himax IC using the SPI interface to
> acquire HID packets.
>
> After confirmed the IC's exsitence the driver loads the firmware
> image from flash to get the HID report descriptor, VID and PID.
> And use those information to register HID device.
>
> Signed-off-by: Allen_Lin <allencl_lin@hotmail.com>
> ---
> MAINTAINERS | 1 +
> drivers/hid/Kconfig | 7 +
> drivers/hid/Makefile | 2 +
> drivers/hid/hid-himax.c | 1768 +++++++++++++++++++++++++++++++++++++++
> drivers/hid/hid-himax.h | 288 +++++++
> 5 files changed, 2066 insertions(+)
> create mode 100644 drivers/hid/hid-himax.c
> create mode 100644 drivers/hid/hid-himax.h
>
...
> diff --git a/drivers/hid/hid-himax.c b/drivers/hid/hid-himax.c
> new file mode 100644
> index 000000000000..f8a417e07f0c
> --- /dev/null
> +++ b/drivers/hid/hid-himax.c
> @@ -0,0 +1,1768 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Himax hx83102j SPI Driver Code for HID.
> + *
> + * Copyright (C) 2024 Himax Corporation.
> + */
...
> +/**
> + * hx83102j_sense_off() - Stop MCU and enter safe mode
> + * @ts: Himax touch screen data
> + * @check_en: Check if need to ensure FW is stopped by its owne process
> + *
> + * Sense off is a process to make sure the MCU inside the touch chip is stopped.
> + * The process has two stage, first stage is to request FW to stop. Write
> + * HIMAX_REG_DATA_FW_GO_SAFEMODE to HIMAX_REG_ADDR_CTRL_FW tells the FW to stop by its own.
> + * Then read back the FW status to confirm the FW is stopped. When check_en is true,
> + * the function will resend the stop FW command until the retry limit reached.
> + * There maybe a chance that the FW is not stopped by its own, in this case, the
> + * safe mode in next stage still stop the MCU, but FW internal flag may not be
> + * configured correctly. The second stage is to enter safe mode and reset TCON.
> + * Safe mode is a mode that the IC circuit ensure the internal MCU is stopped.
> + * Since this IC is TDDI, the TCON need to be reset to make sure the IC is ready
> + * for next operation.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int hx83102j_sense_off(struct himax_ts_data *ts, bool check_en)
> +{
> + int ret;
> + u32 retry_cnt;
> + const u32 stop_fw_retry_limit = 35;
> + const u32 enter_safe_mode_retry_limit = 5;
> + const union himax_dword_data safe_mode = {
> + .dword = cpu_to_le32(HIMAX_REG_DATA_FW_GO_SAFEMODE)
> + };
> + union himax_dword_data data;
> +
> + dev_info(ts->dev, "%s: check %s\n", __func__, check_en ? "True" : "False");
> + if (!check_en)
> + goto without_check;
> +
> + for (retry_cnt = 0; retry_cnt < stop_fw_retry_limit; retry_cnt++) {
> + if (retry_cnt == 0 ||
> + (data.byte[0] != HIMAX_REG_DATA_FW_GO_SAFEMODE &&
> + data.byte[0] != HIMAX_REG_DATA_FW_RE_INIT &&
> + data.byte[0] != HIMAX_REG_DATA_FW_IN_SAFEMODE)) {
> + ret = himax_mcu_register_write(ts, HIMAX_REG_ADDR_CTRL_FW,
> + safe_mode.byte, 4);
> + if (ret < 0) {
> + dev_err(ts->dev, "%s: stop FW failed\n", __func__);
> + return ret;
> + }
> + }
> + usleep_range(10000, 11000);
> +
> + ret = himax_mcu_register_read(ts, HIMAX_REG_ADDR_FW_STATUS, data.byte, 4);
> + if (ret < 0) {
> + dev_err(ts->dev, "%s: read central state failed\n", __func__);
> + return ret;
> + }
> + if (data.byte[0] != HIMAX_REG_DATA_FW_STATE_RUNNING) {
> + dev_info(ts->dev, "%s: Do not need wait FW, Status = 0x%02X!\n", __func__,
> + data.byte[0]);
> + break;
> + }
> +
> + ret = himax_mcu_register_read(ts, HIMAX_REG_ADDR_CTRL_FW, data.byte, 4);
> + if (ret < 0) {
> + dev_err(ts->dev, "%s: read ctrl FW failed\n", __func__);
> + return ret;
> + }
> + if (data.byte[0] == HIMAX_REG_DATA_FW_IN_SAFEMODE)
> + break;
> + }
> +
> + if (data.byte[0] != HIMAX_REG_DATA_FW_IN_SAFEMODE)
> + dev_warn(ts->dev, "%s: Failed to stop FW!\n", __func__);
> +
> +without_check:
> + for (retry_cnt = 0; retry_cnt < enter_safe_mode_retry_limit; retry_cnt++) {
> + /* set Enter safe mode : 0x31 ==> 0x9527 */
> + data.word[0] = cpu_to_le16(HIMAX_HX83102J_SAFE_MODE_PASSWORD);
> + ret = himax_write(ts, HIMAX_AHB_ADDR_PSW_LB, NULL, data.byte, 2);
> + if (ret < 0) {
> + dev_err(ts->dev, "%s: enter safe mode failed\n", __func__);
> + return ret;
> + }
> +
> + /* Check enter_save_mode */
> + ret = himax_mcu_register_read(ts, HIMAX_REG_ADDR_FW_STATUS, data.byte, 4);
> + if (ret < 0) {
> + dev_err(ts->dev, "%s: read central state failed\n", __func__);
> + return ret;
> + }
> +
> + if (data.byte[0] == HIMAX_REG_DATA_FW_STATE_SAFE_MODE) {
> + dev_info(ts->dev, "%s: Safe mode entered\n", __func__);
> + /* Reset TCON */
> + data.dword = cpu_to_le32(HIMAX_REG_DATA_TCON_RST);
> + ret = himax_mcu_register_write(ts, HIMAX_HX83102J_REG_ADDR_TCON_RST,
> + data.byte, 4);
> + if (ret < 0) {
> + dev_err(ts->dev, "%s: reset TCON failed\n", __func__);
> + return ret;
> + }
> + usleep_range(1000, 1100);
> + return 0;
> + }
> + usleep_range(5000, 5100);
> + hx83102j_pin_reset(ts);
> + }
> + dev_err(ts->dev, "%s: failed!\n", __func__);
> +
> + return -EIO;
> +}
> +
Used generically across HX831xx family (except HX83100A):
https://github.com/HimaxSoftware/HX83112_Android_Driver/blob/939400d4d4bf614bbeff51e1986760b47dde9eab/hxchipset/himax_ic_incell_core.c#L404
Also, HIMAX_HX83102J_SAFE_MODE_PASSWORD doesn't seem specific to the
HX83102J.
This actually apply to a number of the HIMAX_HX83102J_* defines
throughout the code.
> +/**
> + * hx83102j_chip_detect() - Check if the touch chip is HX83102J
> + * @ts: Himax touch screen data
> + *
> + * This function is used to check if the touch chip is HX83102J. The process
> + * start with a hardware reset to the touch chip, then knock the IC bus interface
> + * to wakeup the IC bus interface. Then sense off the MCU to prevent bus conflict
> + * when reading the IC ID. The IC ID is read from the IC register, and compare
> + * with the expected ID. If the ID is matched, the chip is HX83102J. Due to display
> + * IC initial code may not ready before the IC ID is read, the function will retry
> + * to read the IC ID for several times to make sure the IC ID is read correctly.
> + * In any case, the SPI bus shouldn't have error when reading the IC ID, so the
> + * function will return error if the SPI bus has error. When the IC is not HX83102J,
> + * the function will return -ENODEV.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int hx83102j_chip_detect(struct himax_ts_data *ts)
> +{
> + int ret;
> + u32 retry_cnt;
> + const u32 read_icid_retry_limit = 5;
> + const u32 ic_id_mask = GENMASK(31, 8);
> + union himax_dword_data data;
> +
> + hx83102j_pin_reset(ts);
> + ret = himax_mcu_interface_on(ts);
> + if (ret)
> + return ret;
> +
> + ret = hx83102j_sense_off(ts, false);
> + if (ret)
> + return ret;
> +
> + for (retry_cnt = 0; retry_cnt < read_icid_retry_limit; retry_cnt++) {
> + ret = himax_mcu_register_read(ts, HIMAX_REG_ADDR_ICID, data.byte, 4);
> + if (ret) {
> + dev_err(ts->dev, "%s: Read IC ID Fail\n", __func__);
> + return ret;
> + }
> +
> + data.dword = le32_to_cpu(data.dword);
> + if ((data.dword & ic_id_mask) == HIMAX_REG_DATA_ICID) {
> + ts->ic_data.icid = data.dword;
> + dev_info(ts->dev, "%s: Detect IC HX83102J successfully\n", __func__);
> + return 0;
> + }
> + }
> + dev_err(ts->dev, "%s: Read driver ID register Fail! IC ID = %X,%X,%X\n", __func__,
> + data.byte[3], data.byte[2], data.byte[1]);
> +
> + return -ENODEV;
> +}
> +
Same here, this is also used generically across all chips in the family
(except HX83100A).
...
> +/**
> + * hx83102j_read_event_stack() - Read event stack from touch chip
> + * @ts: Himax touch screen data
> + * @buf: Buffer to store the data
> + * @length: Length of data to read
> + *
> + * This function is used to read the event stack from the touch chip. The event stack
> + * is an AHB output buffer, which store the touch report data.
> + *
> + * Return: 0 on success, negative error code on failure
> + */
> +static int hx83102j_read_event_stack(struct himax_ts_data *ts, u8 *buf, u32 length)
> +{
> + u32 i;
> + int ret;
> + const u32 max_trunk_sz = ts->spi_xfer_max_sz - HIMAX_BUS_R_HLEN;
> +
> + for (i = 0; i < length; i += max_trunk_sz) {
> + ret = himax_read(ts, HIMAX_AHB_ADDR_EVENT_STACK, buf + i,
> + min(length - i, max_trunk_sz));
> + if (ret) {
> + dev_err(ts->dev, "%s: read event stack error!\n", __func__);
> + return ret;
> + }
> + }
> +
> + return 0;
> +}
> +
Again, generic across HX831xx (except HX83100A).
Regards,
Felix
^ permalink raw reply
* [PATCH v9 2/8] x86/vmware: Move common macros to vmware.h
From: Alexey Makhalov @ 2024-04-24 23:14 UTC (permalink / raw)
To: linux-kernel, virtualization, bp, hpa, dave.hansen, mingo, tglx
Cc: x86, netdev, richardcochran, linux-input, dmitry.torokhov, zackr,
linux-graphics-maintainer, pv-drivers, timothym, akaher,
dri-devel, daniel, airlied, tzimmermann, mripard,
maarten.lankhorst, horms, kirill.shutemov, Alexey Makhalov,
Nadav Amit
In-Reply-To: <20240424231407.14098-1-alexey.makhalov@broadcom.com>
Move VMware hypercall macros to vmware.h. This is a prerequisite for
the introduction of vmware_hypercall API. No functional changes besides
exporting vmware_hypercall_mode symbol.
Signed-off-by: Alexey Makhalov <alexey.makhalov@broadcom.com>
Reviewed-by: Nadav Amit <nadav.amit@gmail.com>
---
arch/x86/include/asm/vmware.h | 72 +++++++++++++++++++++++++++++------
arch/x86/kernel/cpu/vmware.c | 43 +--------------------
2 files changed, 62 insertions(+), 53 deletions(-)
diff --git a/arch/x86/include/asm/vmware.h b/arch/x86/include/asm/vmware.h
index ac9fc51e2b18..de2533337611 100644
--- a/arch/x86/include/asm/vmware.h
+++ b/arch/x86/include/asm/vmware.h
@@ -8,25 +8,34 @@
/*
* The hypercall definitions differ in the low word of the %edx argument
- * in the following way: the old port base interface uses the port
- * number to distinguish between high- and low bandwidth versions.
+ * in the following way: the old I/O port based interface uses the port
+ * number to distinguish between high- and low bandwidth versions, and
+ * uses IN/OUT instructions to define transfer direction.
*
* The new vmcall interface instead uses a set of flags to select
* bandwidth mode and transfer direction. The flags should be loaded
* into %dx by any user and are automatically replaced by the port
- * number if the VMWARE_HYPERVISOR_PORT method is used.
- *
- * In short, new driver code should strictly use the new definition of
- * %dx content.
+ * number if the I/O port method is used.
*/
-/* Old port-based version */
-#define VMWARE_HYPERVISOR_PORT 0x5658
-#define VMWARE_HYPERVISOR_PORT_HB 0x5659
+#define VMWARE_HYPERVISOR_HB BIT(0)
+#define VMWARE_HYPERVISOR_OUT BIT(1)
+
+#define VMWARE_HYPERVISOR_PORT 0x5658
+#define VMWARE_HYPERVISOR_PORT_HB (VMWARE_HYPERVISOR_PORT | \
+ VMWARE_HYPERVISOR_HB)
+
+#define VMWARE_HYPERVISOR_MAGIC 0x564d5868U
+
+#define VMWARE_CMD_GETVERSION 10
+#define VMWARE_CMD_GETHZ 45
+#define VMWARE_CMD_GETVCPU_INFO 68
+#define VMWARE_CMD_STEALCLOCK 91
+
+#define CPUID_VMWARE_FEATURES_ECX_VMMCALL BIT(0)
+#define CPUID_VMWARE_FEATURES_ECX_VMCALL BIT(1)
-/* Current vmcall / vmmcall version */
-#define VMWARE_HYPERVISOR_HB BIT(0)
-#define VMWARE_HYPERVISOR_OUT BIT(1)
+extern u8 vmware_hypercall_mode;
/* The low bandwidth call. The low word of edx is presumed clear. */
#define VMWARE_HYPERCALL \
@@ -54,4 +63,43 @@
"rep insb", \
"vmcall", X86_FEATURE_VMCALL, \
"vmmcall", X86_FEATURE_VMW_VMMCALL)
+
+#define VMWARE_PORT(cmd, eax, ebx, ecx, edx) \
+ __asm__("inl (%%dx), %%eax" : \
+ "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) : \
+ "a"(VMWARE_HYPERVISOR_MAGIC), \
+ "c"(VMWARE_CMD_##cmd), \
+ "d"(VMWARE_HYPERVISOR_PORT), "b"(UINT_MAX) : \
+ "memory")
+
+#define VMWARE_VMCALL(cmd, eax, ebx, ecx, edx) \
+ __asm__("vmcall" : \
+ "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) : \
+ "a"(VMWARE_HYPERVISOR_MAGIC), \
+ "c"(VMWARE_CMD_##cmd), \
+ "d"(0), "b"(UINT_MAX) : \
+ "memory")
+
+#define VMWARE_VMMCALL(cmd, eax, ebx, ecx, edx) \
+ __asm__("vmmcall" : \
+ "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) : \
+ "a"(VMWARE_HYPERVISOR_MAGIC), \
+ "c"(VMWARE_CMD_##cmd), \
+ "d"(0), "b"(UINT_MAX) : \
+ "memory")
+
+#define VMWARE_CMD(cmd, eax, ebx, ecx, edx) do { \
+ switch (vmware_hypercall_mode) { \
+ case CPUID_VMWARE_FEATURES_ECX_VMCALL: \
+ VMWARE_VMCALL(cmd, eax, ebx, ecx, edx); \
+ break; \
+ case CPUID_VMWARE_FEATURES_ECX_VMMCALL: \
+ VMWARE_VMMCALL(cmd, eax, ebx, ecx, edx); \
+ break; \
+ default: \
+ VMWARE_PORT(cmd, eax, ebx, ecx, edx); \
+ break; \
+ } \
+ } while (0)
+
#endif
diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
index f58c8d669bd3..acd9658f7c4b 100644
--- a/arch/x86/kernel/cpu/vmware.c
+++ b/arch/x86/kernel/cpu/vmware.c
@@ -41,8 +41,6 @@
#define CPUID_VMWARE_INFO_LEAF 0x40000000
#define CPUID_VMWARE_FEATURES_LEAF 0x40000010
-#define CPUID_VMWARE_FEATURES_ECX_VMMCALL BIT(0)
-#define CPUID_VMWARE_FEATURES_ECX_VMCALL BIT(1)
#define VMWARE_HYPERVISOR_MAGIC 0x564D5868
@@ -58,44 +56,6 @@
#define STEALCLOCK_DISABLED 0
#define STEALCLOCK_ENABLED 1
-#define VMWARE_PORT(cmd, eax, ebx, ecx, edx) \
- __asm__("inl (%%dx), %%eax" : \
- "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) : \
- "a"(VMWARE_HYPERVISOR_MAGIC), \
- "c"(VMWARE_CMD_##cmd), \
- "d"(VMWARE_HYPERVISOR_PORT), "b"(UINT_MAX) : \
- "memory")
-
-#define VMWARE_VMCALL(cmd, eax, ebx, ecx, edx) \
- __asm__("vmcall" : \
- "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) : \
- "a"(VMWARE_HYPERVISOR_MAGIC), \
- "c"(VMWARE_CMD_##cmd), \
- "d"(0), "b"(UINT_MAX) : \
- "memory")
-
-#define VMWARE_VMMCALL(cmd, eax, ebx, ecx, edx) \
- __asm__("vmmcall" : \
- "=a"(eax), "=c"(ecx), "=d"(edx), "=b"(ebx) : \
- "a"(VMWARE_HYPERVISOR_MAGIC), \
- "c"(VMWARE_CMD_##cmd), \
- "d"(0), "b"(UINT_MAX) : \
- "memory")
-
-#define VMWARE_CMD(cmd, eax, ebx, ecx, edx) do { \
- switch (vmware_hypercall_mode) { \
- case CPUID_VMWARE_FEATURES_ECX_VMCALL: \
- VMWARE_VMCALL(cmd, eax, ebx, ecx, edx); \
- break; \
- case CPUID_VMWARE_FEATURES_ECX_VMMCALL: \
- VMWARE_VMMCALL(cmd, eax, ebx, ecx, edx); \
- break; \
- default: \
- VMWARE_PORT(cmd, eax, ebx, ecx, edx); \
- break; \
- } \
- } while (0)
-
struct vmware_steal_time {
union {
uint64_t clock; /* stolen time counter in units of vtsc */
@@ -109,7 +69,8 @@ struct vmware_steal_time {
};
static unsigned long vmware_tsc_khz __ro_after_init;
-static u8 vmware_hypercall_mode __ro_after_init;
+u8 vmware_hypercall_mode __ro_after_init;
+EXPORT_SYMBOL_GPL(vmware_hypercall_mode);
static inline int __vmware_platform(void)
{
--
2.39.0
^ permalink raw reply related
* [PATCH v9 1/8] x86/vmware: Correct macro names
From: Alexey Makhalov @ 2024-04-24 23:14 UTC (permalink / raw)
To: linux-kernel, virtualization, bp, hpa, dave.hansen, mingo, tglx
Cc: x86, netdev, richardcochran, linux-input, dmitry.torokhov, zackr,
linux-graphics-maintainer, pv-drivers, timothym, akaher,
dri-devel, daniel, airlied, tzimmermann, mripard,
maarten.lankhorst, horms, kirill.shutemov, Alexey Makhalov
In-Reply-To: <adcbfb9a-a4e1-4a32-b786-6c204d941e9f@broadcom.com>
VCPU_RESERVED and LEGACY_X2APIC are not VMware hypercall commands.
These are bits in return value of VMWARE_CMD_GETVCPU_INFO command.
Change VMWARE_CMD_ prefix to GETVCPU_INFO_ one. And move bit-shift
operation to the macro body.
Signed-off-by: Alexey Makhalov <alexey.makhalov@broadcom.com>
---
arch/x86/kernel/cpu/vmware.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/arch/x86/kernel/cpu/vmware.c b/arch/x86/kernel/cpu/vmware.c
index 11f83d07925e..f58c8d669bd3 100644
--- a/arch/x86/kernel/cpu/vmware.c
+++ b/arch/x86/kernel/cpu/vmware.c
@@ -49,10 +49,11 @@
#define VMWARE_CMD_GETVERSION 10
#define VMWARE_CMD_GETHZ 45
#define VMWARE_CMD_GETVCPU_INFO 68
-#define VMWARE_CMD_LEGACY_X2APIC 3
-#define VMWARE_CMD_VCPU_RESERVED 31
#define VMWARE_CMD_STEALCLOCK 91
+#define GETVCPU_INFO_LEGACY_X2APIC BIT(3)
+#define GETVCPU_INFO_VCPU_RESERVED BIT(31)
+
#define STEALCLOCK_NOT_AVAILABLE (-1)
#define STEALCLOCK_DISABLED 0
#define STEALCLOCK_ENABLED 1
@@ -476,8 +477,8 @@ static bool __init vmware_legacy_x2apic_available(void)
{
uint32_t eax, ebx, ecx, edx;
VMWARE_CMD(GETVCPU_INFO, eax, ebx, ecx, edx);
- return !(eax & BIT(VMWARE_CMD_VCPU_RESERVED)) &&
- (eax & BIT(VMWARE_CMD_LEGACY_X2APIC));
+ return !(eax & GETVCPU_INFO_VCPU_RESERVED) &&
+ (eax & GETVCPU_INFO_LEGACY_X2APIC);
}
#ifdef CONFIG_AMD_MEM_ENCRYPT
--
2.39.0
^ permalink raw reply related
* Re: [PATCH v8 1/7] x86/vmware: Move common macros to vmware.h
From: Alexey Makhalov @ 2024-04-24 23:12 UTC (permalink / raw)
To: Borislav Petkov
Cc: linux-kernel, virtualization, hpa, dave.hansen, mingo, tglx, x86,
netdev, richardcochran, linux-input, dmitry.torokhov, zackr,
linux-graphics-maintainer, pv-drivers, timothym, akaher,
dri-devel, daniel, airlied, tzimmermann, mripard,
maarten.lankhorst, horms, kirill.shutemov, Nadav Amit
In-Reply-To: <20240424160608.GFZikt8JLrTN4M5PG2@fat_crate.local>
On 4/24/24 9:06 AM, Borislav Petkov wrote:
> On Mon, Apr 22, 2024 at 03:56:50PM -0700, Alexey Makhalov wrote:
>> Move VMware hypercall macros to vmware.h. This is a prerequisite for
>> the introduction of vmware_hypercall API. No functional changes besides
>> exporting vmware_hypercall_mode symbol.
>
> Well, I see more.
>
> So code movement patches should be done this way:
>
> * first patch: sole code movement, no changes whatsoever
>
> * follow-on patches: add changes and explain them
>
> Because... (follow me down)...
>
>> @@ -476,8 +431,8 @@ static bool __init vmware_legacy_x2apic_available(void)
>> {
>> uint32_t eax, ebx, ecx, edx;
>> VMWARE_CMD(GETVCPU_INFO, eax, ebx, ecx, edx);
>> - return !(eax & BIT(VMWARE_CMD_VCPU_RESERVED)) &&
>> - (eax & BIT(VMWARE_CMD_LEGACY_X2APIC));
>> + return !(eax & BIT(VCPU_RESERVED)) &&
>> + (eax & BIT(VCPU_LEGACY_X2APIC));
>
> ... what is that change for?
>
> Those bit definitions are clearly vmware-specific. So why are you
> changing them to something generic-ish?
>
> In any case, this patch needs to be split as outlined above.
Thanks for prompt review. The concern is valid.
I've split this patch on 2 pieces:
1. Macro renaming - to use proper prefix GETVCPU_INFO_ instead of
incorrect VMWARE_CMD_.
2. Code movement - the original idea of the patch.
Remaining patches will remain intact.
Thanks,
--Alexey
^ permalink raw reply
* Re: [PATCH] Input: xpad - add support for ASUS ROG RAIKIRI
From: Dmitry Torokhov @ 2024-04-24 22:15 UTC (permalink / raw)
To: Vicki Pfau; +Cc: linux-input
In-Reply-To: <20240404035345.159643-1-vi@endrift.com>
On Wed, Apr 03, 2024 at 08:53:45PM -0700, Vicki Pfau wrote:
> Add the VID/PID for ASUS ROG RAIKIRI to xpad_device and the VID to xpad_table
>
> Signed-off-by: Vicki Pfau <vi@endrift.com>
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: Why does libinput randomly calls my touchpad "SynPS/2 Synaptics" or "Synaptics TM2722-001"?
From: Lyude Paul @ 2024-04-24 21:12 UTC (permalink / raw)
To: Ottavio Caruso; +Cc: Andrew Duggan, linux-input
In-Reply-To: <v08fjo$heo$1@ciao.gmane.io>
FWIW: You might have better luck asking on some of the linux-input
lists.... however
you got lucky because it happens I've both worked on this stack and
dealt with this exact machine haha. So I actually can answer this
(response down below):
On Tue, 2024-04-23 at 15:12 +0100, Ottavio Caruso wrote:
> Hi,
>
> $ sudo X -version
>
> X.Org X Server 1.21.1.7
> X Protocol Version 11, Revision 0
> Current Operating System: Linux t440 6.1.0-20-amd64 #1 SMP
> PREEMPT_DYNAMIC Debian 6.1.85-1 (2024-04-11) x86_64
> Kernel command line: BOOT_IMAGE=/boot/vmlinuz-6.1.0-20-amd64
> root=UUID=42a17f43-89bb-4523-952f-b8d97bcb4a30 ro quiet
> xorg-server 2:21.1.7-3+deb12u7 (https://www.debian.org/support)
> Current version of pixman: 0.42.2
>
> $ xinput --version
> xinput version 1.6.3
> XI version on server: 2.4
>
>
>
> On my old-ish Thinkpad T440, libinput alternatively calls my touchpad
> "SynPS/2 Synaptics" or "Synaptics TM2722-001".
>
> $ grep Synaptics /var/log/messages
> Nov 26 09:12:38 t440 kernel: [18070.908478] psmouse serio1:
> synaptics:
> serio: Synaptics pass-through port at isa0060/serio1/input0
> Nov 26 09:12:38 t440 kernel: [18070.947812] input: SynPS/2 Synaptics
> TouchPad as /devices/platform/i8042/serio1/input/input35
> Nov 26 20:33:19 t440 kernel: [27221.274488] rmi4_f01 rmi4-00.fn01:
> found
> RMI device, manufacturer: Synaptics, product: TM2722-001, fw id: 0
> Nov 26 20:33:19 t440 kernel: [27221.314747] input: Synaptics TM2722-
> 001
> as /devices/pci0000:00/0000:00:1f.3/i2c-0/0-002c/rmi4-
> 00/input/input39
> Nov 27 19:28:05 t440 kernel: [ 6.327297] psmouse serio1:
> synaptics:
> serio: Synaptics pass-through port at isa0060/serio1/input0
> Nov 27 19:28:05 t440 kernel: [ 6.366655] input: SynPS/2 Synaptics
> TouchPad as /devices/platform/i8042/serio1/input/input2
>
> This without even rebooting or suspending the laptop.
I don't think this has anything to do with libinput - Synaptics
touchpads from around this generation will initially get setup as a
PS/2 device during boot. But PS/2 mode is very limited (and somewhat
buggy) functionality wise, so at the first opportunity the synaptics
kernel driver will query the touchpad to figure out if it can be
supported over RMI4. If so, the driver is supposed to switch the
touchpad to that mode and discard the PS/2 device. So - it sounds like
what's happening is that is broken for some reason.
FWIW: I added linux-input to this thread along with our synaptics
contact: Andrew Duggan.
>
> I have some scripts that disable or enable the touchpad (especially
> when
> I use the mouse) and I have to use tricks to accommodate this.
>
> Why does this happen in the first place? How can I troubleshoot it?
>
> Thanks.
>
--
Cheers,
Lyude Paul (she/her)
Software Engineer at Red Hat
^ permalink raw reply
* Issue with psmouse Kernel Module and Trackpoint Drift Correction
From: Tom Aspenwall @ 2024-04-24 20:50 UTC (permalink / raw)
To: linux-input
Hello,
I'm experiencing an annoying issue with the psmouse kernel module on my
Gen3 T14 ThinkPad, which uses an Elan Trackpoint. When I hold the
trackpoint in one direction for about 5 seconds, it seems to recalibrate
to a new center spot. Consequently, when I release the trackpoint, the
mouse cursor moves in the opposite direction for about 5 seconds.
I've tried to disable this feature by adjusting the module parameters
(resync_time, resetafter) through various methods including udev,
modprobe, and sysfs, and even by modifying the source code. However,
none of these adjustments have changed the behavior, though sensitivity
adjustments seem to work as expected.
I've dipped into the psmouse source code, which is a over my head, and
changed default values for the parameters, recompiled the module, and
reloaded it, to no avail. I'm beginning to suspect these parameters do
not control the recalibration "feature". modinfo does state that setting
both parameters to 0 should disable them, but this has not resolved the
issue.
I've confirmed it's likely a psmouse issue, having replicated the
problem on a minimal system setup without a desktop environment or
libinput, using only evtest. I even tried modifying TP_DEF_DRIFT_TIME in
trackpoint.h out of pure desperation.
Is there any way to fix this issue, or could someone provide more
insight into what exactly these parameters (resync_time, resetafter)
control? Also where can I change the length of time that the
recalibration occurs. If the recalibatron where a longer period I doubt
I would hold it for the length of time it takes to trigger the
recalibration, but 5 seconds gets in the way of fine control of the
trackpoint.
My setup is debian sid, gnome on wayland, debian kernel 6.7.9 and 6.8.7
built from source.
Thank you for your assistance,
Tom
tom@linuxtom.com
^ permalink raw reply
* Re: regression fixes sitting in subsystem git trees for a week or longer (was: Re: [PATCH v2] HID: i2c-hid: Revert to await reset ACK before reading report descriptor)
From: Linus Torvalds @ 2024-04-24 18:53 UTC (permalink / raw)
To: Thorsten Leemhuis
Cc: Jiri Kosina, Douglas Anderson, Hans de Goede, linux-input,
linux-kernel, Kenny Levinsen, Benjamin Tissoires,
Linux regressions mailing list
In-Reply-To: <a810561a-14f3-412e-9903-acaba7a36160@leemhuis.info>
On Wed, 24 Apr 2024 at 09:56, Thorsten Leemhuis
<regressions@leemhuis.info> wrote:
>
> out of interest: what's your stance on regression fixes sitting in
> subsystem git trees for a week or longer before being mainlined?
Annoying, but probably depends on circumstances. The fact that it took
a while to even be noticed presumably means it's not common or holding
anything up.
That said, th4e last HID pull I have is from March 14. If the issue is
just that there's nothing else happening, I think people should just
point me to the patch and say "can you apply this single fix?"
Linus
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: input: sun4i-lradc-keys: Add H616 compatible
From: Krzysztof Kozlowski @ 2024-04-24 17:56 UTC (permalink / raw)
To: Andre Przywara
Cc: Hans de Goede, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-input, devicetree, linux-arm-kernel, linux-sunxi,
James McGregor
In-Reply-To: <20240424115539.50efd2f0@donnerap.manchester.arm.com>
On 24/04/2024 12:55, Andre Przywara wrote:
>
>> For sending other people patches, we could disagree. I stand that I
>> would not ever send incorrect patch intentionally.
>
> Sure, and the patch was not incorrect, I trust James that far, because I
> know him and talked to him about this patch and the process before.
>
>> Therefore reviewer's statement of oversight is entirely redundant as well.
>
> Maybe I missed that, but I don't see anything in the documentation that
> would support this statement. The "reviewer's statement of oversight"
> is only expressed by an explicit Reviewed-by: tag, it seems, and none of
> the other tags seem to include that. I would agree that authorship does,
> somewhat naturally, but I don't see that sending or SoB does.
If you send someone's patch, the patch must be correct to your best
knowledge. Therefore Reviewer's statement of oversight does not cover
anything more.
Otherwise such submitter of someone's patches must not be trusted.
>
>> I just cannot send
>> someone's patch without reviewing, thus without adhering to points
>> expressed by statement of oversight.
>
> As you mention elsewhere, this seems to be individual.
> I personally feel that this assumed "implicit review", given through
> the SoB tag or through sending, is weaker than an explicit Rb tag. I agree
> that by sending a patch from someone else I take some kind of
> responsibility for the patch, but in this case I wanted to express
You take full responsibility, not some.
> that I did a proper review of the patch, going beyond the usual process
> checks. Hence the reply with the R-b tag.
>
>>> independent from review. I mean I doubt that every maintainer sending
>>> patches up the chain (when they add their SoB) implies a *review*? Surely
>>
>> Yes, every. This applies to mass-maintainers, like netdev, Greg, Andrew etc.
>>
>> Every patch I apply to my subsystems is reviewed by me. I cannot do
>> else, because that is the requirement of maintainership.
>
> I don't see it that way, I guess many maintainers rely on (thorough)
> reviews from third parties, and just glance over each patch before
I also rely on reviews on other parties, but that does not mean I do not
perform review.
> sending? Doesn't mean that they can and do reviews, but I feel it's not a
> requirement to do so *yourself* for *every* patch?
Reviews are different. If someone trusted performs review, I can relax
and spend less time on it making less thorough review. It's still a review.
>
>> There are however maintainers (see i2c patches or Intel DRM) who accept
>> patches and do not review them. When they review, they provide
>> additional Rb tag + Sob. This is weird because it means when they accept
>> patch, they take it unreviewed! Their SoB does not imply reviewing patch
>> and this is in contrast to kernel process.
>>
>> BTW, Stephen Rothwell mentions this to every maintainer on adding their
>> tree to linux-next ("You will need to ensure that ... reviewed by you
>> (or another maintainer of your subsystem tree)").
>
> But this hints that there must be *some* review taking place, and it's the
> maintainer's responsibility to ensure this. But that doesn't mean that the
> maintainer cannot delegate? And then they would just forward patches
> reviewed by trusted people.
Yes, then can delegate to other maintainer, not to random person.
>
>>> they do agree on the patch (also typically expressed by an Ack), otherwise
>>> they wouldn't send it, but a "review" is still a different thing.
>>
>> IMO, this would mean such maintainers accept code which they do not
>> understand/review/care. They are just patch juggling monkeys who take
>> something and push it further without doing actual work.
>>
>> That's not how maintainership should look like. Maintainer must take
>> reviewed code and, if other maintainers do not review, then they must
>> perform it.
>
> Of course, but I feel this discussion goes into a different direction. I am
> not a maintainer for the sunxi tree, I am a mere messenger here,
> forwarding a patch. And I didn't think this implies an implicit review.
> A did an explicit one, to stress that I did look into the patch more
> thoroughly, and also because we are not exactly drowning in reviewers ;-)
>
>>> The Linux history has both Rb + SoB from the same person and just SoB
>>> signatures, so I assume that it's not implied.
>>
>> It depends on people. As I said, I2C and DRM provide Review tag. For me
>> this is silly and suggest that all my work, that 1000 patches I took,
>> was not reviewed.
>
> If you forward a thousand patches, I wouldn't expect you did review all of
> them *yourself*. This would be an almost impossible task, unless "review"
> just means something like: "uses tabs for indentation".
Obviously if someone forwards 1000 patches the review is much shallower,
less careful. To which extend? It's individual. Now go to several
patches for netdev and USB, from less trusted sources, which did not
receive other reviews and look for answers. You might see really
diligent review.
>
>>>> And you have there SoB which indicates you sent it...
>>>
>>> Yes, but SoB just means I sign off on the legal aspects: that I got the
>>> patches legally, compliant with the GPL, and that I am fine with and
>>> allowed to release them under GPL conditions.
>>> That does not include any code review aspect, AFAICT.
>>
>> So you want to say, that you are fine in sending intentionally buggy
>> code, knowingly incorrect, because your SoB and your "git send-email"
>> does not mean you reviewed it?
>
> Where did I claim that? Of course I would not send intentionally buggy
> code, and of course I did some high level checking of the patch before I
> attached my name to it. But to me this is still different from a proper
> "review", hence my reply.
You did not say that. I am here merely twisting the concept to border
case to show the point.
If you send someone's patch and add Review tag, what does it mean? That
you reviewed this code, so to your best knowledge it is good.
If you send someone's patch and did not add Review tag, what does it
mean? That you did not reviewed this code? So whatever is in the code is
okay? How such case should be understood.
If we take such approach, we must never trust submitters of someone's
else patches, unless they provide review tag. Without review tag, that
person does not take responsibility of that patch.
And that's just not the process which is in practice.
Best regards,
Krzysztof
^ permalink raw reply
* Re: [PATCH v2 1/3] HID: i2c-hid: Rely on HID descriptor fetch to probe
From: Doug Anderson @ 2024-04-24 17:00 UTC (permalink / raw)
To: Kenny Levinsen
Cc: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires, Hans de Goede,
Maxime Ripard, Kai-Heng Feng, Johan Hovold, linux-input,
linux-kernel, Radoslaw Biernacki, Lukasz Majczak
In-Reply-To: <20240423122518.34811-2-kl@kl.wtf>
Hi,
On Tue, Apr 23, 2024 at 5:26 AM Kenny Levinsen <kl@kl.wtf> wrote:
>
> To avoid error messages when a device is not present, b3a81b6c4fc6 added
> an initial bus probe using a dummy i2c_smbus_read_byte() call.
>
> Without this probe, i2c_hid_fetch_hid_descriptor() will fail with
> EREMOTEIO. Propagate the error up so the caller can handle EREMOTEIO
> gracefully, and remove the probe as it is no longer necessary.
>
> Signed-off-by: Kenny Levinsen <kl@kl.wtf>
> ---
> drivers/hid/i2c-hid/i2c-hid-core.c | 20 ++++++--------------
> 1 file changed, 6 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
> index 2df1ab3c31cc..515a80dbf6c7 100644
> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
> @@ -894,12 +894,8 @@ static int i2c_hid_fetch_hid_descriptor(struct i2c_hid *ihid)
> ihid->wHIDDescRegister,
> &ihid->hdesc,
> sizeof(ihid->hdesc));
> - if (error) {
> - dev_err(&ihid->client->dev,
> - "failed to fetch HID descriptor: %d\n",
> - error);
> - return -ENODEV;
> - }
> + if (error)
> + return error;
> }
>
> /* Validate the length of HID descriptor, the 4 first bytes:
> @@ -1014,17 +1010,13 @@ static int __i2c_hid_core_probe(struct i2c_hid *ihid)
> struct hid_device *hid = ihid->hid;
> int ret;
>
> - /* Make sure there is something at this address */
> - ret = i2c_smbus_read_byte(client);
> - if (ret < 0) {
> + ret = i2c_hid_fetch_hid_descriptor(ihid);
> + if (ret == -EREMOTEIO) {
I worry a little bit about keying just off of -EREMOTEIO. If I'm
skimming the code properly it's up to the different i2c bus controller
to decide which error code to return here. Looking at, for instance,
"i2c-qcom-geni.c", I see:
[NACK] = {-ENXIO, "NACK: slv unresponsive, check its power/reset-ln"},
Maybe we should just use dev_dbg() in all cases here when we fail to
fetch the descriptor? ...otherwise I think some boards will start
getting a noisy error message.
...and confirmed that my skim seemed to be accurate. i put your
patches on my system and then changed the system to think my
hid-over-i2c device was at 0x11 instead of the normal 0x10. Now, I
get:
[ 5.973417] i2c_hid_of 4-0011: failed to fetch HID descriptor: -6
[ 5.979701] i2c_hid_of 4-0011: Power on failed: -6
-Doug
^ permalink raw reply
* regression fixes sitting in subsystem git trees for a week or longer (was: Re: [PATCH v2] HID: i2c-hid: Revert to await reset ACK before reading report descriptor)
From: Thorsten Leemhuis @ 2024-04-24 16:56 UTC (permalink / raw)
To: Linus Torvalds
Cc: Jiri Kosina, Douglas Anderson, Hans de Goede, linux-input,
linux-kernel, Kenny Levinsen, Benjamin Tissoires,
Linux regressions mailing list
In-Reply-To: <CAO-hwJJtK2XRHK=HGaNUFb3mQhY5XbNGeCQwuAB0nmG2bjHX-Q@mail.gmail.com>
Linus,
On 23.04.24 16:59, Benjamin Tissoires wrote:
> On Mon, Apr 22, 2024 at 7:11 PM Linux regression tracking (Thorsten
> Leemhuis) <regressions@leemhuis.info> wrote:
>> On 31.03.24 20:24, Kenny Levinsen wrote:
>
> [previous subject: [PATCH v2] HID: i2c-hid: Revert to await reset ACK before reading report descriptor]
>
>>> In af93a167eda9, i2c_hid_parse was changed to continue with reading the
>>> report descriptor before waiting for reset to be acknowledged.
>>>
>>> This has lead to two regressions:
>>
>> Lo! Jiri, Benjamin, quick question: is there a reason why this fix for a
>> 6.8-rc1 regression after more than two and half weeks is not yet
>> mainlined? Or is there some good reason why we should be should be extra
>> cautious?
>
> No special reasons I guess. Neither Jiri nor I have sent a HID update
> for this rc cycle, so it's still there, waiting to be pushed.
> I've been quite busy with BPF lately and dropped the ball slightly on
> the HID maintainer side, but I'm sure we'll send the PR to Linus this
> week or the next.
out of interest: what's your stance on regression fixes sitting in
subsystem git trees for a week or longer before being mainlined?
The quoted patch is such a case. It fixes a regression caused by a
change that made it into 6.8-rc1, but the problem afaik was only
reported on 2024-03-19, e.g. ~nine days after 6.8 was out[1]; Kenny, the
author of the fix, apparently noticed and fixed the problem a bit later
independently[2]. Jiri merged a newer version of the fix on
2024-04-03[3], which was included in -next a day later -- the Thursday
before 6.9-rc3.
The fix thus would even have gotten two days of testing in -next, if
Benjamin or Jiri would have send it your way for that pre-release. But
from Benjamin's statement quoted above it seems the fix might even make
-rc6.
That obviously heavily reduces the time the fix will be tested before
6.9 is released.
It obviously also means that 6.8.y is as of now still unfixed, as the
stable team usually only applies fixes once they landed in mainline.
Which also means that even more people ran into the problem with
6.8.y[4] or mainline even after Jiri merged the patch into the hid tree
-- and maybe some of those people wasted their time on a bisection only
to find out that a fix exists.
That sounds, ehh, sub-optimal to me. Which is why I wonder what's your
stance here, as I encounter similar situations frequently[5] -- which
sometimes is kinda demotivating. :-/
Ciao, Thorsten
[1]
https://lore.kernel.org/all/a587f3f3-e0d5-4779-80a4-a9f7110b0bd2@manjaro.org/
[2] https://lore.kernel.org/all/20240331132332.6694-1-kl@kl.wtf/
[3]
https://lore.kernel.org/all/nycvar.YFH.7.76.2404031401411.20263@cbobk.fhfr.pm/
[4] https://social.lol/@major/112294923280815017
[5] This fix for example:
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?h=master&id=afc89870ea677bd5a44516eb981f7a259b74280c
Reports:
https://lore.kernel.org/lkml/ZYhQ2-OnjDgoqjvt@wens.tw/
https://lore.kernel.org/lkml/1553a526-6f28-4a68-88a8-f35bd22d9894@linumiz.com/
> [...]
>>> 1. We fail to handle reset acknowledgement if it happens while reading
>>> the report descriptor. The transfer sets I2C_HID_READ_PENDING, which
>>> causes the IRQ handler to return without doing anything.
>>>
>>> This affects both a Wacom touchscreen and a Sensel touchpad.
>>>
>>> 2. On a Sensel touchpad, reading the report descriptor this quickly
>>> after reset results in all zeroes or partial zeroes.
>>>
>>> The issues were observed on the Lenovo Thinkpad Z16 Gen 2.
>>>
>>> The change in question was made based on a Microsoft article[0] stating
>>> that Windows 8 *may* read the report descriptor in parallel with
>>> awaiting reset acknowledgement, intended as a slight reset performance
>>> optimization. Perhaps they only do this if reset is not completing
>>> quickly enough for their tastes?
>>>
>>> As the code is not currently ready to read registers in parallel with a
>>> pending reset acknowledgement, and as reading quickly breaks the report
>>> descriptor on the Sensel touchpad, revert to waiting for reset
>>> acknowledgement before proceeding to read the report descriptor.
>>>
>>> [0]: https://learn.microsoft.com/en-us/windows-hardware/drivers/hid/plug-and-play-support-and-power-management
>>>
>>> Fixes: af93a167eda9 ("HID: i2c-hid: Move i2c_hid_finish_hwreset() to after reading the report-descriptor")
>>> Signed-off-by: Kenny Levinsen <kl@kl.wtf>
>>> ---
>>> drivers/hid/i2c-hid/i2c-hid-core.c | 13 ++++---------
>>> 1 file changed, 4 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/hid/i2c-hid/i2c-hid-core.c b/drivers/hid/i2c-hid/i2c-hid-core.c
>>> index 2df1ab3c31cc..72d2bccf5621 100644
>>> --- a/drivers/hid/i2c-hid/i2c-hid-core.c
>>> +++ b/drivers/hid/i2c-hid/i2c-hid-core.c
>>> @@ -735,9 +735,12 @@ static int i2c_hid_parse(struct hid_device *hid)
>>> mutex_lock(&ihid->reset_lock);
>>> do {
>>> ret = i2c_hid_start_hwreset(ihid);
>>> - if (ret)
>>> + if (ret == 0)
>>> + ret = i2c_hid_finish_hwreset(ihid);
>>> + else
>>> msleep(1000);
>>> } while (tries-- > 0 && ret);
>>> + mutex_unlock(&ihid->reset_lock);
>>>
>>> if (ret)
>>> goto abort_reset;
>>> @@ -767,16 +770,8 @@ static int i2c_hid_parse(struct hid_device *hid)
>>> }
>>> }
>>>
>>> - /*
>>> - * Windows directly reads the report-descriptor after sending reset
>>> - * and then waits for resets completion afterwards. Some touchpads
>>> - * actually wait for the report-descriptor to be read before signalling
>>> - * reset completion.
>>> - */
>>> - ret = i2c_hid_finish_hwreset(ihid);
>>> abort_reset:
>>> clear_bit(I2C_HID_RESET_PENDING, &ihid->flags);
>>> - mutex_unlock(&ihid->reset_lock);
>>> if (ret)
>>> goto out;
>>>
>>
>
>
>
^ permalink raw reply
* Re: [PATCH 0/6] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on
From: Doug Anderson @ 2024-04-24 16:24 UTC (permalink / raw)
To: Johan Hovold
Cc: Johan Hovold, Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Linus Walleij, linux-input, devicetree,
linux-arm-msm, linux-kernel
In-Reply-To: <ZijH6EaqWKHWRcdK@hovoldconsulting.com>
Hi,
On Wed, Apr 24, 2024 at 1:50 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Tue, Apr 23, 2024 at 01:36:18PM -0700, Doug Anderson wrote:
> > On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote:
> > > The Elan eKTH5015M touch controller on the X13s requires a 300 ms delay
> > > before sending commands after having deasserted reset during power on.
> > >
> > > This series switches the X13s devicetree to use the Elan specific
> > > binding so that the OS can determine the required power-on sequence and
> > > make sure that the controller is always detected during boot. [1]
> > >
> > > The Elan hid-i2c driver currently asserts reset unconditionally during
> > > suspend, which does not work on the X13s where the touch controller
> > > supply is shared with other peripherals that may remain powered. Holding
> > > the controller in reset can increase power consumption and also leaks
> > > current through the reset circuitry pull ups.
> >
> > Can you provide more details about which devices exactly it shares
> > power with? I'm worried that you may be shooting yourself in the foot
> > to avoid shooting yourself in the arm.
> >
> > Specifically, if those other peripherals that may remain powered ever
> > power themselves off then you'll end up back-driving the touchscreen
> > through the reset line, won't you? Since reset is active low then not
> > asserting reset drives the reset line high and, if you power it off,
> > it can leach power backwards through the reset line. The
> > "goodix,no-reset-during-suspend" property that I added earlier
> > specifically worked on systems where the rail was always-on so I could
> > guarantee that didn't happen.
> >
> > From looking at your dts patch it looks like your power _is_ on an
> > always-on rail so you should be OK, but it should be documented that
> > this only works for always-on rails.
> >
> > ..also, from your patch description it sounds as if (maybe?) you
> > intend to eventually let the rail power off if the trackpad isn't a
> > wakeup source. If you eventually plan to do that then you definitely
> > need something more complex here...
>
> No, that's the whole point: the hardware is designed so that the reset
> line can be left deasserted by the CPU also when the supply is off.
>
> The supply in this case is shared with the keyboard and touchpad, but
> also some other devices which are not yet fully described. As you
> rightly noted, the intention is to allow the supply to eventually be
> disabled when none of these devices are enabled as wakeup sources.
>
> I did not want to get in to too much details on exactly how this
> particular reset circuit is designed, but basically you have a pull up
> to an always-on 1.8 V rail on the CPU side, a FET level shifter, and a
> pull up to the supply voltage on the peripheral side.
>
> With this design, the reset line can be left deasserted by the CPU
> (tri-stated or driven high), but the important part is that the reset
> signal that goes into the controller will be pulled to 3.3 V only when
> the supply is left on and otherwise it will be connected to ground.
Ah, got it. The level shifter isolating things makes sense.
> > I guess one last thought is: what do we do if/when someone needs the
> > same solution but they want multiple sources of touchscreens, assuming
> > we ever get the second-sourcing problem solved well. In that case the
> > different touchscreen drivers might have a different idea of how the
> > GPIO should be left when the driver exits...
>
> The second-source problem is arguable a separate one, and as we've
> discussed in the past, the current approach of describing both devices
> in the devicetree only works when the devices are truly compatible in
> terms of external resources (supplies, gpios, pinconfig). For anything
> more complex, we need a more elaborate implementation.
>
> In this case it should not be a problem, though, as the reset circuit
> should have the same properties regardless of which controller you
> connect (e.g. both nodes would have the 'no-reset-on-power-off'
> property).
The reset circuitry may be the same, but the properties of the
touchscreen might not be. It would be easy to imagine a different
touchscreen that consumes less power when held in reset.
In any case, not a problem we need to solve right now.
-Doug
^ permalink raw reply
* Re: [PATCH 4/6] HID: i2c-hid: elan: fix reset suspend current leakage
From: Doug Anderson @ 2024-04-24 16:24 UTC (permalink / raw)
To: Johan Hovold
Cc: Johan Hovold, Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Linus Walleij, linux-input, devicetree,
linux-arm-msm, linux-kernel, stable
In-Reply-To: <ZijlZw6zm4R9ULBU@hovoldconsulting.com>
Hi,
On Wed, Apr 24, 2024 at 3:56 AM Johan Hovold <johan@kernel.org> wrote:
>
> On Tue, Apr 23, 2024 at 01:37:14PM -0700, Doug Anderson wrote:
> > On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote:
>
> > > @@ -87,12 +104,14 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
> > > ihid_elan->ops.power_up = elan_i2c_hid_power_up;
> > > ihid_elan->ops.power_down = elan_i2c_hid_power_down;
> > >
> > > - /* Start out with reset asserted */
> > > - ihid_elan->reset_gpio =
> > > - devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> > > + ihid_elan->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > > + GPIOD_ASIS);
> >
> > I'm not a huge fan of this part of the change. It feels like the GPIO
> > state should be initialized by the probe function. Right before we
> > call i2c_hid_core_probe() we should be in the state of "powered off"
> > and the reset line should be in a consistent state. If
> > "no_reset_on_power_off" then it should be de-asserted. Else it should
> > be asserted.
>
> First, the reset gpio will be set before probe() returns, just not
> immediately when it is requested.
>
> [ Sure, your panel follower implementation may defer the actual probe of
> the touchscreen even further but I think that's a design flaw in the
> current implementation. ]
>
> Second, the device is not necessarily in the "powered off" state
Logically, the driver treats it as being in "powered off" state,
though. That's why the i2c-hid core makes the call to power it on. IMO
we should strive to make it more of a consistent state, not less of
one.
> as the
> driver leaves the power supplies in whatever state that the boot
> firmware left them in.
I guess it depends on the regulator. ;-) For GPIO-regulators they
aren't in whatever state the boot firmware left them in. For non-GPIO
regulators we (usually) do preserve the state that the boot firmware
left them in.
> Not immediately asserting reset and instead leaving it in the state that
> the boot firmware left it in is also no different from what happens when
> a probe function bails out before requesting the reset line.
>
> > I think GPIOD_ASIS doesn't actually do anything useful for you, right?
> > i2c_hid_core_probe() will power on and the first thing that'll happen
> > there is that the reset line will be unconditionally asserted.
>
> It avoids asserting reset before we need to and thus also avoid the need
> to deassert it on early probe failures (e.g. if one of the regulator
> lookups fails).
I guess so, though I'm of the opinion that we should be robust against
the state that firmware left things in. The firmware's job is to boot
the kernel and make sure that the system is running in a safe/reliable
way, not to optimize the power consumption of the board. If the
firmware left the line configured as "output low" then you'd let that
stand. If it's important for the line to be left in a certain state,
isn't it better to make that explicit?
Also note: if we really end up keeping GPIOD_ASIS, which I'm still not
convinced is the right move, the docs seem to imply that you need to
explicitly set a direction before using it. Your current patch doesn't
do that.
-Doug
^ permalink raw reply
* Re: [PATCH v8 1/7] x86/vmware: Move common macros to vmware.h
From: Borislav Petkov @ 2024-04-24 16:06 UTC (permalink / raw)
To: Alexey Makhalov
Cc: linux-kernel, virtualization, hpa, dave.hansen, mingo, tglx, x86,
netdev, richardcochran, linux-input, dmitry.torokhov, zackr,
linux-graphics-maintainer, pv-drivers, timothym, akaher,
dri-devel, daniel, airlied, tzimmermann, mripard,
maarten.lankhorst, horms, kirill.shutemov, Nadav Amit
In-Reply-To: <20240422225656.10309-2-alexey.makhalov@broadcom.com>
On Mon, Apr 22, 2024 at 03:56:50PM -0700, Alexey Makhalov wrote:
> Move VMware hypercall macros to vmware.h. This is a prerequisite for
> the introduction of vmware_hypercall API. No functional changes besides
> exporting vmware_hypercall_mode symbol.
Well, I see more.
So code movement patches should be done this way:
* first patch: sole code movement, no changes whatsoever
* follow-on patches: add changes and explain them
Because... (follow me down)...
> @@ -476,8 +431,8 @@ static bool __init vmware_legacy_x2apic_available(void)
> {
> uint32_t eax, ebx, ecx, edx;
> VMWARE_CMD(GETVCPU_INFO, eax, ebx, ecx, edx);
> - return !(eax & BIT(VMWARE_CMD_VCPU_RESERVED)) &&
> - (eax & BIT(VMWARE_CMD_LEGACY_X2APIC));
> + return !(eax & BIT(VCPU_RESERVED)) &&
> + (eax & BIT(VCPU_LEGACY_X2APIC));
... what is that change for?
Those bit definitions are clearly vmware-specific. So why are you
changing them to something generic-ish?
In any case, this patch needs to be split as outlined above.
Thx.
--
Regards/Gruss,
Boris.
https://people.kernel.org/tglx/notes-about-netiquette
^ permalink raw reply
* Re: [PATCH 3/3] HID: bpf: lazy load the hid_tail_call entrypoint
From: Benjamin Tissoires @ 2024-04-24 14:17 UTC (permalink / raw)
To: Dan Carpenter
Cc: oe-kbuild, Jiri Kosina, Benjamin Tissoires, lkp, oe-kbuild-all,
linux-input, linux-kernel, stable
In-Reply-To: <74d0e9d1-7ac6-4f08-bab6-76c51e69cebf@moroto.mountain>
On Apr 23 2024, Dan Carpenter wrote:
> Hi Benjamin,
>
> kernel test robot noticed the following build warnings:
>
> url: https://github.com/intel-lab-lkp/linux/commits/Benjamin-Tissoires/HID-bpf-fix-a-comment-in-a-define/20240419-225110
> base: b912cf042072e12e93faa874265b30cc0aa521b9
> patch link: https://lore.kernel.org/r/20240419-hid_bpf_lazy_skel-v1-3-9210bcd4b61c%40kernel.org
> patch subject: [PATCH 3/3] HID: bpf: lazy load the hid_tail_call entrypoint
> config: i386-randconfig-141-20240423 (https://download.01.org/0day-ci/archive/20240423/202404231109.h2IRrMMD-lkp@intel.com/config)
> compiler: clang version 17.0.6 (https://github.com/llvm/llvm-project 6009708b4367171ccdbf4b5905cb6a803753fe18)
>
> If you fix the issue in a separate patch/commit (i.e. not just a new version of
> the same patch/commit), kindly add following tags
> | Reported-by: kernel test robot <lkp@intel.com>
> | Reported-by: Dan Carpenter <dan.carpenter@linaro.org>
> | Closes: https://lore.kernel.org/r/202404231109.h2IRrMMD-lkp@intel.com/
>
> smatch warnings:
> drivers/hid/bpf/hid_bpf_jmp_table.c:478 __hid_bpf_attach_prog() error: uninitialized symbol 'link'.
>
> vim +/link +478 drivers/hid/bpf/hid_bpf_jmp_table.c
>
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 396 noinline int
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 397 __hid_bpf_attach_prog(struct hid_device *hdev, enum hid_bpf_prog_type prog_type,
> 7cdd2108903a4e3 Benjamin Tissoires 2024-01-24 398 int prog_fd, struct bpf_prog *prog, __u32 flags)
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 399 {
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 400 struct bpf_link_primer link_primer;
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 401 struct hid_bpf_link *link;
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 402 struct hid_bpf_prog_entry *prog_entry;
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 403 int cnt, err = -EINVAL, prog_table_idx = -1;
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 404
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 405 mutex_lock(&hid_bpf_attach_lock);
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 406
> 60caa381da7dc38 Benjamin Tissoires 2024-04-19 407 if (!jmp_table.map) {
> 60caa381da7dc38 Benjamin Tissoires 2024-04-19 408 err = hid_bpf_preload_skel();
> 60caa381da7dc38 Benjamin Tissoires 2024-04-19 409 WARN_ONCE(err, "error while preloading HID BPF dispatcher: %d", err);
> 60caa381da7dc38 Benjamin Tissoires 2024-04-19 410 if (err)
> 60caa381da7dc38 Benjamin Tissoires 2024-04-19 411 goto err_unlock;
> ^^^^^^^^^^^^^^^^
> link isn't initialized.
Well spotted! Thanks
I'll send a v2 soon.
Cheers,
Benjamin
>
> 60caa381da7dc38 Benjamin Tissoires 2024-04-19 412 }
> 60caa381da7dc38 Benjamin Tissoires 2024-04-19 413
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 414 link = kzalloc(sizeof(*link), GFP_USER);
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 415 if (!link) {
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 416 err = -ENOMEM;
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 417 goto err_unlock;
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 418 }
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 419
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 420 bpf_link_init(&link->link, BPF_LINK_TYPE_UNSPEC,
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 421 &hid_bpf_link_lops, prog);
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 422
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 423 /* do not attach too many programs to a given HID device */
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 424 cnt = hid_bpf_program_count(hdev, NULL, prog_type);
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 425 if (cnt < 0) {
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 426 err = cnt;
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 427 goto err_unlock;
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 428 }
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 429
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 430 if (cnt >= hid_bpf_max_programs(prog_type)) {
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 431 err = -E2BIG;
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 432 goto err_unlock;
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 433 }
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 434
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 435 prog_table_idx = hid_bpf_insert_prog(prog_fd, prog);
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 436 /* if the jmp table is full, abort */
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 437 if (prog_table_idx < 0) {
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 438 err = prog_table_idx;
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 439 goto err_unlock;
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 440 }
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 441
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 442 if (flags & HID_BPF_FLAG_INSERT_HEAD) {
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 443 /* take the previous prog_entry slot */
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 444 jmp_table.tail = PREV(jmp_table.tail);
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 445 prog_entry = &jmp_table.entries[jmp_table.tail];
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 446 } else {
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 447 /* take the next prog_entry slot */
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 448 prog_entry = &jmp_table.entries[jmp_table.head];
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 449 jmp_table.head = NEXT(jmp_table.head);
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 450 }
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 451
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 452 /* we steal the ref here */
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 453 prog_entry->prog = prog;
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 454 prog_entry->idx = prog_table_idx;
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 455 prog_entry->hdev = hdev;
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 456 prog_entry->type = prog_type;
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 457
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 458 /* finally store the index in the device list */
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 459 err = hid_bpf_populate_hdev(hdev, prog_type);
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 460 if (err) {
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 461 hid_bpf_release_prog_at(prog_table_idx);
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 462 goto err_unlock;
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 463 }
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 464
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 465 link->hid_table_index = prog_table_idx;
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 466
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 467 err = bpf_link_prime(&link->link, &link_primer);
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 468 if (err)
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 469 goto err_unlock;
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 470
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 471 mutex_unlock(&hid_bpf_attach_lock);
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 472
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 473 return bpf_link_settle(&link_primer);
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 474
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 475 err_unlock:
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 476 mutex_unlock(&hid_bpf_attach_lock);
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 477
> 4b9a3f49f02bf68 Benjamin Tissoires 2023-01-13 @478 kfree(link);
> ^^^^
>
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 479
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 480 return err;
> f5c27da4e3c8a2e Benjamin Tissoires 2022-11-03 481 }
>
> --
> 0-DAY CI Kernel Test Service
> https://github.com/intel/lkp-tests/wiki
>
^ permalink raw reply
* [PATCH] Add Logitech HID++ devices
From: kde @ 2024-04-24 11:31 UTC (permalink / raw)
To: lains, hadess, jikos, benjamin.tissoires, linux-input,
linux-kernel
Cc: Allan Sandfeld Jensen, Allan Sandfeld Jensen
From: Allan Sandfeld Jensen <allan.jensen@qt.io>
Adds a few recognized Logitech HID++ capable mice over USB and Bluetooth
Signed-off-by: Allan Sandfeld Jensen <kde@carewolf.com>
---
drivers/hid/hid-logitech-hidpp.c | 14 ++++++++++++--
1 file changed, 12 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 3c00e6ac8e76..6907b8c48c4e 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -4352,13 +4352,17 @@ static const struct hid_device_id hidpp_devices[] = {
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC081) },
{ /* Logitech G903 Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC086) },
+ { /* Logitech G Pro Gaming Mouse over USB */
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC088) },
+ { /* MX Vertical over USB */
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC08A) },
+ { /* Logitech G703 Hero Gaming Mouse over USB */
+ HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC090) },
{ /* Logitech G903 Hero Gaming Mouse over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC091) },
{ /* Logitech G920 Wheel over USB */
HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, USB_DEVICE_ID_LOGITECH_G920_WHEEL),
.driver_data = HIDPP_QUIRK_CLASS_G920 | HIDPP_QUIRK_FORCE_OUTPUT_REPORTS},
- { /* Logitech G Pro Gaming Mouse over USB */
- HID_USB_DEVICE(USB_VENDOR_ID_LOGITECH, 0xC088) },
{ /* MX5000 keyboard over Bluetooth */
HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb305),
@@ -4373,13 +4377,19 @@ static const struct hid_device_id hidpp_devices[] = {
HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb008) },
{ /* MX Master mouse over Bluetooth */
HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb012) },
+ { /* MX Master 2S mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb019) },
{ /* MX Ergo trackball over Bluetooth */
HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01d) },
{ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb01e) },
+ { /* MX Vertical mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb020) },
{ /* MX Master 3 mouse over Bluetooth */
HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb023) },
{ /* MX Master 3S mouse over Bluetooth */
HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb034) },
+ { /* MX Anywhere 3SB mouse over Bluetooth */
+ HID_BLUETOOTH_DEVICE(USB_VENDOR_ID_LOGITECH, 0xb038) },
{}
};
--
2.39.2
^ permalink raw reply related
* Re: [PATCH] Logitech Anywhere 3SB support
From: Allan Sandfeld Jensen @ 2024-04-24 11:30 UTC (permalink / raw)
To: Benjamin Tissoires, Hans de Goede, linux-kernel
Cc: benjamin.tissoires, linux-input
In-Reply-To: <fe2980e3-3204-4572-9c7c-1e960727e1d4@redhat.com>
On Monday 15 April 2024 20:31:14 CEST Hans de Goede wrote:
> Hi,
>
> On 4/15/24 5:54 PM, Benjamin Tissoires wrote:
> > [Ccing Hans as well for input]
> >
> > On Apr 13 2024, kde@carewolf.com wrote:
> >> From: Allan Sandfeld Jensen <allan.jensen@qt.io>
> >
> > FWIW, this patch neesd a commit description and signed-offs
> >
Will add.
>
> FWIW I'm also not in favor of stretching drivers/hid/hid-logitech-dj.c
> even further to also support the new bolt stuff.
>
> AFAIK the new bolt stuff is significantly different.
>
> Allan, I see in your other reply that you are mainly after
> highres scrolling and since the bolt receiver does not do
> per paired device addressing I wonder if you cannot just
> get that by treating the bolt receiver as a wired HIDPP
> device and just directly listing it as such in
> hid-logitech-hidpp.c ?
>
> The whole purpose of hid-logitech-dj.c is to create 1 virtual
> hidpp devices per paired device and with bolt that is not
> possible, so I think that we should circumvent hid-logitech-dj.c
> for bolt and if we want to use any hidpp features do so
> by directly listing the receivers in hid-logitech-hidpp.c .
>
I think the bolt receiver is able to separate devices, but yes, it appears the
way it transmits device IDs and pairs has changed (some new registers it looks
like). I am removing this part of the patch. I am not adding the Bolt receiver
to hid-logitech-hidpp.c either though, because it doesnt work for me, and I
havent invested time yet to figure out what would be needed to get it to work.
I will transmit a patch with just the new bluetooth ID, and add a few more I
managed to find to fill out hid-logitech-hidpp.c
Best regards
Allan Sandfeld Jensen
^ permalink raw reply
* Re: [PATCH 4/6] HID: i2c-hid: elan: fix reset suspend current leakage
From: Johan Hovold @ 2024-04-24 10:56 UTC (permalink / raw)
To: Doug Anderson
Cc: Johan Hovold, Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Linus Walleij, linux-input, devicetree,
linux-arm-msm, linux-kernel, stable
In-Reply-To: <CAD=FV=XP8aCjwE3LfgMy4oBL4xftFg5NkgUFso__54zNp_ZWiA@mail.gmail.com>
On Tue, Apr 23, 2024 at 01:37:14PM -0700, Doug Anderson wrote:
> On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote:
> > @@ -87,12 +104,14 @@ static int i2c_hid_of_elan_probe(struct i2c_client *client)
> > ihid_elan->ops.power_up = elan_i2c_hid_power_up;
> > ihid_elan->ops.power_down = elan_i2c_hid_power_down;
> >
> > - /* Start out with reset asserted */
> > - ihid_elan->reset_gpio =
> > - devm_gpiod_get_optional(&client->dev, "reset", GPIOD_OUT_HIGH);
> > + ihid_elan->reset_gpio = devm_gpiod_get_optional(&client->dev, "reset",
> > + GPIOD_ASIS);
>
> I'm not a huge fan of this part of the change. It feels like the GPIO
> state should be initialized by the probe function. Right before we
> call i2c_hid_core_probe() we should be in the state of "powered off"
> and the reset line should be in a consistent state. If
> "no_reset_on_power_off" then it should be de-asserted. Else it should
> be asserted.
First, the reset gpio will be set before probe() returns, just not
immediately when it is requested.
[ Sure, your panel follower implementation may defer the actual probe of
the touchscreen even further but I think that's a design flaw in the
current implementation. ]
Second, the device is not necessarily in the "powered off" state as the
driver leaves the power supplies in whatever state that the boot
firmware left them in.
Not immediately asserting reset and instead leaving it in the state that
the boot firmware left it in is also no different from what happens when
a probe function bails out before requesting the reset line.
> I think GPIOD_ASIS doesn't actually do anything useful for you, right?
> i2c_hid_core_probe() will power on and the first thing that'll happen
> there is that the reset line will be unconditionally asserted.
It avoids asserting reset before we need to and thus also avoid the need
to deassert it on early probe failures (e.g. if one of the regulator
lookups fails).
We also don't need to worry about timing requirements, which can all be
handled in one place (i.e. in the power up and power down callbacks).
> Having this as "GPIOD_ASIS" makes it feel like the kernel is somehow
> able to maintain continuity of this GPIO line from the BIOS state to
> the kernel, but I don't think it can. I've looked at the "GPIOD_ASIS"
> property before because I've always wanted the ability to have GPIOs
> that could more seamlessly transition their firmware state to their
> kernel state. I don't think the API actually allows it. The fact that
> GPIO regulators don't support this seamless transition (even though it
> would be an obvious feature to add) supports my theory that the API
> doesn't currently allow it. It may be possible to make something work
> on some implementations but I think it's not guaranteed.
>
> Specifically, the docs say:
>
> * GPIOD_ASIS or 0 to not initialize the GPIO at all. The direction must be set
> later with one of the dedicated functions.
>
> So that means that you can't read the pin without making it an input
> (which might change the state if it was previously driving a value)
> and you can't write the pin without making it an output and choosing a
> value to set it to. Basically grabbing a pin with "asis" doesn't allow
> you to do anything with it--it just claims it and doesn't let anyone
> else have it.
These properties may prevent it from being used by the regulator
framework, but GPIOD_ASIS works well in the case of a reset gpio where
we simply leave it in whatever state the firmware left it in if probe
fails before we get to powering on the device.
Johan
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: input: sun4i-lradc-keys: Add H616 compatible
From: Andre Przywara @ 2024-04-24 10:55 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Hans de Goede, Dmitry Torokhov, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, Chen-Yu Tsai, Jernej Skrabec, Samuel Holland,
linux-input, devicetree, linux-arm-kernel, linux-sunxi,
James McGregor
In-Reply-To: <1714205b-39cf-4803-b251-a35f6b9ab3e9@linaro.org>
On Tue, 23 Apr 2024 16:59:31 +0200
Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
Hi,
> On 23/04/2024 14:51, Andre Przywara wrote:
> > On Tue, 23 Apr 2024 14:18:23 +0200
> > Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> >
> > Hi,
> >
> >> On 23/04/2024 12:15, Andre Przywara wrote:
> >>> On Mon, 22 Apr 2024 17:45:10 +0100
> >>> Andre Przywara <andre.przywara@arm.com> wrote:
> >>>
> >>> Hi,
> >>>
> >>>> From: James McGregor <jamcgregor@protonmail.com>
> >>>>
> >>>> The Allwinner H616 SoC has an LRADC which is compatible with the
> >>>> versions in existing SoCs.
> >>>> Add a compatible string for H616, with the R329 fallback. This is the
> >>>> same as the D1, so put them into an enum.
> >>>>
> >>>> Signed-off-by: James McGregor <jamcgregor@protonmail.com>
> >>>> Signed-off-by: Andre Przywara <andre.przywara@arm.com>
> >>>
> >>> Compared the descriptions in the manual between the R392 and the H616, they
> >>> look the same:
> >>>
> >>> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> >>
> >> Why do you review your own patches? Does it mean that you contribute
> >> code which you did not review before?
> >
> > I just merely sent the code on behalf of James, because he had trouble
> > with the email setup (Protonmail has no SMTP), but didn't want to delay
> > the post any longer.
>
> OK, thanks, I suggest using b4 relay in the future.
Sure, will relay that - though this is not trivial to setup either.
> >> This is odd process.
> >
> > I agree, I would have liked it more if James would have sent it himself,
> > and then my review would look more natural, but with my review I
> > wanted to explicitly point out the technical correctness. Besides: I found
> > this ordering issue in the other patch only after sending, so needed to
> > somehow respond anyway.
> > Also I wanted to make the process transparent: someone posts a patch (in
> > this case via a proxy), then it gets reviewed.
> >
> >> Your Review is implied by sending the patch.
> >
> > Is that really true? I was under the impression that sending is
>
> For authorship, both tested and review are implied. You cannot send code
> which you do not think is correct, therefore your authorship fulfills
> entire Reviewer's statement of oversight. There is nothing new said in
> statement of oversight comparing to what authorship says.
Fair enough, and James did, but I am not the author, as shown by the
explicit From: line, having a different name.
I *did* have a look over the patch before sending it, running checkpatch
etc.
But I cannot find anything in submitting-patches.rst that would constitute
that Signed-off-by: includes any kind of technical review, it just seems
to cover the legal aspects of patch deliverance?
And personally for me a *review* means a thorough look at the code,
understanding what it does and how it does it, and checking various
details, for instance by looking into a datasheet.
This is what I did only later, hence the separate review mail.
> Now for testing, I think it is also kind of obvious that whenever we can
> test our own code, we test it.
Granted. I do not have hardware to test, but I ran DT schema checks
before sending, I guess this qualifies as "testing" to some degree.
> For sending other people patches, we could disagree. I stand that I
> would not ever send incorrect patch intentionally.
Sure, and the patch was not incorrect, I trust James that far, because I
know him and talked to him about this patch and the process before.
> Therefore reviewer's statement of oversight is entirely redundant as well.
Maybe I missed that, but I don't see anything in the documentation that
would support this statement. The "reviewer's statement of oversight"
is only expressed by an explicit Reviewed-by: tag, it seems, and none of
the other tags seem to include that. I would agree that authorship does,
somewhat naturally, but I don't see that sending or SoB does.
> I just cannot send
> someone's patch without reviewing, thus without adhering to points
> expressed by statement of oversight.
As you mention elsewhere, this seems to be individual.
I personally feel that this assumed "implicit review", given through
the SoB tag or through sending, is weaker than an explicit Rb tag. I agree
that by sending a patch from someone else I take some kind of
responsibility for the patch, but in this case I wanted to express
that I did a proper review of the patch, going beyond the usual process
checks. Hence the reply with the R-b tag.
> > independent from review. I mean I doubt that every maintainer sending
> > patches up the chain (when they add their SoB) implies a *review*? Surely
>
> Yes, every. This applies to mass-maintainers, like netdev, Greg, Andrew etc.
>
> Every patch I apply to my subsystems is reviewed by me. I cannot do
> else, because that is the requirement of maintainership.
I don't see it that way, I guess many maintainers rely on (thorough)
reviews from third parties, and just glance over each patch before
sending? Doesn't mean that they can and do reviews, but I feel it's not a
requirement to do so *yourself* for *every* patch?
> There are however maintainers (see i2c patches or Intel DRM) who accept
> patches and do not review them. When they review, they provide
> additional Rb tag + Sob. This is weird because it means when they accept
> patch, they take it unreviewed! Their SoB does not imply reviewing patch
> and this is in contrast to kernel process.
>
> BTW, Stephen Rothwell mentions this to every maintainer on adding their
> tree to linux-next ("You will need to ensure that ... reviewed by you
> (or another maintainer of your subsystem tree)").
But this hints that there must be *some* review taking place, and it's the
maintainer's responsibility to ensure this. But that doesn't mean that the
maintainer cannot delegate? And then they would just forward patches
reviewed by trusted people.
> > they do agree on the patch (also typically expressed by an Ack), otherwise
> > they wouldn't send it, but a "review" is still a different thing.
>
> IMO, this would mean such maintainers accept code which they do not
> understand/review/care. They are just patch juggling monkeys who take
> something and push it further without doing actual work.
>
> That's not how maintainership should look like. Maintainer must take
> reviewed code and, if other maintainers do not review, then they must
> perform it.
Of course, but I feel this discussion goes into a different direction. I am
not a maintainer for the sunxi tree, I am a mere messenger here,
forwarding a patch. And I didn't think this implies an implicit review.
A did an explicit one, to stress that I did look into the patch more
thoroughly, and also because we are not exactly drowning in reviewers ;-)
> > The Linux history has both Rb + SoB from the same person and just SoB
> > signatures, so I assume that it's not implied.
>
> It depends on people. As I said, I2C and DRM provide Review tag. For me
> this is silly and suggest that all my work, that 1000 patches I took,
> was not reviewed.
If you forward a thousand patches, I wouldn't expect you did review all of
them *yourself*. This would be an almost impossible task, unless "review"
just means something like: "uses tabs for indentation".
> >> And you have there SoB which indicates you sent it...
> >
> > Yes, but SoB just means I sign off on the legal aspects: that I got the
> > patches legally, compliant with the GPL, and that I am fine with and
> > allowed to release them under GPL conditions.
> > That does not include any code review aspect, AFAICT.
>
> So you want to say, that you are fine in sending intentionally buggy
> code, knowingly incorrect, because your SoB and your "git send-email"
> does not mean you reviewed it?
Where did I claim that? Of course I would not send intentionally buggy
code, and of course I did some high level checking of the patch before I
attached my name to it. But to me this is still different from a proper
"review", hence my reply.
Cheers,
Andre
> Best regards,
> Krzysztof
>
>
^ permalink raw reply
* Re: [PATCH v2 0/3] HID: i2c-hid: Probe and wake device with HID descriptor fetch
From: Łukasz Majczak @ 2024-04-24 10:34 UTC (permalink / raw)
To: Kenny Levinsen
Cc: Jiri Kosina, Dmitry Torokhov, Benjamin Tissoires,
Douglas Anderson, Hans de Goede, Maxime Ripard, Kai-Heng Feng,
Johan Hovold, linux-input, linux-kernel, Radoslaw Biernacki
In-Reply-To: <20240423122518.34811-1-kl@kl.wtf>
On Tue, Apr 23, 2024 at 2:26 PM Kenny Levinsen <kl@kl.wtf> wrote:
>
> This revises my previous patch[0] to add the sleep STM chips seem to
> require as per discussion on the original patch from Lukasz and
> Radoslaw[1]. I had initially tried without as it had not previously been
> needed in the similar logic in our resume path, but it would appear that
> this was simply luck as the affected device was woken up in that case by
> "noise" from other sources.
>
> To reiterate, the idea is to add the retry that Lukasz and Radoslaw
> discovered was necessary, but do away with the dummy smbus probe and
> instead just let HID descriptor fetch retry as needed, aligning more
> with the existing retry logic used after resume while saving some noise
> on the bus and speeding up initialization a tiny bit.
>
> I added Co-developed-by tags, I hope that's appropriate. We should await
> an ACK from Lukasz on it fixing their hardware quirk.
>
> [0]: https://lore.kernel.org/all/20240415170517.18780-1-kl@kl.wtf/
> [1]: https://lore.kernel.org/all/CAE5UKNqPA4SnnXyaB7Hwk0kcKMMQ_DUuxogDphnnvSGP8g1nAQ@mail.gmail.com/
>
Hi Kenny,
Your solution works as it should - I have tested it on my Eve with
enabled debugs and
the retries works as expected with power-on, reboot and suspend/resume paths.
I have also disabled cros_ec_i2c driver to be 100% sure it doesn't do
any i2c transactions on the bus
and again the touchpad initialized successfully (with a retry) on all paths.
So you can add:
Tested-by: Lukasz Majczak <lma@chromium.org>
Reviewed-by: Lukasz Majczak <lma@chromium.org>
Thank you Kenny for your work :)
Best regards,
Lukasz
^ permalink raw reply
* Re: [PATCH v2 19/19] const_structs.checkpatch: add lcd_ops
From: Daniel Thompson @ 2024-04-24 10:08 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Lee Jones, Jingoo Han, Helge Deller, Bruno Prémont,
Jiri Kosina, Benjamin Tissoires, Alexander Shiyan, Sascha Hauer,
Pengutronix Kernel Team, Shawn Guo, Fabio Estevam, dri-devel,
linux-fbdev, linux-kernel, linux-input, linux-arm-kernel, imx,
linux-omap, Thomas Weißschuh
In-Reply-To: <20240424-video-backlight-lcd-ops-v2-19-1aaa82b07bc6@kernel.org>
On Wed, Apr 24, 2024 at 08:33:45AM +0200, Krzysztof Kozlowski wrote:
> 'struct lcd_ops' is not modified by core code.
>
> Suggested-by: Thomas Weißschuh <linux@weissschuh.net>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Reviewed-by: Daniel Thompson <daniel.thompson@linaro.org>
Daniel.
^ permalink raw reply
* Re: [PATCH 0/6] HID/arm64: dts: qcom: sc8280xp-x13s: fix touchscreen power on
From: Johan Hovold @ 2024-04-24 8:50 UTC (permalink / raw)
To: Doug Anderson
Cc: Johan Hovold, Jiri Kosina, Benjamin Tissoires, Dmitry Torokhov,
Rob Herring, Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson,
Konrad Dybcio, Linus Walleij, linux-input, devicetree,
linux-arm-msm, linux-kernel
In-Reply-To: <CAD=FV=W2Y=Sr-=YkKb01XLQsbQJr2b981c9kcfvAc4_5E9XD7g@mail.gmail.com>
On Tue, Apr 23, 2024 at 01:36:18PM -0700, Doug Anderson wrote:
> On Tue, Apr 23, 2024 at 6:46 AM Johan Hovold <johan+linaro@kernel.org> wrote:
> > The Elan eKTH5015M touch controller on the X13s requires a 300 ms delay
> > before sending commands after having deasserted reset during power on.
> >
> > This series switches the X13s devicetree to use the Elan specific
> > binding so that the OS can determine the required power-on sequence and
> > make sure that the controller is always detected during boot. [1]
> >
> > The Elan hid-i2c driver currently asserts reset unconditionally during
> > suspend, which does not work on the X13s where the touch controller
> > supply is shared with other peripherals that may remain powered. Holding
> > the controller in reset can increase power consumption and also leaks
> > current through the reset circuitry pull ups.
>
> Can you provide more details about which devices exactly it shares
> power with? I'm worried that you may be shooting yourself in the foot
> to avoid shooting yourself in the arm.
>
> Specifically, if those other peripherals that may remain powered ever
> power themselves off then you'll end up back-driving the touchscreen
> through the reset line, won't you? Since reset is active low then not
> asserting reset drives the reset line high and, if you power it off,
> it can leach power backwards through the reset line. The
> "goodix,no-reset-during-suspend" property that I added earlier
> specifically worked on systems where the rail was always-on so I could
> guarantee that didn't happen.
>
> From looking at your dts patch it looks like your power _is_ on an
> always-on rail so you should be OK, but it should be documented that
> this only works for always-on rails.
>
> ..also, from your patch description it sounds as if (maybe?) you
> intend to eventually let the rail power off if the trackpad isn't a
> wakeup source. If you eventually plan to do that then you definitely
> need something more complex here...
No, that's the whole point: the hardware is designed so that the reset
line can be left deasserted by the CPU also when the supply is off.
The supply in this case is shared with the keyboard and touchpad, but
also some other devices which are not yet fully described. As you
rightly noted, the intention is to allow the supply to eventually be
disabled when none of these devices are enabled as wakeup sources.
I did not want to get in to too much details on exactly how this
particular reset circuit is designed, but basically you have a pull up
to an always-on 1.8 V rail on the CPU side, a FET level shifter, and a
pull up to the supply voltage on the peripheral side.
With this design, the reset line can be left deasserted by the CPU
(tri-stated or driven high), but the important part is that the reset
signal that goes into the controller will be pulled to 3.3 V only when
the supply is left on and otherwise it will be connected to ground.
> > Note that the latter also affects X13s variants where the touchscreen is
> > not populated as the driver also exits probe() with reset asserted.
>
> I assume driving against an external pull is _probably_ not a huge
> deal (should be a pretty small amount of power), but I agree it would
> be nice to fix.
>
> I'm a bit leery of actively driving the reset pin high (deasserting
> the reset) just to match the pull. It feels like in your case it would
> be better to make it an input w/ no pulls. It almost feels like
> something in the pinctrl system should handle this. Something where
> the pin is default "input no pull" at the board level and when the
> driver exits it should go back to the pinctrl default...
If you look at the DT patch that's essentially what I'm doing by
describing the reset pin as open-drain so that it will be configured as
an input (tristated) when reset is deasserted and only driven low when
reset is asserted.
> I guess one last thought is: what do we do if/when someone needs the
> same solution but they want multiple sources of touchscreens, assuming
> we ever get the second-sourcing problem solved well. In that case the
> different touchscreen drivers might have a different idea of how the
> GPIO should be left when the driver exits...
The second-source problem is arguable a separate one, and as we've
discussed in the past, the current approach of describing both devices
in the devicetree only works when the devices are truly compatible in
terms of external resources (supplies, gpios, pinconfig). For anything
more complex, we need a more elaborate implementation.
In this case it should not be a problem, though, as the reset circuit
should have the same properties regardless of which controller you
connect (e.g. both nodes would have the 'no-reset-on-power-off'
property).
Johan
^ 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