| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <4f2aa5a1-d8db-0fa9-3104-5e4b2a036b36@pengutronix.de>
Date: Tue, 3 Sep 2019 09:43:25 +0200
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: Anson Huang <anson.huang@....com>,
"robh+dt@...nel.org" <robh+dt@...nel.org>,
"mark.rutland@....com" <mark.rutland@....com>,
"shawnguo@...nel.org" <shawnguo@...nel.org>,
"s.hauer@...gutronix.de" <s.hauer@...gutronix.de>,
"kernel@...gutronix.de" <kernel@...gutronix.de>,
"festevam@...il.com" <festevam@...il.com>,
"catalin.marinas@....com" <catalin.marinas@....com>,
"will@...nel.org" <will@...nel.org>,
"dmitry.torokhov@...il.com" <dmitry.torokhov@...il.com>,
Aisheng Dong <aisheng.dong@....com>,
"ulf.hansson@...aro.org" <ulf.hansson@...aro.org>,
Andy Duan <fugang.duan@....com>, Peng Fan <peng.fan@....com>,
Daniel Baluta <daniel.baluta@....com>,
Leonard Crestez <leonard.crestez@....com>,
"mripard@...nel.org" <mripard@...nel.org>,
"olof@...om.net" <olof@...om.net>, "arnd@...db.de" <arnd@...db.de>,
"jagan@...rulasolutions.com" <jagan@...rulasolutions.com>,
"bjorn.andersson@...aro.org" <bjorn.andersson@...aro.org>,
"dinguyen@...nel.org" <dinguyen@...nel.org>,
"marcin.juszkiewicz@...aro.org" <marcin.juszkiewicz@...aro.org>,
"stefan@...er.ch" <stefan@...er.ch>,
"gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
"andriy.shevchenko@...ux.intel.com"
<andriy.shevchenko@...ux.intel.com>,
"yuehaibing@...wei.com" <yuehaibing@...wei.com>,
"tglx@...utronix.de" <tglx@...utronix.de>,
"ronald@...ovation.ch" <ronald@...ovation.ch>,
"m.felsch@...gutronix.de" <m.felsch@...gutronix.de>,
Jacky Bai <ping.bai@....com>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-arm-kernel@...ts.infradead.org"
<linux-arm-kernel@...ts.infradead.org>,
"linux-input@...r.kernel.org" <linux-input@...r.kernel.org>
Cc: dl-linux-imx <linux-imx@....com>
Subject: Re: [PATCH V2 2/5] input: keyboard: imx_sc: Add i.MX system
controller power key support
On 03.09.19 09:35, Anson Huang wrote:
> Hi, Oleksij
>
>> On 03.09.19 08:48, Anson Huang wrote:
>>> Hi, Oleksij
>>>
>>>> On 03.09.19 16:03, Anson Huang wrote:
>>>>> i.MX8QXP is an ARMv8 SoC which has a Cortex-M4 system controller
>>>>> inside, the system controller is in charge of controlling power,
>>>>> clock and power key etc..
>>>>>
>>>>> Adds i.MX system controller power key driver support, Linux kernel
>>>>> has to communicate with system controller via MU (message unit) IPC
>>>>> to get power key's status.
>>>>>
>>>>> Signed-off-by: Anson Huang <Anson.Huang@....com>
>>>>> ---
>>>>> Changes since V1:
>>>>> - remove "wakeup-source" property operation, scu power key uses
>>>> generic scu irq,
>>>>> no need to have this property for device wakeup operation.
>>>>> ---
>>>>> drivers/input/keyboard/Kconfig | 7 ++
>>>>> drivers/input/keyboard/Makefile | 1 +
>>>>> drivers/input/keyboard/imx_sc_pwrkey.c | 169
>>>> +++++++++++++++++++++++++++++++++
>>>>> 3 files changed, 177 insertions(+)
>>>>> create mode 100644 drivers/input/keyboard/imx_sc_pwrkey.c
>>>>>
>>>>> diff --git a/drivers/input/keyboard/Kconfig
>>>>> b/drivers/input/keyboard/Kconfig index 2e6d288..3aaeb9c 100644
>>>>> --- a/drivers/input/keyboard/Kconfig
>>>>> +++ b/drivers/input/keyboard/Kconfig
>>>>> @@ -469,6 +469,13 @@ config KEYBOARD_IMX
>>>>> To compile this driver as a module, choose M here: the
>>>>> module will be called imx_keypad.
>>>>>
>>>>> +config KEYBOARD_IMX_SC_PWRKEY
>>>>> + tristate "IMX SCU Power Key Driver"
>>>>> + depends on IMX_SCU
>>>>> + help
>>>>> + This is the system controller powerkey driver for NXP i.MX SoCs with
>>>>> + system controller inside.
>>>>
>>>> The KEY is configurable over devicetree, why is it called PWRKEY? It
>>>> looks for me as generic SCU key handler.
>>>
>>> We always use it as power key, NOT a generic key, as it has HW
>>> function designed for power key purpose.
>>
>> gpio-key driver is mostly used for power or reboot key. And it is still called
>> gpio-key driver. If it is used for power key only, why is it configurable? I can
>> configure it as KEY_ENTER or some thing different. This driver has not
>> KEY_POWER specific any thing.
>
> Understood, I am making the V3 with all "power" removed, just using the "key".
>
>>
>>>
>>>>
>>>>> config KEYBOARD_NEWTON
>>>>> tristate "Newton keyboard"
>>>>> select SERIO
>>>>> diff --git a/drivers/input/keyboard/Makefile
>>>>> b/drivers/input/keyboard/Makefile index 9510325..9ea5585 100644
>>>>> --- a/drivers/input/keyboard/Makefile
>>>>> +++ b/drivers/input/keyboard/Makefile
>>>>> @@ -29,6 +29,7 @@ obj-$(CONFIG_KEYBOARD_HIL) +=
>> hil_kbd.o
>>>>> obj-$(CONFIG_KEYBOARD_HIL_OLD) += hilkbd.o
>>>>> obj-$(CONFIG_KEYBOARD_IPAQ_MICRO) += ipaq-micro-keys.o
>>>>> obj-$(CONFIG_KEYBOARD_IMX) += imx_keypad.o
>>>>> +obj-$(CONFIG_KEYBOARD_IMX_SC_PWRKEY) += imx_sc_pwrkey.o
>>>>> obj-$(CONFIG_KEYBOARD_HP6XX) += jornada680_kbd.o
>>>>> obj-$(CONFIG_KEYBOARD_HP7XX) += jornada720_kbd.o
>>>>> obj-$(CONFIG_KEYBOARD_LKKBD) += lkkbd.o
>>>>> diff --git a/drivers/input/keyboard/imx_sc_pwrkey.c
>>>>> b/drivers/input/keyboard/imx_sc_pwrkey.c
>>>>> new file mode 100644
>>>>> index 0000000..53aa9a4
>>>>> --- /dev/null
>>>>> +++ b/drivers/input/keyboard/imx_sc_pwrkey.c
>>>>> @@ -0,0 +1,169 @@
>>>>> +// SPDX-License-Identifier: GPL-2.0
>>>>> +/*
>>>>> + * Copyright 2019 NXP.
>>>>> + */
>>>>> +
>>>>> +#include <linux/device.h>
>>>>> +#include <linux/err.h>
>>>>> +#include <linux/firmware/imx/sci.h> #include <linux/init.h>
>>>>> +#include <linux/input.h> #include <linux/interrupt.h> #include
>>>>> +<linux/jiffies.h> #include <linux/kernel.h> #include
>>>>> +<linux/module.h> #include <linux/of.h> #include
>>>>> +<linux/of_address.h> #include <linux/platform_device.h>
>>>>> +
>>>>> +#define DEBOUNCE_TIME 100
>>>>> +#define REPEAT_INTERVAL 60
>>>>> +
>>>>> +#define SC_IRQ_BUTTON 1
>>>>> +#define SC_IRQ_GROUP_WAKE 3
>>>>> +#define IMX_SC_MISC_FUNC_GET_BUTTON_STATUS 18
>>>>> +
>>>>> +struct imx_pwrkey_drv_data {
>>>>> + int keycode;
>>>>> + bool keystate; /* 1: pressed, 0: release */
>>>>> + bool delay_check;
>>>>> + struct delayed_work check_work;
>>>>> + struct input_dev *input;
>>>>> +};
>>>>> +
>>>>> +struct imx_sc_msg_pwrkey {
>>>>> + struct imx_sc_rpc_msg hdr;
>>>>> + u8 state;
>>>>> +};
>>>>> +static struct imx_pwrkey_drv_data *pdata;
>>>>
>>>> Why is it global struct? It seems to be flexible configurable over devicetree.
>>>> So I would assume it should be able to handle more then one button.
>>>> Please remove global variables, make it allocatable per OF node.
>>>
>>> There is ONLY one button available for SC key, but yes, I think I can
>>> make the structure private and get all necessary data from the structure
>> using container_of.
>>
>> And we will never need more then 640 kB RAM ;)
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fen.wi
>> kiquote.org%2Fwiki%2FTalk%3ABill_Gates&data=02%7C01%7Canson.hu
>> ang%40nxp.com%7C4d4d7458087747e0d8f008d7304057e9%7C686ea1d3bc2
>> b4c6fa92cd99c5c301635%7C0%7C0%7C637030925236150243&sdata=w
>> %2FGXBaHfnBdLwjTxjbzWSPeIw3ExL%2Fs9IMOgF1onL6A%3D&reserved
>> =0
>>
>>>
>>>>
>>>> Please use different name "pdata" is usually used as platform data.
>>>> Please, use "priv".
>>>
>>> OK.
>>>
>>>>
>>>>> +static struct imx_sc_ipc *pwrkey_ipc_handle;
>>>>
>>>> same as before, no global variables.
>>>
>>> Will move it into private platform data structure.
>>>
>>>>
>>>>> +
>>>>> +static int imx_sc_pwrkey_notify(struct notifier_block *nb,
>>>>> + unsigned long event, void *group) {
>>>>> + if ((event & SC_IRQ_BUTTON) && (*(u8 *)group ==
>>>> SC_IRQ_GROUP_WAKE)
>>>>> + && !pdata->delay_check) {
>>>>> + pdata->delay_check = 1;
>>>>> + schedule_delayed_work(&pdata->check_work,
>>>>> + msecs_to_jiffies(REPEAT_INTERVAL));
>>>>> + }
>>>>> +
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static void imx_sc_check_for_events(struct work_struct *work) {
>>>>> + struct input_dev *input = pdata->input;
>>>>> + struct imx_sc_msg_pwrkey msg;
>>>>> + struct imx_sc_rpc_msg *hdr = &msg.hdr;
>>>>> + bool state;
>>>>> +
>>>>> + hdr->ver = IMX_SC_RPC_VERSION;
>>>>> + hdr->svc = IMX_SC_RPC_SVC_MISC;
>>>>> + hdr->func = IMX_SC_MISC_FUNC_GET_BUTTON_STATUS;
>>>>> + hdr->size = 1;
>>>>> +
>>>>> + /*
>>>>> + * Current SCU firmware does NOT have return value for
>>>>> + * this API, that means it is always successful.
>>>>> + */
>>>>
>>>> It is not true for the kernel part:
>>>> https://elixir.
>>>>
>> bootlin.com%2Flinux%2Flatest%2Fsource%2Fdrivers%2Ffirmware%2Fimx%2F
>>>> imx-
>>>>
>> scu.c%23L157&data=02%7C01%7Canson.huang%40nxp.com%7C7a5ed3
>>>>
>> ef3b2541e61be808d7303810a9%7C686ea1d3bc2b4c6fa92cd99c5c301635%7C
>>>>
>> 0%7C0%7C637030889669489141&sdata=d3uw6x6WCPeavJu3QYf9o9cxx
>>>> Rx4mJar04fQFLF9EhE%3D&reserved=0
>>>>
>>>> imx_scu_call_rpc() may fail in different ways and provide proper error
>> value.
>>>> Please use it.
>>>
>>> There are about 3 APIs are special, this API is one of them, this API
>>> has no return value from SCU FW API, but it has response data from it,
>>> so that means if we set the response to false, the stack will be free
>>> and mailbox will have NULL pointer issue when response data passed
>>> from SCU FW. If we set the response to true, as the SCU FW has no
>>> return value, the return value will be the msg->func which will be
>>> already failed, that is why we have to skip the return value check. This is
>> one restriction/bug of SCU FW, we will notify SCU FW owner to fix/improve.
>>
>> Ok, I see. imx_scu_call_rpc() can return kernel side errors, for example from
>> imx-scu.c framework EINVAL or ETIMEDOUT or what ever error mbox
>> framework may also provide.
>> Aaaannnndd... it can extract an error from SCU package and return it over
>> same way as other errors.
>>
>> And current SCU version has some bugs, so it is providing wrong error value.
>> Soo... as usual the NXP has decided to make the linux kernel a bit more
>> worse to make the SCU firmware happy? Is it what you trying to describe?
>> Really ?! :D
>>
>> Please. Fix the SCU first. The provide fixed kernel patch.
>
> Understood, I will notify SCU owner to fix it, meanwhile it does NOT block this driver,
> I will add return value check in this driver.
It is great! Thank you!
Kind regards,
Oleksij Rempel
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Powered by blists - more mailing lists