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 <hock.leong.k...@intel.com>; Jan Kiszka > <jan.kis...@siemens.com>; Andy Shevchenko <andy.shevche...@gmail.com> > Cc: Matt Fleming <m...@codeblueprint.co.uk>; Ard Biesheuvel > <ard.biesheu...@linaro.org>; linux-...@vger.kernel.org; Linux Kernel Mailing > List <linux-kernel@vger.kernel.org>; Borislav Petkov <b...@alien8.de> > 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 <andy.shevche...@gmail.com> > >> Cc: Matt Fleming <m...@codeblueprint.co.uk>; Ard Biesheuvel > >> <ard.biesheu...@linaro.org>; linux-...@vger.kernel.org; Linux Kernel > >> Mailing List <linux-kernel@vger.kernel.org>; Borislav Petkov > >> <b...@alien8.de>; Kweh, Hock Leong <hock.leong.k...@intel.com>; Bryan > >> O'Donoghue <pure.lo...@nexus-software.ie> > >> 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 > >>>> <jan.kis...@siemens.com> > >> 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: 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 <andy.shevche...@gmail.com> > Cc: Matt Fleming <m...@codeblueprint.co.uk>; Ard Biesheuvel > <ard.biesheu...@linaro.org>; linux-...@vger.kernel.org; Linux Kernel Mailing > List <linux-kernel@vger.kernel.org>; Borislav Petkov <b...@alien8.de>; Kweh, > Hock Leong <hock.leong.k...@intel.com>; Bryan O'Donoghue > <pure.lo...@nexus-software.ie> > 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 <jan.kis...@siemens.com> > 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
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" <hock.leong.k...@intel.com> 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 <hock.leong.k...@intel.com> --- 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
[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 <hock.leong.k...@intel.com>
> Cc: David S. Miller <da...@davemloft.net>; Joao Pinto
> <joao.pi...@synopsys.com>; Giuseppe CAVALLARO <peppe.cavall...@st.com>;
> seraphin.bonna...@st.com; Jarod Wilson <ja...@redhat.com>; Alexandre
> TORGUE <alexandre.tor...@gmail.com>; Joachim Eastwood
> <manab...@gmail.com>; Niklas Cassel <niklas.cas...@axis.com>; Johan Hovold
> <jo...@kernel.org>; Pavel Machek <pa...@ucw.cz>; lars.pers...@axis.com;
> netdev <net...@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>
> 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
> <hock.leong.k...@intel.com> 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", >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 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", >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 <hock.leong.k...@intel.com> > Cc: David S. Miller <da...@davemloft.net>; Joao Pinto > <joao.pi...@synopsys.com>; Giuseppe CAVALLARO <peppe.cavall...@st.com>; > seraphin.bonna...@st.com; Jarod Wilson <ja...@redhat.com>; Alexandre > TORGUE <alexandre.tor...@gmail.com>; Joachim Eastwood > <manab...@gmail.com>; Niklas Cassel <niklas.cas...@axis.com>; Johan Hovold > <jo...@kernel.org>; Pavel Machek <pa...@ucw.cz>; lars.pers...@axis.com; > netdev <net...@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org> > 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 > <hock.leong.k...@intel.com> wrote: > > From: "Kweh, Hock Leong" <hock.leong.k...@intel.com> > > > > 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
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" <hock.leong.k...@intel.com> 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 <hock.leong.k...@intel.com> --- 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
[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 <hock.leong.k...@intel.com>
> Cc: David S. Miller <da...@davemloft.net>; Joao Pinto
> <joao.pi...@synopsys.com>; Giuseppe CAVALLARO <peppe.cavall...@st.com>;
> seraphin.bonna...@st.com; Jarod Wilson <ja...@redhat.com>; Alexandre
> TORGUE <alexandre.tor...@gmail.com>; Joachim Eastwood
> <manab...@gmail.com>; Niklas Cassel <niklas.cas...@axis.com>; Johan Hovold
> <jo...@kernel.org>; Pavel Machek <pa...@ucw.cz>; lars.pers...@axis.com;
> netdev <net...@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>
> 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
> <hock.leong.k...@intel.com> wrote:
> > From: "Kweh, Hock Leong" <hock.leong.k...@intel.com>
> >
> > 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: 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 <hock.leong.k...@intel.com>; David S. Miller
> <da...@davemloft.net>; Joao Pinto <joao.pi...@synopsys.com>; Giuseppe
> CAVALLARO <peppe.cavall...@st.com>; seraphin.bonna...@st.com; Jarod
> Wilson <ja...@redhat.com>; Andy Shevchenko <andy.shevche...@gmail.com>
> Cc: Alexandre TORGUE <alexandre.tor...@gmail.com>; Joachim Eastwood
> <manab...@gmail.com>; Niklas Cassel <niklas.cas...@axis.com>; Johan Hovold
> <jo...@kernel.org>; pa...@ucw.cz; lars.pers...@axis.com; netdev
> <net...@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org>
> 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" <hock.leong.k...@intel.com>
> >
> > 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 <hock.leong.k...@intel.com>
> > ---
> > 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) {
> >
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" <hock.leong.k...@intel.com>
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 <hock.leong.k...@intel.com>
---
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
[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 <da...@davemloft.net>; Joao Pinto > <joao.pi...@synopsys.com>; Giuseppe CAVALLARO <peppe.cavall...@st.com>; > seraphin.bonna...@st.com; Jarod Wilson <ja...@redhat.com>; Andy > Shevchenko <andy.shevche...@gmail.com> > Cc: Alexandre TORGUE <alexandre.tor...@gmail.com>; Joachim Eastwood > <manab...@gmail.com>; Niklas Cassel <niklas.cas...@axis.com>; Johan Hovold > <jo...@kernel.org>; pa...@ucw.cz; Kweh, Hock Leong > <hock.leong.k...@intel.com>; lars.pers...@axis.com; netdev > <net...@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org> > Subject: [PATCH v2] net: stmmac: fix maxmtu assignment to be within valid > range > > From: "Kweh, Hock Leong" <hock.leong.k...@intel.com> > > 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 <hock.leong.k...@intel.com> 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
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" <hock.leong.k...@intel.com> 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 <hock.leong.k...@intel.com> --- 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 <hock.leong.k...@intel.com> > Cc: David S. Miller <da...@davemloft.net>; Joao Pinto > <joao.pi...@synopsys.com>; Giuseppe CAVALLARO <peppe.cavall...@st.com>; > seraphin.bonna...@st.com; Jarod Wilson <ja...@redhat.com>; Alexandre > TORGUE <alexandre.tor...@gmail.com>; Joachim Eastwood > <manab...@gmail.com>; Niklas Cassel <niklas.cas...@axis.com>; Johan Hovold > <jo...@kernel.org>; Pavel Machek <pa...@ucw.cz>; lars.pers...@axis.com; > netdev <net...@vger.kernel.org>; LKML <linux-kernel@vger.kernel.org> > 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 > <hock.leong.k...@intel.com> wrote: > > From: "Kweh, Hock Leong" <hock.leong.k...@intel.com> > > > > 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
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" <hock.leong.k...@intel.com> 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 <hock.leong.k...@intel.com> --- 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
[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 <f.faine...@gmail.com>; 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 <boon.leong@intel.com>; Voon,
> Weifeng <weifeng.v...@intel.com>; lars.pers...@axis.com; linux-
> ker...@vger.kernel.org; Giuseppe Cavallaro <peppe.cavall...@st.com>;
> Alexandre Torgue <alexandre.tor...@st.com>
> 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 <f.faine...@gmail.com>
> ---
Acked-by: Kweh, Hock Leong <hock.leong.k...@intel.com>
> 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(>napi);
> error_hw_init:
> --
> 2.9.3
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(>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 <hock.leong.k...@intel.com>; David Miller > <da...@davemloft.net> > 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 <boon.leong@intel.com>; Voon, Weifeng > <weifeng.v...@intel.com>; 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 <hock.leong.k...@intel.com> > >> 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 <boon.leong@intel.com>; Voon, Weifeng > >> <weifeng.v...@intel.com>; 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" <hock.leong.k...@intel.com> > >> Date: Tue, 27 Dec 2016 22:42:36 +0800 > >> > >>> From: "Kweh, Hock Leong" <hock.leong.k...@intel.com> > >> > >> 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: 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 <hock.leong.k...@intel.com> > Cc: David Miller <da...@davemloft.net>; 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 > <hock.leong.k...@intel.com> wrote: > >> -Original Message- > >> From: David Miller [mailto:da...@davemloft.net] > >> Sent: Wednesday, December 28, 2016 12:34 AM > >> To: Kweh, Hock Leong <hock.leong.k...@intel.com> > >> 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 <boon.leong@intel.com>; Voon, Weifeng > >> <weifeng.v...@intel.com>; 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" <hock.leong.k...@intel.com> > >> Date: Tue, 27 Dec 2016 22:42:36 +0800 > >> > >> > From: "Kweh, Hock Leong" <hock.leong.k...@intel.com> > >> > >> 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: 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 <hock.leong.k...@intel.com> > 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 > <boon.leong@intel.com>; Voon, Weifeng <weifeng.v...@intel.com>; > 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" <hock.leong.k...@intel.com> > Date: Tue, 27 Dec 2016 22:42:36 +0800 > > > From: "Kweh, Hock Leong" <hock.leong.k...@intel.com> > > 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(>napi); Vs + + return ret; error_mdio_register: - unregister_netdev(ndev); -error_netdev_register: netif_napi_del(>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. 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(>napi); Vs + + return ret; error_mdio_register: - unregister_netdev(ndev); -error_netdev_register: netif_napi_del(>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 <hock.leong.k...@intel.com> > 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 > <boon.leong@intel.com>; Voon, Weifeng <weifeng.v...@intel.com>; > 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" <hock.leong.k...@intel.com> > Date: Tue, 27 Dec 2016 22:42:36 +0800 > > > From: "Kweh, Hock Leong" <hock.leong.k...@intel.com> > > 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
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" <hock.leong.k...@intel.com> 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 <hock.leong.k...@intel.com> --- 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] 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" <hock.leong.k...@intel.com>
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 <f.faine...@gmail.com>
Signed-off-by: Kweh, Hock Leong <hock.leong.k...@intel.com>
---
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(>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(>napi);
error_hw_init:
clk_disable_unprepare(priv->pclk);
--
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(>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(>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 <hock.leong.k...@intel.com> > 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 <boon.leong@intel.com>; > net...@vger.kernel.org; linux-kernel@vger.kernel.org; Voon, Weifeng > <weifeng.v...@intel.com>; lars.pers...@axis.com > Subject: Re: [PATCH] net: stmmac: synchronize stmmac_open and > stmmac_dvr_probe > > From: "Kweh, Hock Leong" <hock.leong.k...@intel.com> > Date: Tue, 27 Dec 2016 19:44:59 +0800 > > > From: "Kweh, Hock Leong" <hock.leong.k...@intel.com> > > > > 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 <hock.leong.k...@intel.com> > > 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: 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 <hock.leong.k...@intel.com>; David S. Miller
> <da...@davemloft.net>; Joao Pinto <joao.pi...@synopsys.com>; Giuseppe
> CAVALLARO <peppe.cavall...@st.com>; seraphin.bonna...@st.com
> Cc: Alexandre TORGUE <alexandre.tor...@gmail.com>; Joachim Eastwood
> <manab...@gmail.com>; Niklas Cassel <niklas.cas...@axis.com>; Johan Hovold
> <jo...@kernel.org>; pa...@ucw.cz; Ong, Boon Leong
> <boon.leong@intel.com>; netdev <net...@vger.kernel.org>; LKML ker...@vger.kernel.org>; Voon, Weifeng <weifeng.v...@intel.com>; Lars
> Persson <lars.pers...@axis.com>
> 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" <hock.leong.k...@intel.com>
> >>
> >> 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(>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(>napi);
> error_hw_init:
> clk_disable_unprepare(priv->pclk);
>
> --
> Florian
Thanks. Will try out to confirm.
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(>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(>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" <hock.leong.k...@intel.com>
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 <hock.leong.k...@intel.com>
---
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(>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(>probe_done);
stmmac_set_ethtool_ops(ndev);
priv->pause = pause;
@@ -3372,6 +3381,7 @@ int stmmac_dvr_probe(struct device *device,
}
}
+ complete_all(>probe_done);
return 0;
error_mdio_register:
--
1.7.9.5
[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(>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(>probe_done);
stmmac_set_ethtool_ops(ndev);
priv->pause = pause;
@@ -3372,6 +3381,7 @@ int stmmac_dvr_probe(struct device *device,
}
}
+ complete_all(>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" <hock.leong.k...@intel.com>
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 <hock.leong.k...@intel.com>
---
.../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
[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
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 <hock.leong.k...@intel.com>
AuthorDate: Mon, 25 Apr 2016 21:07:01 +0100
Committer: Ingo Molnar <mi...@kernel.org>
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 <hock.leong.k...@intel.com>
[ Update comments and Kconfig text ]
Signed-off-by: Matt Fleming <m...@codeblueprint.co.uk>
Reviewed-by: Bryan O'Donoghue <pure.lo...@nexus-software.ie>
Acked-by: Greg Kroah-Hartman <gre...@linuxfoundation.org>
Cc: Andy Lutomirski <l...@amacapital.net>
Cc: Ard Biesheuvel <ard.biesheu...@linaro.org>
Cc: Borislav Petkov <b...@alien8.de>
Cc: Peter Jones <pjo...@redhat.com>
Cc: Peter Zijlstra <pet...@infradead.org>
Cc: Sam Protsenko <semen.protse...@linaro.org>
Cc: Thomas Gleixner <t...@linutronix.de>
Cc: joeyli <j...@suse.com>
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 <mi...@kernel.org>
---
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;
+
[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) {
+ pr_e
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" <hock.leong.k...@intel.com> > > > > 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 <m...@codeblueprint.co.uk> > > Signed-off-by: Kweh, Hock Leong <hock.leong.k...@intel.com> > > --- > > 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
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,
+ _info->reset_type);
+ if (ret) {
+ pr_err("%s: efi_capsule_supported() failed\n",
+ __func__);
+ return ret;
+ }
+
+ cap_info->total_size = cap_hdr->imagesize;
+
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...@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
[PATCH v11 1/1] efi: a misc char interface for user to update efi firmware
From: "Kweh, Hock Leong" <hock.leong.k...@intel.com>
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 <m...@codeblueprint.co.uk>
Signed-off-by: Kweh, Hock Leong <hock.leong.k...@intel.com>
---
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,
+ _info->reset_type);
+ if (ret) {
+ pr_err("%s: efi_capsule_supported() failed\n",
+ __func__);
+
[PATCH v11 0/1] Enable capsule loader interface for efi firmware updating
From: "Kweh, Hock Leong" <hock.leong.k...@intel.com> 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
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(_loader_lock);
> > + ret = efi_capsule_supported(cap_hdr->guid, cap_hdr
> > ->flags,
> > + cap_hdr->imagesize,
> > + _info->reset_type);
> > + mutex_unlock(_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
> -respect to calls into efi_capsule_supported() and
> efi_c
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" <hock.leong.k...@intel.com>
> >
> > 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 <m...@codeblueprint.co.uk>
> > Signed-off-by: Kweh, Hock Leong <hock.leong.k...@intel.com>
> > ---
> > 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 {
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" <hock.leong.k...@intel.com>
> >
> > 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(_loader_lock);
> > + ret = efi_capsule_supported(cap_hdr->guid, cap_hdr
> > ->flags,
> > + cap_hdr->imagesize,
> > + _info->reset_type);
> > + mutex_unlock(_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
> -respect to calls into
[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/
[PATCH v10 1/1] efi: a misc char interface for user to update efi firmware
From: "Kweh, Hock Leong" <hock.leong.k...@intel.com>
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 <m...@codeblueprint.co.uk>
Signed-off-by: Kweh, Hock Leong <hock.leong.k...@intel.com>
---
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", __fu
[PATCH v10 0/1] Enable capsule loader interface for efi firmware updating
From: "Kweh, Hock Leong" <hock.leong.k...@intel.com> 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�:+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(, 1, _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�:+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, 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�:+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(, 1, _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�:+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: 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�:+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�:+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.
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�:+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.
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 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�:+v���zZ+��+zf���h���~i���z� �w���?�&�)ߢf��^jǫy�m��@A�a���
0��h���i
[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/
[PATCH v9 0/1] Enable capsule loader interface for efi firmware updating
From: "Kweh, Hock Leong" <hock.leong.k...@intel.com> 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/
[PATCH v9 1/1] efi: a misc char interface for user to update efi firmware
From: "Kweh, Hock Leong" <hock.leong.k...@intel.com>
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 <matt.flem...@intel.com>
Signed-off-by: Kweh, Hock Leong <hock.leong.k...@intel.com>
---
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;
+
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 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(_loader_lock);
+ ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
+ cap_hdr->imagesize, _info->reset_type);
+ mutex_unlock(_loader_lock);
+ if (ret) {
+ pr_err("%s: efi_capsule_supported() failed\n", __func__);
+ return ret;
+ }
+
+ cap_info->total_size = cap_hdr->imagesize
[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 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" <hock.leong.k...@intel.com> This patch export efi_capsule_supported() function symbol for capsule kernel module to use. Cc: Matt Fleming <matt.flem...@intel.com> Signed-off-by: Kweh, Hock Leong <hock.leong.k...@intel.com> --- 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 0/2] Enable capsule loader interface for efi firmware updating
From: "Kweh, Hock Leong" <hock.leong.k...@intel.com> 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/
[PATCH v8 2/2] efi: a misc char interface for user to update efi firmware
From: "Kweh, Hock Leong" <hock.leong.k...@intel.com>
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 <matt.flem...@intel.com>
Signed-off-by: Kweh, Hock Leong <hock.leong.k...@intel.com>
---
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(_loader_lock);
+ ret = efi_capsule_supported(cap_hdr->guid, cap_hdr->flags,
+ cap_hdr->imagesize, _info->reset_type);
+ mutex_unlock(_loader_lock);
+ if (ret) {
+ pr_err("%s: efi_capsule_supported() failed\n", __func__);
+
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
