mtdoops and non pre-emptible kernel

> On Thu, 2009-08-27 at 09:59 +0100, Matthew Lear wrote:
>> In any case, yes I saw the two paths the code can go, ie if the mtd
>> device's panic_write() is available and we're in interrupt context then
>> use the panic_write function to write to flash, else use the work queue.
>> The path that my scenario takes is always the latter but the write in
>> context of the work queue never happens.
>>
>> If this is because of the small window in which to perform the write and
>> there are other factors coming into play involving scheduling then
>> obviously that's not a direct issue for the mtdoops code.
>>
>> However, the call to mtdoops_console_sync() (which causes the flash
>> write
>> to be initiated from console_unblank() for the ttyMTD console device) is
>> eventually followed by the panic routine spinning in a tight loop with
>> an
>> mdelay(1). There doesn't appear to be anywhere in this path where
>> schedule() is invoked. Because of running a non pre-emptible kernel,
>> there
>> is no way, certainly that I can see, that a context can switch can
>> happen
>> to allow the jobs in the work queue to be run without at least calling
>> schedule() after calling schedule_work() from within
>> mtdoops_console_sync().
>>
>> Maybe I've missed something :-) but calling schedule() after
>> schedule_work() certainly seems to be the correct approach to at least
>> allow the code to do what it's trying to do, especially on non
>> pre-emptible kernels.
>
> That isn't the right solution since calling schedule() is not something
> allowed at that point in the code, particularly in the middle of a
> kernel panic. We really need to detect that we're about to head into the
> panic spining loop and then call the write function directly. How we do
> that I'm not so sure without going into the code in more detail. I
> suspect something has subtly changed in the kernel meaning that
> particular circumstances no longer works :/
>
That's fair enough. I suppose a possible solution would be to do what I've
done locally for testing, ie:


Obviously this performs the flash write in the same context as that of
panic() and negates the need for the registration and usage of the work
routine, so it would simplify things. However, this would/could effect
what happens during a panic. Clearly the thought behind writing to flash
asynchronously was deemed a requirement.

Perhaps it does need another look and some testing on pre-empt and non
pre-empt kernels just to get a feel for if the mtdoops code is still
working as intended. I know that our kernel will be running with the above
patch applied as it's the only way I can reliably log to flash upon panic.

Cheers,
--  Matt

On Thu, 2009-08-27 at 11:30 +0100, Matthew Lear wrote:
> That's fair enough. I suppose a possible solution would be to do what I've
> done locally for testing, ie:
> 
> diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
> index 1a6b3be..2d734e2 100644
> --- a/drivers/mtd/mtdoops.c
> +++ b/drivers/mtd/mtdoops.c
> @@ -335,7 +335,7 @@ static void mtdoops_console_sync(void)
>                 /* Interrupt context, we're going to panic so try and log */
>                 mtdoops_write(cxt, 1);
>         else
> -               schedule_work(&cxt->work_write);
> +               mtdoops_write(cxt, 0);
>  }

I just refreshed my memory a bit. I'd guess your mtd driver doesn't have
a panic_write function which is highly desirable for this kind of use.

How about some code like this?:

	if (mtd->panic_write && in_interrupt())
		/* Interrupt context, we're going to panic so try and log */
		mtdoops_write(cxt, 1);
	else if (in_interrupt())
		/* Interrupt context but with no panic write function */
                /* We're going to crash anyway so we may as well try and log */
		mtdoops_write(cxt, 0);
	else
		schedule_work(&cxt->work_write);

Cheers,

Richard

> On Thu, 2009-08-27 at 11:30 +0100, Matthew Lear wrote:
>> That's fair enough. I suppose a possible solution would be to do what
>> I've
>> done locally for testing, ie:
>>
>> diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
>> index 1a6b3be..2d734e2 100644
>> --- a/drivers/mtd/mtdoops.c
>> +++ b/drivers/mtd/mtdoops.c
>> @@ -335,7 +335,7 @@ static void mtdoops_console_sync(void)
>>                 /* Interrupt context, we're going to panic so try and
>> log */
>>                 mtdoops_write(cxt, 1);
>>         else
>> -               schedule_work(&cxt->work_write);
>> +               mtdoops_write(cxt, 0);
>>  }
>
> I just refreshed my memory a bit. I'd guess your mtd driver doesn't have
> a panic_write function which is highly desirable for this kind of use.
>
> How about some code like this?:
>
> 	if (mtd->panic_write && in_interrupt())
> 		/* Interrupt context, we're going to panic so try and log */
> 		mtdoops_write(cxt, 1);
> 	else if (in_interrupt())
> 		/* Interrupt context but with no panic write function */
>                 /* We're going to crash anyway so we may as well try and
> log */
> 		mtdoops_write(cxt, 0);
> 	else
> 		schedule_work(&cxt->work_write);
>

