Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 12 Aug 2020 18:21:25 -0700
From:   Dmitry Torokhov <dmitry.torokhov@gmail.com>
To:     Raul Rangel <rrangel@google.com>
Cc:     linux-input@vger.kernel.org,
        linux-kernel <linux-kernel@vger.kernel.org>,
        "S, Shirish" <Shirish.S@amd.com>
Subject: Re: PS/2 + i8042 intermixing commands
Message-ID: <20200813012125.GV1665100@dtor-ws>
References: <CAHQZ30ANTeM-pgdYZ4AbgxsnevBJnJgKZ1Kg+Uy8oSXZUvz=og@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAHQZ30ANTeM-pgdYZ4AbgxsnevBJnJgKZ1Kg+Uy8oSXZUvz=og@mail.gmail.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Aug 06, 2020 at 09:28:41AM -0600, Raul Rangel wrote:
> >
> > <- atkbd_event_work->atkbd_set_leds
> > [KB recv data: 0xed] # CMD 10
> > <- Wait, where is the data?
> >
> > <- Continuation of i8042_port_close?
> > [KB recv cmd: 0x60] # CMD #14
> > [KB recv data: 0x43]
> > [KB eaten by STATE_WRITE_CMD_BYTE: 0x43]
> > [KB set CTR_RAM(0x00)=0x43 (old:0x41)]
> > [AUX IRQ enable]
> >
> > <- Here is the data!
> > [KB recv data: 0x00] # CMD 10 (data)
> > [KB Unsupported i8042 data 0x00]
> > [KB recv data: 0x00] <- Did the host retry?
> > [KB Unsupported i8042 data 0x00]
> >
> > <- atkbd_event_work->atkbd_set_repeat_rate
> > [KB recv data: 0xf3] # CMD #11
> > [KB recv data: 0x00]
> > [KB eaten by STATE_SETREP: 0x00]
> >
> > [KB recv cmd: 0xd4] # CMD #15
> > [KB recv data: 0xf2]
> > [STATE_SEND_TO_MOUSE: 0xf2]
> 
> As you can see CMD #10 starts between #13 and #14, and then completes
> after #14. Is this expected behavior?
> 
> I'm not quite sure if #13 and #14 are coming from i8042_port_close. I
> don't have a function trace available, but it seems to fit.
> 
> I found this comment:
> /*
>  * Writers to AUX and KBD ports as well as users issuing i8042_command
>  * directly should acquire i8042_mutex (by means of calling
>  * i8042_lock_chip() and i8042_unlock_ship() helpers) to ensure that
>  * they do not disturb each other (unfortunately in many i8042
>  * implementations write to one of the ports will immediately abort
>  * command that is being processed by another port).
>  */
> static DEFINE_MUTEX(i8042_mutex);
> 
> Does that not mean that i8042_port_close, i8042_enable_kbd_port,
> i8042_enable_aux_port, + any other function that calls i8042_command
> should be taking the lock before calling i8042_command?

Yeah, I think this is right. When I added the mutex it was because 2
deices were clashing with each other and I did not consider that closing
port in the i8042 driver itself may also disturb in-flight command.

Thanks.

-- 
Dmitry