Re: [PATCH] rust: regulator: add a bare minimum regulator abstraction

[Daniel Almeida <daniel.almeida@collabora.com>]

Hi Mark,

> On 19 Feb 2025, at 20:05, Mark Brown <broonie@kernel.org> wrote:
> 
> On Wed, Feb 19, 2025 at 01:25:16PM -0300, Daniel Almeida wrote:
> 
>> Add a bare minimum regulator abstraction to be used by Rust drivers.
>> This abstraction adds a small subset of the regulator API, which is
>> thought to be sufficient for the drivers we have now.
> 
> ...
> 
>> Note that each instance of [`Regulator`] obtained from
>> `Regulator::get()` can only be enabled once. This ensures that the calls
>> to enable and disable are perfectly balanced before `regulator_put()` is
>> called, as mandated by the C API.
> 
> This sounds very wrong, a major use case for consumers is to enable and
> disable regulators at runtime.  I would expect that the Rust code might
> want to have it's own reference counting (and probably should), but not
> being able to disable a regulator after enabling it seems to be a great
> big red flag.
> 
>> +    /// Enable the regulator.
>> +    pub fn enable(&mut self) -> Result {
>> +        if self.enabled {
>> +            return Ok(());
>> +        }
> 
> This isn't what the changelog claimed, it's silently ignoring duplicate
> attempts to enable and...
> 
>> +        // SAFETY: Safe as per the type invariants of `Regulator`.
>> +        let res = to_result(unsafe { bindings::regulator_disable(self.inner.as_ptr()) });
>> +        if res.is_ok() {
>> +            self.enabled = false;
>> +        }
> 
> ...we actually support disabling and reenabling multiple times, it's
> just that there's a maximum of one enable at once from the consumer and
> we silently ignore duplicated enables and disables.  That's better but
> it's asking for trouble.

Maybe there was a language issue on my end here.

When I say once, I mean exactly what the source code shows, i.e.: it can be
enabled once, and further calls will be ignored. If it is disabled, it can then be
enabled once, with all further calls ignored, etc.

What I wanted to say is that this abstraction does not support exactly what you
suggested, i.e.: a refcount of how many times it was enabled.

I agree that this is not what the word “once” means. Sorry for the confusion.

> 
> If you're going to support both enable and disable (and all things
> considered you probably should TBH) it would be better to just reference
> count the enables like the C code does and then complain about it if
> someone tries to free the regulator object without underwinding their
> enables, or just possibly just clean them up.  Silently ignoring
> duplicate enables or overflowing disables doesn't seem like it plays
> well with the safety goals that Rust has, it's asking for bugs - a big
> part of the reason the C API does things the way it does is that it will
> notice if the enables and disables aren't joined up.  You can easily
> wind up with one part of the code disabling the regulator underneath
> another part that's still trying to use the hardware and things tend not
> to go well when that happens.

I thought about that, something like:

```
  fn drop(&mut self) {

   // someone probably forgot to call disable?
    while self.enabled_count > 0 {

            // Try to disable, this will decrement self.enabled_count
            if let Err(e) = self.disable() {

              // But don’t loop forever if it fails
              break;
            }
        }
  }
```

I asked for a few opinions privately and was told that “if the C API prefers not to do that
why should you?”

> 
> Perhaps an enable should be an object that's allocated and carried about
> by whatever's holding the reference, I don't know if that plays nicely
> with how Rust likes to ensure safety with this stuff?

As I said, this doesn’t work very well, unless someone corrects my reasoning on a
previous email. Having a single type “Regulator” is probably much saner than “Regulator”
and “EnabledRegulator”.

Your “refcount” idea works just fine, though. I’ll send a v2.

— Daniel