Hi Mike,
On Tue, Feb 21, 2012 at 2:13 PM, Mike Frysinger vapier@gentoo.org wrote:
On Tuesday 21 February 2012 16:55:39 Simon Glass wrote:
On Tue, Feb 21, 2012 at 8:04 AM, Mike Frysinger wrote:
On Tuesday 21 February 2012 01:27:31 Simon Glass wrote:
On Mon, Feb 20, 2012 at 10:11 PM, Mike Frysinger wrote:
--- a/drivers/gpio/sandbox.c +++ b/drivers/gpio/sandbox.c
/* Access routines for GPIO state */ -static u8 *get_gpio(unsigned gp) +static u8 *get_gpio_flags(unsigned gp) {
- assert(gp < CONFIG_SANDBOX_GPIO_COUNT);
- if (gp >= ARRAY_SIZE(state)) {
- static u8 invalid_flags;
- printf("sandbox_gpio: error: invalid gpio %u\n", gp);
- return &invalid_flags;
- }
I think we want to die / fail the test here, but since we don't have any tests I suppose this is ok for now. I like assert() because it halts.
the problem is that assert() is disabled by default, so by default, we get memory corruption :). i tend to agree with your previous statements (in another thread) that the sandbox should, by default, do arg checking since the sandbox env is expected to be tested/developed under.
extending that logic, i think it makes more sense to get output that includes errors but "works" so people can play around more on the command line without interrupting things. after all, i'd rather see an error message if i was in the middle of typing "gpio ..." on the command line but fat fingered the gpio number and typed "gpio set 199" instead of "gpio set 19".
I think the opposite though - it makes more sense to me that the test fails and reports failure, than continues with bogus results.
any test framework worth using will catch the error message that is displayed, so i don't think that's a big deal
How about you leave the assert in as well - then I will be happy enough.
i'm OK with that
OK
Later we could change assert to always bail on sandbox, or make sandbox always build with DEBUG (although we would need to introduce a way of skipping the output).
we'd have to split the knobs so we could do assert() and not debug()
Yes. Later, maybe.
/* set GPIO port 'gp' as an input */ int gpio_direction_input(unsigned gp) {
- debug("%s: gp = %d\n", __func__, gp);
- debug("%s: gp:%u\n", __func__, gp);
if (check_reserved(gp, __func__)) return -1;
set_gpio_flag(gp, GPIOF_OUTPUT, 0);
return 0;
- return sandbox_gpio_set_direction(gp, 0);
Ick, we shouldn't call that function here - it is in the test code. Same below.
The idea is that this state has two completely separate sides to it - by calling the 'test' functions from the 'U-Boot' functions I think you are going to confuse people a lot.
the way i see it is we have the pin state ("state"), we have direct accessor functions with no error checking so other things can directly manipulate that state (sandbox_gpio_xxx), and we have the generic gpio api (gpio_xxx). i don't think both API's should get to directly manipulate the state ... it's more logical (to me) that the generic gpio api be built off the hardware state api rather than grubbin' around directly.
the only place that gets confusing is when we have one structure that ends up storing the hardware state (pin direction/levels) along side the generic gpio state (pin reservation and friendly label names). although, thinking a little more, we should be able to split that out easily enough -- have an array of labels and if a gpio's label is NULL, we know the pin is not reserved.
What I find confusing is that you implement the external API using the test API - I mean confusing for people reading the code. It would be better (if you want functions to access all the state) if there were an internal access functions which the two sets use. I was trying to keep them as separate as possible.
i thought when we discussed this before that sandbox_gpio_xxx isn't a test api. it *could* be used to seed the initial test state, or to fake out a simple device, but it's more than that. if i was writing a proper simulation of a device connected over gpio lines, that device would need direct access to the pin state and thus would utilize sandbox_gpio_xxx. i wouldn't label this simulated device as "test" code.
so once i have it in my mind that that we've got hardware state, and sandbox_gpio_xxx is how you access that state, the gpio_xxx api fits neatly on top of that. it's no different from having memory mapped registers that represent a block of gpio's -- in one case i'm doing readl(foo) and in the other i'm doing sandbox_gpio_xxx().
if i wanted to push the envelope, i'd even move the sandbox_gpio_xxx logic to a dedicated file in arch/sandbox/cpu/gpio.c. then in order to access the "hardware", you'd have to call the sandbox_gpio_xxx funcs.
Well yes GPIO state could go into state.h one day and be accessed from there as I think we previously discussed when I was motivating state.h. Your change makes more sense in that case than it does now. I didn't do that because we don't yet have a way to load/save state so the need for centralising it isn't there (yet).
Let's go with your approach until we get to that point.
Worse is that (for example) set_gpio_flag() now accesses a bogus GPIO and doesn't stop.
well, it issues an error message for the developer to see, but there's no arbitrary memory access going on.
If you add the assert() then I'm happy with this.
- the test API should fault an invalid access and stop
- the external API should assert() and continue.
"assert() and continue" doesn't make sense ... an assert(), by definition, will halt termination when things fail
You mention above that assert() is a nop when DEBUG is not defined - that's what I meant by that comment. In other words the dev chooses whether to abort or not, but it is definitely flagged as an error.
-mike
Regards, Simon