Re: [RFC PATCH 02/11] rust: add driver abstraction

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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

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 :)


greg k-h

[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux