Re: [PATCH] net: ethtool: don't require CAP_NET_ADMIN for ETHTOOL_GLINKSETTINGS

[Vincent Bernat]

 ❦ 24 novembre 2016 10:55 +0100, Miroslav Lichvar  :

> The ETHTOOL_GLINKSETTINGS command is deprecating the ETHTOOL_GSET
> command and likewise it shouldn't require the CAP_NET_ADMIN
> capability.

Could this patch be pushed to stable branches too?
-- 
Each module should do one thing well.
- The Elements of Programming Style (Kernighan & Plauger)

[PATCH v2] ipv4: Namespaceify tcp_tw_reuse knob

[Haishuang Yan]

Different namespaces might have different requirements to reuse
TIME-WAIT sockets for new connections. This might be required in
cases where different namespace applications are in place which
require TIME_WAIT socket connections to be reduced independently
of the host.

Signed-off-by: Haishuang Yan 

---
Changes in v2:
  - Make the commit message more clearer.
---
 include/net/netns/ipv4.h   |  1 +
 include/net/tcp.h  |  1 -
 net/ipv4/sysctl_net_ipv4.c | 14 +++---
 net/ipv4/tcp_ipv4.c|  4 ++--
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index f0cf5a1..0378e88 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -110,6 +110,7 @@ struct netns_ipv4 {
int sysctl_tcp_orphan_retries;
int sysctl_tcp_fin_timeout;
unsigned int sysctl_tcp_notsent_lowat;
+   int sysctl_tcp_tw_reuse;
 
int sysctl_igmp_max_memberships;
int sysctl_igmp_max_msf;
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 207147b..6061963 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -252,7 +252,6 @@
 extern int sysctl_tcp_rmem[3];
 extern int sysctl_tcp_app_win;
 extern int sysctl_tcp_adv_win_scale;
-extern int sysctl_tcp_tw_reuse;
 extern int sysctl_tcp_frto;
 extern int sysctl_tcp_low_latency;
 extern int sysctl_tcp_nometrics_save;
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 80bc36b..22cbd61 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -433,13 +433,6 @@ static int proc_tcp_fastopen_key(struct ctl_table *ctl, 
int write,
.extra2 = _adv_win_scale_max,
},
{
-   .procname   = "tcp_tw_reuse",
-   .data   = _tcp_tw_reuse,
-   .maxlen = sizeof(int),
-   .mode   = 0644,
-   .proc_handler   = proc_dointvec
-   },
-   {
.procname   = "tcp_frto",
.data   = _tcp_frto,
.maxlen = sizeof(int),
@@ -960,6 +953,13 @@ static int proc_tcp_fastopen_key(struct ctl_table *ctl, 
int write,
.mode   = 0644,
.proc_handler   = proc_dointvec,
},
+   {
+   .procname   = "tcp_tw_reuse",
+   .data   = _net.ipv4.sysctl_tcp_tw_reuse,
+   .maxlen = sizeof(int),
+   .mode   = 0644,
+   .proc_handler   = proc_dointvec
+   },
 #ifdef CONFIG_IP_ROUTE_MULTIPATH
{
.procname   = "fib_multipath_use_neigh",
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 30d81f5..fe9da4f 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -84,7 +84,6 @@
 #include 
 #include 
 
-int sysctl_tcp_tw_reuse __read_mostly;
 int sysctl_tcp_low_latency __read_mostly;
 
 #ifdef CONFIG_TCP_MD5SIG
@@ -120,7 +119,7 @@ int tcp_twsk_unique(struct sock *sk, struct sock *sktw, 
void *twp)
   and use initial timestamp retrieved from peer table.
 */
if (tcptw->tw_ts_recent_stamp &&
-   (!twp || (sysctl_tcp_tw_reuse &&
+   (!twp || (sock_net(sk)->ipv4.sysctl_tcp_tw_reuse &&
 get_seconds() - tcptw->tw_ts_recent_stamp > 1))) {
tp->write_seq = tcptw->tw_snd_nxt + 65535 + 2;
if (tp->write_seq == 0)
@@ -2456,6 +2455,7 @@ static int __net_init tcp_sk_init(struct net *net)
net->ipv4.sysctl_tcp_orphan_retries = 0;
net->ipv4.sysctl_tcp_fin_timeout = TCP_FIN_TIMEOUT;
net->ipv4.sysctl_tcp_notsent_lowat = UINT_MAX;
+   net->ipv4.sysctl_tcp_tw_reuse = 0;
 
return 0;
 fail:
-- 
1.8.3.1

Re: [PATCH] ipv4: Namespaceify tcp_tw_reuse knob

[David Miller]

From: Haishuang Yan 
Date: Sat, 24 Dec 2016 20:43:07 +0800

> Signed-off-by: Haishuang Yan 

You need to provide something more than an empty commit message.

Instead, the commit message must explain why this particular
sysctl should be considered for namespacification and what
the implications, both good and bad, are for such a change.

Re: [PATCH v3 1/2] drivers: net: ethernet: 3com: fix return value

[David Miller]

It is never, ever, appropriate to use the same exact Subject: line
text for two different changes.

Someone looking at "git shortlog" has no way to know what is different
between the two changes.

You must put care and time into constructing Subject: lines because
this text is critical for data mining and analysis done by both humans
and machines.

[PATCH v3 1/2] drivers: net: ethernet: 3com: fix return value

[Thomas Preisner]

In a few cases the err-variable is not set to a negative error code if a
function call fails and thus 0 is returned instead.
It may be better to set err to the appropriate negative error code
before returning.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=188841

Reported-by: Pan Bian 
Signed-off-by: Thomas Preisner 
Signed-off-by: Milan Stephan 
---
 drivers/net/ethernet/3com/typhoon.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/3com/typhoon.c 
b/drivers/net/ethernet/3com/typhoon.c
index a0cacbe..c88b88a 100644
--- a/drivers/net/ethernet/3com/typhoon.c
+++ b/drivers/net/ethernet/3com/typhoon.c
@@ -2404,6 +2404,7 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 
-   if(!is_valid_ether_addr(dev->dev_addr)) {
+   if (!is_valid_ether_addr(dev->dev_addr)) {
err_msg = "Could not obtain valid ethernet address, aborting";
+   err = -EIO;
goto error_out_reset;
}
 
@@ -2411,7 +2412,8 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 * later when we print out the version reported.
 */
INIT_COMMAND_WITH_RESPONSE(_cmd, TYPHOON_CMD_READ_VERSIONS);
-   if(typhoon_issue_command(tp, 1, _cmd, 3, xp_resp) < 0) {
+   err = typhoon_issue_command(tp, 1, _cmd, 3, xp_resp);
+   if (err < 0) {
err_msg = "Could not get Sleep Image version";
goto error_out_reset;
}
@@ -2453,7 +2455,8 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
dev->features = dev->hw_features |
NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_RXCSUM;
 
-   if(register_netdev(dev) < 0) {
+   err = register_netdev(dev);
+   if (err < 0) {
err_msg = "unable to register netdev";
goto error_out_reset;
}
-- 
2.7.4

Re: Re: [PATCH v2 1/2] drivers: net: ethernet: 3com: fix return value

[Thomas Preisner]

Hello.

On Sat, 2016-12-24 at 20:06 +0100, Sergei Shtylyov wrote:
>Hello!
>
>On 12/24/2016 03:02 PM, Thomas Preisner wrote:
>
>> In a few cases the err-variable is not set to a negative error code if a
>> function call fails and thus 0 is returned instead.
>> It may be better to set err to the appropriate negative error code
>> before returning.
>>
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=188841
>>
>> Reported-by: Pan Bian 
>> Signed-off-by: Thomas Preisner 
>> Signed-off-by: Milan Stephan 
>> ---
>>  drivers/net/ethernet/3com/typhoon.c | 7 +--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/3com/typhoon.c 
>> b/drivers/net/ethernet/3com/typhoon.c
>> index a0cacbe..c88b88a 100644
>> --- a/drivers/net/ethernet/3com/typhoon.c
>> +++ b/drivers/net/ethernet/3com/typhoon.c
>[...]
>> @@ -2411,7 +2412,8 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
>> pci_device_id *ent)
>>   * later when we print out the version reported.
>>   */
>>  INIT_COMMAND_WITH_RESPONSE(_cmd, TYPHOON_CMD_READ_VERSIONS);
>> -if(typhoon_issue_command(tp, 1, _cmd, 3, xp_resp) < 0) {
>> +err = typhoon_issue_command(tp, 1, _cmd, 3, xp_resp);
>> +if(err < 0) {
>
>Need a space between *if* and (. Run your patches thru 
>scripts/checkpatch.pl before posting, please.

Those spaces were actually left out purposely: The file in question (typhoon.c)
is missing those spaces between the statements (if, for, while) and the
following opening bracket pretty much always (except 2-3 times) and we figured
that it might be better to keep the coding style consistent since this might
aswell have been intended by the original author.

>
>>  err_msg = "Could not get Sleep Image version";
>>  goto error_out_reset;
>>  }
>> @@ -2453,7 +2455,8 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
>> pci_device_id *ent)
>>  dev->features = dev->hw_features |
>>  NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_RXCSUM;
>>
>> -if(register_netdev(dev) < 0) {
>> +err = register_netdev(dev);
>> +if(err < 0) {
>
>Same here.
>
>[...]
>
>MBR, Sergei

But of course we can provide you with a patchset including those spaces.

With Regards,
Milan and Thomas

[PATCH v3 2/2] drivers: net: ethernet: 3com: fix return value

[Thomas Preisner]

In some cases the return value of a failing function is not being used
and the function typhoon_init_one() returns another negative error
code instead.

Signed-off-by: Thomas Preisner 
Signed-off-by: Milan Stephan 
---
 drivers/net/ethernet/3com/typhoon.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/3com/typhoon.c 
b/drivers/net/ethernet/3com/typhoon.c
index c88b88a..8821a24 100644
--- a/drivers/net/ethernet/3com/typhoon.c
+++ b/drivers/net/ethernet/3com/typhoon.c
@@ -2370,9 +2370,9 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 * 4) Get the hardware address.
 * 5) Put the card to sleep.
 */
-   if (typhoon_reset(ioaddr, WaitSleep) < 0) {
+   err = typhoon_reset(ioaddr, WaitSleep);
+   if (err < 0) {
err_msg = "could not reset 3XP";
-   err = -EIO;
goto error_out_dma;
}
 
@@ -2386,16 +2386,16 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
typhoon_init_interface(tp);
typhoon_init_rings(tp);
 
-   if(typhoon_boot_3XP(tp, TYPHOON_STATUS_WAITING_FOR_HOST) < 0) {
+   err = typhoon_boot_3XP(tp, TYPHOON_STATUS_WAITING_FOR_HOST);
+   if (err < 0) {
err_msg = "cannot boot 3XP sleep image";
-   err = -EIO;
goto error_out_reset;
}
 
INIT_COMMAND_WITH_RESPONSE(_cmd, TYPHOON_CMD_READ_MAC_ADDRESS);
-   if(typhoon_issue_command(tp, 1, _cmd, 1, xp_resp) < 0) {
+   err = typhoon_issue_command(tp, 1, _cmd, 1, xp_resp);
+   if (err < 0) {
err_msg = "cannot read MAC address";
-   err = -EIO;
goto error_out_reset;
}
 
@@ -2430,9 +2430,9 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
if(xp_resp[0].numDesc != 0)
tp->capabilities |= TYPHOON_WAKEUP_NEEDS_RESET;
 
-   if(typhoon_sleep(tp, PCI_D3hot, 0) < 0) {
+   err = typhoon_sleep(tp, PCI_D3hot, 0);
+   if (err < 0) {
err_msg = "cannot put adapter to sleep";
-   err = -EIO;
goto error_out_reset;
}
 
-- 
2.7.4

Re: [PATCHv2 1/5] sh_eth: add generic wake-on-lan support via magic packet

[Sergei Shtylyov]

Hello!

On 12/19/2016 08:11 PM, Geert Uytterhoeven wrote:


One quirk needed for WoL is that the module clock needs to be prevented
from being switched off by Runtime PM. To keep the clock alive the


   I tried to find the code in question and failed, getting muddled in the
RPM maze. Could you point at this code for my education? :-)


In my investigation I observed this (simplified) call graph with regards
to clocks for suspend:

pm_suspend


   There's a long list of the calls skipped here. :-)


  pm_clk_suspend
clk_disable
  clk_core_disable
cpg_mstp_clock_disable

The interesting function here are clk_core_disable(). In that function a
'enable_count' for each clock is decremented and the clock is only
turned of if the count reaches zero, hence cpg_mstp_clock_disable() are
only called if the counter reaches 0. At runtime the enable_count can be
displayed by examining /sys/kernel/debug/clk/clk_summary.


   Well, this is not new to me... it's more interesting how we get there... :-)

[...]

usage count of the clock. Then when Runtime PM decreases the clock usage
count it won't reach 0 and be switched off.


   You mean it does this even though we don't call pr_runtime_put_sync()
as done in sh_eth_close()?


Yes.

I had a look at the pm_runtime_* functions in include/linux/pm_runtime.h
and drivers/base/power/runtime.c and could not find any clock handling.
Maybe they only deal with power domains?


There should be a generic way to prevent a device from being suspended.


   Indeed.


This will make sure the module clock is not disabled, and the power domain
(if applicable) is not powered down.


   I've just bumped into , it looks promising...

[...]

Gr{oetje,eeting}s,

Geert


MBR, Sergei

Re: [PATCH net] net, sched: fix soft lockup in tc_classify

[Daniel Borkmann]

On 12/24/2016 08:34 AM, Cong Wang wrote:

On Thu, Dec 22, 2016 at 4:26 PM, Daniel Borkmann  wrote:

On 12/22/2016 08:05 PM, Cong Wang wrote:

On Wed, Dec 21, 2016 at 1:07 PM, Daniel Borkmann 
wrote:


Ok, you mean for net. In that case I prefer the smaller sized fix to be
honest. It also covers everything from the point where we fetch the chain
via cops->tcf_chain() to the end of the function, which is where most of
the complexity resides, and only the two mentioned commits do the relock,


I really wish the problem is only about relocking, but look at the code,
the deeper reason why we have this bug is the complexity of the logic
inside tc_ctl_tfilter(): 1) the replay logic is hard, we have to make it
idempotent; 2) the request logic itself is hard, because of tc filter
design
and implementation.

This is why I worry more than just relocking.


But do you have a concrete 2nd issue/bug you're seeing? It rather sounds to
me your argument is more about fear of complexity on tc framework itself.
I agree it's complex, and tc_ctl_tfilter() is quite big in itself, where it
would be good to reduce it's complexity into smaller pieces. But it's not
really related to the fix itself, reducing complexity requires significantly
more and deeper work on the code. We can rework tc_ctl_tfilter() in net-next
to try to simplify it, sure, but I don't get why we have to discuss so much
on this matter in this context, really.


Thanks for ignoring my point 1) above... You are dragging the discussion
further.


I don't think so. The analysis and patch I proposed provides an explanation
of how we get into the seen endless loop, it provides a logical fix for it,
which has been reviewed by others and it has been tested extensively that it
resolves the issue, which was easily reproducible for the reporter and that
after the fix it never occurred again. The delta is absolutely simple and
really low risk. Given this function has not much changed over time, also
distros could pick it up that have a much older base kernel than current
stable ones. This initiated follow-up discussion we're having here in general
is dragging the focus away for everyone, and quite frankly I'm getting tired
of discussing it. I have stated my preferences, you have stated yours, and
we're only repeating ourselves in circles which isn't helpful in any way,
the discussion is not about some concrete bug in the logic to fix anymore
(otherwise please name it). Hence my proposal that everything else can wait
and be done in net-next.

Re: [RFC PATCH 4.10 3/6] bpf: Use SHA256 instead of SHA1 for bpf digests

[Daniel Borkmann]

On 12/24/2016 03:22 AM, Andy Lutomirski wrote:

BPF digests are intended to be used to avoid reloading programs that
are already loaded.  For use cases (CRIU?) where untrusted programs
are involved, intentional hash collisions could cause the wrong BPF
program to execute.  Additionally, if BPF digests are ever used
in-kernel to skip verification, a hash collision could give privilege
escalation directly.


Just for the record, digests will never ever be used to skip the
verification step, so I don't know why this idea even comes up
here (?) or is part of the changelog? As this will never be done
anyway, rather drop that part so we can avoid confusion on this?

Wrt untrusted programs, I don't see much of a use on this facility
in general for them. Something like a tail call map would quite
likely only be private to the application. And again, I really doubt
we'll have something like user namespace support in the foreseeable
future. Anyway, that said, I don't really have a big issue if you
want to switch to sha256, though.


SHA1 is no longer considered adequately collision-resistant (see, for
example, all the major browsers dropping support for SHA1
certificates).  Use SHA256 instead.

I moved the digest field to keep all of the bpf program metadata in
the same cache line.

Cc: Daniel Borkmann 
Cc: Alexei Starovoitov 
Signed-off-by: Andy Lutomirski 

Re: [PATCH v2 1/2] drivers: net: ethernet: 3com: fix return value

[Sergei Shtylyov]

Hello!

On 12/24/2016 03:02 PM, Thomas Preisner wrote:


In a few cases the err-variable is not set to a negative error code if a
function call fails and thus 0 is returned instead.
It may be better to set err to the appropriate negative error code
before returning.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=188841

Reported-by: Pan Bian 
Signed-off-by: Thomas Preisner 
Signed-off-by: Milan Stephan 
---
 drivers/net/ethernet/3com/typhoon.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/3com/typhoon.c 
b/drivers/net/ethernet/3com/typhoon.c
index a0cacbe..c88b88a 100644
--- a/drivers/net/ethernet/3com/typhoon.c
+++ b/drivers/net/ethernet/3com/typhoon.c

[...]

@@ -2411,7 +2412,8 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 * later when we print out the version reported.
 */
INIT_COMMAND_WITH_RESPONSE(_cmd, TYPHOON_CMD_READ_VERSIONS);
-   if(typhoon_issue_command(tp, 1, _cmd, 3, xp_resp) < 0) {
+   err = typhoon_issue_command(tp, 1, _cmd, 3, xp_resp);
+   if(err < 0) {


   Need a space between *if* and (. Run your patches thru 
scripts/checkpatch.pl before posting, please.



err_msg = "Could not get Sleep Image version";
goto error_out_reset;
}
@@ -2453,7 +2455,8 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
dev->features = dev->hw_features |
NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_RXCSUM;

-   if(register_netdev(dev) < 0) {
+   err = register_netdev(dev);
+   if(err < 0) {


   Same here.

[...]

MBR, Sergei

Re: [PATCH 6/6] wl1251: Set generated MAC address back to NVS data

[Pali Rohár]

On Saturday 24 December 2016 19:17:30 Pavel Machek wrote:
> Hi!
> 
> > In case there is no valid MAC address kernel generates random one.
> > This patch propagate this generated MAC address back to NVS data
> > which will be uploaded to wl1251 chip. So HW would have same MAC
> > address as linux kernel uses.
> > 
> > return 0;
> >  
> >  }
> > 
> > +static int wl1251_write_nvs_mac(struct wl1251 *wl)
> > +{
> 
> The name is quite confusing, this sounds like writing into
> non-volatile storage.
> 
> > +   int i;
> > +
> > +   if (wl->nvs_len < 0x24)
> > +   return -ENODATA;
> > +
> > +   /* length is 2 and data address is 0x546c (mask is 0xfffe) */
> 
> You don't actually check for the mask.

It is quite complicated. { 0x6d, 0x54 } (= 0x546d) in data represent 
address 0x546c and content are data. You need to apply mask 0xfffe for 
0x546d and you get address where data will be written (so 0x546c).

> > +   if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b]
> > != 0x54) +  return -EINVAL;
> 
> You have two copies of these. Does it make sense to move it to helper
> function?

I'm thinking if checks is really needed. But probably moving it to 
separate function is good idea.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH 5/6] wl1251: Parse and use MAC address from supplied NVS data

[Pali Rohár]

On Saturday 24 December 2016 19:14:21 Pavel Machek wrote:
> On Sat 2016-12-24 17:53:00, Pali Rohár wrote:
> > @@ -1581,10 +1598,16 @@ int wl1251_init_ieee80211(struct wl1251
> > *wl)
> > 
> > wl->hw->queues = 4;
> > 
> > +   if (wl->nvs == NULL && !wl->use_eeprom) {
> > +   ret = wl1251_fetch_nvs(wl);
> > +   if (ret < 0)
> > +   goto out;
> > +   }
> 
> Is goto out here good idea? IMNSHO it is copy bug, it should
> just proceed with generating random address.

No, goto is correct here. wl1251 cannot be initialized without NVS data. 
And when fetching (from userspace) fails it is fatal error.

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH 4/6] wl1251: Generate random MAC address only if driver does not have valid

[Pali Rohár]

On Saturday 24 December 2016 19:08:54 Pavel Machek wrote:
> On Sat 2016-12-24 17:52:59, Pali Rohár wrote:
> > Before this patch driver generated random MAC address every time
> > when was doing initialization. And after that random MAC address
> > could be overwritten with fixed one if provided.
> 
> Before this patch, driver generated random MAC address every time it
> was initialized. After that random MAC address could be overwritten
> with fixed one, if provided.
> 
> > This patch changes order. First it tries to read fixed MAC address
> > and if it fails then driver generates random MAC address.
> 
> I don't quite get where the advantage is supposed to be. Is it that
> "use_eeprom" is set, but reading fails?

Random bytes are read from kernel only if random MAC address is needed. 
And in wl->mac_addr is always either invalid address or permanenent mac 
address which will be used. Without patch in wl->mac_addr can be random 
temporary address for some time...

> The only case where this helps is if wl1251_read_eeprom_mac()
> succeeds but reads invalid address.
> 
> > Signed-off-by: Pali Rohár 
> 
> Acked-by: Pavel Machek 

-- 
Pali Rohár
pali.ro...@gmail.com


signature.asc
Description: This is a digitally signed message part.

Re: [PATCH 6/6] wl1251: Set generated MAC address back to NVS data

[Pavel Machek]

Hi!

> In case there is no valid MAC address kernel generates random one. This
> patch propagate this generated MAC address back to NVS data which will be
> uploaded to wl1251 chip. So HW would have same MAC address as linux kernel
> uses.

>   return 0;
>  }
>  
> +static int wl1251_write_nvs_mac(struct wl1251 *wl)
> +{

The name is quite confusing, this sounds like writing into
non-volatile storage.

> + int i;
> +
> + if (wl->nvs_len < 0x24)
> + return -ENODATA;
> +
> + /* length is 2 and data address is 0x546c (mask is 0xfffe) */

You don't actually check for the mask.

> + if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b] != 
> 0x54)
> + return -EINVAL;

You have two copies of these. Does it make sense to move it to helper
function?

Thanks,
Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH 5/6] wl1251: Parse and use MAC address from supplied NVS data

[Pavel Machek]

On Sat 2016-12-24 17:53:00, Pali Rohár wrote:
> This patch implements parsing MAC address from NVS data which are sent to
> wl1251 chip. Calibration NVS data could contain valid MAC address and it
> will be used instead randomly generated.

will be used instead of randomly generated one.

> This patch also move code for requesting NVS data from userspace to driver

"moves"

> initialization code to make sure that NVS data will be there at time when
> permanent MAC address is needed.

"at a time"

> Calibration NVS data for wl1251 are model specific. Every one device with

"device specific"? "Every device".

> wl1251 chip should have been calibrated in factory and needs to provide own
> calibration data.
> 
> Default example wl1251-nvs.bin data found in linux-firmware repository and

"are found"

> contains MAC address 00:00:20:07:03:09. So this MAC address is marked as

"contain"

> invalid as it is not real device specific address, just example one.
> 
> Format of calibration NVS data can be found at:
> http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt
> 
> Signed-off-by: Pali Rohár 
> ---
>  drivers/net/wireless/ti/wl1251/main.c |   39 
> ++---
>  1 file changed, 31 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/net/wireless/ti/wl1251/main.c 
> b/drivers/net/wireless/ti/wl1251/main.c
> index c3fa0b6..1454ba2 100644
> --- a/drivers/net/wireless/ti/wl1251/main.c
> +++ b/drivers/net/wireless/ti/wl1251/main.c
> @@ -205,13 +205,6 @@ static int wl1251_chip_wakeup(struct wl1251 *wl)
>   goto out;
>   }
>  
> - if (wl->nvs == NULL && !wl->use_eeprom) {
> - /* No NVS from netlink, try to get it from the filesystem */
> - ret = wl1251_fetch_nvs(wl);
> - if (ret < 0)
> - goto out;
> - }
> -
>  out:
>   return ret;
>  }
> @@ -1538,6 +1531,30 @@ static int wl1251_read_eeprom_mac(struct wl1251 *wl)
>   return 0;
>  }
>  
> +static int wl1251_read_nvs_mac(struct wl1251 *wl)
> +{
> + u8 mac[ETH_ALEN];
> + int i;
> +
> + if (wl->nvs_len < 0x24)
> + return -ENODATA;
> +
> + /* length is 2 and data address is 0x546c (mask is 0xfffe) */
> + if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b] != 
> 0x54)
> + return -EINVAL;
> +
> + /* MAC is stored in reverse order */
> + for (i = 0; i < ETH_ALEN; i++)
> + mac[i] = wl->nvs[0x1c + ETH_ALEN - i - 1];
> +
> + /* 00:00:20:07:03:09 is in default example wl1251-nvs.bin, so invalid */

remove "default".

> + if (ether_addr_equal_unaligned(mac, "\x00\x00\x20\x07\x03\x09"))
> + return -EINVAL;
> +
> + memcpy(wl->mac_addr, mac, ETH_ALEN);
> + return 0;
> +}
> +
>  static int wl1251_register_hw(struct wl1251 *wl)
>  {
>   int ret;
> @@ -1581,10 +1598,16 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
>  
>   wl->hw->queues = 4;
>  
> + if (wl->nvs == NULL && !wl->use_eeprom) {
> + ret = wl1251_fetch_nvs(wl);
> + if (ret < 0)
> + goto out;
> + }

Is goto out here good idea? IMNSHO it is copy bug, it should
just proceed with generating random address.

>   if (wl->use_eeprom)
>   ret = wl1251_read_eeprom_mac(wl);
>   else
> - ret = -EINVAL;
> + ret = wl1251_read_nvs_mac(wl);
>  
>   if (ret == 0 && !is_valid_ether_addr(wl->mac_addr))
>   ret = -EINVAL;

Best regards,
Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH 4/6] wl1251: Generate random MAC address only if driver does not have valid

[Pavel Machek]

On Sat 2016-12-24 17:52:59, Pali Rohár wrote:

> Before this patch driver generated random MAC address every time when was
> doing initialization. And after that random MAC address could be
> overwritten with fixed one if provided.

Before this patch, driver generated random MAC address every time it
was initialized. After that random MAC address could be overwritten
with fixed one, if provided.

> This patch changes order. First it tries to read fixed MAC address and if
> it fails then driver generates random MAC address.

I don't quite get where the advantage is supposed to be. Is it that
"use_eeprom" is set, but reading fails?

The only case where this helps is if wl1251_read_eeprom_mac() succeeds
but reads invalid address.

> Signed-off-by: Pali Rohár 

Acked-by: Pavel Machek 

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [PATCH 3/6] wl1251: Update wl->nvs_len after wl->nvs is valid

[Pavel Machek]

On Sat 2016-12-24 17:52:58, Pali Rohár wrote:
> In case kmemdup fails thne wl->nvs_len will contains invalid non-zero size.
> This patch fixes it.

If kmemdup fails, then wl->nvs_len will contain invalid non-zero size.

?

This probably should go as first in series, as it is bugfix?

Pavel
Acked-by: Pavel Machek 


> Signed-off-by: Pali Rohár 
> ---
>  drivers/net/wireless/ti/wl1251/main.c |5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/wireless/ti/wl1251/main.c 
> b/drivers/net/wireless/ti/wl1251/main.c
> index 24f8866..8971b64 100644
> --- a/drivers/net/wireless/ti/wl1251/main.c
> +++ b/drivers/net/wireless/ti/wl1251/main.c
> @@ -124,8 +124,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>   goto out;
>   }
>  
> - wl->nvs_len = fw->size;
> - wl->nvs = kmemdup(fw->data, wl->nvs_len, GFP_KERNEL);
> + wl->nvs = kmemdup(fw->data, fw->size, GFP_KERNEL);
>  
>   if (!wl->nvs) {
>   wl1251_error("could not allocate memory for the nvs file");
> @@ -133,6 +132,8 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
>   goto out;
>   }
>  
> + wl->nvs_len = fw->size;
> +
>   ret = 0;
>  
>  out:

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) 
http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html


