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
--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
