Re: [PATCH 3/6] efivars: efivar_entry API

[Matt Fleming <matt-HNK1S37rvNbeXh+fF434Mdi2O/JbrIOy@public.gmane.org>]

On 10/04/13 16:25, Seiji Aguchi wrote:
>> +	if (efi_guidcmp(entry->var.VendorGuid, vendor))
>> +		return 0;
>> +
>> +	for (i = 0; i < DUMP_NAME_LEN; i++)
>> +		name[i] = entry->var.VariableName[i];
>> +
>> +	if (sscanf(name, "dump-type%u-%u-%d-%lu",
>> +		   cb_data->type, &part, &cnt, &time) == 4) {
>> +		*cb_data->id = part;
>> +		*cb_data->count = cnt;
>> +		cb_data->timespec->tv_sec = time;
>> +		cb_data->timespec->tv_nsec = 0;
>> +	} else if (sscanf(name, "dump-type%u-%u-%lu",
>> +			  cb_data->type, &part, &time) == 3) {
>> +		/*
>> +		 * Check if an old format,
>> +		 * which doesn't support holding
>> +		 * multiple logs, remains.
>> +		 */
>> +		*cb_data->id = part;
>> +		*cb_data->count = 0;
>> +		cb_data->timespec->tv_sec = time;
>> +		cb_data->timespec->tv_nsec = 0;
>> +	} else
>> +		return 0;
>> +
>> +	efivar_entry_size(entry, &size);
> 
> Deadlocking will happen in this efivar_entry_size() because __efivars->lock is already hold 
> in efivar_entry_iter_begin().

Good catch, thanks.

>> @@ -1419,81 +1229,90 @@ static int efi_pstore_write(enum pstore_type_id type,
>>  	for (i = 0; i < DUMP_NAME_LEN; i++)
>>  		efi_name[i] = name[i];
>>
>> -	efivars->ops->set_variable(efi_name, &vendor, PSTORE_EFI_ATTRIBUTES,
>> -				   size, psi->buf);
>> +	ret = efivar_entry_set_safe(efi_name, vendor, PSTORE_EFI_ATTRIBUTES,
>> +				    !pstore_cannot_block_path(reason),
>> +				    size, psi->buf);
>>
>> -	spin_unlock_irqrestore(&efivars->lock, flags);
>> -
>> -	if (reason == KMSG_DUMP_OOPS && efivar_wq_enabled)
>> +	if (size && !ret && reason == KMSG_DUMP_OOPS && efivar_wq_enabled)
> 
> Why do you add (size && !ret) checking?
> If the purpose of this patch is just adding new API, we don't need to modify the logic.

That looks like a bug that slipped in. I'll fix it. Thanks!

-- 
Matt Fleming, Intel Open Source Technology Center