From: Jean Delvare on
Hi Mark,

On Fri, 2 Apr 2010 17:00:59 +0100, Mark Brown wrote:
> On Fri, Apr 02, 2010 at 11:47:50AM -0400, Jerome Oufella wrote:
>
> Please fix your mail client to word wrap paragraphs, I've manually fixed
> this up here.
>
> > Working on drivers/hwmon/sht15.c, I noticed it would return bogus
> > temperatures in my case, where CONFIG_REGULATOR is not set.
>
> > This is due to the following section in drivers/hwmon/sht15.c:
> >
> > /* If a regulator is available, query what the supply voltage actually is!*/
> > data->reg = regulator_get(data->dev, "vcc");
> > if (!IS_ERR(data->reg)) {
> > ...
>
> > Looking at consumer.h, it appears that regulator_get() returns a
> > pointer to its second argument when CONFIG_REGULATOR is not set.
>
> Right, it's just returning something that won't match IS_ERR().

Ugly design. You're casting a random pointer to struct regulator * and
just hope that the user won't make use of it. Right now, you're safe
because the definition of struct regulator is not public, but assuming
it will stay that way forever is somewhat risky.

I've never seen any API doing things that way, and I can't think of a
sane reason for doing things that way. It's pretty error-prone.

> > What would be the proper way to determine if the returned value is a
> > valid regulator ? Would it be safe to check it against the 2nd
> > argument ?
>
> You're asking the wrong question here. The problem here is not that the
> regulator got stubbed out, the problem is that the sht15 driver is not
> checking the return value of regulator_get_voltage() and so is trying to
> use the error code that was returned as a voltage,

Error code? regulator_get_voltage() returns 0, how is that supposed to
be an error code?

> with predictably poor
> results. It is this function that the driver needs to check, not
> regulator_get().

This goes against the expectations. When I have to get a reference to
something and then use that something, I expect the former to fail is
the something is not available for whatever reason. I don't expect to
have to do the latter to realize that the former did not actually work.

> There are a range of reasons why an error might be
> returned when querying the voltage, all of which would cause the same
> result.

I don't quite follow you here.

> It is not sensible to check the return code of regulator_get() for
> anything other than IS_ERR().

Why can't we have the stub form of regulator_get() return NULL or
ERR_PTR(-ENODEV)? This would be a much friendlier API.

--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jean Delvare on
Hi Mark,

On Fri, 2 Apr 2010 19:51:38 +0100, Mark Brown wrote:
> On Fri, Apr 02, 2010 at 06:44:03PM +0200, Jean Delvare wrote:
> > On Fri, 2 Apr 2010 17:00:59 +0100, Mark Brown wrote:
>
> > > Right, it's just returning something that won't match IS_ERR().
>
> > Ugly design. You're casting a random pointer to struct regulator * and
> > just hope that the user won't make use of it. Right now, you're safe
> > because the definition of struct regulator is not public, but assuming
> > it will stay that way forever is somewhat risky.
>
> If this changes we can always provide a more complex stub; for now we're
> OK.
>
> > I've never seen any API doing things that way, and I can't think of a
> > sane reason for doing things that way. It's pretty error-prone.
>
> All we're doing is stubbing out the API so that common case clients
> (which just want to switch on and off their supplies) don't need to
> either depend on it or include lots of conditional code which could
> easily end up masking error conditions. The stub behaves as always
> on fixed voltage regulators which matches what most systems that do
> not use the regulator API actually have.

OK, I get the idea.

> > > You're asking the wrong question here. The problem here is not that the
> > > regulator got stubbed out, the problem is that the sht15 driver is not
> > > checking the return value of regulator_get_voltage() and so is trying to
> > > use the error code that was returned as a voltage,
>
> > Error code? regulator_get_voltage() returns 0, how is that supposed to
> > be an error code?
>
> It's zero volts which is a reasonable out of range value for a
> regulator. We could change the API to return a signed value but I'm
> having a hard time summoning the enthusiasm to do that myself.

Sorry for playing the devil's advocate here, but deciding that 0V is an
error is pretty arbitrary, and deciding that a negative voltage value
is an error is just as arbitrary. Just because these are not common
cases, doesn't mean they can't happen today or tomorrow for very
specific setups. I'd rather see a more robust way to notifier the
caller that an error happened.

> > > with predictably poor
> > > results. It is this function that the driver needs to check, not
> > > regulator_get().
>
> > This goes against the expectations. When I have to get a reference to
> > something and then use that something, I expect the former to fail is
> > the something is not available for whatever reason. I don't expect to
> > have to do the latter to realize that the former did not actually work.
>
> Well, it did work in the sense that there's something there that
> satisfies the expectations of many users. We could change things so
> that the consumers passed in a set of requirements for the regulator but
> that'd be a bit mroe work and at the minute we're mostly relying on
> silly combinations being filtered out at the hardware design stage.
>
> > > There are a range of reasons why an error might be
> > > returned when querying the voltage, all of which would cause the same
> > > result.
>
> > I don't quite follow you here.
>
> Examples include failure to communicate with the hardware (so we don't
> know what state it's in), or hardware that isn't actually doing
> sufficient regulation to supply a voltage (many regulators support a
> more efficient passthrough mode, and sometimes devices are connected
> directly to unregulated supplies for efficiency reasons even if the
> system mostly uses regulators).
>
> > > It is not sensible to check the return code of regulator_get() for
> > > anything other than IS_ERR().
>
> > Why can't we have the stub form of regulator_get() return NULL or
> > ERR_PTR(-ENODEV)? This would be a much friendlier API.
>
> Not really. The overwhemling majority of users only do simple power
> control, they don't need to get or set voltages and are only interested
> in allowing the system to save some power when they're idle. These
> drivers don't even need the power to actually get switched off, they
> just want to allow that possibility. If we returned an error then all
> these consumers would need to conditionalise all their regulator API
> usage and find it hard to distinguish between not having any power and
> normal operation without the regulator API. With the current approach
> these drivers can have a single code path which is used unconditionally
> and will do the right thing on systems with and without the regulator
> API.

OK, I get it. This indeed rules out ERR_PTR(-ENODEV). But what about
NULL? IS_ERR() doesn't catch NULL, so this wouldn't affect the current
users, as you never dereference the struct regulator pointer in the
stubs anyway. And at least it would let drivers that need it cleanly
differentiate between the cases of availability or unavailability of
the real regulator API. Something like (from the hwmon/sht15 driver):

/* If a regulator is available, query what the supply voltage actually is!*/
data->reg = regulator_get(data->dev, "vcc");
if (data->reg && !IS_ERR(data->reg)) {
data->supply_uV = regulator_get_voltage(data->reg);
regulator_enable(data->reg);
/* setup a notifier block to update this if another device
* causes the voltage to change */
data->nb.notifier_call = &sht15_invalidate_voltage;
ret = regulator_register_notifier(data->reg, &data->nb);
}

looks much better than

/* If a regulator is available, query what the supply voltage actually is!*/
data->reg = regulator_get(data->dev, "vcc");
if (!IS_ERR(data->reg)) {
int voltage = regulator_get_voltage(data->reg);
if (voltage) {
data->supply_uV = voltage;
regulator_enable(data->reg);
/* setup a notifier block to update this if
* another device causes the voltage to change */
data->nb.notifier_call = &sht15_invalidate_voltage;
ret = regulator_register_notifier(data->reg, &data->nb);
}
}

IMHO. One less level of indentation, and one less intermediate
variable, too. What do you think?

> The expectation is that users which have a strong requirement that the
> regulator API does more than this will need to depend on the regulator
> API in Kconfig or have ifdefs and so never see the stubs though they
> should still error check since individual operations may fail or not be
> supported.

I guess we could have ifdefs in hwmon/sht15, yes. But OTOH it looks
weird to have a complete stub API for the CONFIG_REGULATOR=n case, and
still require ifdefs from times to times. This is what make me believe
the stub API isn't good enough.

> Looking again at the stubs we should remove the stubs for at least
> setting voltages and current limits from the API since they don't
> currently do the right thing and I can't think of any useful stub
> behaviour. The get operations are more useful as stubs since some
> analogue parts can usefully have their configuration optimised if we
> know their supply voltages but it's just a nice to have and not a
> requirement.

I second that. The stub API should only contain the minimum set of
functions that is required to keep drivers which don't depend on
CONFIG_REGULATOR ifdef-free. This will make its intended use case
clearer.

Thanks,
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
From: Jean Delvare on
Hi Mark,

On Fri, 2 Apr 2010 21:45:03 +0100, Mark Brown wrote:
> On Fri, Apr 02, 2010 at 09:30:10PM +0200, Jean Delvare wrote:
> > looks much better than
> >
> > /* If a regulator is available, query what the supply voltage actually is!*/
> > data->reg = regulator_get(data->dev, "vcc");
> > if (!IS_ERR(data->reg)) {
> > int voltage = regulator_get_voltage(data->reg);
> > if (voltage) {
> > data->supply_uV = voltage;
> > regulator_enable(data->reg);
> > /* setup a notifier block to update this if
> > * another device causes the voltage to change */
> > data->nb.notifier_call = &sht15_invalidate_voltage;
> > ret = regulator_register_notifier(data->reg, &data->nb);
> > }
> > }
>
> In this case you don't need the if (voltage) check - the code that uses
> supply_uV is going to have to cope with it being set to 0 if the driver
> doesn't just give up, and the enable wants to happen anyway (perhaps
> we've got a switchable supply we can't read the voltage of). It should
> never make any odds if the notifier never gets called since the supply
> could be invariant.

We still need to check if (voltage) to not overwrite the previous value
of data->supply_uV with 0. We will probably do that as an immediate fix
to the sht15 driver. But yes, the rest doesn't need a condition.

Still, I'd prefer if drivers were just able to check for data->reg ==
NULL and skip the whole thing. Would you apply the following patch?

* * * * *

From: Jean Delvare <khali(a)linux-fr.org>
Subject: regulator: Let drivers know when they use the stub API

Have the stub variant of regulator_get() return NULL, so that drivers
can (but still don't have to) handle this case specifically.

Signed-off-by: Jean Delvare <khali(a)linux-fr.org>
Cc: Mark Brown <broonie(a)opensource.wolfsonmicro.com>
Cc: Jerome Oufella <jerome.oufella(a)savoirfairelinux.com>
---
include/linux/regulator/consumer.h | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)

--- linux-2.6.34-rc3.orig/include/linux/regulator/consumer.h 2010-03-09 08:25:30.000000000 +0100
+++ linux-2.6.34-rc3/include/linux/regulator/consumer.h 2010-04-03 17:21:08.000000000 +0200
@@ -183,9 +183,13 @@ static inline struct regulator *__must_c
{
/* Nothing except the stubbed out regulator API should be
* looking at the value except to check if it is an error
- * value so the actual return value doesn't matter.
+ * value. Drivers are free to handle NULL specifically by
+ * skipping all regulator API calls, but they don't have to.
+ * Drivers which don't, should make sure they properly handle
+ * corner cases of the API, such as regulator_get_voltage()
+ * returning 0.
*/
- return (struct regulator *)id;
+ return NULL;
}
static inline void regulator_put(struct regulator *regulator)
{


Thanks,
--
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo(a)vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/