Bug 5854

bugme-daemon@bugzilla.kernel.org wrote:
>
> http://bugzilla.kernel.org/show_bug.cgi?id=5854
>

Me too.

This is because the kernel thread which request_firmware() creates is stuck
in D state:

root       954  0.0  0.0     0    0 ?        DW   01:55   0:00 [firmware/dell_r]

Here:


firmware/dell D 00000002     0   954      1           972   198 (L-TLB)
c1a83f3c c1a83f2c 00000002 00000002 00000000 cfd8983f cf9429d8 c1a83efc 
       c055a880 c19cf800 000000d0 cf9429d8 000081a4 00000000 c1a83f20 c019c87d 
       c1af0cf4 c057b37c c1204420 00000000 00000000 7dd06300 003d08bd cff68030 
Call Trace:
 [<c0498a2e>] wait_for_completion+0x8e/0xc8
 [<c02e25fd>] _request_firmware+0x15b/0x18a
 [<c02e26d4>] request_firmware_work_func+0x4e/0xef
 [<c01010e9>] kernel_thread_helper+0x5/0xb

Abhay, what are we trying to do there?

The code seems to be infinitely retrying the request_firmware too.  Could
you please explain the logic behind this, and take a look at fixing it up?

Thanks.

Reply-To: Abhay_Salunke@Dell.com

> bugme-daemon@bugzilla.kernel.org wrote:
> >
> > http://bugzilla.kernel.org/show_bug.cgi?id=5854
> >
> 
> Me too.
> 
> This is because the kernel thread which request_firmware() creates is
> stuck
> in D state:
> 
> root       954  0.0  0.0     0    0 ?        DW   01:55   0:00
> [firmware/dell_r]
> 
> Here:
> 
> 
> firmware/dell D 00000002     0   954      1           972   198
(L-TLB)
> c1a83f3c c1a83f2c 00000002 00000002 00000000 cfd8983f cf9429d8
c1a83efc
>        c055a880 c19cf800 000000d0 cf9429d8 000081a4 00000000 c1a83f20
> c019c87d
>        c1af0cf4 c057b37c c1204420 00000000 00000000 7dd06300 003d08bd
> cff68030
> Call Trace:
>  [<c0498a2e>] wait_for_completion+0x8e/0xc8
>  [<c02e25fd>] _request_firmware+0x15b/0x18a
>  [<c02e26d4>] request_firmware_work_func+0x4e/0xef
>  [<c01010e9>] kernel_thread_helper+0x5/0xb
> 
> Abhay, what are we trying to do there?

The thread is waiting for completion signal; the completion happens when
the user will echo some data in to /sys/class/firmware/dell_rbu/loading.
Wait_for_completion should put the thread in a sleep state not sure why
the load average goes up to 1.
> 
> The code seems to be infinitely retrying the request_firmware too.
Could
> you please explain the logic behind this, and take a look at fixing it
up?
> 
request_firmware is first called when the dell_rbu driver is loaded;
this created the dell_rbu entries in /sys/class/firmware/. To make the
entries persistent as long as the dell_rbu driver remains loaded, they
get recreated in a callback function which gets called as soon as the
user finishes loading the image by echoing 0 to
/sys/class/firmware/dell_rbu/loading.

I am working on another patch which does not require dell_rbu to create
entries every time and lets the user create the entries. The dell_rbu
driver creates entry in /sys/devices/platform/dell_rbu/image_type at
load time. 
The user needs to first echo the required image_type "mono" or "packer"
in to this entry and then the user must echo "init" in to the image_type
entry. 
Echoing "init" currently recreates the dell_rbu entries in
/sys/class/firmware. Thus in the new patch there is no need to create
the dell_rbu entries at load time. The user must always echo "init"
before going ahead with a monolithic image update. And in case of packet
update method the user need to echo "init" in to
/sys/devices/platform/dell_rbu/image_type before starting to download
every packet.  

