On Tue, Feb 11, 2025 at 10:49:34AM -0500, Kurt Borja wrote:
> On Tue Feb 11, 2025 at 10:29 AM -05, Zijun Hu wrote:
> > On 2025/2/10 22:29, Kurt Borja wrote:
> >>> +
> >>> + ret = device_add(dev);
> >>> + if (ret) {
> >>> + pr_err("%s: device_add for faux device '%s' failed with %d\n",
> >>> + __func__, name, ret);
> >>> + put_device(dev);
> >>> + return NULL;
> >>> + }
> >> Now that the probe is synchronous, what do you think about returning
> >> -ENODEV if the device failed to bind to the driver?
> >>
> >
> > Result of device registering @ret is not, should not be, effected by
> > "device binding driver (probe result)"
> >
> > if device binging driver failed, you may return -ENODEV in
> > faux_ops->probe(). not here.
>
> After thinking about this discussion, I understand why this might be the
> expected behavior. I'm thinking about very simple modules that would
> remain loaded even if the probe fails. But of course this may cause
> problems to other modules.
>
> In the end, this is just my opinion so it would be up to Greg to decide.
> However, there is still an issue with the groups added to the device,
> which a user might expect they are tied to an "attached" device
> lifetime and this currently not the case.
I agree with you here, this could be confusing and cause problems, and
we should be creating apis that "work properly and simply". Having a
probe callback is good to add device data like you mention, so that you
can properly add the information before the sysfs files are accessed,
removing that race condition.
> >> This would be useful for modules that may want to unload if the probe
> >> fails.
> >
> > may need to root cause if probe failure happens.
> >
> > how to unload module automatically if probe() failure ?
>
> If we check for !dev->driver, a module might propagate an error to the
> module_init, thus making it fail to load.
Agreed. Thanks so much for your review comments, they are greatly
appreciated. When I get time next week I'll make these changes and send
out some patches.
thanks,
greg k-h
