Re: [PATCH] Input - wacom: put a flag when the led are initialized

[Ping Cheng]

Hi Benjamin,

On Fri, Jun 13, 2014 at 1:29 PM, Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> 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 <benjamin.tissoires@xxxxxxxxxx>

Thank you for your support. But, sorry

NAKed-by: Ping Cheng <pingc@xxxxxxxxx>

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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/