This method will avoid the need for the dell_rbu driver to recreate the
firmware entries on its own; which will also prevent the load average
from climbing as the kernel thread now is short lived and gets created
only on demand.

I will finish and test this patch and send and update.

Thanks
Abhay

<Abhay_Salunke@Dell.com> wrote:
>
> > bugme-daemon@bugzilla.kernel.org wrote:
> > >
> > > http://bugzilla.kernel.org/show_bug.cgi?id=5854
> > >
> > 
> > Me too.
> > 
> > This is because the kernel thread which request_firmware() creates is
> > stuck
> > in D state:
> > 
> > root       954  0.0  0.0     0    0 ?        DW   01:55   0:00
> > [firmware/dell_r]
> > 
> > Here:
> > 
> > 
> > firmware/dell D 00000002     0   954      1           972   198
> (L-TLB)
> > c1a83f3c c1a83f2c 00000002 00000002 00000000 cfd8983f cf9429d8
> c1a83efc
> >        c055a880 c19cf800 000000d0 cf9429d8 000081a4 00000000 c1a83f20
> > c019c87d
> >        c1af0cf4 c057b37c c1204420 00000000 00000000 7dd06300 003d08bd
> > cff68030
> > Call Trace:
> >  [<c0498a2e>] wait_for_completion+0x8e/0xc8
> >  [<c02e25fd>] _request_firmware+0x15b/0x18a
> >  [<c02e26d4>] request_firmware_work_func+0x4e/0xef
> >  [<c01010e9>] kernel_thread_helper+0x5/0xb
> > 
> > Abhay, what are we trying to do there?
> 
> The thread is waiting for completion signal; the completion happens when
> the user will echo some data in to /sys/class/firmware/dell_rbu/loading.
> Wait_for_completion should put the thread in a sleep state not sure why
> the load average goes up to 1.
>

Because wait_for_completion() will wait in state TASK_UNINTERRUPTIBLE (ie:
"D" state).  Such a task contributes to the load average.

> > 
> > The code seems to be infinitely retrying the request_firmware too.
> Could
> > you please explain the logic behind this, and take a look at fixing it
> up?
> > 
> request_firmware is first called when the dell_rbu driver is loaded;
> this created the dell_rbu entries in /sys/class/firmware/. To make the
> entries persistent as long as the dell_rbu driver remains loaded, they
> get recreated in a callback function which gets called as soon as the
> user finishes loading the image by echoing 0 to
> /sys/class/firmware/dell_rbu/loading.
> 
> I am working on another patch which does not require dell_rbu to create
> entries every time and lets the user create the entries. The dell_rbu
> driver creates entry in /sys/devices/platform/dell_rbu/image_type at
> load time. 
> The user needs to first echo the required image_type "mono" or "packer"
> in to this entry and then the user must echo "init" in to the image_type
> entry. 
> Echoing "init" currently recreates the dell_rbu entries in
> /sys/class/firmware. Thus in the new patch there is no need to create
> the dell_rbu entries at load time. The user must always echo "init"
> before going ahead with a monolithic image update. And in case of packet
> update method the user need to echo "init" in to
> /sys/devices/platform/dell_rbu/image_type before starting to download
> every packet.  
> 
> This method will avoid the need for the dell_rbu driver to recreate the
> firmware entries on its own; which will also prevent the load average
> from climbing as the kernel thread now is short lived and gets created
> only on demand.
> 
> I will finish and test this patch and send and update.
> 

ok.  Please carefully describe the new design in the changelog for that
patch.  The current design kind of snuck through without anyone noticing
that it had these problems.

Reply-To: Abhay_Salunke@Dell.com

