Matt,

> There isn't really a formalised interface for dealing with EFI
> variables or struct efivar_entry. This has led to the efivarfs code
> directly accessing '__efivars'.

I agree with you.

I have some comment on a pstore part.

> @@ -1363,86 +1195,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);
> +     entry = kzalloc(sizeof(*entry), GFP_KERNEL);
> +     if (!entry)
> +             return -ENOMEM;

Please don't allocate memory dynamically in the efi_pstore_write() because 
efi_pstore_write() is 
called in an interrupt context like oops and panic.

Also, a whole process of query_info() + entry_set() should be protected by a 
single spin lock()
Because if efi_pstore_write() runs concurrently, a result of query_info() may 
be meaningless...

So, how about introducing query_info_nolock and entry_set_nolock()?

spin_lock()
query_info_nolock()
entry_set_nolock()
spin_unlock()

IMO, efivarfs_file_write() should be fixed as above.

As for efi_pstore_erase() and efi_pstore_read(), it will take some time to 
fully understand your change.

Seiji


--
To unsubscribe from this list: send the line "unsubscribe linux-efi" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to