This solves a bug with the wireless receiver:
- at plug, the wireless receiver does not know which Wacom device it is
connected to, so it does not actually creates all the LEDs
- when the tablet connects, wacom->wacom_wac.features.type is set to the
proper device so that wacom_wac can understand the packets
- when the receiver is unplugged, it detects that a LED should have been
created (based on wacom->wacom_wac.features.type) and tries to remove
it: crash when removing the sysfs group.
Side effect, we can now safely call several times wacom_destroy_leds().
Signed-off-by: Benjamin Tissoires <[email protected]>
---
drivers/input/tablet/wacom.h | 1 +
drivers/input/tablet/wacom_sys.c | 6 ++++++
2 files changed, 7 insertions(+)
diff --git a/drivers/input/tablet/wacom.h b/drivers/input/tablet/wacom.h
index 70b1e71..f13ad31 100644
--- a/drivers/input/tablet/wacom.h
+++ b/drivers/input/tablet/wacom.h
@@ -120,6 +120,7 @@ struct wacom {
u8 hlv; /* status led brightness button pressed (1..127) */
u8 img_lum; /* OLED matrix display brightness */
} led;
+ bool led_initialized;
struct power_supply battery;
};
diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c
index 94096fd..7087b33 100644
--- a/drivers/input/tablet/wacom_sys.c
+++ b/drivers/input/tablet/wacom_sys.c
@@ -1016,12 +1016,18 @@ static int wacom_initialize_leds(struct wacom *wacom)
return error;
}
wacom_led_control(wacom);
+ wacom->led_initialized = true;
return 0;
}
static void wacom_destroy_leds(struct wacom *wacom)
{
+ if (!wacom->led_initialized)
+ return;
+
+ wacom->led_initialized = false;
+
switch (wacom->wacom_wac.features.type) {
case INTUOS4S:
case INTUOS4:
--
1.9.0
Hi Benjamin,
On Fri, Jun 13, 2014 at 1:29 PM, Benjamin Tissoires
<[email protected]> wrote:
> This solves a bug with the wireless receiver:
Your patch does get rid of the crash. But, it does not fix it at the
root cause.
> - at plug, the wireless receiver does not know which Wacom device it is
> connected to, so it does not actually creates all the LEDs
This is the root cause - LEDs are not created for wireless devices.
Neither here, nor later when a real device is connected.
> - when the tablet connects, wacom->wacom_wac.features.type is set to the
> proper device so that wacom_wac can understand the packets
LEDs are not created for any wireless devices since we don't call
wacom_initialize_leds() when real tablets are connected.
> - when the receiver is unplugged, it detects that a LED should have been
> created (based on wacom->wacom_wac.features.type) and tries to remove
> it: crash when removing the sysfs group.
When receiver is unplugged, it remembers the last tablet that
connected to it. If that tablet supports LEDs, wacom_destroy_leds() is
called. But, no LEDs were initialized. That's why it crashes.
> Side effect, we can now safely call several times wacom_destroy_leds().
led_initialized will never be true if we keep wacom_initialize_leds()
inside probe().
To make initialize_leds() and desctroy_leds() work for wireless
devices, we need to move them to wacom_wireless_work() where we know
what type of tablet is connected/disconnected.
> Signed-off-by: Benjamin Tissoires <[email protected]>
Thank you for your support. But, sorry
NAKed-by: Ping Cheng <[email protected]>
Ping
> ---
> drivers/input/tablet/wacom.h | 1 +
> drivers/input/tablet/wacom_sys.c | 6 ++++++
> 2 files changed, 7 insertions(+)
>
> diff --git a/drivers/input/tablet/wacom.h b/drivers/input/tablet/wacom.h
> index 70b1e71..f13ad31 100644
> --- a/drivers/input/tablet/wacom.h
> +++ b/drivers/input/tablet/wacom.h
> @@ -120,6 +120,7 @@ struct wacom {
> u8 hlv; /* status led brightness button pressed (1..127) */
> u8 img_lum; /* OLED matrix display brightness */
> } led;
> + bool led_initialized;
> struct power_supply battery;
> };
>
> diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c
> index 94096fd..7087b33 100644
> --- a/drivers/input/tablet/wacom_sys.c
> +++ b/drivers/input/tablet/wacom_sys.c
> @@ -1016,12 +1016,18 @@ static int wacom_initialize_leds(struct wacom *wacom)
> return error;
> }
> wacom_led_control(wacom);
> + wacom->led_initialized = true;
>
> return 0;
> }
>
> static void wacom_destroy_leds(struct wacom *wacom)
> {
> + if (!wacom->led_initialized)
> + return;
> +
> + wacom->led_initialized = false;
> +
> switch (wacom->wacom_wac.features.type) {
> case INTUOS4S:
> case INTUOS4:
> --
> 1.9.0
>
Hi Ping,
On Jun 13 2014 or thereabouts, Ping Cheng wrote:
> Hi Benjamin,
>
> On Fri, Jun 13, 2014 at 1:29 PM, Benjamin Tissoires
> <[email protected]> wrote:
> > This solves a bug with the wireless receiver:
>
> Your patch does get rid of the crash. But, it does not fix it at the
> root cause.
True, it fixes the crash, does not get rid of the root cause, but it is
still required. See later.
>
> > - at plug, the wireless receiver does not know which Wacom device it is
> > connected to, so it does not actually creates all the LEDs
>
> This is the root cause - LEDs are not created for wireless devices.
> Neither here, nor later when a real device is connected.
Yep
>
> > - when the tablet connects, wacom->wacom_wac.features.type is set to the
> > proper device so that wacom_wac can understand the packets
>
> LEDs are not created for any wireless devices since we don't call
> wacom_initialize_leds() when real tablets are connected.
Yep
>
> > - when the receiver is unplugged, it detects that a LED should have been
> > created (based on wacom->wacom_wac.features.type) and tries to remove
> > it: crash when removing the sysfs group.
>
> When receiver is unplugged, it remembers the last tablet that
> connected to it. If that tablet supports LEDs, wacom_destroy_leds() is
> called. But, no LEDs were initialized. That's why it crashes.
Yep
>
> > Side effect, we can now safely call several times wacom_destroy_leds().
>
> led_initialized will never be true if we keep wacom_initialize_leds()
> inside probe().
If you are talking about wireless devices only, yes, this value will
never be true. That's the purpose of this patch actually :)
>
> To make initialize_leds() and desctroy_leds() work for wireless
> devices, we need to move them to wacom_wireless_work() where we know
> what type of tablet is connected/disconnected.
Unfortunately, this does not work:
- we *can* create the LEDs sysfs in the wireless work
- this will prevent the crash
- the user will think it can control the LEDs
- actually these LEDs control will do nothing because LEDs handling for
wireless devices goes through the WL interface, and not the PEN
interface of the WL receiver.
- we need to implement a specific led_handling for the wireless receiver
(which will need to know which type of tablet is connected to it)
- we still need a way to say that the pen intf which is declared as the
connected device does not has a LED sysfs.
We could add a quirk to the wacom_wac->features saying that the
connection is wireless, so there is no LED attached to the interface.
Still, there will be some work to do to properly handle the LED
configuration from the WL receiver. This work has to be done in the
kernel, but also in the user space (g-s-d) because now, the led control
will not be on the pen device, but on a plain usb device without input.
If you prefer, I can add such a quirk. But my only concern is here to
fix the kernel oops, not to add features which would require more
testing across different hardware and development on the user space
side.
>
> > Signed-off-by: Benjamin Tissoires <[email protected]>
>
> Thank you for your support. But, sorry
>
> NAKed-by: Ping Cheng <[email protected]>
Please reconsider it or validate the quirk approach I mentioned.
Cheers,
Benjamin
Hi Benjamin,
On Monday, June 16, 2014, Benjamin Tissoires
<[email protected]> wrote:
>
> Hi Ping,
>
> On Jun 13 2014 or thereabouts, Ping Cheng wrote:
> > Hi Benjamin,
> >
> > On Fri, Jun 13, 2014 at 1:29 PM, Benjamin Tissoires
> > <[email protected]> wrote:
> > > This solves a bug with the wireless receiver:
> >
> > Your patch does get rid of the crash. But, it does not fix it at the
> > root cause.
>
> True, it fixes the crash, does not get rid of the root cause, but it is
> still required. See later.
>
> >
> > > - at plug, the wireless receiver does not know which Wacom device it is
> > > connected to, so it does not actually creates all the LEDs
> >
> > This is the root cause - LEDs are not created for wireless devices.
> > Neither here, nor later when a real device is connected.
>
> Yep
>
> >
> > > - when the tablet connects, wacom->wacom_wac.features.type is set to the
> > > proper device so that wacom_wac can understand the packets
> >
> > LEDs are not created for any wireless devices since we don't call
> > wacom_initialize_leds() when real tablets are connected.
>
> Yep
>
> >
> > > - when the receiver is unplugged, it detects that a LED should have been
> > > created (based on wacom->wacom_wac.features.type) and tries to remove
> > > it: crash when removing the sysfs group.
> >
> > When receiver is unplugged, it remembers the last tablet that
> > connected to it. If that tablet supports LEDs, wacom_destroy_leds() is
> > called. But, no LEDs were initialized. That's why it crashes.
>
> Yep
>
> >
> > > Side effect, we can now safely call several times wacom_destroy_leds().
> >
> > led_initialized will never be true if we keep wacom_initialize_leds()
> > inside probe().
>
> If you are talking about wireless devices only, yes, this value will
> never be true. That's the purpose of this patch actually :)
>
> >
> > To make initialize_leds() and desctroy_leds() work for wireless
> > devices, we need to move them to wacom_wireless_work() where we know
> > what type of tablet is connected/disconnected.
>
> Unfortunately, this does not work:
> - we *can* create the LEDs sysfs in the wireless work
> - this will prevent the crash
> - the user will think it can control the LEDs
> - actually these LEDs control will do nothing because LEDs handling for
> wireless devices goes through the WL interface, and not the PEN
> interface of the WL receiver.
> - we need to implement a specific led_handling for the wireless receiver
> (which will need to know which type of tablet is connected to it)
> - we still need a way to say that the pen intf which is declared as the
> connected device does not has a LED sysfs.
>
> We could add a quirk to the wacom_wac->features saying that the
> connection is wireless, so there is no LED attached to the interface.
>
> Still, there will be some work to do to properly handle the LED
> configuration from the WL receiver. This work has to be done in the
> kernel, but also in the user space (g-s-d) because now, the led control
> will not be on the pen device, but on a plain usb device without input.
>
> If you prefer, I can add such a quirk. But my only concern is here to
> fix the kernel oops, not to add features which would require more
> testing across different hardware and development on the user space
> side.
>
> >
> > > Signed-off-by: Benjamin Tissoires <[email protected]>
> >
> > Thank you for your support. But, sorry
> >
> > NAKed-by: Ping Cheng <[email protected]>
>
> Please reconsider it or validate the quirk approach I mentioned.
I see your point. My concern was once we fixed the crash here, we
would forget to fix the actual problem.
For the time being, let's make sure we can move on. So,
Acked-by: Ping Cheng <[email protected]>.
Cheers,
Ping