Date:   Mon, 13 Dec 2021 21:40:25 -0800
From:   "dmitry.torokhov" <dmitry.torokhov@gmail.com>
To:     lianzhi chang <changlianzhi@uniontech.com>
Cc:     Greg KH <gregkh@linuxfoundation.org>,
        linux-kernel <linux-kernel@vger.kernel.org>,
        jirislaby <jirislaby@kernel.org>,
        Andy Shevchenko <andriy.shevchenko@linux.intel.com>,
        282827961 <282827961@qq.com>, kernel test robot <lkp@intel.com>,
        Dan Carpenter <dan.carpenter@oracle.com>
Subject: Re: [PATCH v20] tty: Fix the keyboard led light display problem
Message-ID: <YbguSZFyWGUt+Nwh@google.com>
References: <20211213061244.13732-1-changlianzhi@uniontech.com>
 <YbdMTCVybK34HBSz@kroah.com>
 <Ybehbz1LqRqcK+Hh@google.com>
 <tencent_404C1E8253D7D34255D5026C@qq.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <tencent_404C1E8253D7D34255D5026C@qq.com>
Precedence: bulk

On Tue, Dec 14, 2021 at 10:06:38AM +0800, lianzhi chang wrote:
> > On Mon, Dec 13, 2021 at 02:12:44PM +0800, lianzhi chang wrote:
> > > > Use the "ctrl+alt+Fn" key combination to switch the system from tty to
> > > > +#define KDGKBLEDCTL  0x4B73  /* set whether to allow control of keyboard lights */
> > > > +#define KDSKBLEDCTL  0x4B74  /* get whether the keyboard light is currently allowed to be set */
> > >
> > > What userspace code is going to use these new ioctls?
> > >
> > > I still don't understand the problem that this is supposed to be
> > > solving.  How can we have never had a problem until now with regards to
> > > LED settings on keyboards?  What commit caused this to change?  Has it
> > > always been broken for 30 years and no one noticed?
> > 
> > Yes, it's been going on since forever (I guess at least 2.6 where input
> > core was introduced) and nobody really cared as very few people bounce
> > between graphical environment and VTs _and_ use CapsLock or NumLock
> > _and_ have keyboards with these LEDs).
> > 
> > Now, there is a couple-line solution that was in v19, lianzhi chang just
> > needed to drop condition that was being introduced in
> > vt_set_leds_compute_shiftstate(). That would ensure that proper state of
> > LEDs is restored on every VT switch. It also might have resulted in
> > LEDs switching momentarily off before turning on according to the
> > graphical desktop preferences, but I do not thing this is a big issue.
> 
> As you said, in the V19 version, there is a judgment condition in
>  vt_set_leds_compute_shiftstate(). If you delete it, there will be 
> some minor flaws. I always want to solve it perfectly. This may 
> be my selfish intention, but my ability is lacking, which has led 
> to iterating 21 versions.
> 
> > 
> > Now we are building this new infra with new ioctls, etc, for miniscule
> > gain of avoiding this blink. I do not believe this is worth it.
> If the current solution is not worth it, or the solution optimized for v19
> is easier to accept, I will go back to the v19 code to submit.

My recommendation would be to land the adjusted v19 (the one without the
conditional) that fixes original issue of LED state not being restored
on VT switch, and then as a followup, and if you so inclined, offer the
patch that introduces special ioctls to notify keyboard driver that
it should avoid touching LED state completely for a given VT, so that we
can discuss it separately and decide if such functionality is
needed/wanted.

Thanks.

-- 
Dmitry