Hi Richard. Unfortunately, in all the circumstances that I've been using
to force a panic (insmod a ko which just panics, provide some obviously
wrong boot args on the command line etc), I never seem to be in interrupt
context when the mtdoops code syncs and tries to write to flash, ie:

if (mtd->panic_write)
  printk(KERN_ALERT "we've got panic_write()\n");
else if (in_interrupt())
  printk(KERN_ALERT "we're in interrupt\n");
else
printk(KERN_ALERT "we'll just schedule work\n");

always prints the last message (although I know that my mtd driver doesn't
have a panic_write function so the first was never going to appear). So,
even if I have a panic_write function, it would never get called as the
initial condition would be false.

I agree that your code snippet above makes sense and should probably be
put forward for integration, though :-)

I'm still a bit stuck with the fact that I'm panicking, I'm not in
interrupt context but the jobs in the work queue don't get scheduled to
run because nothing can force a context switch so my flash never gets
written to.

I believe that it is a correct guideline to call schedule() after calling
schedule_work(). Is this a true statement? However, as you say, in a panic
situation this is undesirable. That said though, I partially do think that
the mtd console unblank function should adhere to this behaviour and call
schedule(). What I'm trying to say is, just because the mtd console
unblank is called from panic() isn't this breaking the guidelines on the
usage of schedule_work() (ie not calling schedule()afterwards) and making
assumptions about what else is going on in the system?

Cheers,
--  Matt

Matthew Lear wrote:
>> On Thu, 2009-08-27 at 11:30 +0100, Matthew Lear wrote:
>>> That's fair enough. I suppose a possible solution would be to do what
>>> I've
>>> done locally for testing, ie:
>>>
>>> diff --git a/drivers/mtd/mtdoops.c b/drivers/mtd/mtdoops.c
>>> index 1a6b3be..2d734e2 100644
>>> --- a/drivers/mtd/mtdoops.c
>>> +++ b/drivers/mtd/mtdoops.c
>>> @@ -335,7 +335,7 @@ static void mtdoops_console_sync(void)
>>>                 /* Interrupt context, we're going to panic so try and
>>> log */
>>>                 mtdoops_write(cxt, 1);
>>>         else
>>> -               schedule_work(&cxt->work_write);
>>> +               mtdoops_write(cxt, 0);
>>>  }
>> I just refreshed my memory a bit. I'd guess your mtd driver doesn't have
>> a panic_write function which is highly desirable for this kind of use.
>>
>> How about some code like this?:
>>
>> 	if (mtd->panic_write && in_interrupt())
>> 		/* Interrupt context, we're going to panic so try and log */
>> 		mtdoops_write(cxt, 1);
>> 	else if (in_interrupt())
>> 		/* Interrupt context but with no panic write function */
>>                 /* We're going to crash anyway so we may as well try and
>> log */
>> 		mtdoops_write(cxt, 0);
>> 	else
>> 		schedule_work(&cxt->work_write);
>>
> 
> Hi Richard. Unfortunately, in all the circumstances that I've been using
> to force a panic (insmod a ko which just panics, provide some obviously
> wrong boot args on the command line etc), I never seem to be in interrupt
> context when the mtdoops code syncs and tries to write to flash, ie:
> 
> if (mtd->panic_write)
>   printk(KERN_ALERT "we've got panic_write()\n");
> else if (in_interrupt())
>   printk(KERN_ALERT "we're in interrupt\n");
> else
> printk(KERN_ALERT "we'll just schedule work\n");
> 
> always prints the last message (although I know that my mtd driver doesn't
> have a panic_write function so the first was never going to appear). So,
> even if I have a panic_write function, it would never get called as the
> initial condition would be false.
> 
> I agree that your code snippet above makes sense and should probably be
> put forward for integration, though :-)
> 
> I'm still a bit stuck with the fact that I'm panicking, I'm not in
> interrupt context but the jobs in the work queue don't get scheduled to
> run because nothing can force a context switch so my flash never gets
> written to.
> 
> I believe that it is a correct guideline to call schedule() after calling
> schedule_work(). Is this a true statement? However, as you say, in a panic
> situation this is undesirable. That said though, I partially do think that
> the mtd console unblank function should adhere to this behaviour and call
> schedule(). What I'm trying to say is, just because the mtd console
> unblank is called from panic() isn't this breaking the guidelines on the
> usage of schedule_work() (ie not calling schedule()afterwards) and making
> assumptions about what else is going on in the system?
> 
> Cheers,
> --  Matt
> 

Hi Richard - I plan to raise this issue in the kernel bug tracker to
hopefully open up discussion with people who are much more involved with
recent kernel changes than I. The idea being that there may have been
changes may have effected the way that the mtd oops code behaves during
a panic. As the code stands at the moment it does not seem to work
correctly for me. Obviously if this is due to other aspects of my system
being configured incorrectly then that's absolutely fine and I'll ensure
these are resolved :-) I think this is the right thing to do.
Cheers,
--  Matt