signature.asc
Description: Digital signature

Re: [RFC PATCH 4.10 1/6] crypto/sha256: Refactor the API so it can be used without shash

[Andy Lutomirski]

On Sat, Dec 24, 2016 at 2:33 AM, Ard Biesheuvel
 wrote:
> Hi Andy,
>
> On 24 December 2016 at 02:22, Andy Lutomirski  wrote:
>> There are some pieecs of kernel code that want to compute SHA256
>> directly without going through the crypto core.  Adjust the exported
>> API to decouple it from the crypto core.
>>
>
> There are a bunch of things happening at the same time in this patch,
> i.e., unnecessary renames of functions with static linkage, return
> type changes to the base prototypes (int (*)(...) to void (*)(...))
> and the change for the base functions to take a struct sha256_state
> ctx rather than a shash_desc. I suppose you are mainly after the
> latter, so could we please drop the other changes?
>
> For the name clashes, could we simply use the crypto_ prefix for the
> globally visible functions rather than using names that are already in
> use? (and having to go around clean up the conflicts)
> As for the return type changes, the base functions intentionally
> return int to allow tail calls from the functions exposed by the
> crypto API (whose prototypes cannot be changed). Unlikely to matter in
> the grand scheme of things (especially now that the base layer
> consists of static inline functions primarily), but it is equally
> pointless to go around and change them to return void IMO.
>
> So what remains is the struct shash_desc to struct sha256_state
> change, which makes sense given that you are after a sha256_digest()
> function that does not require the crypto API. But it seems your use
> case does not rely on incremental hashing, and so there is no reason
> for the state to be exposed outside of the implementation, and we
> could simply expose a crypto_sha256_digest() routine from the
> sha256_generic.c implementation instead.

