* Re: [PATCH v2] net-fq: Add WARN_ON check for null flow.
From: Cong Wang @ 2018-06-08 0:13 UTC (permalink / raw)
To: Ben Greear; +Cc: Linux Kernel Network Developers
In-Reply-To: <1528415316-6379-1-git-send-email-greearb@candelatech.com>
On Thu, Jun 7, 2018 at 4:48 PM, <greearb@candelatech.com> wrote:
> From: Ben Greear <greearb@candelatech.com>
>
> While testing an ath10k firmware that often crashed under load,
> I was seeing kernel crashes as well. One of them appeared to
> be a dereference of a NULL flow object in fq_tin_dequeue.
>
> I have since fixed the firmware flaw, but I think it would be
> worth adding the WARN_ON in case the problem appears again.
>
> BUG: unable to handle kernel NULL pointer dereference at 000000000000003c
> IP: ieee80211_tx_dequeue+0xfb/0xb10 [mac80211]
Instead of adding WARN_ON(), you need to think about
the locking there, it is suspicious:
fq is from struct ieee80211_local:
struct fq *fq = &local->fq;
tin is from struct txq_info:
struct fq_tin *tin = &txqi->tin;
I don't know if fq and tin are supposed to be 1:1, if not there is
a bug in the locking, because ->new_flows and ->old_flows are
both inside tin instead of fq, but they are protected by fq->lock....
^ permalink raw reply
* Re: [Bug 199643] New: UBSAN: Undefined behaviour in ./include/net/route.h:240:2
From: Eric Dumazet @ 2018-06-08 0:28 UTC (permalink / raw)
To: David Miller, jakub.kicinski; +Cc: dsahern, stephen, netdev
In-Reply-To: <20180607.201142.389815374334636334.davem@davemloft.net>
On 06/07/2018 05:11 PM, David Miller wrote:
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Date: Thu, 7 Jun 2018 17:06:23 -0700
>
>> [ 293.213661] ip_send_unicast_reply+0x1b67/0x1d0e
>
> This calls ip_setup_cork() which can NULL out the 'rt' route
> pointer. Hmmm... :-/
>
UBSAN seems unhappy with dst being NULL in :
dst_release(&rt->dst);
But the code obviously is ready for dst being NULL, it is even documented :)
^ permalink raw reply
* Re: [Bug 199643] New: UBSAN: Undefined behaviour in ./include/net/route.h:240:2
From: Jakub Kicinski @ 2018-06-08 0:49 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, dsahern, stephen, netdev
In-Reply-To: <e05416a5-b4fe-6e9f-cefc-86848fb146cf@gmail.com>
On Thu, 7 Jun 2018 17:28:59 -0700, Eric Dumazet wrote:
> On 06/07/2018 05:11 PM, David Miller wrote:
> > From: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Date: Thu, 7 Jun 2018 17:06:23 -0700
> >
> >> [ 293.213661] ip_send_unicast_reply+0x1b67/0x1d0e
> >
> > This calls ip_setup_cork() which can NULL out the 'rt' route
> > pointer. Hmmm... :-/
> >
>
>
> UBSAN seems unhappy with dst being NULL in :
>
> dst_release(&rt->dst);
>
> But the code obviously is ready for dst being NULL, it is even documented :)
Oh, so the code depends on dst being the first member? Would it make
sense to just cast the pointer instead?
^ permalink raw reply
* Re: next-20180605 - BUG in ipv6_add_addr
From: David Ahern @ 2018-06-08 0:51 UTC (permalink / raw)
To: valdis.kletnieks; +Cc: netdev, linux-kernel
In-Reply-To: <10902.1528416198@turing-police.cc.vt.edu>
On 6/7/18 5:03 PM, valdis.kletnieks@vt.edu wrote:
> On Thu, 07 Jun 2018 16:49:07 -0700, David Ahern said:
>> On 6/7/18 1:17 PM, valdis.kletnieks@vt.edu wrote:
>
>>> [ 1820.832682] BUG: unable to handle kernel NULL pointer dereference at 0000000000000209
>>> [ 1820.832728] RIP: 0010:ipv6_add_addr+0x280/0xd10
>
>>> [ 1820.832888] Call Trace:
>>> [ 1820.832898] ? __local_bh_enable_ip+0x119/0x260
>>> [ 1820.832904] ? ipv6_create_tempaddr+0x259/0x5a0
>>> [ 1820.832912] ? __local_bh_enable_ip+0x139/0x260
>>> [ 1820.832921] ipv6_create_tempaddr+0x2da/0x5a0
>>> [ 1820.832926] ? ipv6_create_tempaddr+0x2da/0x5a0
>>> [ 1820.832941] manage_tempaddrs+0x1a5/0x240
>>> [ 1820.832951] inet6_addr_del+0x20b/0x3b0
>>> [ 1820.832959] ? nla_parse+0xce/0x1e0
>>> [ 1820.832968] inet6_rtm_deladdr+0xd9/0x210
>>> [ 1820.832981] rtnetlink_rcv_msg+0x1d4/0x5f0
>>
>> I am the most likely guilty party. I have been staring at the code for
>> this stack trace for a while and nothing jumps out. Can you send me the
>> kernel config?
>
> Attached. Note that this one happened while I was on wireless at work,
> where we're *heavily* IPv6 (I've had days where I'll work for 2-3 hours before
> I notice that IPv4 didn't dhcp and I've been ipv6-only the whole time.
>
> Also, the interface was config'ed as:
>
> conf/wlp3s0b1/temp_prefered_lft:86400
> conf/wlp3s0b1/temp_valid_lft:604800
> conf/wlp3s0b1/use_tempaddr:2
>
I know you don't have a reliable reproducer, but I did find one spot
where I was too clever and did not initialize a new cfg variable:
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index 89019bf59f46..59c22a25e654 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1324,6 +1324,7 @@ static int ipv6_create_tempaddr(struct
inet6_ifaddr *ifp,
}
}
+ memset(&cfg, 0, sizeof(cfg));
cfg.valid_lft = min_t(__u32, ifp->valid_lft,
idev->cnf.temp_valid_lft + age);
cfg.preferred_lft = cnf_temp_preferred_lft + age -
idev->desync_factor;
^ permalink raw reply related
* Re: [Bug 199643] New: UBSAN: Undefined behaviour in ./include/net/route.h:240:2
From: David Ahern @ 2018-06-08 0:53 UTC (permalink / raw)
To: Jakub Kicinski, Eric Dumazet; +Cc: David Miller, stephen, netdev
In-Reply-To: <20180607174923.13a11d08@cakuba.netronome.com>
On 6/7/18 5:49 PM, Jakub Kicinski wrote:
> On Thu, 7 Jun 2018 17:28:59 -0700, Eric Dumazet wrote:
>> On 06/07/2018 05:11 PM, David Miller wrote:
>>> From: Jakub Kicinski <jakub.kicinski@netronome.com>
>>> Date: Thu, 7 Jun 2018 17:06:23 -0700
>>>
>>>> [ 293.213661] ip_send_unicast_reply+0x1b67/0x1d0e
>>>
>>> This calls ip_setup_cork() which can NULL out the 'rt' route
>>> pointer. Hmmm... :-/
>>>
>>
>>
>> UBSAN seems unhappy with dst being NULL in :
>>
>> dst_release(&rt->dst);
>>
>> But the code obviously is ready for dst being NULL, it is even documented :)
>
> Oh, so the code depends on dst being the first member? Would it make
> sense to just cast the pointer instead?
>
I've been going the other way with 'rt to dst' and 'dst to rt'
transformations.
Perhaps UBSAN should be updated to understand that NULL + 0 is ok.
^ permalink raw reply
* Re: [Bug 199643] New: UBSAN: Undefined behaviour in ./include/net/route.h:240:2
From: Jakub Kicinski @ 2018-06-08 1:02 UTC (permalink / raw)
To: Andrey Ryabinin; +Cc: David Ahern, Eric Dumazet, David Miller, stephen, netdev
In-Reply-To: <e6d18d55-2e35-b888-040f-b31117e38daf@gmail.com>
CC: Andrey
On Thu, 7 Jun 2018 17:53:35 -0700, David Ahern wrote:
> On 6/7/18 5:49 PM, Jakub Kicinski wrote:
> > On Thu, 7 Jun 2018 17:28:59 -0700, Eric Dumazet wrote:
> >> On 06/07/2018 05:11 PM, David Miller wrote:
> >>> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> >>> Date: Thu, 7 Jun 2018 17:06:23 -0700
> >>>
> >>>> [ 293.213661] ip_send_unicast_reply+0x1b67/0x1d0e
> >>>
> >>> This calls ip_setup_cork() which can NULL out the 'rt' route
> >>> pointer. Hmmm... :-/
> >>
> >> UBSAN seems unhappy with dst being NULL in :
> >>
> >> dst_release(&rt->dst);
> >>
> >> But the code obviously is ready for dst being NULL, it is even documented :)
> >
> > Oh, so the code depends on dst being the first member? Would it make
> > sense to just cast the pointer instead?
> >
>
> I've been going the other way with 'rt to dst' and 'dst to rt'
> transformations.
>
> Perhaps UBSAN should be updated to understand that NULL + 0 is ok.
^ permalink raw reply
* Re: [Bug 199637] New: UBSAN: Undefined behaviour in net/ipv4/fib_trie.c:503:6
From: David Ahern @ 2018-06-08 1:27 UTC (permalink / raw)
To: Jakub Kicinski, Stephen Hemminger; +Cc: netdev, David Miller
In-Reply-To: <20180607170759.176186fd@cakuba.netronome.com>
On 6/7/18 5:07 PM, Jakub Kicinski wrote:
>> After recompiling the 4.16.7 kernel with gcc 8.1, UBSAN reports the following:
>>
>> [ 25.427424]
>> ================================================================================
>> [ 25.429680] UBSAN: Undefined behaviour in net/ipv4/fib_trie.c:503:6
>> [ 25.431920] member access within null pointer of type 'struct tnode'
>> [ 25.434153] CPU: 3 PID: 1 Comm: systemd Not tainted 4.16.7-CUSTOM #1
>> [ 25.436384] Hardware name: Gigabyte Technology Co., Ltd.
>> H67MA-UD2H-B3/H67MA-UD2H-B3, BIOS F8 03/27/2012
>> [ 25.438647] Call Trace:
>> [ 25.440889] dump_stack+0x62/0x9f
>> [ 25.443104] ubsan_epilogue+0x9/0x35
>> [ 25.445293] handle_null_ptr_deref+0x80/0x90
>> [ 25.447464] __ubsan_handle_type_mismatch_v1+0x6a/0x80
>> [ 25.449628] tnode_free+0xce/0x120
arguably this one should be guarded:
diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
index 5bc0c89e81e4..32c589059fb3 100644
--- a/net/ipv4/fib_trie.c
+++ b/net/ipv4/fib_trie.c
@@ -501,7 +501,8 @@ static void tnode_free(struct key_vector *tn)
tnode_free_size += TNODE_SIZE(1ul << tn->bits);
node_free(tn);
- tn = container_of(head, struct tnode, rcu)->kv;
+ if (head)
+ tn = container_of(head, struct tnode, rcu)->kv;
}
if (tnode_free_size >= PAGE_SIZE * sync_pages) {
but if head is NULL, tn is set but not dereferenced as the loop breaks.
^ permalink raw reply related
* Proposal
From: Mr. Fawaz KhE. Al Saleh @ 2018-06-08 1:41 UTC (permalink / raw)
--
Good day,
i know you do not know me personally but i have checked your profile
and i see generosity in you, There's an urgent offer attach
to your name here in the office of Mr. Fawaz KhE. Al Saleh Member of
the Board of Directors, Kuveyt Türk Participation Bank (Turkey) and
head of private banking and wealth management
Regards,
Mr. Fawaz KhE. Al Saleh
^ permalink raw reply
* Re: [PATCH net] net/sched: act_simple: fix parsing of TCA_DEFDATA
From: Hangbin Liu @ 2018-06-08 2:07 UTC (permalink / raw)
To: Davide Caratti; +Cc: netdev
In-Reply-To: <bfe72bc56c5caba64cafaea1e19bccf80b6d563b.1528379074.git.dcaratti@redhat.com>
On Thu, Jun 07, 2018 at 03:46:43PM +0200, Davide Caratti wrote:
> use nla_strlcpy() to avoid copying data beyond the length of TCA_DEFDATA
s/TCA_DEFDATA/TCA_DEF_DATA/, incase someone search the commit history but
could not find it.
Thanks
Hangbin
^ permalink raw reply
* [iproute2 1/1] tipc: TIPC_NLA_LINK_NAME value pass on nesting entry TIPC_NLA_LINK
From: Hoang Le @ 2018-06-08 2:19 UTC (permalink / raw)
To: netdev, tipc-discussion, jon.maloy, maloy, ying.xue
In the commit 94f6a80 on next-net, TIPC_NLA_LINK_NAME attribute should be
retrieved and validated via TIPC_NLA_LINK nesting entry in
tipc_nl_node_get_link().
According to that commit, TIPC_NLA_LINK_NAME value passing via
tipc link get command must follow above hierachy.
Acked-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Hoang Le <hoang.h.le@dektech.com.au>
---
tipc/link.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tipc/link.c b/tipc/link.c
index 02f14aadefa6..a2d7c0016bc1 100644
--- a/tipc/link.c
+++ b/tipc/link.c
@@ -97,6 +97,7 @@ static int cmd_link_get_prop(struct nlmsghdr *nlh, const struct cmd *cmd,
{
int prop;
char buf[MNL_SOCKET_BUFFER_SIZE];
+ struct nlattr *attrs;
struct opt *opt;
struct opt opts[] = {
{ "link", OPT_KEYVAL, NULL },
@@ -131,7 +132,9 @@ static int cmd_link_get_prop(struct nlmsghdr *nlh, const struct cmd *cmd,
fprintf(stderr, "error, missing link\n");
return -EINVAL;
}
+ attrs = mnl_attr_nest_start(nlh, TIPC_NLA_LINK);
mnl_attr_put_strz(nlh, TIPC_NLA_LINK_NAME, opt->val);
+ mnl_attr_nest_end(nlh, attrs);
return msg_doit(nlh, link_get_cb, &prop);
}
--
2.1.4
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply related
* Re: [PATCH net-next] net: ipv6: Generate random IID for addresses on RAWIP devices
From: Lorenzo Colitti @ 2018-06-08 2:52 UTC (permalink / raw)
To: YOSHIFUJI Hideaki
Cc: Subash Abhinov Kasiviswanathan, David Miller, netdev,
YOSHIFUJI Hideaki
In-Reply-To: <CAPA1RqBmffQObdTWMxNSNMRTgM9gPjD4KGqk2+MTwdbR34Wkhw@mail.gmail.com>
On Mon, Jun 4, 2018 at 8:51 AM 吉藤英明 <hideaki.yoshifuji@miraclelinux.com> wrote:
>
> > + if (ipv6_get_lladdr(dev, &lladdr, IFA_F_TENTATIVE))
> > + get_random_bytes(eui, 8);
>
> Please be aware of I/G bit and G/L bit.
Actually, I think this is fine. RFC 7136 clarified this, and says:
======
Thus, we can conclude that the value of the "u" bit in IIDs has no
particular meaning. In the case of an IID created from a MAC address
according to RFC 4291, its value is determined by the MAC address,
but that is all.
[...]
Specifications of other forms of 64-bit IIDs MUST specify how all 64
bits are set, but a generic semantic meaning for the "u" and "g" bits
MUST NOT be defined. However, the method of generating IIDs for
specific link types MAY define some local significance for certain
bits.
In all cases, the bits in an IID have no generic semantics; in other
words, they have opaque values. In fact, the whole IID value MUST be
viewed as an opaque bit string by third parties, except possibly in
the local context.
======
That said - we already have a way to form all-random IIDs:
IN6_ADDR_GEN_MODE_RANDOM. Can't you just ensure that links of type
ARPHRD_RAWIP always use IN6_ADDR_GEN_MODE_RANDOM? That might lead to
less special-casing.
^ permalink raw reply
* [PATCH v2 net] net: fddi: fix a possible null-ptr-deref
From: YueHaibing @ 2018-06-08 2:58 UTC (permalink / raw)
To: davem; +Cc: netdev, linux-kernel, YueHaibing
bp->SharedMemAddr is set to NULL while bp->SharedMemSize lesser-or-equal 0,
then memset will trigger null-ptr-deref.
fix it by replacing pci_alloc_consistent with dma_zalloc_coherent.
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
v1->v2: move from pci_dma* to dma_* as Christoph suggested
---
drivers/net/fddi/skfp/skfddi.c | 55 +++++++++++++++++++++---------------------
1 file changed, 28 insertions(+), 27 deletions(-)
diff --git a/drivers/net/fddi/skfp/skfddi.c b/drivers/net/fddi/skfp/skfddi.c
index 2414f1d..72433f3e 100644
--- a/drivers/net/fddi/skfp/skfddi.c
+++ b/drivers/net/fddi/skfp/skfddi.c
@@ -297,11 +297,11 @@ static int skfp_init_one(struct pci_dev *pdev,
return 0;
err_out5:
if (smc->os.SharedMemAddr)
- pci_free_consistent(pdev, smc->os.SharedMemSize,
- smc->os.SharedMemAddr,
- smc->os.SharedMemDMA);
- pci_free_consistent(pdev, MAX_FRAME_SIZE,
- smc->os.LocalRxBuffer, smc->os.LocalRxBufferDMA);
+ dma_free_coherent(&pdev->dev, smc->os.SharedMemSize,
+ smc->os.SharedMemAddr,
+ smc->os.SharedMemDMA);
+ dma_free_coherent(&pdev->dev, MAX_FRAME_SIZE,
+ smc->os.LocalRxBuffer, smc->os.LocalRxBufferDMA);
err_out4:
free_netdev(dev);
err_out3:
@@ -328,17 +328,17 @@ static void skfp_remove_one(struct pci_dev *pdev)
unregister_netdev(p);
if (lp->os.SharedMemAddr) {
- pci_free_consistent(&lp->os.pdev,
- lp->os.SharedMemSize,
- lp->os.SharedMemAddr,
- lp->os.SharedMemDMA);
+ dma_free_coherent(&pdev->dev,
+ lp->os.SharedMemSize,
+ lp->os.SharedMemAddr,
+ lp->os.SharedMemDMA);
lp->os.SharedMemAddr = NULL;
}
if (lp->os.LocalRxBuffer) {
- pci_free_consistent(&lp->os.pdev,
- MAX_FRAME_SIZE,
- lp->os.LocalRxBuffer,
- lp->os.LocalRxBufferDMA);
+ dma_free_coherent(&pdev->dev,
+ MAX_FRAME_SIZE,
+ lp->os.LocalRxBuffer,
+ lp->os.LocalRxBufferDMA);
lp->os.LocalRxBuffer = NULL;
}
#ifdef MEM_MAPPED_IO
@@ -394,7 +394,9 @@ static int skfp_driver_init(struct net_device *dev)
spin_lock_init(&bp->DriverLock);
// Allocate invalid frame
- bp->LocalRxBuffer = pci_alloc_consistent(&bp->pdev, MAX_FRAME_SIZE, &bp->LocalRxBufferDMA);
+ bp->LocalRxBuffer = dma_alloc_coherent(&bp->pdev.dev, MAX_FRAME_SIZE,
+ &bp->LocalRxBufferDMA,
+ GFP_ATOMIC);
if (!bp->LocalRxBuffer) {
printk("could not allocate mem for ");
printk("LocalRxBuffer: %d byte\n", MAX_FRAME_SIZE);
@@ -407,23 +409,22 @@ static int skfp_driver_init(struct net_device *dev)
if (bp->SharedMemSize > 0) {
bp->SharedMemSize += 16; // for descriptor alignment
- bp->SharedMemAddr = pci_alloc_consistent(&bp->pdev,
- bp->SharedMemSize,
- &bp->SharedMemDMA);
+ bp->SharedMemAddr = dma_zalloc_coherent(&bp->pdev.dev,
+ bp->SharedMemSize,
+ &bp->SharedMemDMA,
+ GFP_ATOMIC);
if (!bp->SharedMemAddr) {
printk("could not allocate mem for ");
printk("hardware module: %ld byte\n",
bp->SharedMemSize);
goto fail;
}
- bp->SharedMemHeap = 0; // Nothing used yet.
} else {
bp->SharedMemAddr = NULL;
- bp->SharedMemHeap = 0;
- } // SharedMemSize > 0
+ }
- memset(bp->SharedMemAddr, 0, bp->SharedMemSize);
+ bp->SharedMemHeap = 0;
card_stop(smc); // Reset adapter.
@@ -442,15 +443,15 @@ static int skfp_driver_init(struct net_device *dev)
fail:
if (bp->SharedMemAddr) {
- pci_free_consistent(&bp->pdev,
- bp->SharedMemSize,
- bp->SharedMemAddr,
- bp->SharedMemDMA);
+ dma_free_coherent(&bp->pdev.dev,
+ bp->SharedMemSize,
+ bp->SharedMemAddr,
+ bp->SharedMemDMA);
bp->SharedMemAddr = NULL;
}
if (bp->LocalRxBuffer) {
- pci_free_consistent(&bp->pdev, MAX_FRAME_SIZE,
- bp->LocalRxBuffer, bp->LocalRxBufferDMA);
+ dma_free_coherent(&bp->pdev.dev, MAX_FRAME_SIZE,
+ bp->LocalRxBuffer, bp->LocalRxBufferDMA);
bp->LocalRxBuffer = NULL;
}
return err;
--
2.7.0
^ permalink raw reply related
* Re: [PATCH net] net/sched: act_simple: fix parsing of TCA_DEFDATA
From: Davide Caratti @ 2018-06-08 3:00 UTC (permalink / raw)
To: Hangbin Liu; +Cc: netdev, jhs, xiyou.wangcong, jiri, davem
In-Reply-To: <20180608020754.GT8958@leo.usersys.redhat.com>
On Fri, 2018-06-08 at 10:07 +0800, Hangbin Liu wrote:
> On Thu, Jun 07, 2018 at 03:46:43PM +0200, Davide Caratti wrote:
> > use nla_strlcpy() to avoid copying data beyond the length of TCA_DEFDATA
>
> s/TCA_DEFDATA/TCA_DEF_DATA/, incase someone search the commit history but
> could not find it.
>
> Thanks
> Hangbin
sure, thanks, and after another look I think also the 'Fixes:' tag is
wrong. More probably it was introduced with
fa1b1cff3d06 "net_cls_act: Make act_simple use of netlink policy."
I will send a v2 in minutes.
regards,
--
davide
^ permalink raw reply
* [PATCH net v2] net/sched: act_simple: fix parsing of TCA_DEF_DATA
From: Davide Caratti @ 2018-06-08 3:02 UTC (permalink / raw)
To: Jamal Hadi Salim, Cong Wang, Jiri Pirko; +Cc: David S. Miller, netdev
use nla_strlcpy() to avoid copying data beyond the length of TCA_DEF_DATA
netlink attribute, in case it is less than SIMP_MAX_DATA and it does not
end with '\0' character.
v2: fix errors in the commit message, thanks Hangbin Liu
Fixes: fa1b1cff3d06 ("net_cls_act: Make act_simple use of netlink policy.")
Signed-off-by: Davide Caratti <dcaratti@redhat.com>
---
net/sched/act_simple.c | 15 ++++++---------
1 file changed, 6 insertions(+), 9 deletions(-)
diff --git a/net/sched/act_simple.c b/net/sched/act_simple.c
index 9618b4a83cee..98c4afe7c15b 100644
--- a/net/sched/act_simple.c
+++ b/net/sched/act_simple.c
@@ -53,22 +53,22 @@ static void tcf_simp_release(struct tc_action *a)
kfree(d->tcfd_defdata);
}
-static int alloc_defdata(struct tcf_defact *d, char *defdata)
+static int alloc_defdata(struct tcf_defact *d, const struct nlattr *defdata)
{
d->tcfd_defdata = kzalloc(SIMP_MAX_DATA, GFP_KERNEL);
if (unlikely(!d->tcfd_defdata))
return -ENOMEM;
- strlcpy(d->tcfd_defdata, defdata, SIMP_MAX_DATA);
+ nla_strlcpy(d->tcfd_defdata, defdata, SIMP_MAX_DATA);
return 0;
}
-static void reset_policy(struct tcf_defact *d, char *defdata,
+static void reset_policy(struct tcf_defact *d, const struct nlattr *defdata,
struct tc_defact *p)
{
spin_lock_bh(&d->tcf_lock);
d->tcf_action = p->action;
memset(d->tcfd_defdata, 0, SIMP_MAX_DATA);
- strlcpy(d->tcfd_defdata, defdata, SIMP_MAX_DATA);
+ nla_strlcpy(d->tcfd_defdata, defdata, SIMP_MAX_DATA);
spin_unlock_bh(&d->tcf_lock);
}
@@ -87,7 +87,6 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
struct tcf_defact *d;
bool exists = false;
int ret = 0, err;
- char *defdata;
if (nla == NULL)
return -EINVAL;
@@ -110,8 +109,6 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
return -EINVAL;
}
- defdata = nla_data(tb[TCA_DEF_DATA]);
-
if (!exists) {
ret = tcf_idr_create(tn, parm->index, est, a,
&act_simp_ops, bind, false);
@@ -119,7 +116,7 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
return ret;
d = to_defact(*a);
- ret = alloc_defdata(d, defdata);
+ ret = alloc_defdata(d, tb[TCA_DEF_DATA]);
if (ret < 0) {
tcf_idr_release(*a, bind);
return ret;
@@ -133,7 +130,7 @@ static int tcf_simp_init(struct net *net, struct nlattr *nla,
if (!ovr)
return -EEXIST;
- reset_policy(d, defdata, parm);
+ reset_policy(d, tb[TCA_DEF_DATA], parm);
}
if (ret == ACT_P_CREATED)
--
2.17.0
^ permalink raw reply related
* Re: linux-next: manual merge of the scsi tree with the net-next tree
From: Stephen Rothwell @ 2018-06-08 3:11 UTC (permalink / raw)
To: James Bottomley, Martin K. Petersen, linux-scsi
Cc: Mark Brown, Chad Dupuis, David S. Miller, netdev,
Linux-Next Mailing List, Linux Kernel Mailing List
In-Reply-To: <20180525013810.GG4828@sirena.org.uk>
[-- Attachment #1: Type: text/plain, Size: 1690 bytes --]
Hi all,
On Fri, 25 May 2018 02:38:10 +0100 Mark Brown <broonie@kernel.org> wrote:
>
> Today's linux-next merge of the scsi tree got a conflict in:
>
> drivers/scsi/qedf/qedf.h
>
> between commit:
>
> 8673daf4f55bf3b91 ("qedf: Add get_generic_tlv_data handler.")
>
> from the net-next tree and commit:
>
> 4b9b7fabb39b3e9d7 ("scsi: qedf: Improve firmware debug dump handling")
>
> from the scsi tree.
>
> I fixed it up (see below) and can carry the fix as necessary. This
> is now fixed as far as linux-next is concerned, but any non trivial
> conflicts should be mentioned to your upstream maintainer when your tree
> is submitted for merging. You may also want to consider cooperating
> with the maintainer of the conflicting tree to minimise any particularly
> complex conflicts.
>
> diff --cc drivers/scsi/qedf/qedf.h
> index cabb6af60fb8,2372a40326f8..000000000000
> --- a/drivers/scsi/qedf/qedf.h
> +++ b/drivers/scsi/qedf/qedf.h
> @@@ -501,9 -499,8 +504,10 @@@ extern int qedf_post_io_req(struct qedf
> extern void qedf_process_seq_cleanup_compl(struct qedf_ctx *qedf,
> struct fcoe_cqe *cqe, struct qedf_ioreq *io_req);
> extern int qedf_send_flogi(struct qedf_ctx *qedf);
> +extern void qedf_get_protocol_tlv_data(void *dev, void *data);
> extern void qedf_fp_io_handler(struct work_struct *work);
> +extern void qedf_get_generic_tlv_data(void *dev, struct qed_generic_tlvs *data);
> + extern void qedf_wq_grcdump(struct work_struct *work);
>
> #define FCOE_WORD_TO_BYTE 4
> #define QEDF_MAX_TASK_NUM 0xFFFF
This is now a conflict between the scsi tree and Linus' tree.
--
Cheers,
Stephen Rothwell
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH net] vhost_net: remove VHOST_NET_F_VIRTIO_NET_HDR support
From: Jason Wang @ 2018-06-08 3:50 UTC (permalink / raw)
To: mst, jasowang; +Cc: kvm, virtualization, netdev, linux-kernel
This feature bit is duplicated with VIRTIO_F_ANY_LAYOUT, this means if
a userpsace want to enable VRITIO_F_ANY_LAYOUT,
VHOST_NET_F_VIRTIO_NET_HDR will be implied too. This is wrong and will
break networking. Fixing this by safely removing
VHOST_NET_F_VIRTIO_NET_HDR support. There should be very few or even
no userspace can use this. Further cleanups could be done for
-net-next for safety.
In the future, we need a vhost dedicated feature set/get ioctl()
instead of reusing virtio ones.
Fixes: 4e9fa50c6ccbe ("vhost: move features to core")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 15 +++++----------
1 file changed, 5 insertions(+), 10 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 986058a..83eef52 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -69,7 +69,6 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;"
enum {
VHOST_NET_FEATURES = VHOST_FEATURES |
- (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
(1ULL << VIRTIO_NET_F_MRG_RXBUF) |
(1ULL << VIRTIO_F_IOMMU_PLATFORM)
};
@@ -1255,15 +1254,11 @@ static int vhost_net_set_features(struct vhost_net *n, u64 features)
(1ULL << VIRTIO_F_VERSION_1))) ?
sizeof(struct virtio_net_hdr_mrg_rxbuf) :
sizeof(struct virtio_net_hdr);
- if (features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)) {
- /* vhost provides vnet_hdr */
- vhost_hlen = hdr_len;
- sock_hlen = 0;
- } else {
- /* socket provides vnet_hdr */
- vhost_hlen = 0;
- sock_hlen = hdr_len;
- }
+
+ /* socket provides vnet_hdr */
+ vhost_hlen = 0;
+ sock_hlen = hdr_len;
+
mutex_lock(&n->dev.mutex);
if ((features & (1 << VHOST_F_LOG_ALL)) &&
!vhost_log_access_ok(&n->dev))
--
2.7.4
^ permalink raw reply related
* Re: [v3, 00/10] Support DPAA PTP clock and timestamping
From: Richard Cochran @ 2018-06-08 4:27 UTC (permalink / raw)
To: Yangbo Lu
Cc: netdev, madalin.bucur, Rob Herring, Shawn Guo, David S . Miller,
devicetree, linuxppc-dev, linux-arm-kernel, linux-kernel
In-Reply-To: <20180607092050.46128-1-yangbo.lu@nxp.com>
On Thu, Jun 07, 2018 at 05:20:40PM +0800, Yangbo Lu wrote:
> This patchset is to support DPAA FMAN PTP clock and HW timestamping.
> It had been verified on both ARM platform and PPC platform.
> - The patch #1 to patch #5 are to support DPAA FMAN 1588 timer in
> ptp_qoriq driver.
> - The patch #6 to patch #10 are to add HW timestamping support in
> DPAA ethernet driver.
Right now, net-next is closed for new stuff. You will have to post
the series again after the merge window closes. You can check the
status here:
http://vger.kernel.org/~davem/net-next.html
When you do re-post, you can add my:
Acked-by: Richard Cochran <richardcochran@gmail.com>
^ permalink raw reply
* RE: [v3, 00/10] Support DPAA PTP clock and timestamping
From: Y.b. Lu @ 2018-06-08 4:45 UTC (permalink / raw)
To: Richard Cochran
Cc: netdev@vger.kernel.org, Madalin-cristian Bucur, Rob Herring,
Shawn Guo, David S . Miller, devicetree@vger.kernel.org,
linuxppc-dev@lists.ozlabs.org,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20180608042706.7gfg5p6574ntc2lq@localhost>
> -----Original Message-----
> From: Richard Cochran [mailto:richardcochran@gmail.com]
> Sent: Friday, June 8, 2018 12:27 PM
> To: Y.b. Lu <yangbo.lu@nxp.com>
> Cc: netdev@vger.kernel.org; Madalin-cristian Bucur
> <madalin.bucur@nxp.com>; Rob Herring <robh+dt@kernel.org>; Shawn Guo
> <shawnguo@kernel.org>; David S . Miller <davem@davemloft.net>;
> devicetree@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org
> Subject: Re: [v3, 00/10] Support DPAA PTP clock and timestamping
>
> On Thu, Jun 07, 2018 at 05:20:40PM +0800, Yangbo Lu wrote:
> > This patchset is to support DPAA FMAN PTP clock and HW timestamping.
> > It had been verified on both ARM platform and PPC platform.
> > - The patch #1 to patch #5 are to support DPAA FMAN 1588 timer in
> > ptp_qoriq driver.
> > - The patch #6 to patch #10 are to add HW timestamping support in
> > DPAA ethernet driver.
>
> Right now, net-next is closed for new stuff. You will have to post the series
> again after the merge window closes. You can check the status here:
>
>
> https://emea01.safelinks.protection.outlook.com/?url=http:%2F%2Fvger.kern
> el.org%2F~davem%2Fnet-next.html&data=02%7C01%7Cyangbo.lu%40nxp.co
> m%7Cbaab0b22e7444386c37008d5ccf81b37%7C686ea1d3bc2b4c6fa92cd99
> c5c301635%7C0%7C0%7C636640288347563742&sdata=jCmNlwoeWA50PV4
> w3lKZ%2Fs4akPjw0VV2OrJ3t4FizJ0%3D&reserved=0
>
> When you do re-post, you can add my:
>
> Acked-by: Richard Cochran <richardcochran@gmail.com>
[Y.b. Lu] Get it. And thanks a lot 😊
^ permalink raw reply
* Re: [PATCH net] vhost_net: remove VHOST_NET_F_VIRTIO_NET_HDR support
From: Michael S. Tsirkin @ 2018-06-08 4:46 UTC (permalink / raw)
To: Jason Wang; +Cc: kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1528429842-22835-1-git-send-email-jasowang@redhat.com>
On Fri, Jun 08, 2018 at 11:50:42AM +0800, Jason Wang wrote:
> This feature bit is duplicated with VIRTIO_F_ANY_LAYOUT, this means if
> a userpsace want to enable VRITIO_F_ANY_LAYOUT,
> VHOST_NET_F_VIRTIO_NET_HDR will be implied too. This is wrong and will
> break networking.
What breaks networking exactly? VHOST_NET supported ANY_LAYOUT
from day one. For this reason it does not need to know about
VRITIO_F_ANY_LAYOUT and we reused the bit for other purposes.
> Fixing this by safely removing
> VHOST_NET_F_VIRTIO_NET_HDR support. There should be very few or even
> no userspace can use this.
Quite possibly, but it is hard to be sure. It seems safer to
maintain it unless there's an actual reason something's broken.
> Further cleanups could be done for
> -net-next for safety.
>
> In the future, we need a vhost dedicated feature set/get ioctl()
> instead of reusing virtio ones.
Not just in the future, we might want to switch iommu
to a sane structure without the 64 bit padding bug
right now.
>
> Fixes: 4e9fa50c6ccbe ("vhost: move features to core")
This tag makes no sense here IMHO. Looks like people are using some tool
that just looks at the earliest version where patch won't apply. The
commit in question just moved some code around.
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/vhost/net.c | 15 +++++----------
> 1 file changed, 5 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 986058a..83eef52 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -69,7 +69,6 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;"
>
> enum {
> VHOST_NET_FEATURES = VHOST_FEATURES |
> - (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
> (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
> (1ULL << VIRTIO_F_IOMMU_PLATFORM)
> };
> @@ -1255,15 +1254,11 @@ static int vhost_net_set_features(struct vhost_net *n, u64 features)
> (1ULL << VIRTIO_F_VERSION_1))) ?
> sizeof(struct virtio_net_hdr_mrg_rxbuf) :
> sizeof(struct virtio_net_hdr);
> - if (features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)) {
> - /* vhost provides vnet_hdr */
> - vhost_hlen = hdr_len;
> - sock_hlen = 0;
> - } else {
> - /* socket provides vnet_hdr */
> - vhost_hlen = 0;
> - sock_hlen = hdr_len;
> - }
> +
> + /* socket provides vnet_hdr */
> + vhost_hlen = 0;
> + sock_hlen = hdr_len;
> +
> mutex_lock(&n->dev.mutex);
> if ((features & (1 << VHOST_F_LOG_ALL)) &&
> !vhost_log_access_ok(&n->dev))
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH net] vhost_net: remove VHOST_NET_F_VIRTIO_NET_HDR support
From: Jason Wang @ 2018-06-08 5:07 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: kvm, virtualization, netdev, linux-kernel
In-Reply-To: <20180608074115-mutt-send-email-mst@kernel.org>
On 2018年06月08日 12:46, Michael S. Tsirkin wrote:
> On Fri, Jun 08, 2018 at 11:50:42AM +0800, Jason Wang wrote:
>> This feature bit is duplicated with VIRTIO_F_ANY_LAYOUT, this means if
>> a userpsace want to enable VRITIO_F_ANY_LAYOUT,
>> VHOST_NET_F_VIRTIO_NET_HDR will be implied too. This is wrong and will
>> break networking.
> What breaks networking exactly? VHOST_NET supported ANY_LAYOUT
> from day one. For this reason it does not need to know about
> VRITIO_F_ANY_LAYOUT and we reused the bit for other purposes.
It's the knowledge of vhost_net code it self but not userspace. For
userspace, it should depends on the value of returned by
VHOST_GET_FEATURES. So when userspace can set_features with ANY_LAYOUT,
vhost may think it wants VHOST_NET_F_VIRTIO_NET_HDR.
>
>
>
>> Fixing this by safely removing
>> VHOST_NET_F_VIRTIO_NET_HDR support. There should be very few or even
>> no userspace can use this.
> Quite possibly, but it is hard to be sure. It seems safer to
> maintain it unless there's an actual reason something's broken.
I think not since the feature is negotiated not mandatory?
>
>> Further cleanups could be done for
>> -net-next for safety.
>>
>> In the future, we need a vhost dedicated feature set/get ioctl()
>> instead of reusing virtio ones.
> Not just in the future, we might want to switch iommu
> to a sane structure without the 64 bit padding bug
> right now.
Yes, I hit this bug when introducing V2 of msg IOTLB message.
>
>> Fixes: 4e9fa50c6ccbe ("vhost: move features to core")
> This tag makes no sense here IMHO. Looks like people are using some tool
> that just looks at the earliest version where patch won't apply. The
> commit in question just moved some code around.
Looks not, before this commit, vhost_net won't return ANY_LAYOUT.
Thanks
>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> drivers/vhost/net.c | 15 +++++----------
>> 1 file changed, 5 insertions(+), 10 deletions(-)
>>
>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>> index 986058a..83eef52 100644
>> --- a/drivers/vhost/net.c
>> +++ b/drivers/vhost/net.c
>> @@ -69,7 +69,6 @@ MODULE_PARM_DESC(experimental_zcopytx, "Enable Zero Copy TX;"
>>
>> enum {
>> VHOST_NET_FEATURES = VHOST_FEATURES |
>> - (1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
>> (1ULL << VIRTIO_NET_F_MRG_RXBUF) |
>> (1ULL << VIRTIO_F_IOMMU_PLATFORM)
>> };
>> @@ -1255,15 +1254,11 @@ static int vhost_net_set_features(struct vhost_net *n, u64 features)
>> (1ULL << VIRTIO_F_VERSION_1))) ?
>> sizeof(struct virtio_net_hdr_mrg_rxbuf) :
>> sizeof(struct virtio_net_hdr);
>> - if (features & (1 << VHOST_NET_F_VIRTIO_NET_HDR)) {
>> - /* vhost provides vnet_hdr */
>> - vhost_hlen = hdr_len;
>> - sock_hlen = 0;
>> - } else {
>> - /* socket provides vnet_hdr */
>> - vhost_hlen = 0;
>> - sock_hlen = hdr_len;
>> - }
>> +
>> + /* socket provides vnet_hdr */
>> + vhost_hlen = 0;
>> + sock_hlen = hdr_len;
>> +
>> mutex_lock(&n->dev.mutex);
>> if ((features & (1 << VHOST_F_LOG_ALL)) &&
>> !vhost_log_access_ok(&n->dev))
>> --
>> 2.7.4
^ permalink raw reply
* Re: [PATCH] selftests: bpf: fix urandom_read build issue
From: Y Song @ 2018-06-08 5:08 UTC (permalink / raw)
To: Anders Roxell
Cc: Alexei Starovoitov, Daniel Borkmann, Shuah Khan, netdev, LKML,
linux-kselftest
In-Reply-To: <CADYN=9Ly5UU34CxZFnWfQhNrS8V3gG7=Bdfn9n0h5XphZ8EiVQ@mail.gmail.com>
On Thu, Jun 7, 2018 at 2:43 PM, Anders Roxell <anders.roxell@linaro.org> wrote:
> On 7 June 2018 at 23:17, Y Song <ys114321@gmail.com> wrote:
>> On Thu, Jun 7, 2018 at 12:07 PM, Anders Roxell <anders.roxell@linaro.org> wrote:
>>> On 7 June 2018 at 19:52, Y Song <ys114321@gmail.com> wrote:
>>>> On Thu, Jun 7, 2018 at 3:57 AM, Anders Roxell <anders.roxell@linaro.org> wrote:
>>>>> gcc complains that urandom_read gets built twice.
>>>>>
>>>>> gcc -o tools/testing/selftests/bpf/urandom_read
>>>>> -static urandom_read.c -Wl,--build-id
>>>>> gcc -Wall -O2 -I../../../include/uapi -I../../../lib -I../../../lib/bpf
>>>>> -I../../../../include/generated -I../../../include urandom_read.c
>>>>> urandom_read -lcap -lelf -lrt -lpthread -o
>>>>> tools/testing/selftests/bpf/urandom_read
>>>>> gcc: fatal error: input file
>>>>> ‘tools/testing/selftests/bpf/urandom_read’ is the
>>>>> same as output file
>>>>> compilation terminated.
>>>>> ../lib.mk:110: recipe for target
>>>>> 'tools/testing/selftests/bpf/urandom_read' failed
>>>>
>>>> What is the build/make command to reproduce the above failure?
>>>
>>> make -C tools/testing/selftests
>>
>> Thanks. The patch will break
>> make -C tools/testing/selftests/bpf
>>
>> [yhs@localhost bpf-next]$ make -C tools/testing/selftests/bpf
>> make: Entering directory '/home/yhs/work/bpf-next/tools/testing/selftests/bpf'
>> gcc -o /urandom_read -static urandom_read.c -Wl,--build-id
>> /usr/bin/ld: cannot open output file /urandom_read: Permission denied
>> collect2: error: ld returned 1 exit status
>> make: *** [Makefile:20: /urandom_read] Error 1
>> make: Leaving directory '/home/yhs/work/bpf-next/tools/testing/selftests/bpf'
>> [yhs@localhost bpf-next]$
>
> urgh, I'm sorry, missed that.
>
>>
>> Could you still make the above command work?
>
> $(TEST_CUSTOM_PROGS): $(OUTPUT)/%: %.c
> $(CC) -o $(TEST_CUSTOM_PROGS) -static $< -Wl,--build-id
>
> That worked both with:
> make -C tools/testing/selftests
> and
> make -C tools/testing/selftests/bpf
>
> for me.
>
> what do you think?
This indeed works. You can submit a revised patch and add my Ack.
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* Re: [PATCH 2/5] spi: implement companion-spi driver
From: Oleksij Rempel @ 2018-06-08 6:03 UTC (permalink / raw)
To: Jonas Mark (BT-FIR/ENG1)
Cc: Andy Shevchenko, Wolfgang Grandegger, Marc Kleine-Budde,
linux-can@vger.kernel.org, netdev, Linux Kernel Mailing List,
Heiko Schocher, ZHU Yi (BT-FIR/ENG1-Zhu)
In-Reply-To: <be0d6a5f98e840d29f673d976416a4b2@de.bosch.com>
[-- Attachment #1: Type: text/plain, Size: 7658 bytes --]
Hi Mark,
On Thu, Jun 07, 2018 at 02:58:24PM +0000, Jonas Mark (BT-FIR/ENG1) wrote:
> Hello Andy,
>
> Thank you very much for your feedback.
>
> > > + /*TODO: support mutiple packets in one write in future*/
> >
> > Hmm... Either fix this, or remove comment?
>
> Agreed, we will manage ideas for future changes at a different place.
>
> > > + if (copy_from_user(p.data, buf, sizeof(p)) == 0) {
> > > + if (is_can_type(&p))
> > > + return -EINVAL;
> > > + } else {
> > > + dev_info(parent, "copy from user not succeed in one call\n");
> >
> > Shouldn't it return immediately?
>
> Yes, it should. Will be changed.
>
> >
> > > + }
> >
> > > +
> > > + error = qm_io_txq_in(&priv->pm.qm, buf, count, &copied);
> >
> > ...what the point to call if we got garbage from user space?
>
> Will be changed with the code above.
>
> > > + if (!error) {
> >
> > The usual pattern is to check for errors first.
>
> Understood, will be changed.
>
> > > + wake_up_interruptible(&priv->wait);
> > > + priv->pm.stats.io_tx++;
> > > + return copied;
> > > + } else {
> > > + priv->pm.stats.io_tx_overflows++;
> > > + }
> > > + return error;
> > > +}
> >
> > > + ... qm_io_rxq_out(&priv->pm.qm, buf, count, &copied);
> >
> > > + ... pm_can_data_tx(&priv->pm, port, prio, cf);
> >
> > Oh, oh, oh.
> >
> > These namespaces are too generic, moreover pm is kinda occupied by
> > power management. You bring a lot of confusion here, I think.
>
> We agree and we started thinking about better names. We will send a proposal.
>
> > > + err = pm_can_get_txq_status(pm, port);
> > > + if (!err) {
> >
> > if (err)
> > return err;
>
> Yes, will be changed.
>
> > > + }
> > > + return err;
> >
> >
> > > + int ret, value;
> > > +
> > > + ret = sscanf(buf, "%d", &value);
> > > + if (ret != 1) {
> >
> > > + }
> >
> > You have to be consistent in your code.
> >
> > I've already noticed
> >
> > err
> > error
> >
> > and now
> >
> > ret
> >
> > Choose one and stick with it.
>
> Yes, will be changed.
>
> > Also check your entire patch series' code for consistency.
>
> We will have a look.
>
> > > +static DEVICE_ATTR(dump_packets, S_IRUGO | S_IWUSR,
> > > + show_dump_packets, store_dump_packets);
> >
> > We have helpers, like DEVICE_ATTR_RW().
>
> Will be changed.
>
> > > + ret = snprintf(buf + pos, PAGE_SIZE - pos,
> > > + "\ntx: %u, rx: %u, err: %u\n\n",
> > > + total,
> > > + priv->pm.stats.can_rx_overflows[i],
> > > + priv->pm.stats.can_err_overflows[i]);
> >
> > Please, avoid leading '\n'.
>
> We think we will stick to the existing. Otherweise we would have to
> insert another pair of sprint() and pos += ret. Is it worth that?
>
> >
> > > + gpio_set_value(priv->cs_gpios, priv->cs_gpios_assert);
> >
> > > + gpio_set_value(priv->cs_gpios, !priv->cs_gpios_assert);
> >
> > Can you switch to GPIO descriptor API?
>
> Yes, we are working on it.
>
> > > + unsigned int count = READY_POLL_US / READY_POLL_US_GRAN;
> > > + while (count--) {
> >
> > For counting like this better to have
> > do {
> > } while (--count);
> >
> > The rationale, reader at first glance will know that the loop will
> > iterate at least once.
>
> Agreed, will be changed.
>
> > > + if (slave_is_not_busy(priv))
> > > + return 0;
> > > +
> >
> > > + udelay(READY_POLL_US_GRAN);
> >
> > Should it be atomic?
> > Can it use read_poll_* type of helpers instead?
>
> Yes, it shall be atomic because we need to reduce the latency at
> detecting the de-assertion of the busy signal. We accept that this will
> cost CPU time.
>
> In case the Companion itself is very busy and does not reply quickly we
> are have second polling loop below which sleeps longer and is non-atomic.
I can confirm, this make huge impact on protocol performance. And this
protocol is not really the speed runner.
> > Above comments applicable to entire code you have.
>
> We will look at it.
>
> > > +static void companion_spi_cpu_to_be32(char *buf)
> > > +{
> > > + u32 *buf32 = (u32*)buf;
> > > + int i;
> > > +
> > > + for (i = 0; i < (BCP_PACKET_SIZE / sizeof(u32)); i++, buf32++)
> > > + *buf32 = cpu_to_be32(*buf32);
> > > +}
> >
> > This entire function should be replaced by one of the helpers from
> > include/linux/byteorder/generic.h
> >
> > I guess cpu_to_be32_array() is a right one.
>
> Correct. We are testing the driver against 4.14 and that function is not
> available there. It will be changed later.
>
> > > +static void companion_spi_be32_to_cpu(char *buf)
> > > +{
> > > + u32 *buf32 = (u32*)buf;
> > > + int i;
> > > +
> > > + for (i = 0; i < (BCP_PACKET_SIZE / sizeof(u32)); i++, buf32++)
> > > + *buf32 = be32_to_cpu(*buf32);
> > > +}
> >
> > Ditto.
> >
> > Recommendation: check your code against existing helpers.
>
> Yes, every kernel release brings new helpers. We will have to learn how
> to keep track of the additions.
>
> > > + p = (const struct companion_packet*)transfer->tx_buf;
> >
> > > + companion_spi_cpu_to_be32((char*)transfer->tx_buf);
> >
> > If tx_buf is type of void * all these castings are redundant.
>
> The type is const void*. So the first cast is superfluous, the second
> is not.
>
> > Also looking at the function, did you consider to use whatever SPI
> > core provides, like CS handling, messages handling and so on?
>
> SPI CS has to be done manually in our case because we need to wait
> until the Companion signals that it is ready for the transfer.
>
> Do you have concrete proposals regarding messages handling?
you can send dummy message to set CS.
+ struct spi_transfer t = {
+ .len = 0,
+ .cs_change = 1,
+ };
+ /* send dummy to trigger CS */
+ reinit_completion(&priv->fc_complete);
+ spi_sync_locked(spi, &m);
see my ancient unfinished code:
https://patchwork.kernel.org/patch/9418511/
In general, this part of the code, can be provided as separate driver
which should be called as "SPI 5wire protocol". 3 wires for data, 1 -
chip select, 1 - busy state. Since, the slave cant fit to normal SPI
requirements, and can't be ready within predictable time, busy signal is
needed. Providing this as separate driver or part of SPI framework
should reduce the code for many different drivers.
The actual datagram on top of your spi companion should go to
separate driver. There are similar (almost identical designs)
can :+
char:+
dw: +
gpio:+--->some_multi_end_mux_protocol--->spi_5wire_protocol->spi--->
In my knowledge, only data format on top of spi_5wire_protocol is
different. See my notes for similar topics:
https://github.com/olerem/icc
https://github.com/olerem/spi-5wire
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* Re: [PATCH net-next v2 0/2] net: phy: improve PHY suspend/resume
From: Heiner Kallweit @ 2018-06-08 6:09 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Florian Fainelli, David Miller, netdev@vger.kernel.org
In-Reply-To: <ae4c30a6-7724-a02c-189a-1ff8768528ee@gmail.com>
On 05.06.2018 21:39, Heiner Kallweit wrote:
> On 02.06.2018 22:27, Heiner Kallweit wrote:
>> On 01.06.2018 02:10, Andrew Lunn wrote:
>>>> Configuring the different WoL options isn't handled by writing to
>>>> the PHY registers but by writing to chip / MAC registers.
>>>> Therefore phy_suspend() isn't able to figure out whether WoL is
>>>> enabled or not. Only the parent has the full picture.
>>>
>>> Hi Heiner
>>>
>>> I think you need to look at your different runtime PM domains. If i
>>> understand the code right, you runtime suspend if there is no
>>> link. But for this to work correctly, your PHY needs to keep working.
>>> You also cannot assume all accesses to the PHY go via the MAC. Some
>>> calls will go direct to the PHY, and they can trigger MDIO bus
>>> accesses. So i think you need two runtime PM domains. MAC and MDIO
>>> bus. Maybe just the pll? An MDIO bus is a device, so it can have its
>>> on PM callbacks. It is not clear what you need to resume in order to
>>> make MDIO work.
>>>
>> Thanks for your comments!
>> The actual problem is quite small: I get an error at MDIO suspend,
>> the PHY however is suspended later by the driver's suspend callback
>> anyway. Because the problem is small I'm somewhat reluctant to
>> consider bigger changes like introducing different PM domains.
>>
>> Primary reason for the error is that the network chip is in PCI D3hot
>> at that moment. In addition to that for some of the chips supported by
>> the driver also MDIO-relevant PLL's might be disabled.
>>
>> By the way:
>> When checking PM handling for PHY/MDIO I stumbled across something
>> that can be improved IMO, I'll send a patch for your review.
>>
> I experimented a little and with adding Runtime PM to MDIO bus and
> PHY device I can make it work:
> PHY runtime-resumes before entering suspend and resumes its parent
> (MDIO bus) which then resumes its parent (PCI device).
> However this needs quite some code and is hard to read / understand
> w/o reading through this mail thread.
>
> And in general I still have doubts this is the right way. Let's
> consider the following scenario:
>
> A network driver configures WoL in its suspend callback
> (incl. setting the device to wakeup-enabled).
> The suspend callback of the PHY is called before this and therefore
> has no clue that WoL will be configured a little later, with the
> consequence that it will do an unsolicited power-down.
> The network driver then has to detect this and power-up the PHY again.
> This doesn't seem to make much sense and still my best idea is to
> establish a mechanism that a device can state: I orchestrate PM
> of my children.
>
There's one more way to deal with the issue, an empty dev_pm_domain.
We could do:
struct dev_pm_domain empty = { .ops = NULL };
phydev->mdio.dev.pm_domain = empty;
This overrides the device_type pm ops, however I wouldn't necessarily
consider it an elegant solution. Do you have an opinion on that?
I also checked device links, however this doesn't seem to be the right
concept in our case.
> Heiner
>
>>> It might also help if you do the phy_connect in .ndo_open and
>>> disconnect in .ndo_stop. This is a common pattern in drivers. But some
>>> also do it is probe and remove.
>>>
>> Thanks for the hint. I will move phy_connect_direct accordingly.
>>
>>> Andrew
>>>
>> Heiner
>>
>
^ permalink raw reply
* Re: [PATCH ipsec] vti6: fix PMTU caching and reporting on xmit
From: Steffen Klassert @ 2018-06-08 6:19 UTC (permalink / raw)
To: Eyal Birger; +Cc: netdev, herbert, davem
In-Reply-To: <1528355462-17871-1-git-send-email-eyal.birger@gmail.com>
On Thu, Jun 07, 2018 at 10:11:02AM +0300, Eyal Birger wrote:
> When setting the skb->dst before doing the MTU check, the route PMTU
> caching and reporting is done on the new dst which is about to be
> released.
>
> Instead, PMTU handling should be done using the original dst.
>
> This is aligned with IPv4 VTI.
>
> Signed-off-by: Eyal Birger <eyal.birger@gmail.com>
> Fixes: ccd740cbc6 ("vti6: Add pmtu handling to vti6_xmit.")
Patch applied, thanks for catching this Eyal!
^ permalink raw reply
* Re: [PATCH] selftests: bpf: fix urandom_read build issue
From: Anders Roxell @ 2018-06-08 6:45 UTC (permalink / raw)
To: Y Song
Cc: Alexei Starovoitov, Daniel Borkmann, Shuah Khan, netdev, LKML,
linux-kselftest
In-Reply-To: <CAH3MdRWaVKrYbR14wjn6YjurcHvOFe-=t6g0Me=9=g+GRXqeeg@mail.gmail.com>
On 8 June 2018 at 07:08, Y Song <ys114321@gmail.com> wrote:
> On Thu, Jun 7, 2018 at 2:43 PM, Anders Roxell <anders.roxell@linaro.org> wrote:
>> On 7 June 2018 at 23:17, Y Song <ys114321@gmail.com> wrote:
>>> On Thu, Jun 7, 2018 at 12:07 PM, Anders Roxell <anders.roxell@linaro.org> wrote:
>>>> On 7 June 2018 at 19:52, Y Song <ys114321@gmail.com> wrote:
>>>>> On Thu, Jun 7, 2018 at 3:57 AM, Anders Roxell <anders.roxell@linaro.org> wrote:
>>>>>> gcc complains that urandom_read gets built twice.
>>>>>>
>>>>>> gcc -o tools/testing/selftests/bpf/urandom_read
>>>>>> -static urandom_read.c -Wl,--build-id
>>>>>> gcc -Wall -O2 -I../../../include/uapi -I../../../lib -I../../../lib/bpf
>>>>>> -I../../../../include/generated -I../../../include urandom_read.c
>>>>>> urandom_read -lcap -lelf -lrt -lpthread -o
>>>>>> tools/testing/selftests/bpf/urandom_read
>>>>>> gcc: fatal error: input file
>>>>>> ‘tools/testing/selftests/bpf/urandom_read’ is the
>>>>>> same as output file
>>>>>> compilation terminated.
>>>>>> ../lib.mk:110: recipe for target
>>>>>> 'tools/testing/selftests/bpf/urandom_read' failed
>>>>>
>>>>> What is the build/make command to reproduce the above failure?
>>>>
>>>> make -C tools/testing/selftests
>>>
>>> Thanks. The patch will break
>>> make -C tools/testing/selftests/bpf
>>>
>>> [yhs@localhost bpf-next]$ make -C tools/testing/selftests/bpf
>>> make: Entering directory '/home/yhs/work/bpf-next/tools/testing/selftests/bpf'
>>> gcc -o /urandom_read -static urandom_read.c -Wl,--build-id
>>> /usr/bin/ld: cannot open output file /urandom_read: Permission denied
>>> collect2: error: ld returned 1 exit status
>>> make: *** [Makefile:20: /urandom_read] Error 1
>>> make: Leaving directory '/home/yhs/work/bpf-next/tools/testing/selftests/bpf'
>>> [yhs@localhost bpf-next]$
>>
>> urgh, I'm sorry, missed that.
>>
>>>
>>> Could you still make the above command work?
>>
>> $(TEST_CUSTOM_PROGS): $(OUTPUT)/%: %.c
>> $(CC) -o $(TEST_CUSTOM_PROGS) -static $< -Wl,--build-id
>>
>> That worked both with:
>> make -C tools/testing/selftests
>> and
>> make -C tools/testing/selftests/bpf
>>
>> for me.
>>
>> what do you think?
>
> This indeed works. You can submit a revised patch and add my Ack.
> Acked-by: Yonghong Song <yhs@fb.com>
Thank you for your time reviewing this.
I will send that out shortly.
Cheers,
Anders
^ permalink raw reply
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