On Tue, May 21, 2024 at 11:35:43AM +0200, Greg KH wrote:
> On Tue, May 21, 2024 at 12:30:37AM +0200, Danilo Krummrich wrote:
> > > > +impl<T: DriverOps> Drop for Registration<T> {
> > > > + fn drop(&mut self) {
> > > > + if self.is_registered {
> > > > + // SAFETY: This path only runs if a previous call to `T::register` completed
> > > > + // successfully.
> > > > + unsafe { T::unregister(self.concrete_reg.get()) };
> > >
> > > Can't the rust code ensure that this isn't run if register didn't
> > > succeed? Having a boolean feels really wrong here (can't that race?)
> >
> > No, we want to automatically unregister once this object is destroyed, but not
> > if it was never registered in the first place.
> >
> > This shouldn't be racy, we only ever (un)register things in places like probe()
> > / remove() or module_init() / module_exit().
>
> probe/remove never calls driver_register/unregister on itself, so that's
> not an issue. module_init/exit() does not race with itself and again,
> module_exit() is not called if module_init() fails.
>
> Again, you are adding logic here that is not needed. Or if it really is
> needed, please explain why the C code does not need this today and let's
> work to fix that.
And, to be more explicit, a driver should not have any state of its own,
any "internal state" should always be bound to the device that it is
controlling, NOT generic to the driver itself, as a driver can, and
should, be able to control multiple devices all at the same time without
ever knowing anything is going on. A driver is just code, not data or
state.
Yes, we have bad examples in the kernel where drivers do keep
independent state, but those are the exceptions, never the rule, and I
would argue, should be fixed to not do that. Most were due to being
created before the driver model existed, or programmers being lazy :)
thanks,
greg k-h
