Re: [PATCH v3 00/12] add IRQF_NO_AUTOEN for request_irq

[dmitry.torok...@gmail.com]

On Wed, Jan 27, 2021 at 02:49:21PM +0100, gre...@linuxfoundation.org wrote:
> On Thu, Jan 21, 2021 at 09:38:28PM +, Song Bao Hua (Barry Song) wrote:
> > Hi Thomas, Greg, Dmitry, Marc,
> > Any further comment on this new API? 
> 
> It's not my subsystem, I'll let the irq maintainers handle it :)

Not my subsystem either, but I would like to have this feature
available. I do not like calling irq_set_status_flags() before
request_irq() as at that time we are not ensured of irq ownership, and
using disable_irq() afterwards is indeed awkward.

Thanks.

-- 
Dmitry

Re: [PATCH] FROMLIST: input: add 2 kind of switch

[dmitry.torok...@gmail.com]

On Wed, Oct 21, 2020 at 07:10:35AM +0200, gre...@linuxfoundation.org wrote:
> On Wed, Oct 21, 2020 at 12:12:16PM +0900, HyungJae Im wrote:
> > >From ec9859ee01b7bc0e04255971e0fe97348847dab7 Mon Sep 17 00:00:00 2001
> 
> You sent this 3 times, why?
> 
> And why is this in the body of the email, have you read the "how to send
> your first kernel patch" document at kernelnewbies.org?
> 
> > From: "hj2.im" 
> > Date: Tue, 20 Oct 2020 16:57:04 +0900
> > Subject: [PATCH] FROMLIST: input: add 2 kind of switch
> 
> What does "FROMLIST:" mean?
> 
> > 
> > We need support to various accessories on the device,
> > some switch does not exist in switch list.
> > So added switch for the following purpose.
> > 
> > SW_COVER_ATTACHED is for the checking the cover
> > attached or not on the device. SW_EXT_PEN_ATTACHED is for the
> > checking the external pen attached or not on the device
> > 
> > Signed-off-by: hj2.im 
> 
> As per the kernel documentation, you need to use your real name here,
> please do so.
> 
> > ---
> >  include/linux/mod_devicetable.h| 2 +-
> >  include/uapi/linux/input-event-codes.h | 4 +++-
> >  2 files changed, 4 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/mod_devicetable.h 
> > b/include/linux/mod_devicetable.h
> > index 5b08a473cdba..897f5a3e7721 100644
> > --- a/include/linux/mod_devicetable.h
> > +++ b/include/linux/mod_devicetable.h
> > @@ -320,7 +320,7 @@ struct pcmcia_device_id {
> >  #define INPUT_DEVICE_ID_LED_MAX0x0f
> >  #define INPUT_DEVICE_ID_SND_MAX0x07
> >  #define INPUT_DEVICE_ID_FF_MAX 0x7f
> > -#define INPUT_DEVICE_ID_SW_MAX 0x10
> > +#define INPUT_DEVICE_ID_SW_MAX 0x12
> >  #define INPUT_DEVICE_ID_PROP_MAX   0x1f
> >  
> >  #define INPUT_DEVICE_ID_MATCH_BUS  1
> > diff --git a/include/uapi/linux/input-event-codes.h 
> > b/include/uapi/linux/input-event-codes.h
> > index 0c2e27d28e0a..8ca2acee1f92 100644
> > --- a/include/uapi/linux/input-event-codes.h
> > +++ b/include/uapi/linux/input-event-codes.h
> > @@ -889,7 +889,9 @@
> >  #define SW_MUTE_DEVICE 0x0e  /* set = device disabled */
> >  #define SW_PEN_INSERTED0x0f  /* set = pen inserted */
> >  #define SW_MACHINE_COVER   0x10  /* set = cover closed */
> > -#define SW_MAX 0x10
> > +#define SW_COVER_ATTACHED  0x11  /* set = cover attached */
> > +#define SW_EXT_PEN_ATTACHED0x12  /* set = external pen attached */
> 
> Is there an in-kernel user for these values anywhere?  Please submit
> this patch along with the users at the same time, otherwise this change
> makes no sense at all.

It kind of does, as nowadays most uses come from DT, not from
hard-coding in drivers. However I need to better understand the intended
use of this. How SW_COVER_ATTACHED differs from SW_MACHINE_COVER and the
same for SW_PEN_INSERTED SW_EXT_PEN_ATTACHED.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: hyperv-keyboard: Add the support of hibernation

[dmitry.torok...@gmail.com]

On Thu, Oct 03, 2019 at 06:44:04AM +, Dexuan Cui wrote:
> > From: Dexuan Cui
> > Sent: Wednesday, October 2, 2019 10:35 PM
> > > ... 
> > >
> > > ¯\_(ツ)_/¯ If you do not want to implement hibernation properly in vmbus
> > > code that is totally up to you (have you read in pm.h how freeze() is
> > > different from suspend()?).
> > > Dmitry
> > 
> > I understand freeze() is different from suspend(). Here I treat suspend() 
> > as a
> > heavyweight freeze() for simplicity and IMHO the extra cost of time is
> > neglectable considering the long hibernation process, which can take
> > 5~10+ seconds.
> > 
> > Even if I implement all the pm ops, IMO the issue we're talking about
> > (i.e. the hibernation process can be aborted by user's keyboard/mouse
> > activities) still exists. Actually I think a physical Linux machine should 
> > have
> > the same issue.
> > 
> > In practice, IMO the issue is not a big concern, as the VM usually runs in
> > a remote data center, and the user has no access to the VM's
> > keyboard/mouse. :-)
> > 
> > I hope I understood your comments. I'll post a v2 without the notifier.
> > Please Ack the v2 if it looks good to you.
> > 
> > -- Dexuan
> 
> I think I understood now: it looks the vmbus driver should implement
> a prepare() or freeze(), which calls the hyperv_keyboard driver's
> prepare() or freeze(), which can set the flag or disable the keyboard
> event handling. This way we don't need the notifier.

Right. I think in practice the current suspend implementation can work
as freeze() for the HV keyboard, because in suspend you shut off vmbus
channel, so there should not be wakeup signals anymore. What you do not
want is to have the current resume to be used in place of thaw(), as
there you re-enable the vmbus channel and resume sending wakeup requests
as you are writing out the hibernation image to storage.

I think if vmbus allowed HV keyboard driver to supply empty thaw() and
poweroff() implementations, while using suspend() as freeze() and
resume() as restore(), it would solve the issue for you.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: hyperv-keyboard: Add the support of hibernation

[dmitry.torok...@gmail.com]

On Mon, Sep 30, 2019 at 10:09:27PM +, Dexuan Cui wrote:
> > From: dmitry.torok...@gmail.com 
> > Sent: Friday, September 27, 2019 5:32 PM
> > > ...
> > > pm_wakeup_pending() is tested in a lot of places in the suspend
> > > process and eventually an unintentional keystroke (or mouse movement,
> > > when it comes to the Hyper-V mouse driver drivers/hid/hid-hyperv.c)
> > > causes the whole hibernation process to be aborted. Usually this
> > > behavior is not expected by the user, I think.
> > 
> > Why not? If a device is configured as wakeup source, then it activity
> > should wake up the system, unless you disable it.
> 
> Generally speaking, I agree, but compared to a physical machine, IMO 
> the scenario is a little differnet when it comes to a VM running on Hyper-V:
> on the host there is a window that represents the VM, and the user can
> unintentionally switch the keyboard input focus to the window (or move
> the mouse/cursor over the window) and then the host automatically 
> sends some special keystrokes (and mouse events) , and this aborts the
> hibernation process.  
> 
> And, when it comes to the Hyper-V mouse device, IMO it's easy for the
> user to unintentionally move the mouse after the "hibernation" button
> is clicked. I suppose a physical machine would have the same issue, though.

If waking the machine up by mouse/keyboard activity is not desired in
Hyper-V environment, then simply disable them as wakeup sources.

> 
> > > So, I use the notifier to set the flag variable and with it the driver can
> > > know when it should not call pm_wakeup_hard_event().
> > 
> > No, please implement hibernation support properly, as notifier + flag is
> > a hack. 
> 
> The keyboard/mouse driver can avoid the flag by disabling the 
> keyboard/mouse event handling, but the problem is that they don't know
> when exactly they should disable the event handling. I think the PM
> notifier is the only way to tell the drivers a hibernation process is ongoing.

Whatever initiates hibernation (in userspace) can adjust wakeup sources
as needed if you want them disabled completely.

> 
> Do you think this idea (notifer + disabling event handling) is acceptable?

No, I believe this a hack, that is why I am pushing back on this.

> 
> If not, then I'll have to remove the notifer completely, and document this as
> a known issue to the user: when a hibernation process is started, be careful
> to not switch input focus and not touch the keyboard/mouse until the
> hibernation process is finished. :-)
> 
> > In this particular case you do not want to have your
> > hv_kbd_resume() to be called in place of pm_ops->thaw() as that is what
> > reenables the keyboard vmbus channel and causes the undesired wakeup
> > events. 
> 
> This is only part of the issues. Another example: before the
> pm_ops()->freeze()'s of all the devices are called, pm_wakeup_pending()
> is already tested in a lot of places (e.g. in try_to_freeze_tasks ()) in the 
> suspend process, and can abort the whole suspend process upon the user's
> unintentional input focus switch, keystroke and mouse movement.

How long is the prepare() phase on your systems? User may wiggle mouse
at any time really, even before the notifier fires up.

> 
> > Your vmbus implementation should allow individual drivers to
> > control the set of PM operations that they wish to use, instead of
> > forcing everything through suspend/resume.
> > 
> > Dmitry
> 
> Since the devices are pure software-emulated devices, no PM operation was
> supported in the past, and now suspend/resume are the only two PM operations
> we're going to support. If the idea (notifer + disabling event handling) is 
> not
> good enough, we'll have to document the issue to the user, as I described 
> above.

¯\_(ツ)_/¯ If you do not want to implement hibernation properly in vmbus
code that is totally up to you (have you read in pm.h how freeze() is
different from suspend()?).

Thanks.

-- 
Dmitry

Re: [PATCH] Input: hyperv-keyboard: Add the support of hibernation

[dmitry.torok...@gmail.com]

