RE: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images
> -Original Message- > From: Bryan O'Donoghue [mailto:pure.lo...@nexus-software.ie] > Sent: Friday, February 17, 2017 8:54 AM > To: Kweh, Hock Leong ; Jan Kiszka > ; Andy Shevchenko > Cc: Matt Fleming ; Ard Biesheuvel > ; linux-...@vger.kernel.org; Linux Kernel Mailing > List ; Borislav Petkov > Subject: Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark > images > > > > On 16/02/17 03:00, Kweh, Hock Leong wrote: > >> -Original Message- > >> From: Jan Kiszka [mailto:jan.kis...@siemens.com] > >> Sent: Thursday, February 16, 2017 3:00 AM > >> To: Andy Shevchenko > >> Cc: Matt Fleming ; Ard Biesheuvel > >> ; linux-...@vger.kernel.org; Linux Kernel > >> Mailing List ; Borislav Petkov > >> ; Kweh, Hock Leong ; Bryan > >> O'Donoghue > >> Subject: Re: [PATCH 0/2] efi: Enhance capsule loader to support > >> signed Quark images > >> > >> On 2017-02-15 19:50, Jan Kiszka wrote: > >>> On 2017-02-15 19:46, Andy Shevchenko wrote: > >>>> On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka > >>>> > >> wrote: > >>>>> See patch 2 for the background. > >>>>> > >>>>> Series has been tested on the Galileo Gen2, to exclude > >>>>> regressions, with a firmware.cap without security header and the > >>>>> SIMATIC IOT2040 which requires the header because of its mandatory > secure boot. > >>>> > >>>> Briefly looking to the code it looks like a real hack. > >>>> Sorry, but it would be carefully (re-)designed. > >>> > >>> The interface that the firmware provides us? That should have been > >>> done differently, I agree, but I'm not too much into those firmware > >>> details, specifically when it comes to signatures. > >>> > >>> The Linux code was designed around that suboptimal situation. If > >>> there are better ideas, I'm all ears. > >>> > >> > >> Expanding CC's as requested by Andy. > >> > >> Jan > >> > > > > Hi Jan, > > > > While I upstreaming the capsule loader patches, I did work with > > maintainer Matt and look into this security header created for Quark. > > Eventually both of us agreed that this will not be upstream to > > mainline as it is really a Quark specific implementation. > > What's the logic of that ? > > It should be possible to provide a hook (or a custom function). > > > > > The proper implementation may require to work with UEFI community to > > expand its capsule spec to support signed binary. > > Are you volunteering to do that with - getting the CSH into the UEFI spec ? > > If not then we should have a method to load/ignore a capsule including the > CSH, > if so then we should have a realistic timeline laid out for getting that spec > work > done. > > Hint: I don't believe integrating the CSH into the UEFI standard will > happen... > Hi Jan & Bryan, Please don't get me wrong. I am not rejecting the patch submission. I am just sharing a summary for the discussion that I had had it a year back with maintainer Matt for this topic. From the discussion, we did mention what would be the proper handling to this case. And to have UEFI expand it capsule support and take in signed binary would be a more secured way. So, influencing UEFI community to have such support would be the right move throughout the discussion. That is my summary. Of course, influencing the UEFI community would be the longer path. I think it is worth for try to bring the topic up here again to see if Matt would reconsider it. Regards, Wilson
RE: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark images
> -Original Message- > From: Jan Kiszka [mailto:jan.kis...@siemens.com] > Sent: Thursday, February 16, 2017 3:00 AM > To: Andy Shevchenko > Cc: Matt Fleming ; Ard Biesheuvel > ; linux-...@vger.kernel.org; Linux Kernel Mailing > List ; Borislav Petkov ; Kweh, > Hock Leong ; Bryan O'Donoghue > > Subject: Re: [PATCH 0/2] efi: Enhance capsule loader to support signed Quark > images > > On 2017-02-15 19:50, Jan Kiszka wrote: > > On 2017-02-15 19:46, Andy Shevchenko wrote: > >> On Wed, Feb 15, 2017 at 8:14 PM, Jan Kiszka > wrote: > >>> See patch 2 for the background. > >>> > >>> Series has been tested on the Galileo Gen2, to exclude regressions, > >>> with a firmware.cap without security header and the SIMATIC IOT2040 > >>> which requires the header because of its mandatory secure boot. > >> > >> Briefly looking to the code it looks like a real hack. > >> Sorry, but it would be carefully (re-)designed. > > > > The interface that the firmware provides us? That should have been > > done differently, I agree, but I'm not too much into those firmware > > details, specifically when it comes to signatures. > > > > The Linux code was designed around that suboptimal situation. If there > > are better ideas, I'm all ears. > > > > Expanding CC's as requested by Andy. > > Jan > Hi Jan, While I upstreaming the capsule loader patches, I did work with maintainer Matt and look into this security header created for Quark. Eventually both of us agreed that this will not be upstream to mainline as it is really a Quark specific implementation. The proper implementation may require to work with UEFI community to expand its capsule spec to support signed binary. Regards, Wilson
[PATCH v5] net: stmmac: fix maxmtu assignment to be within valid range
From: "Kweh, Hock Leong" There is no checking valid value of maxmtu when getting it from device tree. This resolution added the checking condition to ensure the assignment is made within a valid range. Signed-off-by: Kweh, Hock Leong --- changelog v5: * revert back that plat->maxmtu > ndev->max_mtu is a valid case when ndev->max_mtu assignment is entering to the else statement * add comment to enchance clarification changelog v4: * add print warning message when maxmtu > max_mtu as well * add maxmtu = JUMBO_LEN into each *_default_data() at stmmac_pci.c changelog v3: * print the warning message only if maxmtu < min_mtu * add maxmtu = JUMBO_LEN at stmmac_pci.c to follow stmmac_platform.c changelog v2: * correction of "devicetree" to "device tree" reported by Andy * print warning message while maxmtu is not in valid range drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 +- drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c |6 ++ 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 92ac006..8e56dc4 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3345,8 +3345,16 @@ int stmmac_dvr_probe(struct device *device, ndev->max_mtu = JUMBO_LEN; else ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN); - if (priv->plat->maxmtu < ndev->max_mtu) + /* Will not overwrite ndev->max_mtu if plat->maxmtu > ndev->max_mtu +* as well as plat->maxmtu < ndev->min_mtu which is a invalid range. +*/ + if ((priv->plat->maxmtu < ndev->max_mtu) && + (priv->plat->maxmtu >= ndev->min_mtu)) ndev->max_mtu = priv->plat->maxmtu; + else if (priv->plat->maxmtu < ndev->min_mtu) + netdev_warn(priv->dev, + "%s: warning: maxmtu having invalid value (%d)\n", + __func__, priv->plat->maxmtu); if (flow_ctrl) priv->flow_ctrl = FLOW_AUTO;/* RX/TX pause on */ diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c index a283177..3da4737 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c @@ -89,6 +89,9 @@ static void stmmac_default_data(struct plat_stmmacenet_data *plat) /* Set default value for unicast filter entries */ plat->unicast_filter_entries = 1; + + /* Set the maxmtu to a default of JUMBO_LEN */ + plat->maxmtu = JUMBO_LEN; } static int quark_default_data(struct plat_stmmacenet_data *plat, @@ -126,6 +129,9 @@ static int quark_default_data(struct plat_stmmacenet_data *plat, /* Set default value for unicast filter entries */ plat->unicast_filter_entries = 1; + /* Set the maxmtu to a default of JUMBO_LEN */ + plat->maxmtu = JUMBO_LEN; + return 0; } -- 1.7.9.5
RE: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid range
> -Original Message-
> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Saturday, January 07, 2017 8:07 AM
> To: Kweh, Hock Leong
> Cc: David S. Miller ; Joao Pinto
> ; Giuseppe CAVALLARO ;
> seraphin.bonna...@st.com; Jarod Wilson ; Alexandre
> TORGUE ; Joachim Eastwood
> ; Niklas Cassel ; Johan Hovold
> ; Pavel Machek ; lars.pers...@axis.com;
> netdev ; LKML
> Subject: Re: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid
> range
>
> On Sat, Jan 7, 2017 at 1:47 AM, Kweh, Hock Leong
> wrote:
>
> >> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> >> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> >> > @@ -3345,8 +3345,14 @@ int stmmac_dvr_probe(struct device *device,
> >> > ndev->max_mtu = JUMBO_LEN;
> >> > else
> >> > ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD +
> NET_IP_ALIGN);
> >> > - if (priv->plat->maxmtu < ndev->max_mtu)
> >>
> >> > + if ((priv->plat->maxmtu < ndev->max_mtu) &&
> >> > + (priv->plat->maxmtu >= ndev->min_mtu))
> >>
> >> > ndev->max_mtu = priv->plat->maxmtu;
> >>
> >> > + else if (priv->plat->maxmtu < ndev->min_mtu)
> >>
> >> And if it > ndev->max_mtu?..
> >>
> >
> > Base on my understanding to the original code, the "maxmtu >= ndev-
> >max_mtu"
> > is meant for products that would want to use the value from logic which is
> > just
> above
> > this statement where you just ask me not to add new line. That the reason
> > the
> > stmmac_platform.c put in "plat->maxmtu = JUMBO_LEN;" as generic and I
> also
> > follow it in stmmac_pci.c.
> >
> > Or do you mean only take maxmtu = JUMBO_LEN for the option to use driver
> itself
> > assignment statement above and all the > max_mtu consider invalid?
>
> So, just answer to the simple question: is it a valid case to have
> plat->maxmtu > ndev->max_mtu? If it so, how is it used?
> Otherwise we need a warning in such case. What did I miss?
>
it is a valid case for priv->plat->maxmtu > ndev->max_mtu if referring
to the statement above it:
/* MTU range: 46 - hw-specific max */
ndev->min_mtu = ETH_ZLEN - ETH_HLEN;
if ((priv->plat->enh_desc) || (priv->synopsys_id >= DWMAC_CORE_4_00))
ndev->max_mtu = JUMBO_LEN;
else
ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN);
When the ndev->max_mtu go into the else statement, then the assignment in
stmmac_platform.c & stammac_pci.c plat->maxmtu = JUMBO_LEN is actually
greater than ndev->max_mtu. That is what I understanding that maxmtu > max_mtu
is an option trick to allow driver assign value through the logic above instead
of getting
it from of_property_read_u32(np, "max-frame-size", &plat->maxmtu); or
*_default_data().
I need to revert back the V4 and submit V5.
> >
> >> > + netdev_warn(priv->dev,
> >> > + "%s: warning: maxmtu having invalid value
> >> > (%d)\n",
> >> > + __func__, priv->plat->maxmtu);
>
>
> >> > + /* Set the maxmtu to a default of JUMBO_LEN in case the
> >> > +* parameter is not defined for the device.
> >> > +*/
> >> > + plat->maxmtu = JUMBO_LEN;
> >>
> >> Please, use *_default_data() hooks for that.
> >>
> >> At some point it might make sense to extract
> >> static int common_default_data() {...}
> >> and call it at the beginning of the rest of *_defautl_data() hooks.
> >>
> >
> > Just try to understand, are you referring changing the code something
> > like this:
> >
> > stmmac_default_data(plat);
> > if (info) {
> > info->pdev = pdev;
> > if (info->setup) {
> > ret = info->setup(plat, info);
> > if (ret)
> > return ret;
> > }
> > }
> >
> > Where all the common code is inside the stmmac_default_data()?
>
> No.
>
> common_default_data()
> {
> ... common defaults among *_default_data() ...
> }
>
> *_default_data()
> {
> ...
> common_default_data();
> ...
> }
>
Ok noted. Will be a separate patch. Thanks.
Regards,
Wilson
> --
> With Best Regards,
> Andy Shevchenko
RE: [PATCH v4] net: stmmac: fix maxmtu assignment to be within valid range
> -Original Message- > From: Andy Shevchenko [mailto:andy.shevche...@gmail.com] > Sent: Saturday, January 07, 2017 8:12 AM > To: Kweh, Hock Leong > Cc: David S. Miller ; Joao Pinto > ; Giuseppe CAVALLARO ; > seraphin.bonna...@st.com; Jarod Wilson ; Alexandre > TORGUE ; Joachim Eastwood > ; Niklas Cassel ; Johan Hovold > ; Pavel Machek ; lars.pers...@axis.com; > netdev ; LKML > Subject: Re: [PATCH v4] net: stmmac: fix maxmtu assignment to be within valid > range > > On Sat, Jan 7, 2017 at 10:10 AM, Kweh, Hock Leong > wrote: > > From: "Kweh, Hock Leong" > > > > There is no checking valid value of maxmtu when getting it from device tree. > > This resolution added the checking condition to ensure the assignment is > > made within a valid range. > > > changelog v4: > > * add print warning message when maxmtu > max_mtu as well > > Yep. > > > * add maxmtu = JUMBO_LEN into each *_default_data() at stmmac_pci.c > > Yep. > > But see comment below. > > P.S. And perhaps next time send into our internal mailing list first for > review. > > > @@ -3345,8 +3345,14 @@ int stmmac_dvr_probe(struct device *device, > > ndev->max_mtu = JUMBO_LEN; > > else > > ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN); > > - if (priv->plat->maxmtu < ndev->max_mtu) > > + if ((priv->plat->maxmtu < ndev->max_mtu) && > > + (priv->plat->maxmtu >= ndev->min_mtu)) > > ndev->max_mtu = priv->plat->maxmtu; > > > + else if ((priv->plat->maxmtu < ndev->min_mtu) || > > +(priv->plat->maxmtu > ndev->max_mtu)) > > + netdev_warn(priv->dev, > > What is the difference to just 'else'? (Returning back to my initial > proposal, I don't remember telling anything about 'else if' concept) > When priv->plat->maxmtu == ndev->max_mtu will not be a warning message. Oh NO ... it is a valid case for priv->plat->maxmtu > ndev->max_mtu if the assignment of ndev->max_mtu is using SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN), which is < JUMBO_LEN, then priv->plat->maxmtu > ndev->max_mtu is valid. Revert back and submit V5. Thanks. > > + "%s: warning: maxmtu having invalid value > > (%d)\n", > > + __func__, priv->plat->maxmtu); > > -- > With Best Regards, > Andy Shevchenko
[PATCH v4] net: stmmac: fix maxmtu assignment to be within valid range
From: "Kweh, Hock Leong" There is no checking valid value of maxmtu when getting it from device tree. This resolution added the checking condition to ensure the assignment is made within a valid range. Signed-off-by: Kweh, Hock Leong --- changelog v4: * add print warning message when maxmtu > max_mtu as well * add maxmtu = JUMBO_LEN into each *_default_data() at stmmac_pci.c changelog v3: * print the warning message only if maxmtu < min_mtu * add maxmtu = JUMBO_LEN at stmmac_pci.c to follow stmmac_platform.c changelog v2: * correction of "devicetree" to "device tree" reported by Andy * print warning message while maxmtu is not in valid range drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |8 +++- drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c |6 ++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 92ac006..f2a352f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3345,8 +3345,14 @@ int stmmac_dvr_probe(struct device *device, ndev->max_mtu = JUMBO_LEN; else ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN); - if (priv->plat->maxmtu < ndev->max_mtu) + if ((priv->plat->maxmtu < ndev->max_mtu) && + (priv->plat->maxmtu >= ndev->min_mtu)) ndev->max_mtu = priv->plat->maxmtu; + else if ((priv->plat->maxmtu < ndev->min_mtu) || +(priv->plat->maxmtu > ndev->max_mtu)) + netdev_warn(priv->dev, + "%s: warning: maxmtu having invalid value (%d)\n", + __func__, priv->plat->maxmtu); if (flow_ctrl) priv->flow_ctrl = FLOW_AUTO;/* RX/TX pause on */ diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c index a283177..3da4737 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c @@ -89,6 +89,9 @@ static void stmmac_default_data(struct plat_stmmacenet_data *plat) /* Set default value for unicast filter entries */ plat->unicast_filter_entries = 1; + + /* Set the maxmtu to a default of JUMBO_LEN */ + plat->maxmtu = JUMBO_LEN; } static int quark_default_data(struct plat_stmmacenet_data *plat, @@ -126,6 +129,9 @@ static int quark_default_data(struct plat_stmmacenet_data *plat, /* Set default value for unicast filter entries */ plat->unicast_filter_entries = 1; + /* Set the maxmtu to a default of JUMBO_LEN */ + plat->maxmtu = JUMBO_LEN; + return 0; } -- 1.7.9.5
RE: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid range
> -Original Message-
> From: Andy Shevchenko [mailto:andy.shevche...@gmail.com]
> Sent: Saturday, January 07, 2017 1:07 AM
> To: Kweh, Hock Leong
> Cc: David S. Miller ; Joao Pinto
> ; Giuseppe CAVALLARO ;
> seraphin.bonna...@st.com; Jarod Wilson ; Alexandre
> TORGUE ; Joachim Eastwood
> ; Niklas Cassel ; Johan Hovold
> ; Pavel Machek ; lars.pers...@axis.com;
> netdev ; LKML
> Subject: Re: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid
> range
>
> On Sat, Jan 7, 2017 at 2:49 AM, Kweh, Hock Leong
> wrote:
> > From: "Kweh, Hock Leong"
> >
> > There is no checking valid value of maxmtu when getting it from device tree.
> > This resolution added the checking condition to ensure the assignment is
> > made within a valid range.
>
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -3345,8 +3345,14 @@ int stmmac_dvr_probe(struct device *device,
> > ndev->max_mtu = JUMBO_LEN;
> > else
> > ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN);
> > - if (priv->plat->maxmtu < ndev->max_mtu)
>
> > +
>
> The lines are logically grouped here. No need to split it. Thus,
> remove this extra line.
>
Ok noted.
> > + if ((priv->plat->maxmtu < ndev->max_mtu) &&
> > + (priv->plat->maxmtu >= ndev->min_mtu))
>
> > ndev->max_mtu = priv->plat->maxmtu;
>
> > + else if (priv->plat->maxmtu < ndev->min_mtu)
>
> And if it > ndev->max_mtu?..
>
Base on my understanding to the original code, the "maxmtu >= ndev->max_mtu"
is meant for products that would want to use the value from logic which is just
above
this statement where you just ask me not to add new line. That the reason the
stmmac_platform.c put in "plat->maxmtu = JUMBO_LEN;" as generic and I also
follow it in stmmac_pci.c.
Or do you mean only take maxmtu = JUMBO_LEN for the option to use driver itself
assignment statement above and all the > max_mtu consider invalid?
> > + netdev_warn(priv->dev,
> > + "%s: warning: maxmtu having invalid value
> > (%d)\n",
> > + __func__, priv->plat->maxmtu);
>
>
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> > @@ -204,6 +204,10 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
> >
> > pci_set_master(pdev);
> >
> > + /* Set the maxmtu to a default of JUMBO_LEN in case the
> > +* parameter is not defined for the device.
> > +*/
> > + plat->maxmtu = JUMBO_LEN;
>
> Please, use *_default_data() hooks for that.
>
> At some point it might make sense to extract
> static int common_default_data() {...}
> and call it at the beginning of the rest of *_defautl_data() hooks.
>
Just try to understand, are you referring changing the code something
like this:
stmmac_default_data(plat);
if (info) {
info->pdev = pdev;
if (info->setup) {
ret = info->setup(plat, info);
if (ret)
return ret;
}
}
Where all the common code is inside the stmmac_default_data()?
Anyway, this patch is focus for fixing the maxmtu assignment in valid range.
I will submit V4 to put "plat->maxmtu = JUMBO_LEN;" into each *_default_data().
Regarding the common_default_data() would be a new patch for better code
structuring. Thanks.
> --
> With Best Regards,
> Andy Shevchenko
RE: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid range
> -Original Message-
> From: Joao Pinto [mailto:joao.pi...@synopsys.com]
> Sent: Saturday, January 07, 2017 12:58 AM
> To: Kweh, Hock Leong ; David S. Miller
> ; Joao Pinto ; Giuseppe
> CAVALLARO ; seraphin.bonna...@st.com; Jarod
> Wilson ; Andy Shevchenko
> Cc: Alexandre TORGUE ; Joachim Eastwood
> ; Niklas Cassel ; Johan Hovold
> ; pa...@ucw.cz; lars.pers...@axis.com; netdev
> ; LKML
> Subject: Re: [PATCH v3] net: stmmac: fix maxmtu assignment to be within valid
> range
>
>
> Hi Wilson,
>
> Às 12:49 AM de 1/7/2017, Kweh, Hock Leong escreveu:
> > From: "Kweh, Hock Leong"
> >
> > There is no checking valid value of maxmtu when getting it from device tree.
> > This resolution added the checking condition to ensure the assignment is
> > made within a valid range.
> >
> > Signed-off-by: Kweh, Hock Leong
> > ---
> > changelog v3:
> > * print the warning message only if maxmtu < min_mtu
> > * add maxmtu = JUMBO_LEN at stmmac_pci.c to follow stmmac_platform.c
> >
> > changelog v2:
> > * correction of "devicetree" to "device tree" reported by Andy
> > * print warning message while maxmtu is not in valid range
> >
> > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |8 +++-
> > drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c |4
> > 2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index 92ac006..ce74ae6 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -3345,8 +3345,14 @@ int stmmac_dvr_probe(struct device *device,
> > ndev->max_mtu = JUMBO_LEN;
> > else
> > ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD +
> NET_IP_ALIGN);
> > - if (priv->plat->maxmtu < ndev->max_mtu)
> > +
> > + if ((priv->plat->maxmtu < ndev->max_mtu) &&
> > + (priv->plat->maxmtu >= ndev->min_mtu))
> > ndev->max_mtu = priv->plat->maxmtu;
> > + else if (priv->plat->maxmtu < ndev->min_mtu)
> > + netdev_warn(priv->dev,
> > + "%s: warning: maxmtu having invalid value (%d)\n",
> > + __func__, priv->plat->maxmtu);
> >
> > if (flow_ctrl)
> > priv->flow_ctrl = FLOW_AUTO;/* RX/TX pause on */
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> > index a283177..e539afe 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
> > @@ -204,6 +204,10 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
> >
> > pci_set_master(pdev);
> >
> > + /* Set the maxmtu to a default of JUMBO_LEN in case the
> > +* parameter is not defined for the device.
> > +*/
> > + plat->maxmtu = JUMBO_LEN;
>
> I suggest to put this configuration in one of the default config functions.
>
> Tahnks.
>
My original thought is to have one line for all *_default_data() instead of
having
multiple same line inside each *_default_data(). If this is not a big concern of
future expand, I can do that. Thanks.
> > if (info) {
> > info->pdev = pdev;
> > if (info->setup) {
> >
[PATCH v3] net: stmmac: fix maxmtu assignment to be within valid range
From: "Kweh, Hock Leong"
There is no checking valid value of maxmtu when getting it from device tree.
This resolution added the checking condition to ensure the assignment is
made within a valid range.
Signed-off-by: Kweh, Hock Leong
---
changelog v3:
* print the warning message only if maxmtu < min_mtu
* add maxmtu = JUMBO_LEN at stmmac_pci.c to follow stmmac_platform.c
changelog v2:
* correction of "devicetree" to "device tree" reported by Andy
* print warning message while maxmtu is not in valid range
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |8 +++-
drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c |4
2 files changed, 11 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 92ac006..ce74ae6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3345,8 +3345,14 @@ int stmmac_dvr_probe(struct device *device,
ndev->max_mtu = JUMBO_LEN;
else
ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN);
- if (priv->plat->maxmtu < ndev->max_mtu)
+
+ if ((priv->plat->maxmtu < ndev->max_mtu) &&
+ (priv->plat->maxmtu >= ndev->min_mtu))
ndev->max_mtu = priv->plat->maxmtu;
+ else if (priv->plat->maxmtu < ndev->min_mtu)
+ netdev_warn(priv->dev,
+ "%s: warning: maxmtu having invalid value (%d)\n",
+ __func__, priv->plat->maxmtu);
if (flow_ctrl)
priv->flow_ctrl = FLOW_AUTO;/* RX/TX pause on */
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
index a283177..e539afe 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_pci.c
@@ -204,6 +204,10 @@ static int stmmac_pci_probe(struct pci_dev *pdev,
pci_set_master(pdev);
+ /* Set the maxmtu to a default of JUMBO_LEN in case the
+* parameter is not defined for the device.
+*/
+ plat->maxmtu = JUMBO_LEN;
if (info) {
info->pdev = pdev;
if (info->setup) {
--
1.7.9.5
RE: [PATCH v2] net: stmmac: fix maxmtu assignment to be within valid range
> -Original Message- > From: Kweh, Hock Leong > Sent: Friday, January 06, 2017 6:08 PM > To: David S. Miller ; Joao Pinto > ; Giuseppe CAVALLARO ; > seraphin.bonna...@st.com; Jarod Wilson ; Andy > Shevchenko > Cc: Alexandre TORGUE ; Joachim Eastwood > ; Niklas Cassel ; Johan Hovold > ; pa...@ucw.cz; Kweh, Hock Leong > ; lars.pers...@axis.com; netdev > ; LKML > Subject: [PATCH v2] net: stmmac: fix maxmtu assignment to be within valid > range > > From: "Kweh, Hock Leong" > > There is no checking valid value of maxmtu when getting it from device tree. > This resolution added the checking condition to ensure the assignment is made > within a valid range. > > Signed-off-by: Kweh, Hock Leong I am going to submit V3. > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |8 +++- > 1 file changed, 7 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 92ac006..4df555e 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -3345,8 +3345,14 @@ int stmmac_dvr_probe(struct device *device, > ndev->max_mtu = JUMBO_LEN; > else > ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + > NET_IP_ALIGN); > - if (priv->plat->maxmtu < ndev->max_mtu) > + > + if ((priv->plat->maxmtu < ndev->max_mtu) && > + (priv->plat->maxmtu >= ndev->min_mtu)) > ndev->max_mtu = priv->plat->maxmtu; > + else if (priv->plat->maxmtu != 0) > + netdev_warn(priv->dev, > + "%s: warning: maxmtu having invalid value (%d)\n", > + __func__, priv->plat->maxmtu); > > if (flow_ctrl) > priv->flow_ctrl = FLOW_AUTO;/* RX/TX pause on */ > -- > 1.7.9.5
[PATCH v2] net: stmmac: fix maxmtu assignment to be within valid range
From: "Kweh, Hock Leong" There is no checking valid value of maxmtu when getting it from device tree. This resolution added the checking condition to ensure the assignment is made within a valid range. Signed-off-by: Kweh, Hock Leong --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |8 +++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 92ac006..4df555e 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3345,8 +3345,14 @@ int stmmac_dvr_probe(struct device *device, ndev->max_mtu = JUMBO_LEN; else ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN); - if (priv->plat->maxmtu < ndev->max_mtu) + + if ((priv->plat->maxmtu < ndev->max_mtu) && + (priv->plat->maxmtu >= ndev->min_mtu)) ndev->max_mtu = priv->plat->maxmtu; + else if (priv->plat->maxmtu != 0) + netdev_warn(priv->dev, + "%s: warning: maxmtu having invalid value (%d)\n", + __func__, priv->plat->maxmtu); if (flow_ctrl) priv->flow_ctrl = FLOW_AUTO;/* RX/TX pause on */ -- 1.7.9.5
RE: [PATCH] net: stmmac: fix maxmtu assignment to be within valid range
> -Original Message- > From: Andy Shevchenko [mailto:andy.shevche...@gmail.com] > Sent: Friday, January 06, 2017 5:07 AM > To: Kweh, Hock Leong > Cc: David S. Miller ; Joao Pinto > ; Giuseppe CAVALLARO ; > seraphin.bonna...@st.com; Jarod Wilson ; Alexandre > TORGUE ; Joachim Eastwood > ; Niklas Cassel ; Johan Hovold > ; Pavel Machek ; lars.pers...@axis.com; > netdev ; LKML > Subject: Re: [PATCH] net: stmmac: fix maxmtu assignment to be within valid > range > > On Thu, Jan 5, 2017 at 12:47 PM, Kweh, Hock Leong > wrote: > > From: "Kweh, Hock Leong" > > > > There is no checking valid value of maxmtu when getting it from devicetree. > > 'Device Tree' or 'device tree' ? Noted & Thanks. Submitting V2. > > > This resolution added the checking condition to ensure the assignment > > is made within a valid range. > > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > index 39eb7a6..683d59f 100644 > > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > > @@ -3319,7 +3319,8 @@ int stmmac_dvr_probe(struct device *device, > > ndev->max_mtu = JUMBO_LEN; > > else > > ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN); > > - if (priv->plat->maxmtu < ndev->max_mtu) > > + if ((priv->plat->maxmtu < ndev->max_mtu) && > > + (priv->plat->maxmtu >= ndev->min_mtu)) > > ndev->max_mtu = priv->plat->maxmtu; > > Perhaps add a warning message on else branch? Noted & Thanks. Submitting V2. > > -- > With Best Regards, > Andy Shevchenko
[PATCH] net: stmmac: fix maxmtu assignment to be within valid range
From: "Kweh, Hock Leong" There is no checking valid value of maxmtu when getting it from devicetree. This resolution added the checking condition to ensure the assignment is made within a valid range. Signed-off-by: Kweh, Hock Leong --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 39eb7a6..683d59f 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -3319,7 +3319,8 @@ int stmmac_dvr_probe(struct device *device, ndev->max_mtu = JUMBO_LEN; else ndev->max_mtu = SKB_MAX_HEAD(NET_SKB_PAD + NET_IP_ALIGN); - if (priv->plat->maxmtu < ndev->max_mtu) + if ((priv->plat->maxmtu < ndev->max_mtu) && + (priv->plat->maxmtu >= ndev->min_mtu)) ndev->max_mtu = priv->plat->maxmtu; if (flow_ctrl) -- 1.7.9.5
RE: [PATCH net] net: stmmac: Fix error path after register_netdev move
> -Original Message-
> From: Florian Fainelli [mailto:f.faine...@gmail.com]
> Sent: Thursday, December 29, 2016 7:45 AM
> To: net...@vger.kernel.org
> Cc: Florian Fainelli ; pa...@ucw.cz;
> joao.pi...@synopsys.com; seraphin.bonna...@st.com;
> alexandre.tor...@gmail.com; manab...@gmail.com; niklas.cas...@axis.com;
> jo...@kernel.org; Ong, Boon Leong ; Voon,
> Weifeng ; lars.pers...@axis.com; linux-
> ker...@vger.kernel.org; Giuseppe Cavallaro ;
> Alexandre Torgue
> Subject: [PATCH net] net: stmmac: Fix error path after register_netdev move
>
> Commit 5701659004d6 ("net: stmmac: Fix race between stmmac_drv_probe and
> stmmac_open") re-ordered how the MDIO bus registration and the network
> device are registered, but missed to unwind the MDIO bus registration in case
> we fail to register the network device.
>
> Fixes: 5701659004d6 ("net: stmmac: Fix race between stmmac_drv_probe and
> stmmac_open")
> Signed-off-by: Florian Fainelli
> ---
Acked-by: Kweh, Hock Leong
> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 9 -
> 1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index 5910ea51f8f6..39eb7a65bb9f 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3366,12 +3366,19 @@ int stmmac_dvr_probe(struct device *device,
> }
>
> ret = register_netdev(ndev);
> - if (ret)
> + if (ret) {
> netdev_err(priv->dev, "%s: ERROR %i registering the device\n",
> __func__, ret);
> + goto error_netdev_register;
> + }
>
> return ret;
>
> +error_netdev_register:
> + if (priv->hw->pcs != STMMAC_PCS_RGMII &&
> + priv->hw->pcs != STMMAC_PCS_TBI &&
> + priv->hw->pcs != STMMAC_PCS_RTBI)
> + stmmac_mdio_unregister(ndev);
> error_mdio_register:
> netif_napi_del(&priv->napi);
> error_hw_init:
> --
> 2.9.3
RE: [PATCH v2] net: stmmac: bug fix to synchronize stmmac_open and stmmac_dvr_probe
> -Original Message- > From: Florian Fainelli [mailto:f.faine...@gmail.com] > Sent: Thursday, December 29, 2016 2:43 AM > To: Kweh, Hock Leong ; David Miller > > Cc: joao.pi...@synopsys.com; peppe.cavall...@st.com; > seraphin.bonna...@st.com; alexandre.tor...@gmail.com; > manab...@gmail.com; niklas.cas...@axis.com; jo...@kernel.org; > pa...@ucw.cz; Ong, Boon Leong ; Voon, Weifeng > ; lars.pers...@axis.com; net...@vger.kernel.org; > linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2] net: stmmac: bug fix to synchronize stmmac_open and > stmmac_dvr_probe > > On 12/27/2016 09:49 PM, Kweh, Hock Leong wrote: > >> -Original Message- > >> From: David Miller [mailto:da...@davemloft.net] > >> Sent: Wednesday, December 28, 2016 12:34 AM > >> To: Kweh, Hock Leong > >> Cc: joao.pi...@synopsys.com; peppe.cavall...@st.com; > >> seraphin.bonna...@st.com; f.faine...@gmail.com; > >> alexandre.tor...@gmail.com; manab...@gmail.com; > >> niklas.cas...@axis.com; jo...@kernel.org; pa...@ucw.cz; Ong, Boon > >> Leong ; Voon, Weifeng > >> ; lars.pers...@axis.com; > >> net...@vger.kernel.org; linux-kernel@vger.kernel.org > >> Subject: Re: [PATCH v2] net: stmmac: bug fix to synchronize > >> stmmac_open and stmmac_dvr_probe > >> > >> From: "Kweh, Hock Leong" > >> Date: Tue, 27 Dec 2016 22:42:36 +0800 > >> > >>> From: "Kweh, Hock Leong" > >> > >> You are not the author of this change, do not take credit for it. > >> > >> You have copied Florian's patch character by character, therefore he > >> is the author. > >> > >> You also didn't CC: the netdev mailing list properly. > > > > Hi David & Florian, > > > > Just to clarify that I do not copy exactly from Florian. > > I have changed it to have proper handling on mdio unregister while > > netdev_register() failed as showed below: > > > > return 0; > > > > -error_mdio_register: > > - unregister_netdev(ndev); > > error_netdev_register: > > + stmmac_mdio_unregister(ndev); > > Although this is required, we can't be doing it in all circumstances, we need > to > mimic what stmmac_drv_remove() does. > > Let me submit an incremental fix which takes care of mdio bus unregistration. > -- > Florian Noted & Thanks. Will test it out once you submitted. Thanks & Regards, Wilson
RE: [PATCH v2] net: stmmac: bug fix to synchronize stmmac_open and stmmac_dvr_probe
> -Original Message- > From: Kishan Sandeep [mailto:sandeepkishan...@gmail.com] > Sent: Wednesday, December 28, 2016 7:56 PM > To: Kweh, Hock Leong > Cc: David Miller ; f.faine...@gmail.com; > joao.pi...@synopsys.com; peppe.cavall...@st.com; > seraphin.bonna...@st.com; alexandre.tor...@gmail.com; > manab...@gmail.com; niklas.cas...@axis.com; jo...@kernel.org; > pa...@ucw.cz; lars.pers...@axis.com; net...@vger.kernel.org; linux- > ker...@vger.kernel.org > Subject: Re: [PATCH v2] net: stmmac: bug fix to synchronize stmmac_open and > stmmac_dvr_probe > > On Wed, Dec 28, 2016 at 7:10 AM, Kweh, Hock Leong > wrote: > >> -Original Message- > >> From: David Miller [mailto:da...@davemloft.net] > >> Sent: Wednesday, December 28, 2016 12:34 AM > >> To: Kweh, Hock Leong > >> Cc: joao.pi...@synopsys.com; peppe.cavall...@st.com; > >> seraphin.bonna...@st.com; f.faine...@gmail.com; > >> alexandre.tor...@gmail.com; manab...@gmail.com; > >> niklas.cas...@axis.com; jo...@kernel.org; pa...@ucw.cz; Ong, Boon > >> Leong ; Voon, Weifeng > >> ; lars.pers...@axis.com; > >> net...@vger.kernel.org; linux-kernel@vger.kernel.org > >> Subject: Re: [PATCH v2] net: stmmac: bug fix to synchronize > >> stmmac_open and stmmac_dvr_probe > >> > >> From: "Kweh, Hock Leong" > >> Date: Tue, 27 Dec 2016 22:42:36 +0800 > >> > >> > From: "Kweh, Hock Leong" > >> > >> You are not the author of this change, do not take credit for it. > >> > >> You have copied Florian's patch character by character, therefore he > >> is the author. > >> > >> You also didn't CC: the netdev mailing list properly. > > > > Noted & Thanks. > > > > Hi Florian, could you submit this fix from your side so that you are the > > author. > > I will help to test out. > > > > Thanks & Regards, > > Wilson > > > I think you can give *--author* for giving author name. Try git commit -am > "commit message" --author="Author_name " Oh ... I am not aware of that. Thanks for informing. :-) Regards, Wilson
RE: [PATCH v2] net: stmmac: bug fix to synchronize stmmac_open and stmmac_dvr_probe
> -Original Message- > From: David Miller [mailto:da...@davemloft.net] > Sent: Wednesday, December 28, 2016 12:34 AM > To: Kweh, Hock Leong > Cc: joao.pi...@synopsys.com; peppe.cavall...@st.com; > seraphin.bonna...@st.com; f.faine...@gmail.com; > alexandre.tor...@gmail.com; manab...@gmail.com; niklas.cas...@axis.com; > jo...@kernel.org; pa...@ucw.cz; Ong, Boon Leong > ; Voon, Weifeng ; > lars.pers...@axis.com; net...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2] net: stmmac: bug fix to synchronize stmmac_open and > stmmac_dvr_probe > > From: "Kweh, Hock Leong" > Date: Tue, 27 Dec 2016 22:42:36 +0800 > > > From: "Kweh, Hock Leong" > > You are not the author of this change, do not take credit for it. > > You have copied Florian's patch character by character, therefore > he is the author. > > You also didn't CC: the netdev mailing list properly. Hi David & Florian, Just to clarify that I do not copy exactly from Florian. I have changed it to have proper handling on mdio unregister while netdev_register() failed as showed below: return 0; -error_mdio_register: - unregister_netdev(ndev); error_netdev_register: + stmmac_mdio_unregister(ndev); +error_mdio_register: netif_napi_del(&priv->napi); Vs + + return ret; error_mdio_register: - unregister_netdev(ndev); -error_netdev_register: netif_napi_del(&priv->napi); Just to point it out here, so that Florian could aware if he/she submit this fix to the mailing list. Thanks & Regards, Wilson
RE: [PATCH v2] net: stmmac: bug fix to synchronize stmmac_open and stmmac_dvr_probe
> -Original Message- > From: David Miller [mailto:da...@davemloft.net] > Sent: Wednesday, December 28, 2016 12:34 AM > To: Kweh, Hock Leong > Cc: joao.pi...@synopsys.com; peppe.cavall...@st.com; > seraphin.bonna...@st.com; f.faine...@gmail.com; > alexandre.tor...@gmail.com; manab...@gmail.com; niklas.cas...@axis.com; > jo...@kernel.org; pa...@ucw.cz; Ong, Boon Leong > ; Voon, Weifeng ; > lars.pers...@axis.com; net...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2] net: stmmac: bug fix to synchronize stmmac_open and > stmmac_dvr_probe > > From: "Kweh, Hock Leong" > Date: Tue, 27 Dec 2016 22:42:36 +0800 > > > From: "Kweh, Hock Leong" > > You are not the author of this change, do not take credit for it. > > You have copied Florian's patch character by character, therefore > he is the author. > > You also didn't CC: the netdev mailing list properly. Noted & Thanks. Hi Florian, could you submit this fix from your side so that you are the author. I will help to test out. Thanks & Regards, Wilson
[PATCH] net: stmmac: fix incorrect bit set in gmac4 mdio addr register
From: "Kweh, Hock Leong" Fixing the gmac4 mdio write access to use MII_GMAC4_WRITE only instead of OR together with MII_WRITE. Signed-off-by: Kweh, Hock Leong --- drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c |4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c index fda01f7..b0344c2 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_mdio.c @@ -116,7 +116,7 @@ static int stmmac_mdio_write(struct mii_bus *bus, int phyaddr, int phyreg, unsigned int mii_address = priv->hw->mii.addr; unsigned int mii_data = priv->hw->mii.data; - u32 value = MII_WRITE | MII_BUSY; + u32 value = MII_BUSY; value |= (phyaddr << priv->hw->mii.addr_shift) & priv->hw->mii.addr_mask; @@ -126,6 +126,8 @@ static int stmmac_mdio_write(struct mii_bus *bus, int phyaddr, int phyreg, & priv->hw->mii.clk_csr_mask; if (priv->plat->has_gmac4) value |= MII_GMAC4_WRITE; + else + value |= MII_WRITE; /* Wait until any existing MII operation is complete */ if (stmmac_mdio_busy_wait(priv->ioaddr, mii_address)) -- 1.7.9.5
[PATCH v2] net: stmmac: bug fix to synchronize stmmac_open and stmmac_dvr_probe
From: "Kweh, Hock Leong"
If kernel module stmmac driver being loaded after OS booted, there is a
race condition between stmmac_open() and stmmac_mdio_register(), which is
invoked inside stmmac_dvr_probe(), and the error is showed in dmesg log as
PHY not found and stmmac_open() failed:
[ 473.919358] stmmaceth :01:00.0 (unnamed net_device) (uninitialized):
stmmac_dvr_probe: warning: cannot get CSR clock
[ 473.919382] stmmaceth :01:00.0: no reset control found
[ 473.919412] stmmac - user ID: 0x10, Synopsys ID: 0x42
[ 473.919429] stmmaceth :01:00.0: DMA HW capability register supported
[ 473.919436] stmmaceth :01:00.0: RX Checksum Offload Engine supported
[ 473.919443] stmmaceth :01:00.0: TX Checksum insertion supported
[ 473.919451] stmmaceth :01:00.0 (unnamed net_device) (uninitialized):
Enable RX Mitigation via HW Watchdog Timer
[ 473.921395] libphy: PHY stmmac-1:00 not found
[ 473.921417] stmmaceth :01:00.0 eth0: Could not attach to PHY
[ 473.921427] stmmaceth :01:00.0 eth0: stmmac_open: Cannot attach to
PHY (error: -19)
[ 473.959710] libphy: stmmac: probed
[ 473.959724] stmmaceth :01:00.0 eth0: PHY ID 01410cc2 at 0 IRQ POLL
(stmmac-1:00) active
[ 473.959728] stmmaceth :01:00.0 eth0: PHY ID 01410cc2 at 1 IRQ POLL
(stmmac-1:01)
[ 473.959731] stmmaceth :01:00.0 eth0: PHY ID 01410cc2 at 2 IRQ POLL
(stmmac-1:02)
[ 473.959734] stmmaceth :01:00.0 eth0: PHY ID 01410cc2 at 3 IRQ POLL
(stmmac-1:03)
The resolution moved the register_netdev() function call to the end of
stmmac_dvr_probe() after stmmac_mdio_register().
Suggested-by: Florian Fainelli
Signed-off-by: Kweh, Hock Leong
---
changelog v2:
* Re-implemented the fix base on David & Florian suggestion and tested.
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 18 +-
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index bb40382..263ac53 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3339,13 +3339,6 @@ int stmmac_dvr_probe(struct device *device,
spin_lock_init(&priv->lock);
- ret = register_netdev(ndev);
- if (ret) {
- netdev_err(priv->dev, "%s: ERROR %i registering the device\n",
- __func__, ret);
- goto error_netdev_register;
- }
-
/* If a specific clk_csr value is passed from the platform
* this means that the CSR Clock Range selection cannot be
* changed at run-time and it is fixed. Viceversa the driver'll try to
@@ -3372,11 +3365,18 @@ int stmmac_dvr_probe(struct device *device,
}
}
+ ret = register_netdev(ndev);
+ if (ret) {
+ netdev_err(priv->dev, "%s: ERROR %i registering the device\n",
+ __func__, ret);
+ goto error_netdev_register;
+ }
+
return 0;
-error_mdio_register:
- unregister_netdev(ndev);
error_netdev_register:
+ stmmac_mdio_unregister(ndev);
+error_mdio_register:
netif_napi_del(&priv->napi);
error_hw_init:
clk_disable_unprepare(priv->pclk);
--
1.7.9.5
RE: [PATCH] net: stmmac: synchronize stmmac_open and stmmac_dvr_probe
> -Original Message- > From: David Miller [mailto:da...@davemloft.net] > Sent: Tuesday, December 27, 2016 12:55 PM > To: Kweh, Hock Leong > Cc: joao.pi...@synopsys.com; peppe.cavall...@st.com; > seraphin.bonna...@st.com; alexandre.tor...@gmail.com; > manab...@gmail.com; niklas.cas...@axis.com; jo...@kernel.org; > pa...@ucw.cz; Ong, Boon Leong ; > net...@vger.kernel.org; linux-kernel@vger.kernel.org; Voon, Weifeng > ; lars.pers...@axis.com > Subject: Re: [PATCH] net: stmmac: synchronize stmmac_open and > stmmac_dvr_probe > > From: "Kweh, Hock Leong" > Date: Tue, 27 Dec 2016 19:44:59 +0800 > > > From: "Kweh, Hock Leong" > > > > If kernel module stmmac driver being loaded after OS booted, there is a > > race condition between stmmac_open() and stmmac_mdio_register(), which is > > invoked inside stmmac_dvr_probe(), and the error is showed in dmesg log as > > PHY not found and stmmac_open() failed: > ... > > The resolution used wait_for_completion_interruptible() to synchronize > > stmmac_open() and stmmac_dvr_probe() to prevent the race condition > > happening. > > > > Signed-off-by: Kweh, Hock Leong > > The proper thing to do is to make sure register_netdevice() is not > invoked until it is %100 safe to call stmmac_open(). Noted & thanks. Will look into it. Regards, Wilson
RE: [PATCH] net: stmmac: synchronize stmmac_open and stmmac_dvr_probe
> -Original Message-
> From: Florian Fainelli [mailto:f.faine...@gmail.com]
> Sent: Tuesday, December 27, 2016 1:14 PM
> To: Kweh, Hock Leong ; David S. Miller
> ; Joao Pinto ; Giuseppe
> CAVALLARO ; seraphin.bonna...@st.com
> Cc: Alexandre TORGUE ; Joachim Eastwood
> ; Niklas Cassel ; Johan Hovold
> ; pa...@ucw.cz; Ong, Boon Leong
> ; netdev ; LKML ker...@vger.kernel.org>; Voon, Weifeng ; Lars
> Persson
> Subject: Re: [PATCH] net: stmmac: synchronize stmmac_open and
> stmmac_dvr_probe
>
>
>
> On 12/26/2016 09:10 PM, Florian Fainelli wrote:
> >
> >
> > On 12/27/2016 03:44 AM, Kweh, Hock Leong wrote:
> >> From: "Kweh, Hock Leong"
> >>
> >> If kernel module stmmac driver being loaded after OS booted, there is a
> >> race condition between stmmac_open() and stmmac_mdio_register(), which
> is
> >> invoked inside stmmac_dvr_probe(), and the error is showed in dmesg log as
> >> PHY not found and stmmac_open() failed:
> >> [ 473.919358] stmmaceth :01:00.0 (unnamed net_device) (uninitialized):
> >>stmmac_dvr_probe: warning: cannot get CSR clock
> >> [ 473.919382] stmmaceth :01:00.0: no reset control found
> >> [ 473.919412] stmmac - user ID: 0x10, Synopsys ID: 0x42
> >> [ 473.919429] stmmaceth :01:00.0: DMA HW capability register
> supported
> >> [ 473.919436] stmmaceth :01:00.0: RX Checksum Offload Engine
> supported
> >> [ 473.919443] stmmaceth :01:00.0: TX Checksum insertion supported
> >> [ 473.919451] stmmaceth :01:00.0 (unnamed net_device) (uninitialized):
> >>Enable RX Mitigation via HW Watchdog Timer
> >> [ 473.921395] libphy: PHY stmmac-1:00 not found
> >> [ 473.921417] stmmaceth :01:00.0 eth0: Could not attach to PHY
> >> [ 473.921427] stmmaceth :01:00.0 eth0: stmmac_open: Cannot attach
> to
> >>PHY (error: -19)
> >> [ 473.959710] libphy: stmmac: probed
> >> [ 473.959724] stmmaceth :01:00.0 eth0: PHY ID 01410cc2 at 0 IRQ POLL
> >>(stmmac-1:00) active
> >> [ 473.959728] stmmaceth :01:00.0 eth0: PHY ID 01410cc2 at 1 IRQ POLL
> >>(stmmac-1:01)
> >> [ 473.959731] stmmaceth :01:00.0 eth0: PHY ID 01410cc2 at 2 IRQ POLL
> >>(stmmac-1:02)
> >> [ 473.959734] stmmaceth :01:00.0 eth0: PHY ID 01410cc2 at 3 IRQ POLL
> >>(stmmac-1:03)
> >>
> >> The resolution used wait_for_completion_interruptible() to synchronize
> >> stmmac_open() and stmmac_dvr_probe() to prevent the race condition
> >> happening.
> >
> > The proper fix for this would be to have register_netdev() be the last
> > thing done in stmmac_drv_probe(), whereas right now, the last thing done
> > is stmmac_mdio_register(), leading the window you are seeing here, where
> > the network interface can be open prior to all resources being set up,
> > including, but not limited to MDIO devices.
>
> Something like the following untested patch should plug this race:
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index bb40382e205d..5910ea51f8f6 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -3339,13 +3339,6 @@ int stmmac_dvr_probe(struct device *device,
>
> spin_lock_init(&priv->lock);
>
> - ret = register_netdev(ndev);
> - if (ret) {
> - netdev_err(priv->dev, "%s: ERROR %i registering the
> device\n",
> - __func__, ret);
> - goto error_netdev_register;
> - }
> -
> /* If a specific clk_csr value is passed from the platform
> * this means that the CSR Clock Range selection cannot be
> * changed at run-time and it is fixed. Viceversa the driver'll
> try to
> @@ -3372,11 +3365,14 @@ int stmmac_dvr_probe(struct device *device,
> }
> }
>
> - return 0;
> + ret = register_netdev(ndev);
> + if (ret)
> + netdev_err(priv->dev, "%s: ERROR %i registering the
> device\n",
> + __func__, ret);
> +
> + return ret;
>
> error_mdio_register:
> - unregister_netdev(ndev);
> -error_netdev_register:
> netif_napi_del(&priv->napi);
> error_hw_init:
> clk_disable_unprepare(priv->pclk);
>
> --
> Florian
Thanks. Will try out to confirm.
Regards,
Wilson
[PATCH] net: stmmac: synchronize stmmac_open and stmmac_dvr_probe
From: "Kweh, Hock Leong"
If kernel module stmmac driver being loaded after OS booted, there is a
race condition between stmmac_open() and stmmac_mdio_register(), which is
invoked inside stmmac_dvr_probe(), and the error is showed in dmesg log as
PHY not found and stmmac_open() failed:
[ 473.919358] stmmaceth :01:00.0 (unnamed net_device) (uninitialized):
stmmac_dvr_probe: warning: cannot get CSR clock
[ 473.919382] stmmaceth :01:00.0: no reset control found
[ 473.919412] stmmac - user ID: 0x10, Synopsys ID: 0x42
[ 473.919429] stmmaceth :01:00.0: DMA HW capability register supported
[ 473.919436] stmmaceth :01:00.0: RX Checksum Offload Engine supported
[ 473.919443] stmmaceth :01:00.0: TX Checksum insertion supported
[ 473.919451] stmmaceth :01:00.0 (unnamed net_device) (uninitialized):
Enable RX Mitigation via HW Watchdog Timer
[ 473.921395] libphy: PHY stmmac-1:00 not found
[ 473.921417] stmmaceth :01:00.0 eth0: Could not attach to PHY
[ 473.921427] stmmaceth :01:00.0 eth0: stmmac_open: Cannot attach to
PHY (error: -19)
[ 473.959710] libphy: stmmac: probed
[ 473.959724] stmmaceth :01:00.0 eth0: PHY ID 01410cc2 at 0 IRQ POLL
(stmmac-1:00) active
[ 473.959728] stmmaceth :01:00.0 eth0: PHY ID 01410cc2 at 1 IRQ POLL
(stmmac-1:01)
[ 473.959731] stmmaceth :01:00.0 eth0: PHY ID 01410cc2 at 2 IRQ POLL
(stmmac-1:02)
[ 473.959734] stmmaceth :01:00.0 eth0: PHY ID 01410cc2 at 3 IRQ POLL
(stmmac-1:03)
The resolution used wait_for_completion_interruptible() to synchronize
stmmac_open() and stmmac_dvr_probe() to prevent the race condition
happening.
Signed-off-by: Kweh, Hock Leong
---
drivers/net/ethernet/stmicro/stmmac/stmmac.h |1 +
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++
2 files changed, 11 insertions(+)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index eab04ae..5daf8a5 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -131,6 +131,7 @@ struct stmmac_priv {
u32 rx_tail_addr;
u32 tx_tail_addr;
u32 mss;
+ struct completion probe_done;
#ifdef CONFIG_DEBUG_FS
struct dentry *dbgfs_dir;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index bb40382..28e85f6 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1770,6 +1770,14 @@ static int stmmac_open(struct net_device *dev)
struct stmmac_priv *priv = netdev_priv(dev);
int ret;
+ ret = wait_for_completion_interruptible(&priv->probe_done);
+ if (ret) {
+ netdev_err(priv->dev,
+ "%s: Interrupted while waiting probe completion\n",
+ __func__);
+ return ret;
+ }
+
stmmac_check_ether_addr(priv);
if (priv->hw->pcs != STMMAC_PCS_RGMII &&
@@ -3226,6 +3234,7 @@ int stmmac_dvr_probe(struct device *device,
priv = netdev_priv(ndev);
priv->device = device;
priv->dev = ndev;
+ init_completion(&priv->probe_done);
stmmac_set_ethtool_ops(ndev);
priv->pause = pause;
@@ -3372,6 +3381,7 @@ int stmmac_dvr_probe(struct device *device,
}
}
+ complete_all(&priv->probe_done);
return 0;
error_mdio_register:
--
1.7.9.5
[PATCH] iio: fix pressure data output unit in hid-sensor-attributes
From: "Kweh, Hock Leong"
According to IIO ABI definition, IIO_PRESSURE data output unit is
kilopascal:
http://lxr.free-electrons.com/source/Documentation/ABI/testing/sysfs-bus-iio
This patch fix output unit of HID pressure sensor IIO driver from pascal to
kilopascal to follow IIO ABI definition.
Signed-off-by: Kweh, Hock Leong
---
.../iio/common/hid-sensors/hid-sensor-attributes.c |4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
index e81f434..dc33c1d 100644
--- a/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
+++ b/drivers/iio/common/hid-sensors/hid-sensor-attributes.c
@@ -56,8 +56,8 @@ static struct {
{HID_USAGE_SENSOR_ALS, 0, 1, 0},
{HID_USAGE_SENSOR_ALS, HID_USAGE_SENSOR_UNITS_LUX, 1, 0},
- {HID_USAGE_SENSOR_PRESSURE, 0, 10, 0},
- {HID_USAGE_SENSOR_PRESSURE, HID_USAGE_SENSOR_UNITS_PASCAL, 1, 0},
+ {HID_USAGE_SENSOR_PRESSURE, 0, 100, 0},
+ {HID_USAGE_SENSOR_PRESSURE, HID_USAGE_SENSOR_UNITS_PASCAL, 0, 1000},
};
static int pow_10(unsigned power)
--
1.7.9.5
RE: [PATCH] efi/capsule: Make efi_capsule_pending() lockless
> -Original Message- > From: Matt Fleming [mailto:m...@codeblueprint.co.uk] > Sent: Wednesday, May 04, 2016 10:36 PM > To: Kweh, Hock Leong; Bryan O'Donoghue > Cc: linux-...@vger.kernel.org; linux-kernel@vger.kernel.org; Ard Biesheuvel; > joeyli; Borislav Petkov > Subject: Re: [PATCH] efi/capsule: Make efi_capsule_pending() lockless > > On Wed, 04 May, at 02:20:42PM, Borislav Petkov wrote: > > > > Blergh. > > Wilson, Bryan, what kind of rollback support does the Intel Quark have if its > firmware update is interrupted? > > The interruption could be for a number of reasons including power loss, or > the example in this case, rebooting due to panic(). If not mistaken, the EFI firmware will not update a partially uploaded binary due to checksum error. User is required to re-update the efi capsule again on the next boot up. Regards, Wilson
[tip:efi/core] efi: Add misc char driver interface to update EFI firmware
Commit-ID: 65117f1aa1b2d145fd5ca376bde642794d0aae1b
Gitweb: http://git.kernel.org/tip/65117f1aa1b2d145fd5ca376bde642794d0aae1b
Author: Kweh, Hock Leong
AuthorDate: Mon, 25 Apr 2016 21:07:01 +0100
Committer: Ingo Molnar
CommitDate: Thu, 28 Apr 2016 11:34:05 +0200
efi: Add misc char driver interface to update EFI firmware
This patch introduces a kernel module to expose a capsule loader
interface (misc char device file note) for users to upload capsule
binaries.
Example:
cat firmware.bin > /dev/efi_capsule_loader
Any upload error will be returned while doing "cat" through file
operation write() function call.
Signed-off-by: Kweh, Hock Leong
[ Update comments and Kconfig text ]
Signed-off-by: Matt Fleming
Reviewed-by: Bryan O'Donoghue
Acked-by: Greg Kroah-Hartman
Cc: Andy Lutomirski
Cc: Ard Biesheuvel
Cc: Borislav Petkov
Cc: Peter Jones
Cc: Peter Zijlstra
Cc: Sam Protsenko
Cc: Thomas Gleixner
Cc: joeyli
Cc: linux-...@vger.kernel.org
Link:
http://lkml.kernel.org/r/1461614832-17633-30-git-send-email-m...@codeblueprint.co.uk
Signed-off-by: Ingo Molnar
---
drivers/firmware/efi/Kconfig | 10 +
drivers/firmware/efi/Makefile | 1 +
drivers/firmware/efi/capsule-loader.c | 343 ++
3 files changed, 354 insertions(+)
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 0b0b635..6394152 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -102,6 +102,16 @@ config EFI_BOOTLOADER_CONTROL
bootloader reads this reboot reason and takes particular
action according to its policy.
+config EFI_CAPSULE_LOADER
+ tristate "EFI capsule loader"
+ depends on EFI
+ help
+ This option exposes a loader interface "/dev/efi_capsule_loader" for
+ users to load EFI capsules. This driver requires working runtime
+ capsule support in the firmware, which many OEMs do not provide.
+
+ Most users should say N.
+
endmenu
config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index fb8ad5d..a219640 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -24,3 +24,4 @@ obj-$(CONFIG_EFI_BOOTLOADER_CONTROL) += efibc.o
arm-obj-$(CONFIG_EFI) := arm-init.o arm-runtime.o
obj-$(CONFIG_ARM) += $(arm-obj-y)
obj-$(CONFIG_ARM64)+= $(arm-obj-y)
+obj-$(CONFIG_EFI_CAPSULE_LOADER) += capsule-loader.o
diff --git a/drivers/firmware/efi/capsule-loader.c
b/drivers/firmware/efi/capsule-loader.c
new file mode 100644
index 000..c99c24b
--- /dev/null
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -0,0 +1,343 @@
+/*
+ * EFI capsule loader driver.
+ *
+ * Copyright 2015 Intel Corporation
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#define pr_fmt(fmt) "efi: " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define NO_FURTHER_WRITE_ACTION -1
+
+struct capsule_info {
+ boolheader_obtained;
+ int reset_type;
+ longindex;
+ size_t count;
+ size_t total_size;
+ struct page **pages;
+ size_t page_bytes_remain;
+};
+
+/**
+ * efi_free_all_buff_pages - free all previous allocated buffer pages
+ * @cap_info: pointer to current instance of capsule_info structure
+ *
+ * In addition to freeing buffer pages, it flags NO_FURTHER_WRITE_ACTION
+ * to cease processing data in subsequent write(2) calls until close(2)
+ * is called.
+ **/
+static void efi_free_all_buff_pages(struct capsule_info *cap_info)
+{
+ while (cap_info->index > 0)
+ __free_page(cap_info->pages[--cap_info->index]);
+
+ cap_info->index = NO_FURTHER_WRITE_ACTION;
+}
+
+/**
+ * efi_capsule_setup_info - obtain the efi capsule header in the binary and
+ * setup capsule_info structure
+ * @cap_info: pointer to current instance of capsule_info structure
+ * @kbuff: a mapped first page buffer pointer
+ * @hdr_bytes: the total received number of bytes for efi header
+ **/
+static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
+ void *kbuff, size_t hdr_bytes)
+{
+ efi_capsule_header_t *cap_hdr;
+ size_t pages_needed;
+ int ret;
+ void *temp_page;
+
+ /* Only process data block that is larger than efi header size */
+ if (hdr_bytes < sizeof(efi_capsule_header_t))
+ return 0;
+
+ /* Reset back to the correct offset of header */
+ cap_hdr = kbuff - cap_info->count;
+ pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >> PAGE_SHIFT;
+
+ if (pages_needed == 0) {
+
RE: [PATCH v11 1/1] efi: a misc char interface for user to update efi firmware
> -Original Message- > From: Matt Fleming [mailto:m...@console-pimps.org] > Sent: Monday, February 08, 2016 11:14 PM > > On Fri, 29 Jan, at 12:39:54PM, Kweh Hock Leong wrote: > > From: "Kweh, Hock Leong" > > > > Introducing a kernel module to expose capsule loader interface > > (misc char device file note) for users to upload capsule binaries. > > > > Example: > > cat firmware.bin > /dev/efi_capsule_loader > > > > This patch also export efi_capsule_supported() function symbol for > > verifying the submitted capsule header in this kernel module. > > > > Cc: Matt Fleming > > Signed-off-by: Kweh, Hock Leong > > --- > > drivers/firmware/efi/Kconfig |9 + > > drivers/firmware/efi/Makefile |1 + > > drivers/firmware/efi/capsule-loader.c | 334 > + > > drivers/firmware/efi/capsule.c|1 + > > 4 files changed, 345 insertions(+) > > create mode 100644 drivers/firmware/efi/capsule-loader.c > > OK, I've picked this up. I did make some modifications based on > Bryan's comments from v10 of this patch. > > You can see a diff of the changes below, but roughly, > > 1. Use Bryan's comments for changelog and expand it a bit > 2. Add more detail to Kconfig entry, most people don't need this > 3. Sprinkled comments from Bryan in the kerneldoc comments > 4. Deleted a level of indentation in efi_capsule_setup_info() > 5. Local variable instead of indexing ->pages in efi_capsule_write() > 6. Fix memory leak in efi_capsule_open() > > My plan is to include this patch in the EFI capsule series, and resend > the whole thing out. > Thank Matt and also appreciate others who helped on reviewing, testing as well as provided design idea. Thanks & Regards, Wilson
[PATCH v11 0/1] Enable capsule loader interface for efi firmware updating
From: "Kweh, Hock Leong" Dear maintainers & communities, This patchset is created on top of Matt's patchset: 1.)https://lkml.org/lkml/2014/10/7/390 "[PATCH 1/2] efi: Move efi_status_to_err() to efi.h" 2.)https://lkml.org/lkml/2014/10/7/391 "[PATCH 2/2] efi: Capsule update support" It expose a misc char interface for user to upload the capsule binary and calling efi_capsule_update() API to pass the binary to EFI firmware. The steps to update efi firmware are: 1.) cat firmware.cap > /dev/efi_capsule_loader 2.) reboot Any failed upload error message will be returned while doing "cat" through file operation write() function call. Tested the code with Intel Quark Galileo GEN1 platform. Thanks. --- changelog v11: * rebase to v4.5-rc1 * correct vocabulary base on Bryan's comments * Optimise ->pages & ->index code flow base on Matt's comments changelog v10: * rebase to v4.4 * added efi runtime services check at efi_capsule_loader_init() * fixed checkpatch issues * code clean up base on Borislav's comments changelog v9: * squash 2 patches to become 1 patch * change function param to pass in cap_info instead of file structure * perform both alloc inside efi_capsule_setup_info() * change to use multiple exit labels instead of one function call * further code clean up base on Matt's comments changelog v8: * further clean up on kunmap() & efi_free_all_buff_pages() * design enhanced to support 1st few writes are less than efi header size * removed support to padding capsule and flag error once the upload size bigger than header defined size changelog v7: * add successful message printed in dmesg * shorten the code in efi_capsule_write() by splitting out efi_capsule_setup_info() & efi_capsule_submit_update() functions * design added capability to support multiple file open action * re-write those comments by following standard format * design added the "uncomplete" error return through flush() file operation changelog v6: * clean up on error handling for better code flow and review * clean up on pr_err() for critical error only * design taking care writing block that below PAGE_SIZE * once error has occurred, design will return -EIO until file close * document design expectations/scenarios in the code * change the dynamic allocation cap_info struct to statically allocated changelog v5: * changed to new design without leveraging firmware_class API * use misc_char device interface instead of sysfs * error return through file Write() function call Kweh, Hock Leong (1): efi: a misc char interface for user to update efi firmware drivers/firmware/efi/Kconfig |9 + drivers/firmware/efi/Makefile |1 + drivers/firmware/efi/capsule-loader.c | 334 + drivers/firmware/efi/capsule.c|1 + 4 files changed, 345 insertions(+) create mode 100644 drivers/firmware/efi/capsule-loader.c -- 1.7.9.5
[PATCH v11 1/1] efi: a misc char interface for user to update efi firmware
From: "Kweh, Hock Leong"
Introducing a kernel module to expose capsule loader interface
(misc char device file note) for users to upload capsule binaries.
Example:
cat firmware.bin > /dev/efi_capsule_loader
This patch also export efi_capsule_supported() function symbol for
verifying the submitted capsule header in this kernel module.
Cc: Matt Fleming
Signed-off-by: Kweh, Hock Leong
---
drivers/firmware/efi/Kconfig |9 +
drivers/firmware/efi/Makefile |1 +
drivers/firmware/efi/capsule-loader.c | 334 +
drivers/firmware/efi/capsule.c|1 +
4 files changed, 345 insertions(+)
create mode 100644 drivers/firmware/efi/capsule-loader.c
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index e1670d5..73c14af 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -87,6 +87,15 @@ config EFI_RUNTIME_WRAPPERS
config EFI_ARMSTUB
bool
+config EFI_CAPSULE_LOADER
+ tristate "EFI capsule loader"
+ depends on EFI
+ help
+ This option exposes a loader interface "/dev/efi_capsule_loader" for
+ users to load EFI capsules.
+
+ If unsure, say N.
+
endmenu
config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index e4468c7..7d6e1e8 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -18,6 +18,7 @@ obj-$(CONFIG_EFI_RUNTIME_MAP) += runtime-map.o
obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o
obj-$(CONFIG_EFI_STUB) += libstub/
obj-$(CONFIG_EFI_FAKE_MEMMAP) += fake_mem.o
+obj-$(CONFIG_EFI_CAPSULE_LOADER) += capsule-loader.o
arm-obj-$(CONFIG_EFI) := arm-init.o arm-runtime.o
obj-$(CONFIG_ARM) += $(arm-obj-y)
diff --git a/drivers/firmware/efi/capsule-loader.c
b/drivers/firmware/efi/capsule-loader.c
new file mode 100644
index 000..acffd76
--- /dev/null
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -0,0 +1,334 @@
+/*
+ * EFI capsule loader driver.
+ *
+ * Copyright 2015 Intel Corporation
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#define pr_fmt(fmt) "EFI: " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define NO_FURTHER_WRITE_ACTION -1
+
+struct capsule_info {
+ boolheader_obtained;
+ int reset_type;
+ longindex;
+ size_t count;
+ size_t total_size;
+ struct page **pages;
+ size_t page_bytes_remain;
+};
+
+/**
+ * efi_free_all_buff_pages - free all previous allocated buffer pages
+ * @cap_info: pointer to current instance of capsule_info structure
+ *
+ * In addition of freeing buffer pages, it flags NO_FURTHER_WRITE_ACTION
+ * to cease processing data in subsequent write(2) calls until close(2)
+ * is called.
+ **/
+static void efi_free_all_buff_pages(struct capsule_info *cap_info)
+{
+ while (cap_info->index > 0)
+ __free_page(cap_info->pages[--cap_info->index]);
+
+ cap_info->index = NO_FURTHER_WRITE_ACTION;
+}
+
+/**
+ * efi_capsule_setup_info - to obtain the efi capsule header in the binary and
+ * setup capsule_info structure
+ * @cap_info: pointer to current instance of capsule_info structure
+ * @kbuff: a mapped first page buffer pointer
+ * @hdr_bytes: the total received number of bytes for efi header
+ **/
+static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
+ void *kbuff, size_t hdr_bytes)
+{
+ int ret;
+ void *temp_page;
+
+ /* Only process data block that is larger than a efi header size */
+ if (hdr_bytes >= sizeof(efi_capsule_header_t)) {
+ /* Reset back to the correct offset of header */
+ efi_capsule_header_t *cap_hdr = kbuff - cap_info->count;
+ size_t pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
+ PAGE_SHIFT;
+
+ if (pages_needed == 0) {
+ pr_err("%s: pages count invalid\n", __func__);
+ return -EINVAL;
+ }
+
+ /* Check if the capsule binary supported */
+ ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
+ cap_hdr->imagesize,
+ &cap_info->reset_type);
+ if (ret) {
+ pr_err("%s: efi_capsule_supported() failed\n",
+ __func__);
+ return ret;
+ }
+
+ cap_info->total_size = cap_hdr->
RE: [PATCH v10 1/1] efi: a misc char interface for user to update efi firmware
> -Original Message- > From: Matt Fleming [mailto:m...@codeblueprint.co.uk] > Sent: Thursday, January 28, 2016 8:16 PM > > On Tue, 26 Jan, at 03:10:03AM, Kweh Hock Leong wrote: > > > > > > This mutex is not needed. It doesn't protect anything in your code. > > > > This is to synchronize/serializes one of the instant is calling > efi_capsule_supported() > > and the other one is calling efi_capsule_update() which they are exported > symbol > > from capsule.c. > > You don't need to synchronise them. > > Looking at my original capsule patches I can see why you might be > doing this locking, but it's unnecessary. You don't need to serialise > access to efi_capsule_supported() and efi_capsule_update(). > > Internally to the core EFI capsule code we need to ensure that we > don't allow capsules with conflicting reset types to be sent to the > firmware (how would we know the type of reset to perform?), but that > should be entirely transparent to your driver. > > I'm planning on re-sending my capsule patches, so all this should > become clearer. > Ok. Noted. Will remove it and submit for v11. > > > This function would be much more simple if you handled the above > > > condition differently. > > > > > > Instead of having code in efi_capsule_setup_info() to allocate the > > > initial ->pages memory, why not just allocate it in efi_capsule_open() > > > at the same time as you allocate the private_data? You can then > > > free it in efi_capsule_release() (you're leaking it at the moment). > > > > > > You are always going to need at least one element in ->pages for > > > successful operation, so you might as well allocate it up front. > > > > Just want to double check I understand you correctly. Do you mean > > I should free ->pages during close(2) but not free the ->pages[X] if > > there is any successfully submitted? > > Yes, that's correct. Ok. Noted. Thanks & Regards, Wilson
RE: [PATCH v10 1/1] efi: a misc char interface for user to update efi firmware
> -Original Message-
> From: Matt Fleming [mailto:m...@console-pimps.org]
> Sent: Thursday, January 21, 2016 7:52 PM
>
> On Fri, 18 Dec, at 08:13:01PM, Kweh Hock Leong wrote:
> > From: "Kweh, Hock Leong"
> >
> > Introducing a kernel module to expose capsule loader interface
> > (misc char device file note) for user to upload capsule binaries.
> >
> > Example method to load the capsule binary:
> > cat firmware.bin > /dev/efi_capsule_loader
> >
> > This patch also export efi_capsule_supported() function symbol for
> > verifying the submitted capsule header in this kernel module.
> >
> > Cc: Matt Fleming
> > Signed-off-by: Kweh, Hock Leong
> > ---
> > drivers/firmware/efi/Kconfig | 10
> > drivers/firmware/efi/Makefile |1
> > drivers/firmware/efi/capsule-loader.c | 355
> +
> > drivers/firmware/efi/capsule.c|1
> > 4 files changed, 367 insertions(+)
> > create mode 100644 drivers/firmware/efi/capsule-loader.c
>
> [...]
>
> > +#define pr_fmt(fmt) "EFI: " fmt
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define NO_FURTHER_WRITE_ACTION -1
> > +
> > +struct capsule_info {
> > + boolheader_obtained;
> > + int reset_type;
> > + longindex;
> > + size_t count;
> > + size_t total_size;
> > + struct page **pages;
> > + size_t page_bytes_remain;
> > +};
> > +
> > +static DEFINE_MUTEX(capsule_loader_lock);
>
> This mutex is not needed. It doesn't protect anything in your code.
This is to synchronize/serializes one of the instant is calling
efi_capsule_supported()
and the other one is calling efi_capsule_update() which they are exported symbol
from capsule.c.
>
> > +/**
> > + * efi_free_all_buff_pages - free all previous allocated buffer pages
> > + * @cap_info: pointer to current instance of capsule_info structure
> > + *
> > + * Besides freeing the buffer pages, it also flagged an
> > + * NO_FURTHER_WRITE_ACTION flag for indicating to the next write
> entries
> > + * that there is a flaw happened and any subsequence actions are not
> > + * valid unless close(2).
> > + **/
> > +static void efi_free_all_buff_pages(struct capsule_info *cap_info)
> > +{
> > + while (cap_info->index > 0)
> > + __free_page(cap_info->pages[--cap_info->index]);
> > +
> > + kfree(cap_info->pages);
> > +
> > + cap_info->index = NO_FURTHER_WRITE_ACTION;
> > +}
> > +
> > +/**
> > + * efi_capsule_setup_info - to obtain the efi capsule header in the binary
> and
> > + * setup capsule_info structure
> > + * @cap_info: pointer to current instance of capsule_info structure
> > + * @kbuff: a mapped 1st page buffer pointer
> > + * @hdr_bytes: the total bytes of received efi header
> > + **/
> > +static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
> > + void *kbuff, size_t hdr_bytes)
> > +{
> > + int ret;
> > + void *temp_page;
> > +
> > + /* Handling 1st block is less than header size */
> > + if (hdr_bytes < sizeof(efi_capsule_header_t)) {
> > + if (!cap_info->pages) {
> > + cap_info->pages = kzalloc(sizeof(void *),
> GFP_KERNEL);
> > + if (!cap_info->pages) {
> > + pr_debug("%s: kzalloc() failed\n", __func__);
> > + return -ENOMEM;
> > + }
> > + }
>
> This function would be much more simple if you handled the above
> condition differently.
>
> Instead of having code in efi_capsule_setup_info() to allocate the
> initial ->pages memory, why not just allocate it in efi_capsule_open()
> at the same time as you allocate the private_data? You can then
> free it in efi_capsule_release() (you're leaking it at the moment).
>
> You are always going to need at least one element in ->pages for
> successful operation, so you might as well allocate it up front.
Just want to double check I understand you correctly. Do you mean
I should free ->pages during close(2) but not free the ->pages[X] if
there is any successfully submitted?
>
> > + } else {
> > + /* Reset back to the correct offset of header */
> > +
RE: [PATCH v10 1/1] efi: a misc char interface for user to update efi firmware
> -Original Message-
> From: Bryan O'Donoghue [mailto:pure.lo...@nexus-software.ie]
> Sent: Tuesday, December 22, 2015 1:04 AM
>
> Small nit.
>
Hi, Sorry for the late response. Happy New Year.
> On Fri, 2015-12-18 at 20:13 +0800, Kweh, Hock Leong wrote:
> > From: "Kweh, Hock Leong"
> >
> > Introducing a kernel module to expose capsule loader interface
> > (misc char device file note) for user to upload capsule binaries.
>
> This patch ? introduces a kernel module to expose a capsule loader
> interface... for a user ...
>
> >
> > Example method to load the capsule binary:
>
> Example:
>
Noted.
> > cat firmware.bin > /dev/efi_capsule_loader
> >
> > This patch also export efi_capsule_supported() function symbol for
> > verifying the submitted capsule header in this kernel module.
>
> 1. Should be a separate patch
> 2. Suggested, rewording for that patch log
>
> 2/2 Title
> This patch exports efi_capsule_supported to enable verification of the
> capsule header.
>
> That said - who is the user of this symbol ? Why is it needed ? Anyway
> please consider splitting and rewording.
>
Answered by Borislav.
> >
> > +config EFI_CAPSULE_LOADER
> > + tristate "EFI capsule loader"
> > + depends on EFI
> > + help
> > + This option exposes a loader interface
> > "/dev/efi_capsule_loader" for
> > + user to load EFI capsule binary and update the EFI
> > firmware. After
> > + a successful loading, a system reboot is required.
> > +
> > + If unsure, say N.
> > +
>
> This option exposes a loader interface... for a user to load an EFI
> capsule.
>
> A capsule isn't necessarily exclusively for updating of firmware either
> AFAIK - so I suggest dropping.
>
> http://www.intel.com/content/www/us/en/architecture-and-
> technology/unif
> ied-extensible-firmware-interface/efi-capsule-specification.html
>
> Quote "Capsules were designed for and are intended to be the major
> vehicle for delivering firmware volume changes. There is no requirement
> that capsules be limited to that function."
>
Noted.
> >
> > +
> > +/**
> > + * efi_free_all_buff_pages - free all previous allocated buffer
> > pages
> > + * @cap_info: pointer to current instance of capsule_info structure
> > + *
> > + * Besides freeing the buffer pages, it also flagged an
> > + * NO_FURTHER_WRITE_ACTION flag for indicating to the next
> > write entries
> > + * that there is a flaw happened and any subsequence actions
> > are not
> > + * valid unless close(2).
> > + **/
>
> The description needs to be reworded.
>
> In addition to freeing buffer pages, we flag NO_FURTHER_WRITE_ACTION on
> error and will cease to process data in subsequent efi_capsule_write()
> calls.
>
> (or similar)
>
Noted.
> > +
> > +/**
> > + * efi_capsule_setup_info - to obtain the efi capsule header in the
> > binary and
> > + * setup capsule_info structure
> > + * @cap_info: pointer to current instance of capsule_info structure
> > + * @kbuff: a mapped 1st page buffer pointer
>
> first not 1st
>
Noted.
> > + * @hdr_bytes: the total bytes of received efi header
>
> the total number of received bytes of the EFI header
>
Noted.
> > + **/
> > +static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
> > + void *kbuff, size_t hdr_bytes)
> > +{
> > + int ret;
> > + void *temp_page;
> > +
> > + /* Handling 1st block is less than header size */
> first or initial
> I think you mean to say "Ensure first
>
No. I mean handling. I will change to "first"
> > +
> > + /* Check if the capsule binary supported */
> > + mutex_lock(&capsule_loader_lock);
> > + ret = efi_capsule_supported(cap_hdr->guid, cap_hdr
> > ->flags,
> > + cap_hdr->imagesize,
> > + &cap_info->reset_type);
> > + mutex_unlock(&capsule_loader_lock);
>
> What does this mutex really do ? You hold it here around
> efi_capsule_supported() in the call
>
> chain efi_capsule_write() => efi_capsule_setup_info()
>
> and then again inside of efi_capsule_submit_update() the call chain
>
> chain efi_capsule_write() => efi_capsule_submit_update()
>
> So if its valid to ensure you are synchronizing parallel contexts with
> -respe
[PATCH v10 1/1] efi: a misc char interface for user to update efi firmware
From: "Kweh, Hock Leong"
Introducing a kernel module to expose capsule loader interface
(misc char device file note) for user to upload capsule binaries.
Example method to load the capsule binary:
cat firmware.bin > /dev/efi_capsule_loader
This patch also export efi_capsule_supported() function symbol for
verifying the submitted capsule header in this kernel module.
Cc: Matt Fleming
Signed-off-by: Kweh, Hock Leong
---
drivers/firmware/efi/Kconfig | 10
drivers/firmware/efi/Makefile |1
drivers/firmware/efi/capsule-loader.c | 355 +
drivers/firmware/efi/capsule.c|1
4 files changed, 367 insertions(+)
create mode 100644 drivers/firmware/efi/capsule-loader.c
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index e1670d5..dc2a912 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -87,6 +87,16 @@ config EFI_RUNTIME_WRAPPERS
config EFI_ARMSTUB
bool
+config EFI_CAPSULE_LOADER
+ tristate "EFI capsule loader"
+ depends on EFI
+ help
+ This option exposes a loader interface "/dev/efi_capsule_loader" for
+ user to load EFI capsule binary and update the EFI firmware. After
+ a successful loading, a system reboot is required.
+
+ If unsure, say N.
+
endmenu
config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index fabf801..cf9d9d3 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -18,3 +18,4 @@ obj-$(CONFIG_EFI_RUNTIME_MAP) += runtime-map.o
obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o
obj-$(CONFIG_EFI_STUB) += libstub/
obj-$(CONFIG_EFI_FAKE_MEMMAP) += fake_mem.o
+obj-$(CONFIG_EFI_CAPSULE_LOADER) += capsule-loader.o
diff --git a/drivers/firmware/efi/capsule-loader.c
b/drivers/firmware/efi/capsule-loader.c
new file mode 100644
index 000..e1214bb
--- /dev/null
+++ b/drivers/firmware/efi/capsule-loader.c
@@ -0,0 +1,355 @@
+/*
+ * EFI capsule loader driver.
+ *
+ * Copyright 2015 Intel Corporation
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#define pr_fmt(fmt) "EFI: " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define NO_FURTHER_WRITE_ACTION -1
+
+struct capsule_info {
+ boolheader_obtained;
+ int reset_type;
+ longindex;
+ size_t count;
+ size_t total_size;
+ struct page **pages;
+ size_t page_bytes_remain;
+};
+
+static DEFINE_MUTEX(capsule_loader_lock);
+
+/**
+ * efi_free_all_buff_pages - free all previous allocated buffer pages
+ * @cap_info: pointer to current instance of capsule_info structure
+ *
+ * Besides freeing the buffer pages, it also flagged an
+ * NO_FURTHER_WRITE_ACTION flag for indicating to the next write entries
+ * that there is a flaw happened and any subsequence actions are not
+ * valid unless close(2).
+ **/
+static void efi_free_all_buff_pages(struct capsule_info *cap_info)
+{
+ while (cap_info->index > 0)
+ __free_page(cap_info->pages[--cap_info->index]);
+
+ kfree(cap_info->pages);
+
+ cap_info->index = NO_FURTHER_WRITE_ACTION;
+}
+
+/**
+ * efi_capsule_setup_info - to obtain the efi capsule header in the binary and
+ * setup capsule_info structure
+ * @cap_info: pointer to current instance of capsule_info structure
+ * @kbuff: a mapped 1st page buffer pointer
+ * @hdr_bytes: the total bytes of received efi header
+ **/
+static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
+ void *kbuff, size_t hdr_bytes)
+{
+ int ret;
+ void *temp_page;
+
+ /* Handling 1st block is less than header size */
+ if (hdr_bytes < sizeof(efi_capsule_header_t)) {
+ if (!cap_info->pages) {
+ cap_info->pages = kzalloc(sizeof(void *), GFP_KERNEL);
+ if (!cap_info->pages) {
+ pr_debug("%s: kzalloc() failed\n", __func__);
+ return -ENOMEM;
+ }
+ }
+ } else {
+ /* Reset back to the correct offset of header */
+ efi_capsule_header_t *cap_hdr = kbuff - cap_info->count;
+ size_t pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
+ PAGE_SHIFT;
+
+ if (pages_needed == 0) {
+ pr_err("%s: pages count invalid\n", __func__);
+ return -EINVAL;
+ }
+
+ /* Check if the capsule binary s
[PATCH v10 0/1] Enable capsule loader interface for efi firmware updating
From: "Kweh, Hock Leong" Dear maintainers & communities, This patchset is created on top of Matt's patchset: 1.)https://lkml.org/lkml/2014/10/7/390 "[PATCH 1/2] efi: Move efi_status_to_err() to efi.h" 2.)https://lkml.org/lkml/2014/10/7/391 "[PATCH 2/2] efi: Capsule update support" It expose a misc char interface for user to upload the capsule binary and calling efi_capsule_update() API to pass the binary to EFI firmware. The steps to update efi firmware are: 1.) cat firmware.cap > /dev/efi_capsule_loader 2.) reboot Any failed upload error message will be returned while doing "cat" through file operation write() function call. Tested the code with Intel Quark Galileo GEN1 platform. Thanks. --- changelog v10: * rebase to v4.4 * added efi runtime services check at efi_capsule_loader_init() * fixed checkpatch issues * code clean up base on Borislav's comments changelog v9: * squash 2 patches to become 1 patch * change function param to pass in cap_info instead of file structure * perform both alloc inside efi_capsule_setup_info() * change to use multiple exit labels instead of one function call * further code clean up base on Matt's comments changelog v8: * further clean up on kunmap() & efi_free_all_buff_pages() * design enhanced to support 1st few writes are less than efi header size * removed support to padding capsule and flag error once the upload size bigger than header defined size changelog v7: * add successful message printed in dmesg * shorten the code in efi_capsule_write() by splitting out efi_capsule_setup_info() & efi_capsule_submit_update() functions * design added capability to support multiple file open action * re-write those comments by following standard format * design added the "uncomplete" error return through flush() file operation changelog v6: * clean up on error handling for better code flow and review * clean up on pr_err() for critical error only * design taking care writing block that below PAGE_SIZE * once error has occurred, design will return -EIO until file close * document design expectations/scenarios in the code * change the dynamic allocation cap_info struct to statically allocated changelog v5: * changed to new design without leveraging firmware_class API * use misc_char device interface instead of sysfs * error return through file Write() function call Kweh, Hock Leong (1): efi: a misc char interface for user to update efi firmware drivers/firmware/efi/Kconfig | 10 drivers/firmware/efi/Makefile |1 drivers/firmware/efi/capsule-loader.c | 355 + drivers/firmware/efi/capsule.c|1 4 files changed, 367 insertions(+) create mode 100644 drivers/firmware/efi/capsule-loader.c -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v9 1/1] efi: a misc char interface for user to update efi firmware
> -Original Message-
> From: Borislav Petkov [mailto:b...@alien8.de]
> Sent: Wednesday, December 16, 2015 7:26 PM
> To: Kweh, Hock Leong
> Cc: Matt Fleming; Greg Kroah-Hartman; Ong, Boon Leong; LKML; linux-
> e...@vger.kernel.org; Sam Protsenko; Peter Jones; Andy Lutomirski; Roy
> Franz; James Bottomley; Linux FS Devel; Anvin, H Peter; 'Matt Fleming'
> Subject: Re: [PATCH v9 1/1] efi: a misc char interface for user to update efi
> firmware
>
> On Wed, Dec 16, 2015 at 11:09:50AM +, Kweh, Hock Leong wrote:
> > So, my conclusion is that this module is not able to be tested on QEMU
> > environment.
>
> That's not the point.
>
> The module should better handle writing to the device file gracefully
> and not explode. Regardless of whether it is running on an EFI system or
> not.
>
> efi_capsule_loader_init() simply loads the driver on *any* system,
> even a !UEFI one. And when I write some garbage to the device file, it
> explodes.
>
> What it should do instead is check whether it is being loaded on en EFI
> system and whether all it needs to function properly is initialized
> already, like runtime services. If not, it should refuse to load.
>
> --
> Regards/Gruss,
> Boris.
Hi Borislav,
I catch your point now. I will fix that in v10 patch.
Thanks & Regards,
Wilson
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v9 1/1] efi: a misc char interface for user to update efi firmware
> -Original Message-
> From: Borislav Petkov [mailto:b...@alien8.de]
> Sent: Wednesday, November 04, 2015 4:00 AM
>
> On Mon, Nov 02, 2015 at 06:47:29AM +0000, Kweh, Hock Leong wrote:
> > By looking at your dmesg log, the above print out message seem that
> > someone has called the flush() after the write(2). In my environment,
> flush()
> > only being called in 2 places which are before write(2) and during close(2).
> > The dmesg log seems that your environment is running write(2) and flush()
> in
> > different threads and are parallel. Could you help me to double confirm this
> and it
> > would be good if you could told me when the flush() is exactly being called
> in
> > your environment. The info really help me on debugging.
>
> I don't know what you mean: I simply do
>
> cat /bin/ls > /dev/efi_capsule_loader
>
> as root in an SMP kvm guest. And it explodes. Nothing special, just this
> one command.
>
> I guess you could try to reproduce it, here's how I start it:
>
> qemu-system-x86_64
> -enable-kvm
> -gdb tcp::1234
> -cpu Opteron_G5
> -m 2048
> -hda /home/boris/kvm/debian/sid-x86_64.img
> -hdb /home/boris/kvm/swap.img
> -boot menu=off,order=c
> -localtime
> -net nic,model=rtl8139
> -net user,hostfwd=tcp::1235-:22
> -usbdevice tablet
> -kernel /home/boris/kernel/linux-2.6/arch/x86/boot/bzImage
> -append "root=/dev/sda1 resume=/dev/sdb1 debug ignore_loglevel
> log_buf_len=16M earlyprintk=ttyS0,115200 console=ttyS0,115200
> console=tty0"
> -monitor pty
> -virtfs local,path=/tmp,mount_tag=tmp,security_model=none
> -serial file:/home/boris/kvm/test-x86_64-1235.log
> -snapshot
> -smp 8
>
> HTH.
>
> --
> Regards/Gruss,
> Boris.
Hi Borislav,
Finally able to free up 25GB space to setup a QEMU VM with Debian v8.2.0
system and look into this issue. Located the NULL pointer happened at code line:
status = efi.query_capsule_caps(&capsule, 1, &max_size, reset);
which is inside function efi_capsule_supported(). This function call is
initialized by
EFI Firmware run-time service table. So, I believe the QEMU do not emulate the
EFI Firmware run-time service API calls. This is why when come to this line it
hit
the NULL pointer issue.
So, my conclusion is that this module is not able to be tested on QEMU
environment.
Thanks & Regards,
Wilson
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v9 1/1] efi: a misc char interface for user to update efi firmware
> -Original Message- > From: Borislav Petkov [mailto:b...@alien8.de] > Sent: Wednesday, November 04, 2015 4:15 AM > > On Mon, Nov 02, 2015 at 07:17:28AM +0000, Kweh, Hock Leong wrote: > > This is not a return value to indicate what is going now. It is a flag > > used in "cap_info->index" which positive value has a meaning of index > > number. I am using the negative value for the flag which similar to > > the implementation of pointer & error pointer (ERR_PTR). > > Ok, but that doesn't make any sense: you're assigning UPLOAD_DONE to > cap_info->index only once in efi_capsule_submit_update() and you're not > testing it anywhere. Yeah, yeah, you're implicitly testing for it by > doing the "< 0" check. > > So simply assign -1 to ->index to mean *any* type of error occurred, > remove the defines and you can always test for "< 0" to mean "did > something fail". > > You simply don't need two error values... > Ok. Noted. Thanks & Regards, Wilson
RE: [PATCH v9 1/1] efi: a misc char interface for user to update efi firmware
> -Original Message-
> From: Borislav Petkov [mailto:b...@alien8.de]
> Sent: Sunday, November 01, 2015 8:59 PM
>
> On Sun, Nov 01, 2015 at 11:11:23AM +0000, Kweh, Hock Leong wrote:
> > Hmm If I combine these 2 flags to become one as
> > "NO_MORE_WRITE_ACTION" to better describing the situation, you Okay
> > with it?
>
> I don't understand, why combine?
>
> Why not simply make UPLOAD_DONE a positive value:
>
> #define UPLOAD_DONE 1
> #define ERR_OCCURRED -1
>
> 0 would obviously mean, no errors occurred whatsoever.
>
Hi Boris,
This is not a return value to indicate what is going now. It is a flag used in
"cap_info->index" which positive value has a meaning of index number.
I am using the negative value for the flag which similar to the implementation
of pointer & error pointer (ERR_PTR).
Thanks & Regards,
Wilson
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v9 1/1] efi: a misc char interface for user to update efi firmware
> -Original Message- > From: Borislav Petkov [mailto:b...@alien8.de] > Sent: Sunday, November 01, 2015 6:30 PM > > > > Example method to load the capsule binary: > > cat firmware.bin > /dev/efi_capsule_loader > > $ cat "some_dumb_file" > /dev/efi_capsule_loader > Killed > > and in dmesg: > > [ 34.033982] efi_capsule_loader: efi_capsule_flush: capsule upload not > complete Hi Boris, I have tested "cat /bin/ls > /dev/efi_capsule_loader" in my environment, but I am not able to reproduce the issue. So, it is a bit hard for me to debug the issue with my environment and may need your help on this. By looking at your dmesg log, the above print out message seem that someone has called the flush() after the write(2). In my environment, flush() only being called in 2 places which are before write(2) and during close(2). The dmesg log seems that your environment is running write(2) and flush() in different threads and are parallel. Could you help me to double confirm this and it would be good if you could told me when the flush() is exactly being called in your environment. The info really help me on debugging. Thanks & Regards, Wilson > [ 58.765683] [ cut here ] > [ 58.769349] WARNING: CPU: 5 PID: 3904 at > drivers/firmware/efi/capsule.c:83 efi_capsule_supported+0x103/0x150() > [ 58.775063] Modules linked in: > [ 58.776474] CPU: 5 PID: 3904 Comm: cat Not tainted 4.3.0-rc7+ #3 > [ 58.779044] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > 1.7.5-20140531_083030-gandalf 04/01/2014 > [ 58.783387] 81957aa0 880079793d78 812cb2ea > > [ 58.786749] 880079793db0 81055981 00010102464c457f > > [ 58.790140] 00401e3b 0001 880078660704 > 880079793dc0 > [ 58.793353] Call Trace: > [ 58.794343] [] dump_stack+0x4e/0x84 > [ 58.796416] [] warn_slowpath_common+0x91/0xd0 > [ 58.798773] [] warn_slowpath_null+0x1a/0x20 > [ 58.800962] [] efi_capsule_supported+0x103/0x150 > [ 58.803292] [] efi_capsule_write+0x269/0x390 > [ 58.805563] [] __vfs_write+0x28/0xe0 > [ 58.807591] [] ? __vfs_read+0xaa/0xe0 > [ 58.809612] [] vfs_write+0xb5/0x1a0 > [ 58.811272] [] ? __fget_light+0x6e/0x90 > [ 58.813073] [] SyS_write+0x52/0xc0 > [ 58.814720] [] entry_SYSCALL_64_fastpath+0x16/0x73 > [ 58.816665] ---[ end trace 94c0c141f9b0ec01 ]--- > [ 58.818179] BUG: unable to handle kernel NULL pointer dereference at > (null) > [ 58.820427] IP: [< (null)>] (null) > [ 58.820630] PGD 79af8067 PUD 79781067 PMD 0 > [ 58.820630] Oops: 0010 [#1] PREEMPT SMP > [ 58.820630] Modules linked in: > [ 58.820630] CPU: 5 PID: 3904 Comm: cat Tainted: GW > 4.3.0-rc7+ #3 > [ 58.820630] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS > 1.7.5-20140531_083030-gandalf 04/01/2014 > [ 58.820630] task: 8800771417c0 ti: 88007979 task.ti: > 88007979 > [ 58.820630] RIP: 0010:[<>] [< (null)>] > (null) > [ 58.820630] RSP: 0018:880079793dc8 EFLAGS: 00010282 > [ 58.820630] RAX: 88007a01b4e0 RBX: 00010102464c457f RCX: > 880078660704 > [ 58.820630] RDX: 880079793dd8 RSI: 0001 RDI: > 880079793dd0 > [ 58.820630] RBP: 880079793e08 R08: R09: > > [ 58.820630] R10: R11: 0001 R12: > > [ 58.820630] R13: 00401e3b R14: 0001 R15: > 880078660704 > [ 58.820630] FS: 77fe1700() GS:88007c00() > knlGS: > [ 58.820630] CS: 0010 DS: ES: CR0: 8005003b > [ 58.820630] CR2: CR3: 7ab9 CR4: > 000406e0 > [ 58.820630] Stack: > [ 58.820630] 8157ae24 88007a01b4e0 0002 > 880078660700 > [ 58.820630] 88007706 1000 ea0001dc1800 > 88007706 > [ 58.820630] 880079793e48 8157d559 0402 > 8800799cbc00 > [ 58.820630] Call Trace: > [ 58.820630] [] ? efi_capsule_supported+0x94/0x150 > [ 58.820630] [] efi_capsule_write+0x269/0x390 > [ 58.820630] [] __vfs_write+0x28/0xe0 > [ 58.820630] [] ? __vfs_read+0xaa/0xe0 > [ 58.820630] [] vfs_write+0xb5/0x1a0 > [ 58.820630] [] ? __fget_light+0x6e/0x90 > [ 58.820630] [] SyS_write+0x52/0xc0 > [ 58.820630] [] entry_SYSCALL_64_fastpath+0x16/0x73 > [ 58.820630] Code: Bad RIP value. > [ 58.820630] RIP [< (null)>] (null) > [ 58.820630] RSP > [ 58.820630] CR2: > [ 58.876221] ---[ end trace 94c0c141f9b0ec02 ]--- >
RE: [PATCH v9 1/1] efi: a misc char interface for user to update efi firmware
> -Original Message-
> From: Borislav Petkov [mailto:b...@alien8.de]
> Sent: Sunday, November 01, 2015 6:58 PM
>
> On Sun, Nov 01, 2015 at 10:52:52AM +0000, Kweh, Hock Leong wrote:
> > Could you share me your dumb file? I did perform negative test, but I did
> > not get these dump stack in dmesg. Thanks.
>
> I think almost any file works:
>
> cat /bin/ls > /dev/efi_capsule_loader
Ok. Will try this out.
>
> > > > +#define UPLOAD_DONE -1
> > >
> > > Isn't the fact that upload was finished a success message? If so, why is
> > > it a
> > > negative value?
> >
> > This is to indicate an upload is done and pending for close(2). If a
> subsequence
> > write(2) perform, return error. Comments inputted by Matt and Andy.
>
> But in that case you can return ERR_OCCURRED. UPLOAD_DONE still doesn't
> look like a negative value to me as it signals that the upload was done
> and thus successful as no errors happened during the upload.
>
Hmm If I combine these 2 flags to become one as "NO_MORE_WRITE_ACTION"
to better describing the situation, you Okay with it?
Regards,
Wilson
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v9 1/1] efi: a misc char interface for user to update efi firmware
> -Original Message-
> From: Borislav Petkov [mailto:b...@alien8.de]
> Sent: Sunday, November 01, 2015 6:30 PM
> >
> > Example method to load the capsule binary:
> > cat firmware.bin > /dev/efi_capsule_loader
>
> $ cat "some_dumb_file" > /dev/efi_capsule_loader Killed
>
> and in dmesg:
>
> [ 34.033982] efi_capsule_loader: efi_capsule_flush: capsule upload not
> complete
> [ 58.765683] [ cut here ]
> [ 58.769349] WARNING: CPU: 5 PID: 3904 at
> drivers/firmware/efi/capsule.c:83 efi_capsule_supported+0x103/0x150()
> [ 58.775063] Modules linked in:
> [ 58.776474] CPU: 5 PID: 3904 Comm: cat Not tainted 4.3.0-rc7+ #3
> [ 58.779044] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> 1.7.5-20140531_083030-gandalf 04/01/2014
> [ 58.783387] 81957aa0 880079793d78 812cb2ea
>
> [ 58.786749] 880079793db0 81055981 00010102464c457f
>
> [ 58.790140] 00401e3b 0001 880078660704
> 880079793dc0
> [ 58.793353] Call Trace:
> [ 58.794343] [] dump_stack+0x4e/0x84
> [ 58.796416] [] warn_slowpath_common+0x91/0xd0
> [ 58.798773] [] warn_slowpath_null+0x1a/0x20
> [ 58.800962] [] efi_capsule_supported+0x103/0x150
> [ 58.803292] [] efi_capsule_write+0x269/0x390
> [ 58.805563] [] __vfs_write+0x28/0xe0
> [ 58.807591] [] ? __vfs_read+0xaa/0xe0
> [ 58.809612] [] vfs_write+0xb5/0x1a0
> [ 58.811272] [] ? __fget_light+0x6e/0x90
> [ 58.813073] [] SyS_write+0x52/0xc0
> [ 58.814720] [] entry_SYSCALL_64_fastpath+0x16/0x73
> [ 58.816665] ---[ end trace 94c0c141f9b0ec01 ]---
> [ 58.818179] BUG: unable to handle kernel NULL pointer dereference at
> (null)
> [ 58.820427] IP: [< (null)>] (null)
> [ 58.820630] PGD 79af8067 PUD 79781067 PMD 0
> [ 58.820630] Oops: 0010 [#1] PREEMPT SMP
> [ 58.820630] Modules linked in:
> [ 58.820630] CPU: 5 PID: 3904 Comm: cat Tainted: GW
> 4.3.0-rc7+ #3
> [ 58.820630] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
> 1.7.5-20140531_083030-gandalf 04/01/2014
> [ 58.820630] task: 8800771417c0 ti: 88007979 task.ti:
> 88007979
> [ 58.820630] RIP: 0010:[<>] [< (null)>]
> (null)
> [ 58.820630] RSP: 0018:880079793dc8 EFLAGS: 00010282
> [ 58.820630] RAX: 88007a01b4e0 RBX: 00010102464c457f RCX:
> 880078660704
> [ 58.820630] RDX: 880079793dd8 RSI: 0001 RDI:
> 880079793dd0
> [ 58.820630] RBP: 880079793e08 R08: R09:
>
> [ 58.820630] R10: R11: 0001 R12:
>
> [ 58.820630] R13: 00401e3b R14: 0001 R15:
> 880078660704
> [ 58.820630] FS: 77fe1700() GS:88007c00()
> knlGS:
> [ 58.820630] CS: 0010 DS: ES: CR0: 8005003b
> [ 58.820630] CR2: CR3: 7ab9 CR4:
> 000406e0
> [ 58.820630] Stack:
> [ 58.820630] 8157ae24 88007a01b4e0 0002
> 880078660700
> [ 58.820630] 88007706 1000 ea0001dc1800
> 88007706
> [ 58.820630] 880079793e48 8157d559 0402
> 8800799cbc00
> [ 58.820630] Call Trace:
> [ 58.820630] [] ? efi_capsule_supported+0x94/0x150
> [ 58.820630] [] efi_capsule_write+0x269/0x390
> [ 58.820630] [] __vfs_write+0x28/0xe0
> [ 58.820630] [] ? __vfs_read+0xaa/0xe0
> [ 58.820630] [] vfs_write+0xb5/0x1a0
> [ 58.820630] [] ? __fget_light+0x6e/0x90
> [ 58.820630] [] SyS_write+0x52/0xc0
> [ 58.820630] [] entry_SYSCALL_64_fastpath+0x16/0x73
> [ 58.820630] Code: Bad RIP value.
> [ 58.820630] RIP [< (null)>] (null)
> [ 58.820630] RSP
> [ 58.820630] CR2:
> [ 58.876221] ---[ end trace 94c0c141f9b0ec02 ]---
>
Could you share me your dumb file? I did perform negative test, but I did
not get these dump stack in dmesg. Thanks.
>
> Please integrate checkpatch into your workflow - it can be helpful
> sometimes:
>
> WARNING: 'OCCURED' may be misspelled - perhaps 'OCCURRED'?
> #114: FILE: drivers/firmware/efi/efi-capsule-loader.c:22:
> +#define ERR_OCCURED -2
>
> WARNING: 'OCCURED' may be misspelled - perhaps 'OCCURRED'?
> #132: FILE: drivers/firmware/efi/efi-capsule-loader.c:40:
> + * Besides freeing the buffer pages, it also flagged an ERR_OCCURED
>
> WARNING: 'OCCURED' may be misspelled - perhaps 'OCCURRED'?
> #144: FILE: drivers/firmware/efi/efi-capsule-loader.c:52:
> + cap_info->index = ERR_OCCURED;
>
> WARNING: Possible unnecessary 'out of memory' message
> #399: FILE: drivers/firmware/efi/efi-capsule-loader.c:307:
> + if (!cap_info) {
> + pr_debug("%s: kzalloc() failed\n", __func__);
>
I did use checkpatch. May be my version is different. Will get the
latest version and run it again. Than
[PATCH v9 1/1] efi: a misc char interface for user to update efi firmware
From: "Kweh, Hock Leong"
Introducing a kernel module to expose capsule loader interface
(misc char device file note) for user to upload capsule binaries.
Example method to load the capsule binary:
cat firmware.bin > /dev/efi_capsule_loader
This patch also export efi_capsule_supported() function symbol for
verifying the submitted capsule header in this kernel module.
Cc: Matt Fleming
Signed-off-by: Kweh, Hock Leong
---
drivers/firmware/efi/Kconfig | 10
drivers/firmware/efi/Makefile |1
drivers/firmware/efi/capsule.c|1
drivers/firmware/efi/efi-capsule-loader.c | 356 +
4 files changed, 368 insertions(+)
create mode 100644 drivers/firmware/efi/efi-capsule-loader.c
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index f712d47..0be8ee3 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -60,6 +60,16 @@ config EFI_RUNTIME_WRAPPERS
config EFI_ARMSTUB
bool
+config EFI_CAPSULE_LOADER
+ tristate "EFI capsule loader"
+ depends on EFI
+ help
+ This option exposes a loader interface "/dev/efi_capsule_loader" for
+ user to load EFI capsule binary and update the EFI firmware through
+ system reboot.
+
+ If unsure, say N.
+
endmenu
config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 698846e..5ab031a 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER) += cper.o
obj-$(CONFIG_EFI_RUNTIME_MAP) += runtime-map.o
obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o
obj-$(CONFIG_EFI_STUB) += libstub/
+obj-$(CONFIG_EFI_CAPSULE_LOADER) += efi-capsule-loader.o
diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c
index d8cd75c0..738d437 100644
--- a/drivers/firmware/efi/capsule.c
+++ b/drivers/firmware/efi/capsule.c
@@ -101,6 +101,7 @@ out:
kfree(capsule);
return rv;
}
+EXPORT_SYMBOL_GPL(efi_capsule_supported);
/**
* efi_capsule_update - send a capsule to the firmware
diff --git a/drivers/firmware/efi/efi-capsule-loader.c
b/drivers/firmware/efi/efi-capsule-loader.c
new file mode 100644
index 000..23f7618
--- /dev/null
+++ b/drivers/firmware/efi/efi-capsule-loader.c
@@ -0,0 +1,356 @@
+/*
+ * EFI capsule loader driver.
+ *
+ * Copyright 2015 Intel Corporation
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DEV_NAME "efi_capsule_loader"
+#define UPLOAD_DONE -1
+#define ERR_OCCURED -2
+
+struct capsule_info {
+ boolheader_obtained;
+ int reset_type;
+ longindex;
+ size_t count;
+ size_t total_size;
+ struct page **pages;
+ size_t page_bytes_remain;
+};
+
+static DEFINE_MUTEX(capsule_loader_lock);
+
+/**
+ * efi_free_all_buff_pages - free all previous allocated buffer pages
+ * @cap_info: pointer to current instance of capsule_info structure
+ *
+ * Besides freeing the buffer pages, it also flagged an ERR_OCCURED
+ * flag for indicating to the next write entries that there is a flaw
+ * happened and any subsequence actions are not valid unless close(2).
+ **/
+static void efi_free_all_buff_pages(struct capsule_info *cap_info)
+{
+ while (cap_info->index > 0)
+ __free_page(cap_info->pages[--cap_info->index]);
+
+ kfree(cap_info->pages);
+
+ /* Indicate to the next that error had occurred */
+ cap_info->index = ERR_OCCURED;
+}
+
+/**
+ * efi_capsule_setup_info - to obtain the efi capsule header in the binary and
+ * setup capsule_info structure
+ * @cap_info: pointer to current instance of capsule_info structure
+ * @kbuff: a mapped 1st page buffer pointer
+ * @hdr_bytes: the total bytes of received efi header
+ **/
+static ssize_t efi_capsule_setup_info(struct capsule_info *cap_info,
+ void *kbuff, size_t hdr_bytes)
+{
+ int ret;
+ void *temp_page;
+
+ /* Handling 1st block is less than header size */
+ if (hdr_bytes < sizeof(efi_capsule_header_t)) {
+ if (!cap_info->pages) {
+ cap_info->pages = kzalloc(sizeof(void *), GFP_KERNEL);
+ if (!cap_info->pages) {
+ pr_debug("%s: kzalloc() failed\n", __func__);
+ return -ENOMEM;
+ }
+ }
+ } else {
+ /* Reset back to the correct offset of header */
+
[PATCH v9 0/1] Enable capsule loader interface for efi firmware updating
From: "Kweh, Hock Leong" Dear maintainers & communities, This patchset is created on top of Matt's patchset: 1.)https://lkml.org/lkml/2014/10/7/390 "[PATCH 1/2] efi: Move efi_status_to_err() to efi.h" 2.)https://lkml.org/lkml/2014/10/7/391 "[PATCH 2/2] efi: Capsule update support" It expose a misc char interface for user to upload the capsule binary and calling efi_capsule_update() API to pass the binary to EFI firmware. The steps to update efi firmware are: 1.) cat firmware.cap > /dev/efi_capsule_loader 2.) reboot Any failed upload error message will be returned while doing "cat" through file operation write() function call. Tested the code with Intel Quark Galileo GEN1 platform. Thanks. --- changelog v9: * squash 2 patches to become 1 patch * change function param to pass in cap_info instead of file structure * perform both alloc inside efi_capsule_setup_info() * change to use multiple exit labels instead of one function call * further code clean up base on Matt's comments changelog v8: * further clean up on kunmap() & efi_free_all_buff_pages() * design enhanced to support 1st few writes are less than efi header size * removed support to padding capsule and flag error once the upload size bigger than header defined size changelog v7: * add successful message printed in dmesg * shorten the code in efi_capsule_write() by splitting out efi_capsule_setup_info() & efi_capsule_submit_update() functions * design added capability to support multiple file open action * re-write those comments by following standard format * design added the "uncomplete" error return through flush() file operation changelog v6: * clean up on error handling for better code flow and review * clean up on pr_err() for critical error only * design taking care writing block that below PAGE_SIZE * once error has occurred, design will return -EIO until file close * document design expectations/scenarios in the code * change the dynamic allocation cap_info struct to statically allocated changelog v5: * changed to new design without leveraging firmware_class API * use misc_char device interface instead of sysfs * error return through file Write() function call Kweh, Hock Leong (1): efi: a misc char interface for user to update efi firmware drivers/firmware/efi/Kconfig | 10 drivers/firmware/efi/Makefile |1 drivers/firmware/efi/capsule.c|1 drivers/firmware/efi/efi-capsule-loader.c | 356 + 4 files changed, 368 insertions(+) create mode 100644 drivers/firmware/efi/efi-capsule-loader.c -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v8 2/2] efi: a misc char interface for user to update efi firmware
> -Original Message- > From: Matt Fleming [mailto:m...@console-pimps.org] > Sent: Monday, October 12, 2015 8:11 PM > > > +}; > > + > > +static DEFINE_MUTEX(capsule_loader_lock); > > What does this lock protect? This lock is to protect when one of the instance calling efi_capsule_update() API and the other instance calling efi_capsule_supported() API. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v7 1/2] efi: export efi_capsule_supported() function symbol
> -Original Message- > From: Matt Fleming [mailto:m...@console-pimps.org] > Sent: Sunday, October 11, 2015 6:02 AM > > I agree that it makes sense to fold this patch into your PATCH 2, because then > we know why we need the above symbol to be exported. > Okay, I will squash that into my patch and re-submit v9. Before that, are there any comments to my v8 patchset? Thanks & Regards, Wilson -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v8 0/2] Enable capsule loader interface for efi firmware updating
> -Original Message- > From: Bryan O'Donoghue [mailto:pure.lo...@nexus-software.ie] > Sent: Wednesday, October 07, 2015 11:00 PM > > Wilson. > > Same question as at V7. If you aren't supporting the MFH on Galileo then > what capsule are you using with 0.75 BIOS ? From memory it won't work > without the MFH included ... > > -- > BOD As I mentioned before the Quark Security Header patch will not upstream to mainline and will be carried by Intel. So to test with Quark Platform, I will apply that patch into my kernel. Thanks & Regards, Wilson -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v8 1/2] efi: export efi_capsule_supported() function symbol
From: "Kweh, Hock Leong" This patch export efi_capsule_supported() function symbol for capsule kernel module to use. Cc: Matt Fleming Signed-off-by: Kweh, Hock Leong --- NOTE: This patch will be merge into Matt's [PATCH 2/2] efi: Capsule update support "https://lkml.org/lkml/2014/10/7/391";, if he is agreeing to expose the function interface. drivers/firmware/efi/capsule.c |1 1 file changed, 1 insertion(+) diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c index d8cd75c0..738d437 100644 --- a/drivers/firmware/efi/capsule.c +++ b/drivers/firmware/efi/capsule.c @@ -101,6 +101,7 @@ out: kfree(capsule); return rv; } +EXPORT_SYMBOL_GPL(efi_capsule_supported); /** * efi_capsule_update - send a capsule to the firmware -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v8 2/2] efi: a misc char interface for user to update efi firmware
From: "Kweh, Hock Leong"
Introducing a kernel module to expose capsule loader interface
(misc char device file note) for user to upload capsule binaries.
Example method to load the capsule binary:
cat firmware.bin > /dev/efi_capsule_loader
Cc: Matt Fleming
Signed-off-by: Kweh, Hock Leong
---
drivers/firmware/efi/Kconfig | 10 +
drivers/firmware/efi/Makefile |1 +
drivers/firmware/efi/efi-capsule-loader.c | 356 +
3 files changed, 367 insertions(+)
create mode 100644 drivers/firmware/efi/efi-capsule-loader.c
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index f712d47..0be8ee3 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -60,6 +60,16 @@ config EFI_RUNTIME_WRAPPERS
config EFI_ARMSTUB
bool
+config EFI_CAPSULE_LOADER
+ tristate "EFI capsule loader"
+ depends on EFI
+ help
+ This option exposes a loader interface "/dev/efi_capsule_loader" for
+ user to load EFI capsule binary and update the EFI firmware through
+ system reboot.
+
+ If unsure, say N.
+
endmenu
config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 698846e..5ab031a 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER) += cper.o
obj-$(CONFIG_EFI_RUNTIME_MAP) += runtime-map.o
obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o
obj-$(CONFIG_EFI_STUB) += libstub/
+obj-$(CONFIG_EFI_CAPSULE_LOADER) += efi-capsule-loader.o
diff --git a/drivers/firmware/efi/efi-capsule-loader.c
b/drivers/firmware/efi/efi-capsule-loader.c
new file mode 100644
index 000..b415b4e
--- /dev/null
+++ b/drivers/firmware/efi/efi-capsule-loader.c
@@ -0,0 +1,356 @@
+/*
+ * EFI capsule loader driver.
+ *
+ * Copyright 2015 Intel Corporation
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DEV_NAME "efi_capsule_loader"
+#define UPLOAD_DONE -1
+#define ERR_OCCURED -2
+
+struct capsule_info {
+ charheader_obtained;
+ int reset_type;
+ longindex;
+ unsigned long count;
+ unsigned long total_size;
+ struct page **pages;
+ unsigned long page_count_remain;
+};
+
+static DEFINE_MUTEX(capsule_loader_lock);
+
+/**
+ * efi_free_all_buff_pages - free the current & all previous allocated buffer
+ * pages
+ * @file: file pointer
+ * @kbuff: a mapped buffer pointer
+ * @kbuff_page: the current execution allocated buffer page's pointer which has
+ * not yet assigned to pages[] array
+ *
+ * The input param can be NULL if the current execution has not allocated
+ * any buffer page.
+ **/
+static void efi_free_all_buff_pages(struct file *file, void *kbuff,
+ struct page *kbuff_page)
+{
+ struct capsule_info *cap_info = file->private_data;
+
+ if (kbuff)
+ kunmap(kbuff_page);
+
+ if (kbuff_page)
+ __free_page(kbuff_page);
+
+ while (cap_info->index > 0)
+ __free_page(cap_info->pages[--cap_info->index]);
+
+ kfree(cap_info->pages);
+
+ /* indicate to the next that error had occurred */
+ cap_info->index = ERR_OCCURED;
+}
+
+/**
+ * efi_capsule_setup_info - to obtain the efi capsule header in the binary and
+ * setup capsule_info structure
+ * @file: file pointer
+ * @kbuff: a mapped 1st page buffer pointer
+ **/
+static ssize_t efi_capsule_setup_info(struct file *file, void *kbuff)
+{
+ int ret = 0;
+ struct capsule_info *cap_info = file->private_data;
+ void *temp_page;
+ /* reset back to the correct offset of header */
+ efi_capsule_header_t *cap_hdr = kbuff - cap_info->count;
+ size_t pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
+ PAGE_SHIFT;
+
+ if (pages_needed == 0) {
+ pr_err("%s: pages count invalid\n", __func__);
+ return -EINVAL;
+ }
+
+ /* check if the capsule binary supported */
+ mutex_lock(&capsule_loader_lock);
+ ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
+ cap_hdr->imagesize, &cap_info->reset_type);
+ mutex_unlock(&capsule_loader_lock);
+ if (ret) {
+ pr_err("%s: efi_capsule_supported() failed\n", __func__);
+ return ret;
+ }
+
+ cap_info->tota
[PATCH v8 0/2] Enable capsule loader interface for efi firmware updating
From: "Kweh, Hock Leong" Dear maintainers & communities, This patchset is created on top of Matt's patchset: 1.)https://lkml.org/lkml/2014/10/7/390 "[PATCH 1/2] efi: Move efi_status_to_err() to efi.h" 2.)https://lkml.org/lkml/2014/10/7/391 "[PATCH 2/2] efi: Capsule update support" It expose a misc char interface for user to upload the capsule binary and calling efi_capsule_update() API to pass the binary to EFI firmware. The steps to update efi firmware are: 1.) cat firmware.cap > /dev/efi_capsule_loader 2.) reboot Any failed upload error message will be returned while doing "cat" through file operation write() function call. Tested the code with Intel Quark Galileo GEN1 platform. Thanks. --- NOTE: If Matt agrees with this design, [PATCH v7 1/1] will be squash into his [PATCH 2/2]: https://lkml.org/lkml/2014/10/7/391 for submitting. changelog v8: * further clean up on kunmap() & efi_free_all_buff_pages() * design enhanced to support 1st few writes are less than efi header size * removed support to padding capsule and flag error once the upload size bigger than header defined size changelog v7: * add successful message printed in dmesg * shorten the code in efi_capsule_write() by splitting out efi_capsule_setup_info() & efi_capsule_submit_update() functions * design added capability to support multiple file open action * re-write those comments by following standard format * design added the "uncomplete" error return through flush() file operation changelog v6: * clean up on error handling for better code flow and review * clean up on pr_err() for critical error only * design taking care writing block that below PAGE_SIZE * once error has occurred, design will return -EIO until file close * document design expectations/scenarios in the code * change the dynamic allocation cap_info struct to statically allocated changelog v5: * changed to new design without leveraging firmware_class API * use misc_char device interface instead of sysfs * error return through file Write() function call Kweh, Hock Leong (2): efi: export efi_capsule_supported() function symbol efi: a misc char interface for user to update efi firmware drivers/firmware/efi/Kconfig | 10 drivers/firmware/efi/Makefile |1 drivers/firmware/efi/capsule.c|1 drivers/firmware/efi/efi-capsule-loader.c | 356 + 4 files changed, 368 insertions(+) create mode 100644 drivers/firmware/efi/efi-capsule-loader.c -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v7 1/2] efi: export efi_capsule_supported() function symbol
> -Original Message- > From: Bryan O'Donoghue [mailto:pure.lo...@nexus-software.ie] > Sent: Tuesday, October 06, 2015 10:54 PM > >> > >> Aside from that, I'm curious which types of capsules you've used here > >> too - does it include the MFH header ? Keep in mind the initial > >> firmware that shipped with Galileo will depend on that MFH being > present. > >> > >> > http://download.intel.com/support/processors/quark/sb/quark_secureboo > >> t > >> prm_330234_001.pdf > >> - Section A1 - table 7 ? > >> > >> So if we boot a 4.x kernel with that initial firmware version 0.75 if > >> memory serves - it's important that the capsule.c code handles the MFH. > >> > > > > Already got agreement with Matt that Quark Security Header patch will > > not be upstream to mainline as it is not a standard header. So Intel > > will carry this patch ourselves. > > Right... so what sort of capsule are you testing with ? I am testing on Intel Galileo Gen 1 with bios version v0.7.5, v0.8.0, v1.0.1 & v1.0.2. Thanks & Regards, Wilson -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v7 1/2] efi: export efi_capsule_supported() function symbol
> -Original Message- > From: Bryan O'Donoghue [mailto:pure.lo...@nexus-software.ie] > Sent: Tuesday, October 06, 2015 5:27 AM > > Wilson - trying to test this out on a Galileo Gen2 - which branch are you > doing > this against ? > > I can apply the first patch you're proposing to squash your commit into > > https://lkml.org/lkml/diff/2014/10/7/390/1 > > but then trying to apply the first in your series on top of that patch I get > > deckard@aineko:~/Development/linux$ git > apply ../patches/capsule_wilson/1_2.eml > ../patches/capsule_wilson/1_2.eml:72: trailing whitespace. > EXPORT_SYMBOL_GPL(efi_capsule_supported); > error: drivers/firmware/efi/capsule.c: No such file or directory > > https://kernel.googlesource.com/pub/scm/linux/kernel/git/mfleming/efi/+/ > capsule/drivers/firmware/efi/capsule.c > > > ?? If you are applying Matt's patch https://lkml.org/lkml/diff/2014/10/7/390/1 which had been created 1 year ago to mainline vanilla kernel (Linux 4.3-rc4), you are not able to direct patch in due to the Makefile error below: ~/MyWorks/linux_mainline$ git apply .git/rebase-apply/0001 --reject Checking patch arch/x86/kernel/reboot.c... Hunk #1 succeeded at 527 (offset 11 lines). Checking patch drivers/firmware/efi/Makefile... error: while searching for: # # Makefile for linux kernel # obj-$(CONFIG_EFI) += efi.o vars.o reboot.o obj-$(CONFIG_EFI_VARS) += efivars.o obj-$(CONFIG_EFI_VARS_PSTORE) += efi-pstore.o obj-$(CONFIG_UEFI_CPER) += cper.o error: patch failed: drivers/firmware/efi/Makefile:1 Checking patch drivers/firmware/efi/capsule.c... Checking patch drivers/firmware/efi/reboot.c... Checking patch include/linux/efi.h... Hunk #1 succeeded at 122 (offset 3 lines). Hunk #2 succeeded at 983 (offset 23 lines). Hunk #3 succeeded at 1235 (offset 23 lines). Hunk #4 succeeded at 1317 (offset 23 lines). Applied patch arch/x86/kernel/reboot.c cleanly. Applying patch drivers/firmware/efi/Makefile with 1 rejects... Rejected hunk #1. Applied patch drivers/firmware/efi/capsule.c cleanly. Applied patch drivers/firmware/efi/reboot.c cleanly. Applied patch include/linux/efi.h cleanly. You should resolve the Makefile error and then git add 5 files below: - arch/x86/kernel/reboot.c - drivers/firmware/efi/Makefile - drivers/firmware/efi/reboot.c - include/linux/efi.h - drivers/firmware/efi/capsule.c then you are able to patch in my patchset. > > If so - then why not use the interface here ? > https://kernel.googlesource.com/pub/scm/linux/kernel/git/mfleming/efi/+/ > capsule > > (Sorry I know I'm coming to this thread late) > > Aside from that, I'm curious which types of capsules you've used here too - > does it include the MFH header ? Keep in mind the initial firmware that > shipped with Galileo will depend on that MFH being present. > > http://download.intel.com/support/processors/quark/sb/quark_secureboot > prm_330234_001.pdf > - Section A1 - table 7 ? > > So if we boot a 4.x kernel with that initial firmware version 0.75 if memory > serves - it's important that the capsule.c code handles the MFH. > Already got agreement with Matt that Quark Security Header patch will not be upstream to mainline as it is not a standard header. So Intel will carry this patch ourselves. Thanks & Regards, Wilson
RE: [PATCH v6 2/2] efi: a misc char interface for user to update efi firmware
> -Original Message-
> From: Andy Lutomirski [mailto:l...@amacapital.net]
> Sent: Tuesday, October 06, 2015 3:06 AM
>
> >> And what if cap_hdr isn't written yet?
> >
> > This design mainly targeting a simplest interface that user could
> > upload efi capsule in a single command action: cat capsule.bin >
> > /dev/efi_capsule_loader
> >
> > So, it is expected that efi capsule header is at the starting of the binary
> > file.
> > Already capture this into efi_capsule_write() comment in v7 patchset:
> > https://lkml.org/lkml/2015/10/5/232
> >
> > If you want to enhance this module to support creating efi capsule
> > header for your binary, strongly believe this design can cater the
> > implementation such as adding ioctl to pass in efi guid, flags and so on
> parameters to create the header.
> >
>
> No, that's not what I mean. What I mean is: what if cat writes too little in
> the
> first write call (e.g. 3 bytes).
Yes, I could add a condition checking for this:
if (write_byte < sizeof(efi_capsule_header_t) { ... }
to ensure the 1st block count does not less than the capsule header size.
If not, will return error.
Do you have any idea that in what kind of situation user app will pass in less
than
28 bytes each time?
>
> >
> >>
> >> > + if (ret) {
> >> > + pr_err("%s: efi_capsule_supported() failed\n",
> >> > + __func__);
> >> > + kunmap(kbuff_page);
> >> > + efi_free_all_buff_pages(kbuff_page);
> >> > + return ret;
> >> > + }
> >> > +
> >> > + cap_info.total_size = cap_hdr->imagesize;
> >> > + cap_info.pages = kmalloc_array(pages_needed, sizeof(void
> >> > *),
> >> > + GFP_KERNEL);
> >> > + if (!cap_info.pages) {
> >> > + pr_debug("%s: kmalloc_array() failed\n",
> >> > __func__);
> >> > + kunmap(kbuff_page);
> >> > + efi_free_all_buff_pages(kbuff_page);
> >> > + return -ENOMEM;
> >> > + }
> >> > +
> >> > + cap_info.header_obtained = 1;
> >>
> >> I don't see how you know that the header is obtained.
> >
> > Capsule header is at the starting block of image binary. We can obtain
> > the header through the 1st block of write action.
>
> That's quite an assumption to make.
Answered as above.
>
> >> > + cap_info.pages[cap_info.index++] = kbuff_page;
> >>
> >> Huh? You might now have allocated a whole page.
> >
> > Yes, the efi capsule header does tell the whole image size.
>
> So what? Did you allocate a page in this particular write call? If so, then
> cap_info.index++, etc is okay. If not, it's wrong.
Yes, the allocation is at:
cap_info.pages = kmalloc_array(pages_needed, sizeof(void *),
GFP_KERNEL);
before line:
cap_info.header_obtained = 1;
>
> >> > + }
> >> > + /* indicate capsule binary uploading is done */
> >> > + cap_info.index = -1;
> >>
> >> Should count > cap_info.total_size be an error?
> >>
> >> --Andy
> >
> > Yes, this is why after the write count already reaches the image size
> > stated in efi capsule header, an indicator will be flagged for
> > subsequence write to be returned -EIO as what Matt has commented.
>
> What if *this very same write* writes too much data?
>
I think it is still okay as the data is still within a page and this could
cater the image
binary that padding to page size. Whatever next write that more than the current
page, will return error -EIO.
If you think that should flag an error, I could simply add the condition
checking
to it.
Thanks & Regards,
Wilson
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v6 2/2] efi: a misc char interface for user to update efi firmware
> -Original Message-
> From: Andy Lutomirski [mailto:l...@amacapital.net]
> Sent: Sunday, October 04, 2015 7:16 AM
> > +
> > + /* setup capsule binary info structure */
> > + if (cap_info.header_obtained == 0 && cap_info.index == 0) {
> > + efi_capsule_header_t *cap_hdr = kbuff;
> > + int reset_type;
> > + size_t pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE)
> > >>
> > + PAGE_SHIFT;
> > +
> > + if (pages_needed == 0) {
> > + pr_err("%s: pages count invalid\n", __func__);
> > + kunmap(kbuff_page);
> > + efi_free_all_buff_pages(kbuff_page);
> > + return -EINVAL;
> > + }
> > +
> > + /* check if the capsule binary supported */
> > + ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
> > + cap_hdr->imagesize,
> > + &reset_type);
>
> And what if cap_hdr isn't written yet?
This design mainly targeting a simplest interface that user could upload efi
capsule in a single command action: cat capsule.bin > /dev/efi_capsule_loader
So, it is expected that efi capsule header is at the starting of the binary
file.
Already capture this into efi_capsule_write() comment in v7 patchset:
https://lkml.org/lkml/2015/10/5/232
If you want to enhance this module to support creating efi capsule header for
your binary, strongly believe this design can cater the implementation such as
adding ioctl to pass in efi guid, flags and so on parameters to create the
header.
>
> > + if (ret) {
> > + pr_err("%s: efi_capsule_supported() failed\n",
> > + __func__);
> > + kunmap(kbuff_page);
> > + efi_free_all_buff_pages(kbuff_page);
> > + return ret;
> > + }
> > +
> > + cap_info.total_size = cap_hdr->imagesize;
> > + cap_info.pages = kmalloc_array(pages_needed, sizeof(void *),
> > + GFP_KERNEL);
> > + if (!cap_info.pages) {
> > + pr_debug("%s: kmalloc_array() failed\n", __func__);
> > + kunmap(kbuff_page);
> > + efi_free_all_buff_pages(kbuff_page);
> > + return -ENOMEM;
> > + }
> > +
> > + cap_info.header_obtained = 1;
>
> I don't see how you know that the header is obtained.
Capsule header is at the starting block of image binary. We can
obtain the header through the 1st block of write action. So,
user app is expected to upload the image binary sequentially.
Also captured this into efi_capsule_write() comment in v7 patchset:
https://lkml.org/lkml/2015/10/5/232
>
> > + }
> > +
> > + cap_info.pages[cap_info.index++] = kbuff_page;
>
> Huh? You might now have allocated a whole page.
Yes, the efi capsule header does tell the whole image size.
>
> > + cap_info.count += write_byte;
> > + kunmap(kbuff_page);
> > +
> > + /* submit the full binary to efi_capsule_update() API */
> > + if (cap_info.count >= cap_info.total_size) {
> > + void *cap_hdr_temp;
> > +
> > + cap_hdr_temp = kmap(cap_info.pages[0]);
> > + if (!cap_hdr_temp) {
> > + pr_debug("%s: kmap() failed\n", __func__);
> > + efi_free_all_buff_pages(NULL);
> > + return -EFAULT;
> > + }
> > + ret = efi_capsule_update(cap_hdr_temp, cap_info.pages);
> > + kunmap(cap_info.pages[0]);
> > + if (ret) {
> > + pr_err("%s: efi_capsule_update() failed\n",
> > __func__);
> > + efi_free_all_buff_pages(NULL);
> > + return ret;
> > + }
> > + /* indicate capsule binary uploading is done */
> > + cap_info.index = -1;
>
> Should count > cap_info.total_size be an error?
>
> --Andy
Yes, this is why after the write count already reaches the image size stated in
efi capsule header, an indicator will be flagged for subsequence write to be
returned -EIO as what Matt has commented.
Thanks & Regards,
Wilson
RE: [PATCH v6 0/2] Enable capsule loader interface for efi firmware updating
> -Original Message- > From: Borislav Petkov [mailto:b...@alien8.de] > Sent: Saturday, October 03, 2015 5:06 PM > On Sat, Oct 03, 2015 at 03:18:41AM +, Kweh, Hock Leong wrote: > > > What does the error case look like? A standard glibc message about > > > write(2) failing? > > > > > > > Any upload fail error like -ENOMEM, -EINVAL, -EIO as well as error > > returned by efi_capsule_update() API. > > All I'm asking is, how does the user know that the upload didn't succeed? > I think it should depend on user app about which API they are using. If they are using syscall then errors would be returned through write(2). If they are using libc APIs fwrite, fputs and fprintf, then the errors would return through those APIs. However, this design is targeting the simple upload action "cat capsule.bin > /dev/efi_capsule_loader", so the errors should be returned through cat() or I/O redirection mechanism from shell terminal. Am I answered your question? Btw, I have an out topic question: I do notice you guys wrote in the message that a function look like write(2) or close(2). What actually the "2" mean there? Thanks & regards, Wilson
RE: [PATCH v7 1/2] efi: export efi_capsule_supported() function symbol
> -Original Message- > From: Borislav Petkov [mailto:b...@alien8.de] > Sent: Monday, October 05, 2015 9:14 PM > > So this one is still a separate patch. > > If you're going to ignore review comments, maybe I should stop wasting my > time reviewing your stuff... > > -- > Regards/Gruss, > Boris. Already follow what you have suggested, put a note under --- line: https://lkml.org/lkml/2015/10/5/230 (at line 25 - 27) Thanks for the review comments. Regards, Wilson
[PATCH v7 1/2] efi: export efi_capsule_supported() function symbol
From: "Kweh, Hock Leong" This patch export efi_capsule_supported() function symbol for capsule kernel module to use. Cc: Matt Fleming Signed-off-by: Kweh, Hock Leong --- drivers/firmware/efi/capsule.c |1 + 1 file changed, 1 insertion(+) diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c index d8cd75c0..738d437 100644 --- a/drivers/firmware/efi/capsule.c +++ b/drivers/firmware/efi/capsule.c @@ -101,6 +101,7 @@ out: kfree(capsule); return rv; } +EXPORT_SYMBOL_GPL(efi_capsule_supported); /** * efi_capsule_update - send a capsule to the firmware -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v7 2/2] efi: a misc char interface for user to update efi firmware
From: "Kweh, Hock Leong"
Introducing a kernel module to expose capsule loader interface
(misc char device file note) for user to upload capsule binaries.
Example method to load the capsule binary:
cat firmware.bin > /dev/efi_capsule_loader
Cc: Matt Fleming
Signed-off-by: Kweh, Hock Leong
---
drivers/firmware/efi/Kconfig | 10 +
drivers/firmware/efi/Makefile |1 +
drivers/firmware/efi/efi-capsule-loader.c | 336 +
3 files changed, 347 insertions(+)
create mode 100644 drivers/firmware/efi/efi-capsule-loader.c
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index f712d47..0be8ee3 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -60,6 +60,16 @@ config EFI_RUNTIME_WRAPPERS
config EFI_ARMSTUB
bool
+config EFI_CAPSULE_LOADER
+ tristate "EFI capsule loader"
+ depends on EFI
+ help
+ This option exposes a loader interface "/dev/efi_capsule_loader" for
+ user to load EFI capsule binary and update the EFI firmware through
+ system reboot.
+
+ If unsure, say N.
+
endmenu
config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 698846e..5ab031a 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER) += cper.o
obj-$(CONFIG_EFI_RUNTIME_MAP) += runtime-map.o
obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o
obj-$(CONFIG_EFI_STUB) += libstub/
+obj-$(CONFIG_EFI_CAPSULE_LOADER) += efi-capsule-loader.o
diff --git a/drivers/firmware/efi/efi-capsule-loader.c
b/drivers/firmware/efi/efi-capsule-loader.c
new file mode 100644
index 000..9c91658
--- /dev/null
+++ b/drivers/firmware/efi/efi-capsule-loader.c
@@ -0,0 +1,336 @@
+/*
+ * EFI capsule loader driver.
+ *
+ * Copyright 2015 Intel Corporation
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DEV_NAME "efi_capsule_loader"
+#define UPLOAD_DONE -1
+#define ERR_OCCURED -2
+
+struct capsule_info {
+ charheader_obtained;
+ int reset_type;
+ longindex;
+ unsigned long count;
+ unsigned long total_size;
+ struct page **pages;
+ unsigned long page_count_remain;
+};
+
+static DEFINE_MUTEX(capsule_loader_lock);
+
+/**
+ * efi_free_all_buff_pages - free the current & all previous allocated buffer
+ * pages
+ * @file: file pointer
+ * @kbuff_page: the current execution allocated buffer page's pointer which has
+ * not yet assigned to pages[] array
+ *
+ * The input param can be NULL if the current execution has not allocated
+ * any buffer page.
+ **/
+static void efi_free_all_buff_pages(struct file *file, struct page *kbuff_page)
+{
+ struct capsule_info *cap_info = file->private_data;
+
+ if (kbuff_page)
+ __free_page(kbuff_page);
+
+ if (cap_info->index > 0)
+ while (cap_info->index > 0)
+ __free_page(cap_info->pages[--cap_info->index]);
+
+ if (cap_info->header_obtained)
+ kfree(cap_info->pages);
+
+ /* indicate to the next that error had occurred */
+ cap_info->index = ERR_OCCURED;
+}
+
+/**
+ * efi_capsule_setup_info - to obtain the efi capsule header in the binary and
+ * setup capsule_info structure
+ * @file: file pointer
+ * @kbuff: a mapped 1st page buffer pointer
+ **/
+static ssize_t efi_capsule_setup_info(struct file *file, void *kbuff)
+{
+ int ret = 0;
+ struct capsule_info *cap_info = file->private_data;
+ efi_capsule_header_t *cap_hdr = kbuff;
+ size_t pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
+ PAGE_SHIFT;
+
+ if (pages_needed == 0) {
+ pr_err("%s: pages count invalid\n", __func__);
+ return -EINVAL;
+ }
+
+ /* check if the capsule binary supported */
+ mutex_lock(&capsule_loader_lock);
+ ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
+ cap_hdr->imagesize, &cap_info->reset_type);
+ mutex_unlock(&capsule_loader_lock);
+ if (ret) {
+ pr_err("%s: efi_capsule_supported() failed\n", __func__);
+ return ret;
+ }
+
+ cap_info->total_size = cap_hdr->imagesize;
+ cap_info->pages = kmalloc_array(pages_needed, sizeof(void *),
+
[PATCH v7 0/2] Enable capsule loader interface for efi firmware updating
From: "Kweh, Hock Leong" Dear maintainers & communities, This patchset is created on top of Matt's patchset: 1.)https://lkml.org/lkml/2014/10/7/390 "[PATCH 1/2] efi: Move efi_status_to_err() to efi.h" 2.)https://lkml.org/lkml/2014/10/7/391 "[PATCH 2/2] efi: Capsule update support" It expose a misc char interface for user to upload the capsule binary and calling efi_capsule_update() API to pass the binary to EFI firmware. The steps to update efi firmware are: 1.) cat firmware.cap > /dev/efi_capsule_loader 2.) reboot Any failed upload error message will be returned while doing "cat" through Write() function call. Tested the code with Intel Quark Galileo platform. Thanks. --- NOTE: If Matt agrees with this design, [PATCH v7 1/1] will be squash into his [PATCH 2/2]: https://lkml.org/lkml/2014/10/7/391 for submitting changelog v7: * add successful message printed in dmesg * shorten the code in efi_capsule_write() by splitting out efi_capsule_setup_info() & efi_capsule_submit_update() functions * design added capability to support multiple file open action * re-write those comments by following standard format * design added the "uncomplete" error return through flush() file operation changelog v6: * clean up on error handling for better code flow and review * clean up on pr_err() for critical error only * design taking care writing block that below PAGE_SIZE * once error has occurred, design will return -EIO until file close * document design expectations/scenarios in the code * change the dynamic allocation cap_info struct to statically allocated changelog v5: * changed to new design without leveraging firmware_class API * use misc_char device interface instead of sysfs * error return through file Write() function call Kweh, Hock Leong (2): efi: export efi_capsule_supported() function symbol efi: a misc char interface for user to update efi firmware drivers/firmware/efi/Kconfig | 10 drivers/firmware/efi/Makefile |1 drivers/firmware/efi/capsule.c|1 drivers/firmware/efi/efi-capsule-loader.c | 336 + 4 files changed, 348 insertions(+) create mode 100644 drivers/firmware/efi/efi-capsule-loader.c -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v6 0/2] Enable capsule loader interface for efi firmware updating
> -Original Message- > From: Borislav Petkov [mailto:b...@alien8.de] > Sent: Saturday, October 03, 2015 1:37 AM > To: Kweh, Hock Leong > Cc: Matt Fleming; Greg Kroah-Hartman; Ong, Boon Leong; LKML; linux- > e...@vger.kernel.org; Sam Protsenko; Peter Jones; Andy Lutomirski; Roy > Franz; James Bottomley; Linux FS Devel; Fleming, Matt > Subject: Re: [PATCH v6 0/2] Enable capsule loader interface for efi firmware > updating > > On Fri, Oct 02, 2015 at 05:05:52AM +0800, Kweh, Hock Leong wrote: > > From: "Kweh, Hock Leong" > > > > Dear maintainers & communities, > > > > This patchset is created on top of Matt's patchset: > > 1.)https://lkml.org/lkml/2014/10/7/390 > > "[PATCH 1/2] efi: Move efi_status_to_err() to efi.h" > > 2.)https://lkml.org/lkml/2014/10/7/391 > > "[PATCH 2/2] efi: Capsule update support" > > > > It expose a misc char interface for user to upload the capsule binary > > and calling efi_capsule_update() API to pass the binary to EFI firmware. > > > > The steps to update efi firmware are: > > 1.) cat firmware.cap > /dev/efi_capsule_loader > > 2.) reboot > > > > Any failed upload error message will be returned while doing "cat" > > through > > Write() function call. > > > > Tested the code with Intel Quark Galileo platform. > > What does the error case look like? A standard glibc message about > write(2) failing? > Any upload fail error like -ENOMEM, -EINVAL, -EIO as well as error returned by efi_capsule_update() API. Thanks.
RE: [PATCH v6 1/2] efi: export efi_capsule_supported() function symbol
> -Original Message- > From: Borislav Petkov [mailto:b...@alien8.de] > Sent: Saturday, October 03, 2015 1:37 AM > To: Kweh, Hock Leong > Cc: Matt Fleming; Greg Kroah-Hartman; Ong, Boon Leong; LKML; linux- > e...@vger.kernel.org; Sam Protsenko; Peter Jones; Andy Lutomirski; Roy > Franz; James Bottomley; Linux FS Devel; Fleming, Matt > Subject: Re: [PATCH v6 1/2] efi: export efi_capsule_supported() function > symbol > > On Fri, Oct 02, 2015 at 05:05:53AM +0800, Kweh, Hock Leong wrote: > > From: "Kweh, Hock Leong" > > > > This patch export efi_capsule_supported() function symbol for capsule > > kernel module to use. > > > > Cc: Matt Fleming > > Signed-off-by: Kweh, Hock Leong > > --- > > drivers/firmware/efi/capsule.c |1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/drivers/firmware/efi/capsule.c > > b/drivers/firmware/efi/capsule.c index d8cd75c0..738d437 100644 > > --- a/drivers/firmware/efi/capsule.c > > +++ b/drivers/firmware/efi/capsule.c > > @@ -101,6 +101,7 @@ out: > > kfree(capsule); > > return rv; > > } > > +EXPORT_SYMBOL_GPL(efi_capsule_supported); > > > > /** > > * efi_capsule_update - send a capsule to the firmware > > Why is this a separate patch? > It is because the author of this code is Matt. Submitting this, allows him to easily squash into his patch: https://lkml.org/lkml/2014/10/7/391 Thanks.
[PATCH v6 1/2] efi: export efi_capsule_supported() function symbol
From: "Kweh, Hock Leong" This patch export efi_capsule_supported() function symbol for capsule kernel module to use. Cc: Matt Fleming Signed-off-by: Kweh, Hock Leong --- drivers/firmware/efi/capsule.c |1 + 1 file changed, 1 insertion(+) diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c index d8cd75c0..738d437 100644 --- a/drivers/firmware/efi/capsule.c +++ b/drivers/firmware/efi/capsule.c @@ -101,6 +101,7 @@ out: kfree(capsule); return rv; } +EXPORT_SYMBOL_GPL(efi_capsule_supported); /** * efi_capsule_update - send a capsule to the firmware -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v6 0/2] Enable capsule loader interface for efi firmware updating
From: "Kweh, Hock Leong" Dear maintainers & communities, This patchset is created on top of Matt's patchset: 1.)https://lkml.org/lkml/2014/10/7/390 "[PATCH 1/2] efi: Move efi_status_to_err() to efi.h" 2.)https://lkml.org/lkml/2014/10/7/391 "[PATCH 2/2] efi: Capsule update support" It expose a misc char interface for user to upload the capsule binary and calling efi_capsule_update() API to pass the binary to EFI firmware. The steps to update efi firmware are: 1.) cat firmware.cap > /dev/efi_capsule_loader 2.) reboot Any failed upload error message will be returned while doing "cat" through Write() function call. Tested the code with Intel Quark Galileo platform. Thanks. --- changelog v6: * clean up on error handling for better code flow and review * clean up on pr_err() for critical error only * design taking care writing block that below PAGE_SIZE * once error has occurred, design will return -EIO until file close * document design expectations/scenarios in the code * change the dynamic allocation cap_info struct to statically allocated changelog v5: * changed to new design without leveraging firmware_class API * use misc_char device interface instead of sysfs * error return through file Write() function call Kweh, Hock Leong (2): efi: export efi_capsule_supported() function symbol efi: a misc char interface for user to update efi firmware drivers/firmware/efi/Kconfig | 10 ++ drivers/firmware/efi/Makefile |1 drivers/firmware/efi/capsule.c|1 drivers/firmware/efi/efi-capsule-loader.c | 246 + 4 files changed, 258 insertions(+) create mode 100644 drivers/firmware/efi/efi-capsule-loader.c -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v6 2/2] efi: a misc char interface for user to update efi firmware
From: "Kweh, Hock Leong"
Introducing a kernel module to expose capsule loader interface
(misc char device file note) for user to upload capsule binaries.
Example method to load the capsule binary:
cat firmware.bin > /dev/efi_capsule_loader
Cc: Matt Fleming
Signed-off-by: Kweh, Hock Leong
---
drivers/firmware/efi/Kconfig | 10 ++
drivers/firmware/efi/Makefile |1 +
drivers/firmware/efi/efi-capsule-loader.c | 246 +
3 files changed, 257 insertions(+)
create mode 100644 drivers/firmware/efi/efi-capsule-loader.c
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index f712d47..0be8ee3 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -60,6 +60,16 @@ config EFI_RUNTIME_WRAPPERS
config EFI_ARMSTUB
bool
+config EFI_CAPSULE_LOADER
+ tristate "EFI capsule loader"
+ depends on EFI
+ help
+ This option exposes a loader interface "/dev/efi_capsule_loader" for
+ user to load EFI capsule binary and update the EFI firmware through
+ system reboot.
+
+ If unsure, say N.
+
endmenu
config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 698846e..5ab031a 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER) += cper.o
obj-$(CONFIG_EFI_RUNTIME_MAP) += runtime-map.o
obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o
obj-$(CONFIG_EFI_STUB) += libstub/
+obj-$(CONFIG_EFI_CAPSULE_LOADER) += efi-capsule-loader.o
diff --git a/drivers/firmware/efi/efi-capsule-loader.c
b/drivers/firmware/efi/efi-capsule-loader.c
new file mode 100644
index 000..571e0c8
--- /dev/null
+++ b/drivers/firmware/efi/efi-capsule-loader.c
@@ -0,0 +1,246 @@
+/*
+ * EFI capsule loader driver.
+ *
+ * Copyright 2015 Intel Corporation
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DEV_NAME "efi_capsule_loader"
+
+static struct capsule_info {
+ charheader_obtained;
+ longindex;
+ unsigned long count;
+ unsigned long total_size;
+ struct page **pages;
+ unsigned long page_count_remain;
+} cap_info;
+
+static DEFINE_MUTEX(capsule_loader_lock);
+
+/*
+ * This function will free the current & all previous allocated buffer pages.
+ * The input parameter is the allocated buffer page for current execution which
+ * has not yet assigned to pages array. The input param can be NULL if the
+ * current execution has not allocated any buffer page.
+ */
+static void efi_free_all_buff_pages(struct page *kbuff_page)
+{
+ if (!kbuff_page)
+ __free_page(kbuff_page);
+
+ if (cap_info.index > 0)
+ while (cap_info.index > 0)
+ __free_page(cap_info.pages[--cap_info.index]);
+
+ if (cap_info.header_obtained)
+ kfree(cap_info.pages);
+
+ /* indicate to the next that error had occurred */
+ cap_info.index = -2;
+}
+
+/*
+ * This function will store the capsule binary and pass it to
+ * efi_capsule_update() API in capsule.c.
+ *
+ * Expectation:
+ * - userspace tool should start at the beginning of capsule binary and pass
+ * data in sequential.
+ * - user should close and re-open this file note in order to upload more
+ * capsules.
+ * - any error occurred, user should close the file and restart the operation
+ * for the next try otherwise -EIO will be returned until the file is closed.
+ */
+static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
+size_t count, loff_t *offp)
+{
+ int ret = 0;
+ struct page *kbuff_page;
+ void *kbuff;
+ size_t write_byte;
+
+ pr_debug("%s: Entering Write()\n", __func__);
+ if (count == 0)
+ return 0;
+
+ /* return error while error had occurred or capsule uploading is done */
+ if (cap_info.index < 0)
+ return -EIO;
+
+ /* only alloc a new page when the current page is full */
+ if (!cap_info.page_count_remain) {
+ kbuff_page = alloc_page(GFP_KERNEL);
+ if (!kbuff_page) {
+ pr_debug("%s: alloc_page() failed\n", __func__);
+ efi_free_all_buff_pages(NULL);
+ return -ENOMEM;
+ }
+ cap_info.page_count_remain = PAGE_SIZE;
+ } else {
+ kbuff_page = cap_info.pages[--cap_info.index];
+ }
+ kbuff = kmap(kbuff_page);
+ if (!kbuff) {
+
RE: [PATCH v5 2/2] efi: a misc char interface for user to update efi firmware
> -Original Message-
> From: Matt Fleming [mailto:m...@codeblueprint.co.uk]
> Sent: Thursday, August 27, 2015 10:43 PM
> >
> > Introducing a kernel module to expose capsule loader interface
> > (misc char device file note) for user to upload capsule binaries.
> >
> > Example method to load the capsule binary:
> > cat firmware.bin > /dev/efi_capsule_loader
>
> OK interesting, we're going down the misc char device route - Andy
> might be happier, even if there is no ioctl(2) support.
>
> > Cc: Matt Fleming
> > Signed-off-by: Kweh, Hock Leong
> > ---
> > drivers/firmware/efi/Kconfig | 10 ++
> > drivers/firmware/efi/Makefile |1 +
> > drivers/firmware/efi/efi-capsule-loader.c | 218
> +
> > 3 files changed, 229 insertions(+)
> > create mode 100644 drivers/firmware/efi/efi-capsule-loader.c
>
> [...]
>
> > diff --git a/drivers/firmware/efi/efi-capsule-loader.c
> b/drivers/firmware/efi/efi-capsule-loader.c
> > new file mode 100644
> > index 000..1da8608
> > --- /dev/null
> > +++ b/drivers/firmware/efi/efi-capsule-loader.c
> > @@ -0,0 +1,218 @@
> > +/*
> > + * EFI capsule loader driver.
> > + *
> > + * Copyright 2015 Intel Corporation
> > + *
> > + * This file is part of the Linux kernel, and is made available under
> > + * the terms of the GNU General Public License version 2.
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +#include
> > +
> > +#define DEV_NAME "efi_capsule_loader"
> > +
> > +struct capsule_info {
> > + int curr_index;
> > + int curr_size;
> > + int total_size;
>
> It's totally conceivable that a capsule might be greater than 2GB in
> size. In which case, 'int' is the wrong data type for these size
> fields. Perhaps 'unsigned long' ?
>
> I'd suggest swapping 'curr_index' for simply 'index' and 'curr_size'
> for 'count' or 'bytes'.
>
Will do.
> > + struct page **pages;
> > +};
> > +
> > +static DEFINE_MUTEX(capsule_loader_lock);
> > +
> > +/*
> > + * This function will store the capsule binary and pass it to
> > + * efi_capsule_update() API in capsule.c
> > + */
> > +static ssize_t efi_capsule_write(struct file *file, const char __user
> > *buff,
> > +size_t count, loff_t *offp)
> > +{
> > + int ret = 0;
> > + struct capsule_info *cap_info = file->private_data;
> > + struct page *kbuff_page;
> > + void *kbuff;
> > +
> > + pr_debug("%s: Entering Write()\n", __func__);
> > + if (count == 0)
> > + return 0;
> > +
> > + if (cap_info->curr_index == -1)
> > + return count;
>
> Shouldn't we be returning an error here to signal that the driver
> wasn't expecting any more data to be sent? Otherwise the caller will
> think everything worked out fine, but it didn't. See my comments below
> about the success/failure design.
>
Ok, not a problem.
> > +
> > + kbuff_page = alloc_page(GFP_KERNEL);
> > + if (!kbuff_page) {
> > + pr_err("%s: alloc_page() failed\n", __func__);
> > + if (!cap_info->curr_index)
> > + return -ENOMEM;
> > + ret = -ENOMEM;
> > + goto failed;
> > + }
>
> If the caller writes less than PAGE_SIZE bytes at a time this could be
> incredibly wasteful. We should use the remaining space from any
> previous page allocations.
>
Ok, will re-think about this part.
> > +
> > + kbuff = kmap(kbuff_page);
> > + if (!kbuff) {
> > + pr_err("%s: kmap() failed\n", __func__);
> > + if (cap_info->curr_index)
> > + cap_info->pages[cap_info->curr_index++] =
> kbuff_page;
> > + ret = -EFAULT;
> > + goto failed;
> > + }
> > +
> > + /* copy capsule binary data from user space to kernel space buffer */
> > + if (copy_from_user(kbuff, buff, min_t(size_t, count, PAGE_SIZE))) {
> > + pr_err("%s: copy_from_user() failed\n", __func__);
> > + if (cap_info->curr_index)
> > + cap_info->pages[cap_info->c
[PATCH v5 1/2] efi: export efi_capsule_supported() function symbol
From: "Kweh, Hock Leong" This patch export efi_capsule_supported() function symbol for capsule kernel module to use. Cc: Matt Fleming Signed-off-by: Kweh, Hock Leong --- drivers/firmware/efi/capsule.c |1 + 1 file changed, 1 insertion(+) diff --git a/drivers/firmware/efi/capsule.c b/drivers/firmware/efi/capsule.c index d8cd75c0..738d437 100644 --- a/drivers/firmware/efi/capsule.c +++ b/drivers/firmware/efi/capsule.c @@ -101,6 +101,7 @@ out: kfree(capsule); return rv; } +EXPORT_SYMBOL_GPL(efi_capsule_supported); /** * efi_capsule_update - send a capsule to the firmware -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[PATCH v5 2/2] efi: a misc char interface for user to update efi firmware
From: "Kweh, Hock Leong"
Introducing a kernel module to expose capsule loader interface
(misc char device file note) for user to upload capsule binaries.
Example method to load the capsule binary:
cat firmware.bin > /dev/efi_capsule_loader
Cc: Matt Fleming
Signed-off-by: Kweh, Hock Leong
---
drivers/firmware/efi/Kconfig | 10 ++
drivers/firmware/efi/Makefile |1 +
drivers/firmware/efi/efi-capsule-loader.c | 218 +
3 files changed, 229 insertions(+)
create mode 100644 drivers/firmware/efi/efi-capsule-loader.c
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index f712d47..0be8ee3 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -60,6 +60,16 @@ config EFI_RUNTIME_WRAPPERS
config EFI_ARMSTUB
bool
+config EFI_CAPSULE_LOADER
+ tristate "EFI capsule loader"
+ depends on EFI
+ help
+ This option exposes a loader interface "/dev/efi_capsule_loader" for
+ user to load EFI capsule binary and update the EFI firmware through
+ system reboot.
+
+ If unsure, say N.
+
endmenu
config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 698846e..5ab031a 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER) += cper.o
obj-$(CONFIG_EFI_RUNTIME_MAP) += runtime-map.o
obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o
obj-$(CONFIG_EFI_STUB) += libstub/
+obj-$(CONFIG_EFI_CAPSULE_LOADER) += efi-capsule-loader.o
diff --git a/drivers/firmware/efi/efi-capsule-loader.c
b/drivers/firmware/efi/efi-capsule-loader.c
new file mode 100644
index 000..1da8608
--- /dev/null
+++ b/drivers/firmware/efi/efi-capsule-loader.c
@@ -0,0 +1,218 @@
+/*
+ * EFI capsule loader driver.
+ *
+ * Copyright 2015 Intel Corporation
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DEV_NAME "efi_capsule_loader"
+
+struct capsule_info {
+ int curr_index;
+ int curr_size;
+ int total_size;
+ struct page **pages;
+};
+
+static DEFINE_MUTEX(capsule_loader_lock);
+
+/*
+ * This function will store the capsule binary and pass it to
+ * efi_capsule_update() API in capsule.c
+ */
+static ssize_t efi_capsule_write(struct file *file, const char __user *buff,
+size_t count, loff_t *offp)
+{
+ int ret = 0;
+ struct capsule_info *cap_info = file->private_data;
+ struct page *kbuff_page;
+ void *kbuff;
+
+ pr_debug("%s: Entering Write()\n", __func__);
+ if (count == 0)
+ return 0;
+
+ if (cap_info->curr_index == -1)
+ return count;
+
+ kbuff_page = alloc_page(GFP_KERNEL);
+ if (!kbuff_page) {
+ pr_err("%s: alloc_page() failed\n", __func__);
+ if (!cap_info->curr_index)
+ return -ENOMEM;
+ ret = -ENOMEM;
+ goto failed;
+ }
+
+ kbuff = kmap(kbuff_page);
+ if (!kbuff) {
+ pr_err("%s: kmap() failed\n", __func__);
+ if (cap_info->curr_index)
+ cap_info->pages[cap_info->curr_index++] = kbuff_page;
+ ret = -EFAULT;
+ goto failed;
+ }
+
+ /* copy capsule binary data from user space to kernel space buffer */
+ if (copy_from_user(kbuff, buff, min_t(size_t, count, PAGE_SIZE))) {
+ pr_err("%s: copy_from_user() failed\n", __func__);
+ if (cap_info->curr_index)
+ cap_info->pages[cap_info->curr_index++] = kbuff_page;
+ kunmap(kbuff_page);
+ ret = -EFAULT;
+ goto failed;
+ }
+
+ /* setup capsule binary info structure */
+ if (cap_info->curr_index == 0) {
+ efi_capsule_header_t *cap_hdr = kbuff;
+ int reset_type;
+ int pages_needed = ALIGN(cap_hdr->imagesize, PAGE_SIZE) >>
+ PAGE_SHIFT;
+
+ if (pages_needed <= 0) {
+ pr_err("%s: pages count invalid\n", __func__);
+ ret = -EINVAL;
+ kunmap(kbuff_page);
+ goto failed;
+ }
+
+ /* check if the capsule binary supported */
+ ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
+
[PATCH v5 0/2] Enable capsule loader interface for efi firmware
From: "Kweh, Hock Leong" Dear maintainers & communities, This patchset is created on top of Matt's patchset: 1.)https://lkml.org/lkml/2014/10/7/390 "[PATCH 1/2] efi: Move efi_status_to_err() to efi.h" 2.)https://lkml.org/lkml/2014/10/7/391 "[PATCH 2/2] efi: Capsule update support" It expose a misc char interface for user to upload the capsule binary and calling efi_capsule_update() API to pass the binary to EFI firmware. The steps to update efi firmware are: 1.) cat firmware.cap > /dev/efi_capsule_loader 2.) reboot Any failed upload error message will be returned while doing "cat" through Write() function call. Tested the code with Intel Quark Galileo platform. Thanks. --- changelog v5: * changed to new design without leveraging firmware_class API * use misc_char device interface instead of sysfs * error return through file Write() function call Kweh, Hock Leong (2): efi: export efi_capsule_supported() function symbol efi: a misc char interface for user to update efi firmware drivers/firmware/efi/Kconfig | 10 ++ drivers/firmware/efi/Makefile |1 drivers/firmware/efi/capsule.c|1 drivers/firmware/efi/efi-capsule-loader.c | 218 + 4 files changed, 230 insertions(+) create mode 100644 drivers/firmware/efi/efi-capsule-loader.c -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [RFC 0/3] Add capsule update using error on close semantics
> -Original Message- > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] > Sent: Thursday, April 30, 2015 7:08 AM > To: linux-...@vger.kernel.org > Cc: Kweh, Hock Leong; LKML; Andy Lutomirski; Greg Kroah-Hartman; Peter > Jones > Subject: [RFC 0/3] Add capsule update using error on close semantics > > This is a straw man implementation. The three patches firstly thread the > needed ->flush() file op through sysfs and kernfs. The next one extracts > transaction buffer handling from firmware_class.c and makes it generic in a > lib helper and the third patch adds a bare bones capsule update (I suspect > the latter needs more work, since it doesn't implement the scatterlist). > > James Bottomley (3): > sysfs,kernfs: add flush operation > firmware_class: split out transaction helpers > efi: add capsule update capability via sysfs > > drivers/base/firmware_class.c | 117 --- > drivers/firmware/efi/Makefile | 2 +- > drivers/firmware/efi/capsule.c | 78 + > drivers/firmware/efi/capsule.h | 2 + > drivers/firmware/efi/efi.c | 8 +++ > fs/kernfs/file.c | 16 + > fs/sysfs/file.c| 16 + > include/linux/kernfs.h | 2 + > include/linux/sysfs.h | 2 + > include/linux/transaction_helper.h | 26 +++ > lib/Makefile | 2 +- > lib/transaction_helper.c | 137 > + > 12 files changed, 304 insertions(+), 104 deletions(-) create mode 100644 > drivers/firmware/efi/capsule.c create mode 100644 > drivers/firmware/efi/capsule.h create mode 100644 > include/linux/transaction_helper.h > create mode 100644 lib/transaction_helper.c > > James Hi James, I like the sysfs enhancement but require Greg to buy in the idea. For the efi capsule part, Matt has implemented some APIs where you could get the patch at http://permalink.gmane.org/gmane.linux.kernel.efi/4837. So, I would think that leveraging the APIs that Matt has created is a better choice. Thanks & Regards, Wilson
RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message- > From: Andy Lutomirski [mailto:l...@amacapital.net] > Sent: Thursday, April 30, 2015 2:41 AM > > On Wed, Apr 29, 2015 at 4:23 AM, Kweh, Hock Leong > wrote: > > > > Dear communities, > > > > I agree with James. Due to different people may have different needs. > > But from our side, we would just like to have a simple interface for > > us to upload the efi capsule and perform update. We do not have any > > use case or need to get info from QueryCapsuleUpdate(). Let me give a > suggestion here: > > please allow me to focus on deliver this simple loading interface and > > upstream it. Then later whoever has the actual use case or needs on > > the ioctl implementation, he or she could enhance base on this simple > loading interface. > > What do you guys think? > > > > Let me summarize the latest design idea: > > - No longer leverage on firmware class but use misc device > > - Do not use platform device but use device_create() > > - User just need to perform "cat file.bin > /sys/.../capsule_loader" > > in the shell > > If you do this, there's no need for the misc device. I do this so that in the future when someone want to implement the Ioctl(), he or she can base on this and expand it. > > > - File operation functions include: open(), read(), write() and > > flush() > > - Perform mutex lock in open() then release the mutex in flush() for > avoiding > >race condition / concurrent loading > > Make sure the mutex operation is killable, then, and maybe even > interruptable. Okay. > > > - Perform the capsule update and error return at flush() function > > > > Is there anything I missed? Any one still have concern with this idea? > > Thanks for providing the ideas as well as the review. > > > > If it works (and cat really does fail reliably), then it seems okay to me. > > However, since I like pulling increasing numbers of my hats, someone should > verify that the common embedded cat implementations are also okay with > this. For example, I haven't yet found any code in busybox's cat > implementation that closes stdout. > > Given that the main targets of this (for now, at least) are embedded, this > might be a problem. > I think we shouldn't focus on the cat implementation for the close issue. My understanding about this action: cat file.bin > /sys//capsule_loader It is actually the ">" (IO redirection) who perform the open write & close to this "/sys//capsule_loader" file note and not the "cat" do it. So, I think your answer can be found at Shell source code. More info about the IO redirection can be found at: http://www.tldp.org/LDP/abs/html/io-redirection.html Busybox Shell Souce code can be found at: https://lxr.missinglinkelectronics.com/busybox/shell/ash.c Regards, Wilson
RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message- > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] > Sent: Tuesday, April 28, 2015 6:52 AM > > On Mon, 2015-04-27 at 15:40 -0700, Andy Lutomirski wrote: > > On Mon, Apr 27, 2015 at 3:35 PM, James Bottomley > > wrote: > > > On Mon, 2015-04-27 at 14:59 -0700, Andy Lutomirski wrote: > > >> On Fri, Apr 24, 2015 at 8:16 AM, James Bottomley > > >> wrote: > > >> > On Fri, 2015-04-24 at 02:14 +, Kweh, Hock Leong wrote: > > >> >> > -Original Message- > > >> >> > From: James Bottomley > > >> >> > [mailto:james.bottom...@hansenpartnership.com] > > >> >> > Sent: Thursday, April 23, 2015 10:10 PM > > >> >> > > > >> >> > On Thu, 2015-04-23 at 08:30 +, Kweh, Hock Leong wrote: > > >> >> > > > -Original Message- > > >> >> > > > From: James Bottomley > > >> >> > [mailto:james.bottom...@hansenpartnership.com] > > >> >> > > > Sent: Wednesday, April 22, 2015 11:19 PM > > >> >> > > > > > >> >> > > > > > >> >> > > > Yes, I think we've all agreed we can do it ... it's now a > > >> >> > > > question of whether > > >> >> > we > > >> >> > > > can stomach the ick factor of actually initiating a > > >> >> > > > transaction in close ... I'm > > >> >> > still > > >> >> > > > feeling queasy. > > >> >> > > > > >> >> > > The file "close" here can I understand that the file system > > >> >> > > will call the > > >> >> > "release" > > >> >> > > function at the file_operations struct? > > >> >> > > http://lxr.free-electrons.com/source/include/linux/fs.h#L153 > > >> >> > > 8 > > >> >> > > > > >> >> > > So, James you are meaning that we could initiating the > > >> >> > > update transaction inside the f_ops->release() and return > > >> >> > > the error code if update failed in this function? > > >> >> > > > >> >> > Well, that's what I was thinking. However the return value of > > >> >> > ->release doesn't get propagated in sys_close (or indeed > > >> >> > anywhere ... no idea why it returns an int) thanks to the task > > >> >> > work additions, so we'd actually have to use the operation > > >> >> > whose value is propagated in sys_close() which turns out to be > flush. > > >> >> > > > >> >> > James > > >> >> > > > >> >> > > >> >> Okay, I think I got you. Just to double check for in case: you > > >> >> are meaning to implement it at f_ops->flush() instead of f_ops- > >release(). > > >> > > > >> > Well, what I'm saying is that the only way to propagate an error > > >> > to close is by returning one from the flush file_operation. > > >> > > > >> > Let's cc fsdevel to see if they have any brighter ideas. > > >> > > > >> > The problem is we need to update firmware (several megabytes of > > >> > it) via standard system tools. We're thinking cat to a device. > > >> > The problem is that we need an error code back once the update > > >> > goes through (which it can't until we've fed all the firmware > > >> > data into the system). To use standard unix tools, we have to > > >> > trigger off the standard system calls cat uses and since write() > > >> > will happen in chunks, the only way to commit the transaction is in > close(). > > >> > > > >> > We initially through of initiating the transaction in > > >> > f_ops->release and returning the error code there, but that > > >> > doesn't work because its value isn't actually propagated, so > > >> > we're now thinking of initiating the transaction in f_ops->flush > > >> > instead (this is a device, not a file, so it won't get any other > > >> > flushers). Are there any other ways for us to propaga
RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message-
> From: James Bottomley [mailto:james.bottom...@hansenpartnership.com]
> Sent: Thursday, April 23, 2015 10:10 PM
>
> On Thu, 2015-04-23 at 08:30 +0000, Kweh, Hock Leong wrote:
> > > -Original Message-
> > > From: James Bottomley
> [mailto:james.bottom...@hansenpartnership.com]
> > > Sent: Wednesday, April 22, 2015 11:19 PM
> > >
> > >
> > > Yes, I think we've all agreed we can do it ... it's now a question of
> > > whether
> we
> > > can stomach the ick factor of actually initiating a transaction in close
> > > ... I'm
> still
> > > feeling queasy.
> >
> > The file "close" here can I understand that the file system will call the
> "release"
> > function at the file_operations struct?
> > http://lxr.free-electrons.com/source/include/linux/fs.h#L1538
> >
> > So, James you are meaning that we could initiating the update transaction
> > inside the f_ops->release() and return the error code if update failed in
> > this
> > function?
>
> Well, that's what I was thinking. However the return value of ->release
> doesn't get propagated in sys_close (or indeed anywhere ... no idea why
> it returns an int) thanks to the task work additions, so we'd actually
> have to use the operation whose value is propagated in sys_close() which
> turns out to be flush.
>
> James
>
Okay, I think I got you. Just to double check for in case: you are meaning
to implement it at f_ops->flush() instead of f_ops->release().
Thanks & Regards,
Wilson
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message- > From: James Bottomley [mailto:james.bottom...@hansenpartnership.com] > Sent: Wednesday, April 22, 2015 11:19 PM > > > Yes, I think we've all agreed we can do it ... it's now a question of whether > we > can stomach the ick factor of actually initiating a transaction in close ... > I'm still > feeling queasy. The file "close" here can I understand that the file system will call the "release" function at the file_operations struct? http://lxr.free-electrons.com/source/include/linux/fs.h#L1538 So, James you are meaning that we could initiating the update transaction inside the f_ops->release() and return the error code if update failed in this function? Thanks & Regards, Wilson
RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message- > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] > Sent: Monday, April 20, 2015 10:43 PM > > On Mon, Apr 20, 2015 at 03:28:32AM +0000, Kweh, Hock Leong wrote: > > Regarding the 'reboot require' status, is it critical to have a 1 to 1 > > status > match > > with the capsule upload binary? Is it okay to have one sysfs file note to > > tell > the > > overall status (for example: 10 capsule binaries uploaded but one require > > reboot, so the status shows reboot require is yes)? I am not here trying to > argue > > anything. I am just trying to find out what kind of info is needed but the > sysfs > > could not provide. > > > > Please imagine if your whole Linux system (kernel + rootfs) has to fit into > 6MB > > space and you don't even have the gcc compiler included into the package. > > I believe in this environment, kernel interface + shell command is the only > > interaction that user could work with. > > Why would you have to have gcc on such a system? Why is that a > requirement for having an ioctl/char interface? This is my logic: - Besides writing a C program (for example), I am not aware any shell script could perform an ioctl function call. This led me to if I don't have an execution binary then I need a compiler to compile the source to execution binary. - For embedded product as mentioned above, not all vendors willing to carry the userland tool when they are struggling to fit into small memory space. Yet, you may say this tool would not eat up a lot of space compare to others. But when the source of this tool being upstream-ed to the tools/ kernel tree, we cannot stop people to contribute and make the tool more features support, eventually the embedded product may need to drop the tool. > > And if you only have 6Mb of space, you don't have UEFI, sorry, there's > no way that firmware can get that small. Actually there is. Quark is one of the examples. The kernel + rootfs take up 6MB and the UEFI consume only 2MB, so total size 8MB in the spi chip. If you have an Intel Galileo board, you don't need any mass storage (SD & USB), you are able to boot to Linux console. Thanks & Regards, Wilson -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message- > From: Matt Fleming [mailto:m...@codeblueprint.co.uk] > Sent: Friday, April 17, 2015 10:37 PM > > On Fri, 17 Apr, at 03:49:24PM, Greg KH wrote: > > > > Not really, the kernel namespace is what matters at that point in time. > > > > And maybe it does matter, I haven't thought through all of the issues. > > But passing a path from userspace, to the kernel, to have the kernel > > turn around again and use that path is full of nasty consequences at > > times due to namespaces, let's avoid all of that please. > > Oh crap. The namespace issue is a good point and not something I'd > thought of at all. > > At this point, I think we've really run out of objections to Andy's > suggestion of implementing this as a misc device, right? > > The concern I had about userspace tooling can partly be addressed by > including the source in tools/ in the kernel tree. So provided we do > that, I'm happy enough to see this implemented as a misc device because > the other options we've explored just haven't panned out. > > Objections? > > -- > Matt Fleming, Intel Open Source Technology Center Hi Greg, Matt & Andy, Wait wait a minute. I am lost. I think I have missed something. Let me summarize the message I got from the email threads. = Greg:- Recommends to use "cat file.bin > /sys/.../capsule_loader" due to there is concern on kernel namespace with this submitted design which using the request firmware API. Andy:- Prefer to have an interface that could return the error code and suggested char device where the ioctl can meet the purpose. Matt:- Prefer to use kernel interface only as embedded world may not want to include userland tool. == I think I have missed somewhere why not using "cat file.bin > /sys/.../loader" and now change to misc device. Is it because the ioctl could return a custom error code (for example: platform not supported, capsule header error) where the "cat file.bin > /sys/.../loader" interface cannot do? Hmm .. Regarding the 'reboot require' status, is it critical to have a 1 to 1 status match with the capsule upload binary? Is it okay to have one sysfs file note to tell the overall status (for example: 10 capsule binaries uploaded but one require reboot, so the status shows reboot require is yes)? I am not here trying to argue anything. I am just trying to find out what kind of info is needed but the sysfs could not provide. Please imagine if your whole Linux system (kernel + rootfs) has to fit into 6MB space and you don't even have the gcc compiler included into the package. I believe in this environment, kernel interface + shell command is the only interaction that user could work with. Btw, just to make sure I get it correctly, is misc device refer to the device that created by misc_register() from drivers/char/misc.c and not asked to put this kernel module under drivers/misc/* location, right? And Matt mentioned including the source into tools/* in kernel tree. I have a question: Is this tool can be compiled during kernel compilation and eventually auto included into the rootfs package? Sorry, I am new to OS creation and maybe this is stupid question. Thanks & Regards, Wilson -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message-
> From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> Sent: Wednesday, April 15, 2015 9:19 PM
>
> On Wed, Apr 15, 2015 at 11:32:29AM +0000, Kweh, Hock Leong wrote:
> > > -Original Message-
> > > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> > > Sent: Tuesday, April 14, 2015 10:09 PM
> > >
> > > On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> > > > From: "Kweh, Hock Leong"
> > > >
> > > > Introducing a kernel module to expose capsule loader interface
> > > > for user to upload capsule binaries. This module leverage the
> > > > request_firmware_direct_full_path() to obtain the binary at a
> > > > specific path input by user.
> > > >
> > > > Example method to load the capsule binary:
> > > > echo -n "/path/to/capsule/binary" >
> > > /sys/devices/platform/efi_capsule_loader/capsule_loader
> > >
> > > Ick, why not just have the firmware file location present, and copy it
> > > to the sysfs file directly from userspace, instead of this two-step
> > > process?
> >
> > Err I may not catch your meaning correctly. Are you trying to say
> > that you would prefer the user to perform:
> >
> > cat file.bin > /sys/.../capsule_loader
> >
> > instead of
> >
> > echo -n "/path/to/binary" > /sys//capsule_laoder
>
> Yes. What's the namespace of your /path/to/binary/ and how do you know
> the kernel has the same one when it does the firmware load call? By
> just copying the data with 'cat', you don't have to worry about
> namespace issues at all.
Hi Greg,
Let me double confirm that I understand your concern correctly. You are
trying to tell that some others module may use a 'same' namespace to
request the firmware but never release it. Then when our module trying
to request the firmware by passing in the 'same' namespace, I will get the
previous data instead of the current binary data from the path I want.
Hmm I believe this concern also apply to all the current request_firmware
APIs right? And I believe the coincidence to have 'same' file name namespace
would be higher than full path namespace.
I may not able to control other module developers on the namespace naming.
But I could ensure each firmware request will be released before the next
request firmware happen in this module. This module will return error if it
does not receive a real efi capsule binary.
Hmm ..
>
> > The reason we stick with the firmware_class is because we don't
> > want to replicate a code which already mature and has open API
> > for developer to use.
>
> That's fine, but adding a new api to the firmware interface seems odd to
> me, just because you don't like using /lib/ or any of the other
> "standard" locations for firmware blobs. And note, that path is
> configurable.
If I am not mistaken, I believe you are referring the module param path.
Yes, I do aware of it. If I need a more flexibility method to alter the custom
path, the current design would require system reboot or module re-insmod.
So, leveraging the request_firmware_direct_full_path() could make things
a little bit easier from this point of view.
Besides, this new API actually helps to overcome the confusedness of having
2 or more same file name binaries at the firmware search paths (fw_path_para;
/lib/firmware/update/; /lib/firmware/). Due to user have the possibility to
configure kernel command param / module param for this fw_path_para,
it may have chances to point to a path that have same file name the
/lib/firmware/
also have. With this API, it can make it specific to the path that developer
wants.
>
> > > > + */
> > > > +static void __exit efi_capsule_loader_exit(void)
> > > > +{
> > > > + platform_device_unregister(efi_capsule_pdev);
> > >
> > > This is not a platform device, don't abuse that interface please.
> > >
> > > greg k-h
> >
> > Okay, so you would recommend to use device_register() for this case?
> > Or you would think that this is more suitable to use class_register()?
>
> A class isn't needed, you just want a device right? So just use a
> device, but not a platform device, as that isn't what you have here.
>
> thanks,
>
> greg k-h
Okay, will do this. Thanks.
Regards,
Wilson
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
> -Original Message-
> From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org]
> Sent: Tuesday, April 14, 2015 10:09 PM
>
> On Tue, Apr 14, 2015 at 05:44:56PM +0800, Kweh, Hock Leong wrote:
> > From: "Kweh, Hock Leong"
> >
> > Introducing a kernel module to expose capsule loader interface
> > for user to upload capsule binaries. This module leverage the
> > request_firmware_direct_full_path() to obtain the binary at a
> > specific path input by user.
> >
> > Example method to load the capsule binary:
> > echo -n "/path/to/capsule/binary" >
> /sys/devices/platform/efi_capsule_loader/capsule_loader
>
> Ick, why not just have the firmware file location present, and copy it
> to the sysfs file directly from userspace, instead of this two-step
> process?
Err I may not catch your meaning correctly. Are you trying to say
that you would prefer the user to perform:
cat file.bin > /sys/.../capsule_loader
instead of
echo -n "/path/to/binary" > /sys//capsule_laoder
The reason we stick with the firmware_class is because we don't
want to replicate a code which already mature and has open API
for developer to use.
>
> > +/*
> > + * To remove this kernel module, just perform:
> > + * rmmod efi_capsule_loader.ko
>
> This comment is not needed.
Okay, I will remove this.
>
>
> > + */
> > +static void __exit efi_capsule_loader_exit(void)
> > +{
> > + platform_device_unregister(efi_capsule_pdev);
>
> This is not a platform device, don't abuse that interface please.
>
> greg k-h
Okay, so you would recommend to use device_register() for this case?
Or you would think that this is more suitable to use class_register()?
Thanks & Regards,
Wilson
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 2/2] efi: an sysfs interface for user to update efi firmware
From: "Kweh, Hock Leong"
Introducing a kernel module to expose capsule loader interface
for user to upload capsule binaries. This module leverage the
request_firmware_direct_full_path() to obtain the binary at a
specific path input by user.
Example method to load the capsule binary:
echo -n "/path/to/capsule/binary" >
/sys/devices/platform/efi_capsule_loader/capsule_loader
Cc: Matt Fleming
Signed-off-by: Kweh, Hock Leong
---
drivers/firmware/efi/Kconfig | 12 ++
drivers/firmware/efi/Makefile |1 +
drivers/firmware/efi/efi-capsule-loader.c | 169 +
3 files changed, 182 insertions(+)
create mode 100644 drivers/firmware/efi/efi-capsule-loader.c
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index f712d47..3e84ec0 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -60,6 +60,18 @@ config EFI_RUNTIME_WRAPPERS
config EFI_ARMSTUB
bool
+config EFI_CAPSULE_LOADER
+ tristate "EFI capsule loader"
+ depends on EFI
+ select FW_LOADER
+ help
+ This option exposes a loader interface for user to load EFI
+ capsule binary and update the EFI firmware through system reboot.
+ This feature does not support auto locating capsule binaries at the
+ firmware lib search path.
+
+ If unsure, say N.
+
endmenu
config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 698846e..5ab031a 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER) += cper.o
obj-$(CONFIG_EFI_RUNTIME_MAP) += runtime-map.o
obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o
obj-$(CONFIG_EFI_STUB) += libstub/
+obj-$(CONFIG_EFI_CAPSULE_LOADER) += efi-capsule-loader.o
diff --git a/drivers/firmware/efi/efi-capsule-loader.c
b/drivers/firmware/efi/efi-capsule-loader.c
new file mode 100644
index 000..84b979b
--- /dev/null
+++ b/drivers/firmware/efi/efi-capsule-loader.c
@@ -0,0 +1,169 @@
+/*
+ * EFI capsule loader driver.
+ *
+ * Copyright 2015 Intel Corporation
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define DEV_NAME "efi_capsule_loader"
+
+static DEFINE_MUTEX(capsule_loader_lock);
+static struct platform_device *efi_capsule_pdev;
+
+/*
+ * This function will store the capsule binary and pass it to
+ * efi_capsule_update() API in capsule.c
+ */
+static int efi_capsule_store(const struct firmware *fw)
+{
+ int i;
+ int ret;
+ int count = fw->size;
+ int copy_size = (fw->size > PAGE_SIZE) ? PAGE_SIZE : fw->size;
+ int pages_needed = ALIGN(fw->size, PAGE_SIZE) >> PAGE_SHIFT;
+ struct page **pages;
+ void *page_data;
+ efi_capsule_header_t *capsule = NULL;
+
+ if (!count) {
+ pr_err("%s: Received zero binary size\n", __func__);
+ return -ENOENT;
+ }
+
+ pages = kmalloc_array(pages_needed, sizeof(void *), GFP_KERNEL);
+ if (!pages) {
+ pr_err("%s: kmalloc_array() failed\n", __func__);
+ return -ENOMEM;
+ }
+
+ for (i = 0; i < pages_needed; i++) {
+ pages[i] = alloc_page(GFP_KERNEL);
+ if (!pages[i]) {
+ pr_err("%s: alloc_page() failed\n", __func__);
+ --i;
+ ret = -ENOMEM;
+ goto failed;
+ }
+ }
+
+ for (i = 0; i < pages_needed; i++) {
+ page_data = kmap(pages[i]);
+ memcpy(page_data, fw->data + (i * PAGE_SIZE), copy_size);
+
+ if (i == 0)
+ capsule = (efi_capsule_header_t *)page_data;
+ else
+ kunmap(pages[i]);
+
+ count -= copy_size;
+ if (count < PAGE_SIZE)
+ copy_size = count;
+ }
+
+ ret = efi_capsule_update(capsule, pages);
+ if (ret) {
+ pr_err("%s: efi_capsule_update() failed\n", __func__);
+ --i;
+ goto failed;
+ }
+ kunmap(pages[0]);
+
+ /*
+* we cannot free the pages here due to reboot need that data
+* maintained.
+*/
+ return 0;
+
+failed:
+ while (i >= 0)
+ __free_page(pages[i--]);
+ kfree(pages);
+ return ret;
+}
+
+static ssize_t capsule_loader_store(struct device *dev,
+ struct device_attribute *attr,
+
[PATCH v4 1/2] firmware_loader: introduce new API - request_firmware_direct_full_path()
From: "Kweh, Hock Leong"
Introduce this new API for loading firmware from a specific location
instead of /lib/firmware/ by providing a full path to the firmware
file.
Cc: Ming Lei
Cc: Matt Fleming
Signed-off-by: Kweh, Hock Leong
---
drivers/base/firmware_class.c | 46 -
include/linux/firmware.h |9
2 files changed, 50 insertions(+), 5 deletions(-)
diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 171841a..3ab7bb9 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -111,6 +111,7 @@ static inline long firmware_loading_timeout(void)
#define FW_OPT_FALLBACK0
#endif
#define FW_OPT_NO_WARN (1U << 3)
+#define FW_OPT_FULL_PATH (1U << 4)
struct firmware_cache {
/* firmware_buf instance will be added into the below list */
@@ -318,20 +319,29 @@ fail:
}
static int fw_get_filesystem_firmware(struct device *device,
- struct firmware_buf *buf)
+ struct firmware_buf *buf,
+ unsigned int opt_flags)
{
int i;
int rc = -ENOENT;
char *path = __getname();
+ int path_array_size = 1;
+ static const char * const root_path[] = {"/"};
+ char **temp_path = (char **)root_path;
- for (i = 0; i < ARRAY_SIZE(fw_path); i++) {
+ if (!(opt_flags & FW_OPT_FULL_PATH)) {
+ temp_path = (char **)fw_path;
+ path_array_size = ARRAY_SIZE(fw_path);
+ }
+
+ for (i = 0; i < path_array_size; i++) {
struct file *file;
/* skip the unset customized path */
- if (!fw_path[i][0])
+ if (!temp_path[i][0])
continue;
- snprintf(path, PATH_MAX, "%s/%s", fw_path[i], buf->fw_id);
+ snprintf(path, PATH_MAX, "%s/%s", temp_path[i], buf->fw_id);
file = filp_open(path, O_RDONLY, 0);
if (IS_ERR(file))
@@ -1122,7 +1132,7 @@ _request_firmware(const struct firmware **firmware_p,
const char *name,
}
}
- ret = fw_get_filesystem_firmware(device, fw->priv);
+ ret = fw_get_filesystem_firmware(device, fw->priv, opt_flags);
if (ret) {
if (!(opt_flags & FW_OPT_NO_WARN))
dev_warn(device,
@@ -1210,6 +1220,32 @@ int request_firmware_direct(const struct firmware
**firmware_p,
EXPORT_SYMBOL_GPL(request_firmware_direct);
/**
+ * request_firmware_direct_full_path: - load firmware directly from exact
+ * full path
+ * @firmware_p: pointer to firmware image
+ * @name: full path to the firmware file with file name
+ * @device: device for which firmware is being loaded
+ *
+ * This function works like request_firmware_direct(), but this doesn't
+ * search the /lib/firmware/ for the firmware file. It support exact full
+ * path to the firmware file for loading.
+ **/
+int request_firmware_direct_full_path(const struct firmware **firmware_p,
+ const char *name, struct device *device)
+{
+ int ret;
+
+ __module_get(THIS_MODULE);
+ ret = _request_firmware(firmware_p, name, device,
+ FW_OPT_UEVENT | FW_OPT_NO_WARN |
+ FW_OPT_FULL_PATH);
+ module_put(THIS_MODULE);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(request_firmware_direct_full_path);
+
+/**
* release_firmware: - release the resource associated with a firmware image
* @fw: firmware resource to release
**/
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index 5c41c5e..b7c6435 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -47,6 +47,8 @@ int request_firmware_nowait(
void (*cont)(const struct firmware *fw, void *context));
int request_firmware_direct(const struct firmware **fw, const char *name,
struct device *device);
+int request_firmware_direct_full_path(const struct firmware **fw,
+ const char *name, struct device *device);
void release_firmware(const struct firmware *fw);
#else
@@ -75,5 +77,12 @@ static inline int request_firmware_direct(const struct
firmware **fw,
return -EINVAL;
}
+static inline int request_firmware_direct_full_path(const struct firmware **fw,
+ const char *name,
+ struct device *device)
+{
+ return -EINVAL;
+}
+
#endif
#endif
--
1.7.9.5
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v4 0/2] Enable capsule loader interface for efi firmware updating
From: "Kweh, Hock Leong" Dear maintainers & communities, This patchset is created on top of "efi: Capsule update support" patch: http://permalink.gmane.org/gmane.linux.kernel.efi/4837 It expose a sysfs loader interface for user to upload the capsule binary and calling efi_capsule_update() API to pass the binary to EFI firmware. Thanks. --- changelog v4: * rebase the firmware_class to gregkh/driver-core.git#driver-core-next branch and drop the v3 2nd patch as Zahari already fix that in that branch changelog v3: * changes base on the design discussion in the mailing list: https://lkml.org/lkml/2015/2/24/307 * 1st patch introduce a new API request_firmware_direct_full_path() in firmware_class for developer to pass in full path to the firmware file * 2nd patch fix a bug for commit "firmware_loader: handle timeout via wait_for_completion_interruptible_timeout()" * 3rd patch introduce the capsule loader interface kernel module Kweh, Hock Leong (2): firmware_loader: introduce new API - request_firmware_direct_full_path() efi: an sysfs interface for user to update efi firmware drivers/base/firmware_class.c | 46 +++- drivers/firmware/efi/Kconfig | 12 ++ drivers/firmware/efi/Makefile |1 drivers/firmware/efi/efi-capsule-loader.c | 169 + include/linux/firmware.h |9 ++ 5 files changed, 232 insertions(+), 5 deletions(-) create mode 100644 drivers/firmware/efi/efi-capsule-loader.c -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v3 0/3] Enable a capsule loader interface for user to update
> -Original Message- > From: Borislav Petkov [mailto:b...@alien8.de] > Sent: Friday, April 10, 2015 9:58 PM > > On Sat, Apr 11, 2015 at 03:40:41AM +0800, Kweh, Hock Leong wrote: > > From: "Kweh, Hock Leong" > > > > Hi Guys, > > > > This patchset is created on top of "efi: Capsule update support" patch: > > http://permalink.gmane.org/gmane.linux.kernel.efi/4837 > > > > It expose a sysfs loader interface for user to upload the capsule binary > > and calling efi_capsule_update() API to pass the binary to EFI firmware. > > What I'm missing from those 0/n mails is why we need this? What is > the problem you're trying to solve and why is Peter's userspace-only > solution not enough. > > Now I have a fairly good idea why we might/could/would need the kernel > interface but I don't believe the every reader has followed the whole > discussion. > > So please start with the Why. Try to sell it to me as best as you can. > Details will be discussed later anyway. > > Thanks. > Hi Borislav, I will make the summary, for more detail, you can refer to the threads here: - https://lkml.org/lkml/2015/3/10/418 - https://lkml.org/lkml/2015/3/10/262 Summary: - Kernel interface could provide more flexibility than just firmware updates. - For embedded small foot print devices, not all vendors are affordable to include all userland tools to their product. - Not all platform support ESRT. Regards, Wilson
RE: [PATCHv3 0/1] Intel Quark X1000 DTS thermal driver
> -Original Message- > From: Ong, Boon Leong > Sent: Tuesday, March 31, 2015 7:58 AM > To: Zhang, Rui; edubez...@gmail.com > Cc: linux...@vger.kernel.org; linux-kernel@vger.kernel.org; Ong, Boon > Leong; pure.lo...@nexus-software.ie; Kweh, Hock Leong; > andy.shevche...@gmail.com > Subject: RE: [PATCHv3 0/1] Intel Quark X1000 DTS thermal driver > > Dear maintainer, gentle ping if this driver is ready for more review/inclusion > into Linux v4.1 ? > Thank you very much. > > >-Original Message- > >From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel- > >ow...@vger.kernel.org] On Behalf Of Ong Boon Leong > >Sent: Monday, March 9, 2015 3:43 PM > >To: Zhang, Rui; edubez...@gmail.com; pure.lo...@nexus-software.ie; > >Kweh, Hock Leong; andy.shevche...@gmail.com > >Cc: linux...@vger.kernel.org; linux-kernel@vger.kernel.org > >Subject: [PATCHv3 0/1] Intel Quark X1000 DTS thermal driver > > > >Dear maintainers & communities, > > > >This patch introduces DTS thermal driver for Intel Quark X1000. > >The code implementation is based on intel_soc_dts_thermal.c. > > > >Intel Quark X1000 has one on-die DTS with two configurable trip points: > >critical and hot trip points. However, todate, UEFI BIOS for Quark > >X1000 uses only critical trip point. UEFI BIOS always lock DTS register > >before hand-over to Linux kernel. > > > >The minimalist thermal design is meant to trigger Linux distro to > >gracefully power-down the system when its DTS temperature exceeds the > >configured critical trip point. > > > >In anticipation that other variant of Quark platform may come with UEFI > >BIOS that does not lock DTS register during hand-over, this DTS driver > >is built with logics to handle such case too. > > > >I have tested v1 of the patch on Intel Galileo Gen v2 board and found > >it satisfactory with logs below: > > > > root@quark:/sys/class/thermal/thermal_zone0# echo disabled > mode > > [ 46.276881] intel_quark_dts_thermal: DTS is locked. Cannot disable DTS > > -sh: echo: write error: Operation not permitted > > root@quark:/sys/class/thermal/thermal_zone0# > > root@quark:/sys/class/thermal/thermal_zone0# cat temp > > 53 > > root@quark:/sys/class/thermal/thermal_zone0# cat trip_point_0_temp > > 105 > > root@quark:/sys/class/thermal/thermal_zone0# cat trip_point_0_type > > critical root@quark:/sys/class/thermal/thermal_zone0# cat > > trip_point_1_temp > > 20 > > root@quark:/sys/class/thermal/thermal_zone0# cat trip_point_1_type > > hot root@quark:/sys/class/thermal/thermal_zone0# cat type quark_dts > > > > root@quark:/sys/class/thermal/thermal_zone0# echo 105 > emul_temp > > [ 179.372981] thermal thermal_zone0: critical temperature reached(0 > >C),shutting down > > root@quark:/sys/class/thermal/thermal_zone0# > > [ OK ] Stopped target Multi-User System. > > Stopping Telephony service... > > Stopping Lightning Fast Webserver With Light System > > Requirements... > > Stopping Target Communication Framework agent... > > Stopping Galileo Arduino Layer... > > [ OK ] Stopped target Login Prompts. > > Stopping Getty on tty1... > > Stopping Serial Getty on ttyS1... > > Stopping Login Service... > > Stopping D-Bus System Message Bus... > > Starting Store Sound Card State... > > [ OK ] Stopped Telephony service. > > [ OK ] Stopped Galileo Arduino Layer. > > [ OK ] Stopped Login Service. > > [ OK ] Stopped D-Bus System Message Bus. > > [ OK ] Stopped Target Communication Framework agent. > > [ OK ] Stopped Lightning Fast Webserver With Light System Requirements. > > [ OK ] Stopped WPA supplicant. > > [ OK ] Stopped Getty on tty1. > > [ OK ] Stopped Serial Getty on ttyS1. > > > >Please kindly review the patch at your convenient time and provide me > >feedback for improvement. Appreciate your time and effort. > > > >Thank You > >Ong Boon Leong > >Intel Corp. > > > >--- > >Changes in v3: > >* Kconfig dependency changed to X86_INTEL_QUARK > > > >Changes in v2: > >* Fix several commit write-up grammar, choice of words. > >* Ensure "int ret" in correct order > >* Add comment to explain DTS register field read/write bit operation > >* Change to Dual BSD/GPL license > >* Add logic to ensure safe trip point threshold value being set > > > >Ong Boon Leong (1): > > thermal: intel Quark S
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Andy Lutomirski [mailto:l...@amacapital.net] > Sent: Friday, March 06, 2015 7:09 AM > > On Mar 5, 2015 1:19 AM, "Kweh, Hock Leong" > wrote: > > > > > > This really is not a big deal. User should cope with it. > > > > > > No, it's a big deal, and the user should not cope. > > > > > > The user *should not* be required to have write access to anything in > > > /lib to install a UEFI capsule that they download from their > > > motherboard vendor's website. /lib belongs to the distro, and UEFI > > > capsules do not belong to the distro. In this regard, UEFI capsules > > > are completely unlike your wireless card firmware, your cpu microcode, > > > etc. > > > > > > Imagine systems using NFS root, Atomic-style systems (e.g. ostree), > > > systems that boot off squashfs, etc. They should still be able to > > > load capsules. The basic user interface that should work is: > > > > > > # uefi-load-capsule /path/to/capsule > > > > > > or: > > > > > > # uefi-load-capsule - > > > > > I don't really care how uefi-load-capsule is implemented, as long as > > > it's straightforward, because people will screw it up if it isn't > > > straightforward. > > > > > > Why is it so hard to have a file in sysfs that you write the capsule > > > to using *cat* (not echo) and that will return an error code if cat > > > fails? Is it because you don't know where the end of the capsule is? > > > if so, ioctl is designed for exactly this purpose. > > > > > > TBH, I find this thread kind of ridiculous. The problem that you're > > > trying to solve is extremely simple, the functionality that userspace > > > needs is trivial, and all of these complex proposals for how it should > > > work are an artifact of the fact that the kernel-internal interfaces > > > you're using for it are not well suited to the problem at hand. > > > > > > --Andy > > > > Sorry, I may not catch your point correctly. Are you trying to tell that > > a "normal" user can perform efi capsule update. But a "normal" user > > does not have the right to install or copy the capsule binary into > > "/lib/firmware/". So, there is a need to make this capsule module to > > allow uploading the capsule binary at any path or location other than > > "/lib/firmware/". > > > > Is this what you mean? > > No. Only root should be able to load capsules, but even root may not > be able to write to /lib. > > --Andy > Okay, I accept that only root user can perform the load capsule. It is new to me that root user may not have the access right to "/lib/firmware". But I remember you do mention that CPU micro code and wifi firmware they are different and able to write in /lib/firmware. I am curious why efi capsule binary have such a restriction. What is preventing it being write to that location? I even went to check out the FHS: http://en.wikipedia.org/wiki/Filesystem_Hierarchy_Standard I do not find any restriction calling out for that. Would you mind to educate me on that? Thanks. Regards, Wilson
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message-
> From: Borislav Petkov [mailto:b...@alien8.de]
> Sent: Friday, March 06, 2015 4:14 PM
>
> On Thu, Mar 05, 2015 at 03:08:42PM -0800, Andy Lutomirski wrote:
> > No. Only root should be able to load capsules, but even root may not
> > be able to write to /lib.
>
> So basically what we want to do is:
>
> # cat /any/path/to/efi/capsule/accessible/to/root/efi_capsule.img >
> /sys/firmware/efi/update
>
> Now it can't get any simpler than that and you get error codes too by
> failing the cat if the update fails.
>
> Mind you, I'm using '#' and not '$' as a shell prompt :-)
>
> --
> Regards/Gruss,
> Boris.
>
> ECO tip #101: Trim your mails when you reply.
> --
I believe you guys are more paying attention to the PATH and
whether doing:
# cat /any/path/capsule.bin > /sys/devices/platform/efi_capsule/capsule_load
or doing:
# echo "/any/path/capsule.bin" > /sys/devices/platform/efi_capsule/capsule_load
is not a big issue right?
Regards,
Wilson
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Andy Lutomirski [mailto:l...@amacapital.net] > Sent: Wednesday, March 04, 2015 4:38 AM > > On Mon, Mar 2, 2015 at 9:56 PM, Kweh, Hock Leong > wrote: > > > > Just to call out that using firmware class auto locate binary feature is > > limited > > to locations: > > - "/lib/firmware/updates/" UTS_RELEASE, > > - "/lib/firmware/updates", > > - "/lib/firmware/" UTS_RELEASE, > > - "/lib/firmware" > > and one custom path which inputted during firmware_class module load > > time or kernel boot up time. > > > > It is just not like the user helper interface which allow load the binary at > > any path/location. > > > > This really is not a big deal. User should cope with it. > > No, it's a big deal, and the user should not cope. > > The user *should not* be required to have write access to anything in > /lib to install a UEFI capsule that they download from their > motherboard vendor's website. /lib belongs to the distro, and UEFI > capsules do not belong to the distro. In this regard, UEFI capsules > are completely unlike your wireless card firmware, your cpu microcode, > etc. > > Imagine systems using NFS root, Atomic-style systems (e.g. ostree), > systems that boot off squashfs, etc. They should still be able to > load capsules. The basic user interface that should work is: > > # uefi-load-capsule /path/to/capsule > > or: > > # uefi-load-capsule - > I don't really care how uefi-load-capsule is implemented, as long as > it's straightforward, because people will screw it up if it isn't > straightforward. > > Why is it so hard to have a file in sysfs that you write the capsule > to using *cat* (not echo) and that will return an error code if cat > fails? Is it because you don't know where the end of the capsule is? > if so, ioctl is designed for exactly this purpose. > > TBH, I find this thread kind of ridiculous. The problem that you're > trying to solve is extremely simple, the functionality that userspace > needs is trivial, and all of these complex proposals for how it should > work are an artifact of the fact that the kernel-internal interfaces > you're using for it are not well suited to the problem at hand. > > --Andy Sorry, I may not catch your point correctly. Are you trying to tell that a "normal" user can perform efi capsule update. But a "normal" user does not have the right to install or copy the capsule binary into "/lib/firmware/". So, there is a need to make this capsule module to allow uploading the capsule binary at any path or location other than "/lib/firmware/". Is this what you mean? Regards, Wilson
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Matt Fleming [mailto:m...@console-pimps.org] > Sent: Monday, March 02, 2015 8:30 PM > > On Mon, 02 Mar, at 10:59:00AM, Kweh Hock Leong wrote: > > > -Original Message- > > > From: Borislav Petkov [mailto:b...@alien8.de] > > > Sent: Wednesday, February 25, 2015 8:49 PM > > > > > > On Wed, Feb 25, 2015 at 12:38:20PM +, Kweh, Hock Leong wrote: > > > > The reason we use this interface for efi capsule is that efi capsule > > > > support multi binaries to be uploaded and each binary file name > > > > can be different. > > > > > > So you can write the file path to a second file and reload then, once > > > per file. > > > > Thanks for the suggestion. Did some exploration on this approach and > > would like to continue the discussion together with this suggested design. > > > > Ideal condition: > > - one file note is enough for load binary and status return (capsule_load) > > - load steps would be as simple as just: > >echo [binary file name] > capsule_load > > - status return in the same command action. If failed, the echo will fail > >with the failing status code. > > > > Trade off: > > - does not have the run time flexibility to load any firmware binaries at > >different different path as firmware class only support one custom > >path inputted during boot time/load time. Unless to add new export > >function for firmware class. > > Could you elaborate here? I don't understand this point. Just to call out that using firmware class auto locate binary feature is limited to locations: - "/lib/firmware/updates/" UTS_RELEASE, - "/lib/firmware/updates", - "/lib/firmware/" UTS_RELEASE, - "/lib/firmware" and one custom path which inputted during firmware_class module load time or kernel boot up time. It is just not like the user helper interface which allow load the binary at any path/location. This really is not a big deal. User should cope with it. > > > - if any typo on user level or file path not found, firmware class falls > > back > >to user helper interface. User is required to open another terminal to > >performs: > >-> echo 1 > loading > >-> cat capsule.bin > data > >-> echo 0 > loading > >Or wait for timeout. > > Not if you use request_firmware_direct(), right? > request_firmware_direct() explicitly does not invoke the user helper. > > > - User has the capability to configure the firmware_class time out value. > >If the infinite value is set, the only method to break the infinite > > waiting > >by manually perform "echo -1 > loading" on the other terminal. > > I'm not sure this is a problem if we use request_firmware_direct(). Oops... I miss out this function. Will rework using this function. > > > Are you guys okay with these trade off? > > Or you guys still okay with the previous 2 design idea? > > I quite like the design that Borislav is proposing. Things become a lot > simpler when we stop using request_firmware_nowait(). > > The next question is whether we want to batch passing capsules to the > firmware. The UpdateCapsule() EFI runtime service allows you to chain > capsule blobs together and pass a scatter-gather list. > > If we do want to batch uploading then we need the equivalent of the > 'reload' file from the microcode loader to signal to the kernel that the > userland process has finished uploading capsules, and the kernel can now > send them to the firmware as one chunk. > > Also, Andy previously raised the point about multiple userland processes > passing capsules to the firmware simultaneously and the races that > occur, so we'd need a way to make that atomic. > > I'd much prefer to send one capsule at a time to the firmware, because > it just makes this whole thing simpler. > > Wilson, I'm really looking for your input here with how capsule > uploading works on Quark. If we can just repeatedly call UpdateCapsule() > with one capsule file at a time, that's fine. If we absolutely must > bundle multiple capsule files together then we probably need to provide > some atomicity. > > Thoughts? Quark does not need batch uploading. Even I tested multiple capsules on Quark by doing repeatedly calling UpdateCapsule() is still working for Quark. So, Quark does not require this bundling. > > > > Alternatively, you can combine all the files into a cpio archive like > > > we do with the microcode loader too, and let the kernel parse the cpio >
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Borislav Petkov [mailto:b...@alien8.de] > Sent: Wednesday, February 25, 2015 8:49 PM > > On Wed, Feb 25, 2015 at 12:38:20PM +0000, Kweh, Hock Leong wrote: > > The reason we use this interface for efi capsule is that efi capsule > > support multi binaries to be uploaded and each binary file name > > can be different. > > So you can write the file path to a second file and reload then, once > per file. Thanks for the suggestion. Did some exploration on this approach and would like to continue the discussion together with this suggested design. Ideal condition: - one file note is enough for load binary and status return (capsule_load) - load steps would be as simple as just: echo [binary file name] > capsule_load - status return in the same command action. If failed, the echo will fail with the failing status code. Trade off: - does not have the run time flexibility to load any firmware binaries at different different path as firmware class only support one custom path inputted during boot time/load time. Unless to add new export function for firmware class. - if any typo on user level or file path not found, firmware class falls back to user helper interface. User is required to open another terminal to performs: -> echo 1 > loading -> cat capsule.bin > data -> echo 0 > loading Or wait for timeout. - User has the capability to configure the firmware_class time out value. If the infinite value is set, the only method to break the infinite waiting by manually perform "echo -1 > loading" on the other terminal. Are you guys okay with these trade off? Or you guys still okay with the previous 2 design idea? > > Alternatively, you can combine all the files into a cpio archive like > we do with the microcode loader too, and let the kernel parse the cpio > archive. I believe this will make the implementation complicated compare to above. Regards, Wilson
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message-
> From: Roy Franz [mailto:roy.fr...@linaro.org]
> Sent: Friday, February 27, 2015 1:07 PM
>
> On Sun, Nov 2, 2014 at 7:07 PM, Kweh Hock Leong
> > +/*
> > + * This function will store the capsule binary and pass it to
> > + * efi_capsule_update() API in capsule.c */ static int
> > +efi_capsule_store(const struct firmware *fw) {
> > + int i;
> > + int ret;
> > + int count = fw->size;
> > + int copy_size = (fw->size > PAGE_SIZE) ? PAGE_SIZE : fw->size;
> > + int pages_needed = ALIGN(fw->size, PAGE_SIZE) >> PAGE_SHIFT;
> > + struct page **pages;
> > + void *page_data;
> > + efi_capsule_header_t *capsule = NULL;
> > +
> > + pages = kmalloc_array(pages_needed, sizeof(void *), GFP_KERNEL);
> > + if (!pages) {
> > + pr_err("%s: kmalloc_array() failed\n", __func__);
> > + return -ENOMEM;
> > + }
> > +
> > + for (i = 0; i < pages_needed; i++) {
> > + pages[i] = alloc_page(GFP_KERNEL);
> > + if (!pages[i]) {
> > + pr_err("%s: alloc_page() failed\n", __func__);
> > + --i;
> > + ret = -ENOMEM;
> > + goto failed;
> > + }
> > + }
> > +
> > + for (i = 0; i < pages_needed; i++) {
> > + page_data = kmap(pages[i]);
> > + memcpy(page_data, fw->data + (i * PAGE_SIZE),
> > + copy_size);
> > +
> > + if (i == 0)
> > + capsule = (efi_capsule_header_t *)page_data;
> > + else
> > + kunmap(pages[i]);
> > +
> > + count -= copy_size;
> > + if (count < PAGE_SIZE)
> > + copy_size = count;
> > + }
> > +
> > + ret = efi_capsule_update(capsule, pages);
> > + if (ret) {
> > + pr_err("%s: efi_capsule_update() failed\n", __func__);
> > + --i;
>
> Hi Hock,
>
> What does the decrement of i do here? I looked at
> efi_capsule_update() and didn't see anything that would account for this. It
> looks like in this failure case one page won't get freed.
>
> As an aside, when I was looking at efi_update_capsule, I see that Matt is
> doing very similar operations (array of struct page pointers), but does it
> like
> this (snipped from his patch):
>
> + struct page **block_pgs;
> ...
> + block_pgs = kzalloc(nr_block_pgs * sizeof(*block_pgs), GFP_KERNEL);
> + if (!block_pgs)
> + return -ENOMEM;
> +
> + for (i = 0; i < nr_block_pgs; i++) {
> + block_pgs[i] = alloc_page(GFP_KERNEL);
> + if (!block_pgs[i])
> + goto fail;
> + }
>
> and then can simply free the pages that are not NULL:
> +fail:
> + for (i = 0; i < nr_block_pgs; i++) {
> + if (block_pgs[i])
> + __free_page(block_pgs[i]);
> + }
>
> I think this way is preferable since it doesn't rely on 'i' being unchanged
> at the
> end of the function. I also think it would be nice if the capsule code stuck
> with one idiom for dealing with arrays of page pointers.
>
> Roy
>
Hi Roy,
The reason "i" there have to perform a decrement is because a full for loop
will end with one extra incremented value if it does not break out in the
middle.
And the efi_capsule_store()'s alloc page is to store the binary content and the
efi_capsule_update() from Matt is to store the efi capsule block descriptor
which will point to the binary blocks content. So, they are different.
Regards,
Wilson
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Borislav Petkov [mailto:b...@alien8.de] > Sent: Wednesday, February 25, 2015 7:48 PM > > On Tue, Feb 24, 2015 at 12:49:09PM +0000, Kweh, Hock Leong wrote: > > So this sounds pretty overengineered for no reason, or maybe I'm missing > the reason. > > If I had to give an example from the microcode loader, what we do there > is put the microcode in /lib/firmware/... and do > > echo 1 > /sys/devices/system/cpu/microcode/reload > > which goes and calls reload_store() in > arch/x86/kernel/cpu/microcode/core.c which grabs a mutex, disables CPU > hotplug, etc, etc... > > And this mechanism is as simple as it can get. Maybe capsules can be > loaded like that too? > > Error code can be propagated too, if needed, of course. > > -- > Regards/Gruss, > Boris. > > ECO tip #101: Trim your mails when you reply. > -- Hi Boris, The reason we use this interface for efi capsule is that efi capsule support multi binaries to be uploaded and each binary file name can be different. Regards, Wilson
RE: Re: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Kweh, Hock Leong > Sent: Tuesday, February 24, 2015 6:54 PM > > In callbackfn_efi_capsule, you call request_firmware_nowait. When that > callback is invoked, I think that the /sys/class/firmware/efi-capsule-file > directory doesn't exist at all. > If the callback takes longer than it takes your script to make it through a > full > iteration, then it will try uploading the second capsule before the firmware > class directory is there, so it will fail. > > But I just realized that your script has a loop below to handle that. > It's this: > > oldtime=$(date +%S) > oldtime=$(((time + 2) % 60)) > until [ -f /sys/class/firmware/efi-capsule-file/loading ] > do > newtime=$(date +%S) > if [ $newtime -eq $oldtime ] > then > break > fi > done > > Aside from the fact that this loop itself is racy (it may loop forever if > something goes wrong in the kernel, since $newtime -eq $oldtime may > never happen), it should help, if you're lucky. But there's another bug. > > > Here's the race: > > User: > echo 1 > /sys/class/firmware/efi-capsule-file/loading > cat capsule1 > /sys/class/firmware/efi-capsule-file/data > echo 0 > /sys/class/firmware/efi-capsule-file/loading > Kernel: Be a little slow here due to preemption or whatever. > > User: > -f /sys/class/firmware/efi-capsule-file/loading returns true capsules_loaded > == 0 Assume failure, incorrectly > > Kernel: catch up and increment capsules_loaded. > > If these patches get applied, then I think that the protocol needs to be > documented in Documentation/ABI. It should say something like: > > To upload an EFI capsule, do this: > > Write 1 to /sys/class/firmware/efi-capsule-file/loading > Write the capsule to /sys/class/firmware/efi-capsule-file/data > Write 0 to /sys/class/firmware/efi-capsule-file/loading > > Make sure that /sys/class/firmware/efi-capsule-file disappears and comes > back, perhaps by cd-ing there and waiting for all the files in the directory > to > go away. > > Then, and only then, read capsules_loaded to detect success. > > > Once you've written that doc, please seriously consider whether this > interface is justifiable. I think it sucks. > > --Andy Hi All, After some internal discussion and re-design prototyping & testing on this efi capsule interface kernel module, I would like to start a discussion here on the new idea and wish to get input for the implementation and eventually upstream. This new idea will expose 2 read-only file notes from the efi capsule kernel module - "capsule_ticket" and "capsule_report". The "capsule_ticket" will let user to obtain a NON-ZERO number and then perform a mutex lock. The obtained number will be used later for "capsule_report" status checking. Unlock mutex is done after "echo 0 > loading" at the end of callback function. So the process steps basically look like this: 1.) cat capsule_ticket===> acquire a number and lock mutex then expose firmware_class user helper interface as well as start timer for timeout counting 2.) repeat step 1 if obtained a "0" number 3.) echo 1 > loading 4.) cat bin > data 5.) echo 0 > loading===> stop the timeout counting then unlock mutex at the end of callback routine 6.) cat capsule_report ===> grep the number acquired from beginning for checking succeeded/failed The ticket numbering is starting from 1 to 999 and then rolls over from 1 to 999 again. The "capsule_report" will show the whole list of the acquired entries and will be limited to 128 entries which is capped within one page buffer. The format of this "capsule_report" output will look like: "Capsule $num uploads successful/failed/aborted" There is a 2mins time out (internal) for each of the number acquired. After that, the interface will be closed/disappeared. I have attached a SAMPLE script and kernel module code in case you are not able to understand my description above. I believe this would really take care of the multi-capsule update concurrently issue and also asynchronous status reporting issue. Besides, this will indir
RE: [PATCH] thermal: intel Quark SoC X1000 DTS thermal driver
> -Original Message-
> From: Ong, Boon Leong
> Sent: Monday, February 23, 2015 9:39 AM
> Subject: RE: [PATCH] thermal: intel Quark SoC X1000 DTS thermal driver
>
> >Just to bring out for discussion, do you think we should put a "safety range"
> >for reporting out the critical trip temperature value (mean the value from
> >register minus 1 or 2 degree)?
> >
> >Just wondering if this is needed for the software to have the sufficient
> >shutdown time before the HW make a hard power cut off when the
> >critical trip point is reached.
>
> I assume that the suggestion is meant for the case where thermal register is
> not locked by BIOS. It is not a bad idea to have some protection against
> wrong configuration on critical trip point by user.
> Looking through the data-sheet in Quark, I could not find an recommended
> temperature. So, I propose that we use the same value set by BIOS today
> - 105C as the maximum.
What I mean here is that even the BIOS locks it and used the maximum value
105 °C for the critical trip point, should we -1 or -2 (104/103 °C) in this
driver
to let the system shut down before the actual trip point hit, in case the HW
performs a HW power cut off before the Linux kernel has enough time to
shut down properly?
>
> > > +static struct soc_sensor_entry *alloc_soc_dts(void)
> > > +{
> > > + struct soc_sensor_entry *aux_entry;
> > > + int err;
> > > + u32 out;
> > > + int wr_mask;
> > > +
> > > + aux_entry = kzalloc(sizeof(*aux_entry), GFP_KERNEL);
> >
> >Wondering is it possible to use the resource-managed functions (for e.g.
> >devm_kzalloc())? This could help the driver looks more neat and clean
> >where the resource-managed framework will help you take care all the
> >kfree().
> >
> >Understand that the flow here is to call the thermal_zone_device_register()
> >function after this aux_entry allocation.
> >
> >But thinking would it also working if change the flow to call
> >thermal_zone_device_register() function 1st to obtain the
> >thermal_zone_device
> >then later on perform devm_kzalloc() and assign it back to devdata.
> >
> Ok, it is worth exploring on this devm_kzalloc() for neatness.
> Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH] thermal: intel Quark SoC X1000 DTS thermal driver
> -Original Message- > From: linux-kernel-ow...@vger.kernel.org [mailto:linux-kernel- > ow...@vger.kernel.org] On Behalf Of Ong Boon Leong > Sent: Thursday, February 12, 2015 12:52 AM > To: Zhang, Rui; edubez...@gmail.com > Cc: linux...@vger.kernel.org; linux-kernel@vger.kernel.org > Subject: [PATCH] thermal: intel Quark SoC X1000 DTS thermal driver > > In Intel Quark SoC X1000, there is one on-die digital temperature sensor(DTS). > The DTS offers both hot & critical trip points. > > However, in current distribution of UEFI BIOS for Quark platform, only > critical trip point is configured to be 105 degree Celsius (based on Quark > SW ver1.0.1 and hot trip point is not used due to lack of IRQ. > > There is no active cooling device for Quark SoC, so Quark SoC thermal > management simply expects Linux distro to orderly power-off when > temperature > of DTS exceeds the configured critical trip point. > > Kernel param "polling_delay" in milli-second is used to control the frequency > DTS temperature is read by thermal framework. It is default to 2-second. To > change it, use kernal boot param "intel_quark_dts_thermal.polling_delay=X". > > User interacts with Quark SoC DTS thermal driver through sysfs at: > /sys/class/thermal/thermal_zone0/ > > For examples: > - to read DTS temperature >$ cat temp > - to read critical trip point >$ cat trip_point_0_temp > - to read trip point type >$ cat trip_point_0_type > - to emulate temperature raise to test orderly shutdown by Linux distro >$ echo 105 > emul_temp > > Signed-off-by: Ong Boon Leong > --- > drivers/thermal/Kconfig | 10 + > drivers/thermal/Makefile |1 + > drivers/thermal/intel_quark_dts_thermal.c | 408 > + > 3 files changed, 419 insertions(+) > create mode 100644 drivers/thermal/intel_quark_dts_thermal.c > > diff --git a/drivers/thermal/Kconfig b/drivers/thermal/Kconfig > index f554d25..b80f09f 100644 > --- a/drivers/thermal/Kconfig > +++ b/drivers/thermal/Kconfig > @@ -229,6 +229,16 @@ config INTEL_SOC_DTS_THERMAL > notification methods.The other trip is a critical trip point, which > was set by the driver based on the TJ MAX temperature. > > +config INTEL_QUARK_DTS_THERMAL > + tristate "Intel Quark DTS thermal driver" > + depends on X86 && IOSF_MBI > + help > + Enable this to register Intel Quark SoC (e.g. X1000) platform digital > + temperature sensor (DTS). For X1000 SoC, it has one on-die DTS. > + The DTS will be registered as a thermal zone. There are two trip > points: > + hot & critical. The critical trip point default value is set by > + underlying BIOS/Firmware. > + > config INT340X_THERMAL > tristate "ACPI INT340X thermal drivers" > depends on X86 && ACPI > diff --git a/drivers/thermal/Makefile b/drivers/thermal/Makefile > index 39c4fe8..50c5991 100644 > --- a/drivers/thermal/Makefile > +++ b/drivers/thermal/Makefile > @@ -31,6 +31,7 @@ obj-$(CONFIG_DB8500_CPUFREQ_COOLING)+= > db8500_cpufreq_cooling.o > obj-$(CONFIG_INTEL_POWERCLAMP) += intel_powerclamp.o > obj-$(CONFIG_X86_PKG_TEMP_THERMAL) += x86_pkg_temp_thermal.o > obj-$(CONFIG_INTEL_SOC_DTS_THERMAL) += intel_soc_dts_thermal.o > +obj-$(CONFIG_INTEL_QUARK_DTS_THERMAL)+= > intel_quark_dts_thermal.o > obj-$(CONFIG_TI_SOC_THERMAL) += ti-soc-thermal/ > obj-$(CONFIG_INT340X_THERMAL) += int340x_thermal/ > obj-$(CONFIG_ST_THERMAL) += st/ > diff --git a/drivers/thermal/intel_quark_dts_thermal.c > b/drivers/thermal/intel_quark_dts_thermal.c > new file mode 100644 > index 000..fe1e221 > --- /dev/null > +++ b/drivers/thermal/intel_quark_dts_thermal.c > @@ -0,0 +1,408 @@ > +/* > + * intel_quark_dts_thermal.c > + * Copyright (c) 2015, Intel Corporation. > + * > + * This program is free software; you can redistribute it and/or modify it > + * under the terms and conditions of the GNU General Public License, > + * version 2, as published by the Free Software Foundation. > + * > + * This program is distributed in the hope it will be useful, but WITHOUT > + * ANY WARRANTY; without even the implied warranty of > MERCHANTABILITY or > + * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public > License for > + * more details. > + * > + * Quark DTS thermal driver is implemented by referencing > + * intel_soc_dts_thermal.c. > + */ > + > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +#define X86_FAMILY_QUARK 0x5 > +#define X86_MODEL_QUARK_X10000x9 > + > +/* DTS reset is programmed via QRK_MBI_UNIT_SOC */ > +#define QRK_DTS_REG_OFFSET_RESET 0x34 > +#define QRK_DTS_RESET_BITBIT(0) > + > +/* DTS enable is programmed via QRK_MBI_UNIT_RMU */ > +#define QRK_DTS_REG_OFFSET_ENABLE0xB0 > +#define QRK_DTS_ENABLE_BIT BIT(15) > + > +/* Temperature Register is read via QRK_MBI_UN
RE: [PATCH v2 1/3] firmware loader: Introduce new API - request_firmware_abort()
> -Original Message-
> From: Matt Fleming [mailto:m...@console-pimps.org]
> Sent: Monday, November 17, 2014 11:12 PM
> >
> > - Only doing module unload is required to be aware of this synchronization
> > -> Ensuring the call back does not fall into unloaded code which may
> cause
> > undefined behavior.
> > -> Ensuring the put_device() & module_put() code have finished in
> firmware_class.c
> > function request_firmware_work_func() before the device is
> unregistered
> > and module unloaded happen.
>
> Shouldn't the existing module_{put,get}() and {put,get}_device() calls
> provide all the necessary synchronisation?
>
> Module unload should not be possible while other code is using the
> module (and the module refcnt has been incremented accordindly).
>
> Right?
>
> --
> Matt Fleming, Intel Open Source Technology Center
Hi Matt,
Yes, you are right. If the module refcount is not zero, you will get error
message and returned while you do "rmmod". But I strongly believe if we
have the capability in our code to take care of it by doing synchronization,
we should take care of it in case people are doing "rmmod -f". Don't
you think so?
Regards,
Wilson
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[PATCH v3] firmware loader: fix hung task warning dump
From: "Kweh, Hock Leong" When using request_firmware_nowait() with FW_ACTION_NOHOTPLUG param to expose user helper interface, if the user do not react immediately, after 120 seconds there will be a hung task warning message dumped as below: [ 3000.784235] INFO: task kworker/0:0:8259 blocked for more than 120 seconds. [ 3000.791281] Tainted: GE 3.16.0-rc1-yocto-standard #41 [ 3000.798082] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 3000.806072] kworker/0:0 D cd0075c8 0 8259 2 0x [ 3000.812765] Workqueue: events request_firmware_work_func [ 3000.818253] cd375e18 0046 000e cd0075c8 00f0 cd40ea00 cd375fec 1b883e89 [ 3000.826374] 026b cd40ea00 8000 0001 cd0075c8 cd375de4 c119917f [ 3000.834492] cd563360 cd375df4 c119a0ab cd563360 cd375e24 c119a1d6 [ 3000.842616] Call Trace: [ 3000.845252] [] ? kernfs_next_descendant_post+0x3f/0x50 [ 3000.851543] [] ? kernfs_activate+0x6b/0xc0 [ 3000.856790] [] ? kernfs_add_one+0xd6/0x130 [ 3000.862047] [] schedule+0x22/0x60 [ 3000.866548] [] schedule_timeout+0x175/0x1d0 [ 3000.871887] [] ? __kernfs_create_file+0x71/0xa0 [ 3000.877574] [] ? sysfs_add_file_mode_ns+0xaa/0x180 [ 3000.883533] [] wait_for_completion+0x6f/0xb0 [ 3000.888961] [] ? wake_up_process+0x40/0x40 [ 3000.894219] [] _request_firmware+0x750/0x9f0 [ 3000.899666] [] ? n_tty_receive_buf2+0x1f/0x30 [ 3000.905200] [] request_firmware_work_func+0x22/0x50 [ 3000.911235] [] process_one_work+0x122/0x380 [ 3000.916571] [] worker_thread+0xf9/0x470 [ 3000.921555] [] ? create_and_start_worker+0x50/0x50 [ 3000.927497] [] ? create_and_start_worker+0x50/0x50 [ 3000.933448] [] kthread+0x9f/0xc0 [ 3000.937850] [] ret_from_kernel_thread+0x20/0x30 [ 3000.943548] [] ? kthread_worker_fn+0x100/0x100 This patch change the wait_for_completion() function call to wait_for_completion_interruptible() function call for solving the issue. Cc: Matt Fleming Signed-off-by: Kweh, Hock Leong Acked-by: Ming Lei Acked-by: Greg Kroah-Hartman --- changelog v3: * Breaked this independent patch from efi_capsule_user_helper patchset for upstream. * Added Acked-by. drivers/base/firmware_class.c |4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c index d276e33..fe61f19 100644 --- a/drivers/base/firmware_class.c +++ b/drivers/base/firmware_class.c @@ -908,7 +908,7 @@ static int _request_firmware_load(struct firmware_priv *fw_priv, kobject_uevent(&fw_priv->dev.kobj, KOBJ_ADD); } - wait_for_completion(&buf->completion); + retval = wait_for_completion_interruptible(&buf->completion); cancel_delayed_work_sync(&fw_priv->timeout_work); if (!buf->data) @@ -985,7 +985,7 @@ static int sync_cached_firmware_buf(struct firmware_buf *buf) break; } mutex_unlock(&fw_lock); - wait_for_completion(&buf->completion); + ret = wait_for_completion_interruptible(&buf->completion); mutex_lock(&fw_lock); } mutex_unlock(&fw_lock); -- 1.7.9.5 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
RE: [PATCH v2 1/3] firmware loader: Introduce new API - request_firmware_abort()
> -Original Message- > From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] > Sent: Sunday, November 09, 2014 3:07 AM > > > > > Besides aborting through user helper interface, a new API > > request_firmware_abort() allows kernel driver module to abort the > > request_firmware() / request_firmware_nowait() when they are no longer > > needed. It is useful for cancelling an outstanding firmware load if > > initiated from inside a module where that module is in the process of > > being unloaded, since the unload function may not have a handle to the > > struct firmware_buf. > > > > Note for people who use request_firmware_nowait(): > > You are required to do your own synchronization after you call > > request_firmware_abort() in order to continue unloading the module. > > The example synchronization code shows below: > > > > while (THIS_MODULE && module_refcount(THIS_MODULE)) > > msleep(20); > > As others have pointed out, this isn't ok, and is totally racy and should > never > end up in a driver. Never mess with THIS_MODULE from within the same > module, otherwise it's racy and broken code. > > So can you please rework this to not require this? > > thanks, > > greg k-h Hi everyone, First of all, I would like to apologize if my commit message gives you guys an impression that to use request_firmware_abort(), you guys MUST do the synchronization on your own. But the fact is, it is not a MUST. Below will provide more detail. Regarding this synchronization topic, I would like to open a discussion to get a better approach to handle this problem. Before jumping onto the design, I would like to give a background of why I am doing in this way. - Only doing module unload is required to be aware of this synchronization -> Ensuring the call back does not fall into unloaded code which may cause undefined behavior. -> Ensuring the put_device() & module_put() code have finished in firmware_class.c function request_firmware_work_func() before the device is unregistered and module unloaded happen. - Not all the situations are required to do this synchronization: -> Implementation that use request_firmware() do not need this synchronization due to it will get synced by returning at the same place the caller call request_firmware(). -> Implementation that will not unload the call back code and without relying device refcount / module refcount do not need this synchronization (for e.g. calling the request_firmware_nowait() without passing the MODULE and DEVICE parameters as showed below: request_firmware_nowait(NULL, FW_ACTION_NOHOTPLUG, "xxx", NULL, GFP_KERNEL, NULL, callbackfn);). - Following the original request_firmware_nowait() design thought -> Strongly believe the original design of request_firmware_nowait() that using the get_device(), put_device(), try_get_module() & module_put() also expect the caller side to do the synchronization themselves if they have relying on these counter to continue their works. Let's take out this newly introduce API (request_firmware_abort()) and focus only on the original design (request_firmware_nowait()). If there is a caller design that after the callback happen and it need to do platform_device_unregister(), the original design also expect the caller do its own synchronization before it can do the device unregister. - Use cases that do not need the synchronization: -> Depend on a volatile condition in order to expose firmware upload interface: Like a design only in a particular window period then open the interface, when outside of the window then it need to disable the interface. This design also does not need the synchronization as it do not unload the callback module code and not relying on the device refcount or module refcount to do it stuff. -> System reboot: When system reboot, the system would not unload the module code and also would not unregister the device. So it does not meet the conditions (unload call back code and have dependency on device refcount / module refcount). This does not need the synchronization. Due to the above reasons, in summary, I think the caller has the responsibility to take care of this synchronization whenever needed on the caller side. What do you guys think? Come back to the design if really need to implement it in firmware_class. Below shows some design thought: - Complexity of implementing the synchronization inside firmware_class -> Cannot use module refcount or device refcount to be the synchronization method due to: (1) firmware_work data struct will get free
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Andy Lutomirski [mailto:l...@amacapital.net] > > #!/bin/sh > > > > old=$(cat > > /sys/devices/platform/efi_capsule_user_helper/capsule_loaded) > > > > for arg in "$@" > > do > > if [ -f $arg ] > > then > > echo 1 > /sys/class/firmware/efi-capsule-file/loading > > cat $arg > /sys/class/firmware/efi-capsule-file/data > > echo 0 > /sys/class/firmware/efi-capsule-file/loading > > I think you have a race. Try putting msleep(1000) after the > request_firmware_nowait call, and I bet this will fail on the second try. Sorry for the late response. I don't really catch the race condition that you are referring? Are you trying to tell that the user script could run faster before the previous callback function actually end? Will such scenario happen? In the callback function, after the request_firmware_nowait(), I don't have any codes will delay the callback function to end. Besides, there is a mutex_lock protecting the request_firmware_nowait() calling. Won't that take care of the issue? > > > > > oldtime=$(date +%S) > > oldtime=$(((time + 2) % 60)) > > until [ -f /sys/class/firmware/efi-capsule-file/loading ] > > do > > newtime=$(date +%S) > > if [ $newtime -eq $oldtime ] > > then > > break > > fi > > done > > > > old=$((old + 1)) > > new=$(cat > > /sys/devices/platform/efi_capsule_user_helper/capsule_loaded) > > I think that firmware_class doesn't call the callback until after loading is > closed > for the second time. If so, then this is racy. Try inserting msleep(1000) > at the > beginning of your callback and uploading a capsule that should load > successfully -- this will report failure, but a future upload may get very > confused. Also, what does the firmware class do when simultaneous > uploads of the same file with different contents are in flight? Is that > possible? Sorry again, I can't really catch you on this race condition statement. Are you trying to tell if user is doing this: echo 1 > /sys/class/firmware/efi-capsule-file/loading cat capsule1 > /sys/class/firmware/efi-capsule-file/data cat capsule2 > /sys/class/firmware/efi-capsule-file/data echo 0 > /sys/class/firmware/efi-capsule-file/loading If so, capsule2 will be the one we will obtain in the callback function. Regards, Wilson
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message-
> From: Andy Lutomirski [mailto:l...@amacapital.net]
> Sent: Wednesday, November 05, 2014 12:36 AM
>
> Am I missing something here? The current proposal is missing the
> success/failure part, unless you count the loaded count (in a different sysfs
> directory) as a useful interface for that.
Here is my sample shell script which allow me to do multi capsule binaries
upload
and obtain error message if error occur:
#!/bin/sh
old=$(cat /sys/devices/platform/efi_capsule_user_helper/capsule_loaded)
for arg in "$@"
do
if [ -f $arg ]
then
echo 1 > /sys/class/firmware/efi-capsule-file/loading
cat $arg > /sys/class/firmware/efi-capsule-file/data
echo 0 > /sys/class/firmware/efi-capsule-file/loading
oldtime=$(date +%S)
oldtime=$(((time + 2) % 60))
until [ -f /sys/class/firmware/efi-capsule-file/loading ]
do
newtime=$(date +%S)
if [ $newtime -eq $oldtime ]
then
break
fi
done
old=$((old + 1))
new=$(cat
/sys/devices/platform/efi_capsule_user_helper/capsule_loaded)
if [ ! $new -eq $old ]
then
echo "Capsule binary $arg upload failed"
dmesg | tail | grep -v platform | grep -e efi
exit 1
fi
else
echo "File $arg not found !!"
fi
done
exit 0
Regards,
Wilson
N�r��yb�X��ǧv�^�){.n�+{zX����ܨ}���Ơz�&j:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
RE: [PATCH v2 0/3] Enable user helper interface for efi capsule update
> -Original Message- > From: Fleming, Matt > Sent: Tuesday, November 04, 2014 10:08 PM > To: Greg Kroah-Hartman > > > > Good point, I don't know. > > > > Who ever wrote this code should know this, can someone please provide > > a use-case for how this is all supposed to work? > > That would be Wilson. He's wanting to use this to send updates to the > firmware on the Intel Quark, I think. Hi Matt, I believe Greg already got that from the commit message on my patch 3/3. Thanks. Regards, Wilson
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Andy Lutomirski [mailto:l...@amacapital.net] > Sent: Tuesday, November 04, 2014 2:32 PM > > It seems like a large fraction of the code in this module exists just to work > around the fact that request_firmware doesn't do what you want it to do. > You have code to: > > - Prevent the /lib/firmware mechanism from working. > - Avoid a deadlock by doing strange things in the unload code. > - Allow more than one capsule per module load. (Isn't this hard to use? > User code will have to wait for the next firmware request before sending a > second capsule.) I did try to upload more than one capsule binaries and I don't observe a long wait is required. In fact, it seem to me the interface is instantly re-created. > > All of this is for dubious gain. You have to do three separate opens in > sysfs to > upload a capsule, and there's no way to report back to userspace whether > the EFI call worked and whether a reboot is needed. I have not encounter any capsule update that does not require reboot. Base on my understanding, the EFI firmware only do the binary decoding during the next reboot. If the binary is broken/corrupted, you would only know during the next reboot. On kernel driver point of view, it just registers the binary by calling the EFI API UpdateCapsule(). May be Matt you could help out with this? Regarding the EFI call failed message, besides obtains from the dmesg log, you also can verify that through this sysfs: /sys/devices/platform/efi_capsule_user_helper/capsule_loaded I did mention this in the commit message as showed below: Besides the request_firmware user helper interface, this module also exposes a 'capsule_loaded' file note for user to verify the number of successfully uploaded capsule binaries. This file note has the read only attribute. > > What's the benefit of using the firmware interface here? > > --Andy
RE: [PATCH v2 3/3] efi: Capsule update with user helper interface
> -Original Message- > From: Andy Lutomirski [mailto:l...@amacapital.net] > > > > Andy, here's the steps to load a capsule. I don't have a problem with > > this, it's userspace driven, no "autoloading" of files in /lib/, and > > works with sysfs. > > > > Have any objection to it, I don't. Thanks Greg for helping me on the explanation. I would like to apologize if my cover letter/commit messages did misleading. > > After reading the code, I still have objections. > > The full workflow seems to be, from the user's POV: > > 1. load the module > > 2. hope that there isn't a file called /lib/firmware/efi-capsule-file > > 3. echo 1 >.../loading > > 4. cat firmware >.../data > > 5. echo 0 >.../loading > > 6. efi_update_capsule gets called. The return value ends up in the kernel > logs somewhere but doesn't seem to make it to userspace. > > 7. reboot(), but only if the capsule you loaded requires a reboot, which may > or may not be detectable without the kernel's help (I'm not sure about this > point). > > If you want to load a second capsule (which seems like a reasonable thing to > do if the first capsule was the kind that is processed immediately), then > rmmod -f the module and start over? You no need to do rmmod in order to upload the 2nd capsule binary. You just need to do the 3 steps as you mentioned in your 3, 4 and 5 for your 2nd or 3rd capsule binaries. Then the last, you do the reboot. > > This seems like an unpleasant interface. I think that ioctl or a dedicated > custom sysfs file would be considerably nicer. It would allow you to upload a > capsule and get an indication of success or failure all at once, and it lets > you > load more than one capsule. > Also, it gets rid of some of the really weird module refcounting stuff that's > going on -- the only unusual thing the module would do would be to pin itself > once a reboot-requiring capsule is loaded. > > --Andy > Regarding the synchronization, it is only required for module unload. The code is designed in such a way that allow to be built as a kernel module or built into the kernel. If you choose the built in kernel method, you won't come into the module unload situation which require the synchronization. Regards, Wilson
[PATCH v2 3/3] efi: Capsule update with user helper interface
From: "Kweh, Hock Leong"
Introducing a kernel module to expose user helper interface for
user to upload capsule binaries. This module leverage the
request_firmware_nowait() to expose an interface to user.
Example steps to load the capsule binary:
1.) echo 1 > /sys/class/firmware/efi-capsule-file/loading
2.) cat capsule.bin > /sys/class/firmware/efi-capsule-file/data
3.) echo 0 > /sys/class/firmware/efi-capsule-file/loading
Whereas, this module does not allow the capsule binaries to be
obtained from the request_firmware library search path. If any
capsule binary loaded from firmware seach path, the module will
stop functioning.
Besides the request_firmware user helper interface, this module
also expose a 'capsule_loaded' file note for user to verify
the number of successfully uploaded capsule binaries. This
file note has the read only attribute.
Cc: Matt Fleming
Signed-off-by: Kweh, Hock Leong
---
drivers/firmware/efi/Kconfig | 13 ++
drivers/firmware/efi/Makefile |1 +
drivers/firmware/efi/efi-capsule-user-helper.c | 246
3 files changed, 260 insertions(+)
create mode 100644 drivers/firmware/efi/efi-capsule-user-helper.c
diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index f712d47..7dc814e 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -60,6 +60,19 @@ config EFI_RUNTIME_WRAPPERS
config EFI_ARMSTUB
bool
+config EFI_CAPSULE_USER_HELPER
+ tristate "EFI capsule user mode helper"
+ depends on EFI
+ select FW_LOADER
+ select FW_LOADER_USER_HELPER
+ help
+ This option exposes the user mode helper interface for user to load
+ EFI capsule binary and update the EFI firmware after system reboot.
+ This feature does not support auto locating capsule binaries at the
+ firmware lib search path.
+
+ If unsure, say N.
+
endmenu
config UEFI_CPER
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index 698846e..63f6910 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -8,3 +8,4 @@ obj-$(CONFIG_UEFI_CPER) += cper.o
obj-$(CONFIG_EFI_RUNTIME_MAP) += runtime-map.o
obj-$(CONFIG_EFI_RUNTIME_WRAPPERS) += runtime-wrappers.o
obj-$(CONFIG_EFI_STUB) += libstub/
+obj-$(CONFIG_EFI_CAPSULE_USER_HELPER) += efi-capsule-user-helper.o
diff --git a/drivers/firmware/efi/efi-capsule-user-helper.c
b/drivers/firmware/efi/efi-capsule-user-helper.c
new file mode 100644
index 000..84a1628
--- /dev/null
+++ b/drivers/firmware/efi/efi-capsule-user-helper.c
@@ -0,0 +1,246 @@
+/*
+ * EFI capsule user mode helper interface driver.
+ *
+ * Copyright 2014 Intel Corporation
+ *
+ * This file is part of the Linux kernel, and is made available under
+ * the terms of the GNU General Public License version 2.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define CAPSULE_NAME "efi-capsule-file"
+#define DEV_NAME "efi_capsule_user_helper"
+#define STRING_INTEGER_MAX_LENGTH 13
+
+static DEFINE_MUTEX(user_helper_lock);
+static int capsule_count;
+static int stop_request;
+static struct platform_device *efi_capsule_pdev;
+
+/*
+ * This function will store the capsule binary and pass it to
+ * efi_capsule_update() API in capsule.c
+ */
+static int efi_capsule_store(const struct firmware *fw)
+{
+ int i;
+ int ret;
+ int count = fw->size;
+ int copy_size = (fw->size > PAGE_SIZE) ? PAGE_SIZE : fw->size;
+ int pages_needed = ALIGN(fw->size, PAGE_SIZE) >> PAGE_SHIFT;
+ struct page **pages;
+ void *page_data;
+ efi_capsule_header_t *capsule = NULL;
+
+ pages = kmalloc_array(pages_needed, sizeof(void *), GFP_KERNEL);
+ if (!pages) {
+ pr_err("%s: kmalloc_array() failed\n", __func__);
+ return -ENOMEM;
+ }
+
+ for (i = 0; i < pages_needed; i++) {
+ pages[i] = alloc_page(GFP_KERNEL);
+ if (!pages[i]) {
+ pr_err("%s: alloc_page() failed\n", __func__);
+ --i;
+ ret = -ENOMEM;
+ goto failed;
+ }
+ }
+
+ for (i = 0; i < pages_needed; i++) {
+ page_data = kmap(pages[i]);
+ memcpy(page_data, fw->data + (i * PAGE_SIZE), copy_size);
+
+ if (i == 0)
+ capsule = (efi_capsule_header_t *)page_data;
+ else
+ kunmap(pages[i]);
+
+ count -= copy_size;
+ if (count < PAGE_SIZE)
+ copy_size = count;
+ }
+
+ ret = e
