On Thu, Aug 25, 2016 at 10:03:52AM +0200, Rafał Miłecki wrote:
> +static void usbport_trig_activate(struct led_classdev *led_cdev)
> +{
> + struct usbport_trig_data *usbport_data;
> + int err;
> +
> + usbport_data = kzalloc(sizeof(*usbport_data), GFP_KERNEL);
> + if (!usbport_data)
> + return;
> + usbport_data->led_cdev = led_cdev;
> +
> + /* Storing ports */
> + INIT_LIST_HEAD(&usbport_data->ports);
> + usbport_data->ports_dir = kobject_create_and_add("ports",
> + &led_cdev->dev->kobj);
If you _ever_ find yourself in a driver having to use kobject calls,
it's a HUGE hint that you are doing something wrong.
Hint, you are doing this wrong :)
Use an attribute group if you need a subdirectory in sysfs, using a
"raw" kobject like this hides it from all userspace tools and so no
userspace program can see it (hint, try using libudev to access these
files attached to the device...)
> + if (!usbport_data->ports_dir)
> + goto err_free;
> +
> + /* API for ports management */
> + err = device_create_file(led_cdev->dev, &dev_attr_new_port);
> + if (err)
> + goto err_put_ports;
> + err = device_create_file(led_cdev->dev, &dev_attr_remove_port);
> + if (err)
> + goto err_remove_new_port;
Doesn't this race with userspace and fail? Shouldn't the led core be
creating your leds for you?
> +
> + /* Notifications */
> + usbport_data->nb.notifier_call = usbport_trig_notify,
> + led_cdev->trigger_data = usbport_data;
> + usb_register_notify(&usbport_data->nb);
Don't abuse the USB notifier chain with stuff like this please, is that
really necessary? Why can't your hardware just tell you what USB ports
are in use "out of band"?
> +
> + led_cdev->activated = true;
> + return;
> +
> +err_remove_new_port:
> + device_remove_file(led_cdev->dev, &dev_attr_new_port);
> +err_put_ports:
> + kobject_put(usbport_data->ports_dir);
> +err_free:
> + kfree(usbport_data);
> +}
And again, why is this USB specific? Why can't you use this same
userspace api and codebase for PCI ports? For a sdcard port? For a
thunderbolt port?
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
