Hi Jiri,

Thanks for your comments. I added comments in-lined.

Best regards,

Dennis

On 19.12.2016 10:54, Jiri Kosina wrote:

Hi Dennis,

thanks a lot for the patch.

On Fri, 9 Dec 2016, Dennis Wassenberg wrote:

...

+int hid_lenovo_led_set(enum hid_lenovo_led_type led, bool on) +{

• struct led_classdev *dev = NULL;
• struct lenovo_led_list_entry *entry;
• if (led >= HID_LENOVO_LED_MAX)

• return -EINVAL;
  

• hid_lenovo_initial_leds[led] = on ? LED_FULL : LED_OFF;
• list_for_each_entry(entry, &hid_lenovo_leds, list) {

• if (entry->type == led) {
  

• 	dev = entry->dev;
  

• 	break;
  

• }
  

• }

How exactly is this synchronized against lenovo_remove_tpx1cover()?

In case of that the tpx1cover is disconnected it will be removed from hid_lenovo_leds list. That means not included at the hid_lenovo_leds list. In this case dev is still NULL and the function will return -ENODEV. The static array hid_lenovo_initial_leds is still set to store the current state of a LED type. This is used to set the LED appropriately if the tpx1cover is replugged.

Maybe I should add a mutex to protect the hid_lenovo_leds list operations in hid-lenovo.c to fix the case if a unplug occurred concurrently to setting an led?!

...

• if (!dev)

• return -ENODEV;
  

• if (!dev->brightness_set)

• return -ENODEV;
  

• dev->brightness_set(dev, on ? LED_FULL : LED_OFF);
• return 0;

+} +EXPORT_SYMBOL_GPL(hid_lenovo_led_set);

Does this really need to be exported to the whole universe? (I guess this will be further discussed in 4/4).

No, it is sufficient if thinkpad-helper can access it.