On Sat, Sep 21, 2019 at 06:56:04AM +, Dexuan Cui wrote:
> > From: dmitry.torok...@gmail.com 
> > Sent: Thursday, September 19, 2019 9:18 AM
> > 
> > Hi Dexuan,
> > 
> > On Wed, Sep 11, 2019 at 11:36:20PM +, Dexuan Cui wrote:
> > > We need hv_kbd_pm_notify() to make sure the pm_wakeup_hard_event()
> > call
> > > does not prevent the system from entering hibernation: the hibernation
> > > is a relatively long process, which can be aborted by the call
> > > pm_wakeup_hard_event(), which is invoked upon keyboard events.
> > >
> > > diff --git a/drivers/input/serio/hyperv-keyboard.c
> > b/drivers/input/serio/hyperv-keyboard.c
> > > index 88ae7c2..277dc4c 100644
> > > --- a/drivers/input/serio/hyperv-keyboard.c
> > > +++ b/drivers/input/serio/hyperv-keyboard.c
> > > @@ -10,6 +10,7 @@
> > >  #include 
> > >  #include 
> > >  #include 
> > > +#include 
> > >
> > >  /*
> > >   * Current version 1.0
> > > @@ -95,6 +96,9 @@ struct hv_kbd_dev {
> > >   struct completion wait_event;
> > >   spinlock_t lock; /* protects 'started' field */
> > >   bool started;
> > > +
> > > + struct notifier_block pm_nb;
> > > + bool hibernation_in_progress;
> > 
> > Why do you use notifier block instead of exposing proper PM methods if
> > you want to support hibernation?
> > 
> > Dmitry
> 
> Hi,
> In the patch I do implement hv_kbd_suspend() and hv_kbd_resume(), and
> add them into the hv_kbd_drv struct:
> 
> @@ -416,6 +472,8 @@ static struct  hv_driver hv_kbd_drv = {
> .id_table = id_table,
> .probe = hv_kbd_probe,
> .remove = hv_kbd_remove,
> +   .suspend = hv_kbd_suspend,
> +   .resume = hv_kbd_resume,
> 
> The .suspend and .resume callbacks are inroduced by another patch (which
> uses the dev_pm_ops struct):
> 271b2224d42f ("Drivers: hv: vmbus: Implement suspend/resume for VSC drivers 
> for hibernation")
> (which is on the Hyper-V tree's hyperv-next branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/commit/?h=hyperv-next=271b2224d42f88870e6b060924ee374871c131fc
>  )
> 
> The only purpose of the notifier is to set the variable 
> kbd_dev->hibernation_in_progress to true during the hibernation process.
> 
> As I explained in the changelog, the hibernation is a long process (which
> can take 10+ seconds), during which the user may unintentionally touch
> the keyboard, causing key up/down events, which are still handled by
> hv_kbd_on_receive(), which calls pm_wakeup_hard_event(), which
> calls some other functions which increase the global counter
> "pm_abort_suspend", and hence pm_wakeup_pending() becomes true.
> 
> pm_wakeup_pending() is tested in a lot of places in the suspend
> process and eventually an unintentional keystroke (or mouse movement,
> when it comes to the Hyper-V mouse driver drivers/hid/hid-hyperv.c)
> causes the whole hibernation process to be aborted. Usually this
> behavior is not expected by the user, I think.

Why not? If a device is configured as wakeup source, then it activity
should wake up the system, unless you disable it.

> 
> So, I use the notifier to set the flag variable and with it the driver can
> know when it should not call pm_wakeup_hard_event().

No, please implement hibernation support properly, as notifier + flag is
a hack. In this particular case you do not want to have your
hv_kbd_resume() to be called in place of pm_ops->thaw() as that is what
reenables the keyboard vmbus channel and causes the undesired wakeup
events. Your vmbus implementation should allow individual drivers to
control the set of PM operations that they wish to use, instead of
forcing everything through suspend/resume.

Thanks.

-- 
Dmitry

Re: [PATCH] Input: hyperv-keyboard: Add the support of hibernation

[dmitry.torok...@gmail.com]

Hi Dexuan,

On Wed, Sep 11, 2019 at 11:36:20PM +, Dexuan Cui wrote:
> We need hv_kbd_pm_notify() to make sure the pm_wakeup_hard_event() call
> does not prevent the system from entering hibernation: the hibernation
> is a relatively long process, which can be aborted by the call
> pm_wakeup_hard_event(), which is invoked upon keyboard events.
> 
> Signed-off-by: Dexuan Cui 
> ---
> 
> This patch is basically a pure Hyper-V specific change and it has a
> build dependency on the commit 271b2224d42f ("Drivers: hv: vmbus: Implement
> suspend/resume for VSC drivers for hibernation"), which is on Sasha Levin's
> Hyper-V tree's hyperv-next branch:
> https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git/log/?h=hyperv-next
> 
> I request this patch should go through Sasha's tree rather than the
> input subsystemi's tree.
> 
> Hi Dmitry, can you please Ack?
> 
>  drivers/input/serio/hyperv-keyboard.c | 68 
> ---
>  1 file changed, 63 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/input/serio/hyperv-keyboard.c 
> b/drivers/input/serio/hyperv-keyboard.c
> index 88ae7c2..277dc4c 100644
> --- a/drivers/input/serio/hyperv-keyboard.c
> +++ b/drivers/input/serio/hyperv-keyboard.c
> @@ -10,6 +10,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  
>  /*
>   * Current version 1.0
> @@ -95,6 +96,9 @@ struct hv_kbd_dev {
>   struct completion wait_event;
>   spinlock_t lock; /* protects 'started' field */
>   bool started;
> +
> + struct notifier_block pm_nb;
> + bool hibernation_in_progress;

Why do you use notifier block instead of exposing proper PM methods if
you want to support hibernation?

Thanks.

-- 
Dmitry

Re: [PATCH] Input: hyperv-keyboard: Use in-place iterator API in the channel callback

[dmitry.torok...@gmail.com]

On Tue, Aug 20, 2019 at 03:01:23AM +, Dexuan Cui wrote:
> Simplify the ring buffer handling with the in-place API.
> 
> Also avoid the dynamic allocation and the memory leak in the channel
> callback function.
> 
> Signed-off-by: Dexuan Cui 
> ---
> 
> Hi Dmitry, can this patch go through Sasha's hyperv tree:
> https://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git
> 
> This is a purely Hyper-V specific change.

Sure, no problem.

Acked-by: Dmitry Torokhov 

> 
>  drivers/input/serio/hyperv-keyboard.c | 35 
> ++-
>  1 file changed, 6 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/input/serio/hyperv-keyboard.c 
> b/drivers/input/serio/hyperv-keyboard.c
> index 88ae7c2..e486a8a 100644
> --- a/drivers/input/serio/hyperv-keyboard.c
> +++ b/drivers/input/serio/hyperv-keyboard.c
> @@ -237,40 +237,17 @@ static void hv_kbd_handle_received_packet(struct 
> hv_device *hv_dev,
>  
>  static void hv_kbd_on_channel_callback(void *context)
>  {
> + struct vmpacket_descriptor *desc;
>   struct hv_device *hv_dev = context;
> - void *buffer;
> - int bufferlen = 0x100; /* Start with sensible size */
>   u32 bytes_recvd;
>   u64 req_id;
> - int error;
>  
> - buffer = kmalloc(bufferlen, GFP_ATOMIC);
> - if (!buffer)
> - return;
> -
> - while (1) {
> - error = vmbus_recvpacket_raw(hv_dev->channel, buffer, bufferlen,
> -  _recvd, _id);
> - switch (error) {
> - case 0:
> - if (bytes_recvd == 0) {
> - kfree(buffer);
> - return;
> - }
> -
> - hv_kbd_handle_received_packet(hv_dev, buffer,
> -   bytes_recvd, req_id);
> - break;
> + foreach_vmbus_pkt(desc, hv_dev->channel) {
> + bytes_recvd = desc->len8 * 8;
> + req_id = desc->trans_id;
>  
> - case -ENOBUFS:
> - kfree(buffer);
> - /* Handle large packet */
> - bufferlen = bytes_recvd;
> - buffer = kmalloc(bytes_recvd, GFP_ATOMIC);
> - if (!buffer)
> - return;
> - break;
> - }
> + hv_kbd_handle_received_packet(hv_dev, desc, bytes_recvd,
> +   req_id);
>   }
>  }
>  
> -- 
> 1.8.3.1
> 

-- 
Dmitry

Re: [RESEND] input: keyboard: imx: make sure keyboard can always wake up system

[dmitry.torok...@gmail.com]

On Thu, Apr 04, 2019 at 01:40:16AM +, Anson Huang wrote:
> There are several scenarios that keyboard can NOT wake up system
> from suspend, e.g., if a keyboard is depressed between system
> device suspend phase and device noirq suspend phase, the keyboard
> ISR will be called and both keyboard depress and release interrupts
> will be disabled, then keyboard will no longer be able to wake up
> system. Another scenario would be, if a keyboard is kept depressed,
> and then system goes into suspend, the expected behavior would be
> when keyboard is released, system will be waked up, but current
> implementation can NOT achieve that, because both depress and release
> interrupts are disabled in ISR, and the event check is still in
> progress.
> 
> To fix these issues, need to make sure keyboard's depress or release
> interrupt is enabled after noirq device suspend phase, this patch
> moves the suspend/resume callback to noirq suspend/resume phase, and
> enable the corresponding interrupt according to current keyboard status.
> 
> Signed-off-by: Anson Huang 

Applied, thank you.

> ---
>  drivers/input/keyboard/imx_keypad.c | 18 ++
>  1 file changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/input/keyboard/imx_keypad.c 
> b/drivers/input/keyboard/imx_keypad.c
> index cf08f4a..97500a2 100644
> --- a/drivers/input/keyboard/imx_keypad.c
> +++ b/drivers/input/keyboard/imx_keypad.c
> @@ -524,11 +524,12 @@ static int imx_keypad_probe(struct platform_device 
> *pdev)
>   return 0;
>  }
>  
> -static int __maybe_unused imx_kbd_suspend(struct device *dev)
> +static int __maybe_unused imx_kbd_noirq_suspend(struct device *dev)
>  {
>   struct platform_device *pdev = to_platform_device(dev);
>   struct imx_keypad *kbd = platform_get_drvdata(pdev);
>   struct input_dev *input_dev = kbd->input_dev;
> + unsigned short reg_val = readw(kbd->mmio_base + KPSR);
>  
>   /* imx kbd can wake up system even clock is disabled */
>   mutex_lock(_dev->mutex);
> @@ -538,13 +539,20 @@ static int __maybe_unused imx_kbd_suspend(struct device 
> *dev)
>  
>   mutex_unlock(_dev->mutex);
>  
> - if (device_may_wakeup(>dev))
> + if (device_may_wakeup(>dev)) {
> + if (reg_val & KBD_STAT_KPKD)
> + reg_val |= KBD_STAT_KRIE;
> + if (reg_val & KBD_STAT_KPKR)
> + reg_val |= KBD_STAT_KDIE;
> + writew(reg_val, kbd->mmio_base + KPSR);
> +
>   enable_irq_wake(kbd->irq);
> + }
>  
>   return 0;
>  }
>  
> -static int __maybe_unused imx_kbd_resume(struct device *dev)
> +static int __maybe_unused imx_kbd_noirq_resume(struct device *dev)
>  {
>   struct platform_device *pdev = to_platform_device(dev);
>   struct imx_keypad *kbd = platform_get_drvdata(pdev);
> @@ -568,7 +576,9 @@ static int __maybe_unused imx_kbd_resume(struct device 
> *dev)
>   return ret;
>  }
>  
> -static SIMPLE_DEV_PM_OPS(imx_kbd_pm_ops, imx_kbd_suspend, imx_kbd_resume);
> +static const struct dev_pm_ops imx_kbd_pm_ops = {
> + SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_kbd_noirq_suspend, 
> imx_kbd_noirq_resume)
> +};
>  
>  static struct platform_driver imx_keypad_driver = {
>   .driver = {
> -- 
> 2.7.4
> 

-- 
Dmitry

Re: 答复: 答复: 答复: [PATCH] input: alps-fix the issue alps cs19 trackstick do not work.

[dmitry.torok...@gmail.com]

On Tue, Jun 11, 2019 at 07:38:56PM +0200, Pali Rohár wrote:
> On Tuesday 11 June 2019 10:32:28 dmitry.torok...@gmail.com wrote:
> > On Tue, Jun 11, 2019 at 07:17:07PM +0200, Pali Rohár wrote:
> > > On Tuesday 11 June 2019 10:07:07 dmitry.torok...@gmail.com wrote:
> > > > On Tue, Jun 11, 2019 at 09:23:33AM +0200, Pali Rohár wrote:
> > > > > On Tuesday 11 June 2019 12:32:33 Hui Wang wrote:
> > > > > > On 2019/6/11 上午11:23, Hui Wang wrote:
> > > > > > > On 2019/6/11 上午11:05, Xiaoxiao Liu wrote:
> > > > > > > > Hi Pali,
> > > > > > > > 
> > > > > > > > I discussed with our FW team about this problem.
> > > > > > > > We think the V8 method means a touchpad feature  and does not 
> > > > > > > > fit
> > > > > > > > the CS19 trackpoint device.
> > > > > > > > CS19 TrackPoint needn't  use any Absolute (Raw) mode and is 
> > > > > > > > usually
> > > > > > > > use standard mouse data.
> > > > > > > > CS19 TrackPoint device is a completely different device with
> > > > > > > > DualPoint device of Dell/HP.
> > > > > > > > CS19 TrackPoint device is independent  of Touchpad. (Touchpad is
> > > > > > > > connecting by I2C, TrackPoint is directly connecting with PS2 
> > > > > > > > port.)
> > > > > > > > And it has completely another FW.
> > > > > > > > 
> > > > > > > > So we think it is better to use the mouse mode for CS19 
> > > > > > > > trackpoint.
> > > > > > > 
> > > > > > > Maybe here is some mis-understanding,  the mouse mode here 
> > > > > > > doesn't mean
> > > > > > > we use psmouse-base.c for cs19 (bare ps/2 mouse), we plan to use
> > > > > > > trackpoint.c to drive this HW, so this trackpoint has all 
> > > > > > > features a
> > > > > > > trackpoint should have.
> > > > > > > 
> > > > > > And I sent a patch one month ago to let the the trackpoint.c to 
> > > > > > drive this
> > > > > > HW: https://www.spinics.net/lists/linux-input/msg61341.html, maybe 
> > > > > > that
> > > > > > patch is reference.
> > > > > 
> > > > > So instead of creating blacklist, you should check for TP_VARIANT_ALPS
> > > > > in alps.c and disallow its usage.
> > > > > 
> > > > > Or maybe better, move trackpoint.c detect code before alsp.c detect 
> > > > > code
> > > > > in psmouse-base. And no changes in alps.c are needed.
> > > > 
> > > > I'd be very cautions of moving around the protocol detection. It is very
> > > > fragile, so if we can detect trackpoint-only case in alps.c and skip on
> > > > to trackpoint I would prefer it.
> > > 
> > > The main problem is that proposed trackpoint-only check in alps.c is
> > > basically what trackpoint.c is doing for checking if device is
> > > trackpoint (via function trackpoint_start_protocol()).
> > > 
> > > So I'm not sure now what is the best solution...
> > 
> > Unfortunately currently trackpoint is being probed only after we tried
> > Elan, Genius, and Logitech PS2++ protocols, and I am not sure if moving
> > trackpoint around will disturb them or not.
> > 
> > I do not think there is much code duplication by pulling limited version
> > of trackpoint detection code into alps.c and then have it bail out when
> > it sees trackpoint-only device so trackpoint.c can handle it properly.
> 
> Ok. Seems that it is the best solution.
> 
> The last question is, should be use ALPS or Trackpoint protocol? Because
> it looks like that device can be configured to one or other.
> 
> What are pros and cons of these two protocols?

As far as I know the device implements trackpoint protocol, although not
complete version. Several manufacturers started making trackponts once
IBM/Lenovo patents on the original one expired (I think).

The data stream is regular relative PS/2, bit it allows controlling
device behavior a bit, such as press-to-select option and device
sensitivity. IBM/Lenovo version has many more parameters.

Thanks.

-- 
Dmitry

Re: 答复: 答复: 答复: [PATCH] input: alps-fix the issue alps cs19 trackstick do not work.

[dmitry.torok...@gmail.com]

On Tue, Jun 11, 2019 at 07:17:07PM +0200, Pali Rohár wrote:
> On Tuesday 11 June 2019 10:07:07 dmitry.torok...@gmail.com wrote:
> > On Tue, Jun 11, 2019 at 09:23:33AM +0200, Pali Rohár wrote:
> > > On Tuesday 11 June 2019 12:32:33 Hui Wang wrote:
> > > > On 2019/6/11 上午11:23, Hui Wang wrote:
> > > > > On 2019/6/11 上午11:05, Xiaoxiao Liu wrote:
> > > > > > Hi Pali,
> > > > > > 
> > > > > > I discussed with our FW team about this problem.
> > > > > > We think the V8 method means a touchpad feature  and does not fit
> > > > > > the CS19 trackpoint device.
> > > > > > CS19 TrackPoint needn't  use any Absolute (Raw) mode and is usually
> > > > > > use standard mouse data.
> > > > > > CS19 TrackPoint device is a completely different device with
> > > > > > DualPoint device of Dell/HP.
> > > > > > CS19 TrackPoint device is independent  of Touchpad. (Touchpad is
> > > > > > connecting by I2C, TrackPoint is directly connecting with PS2 port.)
> > > > > > And it has completely another FW.
> > > > > > 
> > > > > > So we think it is better to use the mouse mode for CS19 trackpoint.
> > > > > 
> > > > > Maybe here is some mis-understanding,  the mouse mode here doesn't 
> > > > > mean
> > > > > we use psmouse-base.c for cs19 (bare ps/2 mouse), we plan to use
> > > > > trackpoint.c to drive this HW, so this trackpoint has all features a
> > > > > trackpoint should have.
> > > > > 
> > > > And I sent a patch one month ago to let the the trackpoint.c to drive 
> > > > this
> > > > HW: https://www.spinics.net/lists/linux-input/msg61341.html, maybe that
> > > > patch is reference.
> > > 
> > > So instead of creating blacklist, you should check for TP_VARIANT_ALPS
> > > in alps.c and disallow its usage.
> > > 
> > > Or maybe better, move trackpoint.c detect code before alsp.c detect code
> > > in psmouse-base. And no changes in alps.c are needed.
> > 
> > I'd be very cautions of moving around the protocol detection. It is very
> > fragile, so if we can detect trackpoint-only case in alps.c and skip on
> > to trackpoint I would prefer it.
> 
> The main problem is that proposed trackpoint-only check in alps.c is
> basically what trackpoint.c is doing for checking if device is
> trackpoint (via function trackpoint_start_protocol()).
> 
> So I'm not sure now what is the best solution...

Unfortunately currently trackpoint is being probed only after we tried
Elan, Genius, and Logitech PS2++ protocols, and I am not sure if moving
trackpoint around will disturb them or not.

I do not think there is much code duplication by pulling limited version
of trackpoint detection code into alps.c and then have it bail out when
it sees trackpoint-only device so trackpoint.c can handle it properly.

Thanks.

-- 
Dmitry

Re: 答复: 答复: 答复: [PATCH] input: alps-fix the issue alps cs19 trackstick do not work.

[dmitry.torok...@gmail.com]

On Tue, Jun 11, 2019 at 09:23:33AM +0200, Pali Rohár wrote:
> On Tuesday 11 June 2019 12:32:33 Hui Wang wrote:
> > On 2019/6/11 上午11:23, Hui Wang wrote:
> > > On 2019/6/11 上午11:05, Xiaoxiao Liu wrote:
> > > > Hi Pali,
> > > > 
> > > > I discussed with our FW team about this problem.
> > > > We think the V8 method means a touchpad feature  and does not fit
> > > > the CS19 trackpoint device.
> > > > CS19 TrackPoint needn't  use any Absolute (Raw) mode and is usually
> > > > use standard mouse data.
> > > > CS19 TrackPoint device is a completely different device with
> > > > DualPoint device of Dell/HP.
> > > > CS19 TrackPoint device is independent  of Touchpad. (Touchpad is
> > > > connecting by I2C, TrackPoint is directly connecting with PS2 port.)
> > > > And it has completely another FW.
> > > > 
> > > > So we think it is better to use the mouse mode for CS19 trackpoint.
> > > 
> > > Maybe here is some mis-understanding,  the mouse mode here doesn't mean
> > > we use psmouse-base.c for cs19 (bare ps/2 mouse), we plan to use
> > > trackpoint.c to drive this HW, so this trackpoint has all features a
> > > trackpoint should have.
> > > 
> > And I sent a patch one month ago to let the the trackpoint.c to drive this
> > HW: https://www.spinics.net/lists/linux-input/msg61341.html, maybe that
> > patch is reference.
> 
> So instead of creating blacklist, you should check for TP_VARIANT_ALPS
> in alps.c and disallow its usage.
> 
> Or maybe better, move trackpoint.c detect code before alsp.c detect code
> in psmouse-base. And no changes in alps.c are needed.

I'd be very cautions of moving around the protocol detection. It is very
fragile, so if we can detect trackpoint-only case in alps.c and skip on
to trackpoint I would prefer it.

Thanks.

-- 
Dmitry

Re: [RESEND] input: keyboard: imx: make sure keyboard can always wake up system

[dmitry.torok...@gmail.com]

Hi Anson,
On Thu, Apr 04, 2019 at 01:40:16AM +, Anson Huang wrote:
> There are several scenarios that keyboard can NOT wake up system
> from suspend, e.g., if a keyboard is depressed between system
> device suspend phase and device noirq suspend phase, the keyboard
> ISR will be called and both keyboard depress and release interrupts
> will be disabled, then keyboard will no longer be able to wake up
> system. Another scenario would be, if a keyboard is kept depressed,
> and then system goes into suspend, the expected behavior would be
> when keyboard is released, system will be waked up, but current
> implementation can NOT achieve that, because both depress and release
> interrupts are disabled in ISR, and the event check is still in
> progress.
> 
> To fix these issues, need to make sure keyboard's depress or release
> interrupt is enabled after noirq device suspend phase, this patch
> moves the suspend/resume callback to noirq suspend/resume phase, and
> enable the corresponding interrupt according to current keyboard status.

I believe it is possible for IRQ to be disabled and still  being enabled
as wakeup source. What happens if you call disable_irq() before
disabling the clock?

Thanks.

-- 
Dmitry

Re: [PATCH] input: imx6ul_tsc: use devm_platform_ioremap_resource() to simplify code

[dmitry.torok...@gmail.com]

On Mon, Apr 01, 2019 at 05:19:55AM +, Anson Huang wrote:
> Use the new helper devm_platform_ioremap_resource() which wraps the
> platform_get_resource() and devm_ioremap_resource() together, to
> simplify the code.
> 
> Signed-off-by: Anson Huang 

Applied, thank you.

> ---
>  drivers/input/touchscreen/imx6ul_tsc.c | 8 ++--
>  1 file changed, 2 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/imx6ul_tsc.c 
> b/drivers/input/touchscreen/imx6ul_tsc.c
> index c10fc59..e04eecd 100644
> --- a/drivers/input/touchscreen/imx6ul_tsc.c
> +++ b/drivers/input/touchscreen/imx6ul_tsc.c
> @@ -364,8 +364,6 @@ static int imx6ul_tsc_probe(struct platform_device *pdev)
>   struct device_node *np = pdev->dev.of_node;
>   struct imx6ul_tsc *tsc;
>   struct input_dev *input_dev;
> - struct resource *tsc_mem;
> - struct resource *adc_mem;
>   int err;
>   int tsc_irq;
>   int adc_irq;
> @@ -403,16 +401,14 @@ static int imx6ul_tsc_probe(struct platform_device 
> *pdev)
>   return err;
>   }
>  
> - tsc_mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - tsc->tsc_regs = devm_ioremap_resource(>dev, tsc_mem);
> + tsc->tsc_regs = devm_platform_ioremap_resource(pdev, 0);
>   if (IS_ERR(tsc->tsc_regs)) {
>   err = PTR_ERR(tsc->tsc_regs);
>   dev_err(>dev, "failed to remap tsc memory: %d\n", err);
>   return err;
>   }
>  
> - adc_mem = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> - tsc->adc_regs = devm_ioremap_resource(>dev, adc_mem);
> + tsc->adc_regs = devm_platform_ioremap_resource(pdev, 1);
>   if (IS_ERR(tsc->adc_regs)) {
>   err = PTR_ERR(tsc->adc_regs);
>   dev_err(>dev, "failed to remap adc memory: %d\n", err);
> -- 
> 2.7.4
> 

-- 
Dmitry

Re: [PATCH] input: keyboard: imx: use devm_platform_ioremap_resource() to simplify code

[dmitry.torok...@gmail.com]

On Mon, Apr 01, 2019 at 05:28:12AM +, Anson Huang wrote:
> Use the new helper devm_platform_ioremap_resource() which wraps the
> platform_get_resource() and devm_ioremap_resource() together, to
> simplify the code.
> 
> Signed-off-by: Anson Huang 

Applied, thank you.

> ---
>  drivers/input/keyboard/imx_keypad.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/input/keyboard/imx_keypad.c 
> b/drivers/input/keyboard/imx_keypad.c
> index 539cb67..cf08f4a 100644
> --- a/drivers/input/keyboard/imx_keypad.c
> +++ b/drivers/input/keyboard/imx_keypad.c
> @@ -422,7 +422,6 @@ static int imx_keypad_probe(struct platform_device *pdev)
>   dev_get_platdata(>dev);
>   struct imx_keypad *keypad;
>   struct input_dev *input_dev;
> - struct resource *res;
>   int irq, error, i, row, col;
>  
>   if (!keymap_data && !pdev->dev.of_node) {
> @@ -455,8 +454,7 @@ static int imx_keypad_probe(struct platform_device *pdev)
>   timer_setup(>check_matrix_timer,
>   imx_keypad_check_for_events, 0);
>  
> - res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> - keypad->mmio_base = devm_ioremap_resource(>dev, res);
> + keypad->mmio_base = devm_platform_ioremap_resource(pdev, 0);
>   if (IS_ERR(keypad->mmio_base))
>   return PTR_ERR(keypad->mmio_base);
>  
> -- 
> 2.7.4
> 

-- 
Dmitry

Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock

[dmitry.torok...@gmail.com]

On Tue, Apr 23, 2019 at 12:51:13PM +0530, Mukesh Ojha wrote:
> On 4/23/2019 8:58 AM, dmitry.torok...@gmail.com wrote:
> > On Fri, Apr 19, 2019 at 02:13:48PM +0530, Mukesh Ojha wrote:
> > > On 4/19/2019 12:41 PM, dmitry.torok...@gmail.com wrote:
> > > > On Fri, Apr 19, 2019 at 12:17:44PM +0530, Mukesh Ojha wrote:
> > > > > On 4/18/2019 11:55 AM, Mukesh Ojha wrote:
> > > > > > On 4/18/2019 7:13 AM, dmitry.torok...@gmail.com wrote:
> > > > > > > > On 4/10/2019 1:29 PM, Mukesh Ojha wrote:
> > > > > > > > > uinput_destroy_device() gets called from two places. In one 
> > > > > > > > > place,
> > > > > > > > > uinput_ioctl_handler() where it is protected under a lock
> > > > > > > > > udev->mutex but there is no protection on udev device from 
> > > > > > > > > freeing
> > > > > > > > > inside uinput_release().
> > > > > > > uinput_release() should be called when last file handle to the 
> > > > > > > uinput
> > > > > > > instance is being dropped, so there should be no other users and 
> > > > > > > thus we
> > > > > > > can't be racing with anyone.
> > > > > > Lets say an example where i am creating input device quite 
> > > > > > frequently
> > > > > > 
> > > > > > [   97.836603] input: syz0 as /devices/virtual/input/input262
> > > > > > [   97.845589] input: syz0 as /devices/virtual/input/input261
> > > > > > [   97.849415] input: syz0 as /devices/virtual/input/input263
> > > > > > [   97.856479] input: syz0 as /devices/virtual/input/input264
> > > > > > [   97.936128] input: syz0 as /devices/virtual/input/input265
> > > > > > 
> > > > > > e.g input265
> > > > > > 
> > > > > > while input265 gets created [1] and handlers are getting registered 
> > > > > > on
> > > > > > that device*fput* gets called on
> > > > > > that device as user space got to know that input265 is created and 
> > > > > > its
> > > > > > reference is still 1(rare but possible).
> > > > Are you saying that there are 2 threads sharing the same file
> > > > descriptor, one issuing the registration ioctl while the other closing
> > > > the same fd?
> > > Dmitry,
> > > 
> > > I don't have a the exact look inside the app here, but this looks like the
> > > same as it is able to do
> > > fput on the uinput device.
> > > 
> > > FYI
> > > Syskaller app is running in userspace (which is for syscall fuzzing) on
> > > kernel which is enabled
> > > with various config fault injection, FAULT_INJECTION,FAIL_SLAB,
> > > FAIL_PAGEALLOC, KASAN etc.
> > Mukesh,
> > 
> > We need to understand exactly the failure mode. I do not think that
> > introducing another mutex into uinput actually fixes the issue, as we do
> > not order mutex acquisition, so I think it is still possible for the
> > release function to acquire the mutex and run first, and then ioctl
> > would run with freed object.
> 
> I have taken care this case from ioctl and release point of view.
> 
> Even if the release gets called first it will make the
> file->private_data=NULL.
> and further call to ioctl will not be a problem as the check is already
> there.

Al, do we have any protections in VFS layer from userspace hanging onto
a file descriptor and calling ioctl() on it even as another thread
calls close() on the same fd?

Should the issue be solved by individual drivers, or more careful
accounting for currently running operations is needed at VFS layer?

Thanks.

-- 
Dmitry

Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock

[dmitry.torok...@gmail.com]

On Fri, Apr 19, 2019 at 02:13:48PM +0530, Mukesh Ojha wrote:
> 
> On 4/19/2019 12:41 PM, dmitry.torok...@gmail.com wrote:
> > Hi Mukesh,
> > 
> > On Fri, Apr 19, 2019 at 12:17:44PM +0530, Mukesh Ojha wrote:
> > > For some reason my last mail did not get delivered,  sending it again.
> > > 
> > > 
> > > On 4/18/2019 11:55 AM, Mukesh Ojha wrote:
> > > > 
> > > > On 4/18/2019 7:13 AM, dmitry.torok...@gmail.com wrote:
> > > > > Hi Mukesh,
> > > > > 
> > > > > On Mon, Apr 15, 2019 at 03:35:51PM +0530, Mukesh Ojha wrote:
> > > > > > Hi Dmitry,
> > > > > > 
> > > > > > Can you please have a look at this patch ? as this seems to 
> > > > > > reproducing
> > > > > > quite frequently
> > > > > > 
> > > > > > Thanks,
> > > > > > Mukesh
> > > > > > 
> > > > > > On 4/10/2019 1:29 PM, Mukesh Ojha wrote:
> > > > > > > uinput_destroy_device() gets called from two places. In one place,
> > > > > > > uinput_ioctl_handler() where it is protected under a lock
> > > > > > > udev->mutex but there is no protection on udev device from freeing
> > > > > > > inside uinput_release().
> > > > > uinput_release() should be called when last file handle to the uinput
> > > > > instance is being dropped, so there should be no other users and thus 
> > > > > we
> > > > > can't be racing with anyone.
> > > > Lets say an example where i am creating input device quite frequently
> > > > 
> > > > [   97.836603] input: syz0 as /devices/virtual/input/input262
> > > > [   97.845589] input: syz0 as /devices/virtual/input/input261
> > > > [   97.849415] input: syz0 as /devices/virtual/input/input263
> > > > [   97.856479] input: syz0 as /devices/virtual/input/input264
> > > > [   97.936128] input: syz0 as /devices/virtual/input/input265
> > > > 
> > > > e.g input265
> > > > 
> > > > while input265 gets created [1] and handlers are getting registered on
> > > > that device*fput* gets called on
> > > > that device as user space got to know that input265 is created and its
> > > > reference is still 1(rare but possible).
> > Are you saying that there are 2 threads sharing the same file
> > descriptor, one issuing the registration ioctl while the other closing
> > the same fd?
> 
> Dmitry,
> 
> I don't have a the exact look inside the app here, but this looks like the
> same as it is able to do
> fput on the uinput device.
> 
> FYI
> Syskaller app is running in userspace (which is for syscall fuzzing) on
> kernel which is enabled
> with various config fault injection, FAULT_INJECTION,FAIL_SLAB,
> FAIL_PAGEALLOC, KASAN etc.

Mukesh,

We need to understand exactly the failure mode. I do not think that
introducing another mutex into uinput actually fixes the issue, as we do
not order mutex acquisition, so I think it is still possible for the
release function to acquire the mutex and run first, and then ioctl
would run with freed object.

My guess that this needs to be fixed in VFS layer.

Thanks.

-- 
Dmitry

Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock

[dmitry.torok...@gmail.com]

Hi Mukesh,

On Fri, Apr 19, 2019 at 12:17:44PM +0530, Mukesh Ojha wrote:
> For some reason my last mail did not get delivered,  sending it again.
> 
> 
> On 4/18/2019 11:55 AM, Mukesh Ojha wrote:
> > 
> > 
> > On 4/18/2019 7:13 AM, dmitry.torok...@gmail.com wrote:
> > > Hi Mukesh,
> > > 
> > > On Mon, Apr 15, 2019 at 03:35:51PM +0530, Mukesh Ojha wrote:
> > > > Hi Dmitry,
> > > > 
> > > > Can you please have a look at this patch ? as this seems to reproducing
> > > > quite frequently
> > > > 
> > > > Thanks,
> > > > Mukesh
> > > > 
> > > > On 4/10/2019 1:29 PM, Mukesh Ojha wrote:
> > > > > uinput_destroy_device() gets called from two places. In one place,
> > > > > uinput_ioctl_handler() where it is protected under a lock
> > > > > udev->mutex but there is no protection on udev device from freeing
> > > > > inside uinput_release().
> > > uinput_release() should be called when last file handle to the uinput
> > > instance is being dropped, so there should be no other users and thus we
> > > can't be racing with anyone.
> > 
> > Lets say an example where i am creating input device quite frequently
> > 
> > [   97.836603] input: syz0 as /devices/virtual/input/input262
> > [   97.845589] input: syz0 as /devices/virtual/input/input261
> > [   97.849415] input: syz0 as /devices/virtual/input/input263
> > [   97.856479] input: syz0 as /devices/virtual/input/input264
> > [   97.936128] input: syz0 as /devices/virtual/input/input265
> > 
> > e.g input265
> > 
> > while input265 gets created [1] and handlers are getting registered on
> > that device*fput* gets called on
> > that device as user space got to know that input265 is created and its
> > reference is still 1(rare but possible).

Are you saying that there are 2 threads sharing the same file
descriptor, one issuing the registration ioctl while the other closing
the same fd?

Thanks.

-- 
Dmitry

Re: [PATCH v2] Input: uinput: Avoid Object-Already-Free with a global lock

[dmitry.torok...@gmail.com]

Hi Mukesh,

On Mon, Apr 15, 2019 at 03:35:51PM +0530, Mukesh Ojha wrote:
> 
> Hi Dmitry,
> 
> Can you please have a look at this patch ? as this seems to reproducing
> quite frequently
> 
> Thanks,
> Mukesh
> 
> On 4/10/2019 1:29 PM, Mukesh Ojha wrote:
> > uinput_destroy_device() gets called from two places. In one place,
> > uinput_ioctl_handler() where it is protected under a lock
> > udev->mutex but there is no protection on udev device from freeing
> > inside uinput_release().

uinput_release() should be called when last file handle to the uinput
instance is being dropped, so there should be no other users and thus we
can't be racing with anyone.

> > 
> > This can result in Object-Already-Free case where uinput parent
> > device already got freed while a child being inserted inside it.
> > That result in a double free case for parent while kernfs_put()
> > being done for child in a failure path of adding a node.

Can you please describe scenario in more detail? How do you free the
parent device while child input device is being registered?

Thanks.

- 
Dmitry

Re: [PATCH] input: keyboard: snvs: use dev_pm_set_wake_irq() to simplify code

[dmitry.torok...@gmail.com]

On Wed, Mar 27, 2019 at 06:08:05AM +, Anson Huang wrote:
> With calling dev_pm_set_wake_irq() to set SNVS ON/OFF button
> as wakeup source for suspend, generic wake irq mechanism
> will automatically enable it as wakeup source when suspend,
> then the enable_irq_wake()/disable_irq_wake() can be removed
> in suspend/resume callback, it simplifies the code.
> 
> Signed-off-by: Anson Huang 

Applied, thank you.

> ---
>  drivers/input/keyboard/snvs_pwrkey.c | 30 --
>  1 file changed, 4 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c 
> b/drivers/input/keyboard/snvs_pwrkey.c
> index 4c67cf3..5342d8d 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -15,6 +15,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -167,28 +168,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device 
> *pdev)
>   }
>  
>   device_init_wakeup(>dev, pdata->wakeup);
> -
> - return 0;
> -}
> -
> -static int __maybe_unused imx_snvs_pwrkey_suspend(struct device *dev)
> -{
> - struct platform_device *pdev = to_platform_device(dev);
> - struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> -
> - if (device_may_wakeup(>dev))
> - enable_irq_wake(pdata->irq);
> -
> - return 0;
> -}
> -
> -static int __maybe_unused imx_snvs_pwrkey_resume(struct device *dev)
> -{
> - struct platform_device *pdev = to_platform_device(dev);
> - struct pwrkey_drv_data *pdata = platform_get_drvdata(pdev);
> -
> - if (device_may_wakeup(>dev))
> - disable_irq_wake(pdata->irq);
> + error = dev_pm_set_wake_irq(>dev, pdata->irq);
> + if (error)
> + dev_err(>dev, "irq wake enable failed.\n");
>  
>   return 0;
>  }
> @@ -199,13 +181,9 @@ static const struct of_device_id imx_snvs_pwrkey_ids[] = 
> {
>  };
>  MODULE_DEVICE_TABLE(of, imx_snvs_pwrkey_ids);
>  
> -static SIMPLE_DEV_PM_OPS(imx_snvs_pwrkey_pm_ops, imx_snvs_pwrkey_suspend,
> - imx_snvs_pwrkey_resume);
> -
>  static struct platform_driver imx_snvs_pwrkey_driver = {
>   .driver = {
>   .name = "snvs_pwrkey",
> - .pm = _snvs_pwrkey_pm_ops,
>   .of_match_table = imx_snvs_pwrkey_ids,
>   },
>   .probe = imx_snvs_pwrkey_probe,
> -- 
> 2.7.4
> 

-- 
Dmitry

Re: [PATCH V2] input: keyboard: snvs: initialize necessary driver data before enabling IRQ

[dmitry.torok...@gmail.com]

On Wed, Mar 27, 2019 at 06:07:06AM +, Anson Huang wrote:
> SNVS IRQ is requested before necessary driver data initialized,
> if there is a pending IRQ during driver probe phase, kernel
> NULL pointer panic will occur in IRQ handler. To avoid such
> scenario, just initialize necessary driver data before enabling
> IRQ. This patch is inspired by NXP's internal kernel tree.
> 
> Fixes: d3dc6e232215 ("input: keyboard: imx: add snvs power key driver")
> Signed-off-by: Anson Huang 

Applied, thank you.

> ---
> Changes since V1:
>   - move the platform data initialization to before IRQ enable instead of 
> moving the IRQ enable
> to the end of probe function.
> ---
>  drivers/input/keyboard/snvs_pwrkey.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c 
> b/drivers/input/keyboard/snvs_pwrkey.c
> index effb632..4c67cf3 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -148,6 +148,9 @@ static int imx_snvs_pwrkey_probe(struct platform_device 
> *pdev)
>   return error;
>   }
>  
> + pdata->input = input;
> + platform_set_drvdata(pdev, pdata);
> +
>   error = devm_request_irq(>dev, pdata->irq,
>  imx_snvs_pwrkey_interrupt,
>  0, pdev->name, pdev);
> @@ -163,9 +166,6 @@ static int imx_snvs_pwrkey_probe(struct platform_device 
> *pdev)
>   return error;
>   }
>  
> - pdata->input = input;
> - platform_set_drvdata(pdev, pdata);
> -
>   device_init_wakeup(>dev, pdata->wakeup);
>  
>   return 0;
> -- 
> 2.7.4
> 

-- 
Dmitry

Re: [PATCH 1/2] input: keyboard: imx: no need to control interrupt status in event check

[dmitry.torok...@gmail.com]

Hi Anson,

On Fri, Mar 29, 2019 at 07:00:43AM +, Anson Huang wrote:
> There is no need to enable release interrupt and disable depress
> interrupt in event check, as a timer is setup for checking these
> events rather than interrupts.

But won't using release interrupt allow signalling key release earlier?

> 
> Signed-off-by: Anson Huang 
> ---
>  drivers/input/keyboard/imx_keypad.c | 5 -
>  1 file changed, 5 deletions(-)
> 
> diff --git a/drivers/input/keyboard/imx_keypad.c 
> b/drivers/input/keyboard/imx_keypad.c
> index 539cb67..7e32c36 100644
> --- a/drivers/input/keyboard/imx_keypad.c
> +++ b/drivers/input/keyboard/imx_keypad.c
> @@ -276,11 +276,6 @@ static void imx_keypad_check_for_events(struct 
> timer_list *t)
>   reg_val = readw(keypad->mmio_base + KPSR);
>   reg_val |= KBD_STAT_KPKR | KBD_STAT_KRSS;
>   writew(reg_val, keypad->mmio_base + KPSR);
> -
> - reg_val = readw(keypad->mmio_base + KPSR);
> - reg_val |= KBD_STAT_KRIE;
> - reg_val &= ~KBD_STAT_KDIE;
> - writew(reg_val, keypad->mmio_base + KPSR);
>   }
>  }
>  
> -- 
> 2.7.4
> 

