From: Dave Jiang <[email protected]>
The efivars module already scans all available variables, normalizes the
variable names, and stores them in a list. Rather than duplicate this
to efi runtime services interface let drivers query variable data by
GUID.
This is needed by the isci driver which relies on an efi variable to
store critical platform parameters like gloablly unique sas addresses
and phy configuration parameters. This is similar to the
pci_map_biosrom() enabling that allows the isci driver to retrieve the
same data in the non-efi case.
For the built-in case efivars is moved to subsys_initcall.
Signed-off-by: Dave Jiang <[email protected]>
Signed-off-by: Dan Williams <[email protected]>
---
Not sure who looks after efivars.c, but get_maintainer.pl and git shortlog
fingered Greg as a likely target.
We are currently targeting a late merge of the isci driver through the
staging tree into 2.6.39. This is a pre-requisite to be able to use the
driver on an efi enabled platform.
--
Dan
drivers/firmware/efivars.c | 59 ++++++++++++++++++++++++++++++--------------
include/linux/efi.h | 22 ++++++++++++++++
2 files changed, 62 insertions(+), 19 deletions(-)
diff --git a/drivers/firmware/efivars.c b/drivers/firmware/efivars.c
index 2a62ec6..e139dac 100644
--- a/drivers/firmware/efivars.c
+++ b/drivers/firmware/efivars.c
@@ -100,23 +100,6 @@ MODULE_VERSION(EFIVARS_VERSION);
static DEFINE_SPINLOCK(efivars_lock);
static LIST_HEAD(efivar_list);
-/*
- * The maximum size of VariableName + Data = 1024
- * Therefore, it's reasonable to save that much
- * space in each part of the structure,
- * and we use a page for reading/writing.
- */
-
-struct efi_variable {
- efi_char16_t VariableName[1024/sizeof(efi_char16_t)];
- efi_guid_t VendorGuid;
- unsigned long DataSize;
- __u8 Data[1024];
- efi_status_t Status;
- __u32 Attributes;
-} __attribute__((packed));
-
-
struct efivar_entry {
struct efi_variable var;
struct list_head list;
@@ -169,6 +152,45 @@ utf8_strsize(efi_char16_t *data, unsigned long maxlength)
return utf8_strlen(data, maxlength/sizeof(efi_char16_t)) * sizeof(efi_char16_t);
}
+efi_status_t
+efivar_get_data_from_guid(struct efi_variable *evar)
+{
+ struct efivar_entry *search_efivar;
+ unsigned long strsize;
+ efi_status_t status;
+ int found = 0;
+
+ spin_lock(&efivars_lock);
+
+ /* make sure this EFI variable is there */
+ list_for_each_entry(search_efivar, &efivar_list, list) {
+ if (!efi_guidcmp(search_efivar->var.VendorGuid,
+ evar->VendorGuid)) {
+ found = 1;
+ break;
+ }
+ }
+
+ if (!found) {
+ spin_unlock(&efivars_lock);
+ return EFI_NOT_FOUND;
+ }
+
+ strsize = utf8_strsize(search_efivar->var.VariableName, 1024);
+ memcpy(evar->VariableName, search_efivar->var.VariableName, strsize);
+
+ evar->DataSize = 1024;
+ status = efi.get_variable(evar->VariableName,
+ &evar->VendorGuid,
+ &evar->Attributes,
+ &evar->DataSize,
+ evar->Data);
+ spin_unlock(&efivars_lock);
+
+ return status;
+}
+EXPORT_SYMBOL(efivar_get_data_from_guid);
+
static efi_status_t
get_var_data(struct efi_variable *var)
{
@@ -757,6 +779,5 @@ efivars_exit(void)
kobject_put(efi_kobj);
}
-module_init(efivars_init);
+subsys_initcall(efivars_init);
module_exit(efivars_exit);
-
diff --git a/include/linux/efi.h b/include/linux/efi.h
index fb737bc..4230bc5 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -243,6 +243,19 @@ struct efi_memory_map {
unsigned long desc_size;
};
+/* The maximum size of VariableName + Data = 1024 Therefore, it's
+ * reasonable to save that much space in each part of the structure, and
+ * we use a page for reading/writing.
+ */
+struct efi_variable {
+ efi_char16_t VariableName[1024/sizeof(efi_char16_t)];
+ efi_guid_t VendorGuid;
+ unsigned long DataSize;
+ __u8 Data[1024];
+ efi_status_t Status;
+ __u32 Attributes;
+} __attribute__((packed));
+
#define EFI_INVALID_TABLE_ADDR (~0UL)
/*
@@ -326,6 +339,15 @@ static inline int efi_range_is_wc(unsigned long start, unsigned long len)
extern int __init efi_setup_pcdp_console(char *);
#endif
+#if defined(CONFIG_EFI_VARS) || defined(CONFIG_EFI_VARS_MODULE)
+extern efi_status_t efivar_get_data_from_guid(struct efi_variable *evar);
+#else
+static inline efi_status_t efivar_get_data_from_guid(struct efi_variable *evar)
+{
+ return EFI_NOT_FOUND;
+}
+#endif
+
/*
* We play games with efi_enabled so that the compiler will, if possible, remove
* EFI-related code altogether.
On Fri, Mar 18, 2011 at 03:16:22PM -0700, Dan Williams wrote:
> From: Dave Jiang <[email protected]>
>
> The efivars module already scans all available variables, normalizes the
> variable names, and stores them in a list. Rather than duplicate this
> to efi runtime services interface let drivers query variable data by
> GUID.
>
> This is needed by the isci driver which relies on an efi variable to
> store critical platform parameters like gloablly unique sas addresses
> and phy configuration parameters. This is similar to the
> pci_map_biosrom() enabling that allows the isci driver to retrieve the
> same data in the non-efi case.
>
> For the built-in case efivars is moved to subsys_initcall.
>
> Signed-off-by: Dave Jiang <[email protected]>
> Signed-off-by: Dan Williams <[email protected]>
> ---
> Not sure who looks after efivars.c, but get_maintainer.pl and git shortlog
> fingered Greg as a likely target.
Yes, that is me.
> We are currently targeting a late merge of the isci driver through the
> staging tree into 2.6.39. This is a pre-requisite to be able to use the
> driver on an efi enabled platform.
You are? That's news to me, why didn't you ask the staging tree
maintainer about this?
I needed all patches in linux-next _before_ the merge window opened to
be able to accept it. So, sorry, it will have to wait until .40.
Please send me the isci driver and I will be glad to queue it up for
staging for that kernel.
thanks,
greg k-h
On Fri, Mar 18, 2011 at 3:50 PM, Greg KH <[email protected]> wrote:
>> Not sure who looks after efivars.c, but get_maintainer.pl and git shortlog
>> fingered Greg as a likely target.
>
> Yes, that is me.
Sorry Greg, didn't mean to refer to you in the third person.
>
>> We are currently targeting a late merge of the isci driver through the
>> staging tree into 2.6.39. ?This is a pre-requisite to be able to use the
>> driver on an efi enabled platform.
>
> You are? ?That's news to me, why didn't you ask the staging tree
> maintainer about this?
This was a very recent, two days ago discussion on linux-scsi [1] with
Jeff and Christoph that you should have been copied on...
> I needed all patches in linux-next _before_ the merge window opened to
> be able to accept it.
Yes, I know, and as dmaengine maintainer I also hate being ambushed by
last minute patches, but now I am unfortunately one of those annoying
people on the other side of the coin.
The review has been intermittent until very recently. Given that
fact, James was understandably not ready to take it into
drivers/scsi/. So the thought was to leave time for the review to pan
out and then attempt a post -rc1 merge through scsi or staging.
> So, sorry, it will have to wait until .40.
> Please send me the isci driver and I will be glad to queue it up for
> staging for that kernel.
We will submit a revised driver in the next few weeks.
--
Dan
[1]: http://marc.info/?l=linux-scsi&m=130022318906252&w=2
On Fri, Mar 18, 2011 at 04:10:10PM -0700, Dan Williams wrote:
> > I needed all patches in linux-next _before_ the merge window opened to
> > be able to accept it.
>
> Yes, I know, and as dmaengine maintainer I also hate being ambushed by
> last minute patches, but now I am unfortunately one of those annoying
> people on the other side of the coin.
Then you should know better than to try to go around the well-known
rules :)
> The review has been intermittent until very recently. Given that
> fact, James was understandably not ready to take it into
> drivers/scsi/. So the thought was to leave time for the review to pan
> out and then attempt a post -rc1 merge through scsi or staging.
There are no "post -rc1" merges for staging, it follows the same rules
as the rest of the kernel. Which means that you will have to wait for
.40 before it can be merged, sorry.
> > So, sorry, it will have to wait until .40.
> > Please send me the isci driver and I will be glad to queue it up for
> > staging for that kernel.
>
> We will submit a revised driver in the next few weeks.
Fine, I'll queue it up for .40 then.
greg k-h
On 3/18/2011 5:22 PM, Greg KH wrote:
> On Fri, Mar 18, 2011 at 04:10:10PM -0700, Dan Williams wrote:
>>> I needed all patches in linux-next _before_ the merge window opened to
>>> be able to accept it.
>>
>> Yes, I know, and as dmaengine maintainer I also hate being ambushed by
>> last minute patches, but now I am unfortunately one of those annoying
>> people on the other side of the coin.
>
> Then you should know better than to try to go around the well-known
> rules :)
Yes...
/me about to push his luck
...I also know the rules can sometimes be bent:
$ git describe --contains 9d200153
v2.6.35-rc2~14^2~15
commit 9d20015391dfc47f6371492925cc0333ac403414
Author: Stepan Moskovchenko <[email protected]>
Date: Wed May 19 11:03:30 2010 -0700
Staging: add MSM framebuffer driver
I see you got flamed for that:
"I pulled, but quite frankly, I don't want to see this kind of pull
request again. There's just no _point_.
I'll take new drivers outside the merge window, but there has to be some
_reason_ for them. See the whole SCSI discussion a few merge windows
ago. The new driver needs to improve the life of somebody to the point
where I want to feel that there is a _reason_ for pulling it outside the
merge window.
These drivers? Not so much. Not even f*cking close.
In other words: tell me why the new drivers couldn't just have waited
for the next merge window? Really?"
As Jeff pointed out:
"It seemed like this was turning into another driver that would get held
outside the kernel until it's "perfect." If that is the case, Linus has
also made it clear we should get drivers for high volume, shipping
hardware into the kernel, even if its staging, if the alternative is to
deny users the driver."
So yes, we are targeting that exception. I'm up for taking the heat
directly if you want... because the pull request will need to backed up
with justification.
--
Dan
On Fri, Mar 18, 2011 at 06:15:47PM -0700, Dan Williams wrote:
> On 3/18/2011 5:22 PM, Greg KH wrote:
> >On Fri, Mar 18, 2011 at 04:10:10PM -0700, Dan Williams wrote:
> >>>I needed all patches in linux-next _before_ the merge window opened to
> >>>be able to accept it.
> >>
> >>Yes, I know, and as dmaengine maintainer I also hate being ambushed by
> >>last minute patches, but now I am unfortunately one of those annoying
> >>people on the other side of the coin.
> >
> >Then you should know better than to try to go around the well-known
> >rules :)
>
> Yes...
>
> /me about to push his luck
<snip>
> As Jeff pointed out:
> "It seemed like this was turning into another driver that would get
> held outside the kernel until it's "perfect." If that is the case,
> Linus has also made it clear we should get drivers for high volume,
> shipping hardware into the kernel, even if its staging, if the
> alternative is to deny users the driver."
That's fine, _BUT_ you are trying to go around the rules for the merge
window, which isn't acceptable. Also note that your driver isn't
self-contained, it needs this change at the least, right? Any others?
> So yes, we are targeting that exception. I'm up for taking the heat
> directly if you want... because the pull request will need to
> backed up with justification.
No, sorry, I'll not take this for .40, all of my trees are merged with
Linus now for .40 and I'll only be sending him bugfixes until the .41
merge window opens up.
Remember, it's only a 3 month wait, you knew about this _WAY_ in
advance, so it's not like this is something new, or out of the ordinary
at all. Because of that, I fail to see why this is somehow not
expected.
On a personal note, I'm going to be very scarse for the next 3 weeks due
to conferences and travel for spring break, so I physically don't have
the time to do any more merges like this with Linus.
thanks,
greg k-h
On Sat, 2011-03-19 at 17:14 -0700, Greg KH wrote:
> On Fri, Mar 18, 2011 at 06:15:47PM -0700, Dan Williams wrote:
> > On 3/18/2011 5:22 PM, Greg KH wrote:
> > >On Fri, Mar 18, 2011 at 04:10:10PM -0700, Dan Williams wrote:
> > >>>I needed all patches in linux-next _before_ the merge window opened to
> > >>>be able to accept it.
> > >>
> > >>Yes, I know, and as dmaengine maintainer I also hate being ambushed by
> > >>last minute patches, but now I am unfortunately one of those annoying
> > >>people on the other side of the coin.
> > >
> > >Then you should know better than to try to go around the well-known
> > >rules :)
> >
> > Yes...
> >
> > /me about to push his luck
>
> <snip>
>
> > As Jeff pointed out:
> > "It seemed like this was turning into another driver that would get
> > held outside the kernel until it's "perfect." If that is the case,
> > Linus has also made it clear we should get drivers for high volume,
> > shipping hardware into the kernel, even if its staging, if the
> > alternative is to deny users the driver."
>
> That's fine, _BUT_ you are trying to go around the rules for the merge
> window, which isn't acceptable. Also note that your driver isn't
> self-contained, it needs this change at the least, right? Any others?
>
> > So yes, we are targeting that exception. I'm up for taking the heat
> > directly if you want... because the pull request will need to
> > backed up with justification.
>
> No, sorry, I'll not take this for .40, all of my trees are merged with
> Linus now for .40 and I'll only be sending him bugfixes until the .41
> merge window opens up.
>
> Remember, it's only a 3 month wait, you knew about this _WAY_ in
> advance, so it's not like this is something new, or out of the ordinary
> at all. Because of that, I fail to see why this is somehow not
> expected.
>
> On a personal note, I'm going to be very scarse for the next 3 weeks due
> to conferences and travel for spring break, so I physically don't have
> the time to do any more merges like this with Linus.
So, here's the deal: You get this driver to a mergeable state, which
means all of the problems Christoph, others and I have outlined
completely fixed or well on the way to being in two months and I will
take it under the merge window exception for new drivers. However, I
mean seriously cleaned up and shiny (and dumping tens of thousands of
lines of flue code and other oddities), not cleaned up for staging.
Further you're going to have to use standard methods to store and
retrieve data, not esoteric efi ones that rely on changes outside SCSI
(you aren't the first people to need to set a SAS address and other
things, so we have a request firmware like method for it).
If you can't make it in two months .. can you do it? I'll extend the
offer to the .40 which makes the cutoff around 5 months, if you need
extra time.
What I suggest is that you take about a month and a half to make all the
changes and then do a patch code drop on linux-scsi. If you're less
sure about what you need to do, do one after three weeks and we'll redo
the assessment.
James
On 3/19/2011 5:14 PM, Greg KH wrote:
> On Fri, Mar 18, 2011 at 06:15:47PM -0700, Dan Williams wrote:
>> On 3/18/2011 5:22 PM, Greg KH wrote:
>>> On Fri, Mar 18, 2011 at 04:10:10PM -0700, Dan Williams wrote:
>>>>> I needed all patches in linux-next _before_ the merge window opened to
>>>>> be able to accept it.
>>>>
>>>> Yes, I know, and as dmaengine maintainer I also hate being ambushed by
>>>> last minute patches, but now I am unfortunately one of those annoying
>>>> people on the other side of the coin.
>>>
>>> Then you should know better than to try to go around the well-known
>>> rules :)
>>
>> Yes...
>>
>> /me about to push his luck
>
> <snip>
>
>> As Jeff pointed out:
>> "It seemed like this was turning into another driver that would get
>> held outside the kernel until it's "perfect." If that is the case,
>> Linus has also made it clear we should get drivers for high volume,
>> shipping hardware into the kernel, even if its staging, if the
>> alternative is to deny users the driver."
>
> That's fine, _BUT_ you are trying to go around the rules for the merge
> window, which isn't acceptable. Also note that your driver isn't
> self-contained, it needs this change at the least, right? Any others?
This efi export was a late discovery, we tried to use the existing
exports (raw efi runtime services) but it was not clean (needed to
duplicate utf-8 encoding in the driver). The other external changes/bug
fixes needed for this driver were submitted weeks ago.
>> So yes, we are targeting that exception. I'm up for taking the heat
>> directly if you want... because the pull request will need to
>> backed up with justification.
>
> No, sorry, I'll not take this for .40, all of my trees are merged with
> Linus now for .40 and I'll only be sending him bugfixes until the .41
> merge window opens up.
>
> Remember, it's only a 3 month wait, you knew about this _WAY_ in
> advance, so it's not like this is something new, or out of the ordinary
> at all. Because of that, I fail to see why this is somehow not
> expected.
Yes, we knew about this way in advance, we brought you in too late for a
.39-staging discussion. Appreciate the consideration and understand the
outcome.
--
Dan
On 3/19/2011 6:13 PM, James Bottomley wrote:
> On Sat, 2011-03-19 at 17:14 -0700, Greg KH wrote:
>> On Fri, Mar 18, 2011 at 06:15:47PM -0700, Dan Williams wrote:
>>> On 3/18/2011 5:22 PM, Greg KH wrote:
>>>> On Fri, Mar 18, 2011 at 04:10:10PM -0700, Dan Williams wrote:
>>>>>> I needed all patches in linux-next _before_ the merge window opened to
>>>>>> be able to accept it.
>>>>>
>>>>> Yes, I know, and as dmaengine maintainer I also hate being ambushed by
>>>>> last minute patches, but now I am unfortunately one of those annoying
>>>>> people on the other side of the coin.
>>>>
>>>> Then you should know better than to try to go around the well-known
>>>> rules :)
>>>
>>> Yes...
>>>
>>> /me about to push his luck
>>
>> <snip>
>>
>>> As Jeff pointed out:
>>> "It seemed like this was turning into another driver that would get
>>> held outside the kernel until it's "perfect." If that is the case,
>>> Linus has also made it clear we should get drivers for high volume,
>>> shipping hardware into the kernel, even if its staging, if the
>>> alternative is to deny users the driver."
>>
>> That's fine, _BUT_ you are trying to go around the rules for the merge
>> window, which isn't acceptable. Also note that your driver isn't
>> self-contained, it needs this change at the least, right? Any others?
>>
>>> So yes, we are targeting that exception. I'm up for taking the heat
>>> directly if you want... because the pull request will need to
>>> backed up with justification.
>>
>> No, sorry, I'll not take this for .40, all of my trees are merged with
>> Linus now for .40 and I'll only be sending him bugfixes until the .41
>> merge window opens up.
>>
>> Remember, it's only a 3 month wait, you knew about this _WAY_ in
>> advance, so it's not like this is something new, or out of the ordinary
>> at all. Because of that, I fail to see why this is somehow not
>> expected.
>>
>> On a personal note, I'm going to be very scarse for the next 3 weeks due
>> to conferences and travel for spring break, so I physically don't have
>> the time to do any more merges like this with Linus.
>
> So, here's the deal: You get this driver to a mergeable state, which
> means all of the problems Christoph, others and I have outlined
> completely fixed or well on the way to being in two months and I will
> take it under the merge window exception for new drivers. However, I
> mean seriously cleaned up and shiny (and dumping tens of thousands of
> lines of flue code and other oddities), not cleaned up for staging.
Sounds fair.
> Further you're going to have to use standard methods to store and
> retrieve data, not esoteric efi ones that rely on changes outside SCSI
> (you aren't the first people to need to set a SAS address and other
> things, so we have a request firmware like method for it).
We do have a request_firmware() interface and a binary blob generation
utility in drivers/scsi/isci/firmware/, but that is only for the
fallback (unable to retrieve from platform-firmware) case and should
probably be considered a bios bug. The expectation is that globally
unique sas address, phy signal-amplification, and default port
configuration settings are a per-board property set by the OEM in the
factory.
Ideally this would have been something that just showed up in an
option-rom bar on the device, but that did not happen so it is left to
software. For legacy-bios we scan adapter rom space looking for our
table, for efi it's comparatively cleaner we just grab this efi variable
identified by its own GUID.
We could have userspace rebuild the firmware image based on what it
finds in the platform firmware... but that becomes a messy management
step and gets confusing if one ever wants to override the platform
defaults with a custom blob.
Other options?
> If you can't make it in two months .. can you do it? I'll extend the
> offer to the .40 which makes the cutoff around 5 months, if you need
> extra time.
Also fair. I suspect we can get through the backlog in that time.
> What I suggest is that you take about a month and a half to make all the
> changes and then do a patch code drop on linux-scsi. If you're less
> sure about what you need to do, do one after three weeks and we'll redo
> the assessment.
Ok. I'll continue to post "[GIT] isci" updates to announce the
intermediate state, but will drop full patch sets to the mailing list
when we have upstream merge candidates ready for review.
--
Dan
On Mon, Mar 21, 2011 at 12:07:18PM -0700, Dan Williams wrote:
> Ideally this would have been something that just showed up in an
> option-rom bar on the device, but that did not happen so it is left
> to software. For legacy-bios we scan adapter rom space looking for
> our table, for efi it's comparatively cleaner we just grab this efi
> variable identified by its own GUID.
In the worse case a late merged icsi driver just wouldn't support
EFI-based boards out ot the box until 2.6.40. I don't think that
is too much of an issue.
On Fri, Mar 18, 2011 at 03:16:22PM -0700, Dan Williams wrote:
> From: Dave Jiang <[email protected]>
>
> The efivars module already scans all available variables, normalizes the
> variable names, and stores them in a list. Rather than duplicate this
> to efi runtime services interface let drivers query variable data by
> GUID.
Mike Waychison made a lot of good edits to the driver which are staged
for .39 in gregkh's tree. Some of those changes will conflict with
your patch here, specifically the definition of struct efi_variable
moved similar to what you have done here.
You'll want to rebase your changes accordingly.
Thanks,
Matt
--
Matt Domsch
Technology Strategist
Dell | Office of the CTO