[Skiboot] [PATCH 1/2] Change load_resource() API to be all about preloading.

[Stewart Smith]

Joel Stanley <joel at jms.id.au> writes:
> On Tue, 2015-03-24 at 14:31 +1100, Stewart Smith wrote:
>> No functional changes in what happens, just have two calls, one for
>> queueing preload the other for waiting until it has loaded.
>> 
>> future patches will introduce platform specific queueing.
>
> I started to implement these calls for async loading on the BMC
> platforms and had a few questions come up.

Excellent!
>> --- a/core/flash.c
>> +++ b/core/flash.c
>> @@ -522,8 +522,8 @@ end:
>>   * load a resource from FLASH
>>   * buf and len shouldn't account for ECC even if partition is ECCed.
>>   */
>> -bool flash_load_resource(enum resource_id id, uint32_t subid,
>> -		void *buf, size_t *len)
>> +int flash_start_preload_resource(enum resource_id id, uint32_t subid,
>> +				 void *buf, size_t *len)
>
> Not convinced it's worth changing from bool to the opal error codes as
> the callers only check for success. Up to you though.

I wasn't sure either... I deferred to bitter debugging experience of
prefering "error X" rather than "error" :)

>> --- a/core/platform.c
>> +++ b/core/platform.c
>
>> +
>> +int wait_for_resource_loaded(enum resource_id id, uint32_t idx)
>> +{
>> +	int r = resource_loaded(id, idx);
>> +
>> +	while(r == OPAL_BUSY) {
>> +		opal_run_pollers();
>> +		time_wait_nopoll(msecs_to_tb(5));
>
> time_wait_ms_nopoll(5);
>
>> +		cpu_relax();
>
> time_wait calls cpu_relax while it loops.

Good catch. Fixed.

>> diff --git a/include/platform.h b/include/platform.h
>> index bdf8bdf..2900b4e 100644
>> --- a/include/platform.h
>> +++ b/include/platform.h
>> @@ -129,11 +129,24 @@ struct platform {
>>  	int		(*elog_commit)(struct errorlog *buf);
>>  
>>  	/*
>> -	 * Load an external resource (eg, kernel payload) into a preallocated
>> -	 * buffer. Returns true on success.
>> +	 * Initiate loading an external resource (e.g. kernel payload, OCC)
>> +	 * into a preallocated buffer.
>> +	 * This is designed to asynchronously load external resources.
>
> Not sure that the last line is true. It could say "The implementation
> should asynchronously load external resources.", but I'm not sure if
> that adds much.
>
>> +	 * Returns OPAL_SUCCESS or error.
>>  	 */
>> -	bool		(*load_resource)(enum resource_id id, uint32_t idx,
>> -					 void *buf, size_t *len);
>> +	int		(*start_preload_resource)(enum resource_id id,
>> +						  uint32_t idx,
>> +						  void *buf, size_t *len);
>
> With this scheme we need to know the final size in the synch path. On
> the BMC platforms this would mean searching the TOC for the partition
> we're after. We could hide more of the work if the size was returned
> asynchronously.

The API is that the length is updated as it's fetced, which is kind of
weird considering you supply the pointer to start_preload_resource() and
it's not valid until the resource is loaded.

The FSP code does this in fsp_fetch_lid_complete(), where for each chunk
it updates size with what's been fetched.

So it's okay to update as it goes. size only valid once resource is loaded.

>> --- a/platforms/astbmc/habanero.c
>> +++ b/platforms/astbmc/habanero.c
>> @@ -53,6 +53,6 @@ DECLARE_PLATFORM(habanero) = {
>>  	.cec_power_down         = astbmc_ipmi_power_down,
>>  	.cec_reboot             = astbmc_ipmi_reboot,
>>  	.elog_commit		= ipmi_elog_commit,
>> -	.load_resource		= flash_load_resource,
>> +	.start_preload_resource	= flash_start_preload_resource,
>
> I think we should leave the name of the flash functions as they were as
> they do not (yet) do any preloading.

ack. fixed.