> -----Original Message-----
> From: Andrew Morton [mailto:akpm@osdl.org]
> Sent: Monday, January 09, 2006 6:29 PM
> To: Salunke, Abhay
> Cc: bugme-daemon@bugzilla.kernel.org
> Subject: Re: [Bugme-new] [Bug 5854] New: dell firmware loader makes
load
> grow up to 1, and permanently stay there
> 
> <Abhay_Salunke@Dell.com> wrote:
> >
> > > bugme-daemon@bugzilla.kernel.org wrote:
> > > >
> > > > http://bugzilla.kernel.org/show_bug.cgi?id=5854
> > > >
> > >
> > > Me too.
> > >
> > > This is because the kernel thread which request_firmware() creates
is
> > > stuck
> > > in D state:
> > >
> > > root       954  0.0  0.0     0    0 ?        DW   01:55   0:00
> > > [firmware/dell_r]
> > >
> > > Here:
> > >
> > >
> > > firmware/dell D 00000002     0   954      1           972   198
> > (L-TLB)
> > > c1a83f3c c1a83f2c 00000002 00000002 00000000 cfd8983f cf9429d8
> > c1a83efc
> > >        c055a880 c19cf800 000000d0 cf9429d8 000081a4 00000000
c1a83f20
> > > c019c87d
> > >        c1af0cf4 c057b37c c1204420 00000000 00000000 7dd06300
003d08bd
> > > cff68030
> > > Call Trace:
> > >  [<c0498a2e>] wait_for_completion+0x8e/0xc8
> > >  [<c02e25fd>] _request_firmware+0x15b/0x18a
> > >  [<c02e26d4>] request_firmware_work_func+0x4e/0xef
> > >  [<c01010e9>] kernel_thread_helper+0x5/0xb
> > >
> > > Abhay, what are we trying to do there?
> >
> > The thread is waiting for completion signal; the completion happens
when
> > the user will echo some data in to
/sys/class/firmware/dell_rbu/loading.
> > Wait_for_completion should put the thread in a sleep state not sure
why
> > the load average goes up to 1.
> >
> 
> Because wait_for_completion() will wait in state TASK_UNINTERRUPTIBLE
(ie:
> "D" state).  Such a task contributes to the load average.
> 
> > >
> > > The code seems to be infinitely retrying the request_firmware too.
> > Could
> > > you please explain the logic behind this, and take a look at
fixing it
> > up?
> > >
> > request_firmware is first called when the dell_rbu driver is loaded;
> > this created the dell_rbu entries in /sys/class/firmware/. To make
the
> > entries persistent as long as the dell_rbu driver remains loaded,
they
> > get recreated in a callback function which gets called as soon as
the
> > user finishes loading the image by echoing 0 to
> > /sys/class/firmware/dell_rbu/loading.
> >
> > I am working on another patch which does not require dell_rbu to
create
> > entries every time and lets the user create the entries. The
dell_rbu
> > driver creates entry in /sys/devices/platform/dell_rbu/image_type at
> > load time.
> > The user needs to first echo the required image_type "mono" or
"packer"
> > in to this entry and then the user must echo "init" in to the
image_type
> > entry.
> > Echoing "init" currently recreates the dell_rbu entries in
> > /sys/class/firmware. Thus in the new patch there is no need to
create
> > the dell_rbu entries at load time. The user must always echo "init"
> > before going ahead with a monolithic image update. And in case of
packet
> > update method the user need to echo "init" in to
> > /sys/devices/platform/dell_rbu/image_type before starting to
download
> > every packet.
> >
> > This method will avoid the need for the dell_rbu driver to recreate
the
> > firmware entries on its own; which will also prevent the load
average
> > from climbing as the kernel thread now is short lived and gets
created
> > only on demand.
> >
> > I will finish and test this patch and send and update.
> >
> 
> ok.  Please carefully describe the new design in the changelog for
that
> patch.  The current design kind of snuck through without anyone
noticing
> that it had these problems.
Andrew,
I have sent a patch which fixes this bug to the LKML and also copied to
you. This patch was tested by me and works fine. 
Thanks
Abhay