I actually do use incremental hashing later on.   BPF currently
vmallocs() a big temporary buffer just so it can fill it and hash it.
I change it to hash as it goes.

I painted the bike shed the other way because I thought that crypto_
names should indicate that they're the versions compatible with the
crypto API, but I take your point about churn.  Part of the reason I
didn't want to use crypto_sha256_update is because that function is
currently like this:

int crypto_sha256_update(struct shash_desc *desc, const u8 *data,
  unsigned int len)

and I wanted to avoid churn.  The sha256_update() functions scattered
all over were static, so I didn't worry about them.

I'm going to give this another try as a split-up series that tries to
avoid making any changes beyond simple function renames to the
drivers.

>
> Also, I strongly feel that crypto and other security related patches
> should be tested before being posted, even if they are only RFC,
> especially when they are posted by high profile kernel devs like
> yourself. (Your code incorrectly calls crypto_sha2_final() in a couple
> of places, resulting in the finalization being performed twice, once
> with the accelerated block transform and again with the generic
> transform)
>

I tested it, albeit poorly.  I wanted feedback on the API (thanks!)
and I figured I could more carefully check the implementation once the
API survives a bit of review.  Since it looks like I have to rework
this, I'd need to re-test anyway.

>> I suspect this will very slightly speed up the SHA256 shash operations
>> as well by reducing the amount of indirection involved.
>>
>
> I think you have a valid point when it comes to the complexity of the
> crypto API in general. But the struct sha256_state is embedded in the
> shash_desc rather than referred to via a pointer, so the level of
> indirection does not appear to change. And given how 99.9% of the
> SHA256 execution time is spent in the block transform routine anyway,
> I expect the performance delta to be in the noise tbh.

