* [PATCH][net-next] esp6: fix incorrect null pointer check on xo
From: Colin King @ 2017-04-18 14:06 UTC (permalink / raw)
To: David S . Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The check for xo being null is incorrect, currently it is checking
for non-null, it should be checking for null.
Detected with CoverityScan, CID#1429349 ("Dereference after null check")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
net/ipv6/esp6_offload.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/ipv6/esp6_offload.c b/net/ipv6/esp6_offload.c
index 1cceeee7cc33..95f10728abaa 100644
--- a/net/ipv6/esp6_offload.c
+++ b/net/ipv6/esp6_offload.c
@@ -120,7 +120,7 @@ static struct sk_buff *esp6_gso_segment(struct sk_buff *skb,
netdev_features_t esp_features = features;
struct xfrm_offload *xo = xfrm_offload(skb);
- if (xo)
+ if (!xo)
goto out;
seq = xo->seq.low;
--
2.11.0
^ permalink raw reply related
* Re: Marvell phy errata origins?
From: Andrew Lunn @ 2017-04-18 14:04 UTC (permalink / raw)
To: Daniel Walker
Cc: Florian Fainelli, Andy Fleming, Harini Katakam,
netdev@vger.kernel.org, HEMANT RAMDASI,
Julius Hemanth Pitti -X (jpitti - MONTA VISTA SOFTWARE INC at Cisco)
In-Reply-To: <59691a35-a89c-f9ca-c7b0-73134a737f4c@cisco.com>
On Tue, Apr 18, 2017 at 06:16:33AM -0700, Daniel Walker wrote:
>
> Hi,
>
> Cisco is using a Marvell 88E1112 phy. It seems to be fairly similar
> to the 88E1111 which Harini added a fix for.
Hi Daniel
If you look at Marvell reference drive, DSDT, they are actually quite
different. Different virtual cable tester, different downshift
configuration, different packet generator, different loopback. I would
say they are different generations of PHY.
> In Harini's commit
> message for ,
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/phy/marvell.c?id=3ec0a0f10ceb
>
> "This function has a sequence accessing Page 5 and Register 31, both
> of which are not defined or reserved for this PHY"
>
> For the 88E1112 we see that these are "Factory Test Modes" which the
> contents of are not documented. They aren't really "not defied", and
> aren't really "reserved" .. Marvell support claims they don't
> support these drivers, and Freescale seems to be adding these
> drivers, and the line we are looking at.
>
> We had some issues with our PHY which were corrected with the same
> patch Harini used but modified for the M88E1112. We're trying to get
> to the bottom of where this code came from and what it was suppose
> to be doing.
I tried to find this errata fix in the Marvell reference code. And
failed to find it. But it is "Vendor Crap" code, hard to find anything
in it.
My guess is, this errata just applies to one model of PHY, maybe even
one revision of one model of a PHY. The hard bit is figuring out what
actually needs it. Do you have access to Marvell datasheets?
Otherwise, go back to what appears to be the first commit for this
driver:
commit 00db8189d984d6c51226dafbbe4a667ce9b7d5da
Author: Andy Fleming <afleming@freescale.com>
Date: Sat Jul 30 19:31:23 2005 -0400
This patch adds a PHY Abstraction Layer to the Linux Kernel, enabling
ethernet drivers to remain as ignorant as is reasonable of the connected
PHY's design and operation details.
Signed-off-by: Andy Fleming <afleming@freescale.com>
Signed-off-by: Jeff Garzik <jgarzik@pobox.com>
It seems a reasonable guess that any PHY added after this commit does
not need the errata.
Andrew
^ permalink raw reply
* Marvell phy errata origins?
From: Daniel Walker @ 2017-04-18 13:16 UTC (permalink / raw)
To: Florian Fainelli, Andy Fleming, Harini Katakam,
netdev@vger.kernel.org, HEMANT RAMDASI,
Julius Hemanth Pitti -X (jpitti - MONTA VISTA SOFTWARE INC at Cisco)
Hi,
Cisco is using a Marvell 88E1112 phy. It seems to be fairly similar to
the 88E1111 which Harini added a fix for. In Harini's commit message for ,
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/net/phy/marvell.c?id=3ec0a0f10ceb
"This function has a sequence accessing Page 5 and Register 31, both of
which are not defined or reserved for this PHY"
For the 88E1112 we see that these are "Factory Test Modes" which the
contents of are not documented. They aren't really "not defied", and
aren't really "reserved" .. Marvell support claims they don't support
these drivers, and Freescale seems to be adding these drivers, and the
line we are looking at.
We had some issues with our PHY which were corrected with the same patch
Harini used but modified for the M88E1112. We're trying to get to the
bottom of where this code came from and what it was suppose to be doing.
Here are the problem lines where were removed,
drivers/net/phy/marvell.c:marvell_config_aneg()
266 err = phy_write(phydev, 0x1d, 0x1f);
267 if (err < 0)
268 return err;
269
270 err = phy_write(phydev, 0x1e, 0x200c);
271 if (err < 0)
272 return err;
273
274 err = phy_write(phydev, 0x1d, 0x5);
275 if (err < 0)
276 return err;
277
278 err = phy_write(phydev, 0x1e, 0);
279 if (err < 0)
280 return err;
281
282 err = phy_write(phydev, 0x1e, 0x100);
283 if (err < 0)
284 return err;
Does anyone have any clues as to why this was added?
Thanks,
Daniel
^ permalink raw reply
* RE: Re: [PATCH net-next 1/4] ixgbe: sparc: rename the ARCH_WANT_RELAX_ORDER to IXGBE_ALLOW_RELAXED_ORDER
From: David Laight @ 2017-04-18 13:25 UTC (permalink / raw)
To: 'Gabriele Paoloni', davem@davemloft.net
Cc: Catalin Marinas, Will Deacon, Mark Rutland, Robin Murphy,
jeffrey.t.kirsher@intel.com, alexander.duyck@gmail.com,
linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org,
Dingtianhong, Linuxarm
In-Reply-To: <EE11001F9E5DDD47B7634E2F8A612F2E205199C0@FRAEML521-MBX.china.huawei.com>
From: Gabriele Paoloni
> Sent: 13 April 2017 10:11
> > > Till now only the Intel ixgbe could support enable
> > > Relaxed ordering in the drivers for special architecture,
> > > but the ARCH_WANT_RELAX_ORDER is looks like a general name
> > > for all arch, so rename to a specific name for intel
> > > card looks more appropriate.
> > >
> > > Signed-off-by: Ding Tianhong <dingtianhong@huawei.com>
> >
> > This is not a driver specific facility.
> >
> > Any driver can test this symbol and act accordingly.
> >
> > Just because IXGBE is the first and only user, doesn't mean
> > the facility is driver specific.
>
>
> Please correct me if I am wrong but my understanding is that the standard
> way to enable/disable relaxed ordering is to set/clear bit 4 of the Device
> Control Register (PCI_EXP_DEVCTL_RELAX_EN 0x0010 /* Enable relaxed
> ordering */).
> Now I have looked up for all drivers either enabling or disabling relaxed
> ordering and none of them seems to need a symbol to decide whether to
> enable it or not.
> Also it seems to me enabling/disabling relaxed ordering is never bound to the
> host architecture.
>
> So why this should be (or it is expected to be) a generic symbol?
> Wouldn't it be more correct to have this as a driver specific symbol now and
> move it to a generic one later once we have other drivers requiring it?
'Relaxed ordering' is a bit in the TLP header.
A device (like the ixgbe hardware) can set it for some transactions and
still have the transactions 'ordered enough' for the driver to work.
(If all transactions have 'relaxed ordering' set then I suspect it is
almost impossible to write a working driver.)
The host side could (probably does) have a bit to enable 'relaxed ordering',
it could also enforce stronger ordering than required by the PCIe spec
(eg keeping reads in order).
Clearly, on some sparc64 systems, ixgbe needs to use 'relaxed ordering'.
To me this requires two separate bits be enabled:
1) to the ixgbe driver to generate the 'relaxed' TLP.
2) to the host to actually act on them.
If the ixgbe driver works when both are enabled why have options to
disable either (except for bug-finding)?
David
^ permalink raw reply
* Re: Re: [PATCH v2] cpsw: ethtool: add support for getting/setting EEE registers
From: Niklas Cassel @ 2017-04-18 13:23 UTC (permalink / raw)
To: Florian Fainelli, Giuseppe CAVALLARO, Andrew Lunn, Yegor Yefremov
Cc: netdev, linux-omap@vger.kernel.org, Grygorii Strashko,
N, Mugunthan V, Rami Rosen, Fabrice GASNIER
In-Reply-To: <eb4a3c77-6bf4-8749-85a3-b614bff69cf2@gmail.com>
On 01/04/2017 03:33 PM, Florian Fainelli wrote:
> On 12/02/2016 09:48 AM, Florian Fainelli wrote:
>>>> Peppe, any thoughts on this?
>>>
>>> I share what you say.
>>>
>>> In sum, the EEE management inside the stmmac is:
>>>
>>> - the driver looks at own HW cap register if EEE is supported
>>>
>>> (indeed the user could keep disable EEE if bugged on some HW
>>> + Alex, Fabrice: we had some patches for this to propose where we
>>> called the phy_ethtool_set_eee to disable feature at phy
>>> level
>>>
>>> - then the stmmac asks PHY layer to understand if transceiver and
>>> partners are EEE capable.
>>>
>>> - If all matches the EEE is actually initialized.
>>>
>>> the logic above should be respected when use ethtool, hmm, I will
>>> check the stmmac_ethtool_op_set_eee asap.
>>>
>>> Hoping this is useful
>>
>> This is definitively useful, the only part that I am struggling to
>> understand in phy_init_eee() is this:
>>
>> eee_adv = phy_read_mmd_indirect(phydev, MDIO_AN_EEE_ADV,
>> MDIO_MMD_AN);
>> if (eee_adv <= 0)
>> goto eee_exit_err;
>>
>> if we are not already advertising EEE in the PHY's MMIO_MMD_AN page, by
>> the time we call phy_init_eee(), then we cannot complete the EEE
>> configuration at the PHY level, and presumably we should abort the EEE
>> configuration at the MAC level.
>>
>> While this condition makes sense if e.g: you are re-negotiating the link
>> with your partner for instance and if EEE was already advertised, the
>> very first time this function is called, it seems to be like we should
>> skip the check, because phy_init_eee() should actually tell us if, as a
>> result of a successful check, we should be setting EEE as something we
>> advertise?
>>
>> Do you remember what was the logic behind this check when you added it?
>
> Peppe, can you remember why phy_init_eee() was written in a way that you
> need to have already locally advertised EEE for the function to
> successfully return? Thank you!
>
I'm curious about this as well.
I can get EEE to work with stmmac, but to be able to turn EEE on,
I need to set eee advertise via ethtool first.
(Tested with 2 different PHYs from different vendors, with their
PHY specific driver enabled.)
Is this the same for all PHYs or are there certain PHYs/PHY drivers
that actually advertise eee by default?
(From reading this mail thread there seems to be a suggestion that
the broadcom PHY driver might advertise eee by default.)
^ permalink raw reply
* Re: Case for reusing netlink PADs WAS(Re: [PATCH net-next 1/1] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jiri Pirko @ 2017-04-18 13:16 UTC (permalink / raw)
To: Jamal Hadi Salim; +Cc: Eric Dumazet, davem, netdev, xiyou.wangcong
In-Reply-To: <560eeb0b-7a34-484a-e4a1-c637b31eba59@mojatatu.com>
Tue, Apr 18, 2017 at 02:48:57PM CEST, jhs@mojatatu.com wrote:
>On 17-04-17 01:08 PM, Eric Dumazet wrote:
>> On Mon, 2017-04-17 at 12:46 -0400, Jamal Hadi Salim wrote:
>>
>> > Of course it is trivial to add this as attributes and 32 bits
>> > for this case is not a big deal because it is done once. I want to talk
>> > about the pads instead ;-> What do you suggest we do with pads?
>>
>> We do nothing with pads. Just leave them.
>>
>> Or, you need something to make sure to not break old user applications.
>>
>> Something like a version.
>>
>>
>
>Dave is not fond of version fields - they tend to be abused
>although looking at the idiag and tpacket stuff there is a case
>to be made for versioning ;->
>
>
>For the patches I posted, I will work on getting an attribute based
>variant of the patches out - but i wanted to have this discussion a
>little more if you bear with me.
>
>Netlink is a wire protocol. When a protocol is defined with rules such
>as alignment (which lead to explicit padding) then those are equivalent
>to "reserved bits" in standard wire protocols. Good practise is:
>all sender zero those bits(MBZ); and all receivers must ignore them
>unless they wish to interpret them. Not everyone follows these rules
>(I remember the havoc ECN caused when TCP/IP started using the different
>reserved fields).
>
>For our case it is _very sad_ that someone actually explicitly defined
>pads - in my opinion for no other purpose other than reuse and then
>we say we cant use them after.
>
>I believe you understand what I was saying, but to clarify, here is
>what i meant:
>---
>struct tcamsg {
> unsigned char tca_family;
> union {
> unsigned char tca__pad1;
> unsigned char tca_flags;
> };
> union {
> unsigned short tca__pad2;
> unsigned char tca_foobar;
> };
>};
>---
>
>This should work with old binaries and kernels.
>It will work with someone referencing sizeof tcamsg.
>It will work with unchanged source that memsets the struct.
>It will work with things that explicitly set the pad1 and pad2.
>
>What is the downside?
It's ugly. Plus user may pass garbage in pads from current apps.
Why you don't just use new attributes? I don't get it.
I still don't understand why people feel need to do struct style message
passing in TLV-designed Netlink interface...
^ permalink raw reply
* Re: [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier
From: Jamal Hadi Salim @ 2017-04-18 13:03 UTC (permalink / raw)
To: Wolfgang Bumiller, netdev; +Cc: David S. Miller, Cong Wang, Roman Mashak
In-Reply-To: <20170418101322.27666-2-w.bumiller@proxmox.com>
On 17-04-18 06:13 AM, Wolfgang Bumiller wrote:
> Whether the reference count has to be decremented depends
> on whether the policy was created. If TCA_ACT_COOKIE is
> passed and an error occurs there, the same condition still
> has to be honored.
>
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
>
> I did not include the Acked-bys here because I've noticed that this
> is still wrong. After reading a bit more and doing more tests with
> different filters I realized that the `name != NULL` case is specific
> to the police filter only. For the other filters this patch here breaks
> refcounting in the error case (I included it for reference only).
> I'm thinking the first patch should be enough. (I've tested forcing the
> other filters into the error path *without* this patch and couldn't
> produce crashes or reference count problems (while with this patch
> applied it was leaking reference counts on creation (which makes sense
> considering tcf_hash_release is used and the ACT_P_CREATED case will
> keep repeating)). (Whereas without both patches simply looking through
> creating and deleting a policing filter pretty much always resulted in
> crashes with various different backtraces.)
>
The error path for cookie failure still needs handling.
In retrospect:
I think you should leave the module_put() to the end,
it protects any unloading while the action is being
processed.
It may be worth remembering a_o->init() results and
conditionally calling tcf_hash_release() based on whether
a creation happened or not. I think your original patch had
a similar idea without the extra variable I am suggesting.
cheers,
jamal
^ permalink raw reply
* [PATCH next RFC] net: sched: mirred: move xmit to tasklet
From: Florian Westphal @ 2017-04-18 13:03 UTC (permalink / raw)
To: netdev; +Cc: Florian Westphal
mirred is prone to deadlocks as it invokes dev_queue_xmit while
holding one or more qdisc locks.
Avoid lock recursions by moving tx context to a tasklet.
Signed-off-by: Florian Westphal <fw@strlen.de>
---
This a stab at removing the lock recursions discussed during netconf.
Taking the cost of the tasklet appears to be the only solution;
i tried to use a percpu 'history' instead but its not clear to
me that this avoids all corner cases.
While this patch doesn't avoid loops we don't hang the kernel
anymore and removing the 'looping' filter makes things calm
down again (there are also other ways to create such loops anyway,
including use of a cable... )
diff --git a/net/sched/act_mirred.c b/net/sched/act_mirred.c
index 1b5549ababd4..3dd61eba4741 100644
--- a/net/sched/act_mirred.c
+++ b/net/sched/act_mirred.c
@@ -17,6 +17,7 @@
#include <linux/string.h>
#include <linux/errno.h>
#include <linux/skbuff.h>
+#include <linux/skb_array.h>
#include <linux/rtnetlink.h>
#include <linux/module.h>
#include <linux/init.h>
@@ -25,10 +26,19 @@
#include <net/net_namespace.h>
#include <net/netlink.h>
#include <net/pkt_sched.h>
+#include <net/dst.h>
#include <linux/tc_act/tc_mirred.h>
#include <net/tc_act/tc_mirred.h>
+#define MIRRED_TXLEN 512
#define MIRRED_TAB_MASK 7
+
+struct mirred_tx_data {
+ struct tasklet_struct mirred_tasklet;
+ struct skb_array skb_array;
+};
+
+static DEFINE_PER_CPU(struct mirred_tx_data, mirred_tx_data);
static LIST_HEAD(mirred_list);
static DEFINE_SPINLOCK(mirred_list_lock);
@@ -158,6 +168,44 @@ static int tcf_mirred_init(struct net *net, struct nlattr *nla,
return ret;
}
+static void mirred_tasklet(unsigned long data)
+{
+ struct mirred_tx_data *d = (void *)data;
+ struct sk_buff *skb;
+ int i;
+
+ for (i = 0; i < MIRRED_TXLEN; i++) {
+ struct net_device *dev;
+
+ skb = skb_array_consume(&d->skb_array);
+ if (!skb)
+ return;
+
+ dev = skb->dev;
+ dev_queue_xmit(skb);
+ dev_put(dev);
+ }
+
+ tasklet_schedule(&d->mirred_tasklet);
+}
+
+static int mirred_xmit(struct sk_buff *skb)
+{
+ struct mirred_tx_data *d = this_cpu_ptr(&mirred_tx_data);
+
+ skb_dst_force(skb);
+ dev_hold(skb->dev);
+
+ if (skb_array_produce_bh(&d->skb_array, skb)) {
+ dev_put(skb->dev);
+ kfree_skb(skb);
+ return -ENOBUFS;
+ }
+
+ tasklet_schedule(&d->mirred_tasklet);
+ return 0;
+}
+
static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
struct tcf_result *res)
{
@@ -217,7 +265,7 @@ static int tcf_mirred(struct sk_buff *skb, const struct tc_action *a,
skb2->skb_iif = skb->dev->ifindex;
skb2->dev = dev;
if (!tcf_mirred_act_wants_ingress(m_eaction))
- err = dev_queue_xmit(skb2);
+ err = mirred_xmit(skb2);
else
err = netif_receive_skb(skb2);
@@ -365,20 +413,66 @@ MODULE_AUTHOR("Jamal Hadi Salim(2002)");
MODULE_DESCRIPTION("Device Mirror/redirect actions");
MODULE_LICENSE("GPL");
+static void mirred_cleanup_pcpu(void)
+{
+ int cpu;
+
+ for_each_possible_cpu(cpu) {
+ struct mirred_tx_data *data;
+
+ data = per_cpu_ptr(&mirred_tx_data, cpu);
+
+ skb_array_cleanup(&data->skb_array);
+ tasklet_kill(&data->mirred_tasklet);
+ }
+}
+
static int __init mirred_init_module(void)
{
- int err = register_netdevice_notifier(&mirred_device_notifier);
- if (err)
+ int cpu, err;
+
+ for_each_possible_cpu(cpu) {
+ struct mirred_tx_data *data;
+
+ data = per_cpu_ptr(&mirred_tx_data, cpu);
+
+ tasklet_init(&data->mirred_tasklet, mirred_tasklet,
+ (unsigned long)data);
+ err = skb_array_init(&data->skb_array, MIRRED_TXLEN, GFP_KERNEL);
+ if (err) {
+ unregister_netdevice_notifier(&mirred_device_notifier);
+
+ while (cpu) {
+ data = per_cpu_ptr(&mirred_tx_data, --cpu);
+ skb_array_cleanup(&data->skb_array);
+ }
+
+ return err;
+ }
+ }
+
+ err = register_netdevice_notifier(&mirred_device_notifier);
+ if (err) {
+ mirred_cleanup_pcpu();
+ return err;
+ }
+
+ err = tcf_register_action(&act_mirred_ops, &mirred_net_ops);
+ if (err) {
+ unregister_netdevice_notifier(&mirred_device_notifier);
+ mirred_cleanup_pcpu();
return err;
+ }
pr_info("Mirror/redirect action on\n");
- return tcf_register_action(&act_mirred_ops, &mirred_net_ops);
+ return 0;
}
static void __exit mirred_cleanup_module(void)
{
tcf_unregister_action(&act_mirred_ops, &mirred_net_ops);
unregister_netdevice_notifier(&mirred_device_notifier);
+ mirred_cleanup_pcpu();
}
module_init(mirred_init_module);
--
2.10.2
^ permalink raw reply related
* Re: [PATCH v2 net 1/2] net sched actions: fix access to uninitialized data
From: Jamal Hadi Salim @ 2017-04-18 12:51 UTC (permalink / raw)
To: Wolfgang Bumiller, netdev; +Cc: David S. Miller, Cong Wang
In-Reply-To: <20170418101322.27666-1-w.bumiller@proxmox.com>
On 17-04-18 06:13 AM, Wolfgang Bumiller wrote:
> Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
> Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
> Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
> ---
> (same as v1)
>
> net/sched/act_api.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/sched/act_api.c b/net/sched/act_api.c
> index b70aa57319ea..8cc883c063f0 100644
> --- a/net/sched/act_api.c
> +++ b/net/sched/act_api.c
> @@ -604,7 +604,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
> if (err < 0)
> goto err_mod;
>
> - if (tb[TCA_ACT_COOKIE]) {
> + if (name == NULL && tb[TCA_ACT_COOKIE]) {
> int cklen = nla_len(tb[TCA_ACT_COOKIE]);
>
> if (cklen > TC_COOKIE_MAX_SIZE) {
>
This one looks good.
cheers,
jamal
^ permalink raw reply
* Re: [Patch net-next v3] net_sched: move the empty tp check from ->destroy() to ->delete()
From: Jamal Hadi Salim @ 2017-04-18 12:51 UTC (permalink / raw)
To: Cong Wang, netdev; +Cc: Daniel Borkmann, John Fastabend, Lucas Bates
In-Reply-To: <1492453840-15816-1-git-send-email-xiyou.wangcong@gmail.com>
On 17-04-17 02:30 PM, Cong Wang wrote:
> Roi reported we could have a race condition where in ->classify() path
> we dereference tp->root and meanwhile a parallel ->destroy() makes it
> a NULL.
>
> This is possible because ->destroy() could be called when deleting
> a filter to check if we are the last one in tp, this tp is still
> linked and visible at that time.
>
> Daniel fixed this in commit d936377414fa
> ("net, sched: respect rcu grace period on cls destruction"), but
> the root cause of this problem is the semantic of ->destroy(), it
> does two things (for non-force case):
>
> 1) check if tp is empty
> 2) if tp is empty we could really destroy it
>
> and its caller, if cares, needs to check its return value to see if
> it is really destroyed. Therefore we can't unlink tp unless we know
> it is empty.
>
> As suggested by Daniel, we could actually move the test logic to ->delete()
> so that we can safely unlink tp after ->delete() tells us the last one is
> just deleted and before ->destroy().
>
> What's more, even we unlink it before ->destroy(), it could still have
> readers since we don't wait for a grace period here, we should not modify
> tp->root in ->destroy() either.
>
> Reported-by: Roi Dayan <roid@mellanox.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: John Fastabend <john.fastabend@gmail.com>
> Cc: Jamal Hadi Salim <jhs@mojatatu.com>
> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
Looks good to me, Daniel?
Also - if we are not in a hurry could I/Lucas (and maybe Roi) be given
the chance to test it?
cheers,
jamal
^ permalink raw reply
* Case for reusing netlink PADs WAS(Re: [PATCH net-next 1/1] net sched actions: dump more than TCA_ACT_MAX_PRIO actions per batch
From: Jamal Hadi Salim @ 2017-04-18 12:48 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Jiri Pirko, davem, netdev, xiyou.wangcong
In-Reply-To: <1492448905.10587.109.camel@edumazet-glaptop3.roam.corp.google.com>
On 17-04-17 01:08 PM, Eric Dumazet wrote:
> On Mon, 2017-04-17 at 12:46 -0400, Jamal Hadi Salim wrote:
>
>> Of course it is trivial to add this as attributes and 32 bits
>> for this case is not a big deal because it is done once. I want to talk
>> about the pads instead ;-> What do you suggest we do with pads?
>
> We do nothing with pads. Just leave them.
>
> Or, you need something to make sure to not break old user applications.
>
> Something like a version.
>
>
Dave is not fond of version fields - they tend to be abused
although looking at the idiag and tpacket stuff there is a case
to be made for versioning ;->
For the patches I posted, I will work on getting an attribute based
variant of the patches out - but i wanted to have this discussion a
little more if you bear with me.
Netlink is a wire protocol. When a protocol is defined with rules such
as alignment (which lead to explicit padding) then those are equivalent
to "reserved bits" in standard wire protocols. Good practise is:
all sender zero those bits(MBZ); and all receivers must ignore them
unless they wish to interpret them. Not everyone follows these rules
(I remember the havoc ECN caused when TCP/IP started using the different
reserved fields).
For our case it is _very sad_ that someone actually explicitly defined
pads - in my opinion for no other purpose other than reuse and then
we say we cant use them after.
I believe you understand what I was saying, but to clarify, here is
what i meant:
---
struct tcamsg {
unsigned char tca_family;
union {
unsigned char tca__pad1;
unsigned char tca_flags;
};
union {
unsigned short tca__pad2;
unsigned char tca_foobar;
};
};
---
This should work with old binaries and kernels.
It will work with someone referencing sizeof tcamsg.
It will work with unchanged source that memsets the struct.
It will work with things that explicitly set the pad1 and pad2.
What is the downside?
cheers,
jamal
^ permalink raw reply
* Re: [PATCH net-next] ravb: Fix ravb_ptp_interrupt clear interrupt all status
From: Sergei Shtylyov @ 2017-04-18 12:47 UTC (permalink / raw)
To: Simon Horman, David Miller; +Cc: Magnus Damm, netdev, linux-renesas-soc
In-Reply-To: <1492476933-7952-1-git-send-email-horms+renesas@verge.net.au>
Hello!
On 04/18/2017 03:55 AM, Simon Horman wrote:
> From: Tsutomu Izawa <tsutomu.izawa.xk@renesas.com>
>
> This patch fixes ravb_ptp_interrupt clears GIS register of all interrupts
Clearing, maybe?
> status. It corrects to clear PTCF bit or PTMF bit.
Well, it's safer to clear all of them, I think...
> Also it fixes returned value to IRQ_HANDLED or IRQ_NONE.
>
> Signed-off-by: Tsutomu Izawa <tsutomu.izawa.xk@renesas.com>
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
> ---
> drivers/net/ethernet/renesas/ravb.h | 4 ++--
> drivers/net/ethernet/renesas/ravb_main.c | 12 ++++--------
> drivers/net/ethernet/renesas/ravb_ptp.c | 15 ++++++++++++---
> 3 files changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 0525bd696d5d..fbfdefa659ce 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index 8cfc4a54f2dc..747686386513 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -820,10 +820,8 @@ static irqreturn_t ravb_interrupt(int irq, void *dev_id)
> }
>
> /* gPTP interrupt status summary */
> - if (iss & ISS_CGIS) {
> - ravb_ptp_interrupt(ndev);
> - result = IRQ_HANDLED;
> - }
> + if (iss & ISS_CGIS)
> + result = ravb_ptp_interrupt(ndev);
No, this reintroduces the same kind of bug that we've fixed already:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=38c848c73180fb1d142a39e664b6d9663751f6b2
[...]
> @@ -853,10 +851,8 @@ static irqreturn_t ravb_multi_interrupt(int irq, void *dev_id)
> }
>
> /* gPTP interrupt status summary */
> - if (iss & ISS_CGIS) {
> - ravb_ptp_interrupt(ndev);
> - result = IRQ_HANDLED;
> - }
> + if (iss & ISS_CGIS)
> + result = ravb_ptp_interrupt(ndev);
Here as well... look at the other handlers returning success.
[...]
MBR, Sergei
^ permalink raw reply
* [PATCH net-next v3] bindings: net: stmmac: add missing note about LPI interrupt
From: Niklas Cassel @ 2017-04-18 12:39 UTC (permalink / raw)
To: Rob Herring, Mark Rutland, David S. Miller, Joao Pinto,
Niklas Cassel, Alexandre TORGUE, Giuseppe CAVALLARO,
Thierry Reding, Eric Engestrom
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
From: Niklas Cassel <niklas.cassel-VrBV9hrLPhE@public.gmane.org>
The hardware has a LPI interrupt.
There is already code in the stmmac driver to parse and handle the
interrupt. However, this information was missing from the DT binding.
At the same time, improve the description of the existing interrupts.
Signed-off-by: Niklas Cassel <niklas.cassel-VrBV9hrLPhE@public.gmane.org>
---
Documentation/devicetree/bindings/net/stmmac.txt | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
index f652b0c384ce..c3a7be6615c5 100644
--- a/Documentation/devicetree/bindings/net/stmmac.txt
+++ b/Documentation/devicetree/bindings/net/stmmac.txt
@@ -7,9 +7,12 @@ Required properties:
- interrupt-parent: Should be the phandle for the interrupt controller
that services interrupts for this device
- interrupts: Should contain the STMMAC interrupts
-- interrupt-names: Should contain the interrupt names "macirq"
- "eth_wake_irq" if this interrupt is supported in the "interrupts"
- property
+- interrupt-names: Should contain a list of interrupt names corresponding to
+ the interrupts in the interrupts property, if available.
+ Valid interrupt names are:
+ - "macirq" (combined signal for various interrupt events)
+ - "eth_wake_irq" (the interrupt to manage the remote wake-up packet detection)
+ - "eth_lpi" (the interrupt that occurs when Tx or Rx enters/exits LPI state)
- phy-mode: See ethernet.txt file in the same directory.
- snps,reset-gpio gpio number for phy reset.
- snps,reset-active-low boolean flag to indicate if phy reset is active low.
@@ -152,8 +155,8 @@ Examples:
compatible = "st,spear600-gmac";
reg = <0xe0800000 0x8000>;
interrupt-parent = <&vic1>;
- interrupts = <24 23>;
- interrupt-names = "macirq", "eth_wake_irq";
+ interrupts = <24 23 22>;
+ interrupt-names = "macirq", "eth_wake_irq", "eth_lpi";
mac-address = [000000000000]; /* Filled in by U-Boot */
max-frame-size = <3800>;
phy-mode = "gmii";
--
2.11.0
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply related
* Re: net/ipv4: use-after-free in ip_queue_xmit
From: Andrey Konovalov @ 2017-04-18 12:15 UTC (permalink / raw)
To: Cong Wang
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML, Eric Dumazet,
Dmitry Vyukov, Kostya Serebryany, syzkaller
In-Reply-To: <CAM_iQpU_frn6bB6HHrRrF6omKDoXjxZmSoJZSu=PjZ35pUVOuA@mail.gmail.com>
On Thu, Apr 13, 2017 at 9:59 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Thu, Apr 13, 2017 at 11:49 AM, Andrey Konovalov
> <andreyknvl@google.com> wrote:
>> On Mon, Apr 10, 2017 at 7:46 PM, Andrey Konovalov <andreyknvl@google.com> wrote:
>>> On Mon, Apr 10, 2017 at 7:42 PM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>>> On Mon, Apr 10, 2017 at 7:40 AM, Andrey Konovalov <andreyknvl@google.com> wrote:
>>>>> Hi,
>>>>>
>>>>> I've got the following error report while fuzzing the kernel with syzkaller.
>>>>>
>>>>> On commit 39da7c509acff13fc8cb12ec1bb20337c988ed36 (4.11-rc6).
>>>>>
>>>>> Unfortunately it's not reproducible.
>>>>>
>>>>> BUG: KASAN: use-after-free in ip_select_ttl include/net/dst.h:176
>>>>> [inline] at addr ffff88006ab3602c
>>>>> BUG: KASAN: use-after-free in ip_queue_xmit+0x1817/0x1a30
>>>>> net/ipv4/ip_output.c:485 at addr ffff88006ab3602c
>>>>> Read of size 4 by task syz-executor1/12627
>>>>> CPU: 3 PID: 12627 Comm: syz-executor1 Not tainted 4.11.0-rc6+ #206
>>>>> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS Bochs 01/01/2011
>>>>> Call Trace:
>>>>> __dump_stack lib/dump_stack.c:16 [inline]
>>>>> dump_stack+0x292/0x398 lib/dump_stack.c:52
>>>>> kasan_object_err+0x1c/0x70 mm/kasan/report.c:164
>>>>> print_address_description mm/kasan/report.c:202 [inline]
>>>>> kasan_report_error mm/kasan/report.c:291 [inline]
>>>>> kasan_report+0x252/0x510 mm/kasan/report.c:347
>>>>> __asan_report_load4_noabort+0x14/0x20 mm/kasan/report.c:367
>>>>> ip_select_ttl include/net/dst.h:176 [inline]
>>>>
>>>> Probably same as the one you reported on ipv4_mtu(), it would
>>>> be nice if you could test the patch I proposed:
>>>>
>>>> https://patchwork.ozlabs.org/patch/747556/
>>>
>>> Applied your patch.
>>
>> Oops, apparently your patch doesn't compile:
>>
>
> Weird, it compiles fine here. Either you have a different config
> or the following piece is missing for some reason?
>
> @@ -69,6 +69,7 @@ struct rtable {
>
> struct list_head rt_uncached;
> struct uncached_list *rt_uncached_list;
> + struct fib_info *fi; /* for refcnt to shared metrics */
> };
Yes, I don't have this field in the rtable struct.
I'm on 39da7c509acff13fc8cb12ec1bb20337c988ed36 (4.11-rc6).
I also don't see it in the cross reference:
http://lxr.free-electrons.com/source/include/net/route.h#L51
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [RFC 1/3] bpf/wireless: add wifimon program type
From: Johannes Berg @ 2017-04-18 11:35 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov
Cc: David Miller, linux-wireless, netdev, ast
In-Reply-To: <1492514882.2472.22.camel@sipsolutions.net>
So actually, come to think of it ...
> > The mapping in samples/bpf/bpf_helpers.h, for example, for
> > mentioned
> > bpf_skb_load_bytes() would also work out of the box, since it takes
> > a
> > void *ctx as an argument, so you can just pass the __wifi_sk_buff
> > pointer as ctx there from program side.
>
> Hah. That's what I was missing - I always assumed the argument was
> "struct __sk_buff *" without ever checking that assumption.
Given this, I think I'll actually make a __wifi_sk_buff.
> The thing is that __wifi_sk_buff doesn't have much information that's
> generally useful available
Because I just realized that this isn't true. To make sense of the SKB
beyond the 802.11 header, which may not be possible at all though due
to encryption happening in software later, it will have to know a few
things like whether or not it was encrypted and if the IV was stripped
etc.
Actually, perhaps we should just restrict it to just look at the header
;-)
johannes
^ permalink raw reply
* Re: ney/key: slab-out-of-bounds in parse_ipsecrequests
From: Steffen Klassert @ 2017-04-18 11:34 UTC (permalink / raw)
To: Herbert Xu
Cc: Cong Wang, Andrey Konovalov, David S. Miller, netdev, LKML,
Dmitry Vyukov, Kostya Serebryany, syzkaller, Eric Dumazet
In-Reply-To: <20170413103559.GA23263@gondor.apana.org.au>
On Thu, Apr 13, 2017 at 06:35:59PM +0800, Herbert Xu wrote:
> On Wed, Apr 12, 2017 at 05:39:22PM -0700, Cong Wang wrote:
> > On Wed, Apr 12, 2017 at 8:02 AM, Andrey Konovalov <andreyknvl@google.com> wrote:
> > > Hi,
> > >
> > > I've got the following error report while fuzzing the kernel with syzkaller.
> > >
> > > On commit 39da7c509acff13fc8cb12ec1bb20337c988ed36 (4.11-rc6).
> > >
> > > A reproducer and .config are attached.
> > >
> > > When subtracting rq->sadb_x_ipsecrequest_len from len it can become
> > > negative and the while loop condition remains true.
> >
> > Good catch! Seems the fix is pretty straight forward:
> >
> > diff --git a/net/key/af_key.c b/net/key/af_key.c
> > index c6252ed..cbce595 100644
> > --- a/net/key/af_key.c
> > +++ b/net/key/af_key.c
> > @@ -1945,7 +1945,7 @@ parse_ipsecrequests(struct xfrm_policy *xp,
> > struct sadb_x_policy *pol)
> > if (pol->sadb_x_policy_len * 8 < sizeof(struct sadb_x_policy))
> > return -EINVAL;
> >
> > - while (len >= sizeof(struct sadb_x_ipsecrequest)) {
> > + while (len >= (int)sizeof(struct sadb_x_ipsecrequest)) {
> > if ((err = parse_ipsecrequest(xp, rq)) < 0)
> > return err;
> > len -= rq->sadb_x_ipsecrequest_len;
> >
> > But pol->sadb_x_policy_len and rq->sadb_x_ipsecrequest_len
> > are controllable by user (fortunately root), I am feeling there might
> > be other problem I miss too.
>
> Well the fact that it is negative means that you've already parsed
> crap in the previous loop. This interface really needs to die.
Do you see a chance to remove this? I guess it is not used frequently
anymore, but distros still ship the old ipsec tools.
>
> ---8<---
> Subject: af_key: Fix sadb_x_ipsecrequest parsing
>
> The parsing of sadb_x_ipsecrequest is broken in a number of ways.
> First of all we're not verifying sadb_x_ipsecrequest_len. This
> is needed when the structure carries addresses at the end. Worse
> we don't even look at the length when we parse those optional
> addresses.
>
> The migration code had similar parsing code that's better but
> it also has some deficiencies. The length is overcounted first
> of all as it includes the header itself. It also fails to check
> the length before dereferencing the sa_family field.
>
> This patch fixes those problems in parse_sockaddr_pair and then
> uses it in parse_ipsecrequest.
>
> Reported-by: Andrey Konovalov <andreyknvl@google.com>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
Applied to the ipsec tree, thanks Herbert!
^ permalink raw reply
* Re: [RFC 1/3] bpf/wireless: add wifimon program type
From: Johannes Berg @ 2017-04-18 11:28 UTC (permalink / raw)
To: Daniel Borkmann, Alexei Starovoitov
Cc: David Miller, linux-wireless-u79uwXL29TY76Z2rM5mHXA,
netdev-u79uwXL29TY76Z2rM5mHXA, ast-DgEjT+Ai2ygdnm+yROfE0A
In-Reply-To: <58F5F148.1090700-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
On Tue, 2017-04-18 at 12:58 +0200, Daniel Borkmann wrote:
>
> Note that for skbs the data / data_end model (aka direct read /
> write) is also supported. There's also a bpf_skb_pull_data() helper
> that pulls in data from non-linear parts if necessary (f.e. if data /
> data_end test in the program fails). bpf_skb_load_bytes() is fine as
> well, comes with function call overhead, though, but depending on the
> use-case that might be just fine, too.
Yeah. I did see this, I just wasn't convinced I wanted the program to
be able to modify the SKB that way. OTOH, it doesn't actually matter if
it does this since that doesn't fundamentally change the SKB, so I
might as well allow it - and hook up data/data_end. In many cases, the
decision will be taken on the 802.11 header only, and that will be in
the linear portion anyway (with any self-respecting driver :p)
> Yeah, *_is_valid_access() callbacks would need to reject these
> members for most of the program types.
Yes, but that's the default case, so there's no real danger.
> Assuming you would introduce __wifi_sk_buff to the uapi, then it
> would be that the kernel internally still operates on an skb, you'd
> still call the program through BPF_PROG_RUN(prog, skb). However, your
> *_convert_ctx_access() would map from __wifi_sk_buff to the actual
> skb member, for example:
>
> [...]
> case offsetof(struct __wifi_sk_buff, len):
> BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, len) != 4);
>
> *insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
> offsetof(struct sk_buff, len));
> break;
> [...]
>
> Your *_func_proto() would just reuse the available skb helpers like:
>
> switch (func_id) {
> case BPF_FUNC_skb_load_bytes:
> return &bpf_skb_load_bytes_proto;
> [...]
> }
>
> So, it's not that you need a local struct xdp_buff -like definition
> in the kernel and convert all helpers to it, reusing skb in kernel
> works just fine this way.
Sure.
> The mapping in samples/bpf/bpf_helpers.h, for example, for mentioned
> bpf_skb_load_bytes() would also work out of the box, since it takes a
> void *ctx as an argument, so you can just pass the __wifi_sk_buff
> pointer as ctx there from program side.
Hah. That's what I was missing - I always assumed the argument was
"struct __sk_buff *" without ever checking that assumption.
> Assuming __wifi_sk_buff would get few wifi specific attributes over
> time which cannot be reused in other types, it's probably fine to
> go with a __wifi_sk_buff uapi definition. If helper functions
> dedicated to wifi program type can extract all necessary information,
> it's probably okay as well to go that route.
The thing is that __wifi_sk_buff doesn't have much information that's
generally useful available - for example, there's not much point in
allowing it to access the raw rate data, having the actual converted
bitrate is much more useful, and that requires a function call since I
don't want the overhead of calculating that in cases the program
doesn't need it.
> If the data passed to such a helper function would eventually be a
> __wifi_sk_buff-like struct that gets extended with further attributes
> over time, then it's probably easier to use __wifi_sk_buff itself as
> a ctx instead of argument for helpers. Reason is that once a helper
> is set in stone, you need to keep compatibility on the passed struct,
> meaning you need to test the passed length of the struct like we do
> in case of bpf_skb_get_tunnel_key() / bpf_skb_set_tunnel_key()
> helpers and only copy meta data up to that length for older programs.
Right.
johannes
^ permalink raw reply
* Re: [RFC 1/3] bpf/wireless: add wifimon program type
From: Daniel Borkmann @ 2017-04-18 10:58 UTC (permalink / raw)
To: Johannes Berg, Alexei Starovoitov
Cc: David Miller, linux-wireless, netdev, ast
In-Reply-To: <1492509349.2472.15.camel@sipsolutions.net>
On 04/18/2017 11:55 AM, Johannes Berg wrote:
> On Fri, 2017-04-14 at 11:51 -0700, Alexei Starovoitov wrote:
>>
>> so today only 'len' field is needed,
>
> Correct, the other __sk_buff fields don't apply.
>
> It's more of an XDP model (with data/data_end), but as the SKB might be
> non-linear at this point I prefer to have the SKB so that skb data
> access (skb_copy_bits equivalent) works.
Note that for skbs the data / data_end model (aka direct read / write)
is also supported. There's also a bpf_skb_pull_data() helper that
pulls in data from non-linear parts if necessary (f.e. if data /
data_end test in the program fails). bpf_skb_load_bytes() is fine as
well, comes with function call overhead, though, but depending on the
use-case that might be just fine, too.
>> but the plan to add wifi specific
>> stuff to bpf context?
>
> Maybe, maybe not.
>
>> If so, adding these very wifi specific fields to __sk_buff will
>> confuse other program types,
>
> I don't think this is true - the verifier still checks that you can't
> actually use them. It might confuse authors though, if not documented
> well.
Yeah, *_is_valid_access() callbacks would need to reject these members
for most of the program types.
>> so new program type (like you did) and new 'struct bpf_wifi_md' are
>> probably cleaner.
>
> The problem is that I still need struct __wifi_sk_buff or so, and need
> to internally let it operate like an SKB, since I need
> bpf_skb_load_bytes().
>
> Now, bpf_helpers declares bpf_skb_load_bytes() to take an argument of
> type "struct __sk_buff *", and thus I either need to provide an alias
> to it, or cast every time I use this function.
Assuming you would introduce __wifi_sk_buff to the uapi, then it
would be that the kernel internally still operates on an skb, you'd
still call the program through BPF_PROG_RUN(prog, skb). However, your
*_convert_ctx_access() would map from __wifi_sk_buff to the actual
skb member, for example:
[...]
case offsetof(struct __wifi_sk_buff, len):
BUILD_BUG_ON(FIELD_SIZEOF(struct sk_buff, len) != 4);
*insn++ = BPF_LDX_MEM(BPF_W, si->dst_reg, si->src_reg,
offsetof(struct sk_buff, len));
break;
[...]
Your *_func_proto() would just reuse the available skb helpers like:
switch (func_id) {
case BPF_FUNC_skb_load_bytes:
return &bpf_skb_load_bytes_proto;
[...]
}
So, it's not that you need a local struct xdp_buff -like definition
in the kernel and convert all helpers to it, reusing skb in kernel
works just fine this way.
The mapping in samples/bpf/bpf_helpers.h, for example, for mentioned
bpf_skb_load_bytes() would also work out of the box, since it takes a
void *ctx as an argument, so you can just pass the __wifi_sk_buff
pointer as ctx there from program side.
Assuming __wifi_sk_buff would get few wifi specific attributes over
time which cannot be reused in other types, it's probably fine to
go with a __wifi_sk_buff uapi definition. If helper functions dedicated
to wifi program type can extract all necessary information, it's
probably okay as well to go that route.
If the data passed to such a helper function would eventually be a
__wifi_sk_buff-like struct that gets extended with further attributes
over time, then it's probably easier to use __wifi_sk_buff itself as
a ctx instead of argument for helpers. Reason is that once a helper
is set in stone, you need to keep compatibility on the passed struct,
meaning you need to test the passed length of the struct like we do
in case of bpf_skb_get_tunnel_key() / bpf_skb_set_tunnel_key() helpers
and only copy meta data up to that length for older programs.
>> Physically the R1 register to bpf program will still be 'struct
>> sk_buff', but from bpf program side it will be:
>> struct bpf_wifi_md {
>> __u32 len;
>> __u32 wifi_things;
>> };
>
> Right, that would work immediately if it's only about the extra fields.
> But it's more about being able to call bpf_skb_load_bytes() easily.
>
> I don't even know if I want to add *any* wifi_things here at all. I
> don't actually have much data available directly at this point, or at
> least not in a format that would be useful, so I think it'd be better
> to have a BPF helper function to obtain wifi_things outside of the
> struct itself, passing the struct bpf_wifi_md * (and thus getting
> struct sk_buff * in the kernel code) to it.
>
>> At the same time if most of the __sk_buff fields can be useful to
>> wifimon, then just use it as-is (without restricting to 'len' only)
>> and add wifi specific fields to it with a comment.
>
> No, I don't think they can ever be useful.
>
> johannes
^ permalink raw reply
* Re: bluetooth 6lowpan interfaces are not virtual anymore
From: Luiz Augusto von Dentz @ 2017-04-18 10:43 UTC (permalink / raw)
To: Alexander Aring
Cc: Network Development, Jukka Rissanen,
linux-wpan-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
Linux Bluetooth
In-Reply-To: <2d178eb8-3fbc-3385-6e0c-fa9941713959-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
Hi Alex,
On Mon, Apr 17, 2017 at 8:56 PM, Alexander Aring <aar-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> Hi,
>
> bluetooth-next contains patches which introduces a queue for bluetooth
> 6LoWPAN interfaces. [0]
>
> At first, the current behaviour is now that 802.15.4 6LoWPAN interfaces
> are virtual and bluetooth 6LoWPAN interfaces are not virtual anymore.
> To have a different handling in both subsystems is _definitely_ wrong.
Well 15.4 and Bluetooth have very different handling, iirc in 15.4 you
do have a real interface which has, in fact, queueing support:
net/mac802154/iface.c:ieee802154_if_setup:
dev->tx_queue_len = 300;
> What does the 6LoWPAN interface?
>
> It will do a protocol change (an adaptation, because 6LoWPAN should
> provide the same functionality as IPv6) from IPv6 to 6LoWPAN (tx) and
> vice versa for (rx). In my opinion this should be handled as a virtual
> interface and not as an interface with a queue.
Then we need a real netif for Bluetooth as well, one that does
queueing since introducing for l2cap_chan makes no sense when most of
time the userspace has a socket which does its own queueing. Btw, I
have the opinion that doing a second interface just to make Bluetooth
behave like 15.4 is very wasteful since we don't have any other
network support except for bnep, which in fact does the same thing
regarding queueing.
> What makes a queue on 6LoWPAN interfaces?
>
> It will queue IPv6 packets which waits for it adaptation (the protocol
> change) with some additional qdisc handling.
> If finally the xmit_do callback will occur it will replace IPv6 header
> with 6LoWPAN, etc. After that it should be queued into some queue on
> link layer side which should be do the transmit finally.
In case of Bluetooth it does the Link Layer using L2CAP, which is not
exactly at Link Layer, in fact it is one of the major problems of
having this in Kernel space since it is similar of doing IP over TCP
tunnel.
> Why I think bluetooth introduced a queue handling on 6LoWPAN interfaces?
>
> Because I think they don't like their own *qdisc* handling on their link
> layer interface. I write *qdisc* here because, they have no net_devices
> and use some kind of own qdisc handling.
No we don't, and except we do change the whole architecture of
Bluetooth to work as a net_device I don't think we can have a similar
design as 15.4. Anyway Bluetooth is not really designed to carry IP,
the L2CAP and other profiles above are mostly a Bluetooth thing so
having a net_device would not buy us much more than queueing for bnep
and ipsp.
> My question: is this correct?
>
> How to fix that (In my opinion):
>
> So commit [0] says something "out of credits" that's what I think it's
> the *qdisc* handling. If you cannot queue anymore -> you need to drop
> it. If you don't like how the current behaviour is, you need to change
> your *qdisc* handling on your link layer interface. Introducing queue at
> 6LoWPAN interfaces will introduce "buffer bloating".
That is not what the code comments says, eg. netif_stop_queue:
* Stop upper layers calling the device hard_start_xmit routine.
* Used for flow control when transmit resources are unavailable.
The fact 15.4, and bnep, uses these APIs makes me believe this is the
proper way of using qdisc handling, the only difference here is at
what level we would be using them.
> ---
>
> I don't care what bluetooth does with the 6LoWPAN interface. If bluetooth
> people wants such behaviour, then I am okay with that.
>
> I will bookmark this mail for the time when somebody reverts it to a
> virtual interface again. I think somebody will change it again, or maybe
> somebody will argument why we need a queue here.
>From what I could grasp from the code except if 6LoWPAN start creating
its own interface, which it doesn't currently, it should never assume
any specific behavior for an interface. 6LoWPAN right now only have
helper functions which the interface code call on TX/RX, and by
looking at code under 15.4 the only thing we would be doing is to set
skb->dev = wdev; (wdev being the real device) as Bluetooth don't use
6lowpan fragmentation.
Btw, another big different in terms of design that it seems 15.4 does
create interfaces to each and every link, in Bluetooth the interface
is per adapter so it is in fact multi-link, I guess in practice 15.4
shall only have one 6lowpan link since it is connected to mesh, but in
Bluetooth we may have more than one Link and I don't think it would be
a good idea to have each of these links as a different interface,
especially not with the same mac address as it seems to be what 15.4
code is doing:
/* Set the lowpan hardware address to the wpan hardware address. */
memcpy(ldev->dev_addr, wdev->dev_addr, IEEE802154_ADDR_LEN);
So at least with IPSP/RFC-7668 the topology is completely different
from what we can see in 15.4, it may happen that this changes with
upcoming changes to Bluetooth, adding yet another topology that we
will need to support. Personally I wouldn't mind if 15.4 and Bluetooth
had more in common, at least at 6lowpan level, so we wouldn't have the
need to create separated modules just to deal with their differences,
but in reality, these technologies are competing for the same market
so I don't think it will happen, unfortunately.
> - Alex
>
> [0] https://git.kernel.org/pub/scm/linux/kernel/git/bluetooth/bluetooth-next.git/commit/?id=814f1b243d2c63928f6aa72d66bec0e5fae2f4a9
--
Luiz Augusto von Dentz
^ permalink raw reply
* Re: [PATCH v2 net-next] net: stmmac: add drop transmit status feature
From: Joao Pinto @ 2017-04-18 10:25 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, Joao Pinto; +Cc: davem, netdev
In-Reply-To: <b8207712-0191-20be-a6dd-ca91d9fa1440@gmail.com>
Hi Florian,
Às 6:43 PM de 4/12/2017, Florian Fainelli escreveu:
> On 04/12/2017 07:58 AM, Andrew Lunn wrote:
>> On Wed, Apr 12, 2017 at 01:07:02PM +0100, Joao Pinto wrote:
>>> Hi Andrew,
>>>
>>> Às 12:56 PM de 4/12/2017, Andrew Lunn escreveu:
>>>> On Wed, Apr 12, 2017 at 10:26:20AM +0100, Joao Pinto wrote:
>>>>> When the Drop Transmit Status bit is set, the Tx packet status
>>>>> received from the MAC is dropped in the MTL. When this bit is reset,
>>>>> the Tx packet status received from the MAC is forwarded to the
>>>>> application. This feature will cause a performance improvement.
>>>>>
>>>>> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
>>>>> ---
>>>>> changes v1->v2:
>>>>> - removed mask from dwmac4_enable_tx_drop()
>>>>>
>>>>> Documentation/devicetree/bindings/net/stmmac.txt | 1 +
>>>>> drivers/net/ethernet/stmicro/stmmac/common.h | 2 ++
>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4.h | 1 +
>>>>> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c | 12 ++++++++++++
>>>>> drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 3 +++
>>>>> drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 2 ++
>>>>> include/linux/stmmac.h | 1 +
>>>>> 7 files changed, 22 insertions(+)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/net/stmmac.txt b/Documentation/devicetree/bindings/net/stmmac.txt
>>>>> index f652b0c..dbcb2cc 100644
>>>>> --- a/Documentation/devicetree/bindings/net/stmmac.txt
>>>>> +++ b/Documentation/devicetree/bindings/net/stmmac.txt
>>>>> @@ -60,6 +60,7 @@ Optional properties:
>>>>> and MAC2MAC connection.
>>>>> - snps,tso: this enables the TSO feature otherwise it will be managed by
>>>>> MAC HW capability register. Only for GMAC4 and newer.
>>>>> +- snps,drop-tx-status: this enables drop tx status
>>>>
>>>> Hi Joao
>>>>
>>>> Was the conclusion from testing that this cannot be turned on by
>>>> default?
>>>
>>> This feature is great for applications that need good performance, but has a
>>> drawback since it has an impact in timestamp feature in Tx. There are some
>>> operations in PTP where the timestamp is given to the host through the TX status
>>> in the descriptor, so this will have an impact.
>>>
>>> There's a way of solving this of course by making the driver checking the
>>> timestamp in the MAC_Tx_Timestamp_Status_XXX registers, but I can only look into
>>> that feature later in the future.
>>
>> So the numbers you show, even if they are not on real hardware that
>> anybody uses, do look good.
>>
>> But i don't like this DT property, it sounds like it can potential do
>> bad things to PTP, and you say it can be done without the DT or PTP
>> problems. So I would personally NACK this patch, and ask you to
>> re-submit when you have solved these issues.
>
> Is not it possible to simply figure out whether this can safely be
> enabled at run time or not? If nobody uses PTP, then turn that on as
> soon as you get someone using PTP, turn this back off?
Yes that might work, and cause a performance improvement since TX processing
would be lighter. According to the R&D, the PTP problem can be overcome by
reading a special register that contains the timestamping, but I will need to
investigate that more.
I am going to start working on TSN features soon, and then I will get the "Drop
Status" topic again with the PTP issue solved and submit the patch again.
>
> The problem is the commit message is extremely scarce about what this
> implies, and quite frankly, I don't understand what "application" means
> here, whether this means:
>
> - application in the sense of the HW IP, which is then, the OS'
> networking stack presumably?
>
> - application in the sense of the OS, which is sort of conflated with
> the socket user?
>
> Worst case, can't you create a new netdev feature (e.g: similar to
> NETIF_F_RXFCS) that allows dynamically turning this on based on user
> configuration? Not suggesting this is the right approach here, but it
> should be thought of. Maybe an ethtool private flag is more appropriate?
>
> One perspective you need to look at is the following:
>
> - as an IP vendor, you would like to exercise every little configuration
> knob that the IP can be synthesized with, and that's fine and fun to do
> as long as you are in your controlled environment, this may result in
> custom DT properties, but those may not be candidate for the upstream
> kernel (just downstream)
Totally agree :), I done that with a local gitlab, since the Drop Status feature
will useful in some test cases I will be working on.
Sometimes we don't have the 100% of the thing and as you say some things work
well in a controlled environment but can be complicated in production
environment if not done to wok out of the box.
>
> - customers want something that works out of the box, don't require
> reading a DT binding (in fact, not reading at all is best) and just get
> the promised performance, and the upstream kernel is the best place to
> get this done, but it needs to be painless
Understand and totally agree with you.
>
> Finally, and completely tangential to this patch, pretty much *every*
> SoC that has a STMMAC is reasonably cheap to buy, so if Synopsys does
> not, you could, with your own personal money acquire a full set of
> Allwinner, Meson, Sunxi and what not platforms and put these in a test
> labs, heck, I'm sure someone could offer these. Most maintainers just
> collect HW as well as tribal knowledge about what typically breaks on
> this or that platform, and then run their tests on said platforms.
>
I managed to get a several prototypes for ETH 3.x, 4.x and latest 5.x to have
more test power, but they are prototypes with the "ideal" design configuration,
so we can have suprises in real world hardware, since the original designs are
then adapted to fit the SoC. I totally understand this topic and I already
raised the question inside, so lets see how it develops.
> Hope this helps!
>
Thank you very much for your e-mail, it was very helpful!
Joao
^ permalink raw reply
* [PATCH v2 net 2/2] net sched actions: decrement module refcount earlier
From: Wolfgang Bumiller @ 2017-04-18 10:13 UTC (permalink / raw)
To: netdev; +Cc: Jamal Hadi Salim, David S. Miller, Cong Wang
In-Reply-To: <20170418101322.27666-1-w.bumiller@proxmox.com>
Whether the reference count has to be decremented depends
on whether the policy was created. If TCA_ACT_COOKIE is
passed and an error occurs there, the same condition still
has to be honored.
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Cc: Jamal Hadi Salim <jhs@mojatatu.com>
---
I did not include the Acked-bys here because I've noticed that this
is still wrong. After reading a bit more and doing more tests with
different filters I realized that the `name != NULL` case is specific
to the police filter only. For the other filters this patch here breaks
refcounting in the error case (I included it for reference only).
I'm thinking the first patch should be enough. (I've tested forcing the
other filters into the error path *without* this patch and couldn't
produce crashes or reference count problems (while with this patch
applied it was leaking reference counts on creation (which makes sense
considering tcf_hash_release is used and the ACT_P_CREATED case will
keep repeating)). (Whereas without both patches simply looking through
creating and deleting a policing filter pretty much always resulted in
crashes with various different backtraces.)
(I'd still like to clarify the comment in the code btw.)
net/sched/act_api.c | 18 ++++++++++--------
1 file changed, 10 insertions(+), 8 deletions(-)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index 8cc883c063f0..4493f1b9d22b 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -604,28 +604,30 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
if (err < 0)
goto err_mod;
+ /* The module count should only go up when a brand new policy was
+ * created, if it exists and is only bound to in a_o->init() then
+ * ACT_P_CREATED is not returned (a zero is), and we need to roll back
+ * the bump caused by tc_lookup_action_n().
+ */
+ if (err != ACT_P_CREATED)
+ module_put(a_o->owner);
+
if (name == NULL && tb[TCA_ACT_COOKIE]) {
int cklen = nla_len(tb[TCA_ACT_COOKIE]);
if (cklen > TC_COOKIE_MAX_SIZE) {
err = -EINVAL;
tcf_hash_release(a, bind);
- goto err_mod;
+ goto err_out;
}
if (nla_memdup_cookie(a, tb) < 0) {
err = -ENOMEM;
tcf_hash_release(a, bind);
- goto err_mod;
+ goto err_out;
}
}
- /* module count goes up only when brand new policy is created
- * if it exists and is only bound to in a_o->init() then
- * ACT_P_CREATED is not returned (a zero is).
- */
- if (err != ACT_P_CREATED)
- module_put(a_o->owner);
return a;
--
2.11.0
^ permalink raw reply related
* [PATCH v2 net 1/2] net sched actions: fix access to uninitialized data
From: Wolfgang Bumiller @ 2017-04-18 10:13 UTC (permalink / raw)
To: netdev; +Cc: Jamal Hadi Salim, David S. Miller, Cong Wang
Signed-off-by: Wolfgang Bumiller <w.bumiller@proxmox.com>
Acked-by: Cong Wang <xiyou.wangcong@gmail.com>
Acked-by: Jamal Hadi Salim <jhs@mojatatu.com>
---
(same as v1)
net/sched/act_api.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sched/act_api.c b/net/sched/act_api.c
index b70aa57319ea..8cc883c063f0 100644
--- a/net/sched/act_api.c
+++ b/net/sched/act_api.c
@@ -604,7 +604,7 @@ struct tc_action *tcf_action_init_1(struct net *net, struct nlattr *nla,
if (err < 0)
goto err_mod;
- if (tb[TCA_ACT_COOKIE]) {
+ if (name == NULL && tb[TCA_ACT_COOKIE]) {
int cklen = nla_len(tb[TCA_ACT_COOKIE]);
if (cklen > TC_COOKIE_MAX_SIZE) {
--
2.11.0
^ permalink raw reply related
* Re: [PATCH v2 0/8] NFC: fix device allocation and nfcmrvl crashes
From: Johan Hovold @ 2017-04-18 10:09 UTC (permalink / raw)
To: Samuel Ortiz
Cc: linux-wireless, netdev, linux-kernel, Johan Hovold,
Aloisio Almeida Jr, Lauro Ramos Venancio
In-Reply-To: <20170330101542.15384-1-johan@kernel.org>
On Thu, Mar 30, 2017 at 12:15:34PM +0200, Johan Hovold wrote:
> This started out with the observation that the nfcmrvl_uart driver
> unconditionally dereferenced the tty class device despite the fact that
> not every tty has an associated struct device (Unix98 ptys). Some
> further changes were needed in the common nfcmrvl code to fully address
> this, some of which also incidentally fixed a few related bugs (e.g.
> resource leaks in error paths).
>
> While fixing this I stumbled over a regression in NFC core that lead to
> broken registration error paths and misnamed workqueues.
>
> Note that this has only been tested by configuring the n_hci line
> discipline for different ttys without any actual NFC hardware connected.
> Johan Hovold (8):
> NFC: fix broken device allocation
> NFC: nfcmrvl_uart: add missing tty-device sanity check
> NFC: nfcmrvl: do not use device-managed resources
> NFC: nfcmrvl: use nfc-device for firmware download
> NFC: nfcmrvl: fix firmware-management initialisation
> NFC: nfcmrvl_uart: fix device-node leak during probe
> NFC: nfcmrvl_usb: use interface as phy device
> NFC: nfcmrvl: allow gpio 0 for reset signalling
Any chance of getting these into 4.12, Samuel?
Note that patches 2 and 4 fixes NULL-derefs that can be triggered by an
unprivileged user.
Thanks,
Johan
^ permalink raw reply
* Re: [RFC 1/3] bpf/wireless: add wifimon program type
From: Johannes Berg @ 2017-04-18 9:55 UTC (permalink / raw)
To: Alexei Starovoitov; +Cc: David Miller, linux-wireless, netdev, daniel, ast
In-Reply-To: <20170414185144.GB41922@ast-mbp.thefacebook.com>
On Fri, 2017-04-14 at 11:51 -0700, Alexei Starovoitov wrote:
>
> so today only 'len' field is needed,
Correct, the other __sk_buff fields don't apply.
It's more of an XDP model (with data/data_end), but as the SKB might be
non-linear at this point I prefer to have the SKB so that skb data
access (skb_copy_bits equivalent) works.
> but the plan to add wifi specific
> stuff to bpf context?
Maybe, maybe not.
> If so, adding these very wifi specific fields to __sk_buff will
> confuse other program types,
I don't think this is true - the verifier still checks that you can't
actually use them. It might confuse authors though, if not documented
well.
> so new program type (like you did) and new 'struct bpf_wifi_md' are
> probably cleaner.
The problem is that I still need struct __wifi_sk_buff or so, and need
to internally let it operate like an SKB, since I need
bpf_skb_load_bytes().
Now, bpf_helpers declares bpf_skb_load_bytes() to take an argument of
type "struct __sk_buff *", and thus I either need to provide an alias
to it, or cast every time I use this function.
> Physically the R1 register to bpf program will still be 'struct
> sk_buff', but from bpf program side it will be:
> struct bpf_wifi_md {
> __u32 len;
> __u32 wifi_things;
> };
Right, that would work immediately if it's only about the extra fields.
But it's more about being able to call bpf_skb_load_bytes() easily.
I don't even know if I want to add *any* wifi_things here at all. I
don't actually have much data available directly at this point, or at
least not in a format that would be useful, so I think it'd be better
to have a BPF helper function to obtain wifi_things outside of the
struct itself, passing the struct bpf_wifi_md * (and thus getting
struct sk_buff * in the kernel code) to it.
> At the same time if most of the __sk_buff fields can be useful to
> wifimon, then just use it as-is (without restricting to 'len' only)
> and add wifi specific fields to it with a comment.
No, I don't think they can ever be useful.
johannes
^ permalink raw reply
* [PATCH net-next 2/3] net: hns: support deferred probe when no mdio
From: Yankejian @ 2017-04-18 10:12 UTC (permalink / raw)
To: davem, salil.mehta, yisen.zhuang, allenhuangsz10, lipeng321,
huangdaode, hjat.hejun, zhangping5, qichengming, qumingguang,
zhouhuiru
Cc: netdev, charles.chenxin, linuxarm
In-Reply-To: <1492510354-11300-1-git-send-email-yankejian@huawei.com>
From: lipeng <lipeng321@huawei.com>
In the hip06 and hip07 SoCs, phy connect to mdio bus.The mdio
module is probed with module_init, and, as such,
is not guaranteed to probe before the HNS driver. So we need
to support deferred probe.
We check for probe deferral in the mac init, so we not init DSAF
when there is no mdio, and free all resource, to later learn that
we need to defer the probe.
Signed-off-by: lipeng <lipeng321@huawei.com>
---
drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c | 34 +++++++++++++++++++----
1 file changed, 28 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
index bdd8cdd..284ebfe 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_mac.c
@@ -735,6 +735,8 @@ static int hns_mac_init_ex(struct hns_mac_cb *mac_cb)
rc = phy_device_register(phy);
if (rc) {
phy_device_free(phy);
+ dev_err(&mdio->dev, "registered phy fail at address %i\n",
+ addr);
return -ENODEV;
}
@@ -745,7 +747,7 @@ static int hns_mac_init_ex(struct hns_mac_cb *mac_cb)
return 0;
}
-static void hns_mac_register_phy(struct hns_mac_cb *mac_cb)
+static int hns_mac_register_phy(struct hns_mac_cb *mac_cb)
{
struct acpi_reference_args args;
struct platform_device *pdev;
@@ -755,24 +757,39 @@ static void hns_mac_register_phy(struct hns_mac_cb *mac_cb)
/* Loop over the child nodes and register a phy_device for each one */
if (!to_acpi_device_node(mac_cb->fw_port))
- return;
+ return 0;
rc = acpi_node_get_property_reference(
mac_cb->fw_port, "mdio-node", 0, &args);
if (rc)
- return;
+ return 0;
addr = hns_mac_phy_parse_addr(mac_cb->dev, mac_cb->fw_port);
if (addr < 0)
- return;
+ return 0;
/* dev address in adev */
pdev = hns_dsaf_find_platform_device(acpi_fwnode_handle(args.adev));
+ if (!pdev) {
+ dev_err(mac_cb->dev, "mac%d mdio pdev is NULL\n",
+ mac_cb->mac_id);
+ return 0;
+ }
+
mii_bus = platform_get_drvdata(pdev);
+ if (!mii_bus) {
+ dev_err(mac_cb->dev,
+ "mac%d mdio is NULL, dsaf will probe again later\n",
+ mac_cb->mac_id);
+ return -EPROBE_DEFER;
+ }
+
rc = hns_mac_register_phydev(mii_bus, mac_cb, addr);
if (!rc)
dev_dbg(mac_cb->dev, "mac%d register phy addr:%d\n",
mac_cb->mac_id, addr);
+
+ return rc;
}
#define MAC_MEDIA_TYPE_MAX_LEN 16
@@ -793,7 +810,7 @@ static void hns_mac_register_phy(struct hns_mac_cb *mac_cb)
*@np:device node
* return: 0 --success, negative --fail
*/
-static int hns_mac_get_info(struct hns_mac_cb *mac_cb)
+static int hns_mac_get_info(struct hns_mac_cb *mac_cb)
{
struct device_node *np;
struct regmap *syscon;
@@ -903,7 +920,10 @@ static int hns_mac_get_info(struct hns_mac_cb *mac_cb)
}
}
} else if (is_acpi_node(mac_cb->fw_port)) {
- hns_mac_register_phy(mac_cb);
+ ret = hns_mac_register_phy(mac_cb);
+
+ if (ret)
+ return ret;
} else {
dev_err(mac_cb->dev, "mac%d cannot find phy node\n",
mac_cb->mac_id);
@@ -1065,6 +1085,7 @@ int hns_mac_init(struct dsaf_device *dsaf_dev)
dsaf_dev->mac_cb[port_id] = mac_cb;
}
}
+
/* init mac_cb for all port */
for (port_id = 0; port_id < max_port_num; port_id++) {
mac_cb = dsaf_dev->mac_cb[port_id];
@@ -1074,6 +1095,7 @@ int hns_mac_init(struct dsaf_device *dsaf_dev)
ret = hns_mac_get_cfg(dsaf_dev, mac_cb);
if (ret)
return ret;
+
ret = hns_mac_init_ex(mac_cb);
if (ret)
return ret;
--
1.9.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox