Message-ID: <F54AEECA5E2B9541821D670476DAE19C2B8AC9DD@PGSMSX102.gar.corp.intel.com>
Date:	Tue, 24 Feb 2015 12:49:09 +0000
From:	"Kweh, Hock Leong" <hock.leong.kweh@...el.com>
To:	Andy Lutomirski <luto@...capital.net>,
	Sam Protsenko <semen.protsenko@...aro.org>,
	Matt Fleming <matt@...sole-pimps.org>,
	Ming Lei <ming.lei@...onical.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC:	"Kweh, Hock Leong" <hock.leong.kweh@...el.com>,
	"Ong, Boon Leong" <boon.leong.ong@...el.com>,
	LKML <linux-kernel@...r.kernel.org>,
	"linux-efi@...r.kernel.org" <linux-efi@...r.kernel.org>
Subject: RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper
 interface

> -----Original Message-----
> From: Kweh, Hock Leong
> Sent: Tuesday, February 24, 2015 6:54 PM
> 
> In callbackfn_efi_capsule, you call request_firmware_nowait.  When that
> callback is invoked, I think that the /sys/class/firmware/efi-capsule-file
> directory doesn't exist at all.
> If the callback takes longer than it takes your script to make it through a full
> iteration, then it will try uploading the second capsule before the firmware
> class directory is there, so it will fail.
> 
> But I just realized that your script has a loop below to handle that.
> It's this:
> 
>                  oldtime=$(date +%S)
>                  oldtime=$(((time + 2) % 60))
>                  until [ -f /sys/class/firmware/efi-capsule-file/loading ]
>                  do
>                          newtime=$(date +%S)
>                          if [ $newtime -eq $oldtime ]
>                          then
>                                  break
>                          fi
>                  done
> 
> Aside from the fact that this loop itself is racy (it may loop forever if
> something goes wrong in the kernel, since $newtime -eq $oldtime may
> never happen), it should help, if you're lucky.  But there's another bug.
> 
> 
> Here's the race:
> 
> User:
> echo 1 > /sys/class/firmware/efi-capsule-file/loading
> cat capsule1 > /sys/class/firmware/efi-capsule-file/data
> echo 0 > /sys/class/firmware/efi-capsule-file/loading
> Kernel: Be a little slow here due to preemption or whatever.
> 
> User:
> -f /sys/class/firmware/efi-capsule-file/loading returns true capsules_loaded
> == 0 Assume failure, incorrectly
> 
> Kernel: catch up and increment capsules_loaded.
> 
> If these patches get applied, then I think that the protocol needs to be
> documented in Documentation/ABI.  It should say something like:
> 
> To upload an EFI capsule, do this:
> 
> Write 1 to /sys/class/firmware/efi-capsule-file/loading
> Write the capsule to /sys/class/firmware/efi-capsule-file/data
> Write 0 to /sys/class/firmware/efi-capsule-file/loading
> 
> Make sure that /sys/class/firmware/efi-capsule-file disappears and comes
> back, perhaps by cd-ing there and waiting for all the files in the directory to
> go away.
> 
> Then, and only then, read capsules_loaded to detect success.
> 
> 
> Once you've written that doc, please seriously consider whether this
> interface is justifiable.  I think it sucks.
> 
> --Andy

Hi All,

After some internal discussion and re-design prototyping & testing on
this efi capsule interface kernel module, I would like to start a discussion
here on the new idea and wish to get input for the implementation and
eventually upstream.

This new idea will expose 2 read-only file notes from the efi capsule kernel
module - "capsule_ticket" and "capsule_report". The "capsule_ticket" will
let user to obtain a NON-ZERO number and then perform a mutex lock. The
obtained number will be used later for "capsule_report" status checking.
Unlock mutex is done after "echo 0 > loading" at the end of callback function.

So the process steps basically look like this:
1.) cat capsule_ticket    =======> acquire a number and lock mutex then
                                                                     expose firmware_class user helper
                                                                     interface as well as start timer for timeout
                                                                     counting
2.) repeat step 1 if obtained a "0" number
3.) echo 1 > loading
4.) cat bin > data
5.) echo 0 > loading        =======> stop the timeout counting  then unlock
                                                                      mutex at the end of callback routine 
6.) cat capsule_report   =======> grep the number acquired from beginning
                                                                      for checking succeeded/failed

The ticket numbering is starting from 1 to 999 and then rolls over from
1 to 999 again. The "capsule_report" will show the whole list of the acquired
entries and will be limited to 128 entries which is capped within one page buffer.

The format of this "capsule_report" output will look like:
"Capsule $num uploads successful/failed/aborted"

There is a 2mins time out (internal) for each of the number acquired.
After that, the interface will be closed/disappeared.

I have attached a SAMPLE script and kernel module code in case you are not
able to understand my description above.

I believe this would really take care of the multi-capsule update concurrently
issue and also asynchronous status reporting issue. Besides, this will indirectly
take care the module unload issue with "rmmod" and not "rmmod -f".
What do you guys think?

----------------------------------------------------------------------------------------------

There is another idea during internal discussion. The 2nd idea would require
some changes to drivers/base/firmware_class.c user helper interface for the
mutex locking as well as status return. Mutex lock will be performed while
doing the 'echo 1 > loading' session and status return will be performed after
the 'echo 0 > loading'. The mutex unlock will be done at 'echo 0 > loading' too
or may be at the status reading session for avoid asynchronous reporting.

This will likely changed the original firmware class user helper interface design
behavior. But this approach could save the 2 file notes from the 1st approach.


Which of the approach do you guys prefer? Thanks.


Regards,
Wilson


View attachment "newcapsule.txt" of type "text/plain" (947 bytes)

Download attachment "0002-efi-Capsule-update-with-user-helper-interface.patch" of type "application/octet-stream" (12646 bytes)

Powered by blists - more mailing lists