Thanks.

-- 
Dmitry

Re: [BUG REPORT] linux-input: keyboard: gpio_keys: False Button Press Event on Wake

[dmitry.torok...@gmail.com]

Hi Ken,

On Wed, Apr 03, 2019 at 05:50:09PM +, Ken Sloat wrote:
> Hello Dmitry,
> 
> I may have found a potential bug in the "gpio_keys" driver. FYI, I am
> running the 4.14 LTS kernel on my system, but from my understanding of
> the issue, it seems that this would still occur in the latest version
> of the kernel.
> 
> The problem: In the 4.14 LTS kernel, both key press and release events
> can generate a wake event. In the 5.x kernel, wake events are
> configurable for press only, release only or "both" (see
> "wakeup-event-action" binding). The issue can occur in the "both" case
> or release/deasserted case. Let's imagine that a system is suspended
> when a gpio key button is pressed, and subsequently resumed when the
> button is released. If we look at the sequence of actions and events
> reported by the input system, we can see the potential problem:
> 
> Button Pressed
> Event Value 1
> System Suspend
> Button Released
> System Wake & Resume
> Event Value 0
> Event Value 1
> Event Value 0
> 
> As you can see the input system will report an extra button
> event/press. This appears to be caused in gpio_keys_gpio_isr by the
> following statement:
> 
> if (bdata->suspended  &&
> (button->type == 0 || button->type == EV_KEY)) {
> /*
>  * Simulate wakeup key press in case the key has
>  * already released by the time we got interrupt
>  * handler to run.
>  */
> input_report_key(bdata->input, button->code, 1);
> }
> 
> This code does not seem to take into account that the wake event may
> have been caused by a button release action, and just assumes we must
> have a button press.
> 
> This can obviously be problematic in the use case I mentioned, as the
> system would be put in a constant loop between waking and sleeping.
> While there are other ways to deal with or react to this issue in the
> userspace, it seems that the driver should probably take this into
> account.
> 