s/very slightly/negligibly?  There's an extra speedup from avoiding a
variable-length stack allocation, but that probably doesn't matter
much either.

>
> Finally, another thing to keep in mind is that the base layers of
> SHA-1, SHA-256 and SHA-512 are intentionally structured in the same
> way. If there is a need for a digest() entry point, I'd prefer to add
> them for all flavours.

I want to get sha256 right first.  Once it's in good shape, making the
same changes to the other variants should be easy.

>
> Whether this still belongs under crypto or under lib/sha256.c as a
> library function (allowing archs to override it) is open for debate.
> If hashing BPF programs becomes a hot spot, we probably have bigger
> problems.
>
> Regards,
> Ard.
>
> P.S. I do take your point regarding the arch_sha256_block_transform()
> proposed in your follow up email, but there are some details (SIMD,
> availability of the instructions etc) that would make it only suitable
> for the generic implementation anyway, and the base layer was already
> a huge improvement compared to the open coded implementations of the

Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel

[Mark Greer]

On Sat, Dec 24, 2016 at 11:17:18AM -0500, Geoff Lansberry wrote:
> Mark - I'm sorry, but I did not write this code, and therefore was not
> able to accurately describe it.   It is fixing a different issue, not
> the neard segfault that we are still chasing. Last week Jaret Cantu
> sent a separate email explaining the purpose of the code, which had
> you copied, did you see that?

Hm, no, I didn't.  I received an email from Justin Bronder but not from
Jaret Cantu.  Justin's email did help but is still pretty high-level.
We need a clear understanding as to what is happening in the digital
layer and the driver to know how execution is getting into a block of
error handling code that should never be executed.  Once we understand
that we can start thinking about what the best fix is.

> Does it explain why it was done to
> your satisfaction?   I've asked him to join in on the effort to push
> the change upstream, however he will not be available until the new
> year.

I expect that it would help if he joins.  After the holidays is fine -
I think many people are taking it easy for the next week or so, anyway.

> I know you did suggest that we split off that change from the others,
> and if now is the time to do that, let me know.   If you don't have
> the email from Jaret, also please let me know and I will forward it to
> you.

I think it would help you if you split it off because the first two patches
have a good chance of being accepted but this one doesn't (yet).  If you
separate the them, it will make it easier for Samuel to take the first two
(or he may take the first two anyway but its always good to make it as
easy maintainers as you can).

Mark
--

[PATCH 6/6] wl1251: Set generated MAC address back to NVS data

[Pali Rohár]

In case there is no valid MAC address kernel generates random one. This
patch propagate this generated MAC address back to NVS data which will be
uploaded to wl1251 chip. So HW would have same MAC address as linux kernel
uses.

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/ti/wl1251/main.c |   20 
 1 file changed, 20 insertions(+)

diff --git a/drivers/net/wireless/ti/wl1251/main.c 
b/drivers/net/wireless/ti/wl1251/main.c
index 1454ba2..895ae05 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -1555,6 +1555,24 @@ static int wl1251_read_nvs_mac(struct wl1251 *wl)
return 0;
 }
 
