Re: [PATCH 1/3] Input: wacom_w8001 - fix teardown order and device lifetime

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Jason,

On Tue, Oct 13, 2026 at 03:41:53PM -0700, Jason Gerecke wrote:
> On Fri, Oct 9, 2026 at 12:25 PM Dmitry Torokhov
> <dmitry.torokhov@gmail.com> wrote:
> >
> > In w8001_disconnect(), serio_close() was called before unregistering
> > the input devices. When input_unregister_device() calls w8001_close()
> > to shut down the hardware, w8001_command() fails because the serio
> > port is already closed, preventing W8001_CMD_STOP from reaching the
> > controller.
> >
> > Additionally, w8001_connect() pre-allocated both pen_dev and touch_dev
> > upfront before serio_open() so w8001_interrupt() could dereference
> > them unconditionally, and then freed unused input devices mid-probe
> > without synchronizing with w8001_interrupt() when pen or touch was
> > absent. That created a race and use-after-free hazard with
> > w8001_interrupt(), as well as a double-free on the fail4 error path.
> >
> > Fix these issues by guarding event reporting in w8001_interrupt()
> > against NULL device pointers, allocating and registering input devices
> > on demand after querying capabilities, synchronizing device pointer
> > publication and teardown with serio_pause_rx, and unregistering input
> > devices before closing the serio port in both disconnect and probe
> > error unwind paths.
> >
> 
> This patch seems to be pretty complicated for a 'Fixes' and I'd feel a
> lot more comfortable reviewing it if it could be broken into smaller
> chunks. I know that might not be possible, but it's pretty hard to
> audit all the changes here. There's nothing that sets off loud alarms,
> but I think at this level of complexity the best I could do is Ack the
> patch...

Sorry, I do not think it makes sense to split it further...

> 
> > Fixes: 66fd9385ee9c ("Input: wacom_w8001 - simplify w8001_remove")
> > Fixes: e0361b70175f ("Input: wacom_w8001 - split the touch and pen devices into two devices")
> > Reported-by: sashiko-bot@kernel.org
> > Assisted-by: LLM
> 
> Just a note that current recommendations are to include the agent name
> and model version in Assisted-by lines:
> 
> https://elixir.bootlin.com/linux/v7.2.9/source/Documentation/process/coding-assistants.rst#L39

Not anymore since 816d9992d9ed ("coding-assistants: simplify
attribution")

The current format is:

	Assisted-by: LLM [TOOL1] [TOOL2]

as in

	Assisted-by: LLM coccinelle sparse

> > @@ -349,7 +353,9 @@ static irqreturn_t w8001_interrupt(struct serio *serio,
> >                  * sends invalid data packets that are larger than
> >                  * W8001_PKTLEN_TPCPEN. Let's start over again.
> >                  */
> > -               if (!w8001->touch_dev && w8001->idx > W8001_PKTLEN_TPCPEN - 1)
> > +               if (!w8001->touch_dev &&
> > +                   (w8001->data[0] & W8001_TAB_MASK) != W8001_TAB_BYTE &&
> > +                   w8001->idx > W8001_PKTLEN_TPCPEN - 1)
> 
> I'm not an expert in the w8001 protocol and don't have docs handy at
> the moment. Can you explain the purpose of adding this new
> W8001_TAB_BYTE check? I have a vague idea that it might have something
> to do with allowing the query response packets to be processed prior
> to pen/touch being initialized?

Yes, that's right.

Previously, w8001->touch_dev was pre-allocated before querying the
hardware, so !w8001->touch_dev was false during probing. Now that
touch_dev is allocated on demand after the queries complete,
w8001->touch_dev is NULL during probing, and without the W8001_TAB_BYTE
check we'd be junking query responses (which are longer than
W8001_PKTLEN_TPCPEN).


Thanks.

-- 
Dmitry