Hi!
> > > +struct rfkill_ops {
> > > +#if defined(CONFIG_RFKILL) || defined(CONFIG_RFKILL_MODULE)
> > > + bool (*poll_hw_block)(void *data);
> > > + bool (*query_state)(void *data);
> > > + void (*set_block)(void *data, bool blocked);
> > > #endif
> >
> > Missing error handling on set_block... could you change that to:
>
> And also on the rest of the hooks. I should have noticed it sooner!
>
> As I said, ACPI platform drivers _do_ get errors when calling the ACPI
> functions, or trying to do an EC transaction. That applies to reads
> just as well as it applies to writes.
And it also doesn't really matter. Yes, the current poll API isn't
prepared to treat errors, but see below for a suggestion.
> I.e. I can get AE_BUSY or something else while querying the thinkpad
> firmware, and then, what should I do to return the data on the
> query_state (and, for that matter, poll_hw_block) hooks?
>
> Since there can be a clear error path from userspace to the rfkill
> driver for reads and writes in many situations (e.g. on all sysfs
> accesses to that driver's rfkill struct), this would be a good reason to
> change the hooks to allow for that error path.
Totally bogus argument -- 99.9% of users will be using rfkill's input
handler which has no way of reacting to errors, and neither does
userspace. And it's wrong too -- the sysfs files don't actually call
into rfkill, they just report the current status.
> Without that, I'd have, for example, to return false (unblocked) on any
> errors from thinkpad-acpi, for safety reasons (a blocked radio is never
> a hazard, but an unblocked one can be). This is the kind of stuff that
> a driver shouldn't need to decide by itself...
Eh? Your first half sentence is correct (up to "on any errors"), the
rest is garbage. Yes, you'd have to return false so that the core
assumes it can do something, but no, it has nothing to do with hazards.
> On a related note, it was not clear to me from looking only at
> rfkill.txt and the linux/rfkill.h headers, how am I supposed to deal
> with the software controlled component of the rfkill controller (the
> hardware rfkill line is quite obvious, though). I know there is a way,
> but I haven't finished reading the core code to know which one, and the
> docs didn't help, so they need an extra paragraph somewhere...
There's a function for that -- but no poll yet.
> Anyway, I suggest changing the hook protypes to this (which does _NOT_
> change the fact that you now track hw and sw separately in the core, but
> lets the rfkill driver deliver both in a single call if it needs to):
>
> int (*poll_block_state)(void *data,
> bool *is_sw_block_active,
> bool *is_hw_block_active);
> int (*query_block_state)(void *data,
> bool *is_sw_block_active,
> bool *is_hw_block_active);
> int (*set_block_state)(void *data, bool do_sw_block);
No way.
I can be convinced to change the prototype of poll_hw_block to
void (*poll_hw_block)(void *data, bool *blocked);
so that drivers can say
"*shrug*, I dunno right now, keep whatever I told you last".
Returning an error, however, is completely pointless.
> The core should ignore the is_* stuff if an error status is returned,
> as a defensive approach against driver bugs: doing it that way is less
> programmer-error-prone than telling people to not touch is_* in case of
> an error.
Bugs happen, and can be fixed; your defensive programming mantra just
makes for messy code that has multiple ways of working "correctly". I
quite obviously disagree -- if you can't read the status don't touch the
variables.
> BTW: it is probably a good idea to document what the driver should do if
> the core tries this on a radio that is hardware blocked, or if the
> driver discovers the radio is hardware blocked the instant it tries to
> software unblock it...
The first case doesn't happen... And it says so in the header file:
* @set_block: turn the transmitter on (blocked == false) or off
* (blocked == true) -- this is called only while the transmitter
* is not hard-blocked. This must be assigned.
The second case _shouldn't_ happen (but can, due to race conditions);
Then you just call set_hw_state before returning.
The third case, which you forgot, works the same as the second and needs
no extra code either.
> It would be nice to have the core export a way to "force" both sw and hw
> states at once, as well. This lets the core optimize the issuing of
> events and other internal changes and avoids bad mojo if the driver does
> sw/hw force calls in the wrong order (which might cause a unblock-block
> fast sequence, etc).
>
> This would let drivers that have the worst case software blocking (it
> exists, hw blocking also exists and is separate, and both sw and hw
> blocking state could change behind the driver's back) work well, without
> causing any extra complexity to the other drivers.
There may be some value in that, do you need polling for both though?
Otherwise you'd need
if (hw_blocked)
set_hw_state(hw_blocked)
set_sw_state(sw_blocked)
else
set_sw_state(sw_blocked)
set_hw_state(hw_blocked)
in the driver which I agree isn't nice and would be better with a
combined function.
> IF you don't want the hooks to return both sw and hw states, you can
> just change the return type from bool to int, and explicitly document
> tha the return status is:
>
> status < 0: error
> status = 0: false, implies no error
> status > 0: true, implies no error
Ick.
If you really do need polling for the sw state as well as the hw state,
I can be convinced to change poll_hw_block to
void (*poll_block)(void *data, bool *hw_blocked, bool *sw_blocked);
You need much better reasons, however, to convince me to add any error
return, I see absolutely no value in that. Neither rfkill's input
handler nor the polling nor userspace has any means to react to errors.
johannes
Attachment:
signature.asc
Description: This is a digitally signed message part