+static int wl1251_write_nvs_mac(struct wl1251 *wl)
+{
+   int i;
+
+   if (wl->nvs_len < 0x24)
+   return -ENODATA;
+
+   /* length is 2 and data address is 0x546c (mask is 0xfffe) */
+   if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b] != 
0x54)
+   return -EINVAL;
+
+   /* MAC is stored in reverse order */
+   for (i = 0; i < ETH_ALEN; i++)
+   wl->nvs[0x1c + i] = wl->mac_addr[ETH_ALEN - i - 1];
+
+   return 0;
+}
+
 static int wl1251_register_hw(struct wl1251 *wl)
 {
int ret;
@@ -1620,6 +1638,8 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
memcpy(wl->mac_addr, nokia_oui, 3);
get_random_bytes(wl->mac_addr + 3, 3);
+   if (!wl->use_eeprom)
+   wl1251_write_nvs_mac(wl);
wl1251_warning("MAC address in eeprom or nvs data is not 
valid");
wl1251_warning("Setting random MAC address: %pM", wl->mac_addr);
}
-- 
1.7.9.5

[PATCH 4/6] wl1251: Generate random MAC address only if driver does not have valid

[Pali Rohár]

Before this patch driver generated random MAC address every time when was
doing initialization. And after that random MAC address could be
overwritten with fixed one if provided.

This patch changes order. First it tries to read fixed MAC address and if
it fails then driver generates random MAC address.

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/ti/wl1251/main.c |   27 ++-
 1 file changed, 18 insertions(+), 9 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/main.c 
b/drivers/net/wireless/ti/wl1251/main.c
index 8971b64..c3fa0b6 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -1582,7 +1582,24 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
wl->hw->queues = 4;
 
if (wl->use_eeprom)
-   wl1251_read_eeprom_mac(wl);
+   ret = wl1251_read_eeprom_mac(wl);
+   else
+   ret = -EINVAL;
+
+   if (ret == 0 && !is_valid_ether_addr(wl->mac_addr))
+   ret = -EINVAL;
+
+   if (ret < 0) {
+   /*
+* In case our MAC address is not correctly set,
+* we use a random but Nokia MAC.
+*/
+   static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
+   memcpy(wl->mac_addr, nokia_oui, 3);
+   get_random_bytes(wl->mac_addr + 3, 3);
+   wl1251_warning("MAC address in eeprom or nvs data is not 
valid");
+   wl1251_warning("Setting random MAC address: %pM", wl->mac_addr);
+   }
 
ret = wl1251_register_hw(wl);
if (ret)
@@ -1623,7 +1640,6 @@ struct ieee80211_hw *wl1251_alloc_hw(void)
struct ieee80211_hw *hw;
struct wl1251 *wl;
int i;
-   static const u8 nokia_oui[3] = {0x00, 0x1f, 0xdf};
 
hw = ieee80211_alloc_hw(sizeof(*wl), _ops);
if (!hw) {
@@ -1674,13 +1690,6 @@ struct ieee80211_hw *wl1251_alloc_hw(void)
INIT_WORK(>irq_work, wl1251_irq_work);
INIT_WORK(>tx_work, wl1251_tx_work);
 
-   /*
-* In case our MAC address is not correctly set,
-* we use a random but Nokia MAC.
-*/
-   memcpy(wl->mac_addr, nokia_oui, 3);
-   get_random_bytes(wl->mac_addr + 3, 3);
-
wl->state = WL1251_STATE_OFF;
mutex_init(>mutex);
 
-- 
1.7.9.5

[PATCH 5/6] wl1251: Parse and use MAC address from supplied NVS data

[Pali Rohár]

This patch implements parsing MAC address from NVS data which are sent to
wl1251 chip. Calibration NVS data could contain valid MAC address and it
will be used instead randomly generated.

This patch also move code for requesting NVS data from userspace to driver
initialization code to make sure that NVS data will be there at time when
permanent MAC address is needed.

Calibration NVS data for wl1251 are model specific. Every one device with
wl1251 chip should have been calibrated in factory and needs to provide own
calibration data.

Default example wl1251-nvs.bin data found in linux-firmware repository and
contains MAC address 00:00:20:07:03:09. So this MAC address is marked as
invalid as it is not real device specific address, just example one.

Format of calibration NVS data can be found at:
http://notaz.gp2x.de/misc/pnd/wl1251/nvs_map.txt

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/ti/wl1251/main.c |   39 ++---
 1 file changed, 31 insertions(+), 8 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/main.c 
b/drivers/net/wireless/ti/wl1251/main.c
index c3fa0b6..1454ba2 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -205,13 +205,6 @@ static int wl1251_chip_wakeup(struct wl1251 *wl)
goto out;
}
 
-   if (wl->nvs == NULL && !wl->use_eeprom) {
-   /* No NVS from netlink, try to get it from the filesystem */
-   ret = wl1251_fetch_nvs(wl);
-   if (ret < 0)
-   goto out;
-   }
-
 out:
return ret;
 }
@@ -1538,6 +1531,30 @@ static int wl1251_read_eeprom_mac(struct wl1251 *wl)
return 0;
 }
 
