Page MenuHomeFreeBSD

gpiobus: gpio_pin_release: convert checks to KASSERTs
AcceptedPublic

Authored by vexeduxr on Mon, Jun 16, 7:45 AM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Jul 1, 8:04 PM
Unknown Object (File)
Mon, Jun 30, 6:07 PM
Unknown Object (File)
Fri, Jun 27, 5:11 PM
Unknown Object (File)
Wed, Jun 25, 6:48 PM
Unknown Object (File)
Sun, Jun 22, 11:30 PM
Unknown Object (File)
Sun, Jun 22, 8:47 AM
Unknown Object (File)
Tue, Jun 17, 8:14 PM
Unknown Object (File)
Tue, Jun 17, 8:13 PM
Subscribers

Details

Reviewers
imp
wulf
mmel

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 64980
Build 61863: arc lint + arc unit

Event Timeline

vexeduxr created this revision.

Motivation? Why do we need gpio_pin_release to return errors?
This creates the problem of freeing the gpio structure in case of an error.
IMHO, I think all errors in gpiobus_release_pin() and gpiobus_acquire_pin() should be converted to panic().
Also passing NULL to gpio_pin_release() should be panic().

Motivation? Why do we need gpio_pin_release to return errors?

It was to have it in line with gpio_pin_acquire. I found it a little weird that only one of them would report errors.

This creates the problem of freeing the gpio structure in case of an error.

At first I was freeing on both failure and success, but I wasn't sure that was the right thing to do..

IMHO, I think all errors in gpiobus_release_pin() and gpiobus_acquire_pin() should be converted to panic().
Also passing NULL to gpio_pin_release() should be panic().

I agree for gpiobus_release_pin, since they're all programming errors. For gpiobus_acquire_pin, I think attempting to acquire a pin twice should just return an error to the caller, it's possible the caller can just recover.

Motivation? Why do we need gpio_pin_release to return errors?

It was to have it in line with gpio_pin_acquire. I found it a little weird that only one of them would report errors.

All *release* functions are usually very difficult to recover, especially if they are called throw multiple layers.

This creates the problem of freeing the gpio structure in case of an error.

At first I was freeing on both failure and success, but I wasn't sure that was the right thing to do..

This leading to a situation where the malloc cannot be released in any way. If gpiobus_release_pin() fails once, then it will always fail. But this is irrelevant if we replace errors with panic...

IMHO, I think all errors in gpiobus_release_pin() and gpiobus_acquire_pin() should be converted to panic().
Also passing NULL to gpio_pin_release() should be panic().

I agree for gpiobus_release_pin, since they're all programming errors. For gpiobus_acquire_pin, I think attempting to acquire a pin twice should just return an error to the caller, it's possible the caller can just recover.

Yep, exactly. I missed the multiple acquisition case originally. Only error value should be better than -1.

vexeduxr retitled this revision from gpiobus: allow gpio_pin_release to return errors to gpiobus: gpio_pin_release: convert checks to KASSERTs.Thu, Jun 19, 2:27 PM

I went for a KASSERT instead of a panic since it's what the other gpio_* functions do. On a non-INVARIANTS kernel, if gpio is actually NULL, it will panic when we try to dereference it anyways.

This revision is now accepted and ready to land.Thu, Jun 19, 2:37 PM