I believe the expectation is that we do not go to sleep with button
still pressed, as we expect it to be released momentarily. Given that we
do not know which edge woke us it is not clear if we can avoid
simulating the keypress event, as this definitely causes "missed press",
at least on some Android devices, where by the time we get to run this
ISR user has already released the button.

Thanks.

-- 
Dmitry

Re: [PATCH] input: keyboard: snvs: make sure irq is handled correctly

[dmitry.torok...@gmail.com]

Hi Anson,

On Wed, Mar 27, 2019 at 02:47:06AM +, Anson Huang wrote:
> SNVS IRQ is requested before necessary driver data initialized,
> if there is a pending IRQ during driver probe phase, kernel
> NULL pointer panic will occur in IRQ handler. To avoid such
> scenario, need to move the IRQ request to after driver data
> initialization done. This patch is inspired by NXP's internal
> kernel tree.
> 
> Fixes: d3dc6e232215 ("input: keyboard: imx: add snvs power key driver")
> Signed-off-by: Anson Huang 
> ---
>  drivers/input/keyboard/snvs_pwrkey.c | 17 -
>  1 file changed, 8 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/input/keyboard/snvs_pwrkey.c 
> b/drivers/input/keyboard/snvs_pwrkey.c
> index effb632..6ff41fd 100644
> --- a/drivers/input/keyboard/snvs_pwrkey.c
> +++ b/drivers/input/keyboard/snvs_pwrkey.c
> @@ -148,15 +148,6 @@ static int imx_snvs_pwrkey_probe(struct platform_device 
> *pdev)
>   return error;
>   }
>  
> - error = devm_request_irq(>dev, pdata->irq,
> -imx_snvs_pwrkey_interrupt,
> -0, pdev->name, pdev);
> -
> - if (error) {
> - dev_err(>dev, "interrupt not available.\n");
> - return error;
> - }
> -
>   error = input_register_device(input);
>   if (error < 0) {
>   dev_err(>dev, "failed to register input device\n");
> @@ -166,6 +157,14 @@ static int imx_snvs_pwrkey_probe(struct platform_device 
> *pdev)
>   pdata->input = input;
>   platform_set_drvdata(pdev, pdata);
>  
> + error = devm_request_irq(>dev, pdata->irq,
> +  imx_snvs_pwrkey_interrupt,
> +  0, pdev->name, pdev);
> + if (error) {
> + dev_err(>dev, "interrupt not available.\n");
> + return error;
> + }

Instead of moving devm_request_irq() around could you simply move
pdata->input = input assignment higher? It is perfectly fine to try
calling input_event() on input device that is allocated but not yet
registered.

> +
>   device_init_wakeup(>dev, pdata->wakeup);

Unrelated suggestion:

Can you try calling dev_pm_set_wake_irq(>dev, pdata->irq) here and
I think you will be able to get rid of suspend/resume methods in the
driver.

Thanks.

-- 
Dmitry

Re: [PATCH v7 0/7] Introduce STPMIC1 PMIC Driver

[dmitry.torok...@gmail.com]

On Fri, Dec 14, 2018 at 12:47:01PM +, Lee Jones wrote:
> On Fri, 30 Nov 2018, Pascal PAILLET-LME wrote:
> 
> > The goal of this patch-set is to propose a driver for the STPMIC1 PMIC from 
> > STMicroelectronics. 
> > The STPMIC1 regulators supply power to an application processor as well as 
> > to external system peripherals such as DDR, Flash memories and system
> > devices. It also features onkey button input and an hardware watchdog.
> > The STPMIC1 is controlled via I2C. 
> > 
> > Main driver is drivers/mfd/stpmic1 that handle I2C regmap configuration and
> > irqchip. stpmic1_regulator, stpmic1_onkey and stpmic1_wdt need stpmic1 mfd
> > as parent.
> > 
> > STPMIC1 MFD and regulator drivers maybe mandatory at boot time.
> > 
> > Pascal Paillet (7):
> > changes in v7:
> > * rebase on regul/for-next
> > 
> >   dt-bindings: mfd: document stpmic1
> >   mfd: stpmic1: add stpmic1 driver
> >   dt-bindings: input: document stpmic1 pmic onkey
> >   input: stpmic1: add stpmic1 onkey driver
> >   dt-bindings: watchdog: document stpmic1 pmic watchdog
> >   watchdog: stpmic1: add stpmic1 watchdog driver
> >   regulator: stpmic1: fix regulator_lock usage
> 
> Could you please remove any patches which have been applied and
> [RESEND]?
> 
> Also, is Dmitry planning on Acking:
> 
>   dt-bindings: input: document stpmic1 pmic onkey
> 
> ... or is Rob's Ack enough?

For bindings I normally defer to Rob (unless I see something that really
bugs me and then I'll speak up).

Thanks.

-- 
Dmitry

Re: [PATCH v4 6/8] input: stpmic1: add stpmic1 onkey driver

[dmitry.torok...@gmail.com]

On Thu, Oct 18, 2018 at 09:02:13AM +, Pascal PAILLET-LME wrote:
> From: pascal paillet 
> 
> The stpmic1 pmic is able to manage an onkey button. This driver exposes
> the stpmic1 onkey as an input device. It can also be configured to
> shut-down the power supplies on a long key-press with an adjustable
> duration.
> 
> Signed-off-by: pascal paillet 

OK, so this has dependency on linux/mfd/stpmic1.h and therefore I expect
it will go in through Lee's tree.

Acked-by: Dmitry Torokhov 

> ---
> changes in v4:
> * remove remove function
> * merge stpmic1_onkey_dt_params() in probe function
> * suppresse struct pmic_onkey_config
> * rename PONKEY_PU_ACTIVE to PONKEY_PU_INACTIVE
> 
>  drivers/input/misc/Kconfig |  11 +++
>  drivers/input/misc/Makefile|   2 +
>  drivers/input/misc/stpmic1_onkey.c | 197 
> +
>  3 files changed, 210 insertions(+)
>  create mode 100644 drivers/input/misc/stpmic1_onkey.c
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index ca59a2b..279fb02 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -851,4 +851,15 @@ config INPUT_SC27XX_VIBRA
> To compile this driver as a module, choose M here. The module will
> be called sc27xx_vibra.
>  
> +config INPUT_STPMIC1_ONKEY
> + tristate "STPMIC1 PMIC Onkey support"
> + depends on MFD_STPMIC1
> + help
> +   Say Y to enable support of onkey embedded into STPMIC1 PMIC. onkey
> +   can be used to wakeup from low power modes and force a shut-down on
> +   long press.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called stpmic1_onkey.
> +
>  endif
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 9d0f9d1..1b44202 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -71,6 +71,7 @@ obj-$(CONFIG_INPUT_SGI_BTNS)+= sgi_btns.o
>  obj-$(CONFIG_INPUT_SIRFSOC_ONKEY)+= sirfsoc-onkey.o
>  obj-$(CONFIG_INPUT_SOC_BUTTON_ARRAY) += soc_button_array.o
>  obj-$(CONFIG_INPUT_SPARCSPKR)+= sparcspkr.o
> +obj-$(CONFIG_INPUT_STPMIC1_ONKEY)+= stpmic1_onkey.o
>  obj-$(CONFIG_INPUT_TPS65218_PWRBUTTON)   += tps65218-pwrbutton.o
>  obj-$(CONFIG_INPUT_TWL4030_PWRBUTTON)+= twl4030-pwrbutton.o
>  obj-$(CONFIG_INPUT_TWL4030_VIBRA)+= twl4030-vibra.o
> @@ -81,3 +82,4 @@ obj-$(CONFIG_INPUT_WM831X_ON)   += wm831x-on.o
>  obj-$(CONFIG_INPUT_XEN_KBDDEV_FRONTEND)  += xen-kbdfront.o
>  obj-$(CONFIG_INPUT_YEALINK)  += yealink.o
>  obj-$(CONFIG_INPUT_IDEAPAD_SLIDEBAR) += ideapad_slidebar.o
> +
> diff --git a/drivers/input/misc/stpmic1_onkey.c 
> b/drivers/input/misc/stpmic1_onkey.c
> new file mode 100644
> index 000..6a7f08b
> --- /dev/null
> +++ b/drivers/input/misc/stpmic1_onkey.c
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) STMicroelectronics 2018
> +// Author: Pascal Paillet  for STMicroelectronics.
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/**
> + * struct stpmic1_onkey - OnKey data
> + * @input_dev:   pointer to input device
> + * @irq_falling: irq that we are hooked on to
> + * @irq_rising:  irq that we are hooked on to
> + */
> +struct stpmic1_onkey {
> + struct input_dev *input_dev;
> + int irq_falling;
> + int irq_rising;
> +};
> +
> +static irqreturn_t onkey_falling_irq(int irq, void *ponkey)
> +{
> + struct stpmic1_onkey *onkey = ponkey;
> + struct input_dev *input_dev = onkey->input_dev;
> +
> + input_report_key(input_dev, KEY_POWER, 1);
> + pm_wakeup_event(input_dev->dev.parent, 0);
> + input_sync(input_dev);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t onkey_rising_irq(int irq, void *ponkey)
> +{
> + struct stpmic1_onkey *onkey = ponkey;
> + struct input_dev *input_dev = onkey->input_dev;
> +
> + input_report_key(input_dev, KEY_POWER, 0);
> + pm_wakeup_event(input_dev->dev.parent, 0);
> + input_sync(input_dev);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int stpmic1_onkey_probe(struct platform_device *pdev)
> +{
> + struct stpmic1 *pmic = dev_get_drvdata(pdev->dev.parent);
> + struct device *dev = >dev;
> + struct input_dev *input_dev;
> + struct stpmic1_onkey *onkey;
> + unsigned int val, reg = 0;
> + int error;
> +
> + onkey = devm_kzalloc(dev, sizeof(*onkey), GFP_KERNEL);
> + if (!onkey)
> + return -ENOMEM;
> +
> + onkey->irq_falling = platform_get_irq_byname(pdev, "onkey-falling");
> + if (onkey->irq_falling < 0) {
> + dev_err(dev, "failed: request IRQ onkey-falling %d\n",
> + onkey->irq_falling);
> + return onkey->irq_falling;
> + }
> +
> + onkey->irq_rising = platform_get_irq_byname(pdev, "onkey-rising");
> + if 

Re: [PATCH v4 6/8] input: stpmic1: add stpmic1 onkey driver

[dmitry.torok...@gmail.com]

On Thu, Oct 18, 2018 at 09:02:13AM +, Pascal PAILLET-LME wrote:
> From: pascal paillet 
> 
> The stpmic1 pmic is able to manage an onkey button. This driver exposes
> the stpmic1 onkey as an input device. It can also be configured to
> shut-down the power supplies on a long key-press with an adjustable
> duration.
> 
> Signed-off-by: pascal paillet 

OK, so this has dependency on linux/mfd/stpmic1.h and therefore I expect
it will go in through Lee's tree.

Acked-by: Dmitry Torokhov 

> ---
> changes in v4:
> * remove remove function
> * merge stpmic1_onkey_dt_params() in probe function
> * suppresse struct pmic_onkey_config
> * rename PONKEY_PU_ACTIVE to PONKEY_PU_INACTIVE
> 
>  drivers/input/misc/Kconfig |  11 +++
>  drivers/input/misc/Makefile|   2 +
>  drivers/input/misc/stpmic1_onkey.c | 197 
> +
>  3 files changed, 210 insertions(+)
>  create mode 100644 drivers/input/misc/stpmic1_onkey.c
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index ca59a2b..279fb02 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -851,4 +851,15 @@ config INPUT_SC27XX_VIBRA
> To compile this driver as a module, choose M here. The module will
> be called sc27xx_vibra.
>  
> +config INPUT_STPMIC1_ONKEY
> + tristate "STPMIC1 PMIC Onkey support"
> + depends on MFD_STPMIC1
> + help
> +   Say Y to enable support of onkey embedded into STPMIC1 PMIC. onkey
> +   can be used to wakeup from low power modes and force a shut-down on
> +   long press.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called stpmic1_onkey.
> +
>  endif
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 9d0f9d1..1b44202 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -71,6 +71,7 @@ obj-$(CONFIG_INPUT_SGI_BTNS)+= sgi_btns.o
>  obj-$(CONFIG_INPUT_SIRFSOC_ONKEY)+= sirfsoc-onkey.o
>  obj-$(CONFIG_INPUT_SOC_BUTTON_ARRAY) += soc_button_array.o
>  obj-$(CONFIG_INPUT_SPARCSPKR)+= sparcspkr.o
> +obj-$(CONFIG_INPUT_STPMIC1_ONKEY)+= stpmic1_onkey.o
>  obj-$(CONFIG_INPUT_TPS65218_PWRBUTTON)   += tps65218-pwrbutton.o
>  obj-$(CONFIG_INPUT_TWL4030_PWRBUTTON)+= twl4030-pwrbutton.o
>  obj-$(CONFIG_INPUT_TWL4030_VIBRA)+= twl4030-vibra.o
> @@ -81,3 +82,4 @@ obj-$(CONFIG_INPUT_WM831X_ON)   += wm831x-on.o
>  obj-$(CONFIG_INPUT_XEN_KBDDEV_FRONTEND)  += xen-kbdfront.o
>  obj-$(CONFIG_INPUT_YEALINK)  += yealink.o
>  obj-$(CONFIG_INPUT_IDEAPAD_SLIDEBAR) += ideapad_slidebar.o
> +
> diff --git a/drivers/input/misc/stpmic1_onkey.c 
> b/drivers/input/misc/stpmic1_onkey.c
> new file mode 100644
> index 000..6a7f08b
> --- /dev/null
> +++ b/drivers/input/misc/stpmic1_onkey.c
> @@ -0,0 +1,197 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) STMicroelectronics 2018
> +// Author: Pascal Paillet  for STMicroelectronics.
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/**
> + * struct stpmic1_onkey - OnKey data
> + * @input_dev:   pointer to input device
> + * @irq_falling: irq that we are hooked on to
> + * @irq_rising:  irq that we are hooked on to
> + */
> +struct stpmic1_onkey {
> + struct input_dev *input_dev;
> + int irq_falling;
> + int irq_rising;
> +};
> +
> +static irqreturn_t onkey_falling_irq(int irq, void *ponkey)
> +{
> + struct stpmic1_onkey *onkey = ponkey;
> + struct input_dev *input_dev = onkey->input_dev;
> +
> + input_report_key(input_dev, KEY_POWER, 1);
> + pm_wakeup_event(input_dev->dev.parent, 0);
> + input_sync(input_dev);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t onkey_rising_irq(int irq, void *ponkey)
> +{
> + struct stpmic1_onkey *onkey = ponkey;
> + struct input_dev *input_dev = onkey->input_dev;
> +
> + input_report_key(input_dev, KEY_POWER, 0);
> + pm_wakeup_event(input_dev->dev.parent, 0);
> + input_sync(input_dev);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int stpmic1_onkey_probe(struct platform_device *pdev)
> +{
> + struct stpmic1 *pmic = dev_get_drvdata(pdev->dev.parent);
> + struct device *dev = >dev;
> + struct input_dev *input_dev;
> + struct stpmic1_onkey *onkey;
> + unsigned int val, reg = 0;
> + int error;
> +
> + onkey = devm_kzalloc(dev, sizeof(*onkey), GFP_KERNEL);
> + if (!onkey)
> + return -ENOMEM;
> +
> + onkey->irq_falling = platform_get_irq_byname(pdev, "onkey-falling");
> + if (onkey->irq_falling < 0) {
> + dev_err(dev, "failed: request IRQ onkey-falling %d\n",
> + onkey->irq_falling);
> + return onkey->irq_falling;
> + }
> +
> + onkey->irq_rising = platform_get_irq_byname(pdev, "onkey-rising");
> + if 

Re: [PATCH v3 6/8] input: stpmic1: add stpmic1 onkey driver

[dmitry.torok...@gmail.com]

Hi Pascal,

On Mon, Oct 08, 2018 at 04:29:41PM +, Pascal PAILLET-LME wrote:
> From: pascal paillet 
> 
> The stpmic1 pmic is able to manage an onkey button. This driver exposes
> the stpmic1 onkey as an input device. It can also be configured to
> shut-down the power supplies on a long key-press with an adjustable
> duration.
> 
> Signed-off-by: pascal paillet 
> ---
> changes in v3:
> * Rename struct stpmic1_dev by struct stpmic1.
> * Replace of_property_ by device_property.
> * Remove log in IRQ handler.
> * Add email address in MODULE_AUTHOR.
> 
>  drivers/input/misc/Kconfig |  11 ++
>  drivers/input/misc/Makefile|   2 +
>  drivers/input/misc/stpmic1_onkey.c | 248 
> +
>  3 files changed, 261 insertions(+)
>  create mode 100644 drivers/input/misc/stpmic1_onkey.c
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index ca59a2b..279fb02 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -851,4 +851,15 @@ config INPUT_SC27XX_VIBRA
> To compile this driver as a module, choose M here. The module will
> be called sc27xx_vibra.
>  
> +config INPUT_STPMIC1_ONKEY
> + tristate "STPMIC1 PMIC Onkey support"
> + depends on MFD_STPMIC1
> + help
> +   Say Y to enable support of onkey embedded into STPMIC1 PMIC. onkey
> +   can be used to wakeup from low power modes and force a shut-down on
> +   long press.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called stpmic1_onkey.
> +
>  endif
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 9d0f9d1..1b44202 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -71,6 +71,7 @@ obj-$(CONFIG_INPUT_SGI_BTNS)+= sgi_btns.o
>  obj-$(CONFIG_INPUT_SIRFSOC_ONKEY)+= sirfsoc-onkey.o
>  obj-$(CONFIG_INPUT_SOC_BUTTON_ARRAY) += soc_button_array.o
>  obj-$(CONFIG_INPUT_SPARCSPKR)+= sparcspkr.o
> +obj-$(CONFIG_INPUT_STPMIC1_ONKEY)+= stpmic1_onkey.o
>  obj-$(CONFIG_INPUT_TPS65218_PWRBUTTON)   += tps65218-pwrbutton.o
>  obj-$(CONFIG_INPUT_TWL4030_PWRBUTTON)+= twl4030-pwrbutton.o
>  obj-$(CONFIG_INPUT_TWL4030_VIBRA)+= twl4030-vibra.o
> @@ -81,3 +82,4 @@ obj-$(CONFIG_INPUT_WM831X_ON)   += wm831x-on.o
>  obj-$(CONFIG_INPUT_XEN_KBDDEV_FRONTEND)  += xen-kbdfront.o
>  obj-$(CONFIG_INPUT_YEALINK)  += yealink.o
>  obj-$(CONFIG_INPUT_IDEAPAD_SLIDEBAR) += ideapad_slidebar.o
> +
> diff --git a/drivers/input/misc/stpmic1_onkey.c 
> b/drivers/input/misc/stpmic1_onkey.c
> new file mode 100644
> index 000..871a087
> --- /dev/null
> +++ b/drivers/input/misc/stpmic1_onkey.c
> @@ -0,0 +1,248 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) STMicroelectronics 2018
> +// Author: Pascal Paillet  for STMicroelectronics.
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/**
> + * struct stpmic1_onkey - OnKey data
> + * @pmic:pointer to STPMIC1 PMIC device
> + * @input_dev:   pointer to input device
> + * @irq_falling: irq that we are hooked on to
> + * @irq_rising:  irq that we are hooked on to
> + */
> +struct stpmic1_onkey {
> + struct stpmic1 *pmic;
> + struct input_dev *input_dev;
> + int irq_falling;
> + int irq_rising;
> +};
> +
> +/**
> + * struct pmic_onkey_config - configuration of pmic PONKEYn
> + * @cc_flag_clear:   value to clear CC flag in case of PowerOff
> + * trigger by longkey press
> + * @onkey_pullup_val:value of PONKEY PullUp (active or 
> inactive)
> + * @power_off_time_sec:  value for long press h/w shutdown event
> + */
> +struct pmic_onkey_config {
> + bool cc_flag_clear;
> + u8 onkey_pullup_val;
> + u8 power_off_time_sec;
> +};
> +
> +static irqreturn_t onkey_falling_irq(int irq, void *ponkey)
> +{
> + struct stpmic1_onkey *onkey = ponkey;
> + struct input_dev *input_dev = onkey->input_dev;
> +
> + input_report_key(input_dev, KEY_POWER, 1);
> + pm_wakeup_event(input_dev->dev.parent, 0);
> + input_sync(input_dev);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t onkey_rising_irq(int irq, void *ponkey)
> +{
> + struct stpmic1_onkey *onkey = ponkey;
> + struct input_dev *input_dev = onkey->input_dev;
> +
> + input_report_key(input_dev, KEY_POWER, 0);
> + pm_wakeup_event(input_dev->dev.parent, 0);
> + input_sync(input_dev);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int stpmic1_onkey_dt_params(struct platform_device *pdev,
> +struct stpmic1_onkey *onkey,
> +struct pmic_onkey_config *config)
> +{
> + struct device *dev = >dev;
> + u32 val;
> +
> + onkey->irq_falling = platform_get_irq_byname(pdev, "onkey-falling");
> + if 

Re: [PATCH v3 6/8] input: stpmic1: add stpmic1 onkey driver

[dmitry.torok...@gmail.com]

Hi Pascal,

On Mon, Oct 08, 2018 at 04:29:41PM +, Pascal PAILLET-LME wrote:
> From: pascal paillet 
> 
> The stpmic1 pmic is able to manage an onkey button. This driver exposes
> the stpmic1 onkey as an input device. It can also be configured to
> shut-down the power supplies on a long key-press with an adjustable
> duration.
> 
> Signed-off-by: pascal paillet 
> ---
> changes in v3:
> * Rename struct stpmic1_dev by struct stpmic1.
> * Replace of_property_ by device_property.
> * Remove log in IRQ handler.
> * Add email address in MODULE_AUTHOR.
> 
>  drivers/input/misc/Kconfig |  11 ++
>  drivers/input/misc/Makefile|   2 +
>  drivers/input/misc/stpmic1_onkey.c | 248 
> +
>  3 files changed, 261 insertions(+)
>  create mode 100644 drivers/input/misc/stpmic1_onkey.c
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index ca59a2b..279fb02 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -851,4 +851,15 @@ config INPUT_SC27XX_VIBRA
> To compile this driver as a module, choose M here. The module will
> be called sc27xx_vibra.
>  
> +config INPUT_STPMIC1_ONKEY
> + tristate "STPMIC1 PMIC Onkey support"
> + depends on MFD_STPMIC1
> + help
> +   Say Y to enable support of onkey embedded into STPMIC1 PMIC. onkey
> +   can be used to wakeup from low power modes and force a shut-down on
> +   long press.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called stpmic1_onkey.
> +
>  endif
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 9d0f9d1..1b44202 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -71,6 +71,7 @@ obj-$(CONFIG_INPUT_SGI_BTNS)+= sgi_btns.o
>  obj-$(CONFIG_INPUT_SIRFSOC_ONKEY)+= sirfsoc-onkey.o
>  obj-$(CONFIG_INPUT_SOC_BUTTON_ARRAY) += soc_button_array.o
>  obj-$(CONFIG_INPUT_SPARCSPKR)+= sparcspkr.o
> +obj-$(CONFIG_INPUT_STPMIC1_ONKEY)+= stpmic1_onkey.o
>  obj-$(CONFIG_INPUT_TPS65218_PWRBUTTON)   += tps65218-pwrbutton.o
>  obj-$(CONFIG_INPUT_TWL4030_PWRBUTTON)+= twl4030-pwrbutton.o
>  obj-$(CONFIG_INPUT_TWL4030_VIBRA)+= twl4030-vibra.o
> @@ -81,3 +82,4 @@ obj-$(CONFIG_INPUT_WM831X_ON)   += wm831x-on.o
>  obj-$(CONFIG_INPUT_XEN_KBDDEV_FRONTEND)  += xen-kbdfront.o
>  obj-$(CONFIG_INPUT_YEALINK)  += yealink.o
>  obj-$(CONFIG_INPUT_IDEAPAD_SLIDEBAR) += ideapad_slidebar.o
> +
> diff --git a/drivers/input/misc/stpmic1_onkey.c 
> b/drivers/input/misc/stpmic1_onkey.c
> new file mode 100644
> index 000..871a087
> --- /dev/null
> +++ b/drivers/input/misc/stpmic1_onkey.c
> @@ -0,0 +1,248 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) STMicroelectronics 2018
> +// Author: Pascal Paillet  for STMicroelectronics.
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/**
> + * struct stpmic1_onkey - OnKey data
> + * @pmic:pointer to STPMIC1 PMIC device
> + * @input_dev:   pointer to input device
> + * @irq_falling: irq that we are hooked on to
> + * @irq_rising:  irq that we are hooked on to
> + */
> +struct stpmic1_onkey {
> + struct stpmic1 *pmic;
> + struct input_dev *input_dev;
> + int irq_falling;
> + int irq_rising;
> +};
> +
> +/**
> + * struct pmic_onkey_config - configuration of pmic PONKEYn
> + * @cc_flag_clear:   value to clear CC flag in case of PowerOff
> + * trigger by longkey press
> + * @onkey_pullup_val:value of PONKEY PullUp (active or 
> inactive)
> + * @power_off_time_sec:  value for long press h/w shutdown event
> + */
> +struct pmic_onkey_config {
> + bool cc_flag_clear;
> + u8 onkey_pullup_val;
> + u8 power_off_time_sec;
> +};
> +
> +static irqreturn_t onkey_falling_irq(int irq, void *ponkey)
> +{
> + struct stpmic1_onkey *onkey = ponkey;
> + struct input_dev *input_dev = onkey->input_dev;
> +
> + input_report_key(input_dev, KEY_POWER, 1);
> + pm_wakeup_event(input_dev->dev.parent, 0);
> + input_sync(input_dev);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t onkey_rising_irq(int irq, void *ponkey)
> +{
> + struct stpmic1_onkey *onkey = ponkey;
> + struct input_dev *input_dev = onkey->input_dev;
> +
> + input_report_key(input_dev, KEY_POWER, 0);
> + pm_wakeup_event(input_dev->dev.parent, 0);
> + input_sync(input_dev);
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int stpmic1_onkey_dt_params(struct platform_device *pdev,
> +struct stpmic1_onkey *onkey,
> +struct pmic_onkey_config *config)
> +{
> + struct device *dev = >dev;
> + u32 val;
> +
> + onkey->irq_falling = platform_get_irq_byname(pdev, "onkey-falling");
> + if 

Re: [PATCH V2 6/8] input: stpmic1: add stpmic1 onkey driver

[dmitry.torok...@gmail.com]

Hi Pascal,

On Fri, Sep 07, 2018 at 12:59:45PM +, Pascal PAILLET-LME wrote:
> From: pascal paillet 
> 
> The stpmic1 pmic is able to manage an onkey button. This driver exposes
> the stpmic1 onkey as an input device. It can also be configured to
> shut-down the power supplies on a long key-press with an adjustable
> duration.
> 
> Signed-off-by: pascal paillet 
> ---
> changes in v2:
> * the hardware component has been renamed from stpmu1 to stpmic1 !
> * change headers
> * handle remarks from Dmitry
> * the irq is threaded because is is nested in a thread; I have added a 
> comment.
> Dmitry, I'm sorry, but I did not catch your comment regarding usage of 
> "generic device property API.". could you tell more ? 

You basically do

s/of_property_/device_property_/

and that's it.

> 
>  drivers/input/misc/Kconfig |  11 ++
>  drivers/input/misc/Makefile|   2 +
>  drivers/input/misc/stpmic1_onkey.c | 257 
> +
>  3 files changed, 270 insertions(+)
>  create mode 100644 drivers/input/misc/stpmic1_onkey.c
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index c25606e..cc82dad 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -841,4 +841,15 @@ config INPUT_RAVE_SP_PWRBUTTON
> To compile this driver as a module, choose M here: the
> module will be called rave-sp-pwrbutton.
>  
> +config INPUT_STPMIC1_ONKEY
> + tristate "STPMIC1 PMIC Onkey support"
> + depends on MFD_STPMIC1
> + help
> +   Say Y to enable support of onkey embedded into STPMIC1 PMIC. onkey
> +   can be used to wakeup from low power modes and force a shut-down on
> +   long press.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called stpmic1_onkey.
> +
>  endif
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 72cde28..f0e11b0 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_INPUT_SGI_BTNS)+= sgi_btns.o
>  obj-$(CONFIG_INPUT_SIRFSOC_ONKEY)+= sirfsoc-onkey.o
>  obj-$(CONFIG_INPUT_SOC_BUTTON_ARRAY) += soc_button_array.o
>  obj-$(CONFIG_INPUT_SPARCSPKR)+= sparcspkr.o
> +obj-$(CONFIG_INPUT_STPMIC1_ONKEY)+= stpmic1_onkey.o
>  obj-$(CONFIG_INPUT_TPS65218_PWRBUTTON)   += tps65218-pwrbutton.o
>  obj-$(CONFIG_INPUT_TWL4030_PWRBUTTON)+= twl4030-pwrbutton.o
>  obj-$(CONFIG_INPUT_TWL4030_VIBRA)+= twl4030-vibra.o
> @@ -80,3 +81,4 @@ obj-$(CONFIG_INPUT_WM831X_ON)   += wm831x-on.o
>  obj-$(CONFIG_INPUT_XEN_KBDDEV_FRONTEND)  += xen-kbdfront.o
>  obj-$(CONFIG_INPUT_YEALINK)  += yealink.o
>  obj-$(CONFIG_INPUT_IDEAPAD_SLIDEBAR) += ideapad_slidebar.o
> +
> diff --git a/drivers/input/misc/stpmic1_onkey.c 
> b/drivers/input/misc/stpmic1_onkey.c
> new file mode 100644
> index 000..170d879
> --- /dev/null
> +++ b/drivers/input/misc/stpmic1_onkey.c
> @@ -0,0 +1,257 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) STMicroelectronics 2018
> +// Author: Pascal Paillet  for STMicroelectronics.
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/**
> + * struct stpmic1_onkey - OnKey data
> + * @pmic:pointer to STPMIC1 PMIC device
> + * @input_dev:   pointer to input device
> + * @irq_falling: irq that we are hooked on to
> + * @irq_rising:  irq that we are hooked on to
> + */
> +struct stpmic1_onkey {
> + struct stpmic1_dev *pmic;
> + struct input_dev *input_dev;
> + int irq_falling;
> + int irq_rising;
> +};
> +
> +/**
> + * struct pmic_onkey_config - configuration of pmic PONKEYn
> + * @turnoff_enabled: value to enable turnoff condition
> + * @cc_flag_clear:   value to clear CC flag in case of PowerOff
> + * trigger by longkey press
> + * @onkey_pullup_val:value of PONKEY PullUp (active or 
> inactive)
> + * @long_press_time_val: value for long press h/w shutdown event
> + */
> +struct pmic_onkey_config {
> + bool turnoff_enabled;
> + bool cc_flag_clear;
> + u8 onkey_pullup_val;
> + u8 long_press_time_val;
> +};
> +
> +static irqreturn_t onkey_falling_irq(int irq, void *ponkey)
> +{
> + struct stpmic1_onkey *onkey = ponkey;
> + struct input_dev *input_dev = onkey->input_dev;
> +
> + input_report_key(input_dev, KEY_POWER, 1);
> + pm_wakeup_event(input_dev->dev.parent, 0);
> + input_sync(input_dev);
> +
> + dev_dbg(_dev->dev, "Pwr Onkey Falling Interrupt received\n");
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t onkey_rising_irq(int irq, void *ponkey)
> +{
> + struct stpmic1_onkey *onkey = ponkey;
> + struct input_dev *input_dev = onkey->input_dev;
> +
> + input_report_key(input_dev, KEY_POWER, 0);
> + pm_wakeup_event(input_dev->dev.parent, 0);
> + input_sync(input_dev);
> +

Re: [PATCH V2 6/8] input: stpmic1: add stpmic1 onkey driver

[dmitry.torok...@gmail.com]

Hi Pascal,

On Fri, Sep 07, 2018 at 12:59:45PM +, Pascal PAILLET-LME wrote:
> From: pascal paillet 
> 
> The stpmic1 pmic is able to manage an onkey button. This driver exposes
> the stpmic1 onkey as an input device. It can also be configured to
> shut-down the power supplies on a long key-press with an adjustable
> duration.
> 
> Signed-off-by: pascal paillet 
> ---
> changes in v2:
> * the hardware component has been renamed from stpmu1 to stpmic1 !
> * change headers
> * handle remarks from Dmitry
> * the irq is threaded because is is nested in a thread; I have added a 
> comment.
> Dmitry, I'm sorry, but I did not catch your comment regarding usage of 
> "generic device property API.". could you tell more ? 

You basically do

s/of_property_/device_property_/

and that's it.

> 
>  drivers/input/misc/Kconfig |  11 ++
>  drivers/input/misc/Makefile|   2 +
>  drivers/input/misc/stpmic1_onkey.c | 257 
> +
>  3 files changed, 270 insertions(+)
>  create mode 100644 drivers/input/misc/stpmic1_onkey.c
> 
> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index c25606e..cc82dad 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -841,4 +841,15 @@ config INPUT_RAVE_SP_PWRBUTTON
> To compile this driver as a module, choose M here: the
> module will be called rave-sp-pwrbutton.
>  
> +config INPUT_STPMIC1_ONKEY
> + tristate "STPMIC1 PMIC Onkey support"
> + depends on MFD_STPMIC1
> + help
> +   Say Y to enable support of onkey embedded into STPMIC1 PMIC. onkey
> +   can be used to wakeup from low power modes and force a shut-down on
> +   long press.
> +
> +   To compile this driver as a module, choose M here: the
> +   module will be called stpmic1_onkey.
> +
>  endif
> diff --git a/drivers/input/misc/Makefile b/drivers/input/misc/Makefile
> index 72cde28..f0e11b0 100644
> --- a/drivers/input/misc/Makefile
> +++ b/drivers/input/misc/Makefile
> @@ -70,6 +70,7 @@ obj-$(CONFIG_INPUT_SGI_BTNS)+= sgi_btns.o
>  obj-$(CONFIG_INPUT_SIRFSOC_ONKEY)+= sirfsoc-onkey.o
>  obj-$(CONFIG_INPUT_SOC_BUTTON_ARRAY) += soc_button_array.o
>  obj-$(CONFIG_INPUT_SPARCSPKR)+= sparcspkr.o
> +obj-$(CONFIG_INPUT_STPMIC1_ONKEY)+= stpmic1_onkey.o
>  obj-$(CONFIG_INPUT_TPS65218_PWRBUTTON)   += tps65218-pwrbutton.o
>  obj-$(CONFIG_INPUT_TWL4030_PWRBUTTON)+= twl4030-pwrbutton.o
>  obj-$(CONFIG_INPUT_TWL4030_VIBRA)+= twl4030-vibra.o
> @@ -80,3 +81,4 @@ obj-$(CONFIG_INPUT_WM831X_ON)   += wm831x-on.o
>  obj-$(CONFIG_INPUT_XEN_KBDDEV_FRONTEND)  += xen-kbdfront.o
>  obj-$(CONFIG_INPUT_YEALINK)  += yealink.o
>  obj-$(CONFIG_INPUT_IDEAPAD_SLIDEBAR) += ideapad_slidebar.o
> +
> diff --git a/drivers/input/misc/stpmic1_onkey.c 
> b/drivers/input/misc/stpmic1_onkey.c
> new file mode 100644
> index 000..170d879
> --- /dev/null
> +++ b/drivers/input/misc/stpmic1_onkey.c
> @@ -0,0 +1,257 @@
> +// SPDX-License-Identifier: GPL-2.0
> +// Copyright (C) STMicroelectronics 2018
> +// Author: Pascal Paillet  for STMicroelectronics.
> +
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +#include 
> +
> +/**
> + * struct stpmic1_onkey - OnKey data
> + * @pmic:pointer to STPMIC1 PMIC device
> + * @input_dev:   pointer to input device
> + * @irq_falling: irq that we are hooked on to
> + * @irq_rising:  irq that we are hooked on to
> + */
> +struct stpmic1_onkey {
> + struct stpmic1_dev *pmic;
> + struct input_dev *input_dev;
> + int irq_falling;
> + int irq_rising;
> +};
> +
> +/**
> + * struct pmic_onkey_config - configuration of pmic PONKEYn
> + * @turnoff_enabled: value to enable turnoff condition
> + * @cc_flag_clear:   value to clear CC flag in case of PowerOff
> + * trigger by longkey press
> + * @onkey_pullup_val:value of PONKEY PullUp (active or 
> inactive)
> + * @long_press_time_val: value for long press h/w shutdown event
> + */
> +struct pmic_onkey_config {
> + bool turnoff_enabled;
> + bool cc_flag_clear;
> + u8 onkey_pullup_val;
> + u8 long_press_time_val;
> +};
> +
> +static irqreturn_t onkey_falling_irq(int irq, void *ponkey)
> +{
> + struct stpmic1_onkey *onkey = ponkey;
> + struct input_dev *input_dev = onkey->input_dev;
> +
> + input_report_key(input_dev, KEY_POWER, 1);
> + pm_wakeup_event(input_dev->dev.parent, 0);
> + input_sync(input_dev);
> +
> + dev_dbg(_dev->dev, "Pwr Onkey Falling Interrupt received\n");
> +
> + return IRQ_HANDLED;
> +}
> +
> +static irqreturn_t onkey_rising_irq(int irq, void *ponkey)
> +{
> + struct stpmic1_onkey *onkey = ponkey;
> + struct input_dev *input_dev = onkey->input_dev;
> +
> + input_report_key(input_dev, KEY_POWER, 0);
> + pm_wakeup_event(input_dev->dev.parent, 0);
> + input_sync(input_dev);
> +

Re: [PATCH] Input: ALPS - fix DualPoint flag for 74 03 28 devices

[dmitry.torok...@gmail.com]

Hi,

On Thu, Nov 16, 2017 at 07:27:02AM +, Masaki Ota wrote:
> Hi, Pali, Aaron,
> 
> Current code is correct device setting, previous code is wrong.
> If the trackstick does not work(DUALPOINT flag disable), Device Firmware 
> setting is wrong.
> 
> But recently I received the same report from Thinkpad L570 user, and I 
> checked this device and found this device Firmware setting is wrong. Sorry 
> for our mistake.
> Is your laptop L570 ?
> 
> I will add code that supports the trackstick for this device.

Sorry for resurrecting this old thread, I am just trying to understand
what went wrong here. Is the sequence of "f0 f0 e9" and "ea ea e9" is
important in getting the correct OTP data and we originally got this
order wrong? It is not clear from the original patch and discussion that
this change was intentional.

Thanks.

> 
> Best Regards,
> Masaki Ota
> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com] 
> Sent: Wednesday, November 15, 2017 5:35 PM
> To: 太田 真喜 Masaki Ota <masaki@jp.alps.com>
> Cc: linux-in...@vger.kernel.org; linux-kernel@vger.kernel.org; 
> dmitry.torok...@gmail.com; Aaron Ma <aaron...@canonical.com>
> Subject: Re: [PATCH] Input: ALPS - fix DualPoint flag for 74 03 28 devices
> 
> On Wednesday 15 November 2017 14:34:04 Aaron Ma wrote:
> > There is a regression of commit 4a646580f793 ("Input: ALPS - fix 
> > two-finger scroll breakage"), ALPS device fails with log:
> > 
> > psmouse serio1: alps: Rejected trackstick packet from non DualPoint 
> > device
> > 
> > ALPS device with id "74 03 28" report OTP[0] data 0xCE after commit 
> > 4a646580f793, after restore the OTP reading order, it becomes to 0x10 
> > as before and reports the right flag.
> > 
> > Fixes: 4a646580f793 ("Input: ALPS - fix two-finger scroll breakage")
> > Cc: <sta...@vger.kernel.org>
> > Signed-off-by: Aaron Ma <aaron...@canonical.com>
> > ---
> >  drivers/input/mouse/alps.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c 
> > index 579b899add26..c59b8f7ca2fc 100644
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -2562,8 +2562,8 @@ static int alps_set_defaults_ss4_v2(struct 
> > psmouse *psmouse,
> >  
> > memset(otp, 0, sizeof(otp));
> >  
> > -   if (alps_get_otp_values_ss4_v2(psmouse, 1, [1][0]) ||
> > -   alps_get_otp_values_ss4_v2(psmouse, 0, [0][0]))
> > +   if (alps_get_otp_values_ss4_v2(psmouse, 0, [0][0]) ||
> > +   alps_get_otp_values_ss4_v2(psmouse, 1, [1][0]))
> > return -1;
> >  
> > alps_update_device_area_ss4_v2(otp, priv);
> 
> Masaki Ota, please look at this patch as it partially revert your commit
> 4a646580f793 ("Input: ALPS - fix two-finger scroll breakage"). Something 
> smells here.
> 
> --
> Pali Rohár
> pali.ro...@gmail.com

-- 
Dmitry

Re: [PATCH] Input: ALPS - fix DualPoint flag for 74 03 28 devices

[dmitry.torok...@gmail.com]

Hi,

On Thu, Nov 16, 2017 at 07:27:02AM +, Masaki Ota wrote:
> Hi, Pali, Aaron,
> 
> Current code is correct device setting, previous code is wrong.
> If the trackstick does not work(DUALPOINT flag disable), Device Firmware 
> setting is wrong.
> 
> But recently I received the same report from Thinkpad L570 user, and I 
> checked this device and found this device Firmware setting is wrong. Sorry 
> for our mistake.
> Is your laptop L570 ?
> 
> I will add code that supports the trackstick for this device.

Sorry for resurrecting this old thread, I am just trying to understand
what went wrong here. Is the sequence of "f0 f0 e9" and "ea ea e9" is
important in getting the correct OTP data and we originally got this
order wrong? It is not clear from the original patch and discussion that
this change was intentional.

Thanks.

> 
> Best Regards,
> Masaki Ota
> -Original Message-
> From: Pali Rohár [mailto:pali.ro...@gmail.com] 
> Sent: Wednesday, November 15, 2017 5:35 PM
> To: 太田 真喜 Masaki Ota 
> Cc: linux-in...@vger.kernel.org; linux-kernel@vger.kernel.org; 
> dmitry.torok...@gmail.com; Aaron Ma 
> Subject: Re: [PATCH] Input: ALPS - fix DualPoint flag for 74 03 28 devices
> 
> On Wednesday 15 November 2017 14:34:04 Aaron Ma wrote:
> > There is a regression of commit 4a646580f793 ("Input: ALPS - fix 
> > two-finger scroll breakage"), ALPS device fails with log:
> > 
> > psmouse serio1: alps: Rejected trackstick packet from non DualPoint 
> > device
> > 
> > ALPS device with id "74 03 28" report OTP[0] data 0xCE after commit 
> > 4a646580f793, after restore the OTP reading order, it becomes to 0x10 
> > as before and reports the right flag.
> > 
> > Fixes: 4a646580f793 ("Input: ALPS - fix two-finger scroll breakage")
> > Cc: 
> > Signed-off-by: Aaron Ma 
> > ---
> >  drivers/input/mouse/alps.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/input/mouse/alps.c b/drivers/input/mouse/alps.c 
> > index 579b899add26..c59b8f7ca2fc 100644
> > --- a/drivers/input/mouse/alps.c
> > +++ b/drivers/input/mouse/alps.c
> > @@ -2562,8 +2562,8 @@ static int alps_set_defaults_ss4_v2(struct 
> > psmouse *psmouse,
> >  
> > memset(otp, 0, sizeof(otp));
> >  
> > -   if (alps_get_otp_values_ss4_v2(psmouse, 1, [1][0]) ||
> > -   alps_get_otp_values_ss4_v2(psmouse, 0, [0][0]))
> > +   if (alps_get_otp_values_ss4_v2(psmouse, 0, [0][0]) ||
> > +   alps_get_otp_values_ss4_v2(psmouse, 1, [1][0]))
> > return -1;
> >  
> > alps_update_device_area_ss4_v2(otp, priv);
> 
> Masaki Ota, please look at this patch as it partially revert your commit
> 4a646580f793 ("Input: ALPS - fix two-finger scroll breakage"). Something 
> smells here.
> 
> --
> Pali Rohár
> pali.ro...@gmail.com

-- 
Dmitry

Re: [PATCH] add SERIO_WIPO

[dmitry.torok...@gmail.com]

Hi Michael,

On Fri, Sep 08, 2017 at 11:43:42AM +, Graichen Michael wrote:
> From d4122cfe5f177198ae80d0c973eb29559c762fd3 Mon Sep 17 00:00:00 2001
> From: dev 
> Date: Fri, 8 Sep 2017 13:21:32 +0200
> Subject: [PATCH] add SERIO_WIPO
> 
> ---
> include/uapi/linux/serio.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/serio.h b/include/uapi/linux/serio.h
> index ac217c6..fc32a0f 100644
> --- a/include/uapi/linux/serio.h
> +++ b/include/uapi/linux/serio.h
> @@ -1,6 +1,6 @@
> /*
>   * Copyright (C) 1999-2002 Vojtech Pavlik
> -*
> + *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms of the GNU General Public License version 2 as published 
> by
>   * the Free Software Foundation.
> @@ -81,5 +81,6 @@
> #define SERIO_EGALAX   0x3f
> #define SERIO_PULSE8_CEC   0x40
> #define SERIO_RAINSHADOW_CEC   0x41
> +#define SERIO_WIPO  0x42

What will be using this new definition?

>  #endif /* _UAPI_SERIO_H */
> --
> 2.7.4
> 

Thanks.

-- 
Dmitry

Re: [PATCH] add SERIO_WIPO

[dmitry.torok...@gmail.com]

Hi Michael,

On Fri, Sep 08, 2017 at 11:43:42AM +, Graichen Michael wrote:
> From d4122cfe5f177198ae80d0c973eb29559c762fd3 Mon Sep 17 00:00:00 2001
> From: dev 
> Date: Fri, 8 Sep 2017 13:21:32 +0200
> Subject: [PATCH] add SERIO_WIPO
> 
> ---
> include/uapi/linux/serio.h | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/serio.h b/include/uapi/linux/serio.h
> index ac217c6..fc32a0f 100644
> --- a/include/uapi/linux/serio.h
> +++ b/include/uapi/linux/serio.h
> @@ -1,6 +1,6 @@
> /*
>   * Copyright (C) 1999-2002 Vojtech Pavlik
> -*
> + *
>   * This program is free software; you can redistribute it and/or modify it
>   * under the terms of the GNU General Public License version 2 as published 
> by
>   * the Free Software Foundation.
> @@ -81,5 +81,6 @@
> #define SERIO_EGALAX   0x3f
> #define SERIO_PULSE8_CEC   0x40
> #define SERIO_RAINSHADOW_CEC   0x41
> +#define SERIO_WIPO  0x42

What will be using this new definition?

>  #endif /* _UAPI_SERIO_H */
> --
> 2.7.4
> 

Thanks.

-- 
Dmitry

Re: [PATCH] Input: i8042: add a check in i8042_interrupt

[dmitry.torok...@gmail.com]

Hi,

On Sat, Jun 24, 2017 at 09:38:48AM +, chenhong (N) wrote:
> Description of problem:
> 
> Encounterd BUG case:
> serio: i8042 KBD port at 0x60,0x64 irq 1
> BUG: unable to handle kernel NULL pointer dereference at 0050
> IP: [] _spin_lock_irqsave+0x1f/0x40
> PGD 0
> Oops: 0002 [#1] SMP
> last sysfs file:
> CPU 0
> Modules linked in:
> 
> Pid: 1, comm: swapper Not tainted 2.6.32-358.el6.x86_64 #1 QEMU Standard PC 
> (i440FX + PIIX, 1996)
> RIP: 0010:[]  [] 
> _spin_lock_irqsave+0x1f/0x40
> RSP: 0018:880028203cc0  EFLAGS: 00010082
> RAX: 0001 RBX:  RCX: 
> RDX: 0282 RSI: 0098 RDI: 0050
> RBP: 880028203cc0 R08: 88013e79c000 R09: 880028203ee0
> R10: 0298 R11: 0282 R12: 0050
> R13:  R14:  R15: 0098
> FS:  () GS:88002820() knlGS:
> CS:  0010 DS: 0018 ES: 0018 CR0: 8005003b
> CR2: 0050 CR3: 01a85000 CR4: 001407f0
> DR0:  DR1:  DR2: 
> DR3:  DR6: 0ff0 DR7: 0400
> Process swapper (pid: 1, threadinfo 88013e79c000, task 88013e79b500)
> Stack:
> 880028203d00 813de186 ff02 
>     0098
>  880028203d70 813e0162 880028203d20 8103b8ac
> Call Trace:
> 
>  [] serio_interrupt+0x36/0xa0
> [] i8042_interrupt+0x132/0x3a0
> [] ? kvm_clock_read+0x1c/0x20
> [] ? kvm_clock_get_cycles+0x9/0x10
> [] handle_IRQ_event+0x60/0x170
> [] ? kvm_guest_apic_eoi_write+0x44/0x50
> [] handle_edge_irq+0xde/0x180
> [] handle_irq+0x49/0xa0
> [] do_IRQ+0x6c/0xf0
> [] ret_from_intr+0x0/0x11
> [] ? __do_softirq+0x73/0x1e0
> [] ? hrtimer_interrupt+0x14b/0x260
> [] ? call_softirq+0x1c/0x30
> [] ? do_softirq+0x65/0xa0
> [] ? irq_exit+0x85/0x90
> [] ? smp_apic_timer_interrupt+0x70/0x9b
> [] ? apic_timer_interrupt+0x13/0x20
> Version-Release number of selected component (if applicable):
> 
> RELEASE: 2.6.32-358.el6.x86_64 (Red Hat Enterprise Linux 6.4)
> VERSION: #1 SMP Thu Feb 21 02:37:52 EST 2013
> 
> The problem was also reproduced on Red Hat Enterprise Linux 
> 7.3-x86_64(3.10.0-514.el7)
> 
> Cause of the problem:
> static irqreturn_t i8042_interrupt(int irq, void *dev_id)
> {
> ..
> serio = port->exists ? port->serio : NULL;
>..
>if (likely(port->exists && !filtered))
>   serio_interrupt(serio, data, dfl);
>..
> }
> 
> static int i8042_start(struct serio *serio)
> {
>struct i8042_port *port = serio->port_data;
>port->exists = true;
>mb();
>return 0;
> }
> 
> 
> i8042_probe
> |
> i8042_setup_kbd --> request_irq(i8042_interrupt)
> |
> i8042_register_ports --> serio_queue_event(SERIO_REGISTER_PORT) --> 
> serio_handle_event --> serio_add_port --> i8042_start
> 
> 
> i8042_start which set port->exists be true may be called during the 
> i8042_interrupt function according to the analysis of initialization order.  
> If port->exists is set to be true after the assignment of serio and before 
> calling of serio_interrupt, a NULL pointer will be passed to serio_interrupt. 
> The latest upstream kernel still has this problem.
> 

Thank you for your report and the correct analysis of the problem.

> 
> Solution for this problem
> Add a check for serio to prevent NULL pointer.
> 
> --- a/drivers/input/serio/i8042.c   2017-06-19 16:44:50.890078100 +0800
> +++ b/drivers/input/serio/i8042.c  2017-06-19 16:45:20.230022700 +0800
> @@ -524,7 +524,7 @@ i8042_interruptspin_unlock_irqrestore(_lock, flags);
> -   if (likely(port->exists && !filtered))
> +  if (likely(port->exists && !filtered && serio))

I do not think we need to check port->exists here, just checking serio
should be enough.

Also, please check Documentation/SubmittingPatches when submitting
patches in the future.

I believe we want the version of the patch below.

Thanks!

-- 
Dmitry

Input: i8042 - fix crash at boot time

From: Dmitry Torokhov <dmitry.torok...@gmail.com>

The driver checks port->exists twice in i8042_interrupt(), first when
trying to assign temporary "serio" variable, and second time when deciding
whether it should call serio_interrupt(). The value of port->exists may
change between the 2 checks, and we may end up calling serio_interrupt()
with a NULL pointer:

BUG: unable to handle kernel NULL 

Re: [PATCH] Input: i8042: add a check in i8042_interrupt

[dmitry.torok...@gmail.com]

Hi,

On Sat, Jun 24, 2017 at 09:38:48AM +, chenhong (N) wrote:
> Description of problem:
> 
> Encounterd BUG case:
> serio: i8042 KBD port at 0x60,0x64 irq 1
> BUG: unable to handle kernel NULL pointer dereference at 0050
> IP: [] _spin_lock_irqsave+0x1f/0x40
> PGD 0
> Oops: 0002 [#1] SMP
> last sysfs file:
> CPU 0
> Modules linked in:
> 
> Pid: 1, comm: swapper Not tainted 2.6.32-358.el6.x86_64 #1 QEMU Standard PC 
> (i440FX + PIIX, 1996)
> RIP: 0010:[]  [] 
> _spin_lock_irqsave+0x1f/0x40
> RSP: 0018:880028203cc0  EFLAGS: 00010082
> RAX: 0001 RBX:  RCX: 
> RDX: 0282 RSI: 0098 RDI: 0050
> RBP: 880028203cc0 R08: 88013e79c000 R09: 880028203ee0
> R10: 0298 R11: 0282 R12: 0050
> R13:  R14:  R15: 0098
> FS:  () GS:88002820() knlGS:
> CS:  0010 DS: 0018 ES: 0018 CR0: 8005003b
> CR2: 0050 CR3: 01a85000 CR4: 001407f0
> DR0:  DR1:  DR2: 
> DR3:  DR6: 0ff0 DR7: 0400
> Process swapper (pid: 1, threadinfo 88013e79c000, task 88013e79b500)
> Stack:
> 880028203d00 813de186 ff02 
>     0098
>  880028203d70 813e0162 880028203d20 8103b8ac
> Call Trace:
> 
>  [] serio_interrupt+0x36/0xa0
> [] i8042_interrupt+0x132/0x3a0
> [] ? kvm_clock_read+0x1c/0x20
> [] ? kvm_clock_get_cycles+0x9/0x10
> [] handle_IRQ_event+0x60/0x170
> [] ? kvm_guest_apic_eoi_write+0x44/0x50
> [] handle_edge_irq+0xde/0x180
> [] handle_irq+0x49/0xa0
> [] do_IRQ+0x6c/0xf0
> [] ret_from_intr+0x0/0x11
> [] ? __do_softirq+0x73/0x1e0
> [] ? hrtimer_interrupt+0x14b/0x260
> [] ? call_softirq+0x1c/0x30
> [] ? do_softirq+0x65/0xa0
> [] ? irq_exit+0x85/0x90
> [] ? smp_apic_timer_interrupt+0x70/0x9b
> [] ? apic_timer_interrupt+0x13/0x20
> Version-Release number of selected component (if applicable):
> 
> RELEASE: 2.6.32-358.el6.x86_64 (Red Hat Enterprise Linux 6.4)
> VERSION: #1 SMP Thu Feb 21 02:37:52 EST 2013
> 
> The problem was also reproduced on Red Hat Enterprise Linux 
> 7.3-x86_64(3.10.0-514.el7)
> 
> Cause of the problem:
> static irqreturn_t i8042_interrupt(int irq, void *dev_id)
> {
> ..
> serio = port->exists ? port->serio : NULL;
>..
>if (likely(port->exists && !filtered))
>   serio_interrupt(serio, data, dfl);
>..
> }
> 
> static int i8042_start(struct serio *serio)
> {
>struct i8042_port *port = serio->port_data;
>port->exists = true;
>mb();
>return 0;
> }
> 
> 
> i8042_probe
> |
> i8042_setup_kbd --> request_irq(i8042_interrupt)
> |
> i8042_register_ports --> serio_queue_event(SERIO_REGISTER_PORT) --> 
> serio_handle_event --> serio_add_port --> i8042_start
> 
> 
> i8042_start which set port->exists be true may be called during the 
> i8042_interrupt function according to the analysis of initialization order.  
> If port->exists is set to be true after the assignment of serio and before 
> calling of serio_interrupt, a NULL pointer will be passed to serio_interrupt. 
> The latest upstream kernel still has this problem.
> 

Thank you for your report and the correct analysis of the problem.

> 
> Solution for this problem
> Add a check for serio to prevent NULL pointer.
> 
> --- a/drivers/input/serio/i8042.c   2017-06-19 16:44:50.890078100 +0800
> +++ b/drivers/input/serio/i8042.c  2017-06-19 16:45:20.230022700 +0800
> @@ -524,7 +524,7 @@ i8042_interruptspin_unlock_irqrestore(_lock, flags);
> -   if (likely(port->exists && !filtered))
> +  if (likely(port->exists && !filtered && serio))

I do not think we need to check port->exists here, just checking serio
should be enough.

Also, please check Documentation/SubmittingPatches when submitting
patches in the future.

I believe we want the version of the patch below.

Thanks!

-- 
Dmitry

Input: i8042 - fix crash at boot time

From: Dmitry Torokhov 

The driver checks port->exists twice in i8042_interrupt(), first when
trying to assign temporary "serio" variable, and second time when deciding
whether it should call serio_interrupt(). The value of port->exists may
change between the 2 checks, and we may end up calling serio_interrupt()
with a NULL pointer:

BUG: unable to handle kernel NULL pointer dereference at 0050
IP: [] _spin_lock_irqsave+0x1f/0x40
PGD 0
Oops: 0002 [#1] SMP
last sysfs file:
CPU 0
Modules linked in:

Pid: 1, comm: swapper Not tainted 2.6.32-358.el6.x86_64 #1 QEMU Standard PC 
(i440FX + PIIX, 1996)
RIP: 0010:[]  [] 
_spin_lock_irqsave+0x1f/0x40
RSP: 0018:880028203cc0  EFLAGS: 00010082
RAX: 0001 RBX:  RCX: 
RDX: 0282 RSI: 

Re: [REGRESSION][v4.9.y][v4.10.y] Input: ALPS - set DualPoint flag for 74 03 28 devices

[dmitry.torok...@gmail.com]

Hi Masaki,

On Tue, Mar 07, 2017 at 08:40:20AM +, Masaki Ota wrote:
> Hi, Paul,
> 
> I have modified the source code. 
> 
> There are three issues.
> #1. E7=73 03 28 Button pad device is not assigned "ALPS_BUTTONPAD" Flag 
> correctly.
> #2. E7=73 03 28 devices are not assigned " ALPS_DUALPOINT" Flag correctly.
> #3. E7=73 03 28 device cursor speed is slow.
> 
> Actually, the cause is all the same.
> E7=73 03 28 device does not get correct information from OTP.
> OTP is the device information byte. I specified correct bit.
> 
> Then, I think it is also the issue.
> #4. Stick Pointer cursor speed is fast.
> 
> I will check Stick parameter.

Could you please submit this as a proper patch now that we have
confirmation that it works? Also, I wonder if V8PLUS should be full
protocol instead of a flag.

Thanks.

-- 
Dmitry

Re: [REGRESSION][v4.9.y][v4.10.y] Input: ALPS - set DualPoint flag for 74 03 28 devices

[dmitry.torok...@gmail.com]

Hi Masaki,

On Tue, Mar 07, 2017 at 08:40:20AM +, Masaki Ota wrote:
> Hi, Paul,
> 
> I have modified the source code. 
> 
> There are three issues.
> #1. E7=73 03 28 Button pad device is not assigned "ALPS_BUTTONPAD" Flag 
> correctly.
> #2. E7=73 03 28 devices are not assigned " ALPS_DUALPOINT" Flag correctly.
> #3. E7=73 03 28 device cursor speed is slow.
> 
> Actually, the cause is all the same.
> E7=73 03 28 device does not get correct information from OTP.
> OTP is the device information byte. I specified correct bit.
> 
> Then, I think it is also the issue.
> #4. Stick Pointer cursor speed is fast.
> 
> I will check Stick parameter.

Could you please submit this as a proper patch now that we have
confirmation that it works? Also, I wonder if V8PLUS should be full
protocol instead of a flag.

Thanks.

-- 
Dmitry

Re: [REGRESSION][v4.9.y][v4.10.y] Input: ALPS - set DualPoint flag for 74 03 28 devices

[dmitry.torok...@gmail.com]

Hi Nick,

On Mon, Mar 06, 2017 at 03:59:00PM +, Nick Fletcher wrote:
> HI,
> 
> I installed the two drivers as requested.  Results of dmesg | grep
> 'two-finger' below..

Could you please also tell us whether the devoice only has touchpad, or
if it has touchpad/starckstick combo?

Thanks.

-- 
Dmitry

Re: [REGRESSION][v4.9.y][v4.10.y] Input: ALPS - set DualPoint flag for 74 03 28 devices

[dmitry.torok...@gmail.com]

Hi Nick,

On Mon, Mar 06, 2017 at 03:59:00PM +, Nick Fletcher wrote:
> HI,
> 
> I installed the two drivers as requested.  Results of dmesg | grep
> 'two-finger' below..

Could you please also tell us whether the devoice only has touchpad, or
if it has touchpad/starckstick combo?

Thanks.

-- 
Dmitry

Re: [PATCH v3 5/9] VMware balloon: Show capabilities of balloon and resulting capabilities in the debug-fs node.

[dmitry.torok...@gmail.com]

On Wed, Aug 05, 2015 at 08:22:35PM +, Philip Moltmann wrote:
> Hi,
> 
> > >  MODULE_AUTHOR("VMware, Inc.");
> > >  MODULE_DESCRIPTION("VMware Memory Control (Balloon) Driver");
> > > -MODULE_VERSION("1.3.2.0-k");
> > > +MODULE_VERSION("1.3.3.0-k");
> > 
> > This constant change of module version is annoying, is it really even
> > needed?
> > 
> > I'll take this, but seriously consider just dropping it entirely as 
> > it
> > doesn't mean anything now that the driver is in the kernel tree.
> 
> I think this is meant so that we can track which patches got backported
> into RHEL and SLES.

That assumes that RHEL and SLES always take everything that is in
mainline, which I would not count. I.e if you have a security fix and
also change version to 1.3.4.0-k and RedHat picks it up is the driver
that they have really 1.3.4.0-k? If not then what?

You really need to keep track of the substance of the changes needing to
go into each distribution.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v3 5/9] VMware balloon: Show capabilities of balloon and resulting capabilities in the debug-fs node.

[dmitry.torok...@gmail.com]

On Wed, Aug 05, 2015 at 08:22:35PM +, Philip Moltmann wrote:
 Hi,
 
MODULE_AUTHOR(VMware, Inc.);
MODULE_DESCRIPTION(VMware Memory Control (Balloon) Driver);
   -MODULE_VERSION(1.3.2.0-k);
   +MODULE_VERSION(1.3.3.0-k);
  
  This constant change of module version is annoying, is it really even
  needed?
  
  I'll take this, but seriously consider just dropping it entirely as 
  it
  doesn't mean anything now that the driver is in the kernel tree.
 
 I think this is meant so that we can track which patches got backported
 into RHEL and SLES.

That assumes that RHEL and SLES always take everything that is in
mainline, which I would not count. I.e if you have a security fix and
also change version to 1.3.4.0-k and RedHat picks it up is the driver
that they have really 1.3.4.0-k? If not then what?

You really need to keep track of the substance of the changes needing to
go into each distribution.

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/9] VMware balloon: Do not limit the amount of frees and allocations in non-sleep mode.

[dmitry.torok...@gmail.com]

On Fri, Jun 12, 2015 at 03:40:42PM +, Philip Moltmann wrote:
> Hi,
> > > 
> > > Three improvements contribute to the overall faster speed:
> > > - batched operations reduce the hypervisor overhead per page
> > > - 2m instead of 4k buffer reduce the hypervisor overhead per page
> > > - removing the rate-limiting for non-sleep allocations allows the 
> > > guest
> > > operating system to reclaim memory as fast as it can instead of
> > > artificially limiting it.
> > > 
> > > Any of these improvements is great by itself and helps a lot. The
> > > combination of all three makes a rather dramatic difference.
> > > 
> > > We cause hypervisor-level swapping if the balloon driver does not
> > > reclaim fast enough. As any of these improvements increases 
> > > reclamation
> > > speed, we reduce swapping risk in any case.
> > > 
> > > Unfortunately the first two improvements rely on hypervisor 
> > > support,
> > > the last does not.
> > 
> > As far as I can understand the justification for removing the limit
> > (improvement #3) is that we have #1 and #2, at least that's how I 
> > read
> > the patch description. I am saying: what if you running on a 
> > hypervisor
> > that does not support neither #1 nor #2? What was the first release 
> > that
> > of ESXi supports batching and 2M pages? What about workstation (I 
> > don't
> > recall if it started using ballooning at some point)?
> 
> I see how caused this confusion. The rate limiting was there to not
> cause the guest OS to stall while doing nothing else than ballooning.
> With the batching the time spend ballooning is smaller, hence this is
> less of a problem when these features are available.
> 
> Independent of that the yielding in the ballooning loop should help to
> reduce stalling. Also hypervisor swapping - because we rate limited
> ballooning - causes much worse stalling than in the balloon driver.

OK, fair enough. Please update the patch description to reflect that the
rate limiting is useful on its own and does not require additional
hypervisor changes (although when they present they improve the behavior
even further).

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/9] VMware balloon: Do not limit the amount of frees and allocations in non-sleep mode.

[dmitry.torok...@gmail.com]

On Fri, Jun 12, 2015 at 03:06:56PM +, Philip Moltmann wrote:
> Hi,
> 
> thanks for taking so much interest in this driver. It is quite good
> that our design choices get scrutinized by non-current VMware
> employees.
> 
> 
> > I understand that you negotiate the capabilities between hypervisor 
> > and
> > the balloon driver, however that was not my concern (and I am sorry 
> > that
> > I did not express it properly).
> > 
> > The patch description stated:
> > 
> > "Before this patch the slow memory transfer would cause the 
> > destination
> > VM to have internal swapping until all memory is transferred. Now the
> > memory is transferred fast enough so that the destination VM does not
> > swap."
> > 
> > As far as I understand the improvements in memory transfer speed 
> > hinge
> > on the availability of batched operations, you however remove the 
> > limits
> > on non-sleep allocations unconditionally. Thus my question: on older
> > ESXi's that do not support batcher operations won't this cause VM to
> > start swapping?
> 
> Three improvements contribute to the overall faster speed:
> - batched operations reduce the hypervisor overhead per page
> - 2m instead of 4k buffer reduce the hypervisor overhead per page
> - removing the rate-limiting for non-sleep allocations allows the guest
> operating system to reclaim memory as fast as it can instead of
> artificially limiting it.
> 
> Any of these improvements is great by itself and helps a lot. The
> combination of all three makes a rather dramatic difference.
> 
> We cause hypervisor-level swapping if the balloon driver does not
> reclaim fast enough. As any of these improvements increases reclamation
> speed, we reduce swapping risk in any case.
> 
> Unfortunately the first two improvements rely on hypervisor support,
> the last does not.

As far as I can understand the justification for removing the limit
(improvement #3) is that we have #1 and #2, at least that's how I read
the patch description. I am saying: what if you running on a hypervisor
that does not support neither #1 nor #2? What was the first release that
of ESXi supports batching and 2M pages? What about workstation (I don't
recall if it started using ballooning at some point)?

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/9] VMware balloon: Do not limit the amount of frees and allocations in non-sleep mode.

[dmitry.torok...@gmail.com]

Hi Philip,

On Thu, Jun 11, 2015 at 08:10:07PM +, Philip Moltmann wrote:
> Hi,
> 
> sorry for taking so long to address your concerns.
> 
> > What happens if you run this new driver on an older hypervisor that 
> > does not support batched operations?
> 
> When the driver starts or when it gets reset the driver checks for the
> capabilities of the hypervisor in vmballoon_send_start. Then it resets
> it state and only uses the available functionality.
> 
> A reset happens any time the VM get hot migrated, snapshotted, resumed,
> etc.
> 
> I tested this driver on various versions of ESXi to have a full set of
> possible capabilities.

I understand that you negotiate the capabilities between hypervisor and
the balloon driver, however that was not my concern (and I am sorry that
I did not express it properly).

The patch description stated:

"Before this patch the slow memory transfer would cause the destination
VM to have internal swapping until all memory is transferred. Now the
memory is transferred fast enough so that the destination VM does not
swap."

As far as I understand the improvements in memory transfer speed hinge
on the availability of batched operations, you however remove the limits
on non-sleep allocations unconditionally. Thus my question: on older
ESXi's that do not support batcher operations won't this cause VM to
start swapping?

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/9] VMware balloon: Do not limit the amount of frees and allocations in non-sleep mode.

[dmitry.torok...@gmail.com]

Hi Philip,

On Thu, Jun 11, 2015 at 08:10:07PM +, Philip Moltmann wrote:
 Hi,
 
 sorry for taking so long to address your concerns.
 
  What happens if you run this new driver on an older hypervisor that 
  does not support batched operations?
 
 When the driver starts or when it gets reset the driver checks for the
 capabilities of the hypervisor in vmballoon_send_start. Then it resets
 it state and only uses the available functionality.
 
 A reset happens any time the VM get hot migrated, snapshotted, resumed,
 etc.
 
 I tested this driver on various versions of ESXi to have a full set of
 possible capabilities.

I understand that you negotiate the capabilities between hypervisor and
the balloon driver, however that was not my concern (and I am sorry that
I did not express it properly).

The patch description stated:

Before this patch the slow memory transfer would cause the destination
VM to have internal swapping until all memory is transferred. Now the
memory is transferred fast enough so that the destination VM does not
swap.

As far as I understand the improvements in memory transfer speed hinge
on the availability of batched operations, you however remove the limits
on non-sleep allocations unconditionally. Thus my question: on older
ESXi's that do not support batcher operations won't this cause VM to
start swapping?

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/9] VMware balloon: Do not limit the amount of frees and allocations in non-sleep mode.

[dmitry.torok...@gmail.com]

On Fri, Jun 12, 2015 at 03:40:42PM +, Philip Moltmann wrote:
 Hi,
   
   Three improvements contribute to the overall faster speed:
   - batched operations reduce the hypervisor overhead per page
   - 2m instead of 4k buffer reduce the hypervisor overhead per page
   - removing the rate-limiting for non-sleep allocations allows the 
   guest
   operating system to reclaim memory as fast as it can instead of
   artificially limiting it.
   
   Any of these improvements is great by itself and helps a lot. The
   combination of all three makes a rather dramatic difference.
   
   We cause hypervisor-level swapping if the balloon driver does not
   reclaim fast enough. As any of these improvements increases 
   reclamation
   speed, we reduce swapping risk in any case.
   
   Unfortunately the first two improvements rely on hypervisor 
   support,
   the last does not.
  
  As far as I can understand the justification for removing the limit
  (improvement #3) is that we have #1 and #2, at least that's how I 
  read
  the patch description. I am saying: what if you running on a 
  hypervisor
  that does not support neither #1 nor #2? What was the first release 
  that
  of ESXi supports batching and 2M pages? What about workstation (I 
  don't
  recall if it started using ballooning at some point)?
 
 I see how caused this confusion. The rate limiting was there to not
 cause the guest OS to stall while doing nothing else than ballooning.
 With the batching the time spend ballooning is smaller, hence this is
 less of a problem when these features are available.
 
 Independent of that the yielding in the ballooning loop should help to
 reduce stalling. Also hypervisor swapping - because we rate limited
 ballooning - causes much worse stalling than in the balloon driver.

OK, fair enough. Please update the patch description to reflect that the
rate limiting is useful on its own and does not require additional
hypervisor changes (although when they present they improve the behavior
even further).

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH 6/9] VMware balloon: Do not limit the amount of frees and allocations in non-sleep mode.

[dmitry.torok...@gmail.com]

On Fri, Jun 12, 2015 at 03:06:56PM +, Philip Moltmann wrote:
 Hi,
 
 thanks for taking so much interest in this driver. It is quite good
 that our design choices get scrutinized by non-current VMware
 employees.
 
 
  I understand that you negotiate the capabilities between hypervisor 
  and
  the balloon driver, however that was not my concern (and I am sorry 
  that
  I did not express it properly).
  
  The patch description stated:
  
  Before this patch the slow memory transfer would cause the 
  destination
  VM to have internal swapping until all memory is transferred. Now the
  memory is transferred fast enough so that the destination VM does not
  swap.
  
  As far as I understand the improvements in memory transfer speed 
  hinge
  on the availability of batched operations, you however remove the 
  limits
  on non-sleep allocations unconditionally. Thus my question: on older
  ESXi's that do not support batcher operations won't this cause VM to
  start swapping?
 
 Three improvements contribute to the overall faster speed:
 - batched operations reduce the hypervisor overhead per page
 - 2m instead of 4k buffer reduce the hypervisor overhead per page
 - removing the rate-limiting for non-sleep allocations allows the guest
 operating system to reclaim memory as fast as it can instead of
 artificially limiting it.
 
 Any of these improvements is great by itself and helps a lot. The
 combination of all three makes a rather dramatic difference.
 
 We cause hypervisor-level swapping if the balloon driver does not
 reclaim fast enough. As any of these improvements increases reclamation
 speed, we reduce swapping risk in any case.
 
 Unfortunately the first two improvements rely on hypervisor support,
 the last does not.

As far as I can understand the justification for removing the limit
(improvement #3) is that we have #1 and #2, at least that's how I read
the patch description. I am saying: what if you running on a hypervisor
that does not support neither #1 nor #2? What was the first release that
of ESXi supports batching and 2M pages? What about workstation (I don't
recall if it started using ballooning at some point)?

Thanks.

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v12 01/19] input: cyapa: modify code to following kernel code style

[dmitry.torok...@gmail.com]

Dudley, Jeremiah,

On Thu, Dec 04, 2014 at 05:54:43AM +, Dudley Du wrote:
> Jeremiah,
> 
> I didn't submitted this fix patch, but Dmitry has been helping apply the fix 
> patch quickly, maybe todoay or tommorrow.

The patches that I apply for the next merge window should appear first
in my 'next' branch in

git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git

and then in the linux-next and eventually in mainline.

> 
> For the pathces that I made were worked based off of kernel mainline.

I resolved the conflicts and applied this 1/19 patch, next time you are
re-posting the series could you please base them off my 'next' branch?
It will help me applhy them much more easily.

Thanks!

-- 
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH v12 01/19] input: cyapa: modify code to following kernel code style

[dmitry.torok...@gmail.com]

Dudley, Jeremiah,

On Thu, Dec 04, 2014 at 05:54:43AM +, Dudley Du wrote:
 Jeremiah,
 
 I didn't submitted this fix patch, but Dmitry has been helping apply the fix 
 patch quickly, maybe todoay or tommorrow.

The patches that I apply for the next merge window should appear first
in my 'next' branch in

git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git

and then in the linux-next and eventually in mainline.

 
 For the pathces that I made were worked based off of kernel mainline.

I resolved the conflicts and applied this 1/19 patch, next time you are
re-posting the series could you please base them off my 'next' branch?
It will help me applhy them much more easily.

Thanks!

-- 
Dmitry
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/