On Sat, May 26, 2018 at 02:31:23PM +0200, Uwe Kleine-König wrote: > 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? Ah, yeah, if userspace knows that the object has changed, then yes, it will handle it properly. Adding random sysfs files to kobjects without doing that is a very bad idea, and at first glance a while ago, when you asked me on irc, I thought that is what was happening. So all is fine here. It's not the nicest, but it works. thanks, greg k-h -- 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