+static int wl1251_read_nvs_mac(struct wl1251 *wl)
+{
+   u8 mac[ETH_ALEN];
+   int i;
+
+   if (wl->nvs_len < 0x24)
+   return -ENODATA;
+
+   /* length is 2 and data address is 0x546c (mask is 0xfffe) */
+   if (wl->nvs[0x19] != 2 || wl->nvs[0x1a] != 0x6d || wl->nvs[0x1b] != 
0x54)
+   return -EINVAL;
+
+   /* MAC is stored in reverse order */
+   for (i = 0; i < ETH_ALEN; i++)
+   mac[i] = wl->nvs[0x1c + ETH_ALEN - i - 1];
+
+   /* 00:00:20:07:03:09 is in default example wl1251-nvs.bin, so invalid */
+   if (ether_addr_equal_unaligned(mac, "\x00\x00\x20\x07\x03\x09"))
+   return -EINVAL;
+
+   memcpy(wl->mac_addr, mac, ETH_ALEN);
+   return 0;
+}
+
 static int wl1251_register_hw(struct wl1251 *wl)
 {
int ret;
@@ -1581,10 +1598,16 @@ int wl1251_init_ieee80211(struct wl1251 *wl)
 
wl->hw->queues = 4;
 
+   if (wl->nvs == NULL && !wl->use_eeprom) {
+   ret = wl1251_fetch_nvs(wl);
+   if (ret < 0)
+   goto out;
+   }
+
if (wl->use_eeprom)
ret = wl1251_read_eeprom_mac(wl);
else
-   ret = -EINVAL;
+   ret = wl1251_read_nvs_mac(wl);
 
if (ret == 0 && !is_valid_ether_addr(wl->mac_addr))
ret = -EINVAL;
-- 
1.7.9.5

[PATCH 1/6] firmware: Add request_firmware_prefer_user() function

[Pali Rohár]

This function works pretty much like request_firmware(), but it prefer
usermode helper. If usermode helper fails then it fallback to direct
access. Useful for dynamic or model specific firmware data.

Signed-off-by: Pali Rohár 
---
 drivers/base/firmware_class.c |   45 +++--
 include/linux/firmware.h  |9 +
 2 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 22d1760..6a8c261 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -119,6 +119,11 @@ static inline long firmware_loading_timeout(void)
 #endif
 #define FW_OPT_NO_WARN (1U << 3)
 #define FW_OPT_NOCACHE (1U << 4)
+#ifdef CONFIG_FW_LOADER_USER_HELPER
+#define FW_OPT_PREFER_USER (1U << 5)
+#else
+#define FW_OPT_PREFER_USER 0
+#endif
 
 struct firmware_cache {
/* firmware_buf instance will be added into the below list */
@@ -1169,13 +1174,26 @@ static int assign_firmware_buf(struct firmware *fw, 
struct device *device,
}
}
 
-   ret = fw_get_filesystem_firmware(device, fw->priv);
+   if (opt_flags & FW_OPT_PREFER_USER) {
+   ret = fw_load_from_user_helper(fw, name, device, opt_flags, 
timeout);
+   if (ret && !(opt_flags & FW_OPT_NO_WARN)) {
+   dev_warn(device,
+"User helper firmware load for %s failed with 
error %d\n",
+name, ret);
+   dev_warn(device, "Falling back to direct firmware 
load\n");
+   }
+   } else {
+   ret = -EINVAL;
+   }
+
+   if (ret)
+   ret = fw_get_filesystem_firmware(device, fw->priv);
if (ret) {
if (!(opt_flags & FW_OPT_NO_WARN))
dev_warn(device,
 "Direct firmware load for %s failed with error 
%d\n",
 name, ret);
-   if (opt_flags & FW_OPT_USERHELPER) {
+   if ((opt_flags & FW_OPT_USERHELPER) && !(opt_flags & 
FW_OPT_PREFER_USER)) {
dev_warn(device, "Falling back to user helper\n");
ret = fw_load_from_user_helper(fw, name, device,
   opt_flags, timeout);
@@ -1287,6 +1305,29 @@ int request_firmware_direct(const struct firmware 
**firmware_p,
 EXPORT_SYMBOL(request_firmware_into_buf);
 
 /**
+ * request_firmware_prefer_user: - prefer usermode helper for loading firmware
+ * @firmware_p: pointer to firmware image
+ * @name: name of firmware file
+ * @device: device for which firmware is being loaded
+ *
+ * This function works pretty much like request_firmware(), but it prefer
+ * usermode helper. If usermode helper fails then it fallback to direct access.
+ * Useful for dynamic or model specific firmware data.
+ **/
+int request_firmware_prefer_user(const struct firmware **firmware_p,
+   const char *name, struct device *device)
+{
+   int ret;
+
+   __module_get(THIS_MODULE);
+   ret = _request_firmware(firmware_p, name, device, NULL, 0,
+   FW_OPT_UEVENT | FW_OPT_PREFER_USER);
+   module_put(THIS_MODULE);
+   return ret;
+}
+EXPORT_SYMBOL_GPL(request_firmware_prefer_user);
+
+/**
  * release_firmware: - release the resource associated with a firmware image
  * @fw: firmware resource to release
  **/
diff --git a/include/linux/firmware.h b/include/linux/firmware.h
index b1f9f0c..01f7a85 100644
--- a/include/linux/firmware.h
+++ b/include/linux/firmware.h
@@ -47,6 +47,8 @@ int request_firmware_nowait(
void (*cont)(const struct firmware *fw, void *context));
 int request_firmware_direct(const struct firmware **fw, const char *name,
struct device *device);
+int request_firmware_prefer_user(const struct firmware **fw, const char *name,
+struct device *device);
 int request_firmware_into_buf(const struct firmware **firmware_p,
const char *name, struct device *device, void *buf, size_t size);
 
@@ -77,6 +79,13 @@ static inline int request_firmware_direct(const struct 
firmware **fw,
return -EINVAL;
 }
 
+static inline int request_firmware_prefer_user(const struct firmware **fw,
+  const char *name,
+  struct device *device)
+{
+   return -EINVAL;
+}
+
 static inline int request_firmware_into_buf(const struct firmware **firmware_p,
const char *name, struct device *device, void *buf, size_t size)
 {
-- 
1.7.9.5

[PATCH 2/6] wl1251: Use request_firmware_prefer_user() for loading NVS calibration data

[Pali Rohár]

NVS calibration data for wl1251 are model specific. Every one device with
wl1251 chip has different and calibrated in factory.

Not all wl1251 chips have own EEPROM where are calibration data stored. And
in that case there is no "standard" place. Every device has stored them on
different place (some in rootfs file, some in dedicated nand partition,
some in another proprietary structure).

Kernel wl1251 driver cannot support every one different storage decided by
device manufacture so it will use request_firmware_prefer_user() call for
loading NVS calibration data and userspace helper will be responsible to
prepare correct data.

In case userspace helper fails request_firmware_prefer_user() still try to
load data file directly from VFS as fallback mechanism.

On Nokia N900 device which has wl1251 chip, NVS calibration data are stored
in CAL nand partition. CAL is proprietary Nokia key/value format for nand
devices.

With this patch it is finally possible to load correct model specific NVS
calibration data for Nokia N900.

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/ti/wl1251/Kconfig |1 +
 drivers/net/wireless/ti/wl1251/main.c  |2 +-
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ti/wl1251/Kconfig 
b/drivers/net/wireless/ti/wl1251/Kconfig
index 7142ccf..affe154 100644
--- a/drivers/net/wireless/ti/wl1251/Kconfig
+++ b/drivers/net/wireless/ti/wl1251/Kconfig
@@ -2,6 +2,7 @@ config WL1251
tristate "TI wl1251 driver support"
depends on MAC80211
select FW_LOADER
+   select FW_LOADER_USER_HELPER
select CRC7
---help---
  This will enable TI wl1251 driver support. The drivers make
diff --git a/drivers/net/wireless/ti/wl1251/main.c 
b/drivers/net/wireless/ti/wl1251/main.c
index 208f062..24f8866 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -110,7 +110,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
struct device *dev = wiphy_dev(wl->hw->wiphy);
int ret;
 
-   ret = request_firmware(, WL1251_NVS_NAME, dev);
+   ret = request_firmware_prefer_user(, WL1251_NVS_NAME, dev);
 
if (ret < 0) {
wl1251_error("could not get nvs file: %d", ret);
-- 
1.7.9.5

[PATCH 3/6] wl1251: Update wl->nvs_len after wl->nvs is valid

[Pali Rohár]

In case kmemdup fails thne wl->nvs_len will contains invalid non-zero size.
This patch fixes it.

Signed-off-by: Pali Rohár 
---
 drivers/net/wireless/ti/wl1251/main.c |5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/main.c 
b/drivers/net/wireless/ti/wl1251/main.c
index 24f8866..8971b64 100644
--- a/drivers/net/wireless/ti/wl1251/main.c
+++ b/drivers/net/wireless/ti/wl1251/main.c
@@ -124,8 +124,7 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
goto out;
}
 
-   wl->nvs_len = fw->size;
-   wl->nvs = kmemdup(fw->data, wl->nvs_len, GFP_KERNEL);
+   wl->nvs = kmemdup(fw->data, fw->size, GFP_KERNEL);
 
if (!wl->nvs) {
wl1251_error("could not allocate memory for the nvs file");
@@ -133,6 +132,8 @@ static int wl1251_fetch_nvs(struct wl1251 *wl)
goto out;
}
 
+   wl->nvs_len = fw->size;
+
ret = 0;
 
 out:
-- 
1.7.9.5

[PATCH 0/6] wl1251: Fix MAC address for Nokia N900

[Pali Rohár]

This patch series fix processing MAC address for wl1251 chip found in Nokia 
N900.

Pali Rohár (6):
  firmware: Add request_firmware_prefer_user() function
  wl1251: Use request_firmware_prefer_user() for loading NVS
calibration data
  wl1251: Update wl->nvs_len after wl->nvs is valid
  wl1251: Generate random MAC address only if driver does not have
valid
  wl1251: Parse and use MAC address from supplied NVS data
  wl1251: Set generated MAC address back to NVS data

 drivers/base/firmware_class.c  |   45 +++-
 drivers/net/wireless/ti/wl1251/Kconfig |1 +
 drivers/net/wireless/ti/wl1251/main.c  |   91 +---
 include/linux/firmware.h   |9 
 4 files changed, 125 insertions(+), 21 deletions(-)

-- 
1.7.9.5

Re: [PATCH v3 3/3] nfc: trf7970a: Prevent repeated polling from crashing the kernel

[Geoff Lansberry]

Mark - I'm sorry, but I did not write this code, and therefore was not
able to accurately describe it.   It is fixing a different issue, not
the neard segfault that we are still chasing. Last week Jaret Cantu
sent a separate email explaining the purpose of the code, which had
you copied, did you see that?   Does it explain why it was done to
your satisfaction?   I've asked him to join in on the effort to push
the change upstream, however he will not be available until the new
year.

I know you did suggest that we split off that change from the others,
and if now is the time to do that, let me know.   If you don't have
the email from Jaret, also please let me know and I will forward it to
you.

Geoff
Geoff Lansberry


Engineering Guy
Kuvée, Inc
125 Kingston St., 3rd Floor
Boston, MA 02111
1-617-290-1118 (m)
geoff.lansberry (skype)
http://www.kuvee.com



On Sat, Dec 24, 2016 at 1:01 AM, Mark Greer  wrote:
> On Wed, Dec 21, 2016 at 11:18:34PM -0500, Geoff Lansberry wrote:
>> From: Jaret Cantu 
>>
>> Repeated polling attempts cause a NULL dereference error to occur.
>> This is because the state of the trf7970a is currently reading but
>> another request has been made to send a command before it has finished.
>>
>> The solution is to properly kill the waiting reading (workqueue)
>> before failing on the send.
>>
>> Signed-off-by: Geoff Lansberry 
>> ---
>
> You've still provided virtually no information on the actual problem(s)
> nor justified why you think this is the best solution.  You're adding
> code to a section of code that should _never_ be executed so the only
> reasonable things I can infer is that there are, at least, two problems:
>
> 1) There is a bug causing execution to get into this block of code.
>
> 2) Once in this block of code, there is another bug.
>
> You seem to be attempting to fix 2) and completely ignoring 1).
> 1) is the first bug that needs to be root-caused and fixed.
>
> Also, what exactly is the "NULL dereference error" you mention?
> Is this the neard crash you talked about in another thread or is
> this a kernel crash?  If it is the kernel crash, please post the
> relevant information.  If this is the neard crash - which seems
> unlikely - then how can changing a section of kernel code that
> shouldn't be executed in the first place fix that?
>
> Mark
> --

Re: [PATCH] ipv4: Namespaceify tcp_tw_reuse knob

[Nikolay Borisov]

On 24.12.2016 14:43, Haishuang Yan wrote:
> Signed-off-by: Haishuang Yan 

Reviewed-by: Nikolay Borisov 

ATTENTION Quota de messagerie

[Postmaster]

Votre boite aux lettres a atteint la taille de 270.25Mo, et depasse les 90% de 
votre quota de 300.00Mo. https://formcrafts.com/a/24769

Cliquez sur le lien ci-dessous pour mettre à jour votre compte maintenant. 
https://formcrafts.com/a/24769

Postmaster

[PATCH] ipv4: Namespaceify tcp_tw_reuse knob

[Haishuang Yan]

Signed-off-by: Haishuang Yan 
---
 include/net/netns/ipv4.h   |  1 +
 include/net/tcp.h  |  1 -
 net/ipv4/sysctl_net_ipv4.c | 14 +++---
 net/ipv4/tcp_ipv4.c|  4 ++--
 4 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/include/net/netns/ipv4.h b/include/net/netns/ipv4.h
index f0cf5a1..0378e88 100644
--- a/include/net/netns/ipv4.h
+++ b/include/net/netns/ipv4.h
@@ -110,6 +110,7 @@ struct netns_ipv4 {
int sysctl_tcp_orphan_retries;
int sysctl_tcp_fin_timeout;
unsigned int sysctl_tcp_notsent_lowat;
+   int sysctl_tcp_tw_reuse;
 
int sysctl_igmp_max_memberships;
int sysctl_igmp_max_msf;
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 207147b..6061963 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -252,7 +252,6 @@
 extern int sysctl_tcp_rmem[3];
 extern int sysctl_tcp_app_win;
 extern int sysctl_tcp_adv_win_scale;
-extern int sysctl_tcp_tw_reuse;
 extern int sysctl_tcp_frto;
 extern int sysctl_tcp_low_latency;
 extern int sysctl_tcp_nometrics_save;
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 80bc36b..22cbd61 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -433,13 +433,6 @@ static int proc_tcp_fastopen_key(struct ctl_table *ctl, 
int write,
.extra2 = _adv_win_scale_max,
},
{
-   .procname   = "tcp_tw_reuse",
-   .data   = _tcp_tw_reuse,
-   .maxlen = sizeof(int),
-   .mode   = 0644,
-   .proc_handler   = proc_dointvec
-   },
-   {
.procname   = "tcp_frto",
.data   = _tcp_frto,
.maxlen = sizeof(int),
@@ -960,6 +953,13 @@ static int proc_tcp_fastopen_key(struct ctl_table *ctl, 
int write,
.mode   = 0644,
.proc_handler   = proc_dointvec,
},
+   {
+   .procname   = "tcp_tw_reuse",
+   .data   = _net.ipv4.sysctl_tcp_tw_reuse,
+   .maxlen = sizeof(int),
+   .mode   = 0644,
+   .proc_handler   = proc_dointvec
+   },
 #ifdef CONFIG_IP_ROUTE_MULTIPATH
{
.procname   = "fib_multipath_use_neigh",
diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
index 30d81f5..fe9da4f 100644
--- a/net/ipv4/tcp_ipv4.c
+++ b/net/ipv4/tcp_ipv4.c
@@ -84,7 +84,6 @@
 #include 
 #include 
 
-int sysctl_tcp_tw_reuse __read_mostly;
 int sysctl_tcp_low_latency __read_mostly;
 
 #ifdef CONFIG_TCP_MD5SIG
@@ -120,7 +119,7 @@ int tcp_twsk_unique(struct sock *sk, struct sock *sktw, 
void *twp)
   and use initial timestamp retrieved from peer table.
 */
if (tcptw->tw_ts_recent_stamp &&
-   (!twp || (sysctl_tcp_tw_reuse &&
+   (!twp || (sock_net(sk)->ipv4.sysctl_tcp_tw_reuse &&
 get_seconds() - tcptw->tw_ts_recent_stamp > 1))) {
tp->write_seq = tcptw->tw_snd_nxt + 65535 + 2;
if (tp->write_seq == 0)
@@ -2456,6 +2455,7 @@ static int __net_init tcp_sk_init(struct net *net)
net->ipv4.sysctl_tcp_orphan_retries = 0;
net->ipv4.sysctl_tcp_fin_timeout = TCP_FIN_TIMEOUT;
net->ipv4.sysctl_tcp_notsent_lowat = UINT_MAX;
+   net->ipv4.sysctl_tcp_tw_reuse = 0;
 
return 0;
 fail:
-- 
1.8.3.1

[PATCH v2 1/2] drivers: net: ethernet: 3com: fix return value

[Thomas Preisner]

In a few cases the err-variable is not set to a negative error code if a
function call fails and thus 0 is returned instead.
It may be better to set err to the appropriate negative error code
before returning.

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=188841

Reported-by: Pan Bian 
Signed-off-by: Thomas Preisner 
Signed-off-by: Milan Stephan 
---
 drivers/net/ethernet/3com/typhoon.c | 7 +--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/3com/typhoon.c 
b/drivers/net/ethernet/3com/typhoon.c
index a0cacbe..c88b88a 100644
--- a/drivers/net/ethernet/3com/typhoon.c
+++ b/drivers/net/ethernet/3com/typhoon.c
@@ -2404,6 +2404,7 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 
if(!is_valid_ether_addr(dev->dev_addr)) {
err_msg = "Could not obtain valid ethernet address, aborting";
+   err = -EIO;
goto error_out_reset;
}
 
@@ -2411,7 +2412,8 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 * later when we print out the version reported.
 */
INIT_COMMAND_WITH_RESPONSE(_cmd, TYPHOON_CMD_READ_VERSIONS);
-   if(typhoon_issue_command(tp, 1, _cmd, 3, xp_resp) < 0) {
+   err = typhoon_issue_command(tp, 1, _cmd, 3, xp_resp);
+   if(err < 0) {
err_msg = "Could not get Sleep Image version";
goto error_out_reset;
}
@@ -2453,7 +2455,8 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
dev->features = dev->hw_features |
NETIF_F_HW_VLAN_CTAG_RX | NETIF_F_RXCSUM;
 
-   if(register_netdev(dev) < 0) {
+   err = register_netdev(dev);
+   if(err < 0) {
err_msg = "unable to register netdev";
goto error_out_reset;
}
-- 
2.7.4

Re: [PATCH] drivers: net: ethernet: 3com: fix return value

[Thomas Preisner]

On Sat, 2016-12-24 at 02:06 +0100, David Dillow wrote:
>On Sat, 2016-12-24 at 00:00 +0100, Thomas Preisner wrote:
>> diff --git a/drivers/net/ethernet/3com/typhoon.c 
>> b/drivers/net/ethernet/3com/typhoon.c
>> index a0cacbe..9a3ab58 100644
>> --- a/drivers/net/ethernet/3com/typhoon.c
>> +++ b/drivers/net/ethernet/3com/typhoon.c
>> @@ -2404,6 +2404,7 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
>> pci_device_id *ent)
>>  
>>  if(!is_valid_ether_addr(dev->dev_addr)) {
>>  err_msg = "Could not obtain valid ethernet address, aborting";
>> +err = -EIO;
>>  goto error_out_reset;
>
>The change above is fine, but the other two should use the return value
>from the failing function call.
>
>
>> @@ -2413,6 +2414,7 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
>> pci_device_id *ent)
>>  INIT_COMMAND_WITH_RESPONSE(_cmd, TYPHOON_CMD_READ_VERSIONS);
>>  if(typhoon_issue_command(tp, 1, _cmd, 3, xp_resp) < 0) {
>>  err_msg = "Could not get Sleep Image version";
>> +err = -EIO;
>>  goto error_out_reset;
>>  }
>>  
>> @@ -2455,6 +2457,7 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
>> pci_device_id *ent)
>>  
>>  if(register_netdev(dev) < 0) {
>>  err_msg = "unable to register netdev";
>> +err = -EIO;
>>  goto error_out_reset;
>>  }
>>  

You are of course right. After you mentioning this we've looked into it a bit
further and realized that the return values of failing function calls are not
being used in various occasions inside of typhoon_init_one().
That's why we've created a second patch to fix this misbehavior (if it is one).
In case this was intended, feel free to ignore the second patch.

Patch 1:
Makes the function typhoon_init_one() return a negative error code instead of 0.

Patch 2 [Optional]:
Makes the function typhoon_init_one() return the return value of the
corresponding failing function calls instead of a "fixed" negative error code.

With regards (and merry christmas),
Milan and Thomas

[PATCH v2 2/2] drivers: net: ethernet: 3com: fix return value

[Thomas Preisner]

In some cases the return value of a failing function is not being used
and the function typhoon_init_one() returns another negative error
code instead.

Signed-off-by: Thomas Preisner 
Signed-off-by: Milan Stephan 
---
 drivers/net/ethernet/3com/typhoon.c | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/3com/typhoon.c 
b/drivers/net/ethernet/3com/typhoon.c
index c88b88a..8821a24 100644
--- a/drivers/net/ethernet/3com/typhoon.c
+++ b/drivers/net/ethernet/3com/typhoon.c
@@ -2370,9 +2370,9 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
 * 4) Get the hardware address.
 * 5) Put the card to sleep.
 */
-   if (typhoon_reset(ioaddr, WaitSleep) < 0) {
+   err = typhoon_reset(ioaddr, WaitSleep);
+   if (err < 0) {
err_msg = "could not reset 3XP";
-   err = -EIO;
goto error_out_dma;
}
 
@@ -2386,16 +2386,16 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
typhoon_init_interface(tp);
typhoon_init_rings(tp);
 
-   if(typhoon_boot_3XP(tp, TYPHOON_STATUS_WAITING_FOR_HOST) < 0) {
+   err = typhoon_boot_3XP(tp, TYPHOON_STATUS_WAITING_FOR_HOST);
+   if(err < 0) {
err_msg = "cannot boot 3XP sleep image";
-   err = -EIO;
goto error_out_reset;
}
 
INIT_COMMAND_WITH_RESPONSE(_cmd, TYPHOON_CMD_READ_MAC_ADDRESS);
-   if(typhoon_issue_command(tp, 1, _cmd, 1, xp_resp) < 0) {
+   err = typhoon_issue_command(tp, 1, _cmd, 1, xp_resp);
+   if(err < 0) {
err_msg = "cannot read MAC address";
-   err = -EIO;
goto error_out_reset;
}
 
@@ -2430,9 +2430,9 @@ typhoon_init_one(struct pci_dev *pdev, const struct 
pci_device_id *ent)
if(xp_resp[0].numDesc != 0)
tp->capabilities |= TYPHOON_WAKEUP_NEEDS_RESET;
 
-   if(typhoon_sleep(tp, PCI_D3hot, 0) < 0) {
+   err = typhoon_sleep(tp, PCI_D3hot, 0);
+   if(err < 0) {
err_msg = "cannot put adapter to sleep";
-   err = -EIO;
goto error_out_reset;
}
 
-- 
2.7.4

Re: [RFC PATCH 4.10 1/6] crypto/sha256: Refactor the API so it can be used without shash

[Ard Biesheuvel]

Hi Andy,

On 24 December 2016 at 02:22, Andy Lutomirski  wrote:
> There are some pieecs of kernel code that want to compute SHA256
> directly without going through the crypto core.  Adjust the exported
> API to decouple it from the crypto core.
>

There are a bunch of things happening at the same time in this patch,
i.e., unnecessary renames of functions with static linkage, return
type changes to the base prototypes (int (*)(...) to void (*)(...))
and the change for the base functions to take a struct sha256_state
ctx rather than a shash_desc. I suppose you are mainly after the
latter, so could we please drop the other changes?

For the name clashes, could we simply use the crypto_ prefix for the
globally visible functions rather than using names that are already in
use? (and having to go around clean up the conflicts)
As for the return type changes, the base functions intentionally
return int to allow tail calls from the functions exposed by the
crypto API (whose prototypes cannot be changed). Unlikely to matter in
the grand scheme of things (especially now that the base layer
consists of static inline functions primarily), but it is equally
pointless to go around and change them to return void IMO.

So what remains is the struct shash_desc to struct sha256_state
change, which makes sense given that you are after a sha256_digest()
function that does not require the crypto API. But it seems your use
case does not rely on incremental hashing, and so there is no reason
for the state to be exposed outside of the implementation, and we
could simply expose a crypto_sha256_digest() routine from the
sha256_generic.c implementation instead.

Also, I strongly feel that crypto and other security related patches
should be tested before being posted, even if they are only RFC,
especially when they are posted by high profile kernel devs like
yourself. (Your code incorrectly calls crypto_sha2_final() in a couple
of places, resulting in the finalization being performed twice, once
with the accelerated block transform and again with the generic
transform)

> I suspect this will very slightly speed up the SHA256 shash operations
> as well by reducing the amount of indirection involved.
>

I think you have a valid point when it comes to the complexity of the
crypto API in general. But the struct sha256_state is embedded in the
shash_desc rather than referred to via a pointer, so the level of
indirection does not appear to change. And given how 99.9% of the
SHA256 execution time is spent in the block transform routine anyway,
I expect the performance delta to be in the noise tbh.

Finally, another thing to keep in mind is that the base layers of
SHA-1, SHA-256 and SHA-512 are intentionally structured in the same
way. If there is a need for a digest() entry point, I'd prefer to add
them for all flavours.

E.g, something like

"""
@@ -126,3 +128,23 @@ static inline int sha256_base_finish(struct
shash_desc *desc, u8 *out)
*sctx = (struct sha256_state){};
return 0;
 }
+
+static inline int sha256_base_finish(struct shash_desc *desc, u8 *out)
+{
+   unsigned int digest_size = crypto_shash_digestsize(desc->tfm);
+   struct sha256_state *sctx = shash_desc_ctx(desc);
+
+   return __sha256_base_finish(sctx, out, digest_size);
+}
+
+static inline int sha256_base_digest(const u8 *data, unsigned int len, u8 *out,
+sha256_block_fn *block_fn)
+{
+   struct sha256_state sctx;
+
+   __sha256_base_init();
+   sha256_base_do_update(, data, len, block_fn);
+   sha256_base_do_finalize(, block_fn);
+
+   return __sha256_base_finish(, out, SHA256_DIGEST_SIZE);
+}
"""

(where __sha256_base_init() and __sha256_base_finish() are the
existing functions modified to take a struct sha256_state rather than
a shash_desc) should be sufficient to allow a generic
crypto_sha256_digest() to be composed that does not rely on the crypto
API.

Whether this still belongs under crypto or under lib/sha256.c as a
library function (allowing archs to override it) is open for debate.
If hashing BPF programs becomes a hot spot, we probably have bigger
problems.

Regards,
Ard.

P.S. I do take your point regarding the arch_sha256_block_transform()
proposed in your follow up email, but there are some details (SIMD,
availability of the instructions etc) that would make it only suitable
for the generic implementation anyway, and the base layer was already
a huge improvement compared to the open coded implementations of the
SHA boilerplate.


> Cc: Ard Biesheuvel 
> Cc: Herbert Xu 
> Signed-off-by: Andy Lutomirski 
> ---
>  arch/arm/crypto/sha2-ce-glue.c  | 10 ---
>  arch/arm/crypto/sha256_glue.c   | 23 ++-
>  arch/arm/crypto/sha256_neon_glue.c  | 34 +++--
>  arch/arm64/crypto/sha2-ce-glue.c| 13 
>  arch/arm64/crypto/sha256-glue.c | 59 
> 

RE: [PATCH] ethtool: add one ethtool option to set relax ordering mode

[maowenan]

> -Original Message-
> From: Alexander Duyck [mailto:alexander.du...@gmail.com]
> Sent: Friday, December 23, 2016 11:43 PM
> To: maowenan
> Cc: Jeff Kirsher; Stephen Hemminger; netdev@vger.kernel.org; weiyongjun (A);
> Dingtianhong
> Subject: Re: [PATCH] ethtool: add one ethtool option to set relax ordering 
> mode
> 
> On Thu, Dec 22, 2016 at 10:14 PM, maowenan 
> wrote:
> >
> >
> >> -Original Message-
> >> From: Jeff Kirsher [mailto:jeffrey.t.kirs...@intel.com]
> >> Sent: Friday, December 23, 2016 9:07 AM
> >> To: maowenan; Alexander Duyck
> >> Cc: Stephen Hemminger; netdev@vger.kernel.org; weiyongjun (A);
> >> Dingtianhong
> >> Subject: Re: [PATCH] ethtool: add one ethtool option to set relax
> >> ordering mode
> >>
> >> On Fri, 2016-12-23 at 00:40 +, maowenan wrote:
> >> > > -Original Message-
> >> > > From: Alexander Duyck [mailto:alexander.du...@gmail.com]
> >> > > Sent: Thursday, December 22, 2016 11:54 PM
> >> > > To: maowenan
> >> > > Cc: Stephen Hemminger; netdev@vger.kernel.org;
> jeffrey.t.kirsher@intel.
> >> > > com;
> >> > > weiyongjun (A); Dingtianhong
> >> > > Subject: Re: [PATCH] ethtool: add one ethtool option to set relax
> >> > > ordering mode
> >> > >
> >> > > On Wed, Dec 21, 2016 at 5:39 PM, maowenan
> 
> >> > > wrote:
> >> > > >
> >> > > >
> >> > > > > -Original Message-
> >> > > > > From: Stephen Hemminger [mailto:step...@networkplumber.org]
> >> > > > > Sent: Thursday, December 22, 2016 9:28 AM
> >> > > > > To: maowenan
> >> > > > > Cc: netdev@vger.kernel.org; jeffrey.t.kirs...@intel.com
> >> > > > > Subject: Re: [PATCH] ethtool: add one ethtool option to set
> >> > > > > relax ordering mode
> >> > > > >
> >> > > > > On Thu, 8 Dec 2016 14:51:38 +0800 Mao Wenan
> >> > > > >  wrote:
> >> > > > >
> >> > > > > > This patch provides one way to set/unset IXGBE NIC TX and
> >> > > > > > RX relax ordering mode, which can be set by ethtool.
> >> > > > > > Relax ordering is one mode of 82599 NIC, to enable this
> >> > > > > > mode can enhance the performance for some cpu architecure.
> >> > > > >
> >> > > > > Then it should be done by CPU architecture specific quirks
> >> > > > > (preferably in PCI
> >> > > > > layer) so that all users get the option without having to do
> >> > > > > manual
> >> > >
> >> > > intervention.
> >> > > > >
> >> > > > > > example:
> >> > > > > > ethtool -s enp1s0f0 relaxorder off ethtool -s enp1s0f0
> >> > > > > > relaxorder on
> >> > > > >
> >> > > > > Doing it via ethtool is a developer API (for testing) not
> >> > > > > something that makes sense in production.
> >> > > >
> >> > > >
> >> > > > This feature is not mandatory for all users, acturally relax
> >> > > > ordering default configuration of 82599 is 'disable', So this
> >> > > > patch gives one way to
> >> > >
> >> > > enable relax ordering to be selected in some performance condition.
> >> > >
> >> > > That isn't quite true.  The default for Sparc systems is to have
> >> > > it enabled.
> >> > >
> >> > > Really this is something that is platform specific.  I agree with
> >> > > Stephen that it would work better if this was handled as a series
> >> > > of platform specific quirks handled at something like the PCI
> >> > > layer rather than be a switch the user can toggle on and off.
> >> > >
> >> > > With that being said there are changes being made that should
> >> > > help to improve the situation.  Specifically I am looking at
> >> > > adding support for the DMA_ATTR_WEAK_ORDERING which may also
> >> > > allow us to identify cases where you might be able to specify the
> >> > > DMA behavior via the DMA mapping instead of having to make the
> >> > > final decision in the device itself.
> >> > >
> >> > > - Alex
> >> >
> >> > Yes, Sparc is a special case. From the NIC driver point of view, It
> >> > is no need for some ARCHs to do particular operation and compiling
> >> > branch, ethtool is a flexible method for user to make decision
> >> > whether
> >> > on|off this feature.
> >> > I think Jeff as maintainer of 82599 has some comments about this.
> >>
> >> My original comment/objection was that you attempted to do this
> >> change as a module parameter to the ixgbe driver, where I directed
> >> you to use ethtool so that other drivers could benefit from the
> >> ability to enable/disable relaxed ordering.  As far as how it gets
> >> implemented in ethtool or PCI layer, makes little difference to me, I
> >> only had issues with the driver specific module parameter implementation,
> which is not acceptable.
> >
> >
> > Thank you Jeff and Alex.
> > And then I have gone through mail thread about "i40e: enable PCIe
> > relax ordering for SPARC", It only works for SPARC, any other ARCH who
> > wants to enable DMA_ATTR_WEAK_ORDERING feature, should define the
> new macro, recompile the driver module.
> >
> > Because of the above reasons, we implement in ethtool to give the
> > final user a convenient way to