On Sat, May 26, 2018 at 12:09:58AM +0200, Jacek Anaszewski wrote: > Hi Uwe, > > On 05/25/2018 04:41 PM, Uwe Kleine-König wrote: > > Hello, > > > > On Wed, May 16, 2018 at 09:46:20PM +0200, Jacek Anaszewski wrote: > > > On 05/15/2018 10:14 PM, Uwe Kleine-König wrote: > > > > I looked into this driver and it does forbidden stuff. You must not call > > > > device_create_file() for an already bound device. (@gregkh: Would it be > > > > a good idea to issue a warning when device_create_file is called too > > > > late, maybe only if CONFIG_DEBUG_KERNEL=y or CONFIG_DEBUG_SOMETHING?) > > > > > > Many LED triggers create their specific sysfs files this way. > > > > It is still not ok, even if others do it this way. > > Nobody seems to have had any problem with this aspect of LED triggers > implementation so far. At least no complaint has been filed for > last 3 years for sure. > > > > > A sane approach to such trigger parameters would include putting a > > > > const struct attribute_group **groups member into the struct > > > > led_trigger and let led_trigger_register do the right thing with it. > > > > > > > > Not sure what "the right thing" is here. Just adding the group > > > > certainly is not. We'd somehow need a new kobject for this. @gregkh: > > > > Maybe you can comment and point out the way to go here? > > > > > > kobject_uevent_env() is called on led_cdev->dev->kobj from > > > led_trigger_set(). > > > > I'm not 100% confident, but I think this doesn't justify adding > > attributes after registration. > > It seems that the main rationale standing behind the rule "You must not > call device_create_file() for an already bound device" is lack of automatic > userspace notification in this case. > > This is corroborated by the fact that this restriction does not apply > to the addition of a whole group, which entails creation of a directory, > and, in turn, a new kobject. > > In case of led-trigger.c, this shortcoming is obviated by explicitly > sending KOBJ_CHANGE upon trigger addition/removal. It allows to notify > the userspace about respective changes in the state of existence of the > files related to the specific trigger. > > This is how I see it, but I'm happy to see more accurate justification > of the discussed rule. I tried to lure gregkh to reply here, up to now without success. :-| @gregkh: When a trigger is activated, this results in a call to device_add_groups (with my series applied) for the led device, and after that envp[0] = kasprintf(GFP_KERNEL, "TRIGGER=%s", name); envp[1] = NULL; kobject_uevent_env(theledskobj, KOBJ_CHANGE, envp); Is this allowed? Best regards Uwe -- Pengutronix e.K. | Uwe Kleine-König | Industrial Linux Solutions | http://www.pengutronix.de/ | -- To unsubscribe from this list: send the line "unsubscribe linux-serial" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
