* Re: [PATCH v13 net-next 01/12] tls: support for Inline tls record
From: Atul Gupta @ 2018-03-28 4:28 UTC (permalink / raw)
To: Stefano Brivio
Cc: davem, herbert, davejwatson, sd, linux-crypto, netdev, werner,
leedom, swise, indranil, ganeshgr
In-Reply-To: <20180327202329.6d0d8896@epycfail>
On 3/27/2018 11:53 PM, Stefano Brivio wrote:
> On Tue, 27 Mar 2018 23:06:30 +0530
> Atul Gupta <atul.gupta@chelsio.com> wrote:
>
>> +static struct tls_context *create_ctx(struct sock *sk)
>> +{
>> + struct inet_connection_sock *icsk = inet_csk(sk);
>> + struct tls_context *ctx;
>> +
>> + /* allocate tls context */
>> + ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>> + if (!ctx)
>> + return NULL;
>> +
>> + icsk->icsk_ulp_data = ctx;
>> + return ctx;
>> +}
>>
>> [...]
>>
>> static int tls_init(struct sock *sk)
>> {
>> int ip_ver = sk->sk_family == AF_INET6 ? TLSV6 : TLSV4;
>> - struct inet_connection_sock *icsk = inet_csk(sk);
>> struct tls_context *ctx;
>> int rc = 0;
>>
>> + if (tls_hw_prot(sk))
>> + goto out;
>> +
>> /* The TLS ulp is currently supported only for TCP sockets
>> * in ESTABLISHED state.
>> * Supporting sockets in LISTEN state will require us
>> @@ -530,12 +624,11 @@ static int tls_init(struct sock *sk)
>> return -ENOTSUPP;
>>
>> /* allocate tls context */
>> - ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
>> + ctx = create_ctx(sk);
>> if (!ctx) {
>> rc = -ENOMEM;
>> goto out;
>> }
>> - icsk->icsk_ulp_data = ctx;
> Why are you changing this?
since create_ctx is called at two place it is assigned in allocating function than duplicate the assignment.
>
> This is now equivalent to the original implementation, except that you
> are "hiding" the assignment of icsk->icsk_ulp_data into a function named
> "create_ctx".
>
> Please also note that you are duplicating the "allocate tls context"
> comment.
will remove this comment.
>
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 4:33 UTC (permalink / raw)
To: Linus Torvalds
Cc: Alexander Duyck, Will Deacon, Sinan Kaya, Arnd Bergmann,
Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <CA+55aFyTeuXPmC5-1JGh2ow-rf5wMA6Vcv0aeS_q0_EnptSK9Q@mail.gmail.com>
On Tue, 2018-03-27 at 16:51 -1000, Linus Torvalds wrote:
> On Tue, Mar 27, 2018 at 3:03 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> >
> > The discussion at hand is about
> >
> > dma_buffer->foo = 1; /* WB */
> > writel(KICK, DMA_KICK_REGISTER); /* UC */
>
> Yes. That certainly is ordered on x86. In fact, afaik it's ordered
> even if that writel() might be of type WC, because that only delays
> writes, it doesn't move them earlier.
Ok so this is our answer ...
... snip ... (thanks for the background info !)
> Oh, the above UC case is absoutely guaranteed.
Good.
Then....
> The only issue really is that 99.9% of all testing gets done on x86
> unless you look at specific SoC drivers.
>
> On ARM, for example, there is likely little reason to care about x86
> memory ordering, because there is almost zero driver overlap between
> x86 and ARM.
>
> *Historically*, the reason for following the x86 IO ordering was
> simply that a lot of architectures used the drivers that were
> developed on x86. The alpha and powerpc workstations were *designed*
> with the x86 IO bus (PCI, then PCIe) and to work with the devices that
> came with it.
>
> ARM? PCIe is almost irrelevant. For ARM servers, if they ever take
> off, sure. But 99.99% of ARM is about their own SoC's, and so "x86
> test coverage" is simply not an issue.
>
> How much of an issue is it for Power? Maybe you decide it's not a big deal.
>
> Then all the above is almost irrelevant.
So the overlap may not be that NIL in practice :-) But even then that
doesn't matter as ARM has been happily implementing the same semantic
you describe above for years, as do we powerpc.
This is why, I want (with your agreement) to define clearly and once
and for all, that the Linux semantics of writel are that it is ordered
with previous writes to coherent memory (*)
This is already what ARM and powerpc provide, from what you say, what
x86 provides, I don't see any reason to keep that badly documented and
have drivers randomly growing useless wmb()'s because they don't think
it works on x86 without them !
Once that's sorted, let's tackle the problem of mmiowb vs. spin_unlock
and the problem of writel_relaxed semantics but as separate issues :-)
Also, can I assume the above ordering with writel() equally applies to
readl() or not ?
IE:
dma_buf->foo = 1;
readl(STUPID_DEVICE_DMA_KICK_ON_READ);
Also works on x86 ? (It does on power, maybe not on ARM).
Cheers,
Ben.
(*) From an Linux API perspective, all of this is only valid if the
memory was allocated by dma_alloc_coherent(). Anything obtained by
dma_map_something() might have been bounced bufferred or might require
extra cache flushes on some architectures, and thus needs
dma_sync_for_{cpu,device} calls.
Cheers,
Ben.
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 4:41 UTC (permalink / raw)
To: Sinan Kaya, Linus Torvalds
Cc: Alexander Duyck, Will Deacon, Arnd Bergmann, Jason Gunthorpe,
David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <bf77be28-051d-79c8-9b65-948c8a254d6d@codeaurora.org>
On Tue, 2018-03-27 at 23:24 -0400, Sinan Kaya wrote:
> On 3/27/2018 10:51 PM, Linus Torvalds wrote:
> > > The discussion at hand is about
> > >
> > > dma_buffer->foo = 1; /* WB */
> > > writel(KICK, DMA_KICK_REGISTER); /* UC */
> >
> > Yes. That certainly is ordered on x86. In fact, afaik it's ordered
> > even if that writel() might be of type WC, because that only delays
> > writes, it doesn't move them earlier.
>
> Now that we clarified x86 myth, Is this guaranteed on all architectures?
If not we need to fix it. It's guaranteed on the "main" ones (arm,
arm64, powerpc, i386, x86_64). We might need to check with other arch
maintainers for the rest.
We really want Linux to provide well defined "sane" semantics for the
basic writel accessors.
Note: We still have open questions about how readl() relates to
surrounding memory accesses. It looks like ARM and powerpc do different
things here.
> We keep getting IA64 exception example. Maybe, this is also corrected since
> then.
I would think ia64 fixed it back when it was all discussed. I was under
the impression all ia64 had "special" was the writel vs. spin_unlock
which requires mmiowb, but maybe that was never completely fixed ?
> Jose Abreu says "I don't know about x86 but arc architecture doesn't
> have a wmb() in the writel() function (in some configs)".
Well, it probably should then.
> As long as we have these exceptions, these wmb() in common drivers is not
> going anywhere and relaxed-arches will continue paying performance penalty.
Well, let's fix them or leave them broken, at this point, it doesn't
matter. We can give all arch maintainers a wakeup call and start making
drivers work based on the documented assumptions.
> I see 15% performance loss on ARM64 servers using Intel i40e network
> drivers and an XL710 adapter due to CPU keeping itself busy doing barriers
> most of the time rather than real work because of sequences like this all over
> the place.
>
> dma_buffer->foo = 1; /* WB */
> wmb()
> writel(KICK, DMA_KICK_REGISTER); /* UC */
>
> I posted several patches last week to remove duplicate barriers on ARM while
> trying to make the code friendly with other architectures.
>
> Basically changing it to
>
> dma_buffer->foo = 1; /* WB */
> wmb()
> writel_relaxed(KICK, DMA_KICK_REGISTER); /* UC */
> mmiowb()
>
> This is a small step in the performance direction until we remove all exceptions.
>
> https://www.spinics.net/lists/netdev/msg491842.html
> https://www.spinics.net/lists/linux-rdma/msg62434.html
> https://www.spinics.net/lists/arm-kernel/msg642336.html
>
> Discussion started to move around the need for relaxed API on PPC and then
> why wmb() question came up.
I'm working on the problem of relaxed APIs for powerpc, but we can keep
that orthogonal. As is, today, a wmb() + writel() and a wmb() +
writel_relaxed() on powerpc are identical. So changing them will not
break us.
But I don't see the point of doing that transformation if we can just
get the straying archs fixed. It's not like any of them has a
significant market presence these days anyway.
Cheers,
Ben.
> Sinan
>
^ permalink raw reply
* Re: [patch net-next RFC 00/12] devlink: introduce port flavours and common phys_port_name generation
From: Stephen Hemminger @ 2018-03-28 5:02 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, idosch, jakub.kicinski, mlxsw, andrew,
vivien.didelot, f.fainelli, michael.chan, ganeshgr, saeedm,
simon.horman, pieter.jansenvanvuuren, john.hurley,
dirk.vandermerwe, alexander.h.duyck, ogerlitz, dsahern,
vijaya.guvva, satananda.burla, raghu.vatsavayi, felix.manlunas,
gospo, sathya.perla, vasundhara-v.volam, tariqt, eranbe,
jeffrey.t.kirsher
In-Reply-To: <20180322105522.8186-1-jiri@resnulli.us>
On Thu, 22 Mar 2018 11:55:10 +0100
Jiri Pirko <jiri@resnulli.us> wrote:
> From: Jiri Pirko <jiri@mellanox.com>
>
> This patchset resolves 2 issues we have right now:
> 1) There are many netdevices / ports in the system, for port, pf, vf
> represenatation but the user has no way to see which is which
There already are a lot of attributes, adding more doesn't necessarily
help make things clearer.
> 2) The ndo_get_phys_port_name is implemented in each driver separatelly,
> which may lead to inconsistent names between drivers.
Why not address that problem. My concern is that your new attribute
will have the same problem.
Also adding pf and vfNNN on the name will make the already tightly squeezed
interface name length a real problem. I have had arguments with people
trying use VLAN 4000 and standard naming policy. Which means you really
can't go that long.
^ permalink raw reply
* Re: possible deadlock in rtnl_lock (5)
From: Dmitry Vyukov @ 2018-03-28 5:56 UTC (permalink / raw)
To: Julian Anastasov
Cc: Florian Westphal, syzbot, netdev, lvs-devel, syzkaller-bugs
In-Reply-To: <alpine.LFD.2.20.1803272227370.3460@ja.home.ssi.bg>
Please keep the Reported-by notice, and reproducer will probably be useful too:
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+a46d6abf9d56b1365a72@syzkaller.appspotmail.com
It will help syzbot understand when the bug is fixed. See footer for details.
If you forward the report, please keep this part and the footer.
syzbot hit the following crash on upstream commit
3eb2ce825ea1ad89d20f7a3b5780df850e4be274 (Sun Mar 25 22:44:30 2018 +0000)
Linux 4.16-rc7
syzbot dashboard link:
https://syzkaller.appspot.com/bug?extid=a46d6abf9d56b1365a72
So far this crash happened 27 times on net-next, upstream.
C reproducer: https://syzkaller.appspot.com/x/repro.c?id=6524202618191872
syzkaller reproducer:
https://syzkaller.appspot.com/x/repro.syz?id=5383267238805504
Raw console output: https://syzkaller.appspot.com/x/log.txt?id=5136472378179584
Kernel config: https://syzkaller.appspot.com/x/.config?id=-8440362230543204781
compiler: gcc (GCC) 7.1.1 20170620
On Tue, Mar 27, 2018 at 9:52 PM, Julian Anastasov <ja@ssi.bg> wrote:
>
> Hello,
>
> On Tue, 27 Mar 2018, Florian Westphal wrote:
>
>> syzbot <syzbot+a46d6abf9d56b1365a72@syzkaller.appspotmail.com> wrote:
>> [ cc Julian and trimming cc list ]
>>
>> > syzkaller688027/4497 is trying to acquire lock:
>> > (rtnl_mutex){+.+.}, at: [<00000000bb14d7fb>] rtnl_lock+0x17/0x20
>> > net/core/rtnetlink.c:74
>>
>> > but task is already holding lock:
>> > IPVS: stopping backup sync thread 4495 ...
>> > (rtnl_mutex){+.+.}, at: [<00000000bb14d7fb>] rtnl_lock+0x17/0x20
>> > net/core/rtnetlink.c:74
>> >
>> > other info that might help us debug this:
>> > Possible unsafe locking scenario:
>> >
>> > CPU0
>> > ----
>> > lock(rtnl_mutex);
>> > lock(rtnl_mutex);
>> >
>> > *** DEADLOCK ***
>> >
>> > May be due to missing lock nesting notation
>>
>> Looks like this is real, commit e0b26cc997d57305b4097711e12e13992580ae34
>> ("ipvs: call rtnl_lock early") added rtnl_lock when starting sync thread
>> but socket close invokes rtnl_lock too:
>
> I see, thanks! I'll have to move the locks into
> start_sync_thread and to split make_{send,receive}_sock
> to {make,setup}_{send,receive}_sock ...
>
>> > stack backtrace:
>> > rtnl_lock+0x17/0x20 net/core/rtnetlink.c:74
>> > ip_mc_drop_socket+0x88/0x230 net/ipv4/igmp.c:2643
>> > inet_release+0x4e/0x1c0 net/ipv4/af_inet.c:413
>> > sock_release+0x8d/0x1e0 net/socket.c:595
>> > start_sync_thread+0x2213/0x2b70 net/netfilter/ipvs/ip_vs_sync.c:1924
>> > do_ip_vs_set_ctl+0x1139/0x1cc0 net/netfilter/ipvs/ip_vs_ctl.c:2389
>
> Regards
>
> --
> Julian Anastasov <ja@ssi.bg>
>
> --
> You received this message because you are subscribed to the Google Groups "syzkaller-bugs" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to syzkaller-bugs+unsubscribe@googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/syzkaller-bugs/alpine.LFD.2.20.1803272227370.3460%40ja.home.ssi.bg.
> For more options, visit https://groups.google.com/d/optout.
^ permalink raw reply
* Re: [PATCH 1/2] af_key: Use DIV_ROUND_UP() instead of open-coded equivalent
From: Steffen Klassert @ 2018-03-28 5:59 UTC (permalink / raw)
To: Kevin Easton; +Cc: Herbert Xu, David S. Miller, netdev, linux-kernel
In-Reply-To: <930c4e2a88e93c6863ddb97df9ebd0fa1b32149e.1522063171.git.kevin@guarana.org>
On Mon, Mar 26, 2018 at 07:39:16AM -0400, Kevin Easton wrote:
> Several places use (x + 7) / 8 to convert from a number of bits to a number
> of bytes. Replace those with DIV_ROUND_UP(x, 8) instead, for consistency
> with other parts of the same file.
>
> Signed-off-by: Kevin Easton <kevin@guarana.org>
Is this a fix or just a cleanup?
If it is just a cleanup, please resent it based on ipsec-next.
^ permalink raw reply
* Re: [PATCH 5/6] rhashtable: support guaranteed successful insertion.
From: Herbert Xu @ 2018-03-28 6:04 UTC (permalink / raw)
To: NeilBrown; +Cc: Thomas Graf, netdev, linux-kernel
In-Reply-To: <87y3id42zo.fsf@notabene.neil.brown.name>
On Wed, Mar 28, 2018 at 08:34:19AM +1100, NeilBrown wrote:
>
> It is easy to get an -EBUSY insertion failure when .disable_count is
> enabled, and I did get that. Blindly propagating that up caused lustre
> to get terribly confused - not too surprising really.
Right, so this failure mode is specific to your patch 6.
I think I see the problem. As it currently stands, we never
need growing when we hit the bucket length limit. We only do
rehashes.
With your patch, you must change it so that we actually try to
grow the table if necessary when we hit the bucket length limit.
Otherwise it will result in the EBUSY that you're seeing.
I laso think that we shouldn't make this a toggle. If we're going
to do disable_count then it should be unconditionally done for
everyone.
However, I personally would prefer a percpu elem count instead of
disabling it completely. Would that be acceptable to your use-case?
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH 4/6] rhashtable: allow a walk of the hash table without missing objects.
From: Herbert Xu @ 2018-03-28 6:07 UTC (permalink / raw)
To: NeilBrown; +Cc: Thomas Graf, netdev, linux-kernel
In-Reply-To: <87po3p421q.fsf@notabene.neil.brown.name>
On Wed, Mar 28, 2018 at 08:54:41AM +1100, NeilBrown wrote:
>
> Possibly.
> I particularly want the interface to require that you pass the
> previously returned object to _continue. That makes it easy to see that
> the object is still being used. If someone changes to code to delete
> the object before the _continue, there should be a strong hint that it
> won't work.
>
> Maybe it would be better to make it a WARN_ON()
>
> if (!obj || WARN_ON(iter->p != obj))
> iter->p = NULL;
This doesn't really protect against the case where obj is removed.
All it proves is that the user saved a copy of obj which we already
did anyway.
To detect an actual removal you'd need to traverse the list.
I have another idea: we could save insert the walkers into the
hash table chain at the end, essentially as a hidden list. We
can mark it with a flag like rht_marker so that normal traversal
doesn't see it.
That way the removal code can simply traverse that list and inform
them that the iterator is invalid.
Cheers,
--
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
^ permalink raw reply
* Re: [PATCH net 0/2] Fix vlan untag and insertion for bridge and vlan with reorder_hdr off
From: Eric Dumazet @ 2018-03-28 6:11 UTC (permalink / raw)
To: David Miller, makita.toshiaki; +Cc: netdev, brandon.carpenter, vyasevic
In-Reply-To: <20180316.100502.1719064295298272655.davem@davemloft.net>
On 03/16/2018 07:05 AM, David Miller wrote:
> From: Toshiaki Makita <makita.toshiaki@lab.ntt.co.jp>
> Date: Tue, 13 Mar 2018 14:51:26 +0900
>
>> As Brandon Carpenter reported[1], sending non-vlan-offloaded packets from
>> bridge devices ends up with corrupted packets. He narrowed down this problem
>> and found that the root cause is in skb_reorder_vlan_header().
>>
>> While I was working on fixing this problem, I found that the function does
>> not work properly for double tagged packets with reorder_hdr off as well.
>>
>> Patch 1 fixes these 2 problems in skb_reorder_vlan_header().
>>
>> And it turned out that fixing skb_reorder_vlan_header() is not sufficient
>> to receive double tagged packets with reorder_hdr off while I was testing the
>> fix. Vlan tags got out of order when vlan devices with reorder_hdr disabled
>> were stacked. Patch 2 fixes this problem.
>>
>> [1] https://www.spinics.net/lists/linux-ethernet-bridging/msg07039.html
>
> Series applied and queued up for -stable, thanks.
>
> I was thinking of pushing back on the addition of the ETH_TLEN UAPI visible
> macro, because I don't see any other system providing that define. But in
> the end I decided that it's harmless and really that header file is the
> correct location for such a definition.
>
> Thank you.
>
syzbot reported some crashes caused by a memmove(..., ..., count=-2)
So something needs to be refined I guess.
BUG: unable to handle kernel paging request at ffff8801cccb8000
IP: __memmove+0x24/0x1a0 arch/x86/lib/memmove_64.S:43
PGD 9cee067 P4D 9cee067 PUD 1d9401063 PMD 1cccb7063 PTE 2810100028101
Oops: 000b [#1] SMP KASAN
Dumping ftrace buffer:
(ftrace buffer empty)
Modules linked in:
CPU: 1 PID: 17663 Comm: syz-executor2 Not tainted 4.16.0-rc7+ #368
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
RIP: 0010:__memmove+0x24/0x1a0 arch/x86/lib/memmove_64.S:43
RSP: 0018:ffff8801cc046e28 EFLAGS: 00010287
RAX: ffff8801ccc244c4 RBX: fffffffffffffffe RCX: fffffffffff6c4c2
RDX: fffffffffffffffe RSI: ffff8801cccb7ffc RDI: ffff8801cccb8000
RBP: ffff8801cc046e48 R08: ffff8801ccc244be R09: ffffed0039984899
R10: 0000000000000001 R11: ffffed0039984898 R12: ffff8801ccc244c4
R13: ffff8801ccc244c0 R14: ffff8801d96b7c06 R15: ffff8801d96b7b40
FS: 00007febd562d700(0000) GS:ffff8801db300000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: ffff8801cccb8000 CR3: 00000001ccb2f006 CR4: 00000000001606e0
DR0: 0000000020000000 DR1: 0000000020000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000600
Call Trace:
memmove include/linux/string.h:360 [inline]
skb_reorder_vlan_header net/core/skbuff.c:5031 [inline]
skb_vlan_untag+0x470/0xc40 net/core/skbuff.c:5061
__netif_receive_skb_core+0x119c/0x3460 net/core/dev.c:4460
__netif_receive_skb+0x2c/0x1b0 net/core/dev.c:4627
netif_receive_skb_internal+0x10b/0x670 net/core/dev.c:4701
netif_receive_skb+0xae/0x390 net/core/dev.c:4725
tun_rx_batched.isra.50+0x5ee/0x870 drivers/net/tun.c:1555
tun_get_user+0x299e/0x3c20 drivers/net/tun.c:1962
tun_chr_write_iter+0xb9/0x160 drivers/net/tun.c:1990
call_write_iter include/linux/fs.h:1782 [inline]
new_sync_write fs/read_write.c:469 [inline]
__vfs_write+0x684/0x970 fs/read_write.c:482
vfs_write+0x189/0x510 fs/read_write.c:544
SYSC_write fs/read_write.c:589 [inline]
SyS_write+0xef/0x220 fs/read_write.c:581
do_syscall_64+0x281/0x940 arch/x86/entry/common.c:287
entry_SYSCALL_64_after_hwframe+0x42/0xb7
RIP: 0033:0x454879
RSP: 002b:00007febd562cc68 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
RAX: ffffffffffffffda RBX: 00007febd562d6d4 RCX: 0000000000454879
RDX: 0000000000000157 RSI: 0000000020000180 RDI: 0000000000000014
RBP: 000000000072bea0 R08: 0000000000000000 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00000000ffffffff
R13: 00000000000006b0 R14: 00000000006fc120 R15: 0000000000000000
Code: 90 90 90 90 90 90 90 48 89 f8 48 83 fa 20 0f 82 03 01 00 00 48 39 fe 7d 0f 49 89 f0 49 01 d0 49 39 f8 0f 8f 9f 00 00 00 48 89 d1 <f3> a4 c3 48 81 fa a8 02 00 00 72 05 40 38 fe 74 3b 48 83 ea 20
RIP: __memmove+0x24/0x1a0 arch/x86/lib/memmove_64.S:43 RSP: ffff8801cc046e28
CR2: ffff8801cccb8000
---[ end trace b21c0866ee797d6d ]---
BUG: unable to handle kernel
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Linus Torvalds @ 2018-03-28 6:14 UTC (permalink / raw)
To: Sinan Kaya
Cc: Benjamin Herrenschmidt, Alexander Duyck, Will Deacon,
Arnd Bergmann, Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Alexander Duyck, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <bf77be28-051d-79c8-9b65-948c8a254d6d@codeaurora.org>
On Tue, Mar 27, 2018 at 5:24 PM, Sinan Kaya <okaya@codeaurora.org> wrote:
>
> Basically changing it to
>
> dma_buffer->foo = 1; /* WB */
> wmb()
> writel_relaxed(KICK, DMA_KICK_REGISTER); /* UC */
> mmiowb()
Why?
Why not just remove the wmb(), and keep the barrier in the writel()?
The above code makes no sense, and just looks stupid to me. It also
generates pointlessly bad code on x86, so it's bad there too.
Linus
^ permalink raw reply
* Re: [patch net-next RFC 00/12] devlink: introduce port flavours and common phys_port_name generation
From: Jiri Pirko @ 2018-03-28 6:17 UTC (permalink / raw)
To: Stephen Hemminger
Cc: netdev, davem, idosch, jakub.kicinski, mlxsw, andrew,
vivien.didelot, f.fainelli, michael.chan, ganeshgr, saeedm,
simon.horman, pieter.jansenvanvuuren, john.hurley,
dirk.vandermerwe, alexander.h.duyck, ogerlitz, dsahern,
vijaya.guvva, satananda.burla, raghu.vatsavayi, felix.manlunas,
gospo, sathya.perla, vasundhara-v.volam, tariqt, eranbe,
jeffrey.t.kirsher
In-Reply-To: <20180327220234.489a54fa@xeon-e3>
Wed, Mar 28, 2018 at 07:02:34AM CEST, stephen@networkplumber.org wrote:
>On Thu, 22 Mar 2018 11:55:10 +0100
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> From: Jiri Pirko <jiri@mellanox.com>
>>
>> This patchset resolves 2 issues we have right now:
>> 1) There are many netdevices / ports in the system, for port, pf, vf
>> represenatation but the user has no way to see which is which
>
>There already are a lot of attributes, adding more doesn't necessarily
>help make things clearer.
How elso you distinguish pfrep/vfrep/cpuport/etc?
>
>> 2) The ndo_get_phys_port_name is implemented in each driver separatelly,
>> which may lead to inconsistent names between drivers.
>
>Why not address that problem. My concern is that your new attribute
>will have the same problem.
I try to address that...
>
>Also adding pf and vfNNN on the name will make the already tightly squeezed
>interface name length a real problem. I have had arguments with people
>trying use VLAN 4000 and standard naming policy. Which means you really
>can't go that long.
Understood. However, I just do what is already done in nfp for example.
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: sockmap: initialize sg table entries properly
From: Prashant Bhole @ 2018-03-28 6:18 UTC (permalink / raw)
To: Daniel Borkmann
Cc: John Fastabend, Alexei Starovoitov, David S . Miller, netdev
In-Reply-To: <c120c2f6-237c-0fbf-4069-2770cedf38e6@iogearbox.net>
On 3/27/2018 6:05 PM, Daniel Borkmann wrote:
> On 03/27/2018 10:41 AM, Prashant Bhole wrote:
>> On 3/27/2018 12:15 PM, John Fastabend wrote:
>>> On 03/25/2018 11:54 PM, Prashant Bhole wrote:
>>>> When CONFIG_DEBUG_SG is set, sg->sg_magic is initialized to SG_MAGIC,
>>>> when sg table is initialized using sg_init_table(). Magic is checked
>>>> while navigating the scatterlist. We hit BUG_ON when magic check is
>>>> failed.
>>>>
>>>> Fixed following things:
>>>> - Initialization of sg table in bpf_tcp_sendpage() was missing,
>>>> initialized it using sg_init_table()
>>>>
>>>> - bpf_tcp_sendmsg() initializes sg table using sg_init_table() before
>>>> entering the loop, but further consumed sg entries are initialized
>>>> using memset. Fixed it by replacing memset with sg_init_table() in
>>>> function bpf_tcp_push()
>>>>
>>>> Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
>>>> ---
>>>> kernel/bpf/sockmap.c | 11 +++++++----
>>>> 1 file changed, 7 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/kernel/bpf/sockmap.c b/kernel/bpf/sockmap.c
>>>> index 69c5bccabd22..8a848a99d768 100644
>>>> --- a/kernel/bpf/sockmap.c
>>>> +++ b/kernel/bpf/sockmap.c
>>>> @@ -312,7 +312,7 @@ static int bpf_tcp_push(struct sock *sk, int apply_bytes,
>>>> md->sg_start++;
>>>> if (md->sg_start == MAX_SKB_FRAGS)
>>>> md->sg_start = 0;
>>>> - memset(sg, 0, sizeof(*sg));
>>>> + sg_init_table(sg, 1);
>>>
>>> Looks OK here.
>>>
>>>> if (md->sg_start == md->sg_end)
>>>> break;
>>>> @@ -763,10 +763,14 @@ static int bpf_tcp_sendpage(struct sock *sk, struct page *page,
>>>> lock_sock(sk);
>>>> - if (psock->cork_bytes)
>>>> + if (psock->cork_bytes) {
>>>> m = psock->cork;
>>>> - else
>>>> + sg = &m->sg_data[m->sg_end];
>>>> + } else {
>>>> m = &md;
>>>> + sg = m->sg_data;
>>>> + sg_init_table(sg, MAX_SKB_FRAGS);
>>>
>>> sg_init_table() does an unnecessary memset() though. We
>>> probably either want a new scatterlist API or just open
>>> code this,
>>>
>>> #ifdef CONFIG_DEBUG_SG
>>> {
>>> unsigned int i;
>>> for (i = 0; i < nents; i++)
>>> sgl[i].sg_magic = SG_MAGIC;
>>> }
>>
>> Similar sg_init_table() is present in bpf_tcp_sendmsg().
>> I agree that it causes unnecessary memset, but I don't agree with open coded fix.
>
> But then lets fix is properly and add a static inline helper to the
> include/linux/scatterlist.h header like ...
>
> static inline void sg_init_debug_marker(struct scatterlist *sgl,
> unsigned int nents)
> {
> #ifdef CONFIG_DEBUG_SG
> unsigned int i;
>
> for (i = 0; i < nents; i++)
> sgl[i].sg_magic = SG_MAGIC;
> #endif
> }
>
> ... and reuse it in all the places that would otherwise open-code this,
> as well as sg_init_table():
>
> void sg_init_table(struct scatterlist *sgl, unsigned int nents)
> {
> memset(sgl, 0, sizeof(*sgl) * nents);
> sg_init_debug_marker(sgl, nents);
> sg_mark_end(&sgl[nents - 1]);
> }
>
> This would be a lot cleaner than having this duplicated in various places.
Daniel, This is a good suggestion. Is it ok if I submit both changes in
a patch series? How scatterlist related changes will be picked up by
other subsystems?
-Prashant
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Linus Torvalds @ 2018-03-28 6:26 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Alexander Duyck, Will Deacon, Sinan Kaya, Arnd Bergmann,
Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <1522211620.7364.94.camel@kernel.crashing.org>
On Tue, Mar 27, 2018 at 6:33 PM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
>
> This is why, I want (with your agreement) to define clearly and once
> and for all, that the Linux semantics of writel are that it is ordered
> with previous writes to coherent memory (*)
Honestly, I think those are the sane semantics. In fact, make it
"ordered with previous writes" full stop, since it's not only ordered
wrt previous writes to memory, but also previous writel's.
> Also, can I assume the above ordering with writel() equally applies to
> readl() or not ?
>
> IE:
> dma_buf->foo = 1;
> readl(STUPID_DEVICE_DMA_KICK_ON_READ);
If that KICK_ON_READ is UC, then that's definitely the case. And
honestly, status registers like that really should always be UC.
But if somebody sets the area WC (which is crazy), then I think it
might be at least debatable. x86 semantics does allow reads to be done
before previous writes (or, put another way, writes to be buffered -
the buffers are ordered so writes don't get re-ordered, but reads can
happen during the buffering).
But UC accesses are always done entirely ordered, and honestly, any
status register that starts a DMA would not make sense any other way.
Of course, you'd have to be pretty odd to want to start a DMA with a
read anyway - partly exactly because it's bad for performance since
reads will be synchronous and not buffered like a write).
Linus
^ permalink raw reply
* Re: [PATCH] vhost-net: add time limitation for tx polling(Internet mail)
From: Jason Wang @ 2018-03-28 6:37 UTC (permalink / raw)
To: haibinzhang(张海斌), mst@redhat.com,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
netdev@vger.kernel.org, linux-kernel@vger.kernel.org
Cc: lidongchen(陈立东),
yunfangtai(台运方)
In-Reply-To: <88D661ADF6AFBF42B2AB88D8E7682B0901FC412A@EXMBX-SZMAIL011.tencent.com>
On 2018年03月28日 12:01, haibinzhang(张海斌) wrote:
> On 2018年03月27日 19:26, Jason wrote
> On 2018年03月27日 17:12, haibinzhang wrote:
>>> handle_tx() will delay rx for a long time when busy tx polling udp packets
>>> with short length(ie: 1byte udp payload), because setting VHOST_NET_WEIGHT
>>> takes into account only sent-bytes but no time.
>> Interesting.
>>
>> Looking at vhost_can_busy_poll() it tries to poke pending vhost work and
>> exit the busy loop if it found one. So I believe something block the
>> work queuing. E.g did reverting 8241a1e466cd56e6c10472cac9c1ad4e54bc65db
>> fix the issue?
> "busy tx polling" means using netperf send udp packets with 1 bytes payload(total 47bytes frame lenght),
> and handle_tx() will be busy sending packets continuously.
>
>>> It's not fair for handle_rx(),
>>> so needs to limit max time of tx polling.
>>>
>>> ---
>>> drivers/vhost/net.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
>>> index 8139bc70ad7d..dc9218a3a75b 100644
>>> --- a/drivers/vhost/net.c
>>> +++ b/drivers/vhost/net.c
>>> @@ -473,6 +473,7 @@ static void handle_tx(struct vhost_net *net)
>>> struct socket *sock;
>>> struct vhost_net_ubuf_ref *uninitialized_var(ubufs);
>>> bool zcopy, zcopy_used;
>>> + unsigned long start = jiffies;
>> Checking jiffies is tricky, need to convert it to ms or whatever others.
>>
>>>
>>> mutex_lock(&vq->mutex);
>>> sock = vq->private_data;
>>> @@ -580,7 +581,7 @@ static void handle_tx(struct vhost_net *net)
>>> else
>>> vhost_zerocopy_signal_used(net, vq);
>>> vhost_net_tx_packet(net);
>>> - if (unlikely(total_len >= VHOST_NET_WEIGHT)) {
>>> + if (unlikely(total_len >= VHOST_NET_WEIGHT) || unlikely(jiffies - start >= 1)) {
>> How value 1 is determined here? And we need a complete test to make sure
>> this won't affect other use cases.
> We just want <1ms ping latency, but actually we are not sure what value is reasonable.
> We have some test results using netperf before this patch as follow,
>
> Udp payload 1byte 100bytes 1000bytes 1400bytes
> Ping avg latency 25ms 10ms 2ms 1.5ms
>
> What is other testcases?
Something like https://patchwork.kernel.org/patch/10151645/.
Btw, you need use time_before() to properly handle jiffies overflow and
I would also suggest you to try something like #packets limit (e.g 64).
For long term, we definitely need more worker threads.
Thanks
>
>> Another thought is introduce another limit of #packets, but this need
>> benchmark too.
>>
>> Thanks
>>
>>> vhost_poll_queue(&vq->poll);
>>> break;
>>> }
>>
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 6:42 UTC (permalink / raw)
To: Linus Torvalds
Cc: Alexander Duyck, Will Deacon, Sinan Kaya, Arnd Bergmann,
Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <CA+55aFw24R3YAujir59GdG+sBBQOA9WyXySEN3AcEYK99ZHuwQ@mail.gmail.com>
On Tue, 2018-03-27 at 20:26 -1000, Linus Torvalds wrote:
> On Tue, Mar 27, 2018 at 6:33 PM, Benjamin Herrenschmidt
> <benh@kernel.crashing.org> wrote:
> >
> > This is why, I want (with your agreement) to define clearly and once
> > and for all, that the Linux semantics of writel are that it is ordered
> > with previous writes to coherent memory (*)
>
> Honestly, I think those are the sane semantics. In fact, make it
> "ordered with previous writes" full stop, since it's not only ordered
> wrt previous writes to memory, but also previous writel's.
Of course. It was somewhat a given that it's ordered vs. any previous
MMIO actually, but it doesn't hurt to spell it out once more.
> > Also, can I assume the above ordering with writel() equally applies to
> > readl() or not ?
> >
> > IE:
> > dma_buf->foo = 1;
> > readl(STUPID_DEVICE_DMA_KICK_ON_READ);
>
> If that KICK_ON_READ is UC, then that's definitely the case. And
> honestly, status registers like that really should always be UC.
>
> But if somebody sets the area WC (which is crazy), then I think it
> might be at least debatable. x86 semantics does allow reads to be done
> before previous writes (or, put another way, writes to be buffered -
> the buffers are ordered so writes don't get re-ordered, but reads can
> happen during the buffering).
Right, for now I worry about UC semantics. Once we have nailed that, we
can look at WC, which is a lot more tricky as archs differs more
widely, but one thing at a time.
> But UC accesses are always done entirely ordered, and honestly, any
> status register that starts a DMA would not make sense any other way.
>
> Of course, you'd have to be pretty odd to want to start a DMA with a
> read anyway - partly exactly because it's bad for performance since
> reads will be synchronous and not buffered like a write).
I have bad memories of old adaptec controllers ...
That said, I think the above might not be right on ARM if we want to
make it the rule, Will, what do you reckon ?
Cheers,
Ben.
^ permalink raw reply
* RE: [PATCH] staging: fsl-dpaa2/ethsw: Fix TCI values overwrite
From: Razvan Stefanescu @ 2018-03-28 6:45 UTC (permalink / raw)
To: Andrew Lunn
Cc: gregkh@linuxfoundation.org, devel@driverdev.osuosl.org,
linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
Alexandru Marginean, Ruxandra Ioana Ciocoi Radulescu,
Ioana Ciornei, Laurentiu Tudor
In-Reply-To: <20180327133740.GI5862@lunn.ch>
> -----Original Message-----
> From: linux-kernel-owner@vger.kernel.org [mailto:linux-kernel-
> owner@vger.kernel.org] On Behalf Of Andrew Lunn
> Sent: Tuesday, March 27, 2018 4:38 PM
> To: Razvan Stefanescu <razvan.stefanescu@nxp.com>
> Cc: gregkh@linuxfoundation.org; devel@driverdev.osuosl.org; linux-
> kernel@vger.kernel.org; netdev@vger.kernel.org; Alexandru Marginean
> <alexandru.marginean@nxp.com>; Ruxandra Ioana Ciocoi Radulescu
> <ruxandra.radulescu@nxp.com>; Ioana Ciornei <ioana.ciornei@nxp.com>;
> Laurentiu Tudor <laurentiu.tudor@nxp.com>; stuyoder@gmail.com
> Subject: Re: [PATCH] staging: fsl-dpaa2/ethsw: Fix TCI values overwrite
>
> On Tue, Mar 27, 2018 at 08:10:50AM -0500, Razvan Stefanescu wrote:
> > Previous implementation overwrites PCP value, assuming the default value
> is
> > 0, instead of 7.
> >
> > Avoid this by modifying helper function ethsw_port_set_tci() to
> > ethsw_port_set_pvid() and make it update only the vlan_id of the tci_cfg
> > struct.
>
> Hi Razvan
>
> It is a good idea to explain acronyms, especially for staging, since
> there are patches for all sorts of devices, can you cannot expect
> everybody to know network specific acronyms.
>
> By PCP you mean Priority Code Point. TCI i have no idea about.
>
> Looking at the code, i think you are changing the flow to become
> read/modify/write, instead of just write, which is overwriting the
> previously configured Priority Code Point?
>
> Please try to add more details to your change logs, to help us
> understand the change.
>
Thank you Andrew. I'll address this in v2.
Best regards,
Razvan Stefanescu
^ permalink raw reply
* [PATCH v2] staging: fsl-dpaa2/ethsw: Fix tag control information value overwrite
From: Razvan Stefanescu @ 2018-03-28 6:46 UTC (permalink / raw)
To: gregkh
Cc: devel, andrew, netdev, alexandru.marginean, linux-kernel,
ioana.ciornei, laurentiu.tudor
The tag control information (TCI) part of the VLAN header contains several
fields, including PCP (priority code point) and PVID (port VLAN id).
Current implementation uses function ethsw_port_set_tci() to set the PVID
value and mistakenly overwrites the rest of the TCI fields with 0,
including PCP which by default has a value of 7.
Fix this by adding support to retrieve TCI set in hardware. Read existing
value and only updated the PVID fields, leaving others unchanged.
Signed-off-by: Razvan Stefanescu <razvan.stefanescu@nxp.com>
---
Changelog
v2: improve patch description
drivers/staging/fsl-dpaa2/ethsw/dpsw-cmd.h | 13 +++++++++
drivers/staging/fsl-dpaa2/ethsw/dpsw.c | 42 ++++++++++++++++++++++++++++++
drivers/staging/fsl-dpaa2/ethsw/dpsw.h | 6 +++++
drivers/staging/fsl-dpaa2/ethsw/ethsw.c | 37 +++++++++++++-------------
4 files changed, 79 insertions(+), 19 deletions(-)
diff --git a/drivers/staging/fsl-dpaa2/ethsw/dpsw-cmd.h b/drivers/staging/fsl-dpaa2/ethsw/dpsw-cmd.h
index 1c203e6..da744f2 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/dpsw-cmd.h
+++ b/drivers/staging/fsl-dpaa2/ethsw/dpsw-cmd.h
@@ -49,6 +49,8 @@
#define DPSW_CMDID_IF_SET_FLOODING DPSW_CMD_ID(0x047)
#define DPSW_CMDID_IF_SET_BROADCAST DPSW_CMD_ID(0x048)
+#define DPSW_CMDID_IF_GET_TCI DPSW_CMD_ID(0x04A)
+
#define DPSW_CMDID_IF_SET_LINK_CFG DPSW_CMD_ID(0x04C)
#define DPSW_CMDID_VLAN_ADD DPSW_CMD_ID(0x060)
@@ -206,6 +208,17 @@ struct dpsw_cmd_if_set_tci {
__le16 conf;
};
+struct dpsw_cmd_if_get_tci {
+ __le16 if_id;
+};
+
+struct dpsw_rsp_if_get_tci {
+ __le16 pad;
+ __le16 vlan_id;
+ u8 dei;
+ u8 pcp;
+};
+
#define DPSW_STATE_SHIFT 0
#define DPSW_STATE_SIZE 4
diff --git a/drivers/staging/fsl-dpaa2/ethsw/dpsw.c b/drivers/staging/fsl-dpaa2/ethsw/dpsw.c
index 9b9bc60..3ea957c 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/dpsw.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/dpsw.c
@@ -529,6 +529,48 @@ int dpsw_if_set_tci(struct fsl_mc_io *mc_io,
}
/**
+ * dpsw_if_get_tci() - Get default VLAN Tag Control Information (TCI)
+ * @mc_io: Pointer to MC portal's I/O object
+ * @cmd_flags: Command flags; one or more of 'MC_CMD_FLAG_'
+ * @token: Token of DPSW object
+ * @if_id: Interface Identifier
+ * @cfg: Tag Control Information Configuration
+ *
+ * Return: Completion status. '0' on Success; Error code otherwise.
+ */
+int dpsw_if_get_tci(struct fsl_mc_io *mc_io,
+ u32 cmd_flags,
+ u16 token,
+ u16 if_id,
+ struct dpsw_tci_cfg *cfg)
+{
+ struct mc_command cmd = { 0 };
+ struct dpsw_cmd_if_get_tci *cmd_params;
+ struct dpsw_rsp_if_get_tci *rsp_params;
+ int err;
+
+ /* prepare command */
+ cmd.header = mc_encode_cmd_header(DPSW_CMDID_IF_GET_TCI,
+ cmd_flags,
+ token);
+ cmd_params = (struct dpsw_cmd_if_get_tci *)cmd.params;
+ cmd_params->if_id = cpu_to_le16(if_id);
+
+ /* send command to mc*/
+ err = mc_send_command(mc_io, &cmd);
+ if (err)
+ return err;
+
+ /* retrieve response parameters */
+ rsp_params = (struct dpsw_rsp_if_get_tci *)cmd.params;
+ cfg->pcp = rsp_params->pcp;
+ cfg->dei = rsp_params->dei;
+ cfg->vlan_id = le16_to_cpu(rsp_params->vlan_id);
+
+ return 0;
+}
+
+/**
* dpsw_if_set_stp() - Function sets Spanning Tree Protocol (STP) state.
* @mc_io: Pointer to MC portal's I/O object
* @cmd_flags: Command flags; one or more of 'MC_CMD_FLAG_'
diff --git a/drivers/staging/fsl-dpaa2/ethsw/dpsw.h b/drivers/staging/fsl-dpaa2/ethsw/dpsw.h
index 3335add..82f80c40 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/dpsw.h
+++ b/drivers/staging/fsl-dpaa2/ethsw/dpsw.h
@@ -306,6 +306,12 @@ int dpsw_if_set_tci(struct fsl_mc_io *mc_io,
u16 if_id,
const struct dpsw_tci_cfg *cfg);
+int dpsw_if_get_tci(struct fsl_mc_io *mc_io,
+ u32 cmd_flags,
+ u16 token,
+ u16 if_id,
+ struct dpsw_tci_cfg *cfg);
+
/**
* enum dpsw_stp_state - Spanning Tree Protocol (STP) states
* @DPSW_STP_STATE_BLOCKING: Blocking state
diff --git a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
index c723a04..ab81a6c 100644
--- a/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
+++ b/drivers/staging/fsl-dpaa2/ethsw/ethsw.c
@@ -50,14 +50,23 @@ static int ethsw_add_vlan(struct ethsw_core *ethsw, u16 vid)
return 0;
}
-static int ethsw_port_set_tci(struct ethsw_port_priv *port_priv,
- struct dpsw_tci_cfg *tci_cfg)
+static int ethsw_port_set_pvid(struct ethsw_port_priv *port_priv, u16 pvid)
{
struct ethsw_core *ethsw = port_priv->ethsw_data;
struct net_device *netdev = port_priv->netdev;
+ struct dpsw_tci_cfg tci_cfg = { 0 };
bool is_oper;
int err, ret;
+ err = dpsw_if_get_tci(ethsw->mc_io, 0, ethsw->dpsw_handle,
+ port_priv->idx, &tci_cfg);
+ if (err) {
+ netdev_err(netdev, "dpsw_if_get_tci err %d\n", err);
+ return err;
+ }
+
+ tci_cfg.vlan_id = pvid;
+
/* Interface needs to be down to change PVID */
is_oper = netif_oper_up(netdev);
if (is_oper) {
@@ -71,17 +80,16 @@ static int ethsw_port_set_tci(struct ethsw_port_priv *port_priv,
}
err = dpsw_if_set_tci(ethsw->mc_io, 0, ethsw->dpsw_handle,
- port_priv->idx, tci_cfg);
+ port_priv->idx, &tci_cfg);
if (err) {
netdev_err(netdev, "dpsw_if_set_tci err %d\n", err);
goto set_tci_error;
}
/* Delete previous PVID info and mark the new one */
- if (port_priv->pvid)
- port_priv->vlans[port_priv->pvid] &= ~ETHSW_VLAN_PVID;
- port_priv->vlans[tci_cfg->vlan_id] |= ETHSW_VLAN_PVID;
- port_priv->pvid = tci_cfg->vlan_id;
+ port_priv->vlans[port_priv->pvid] &= ~ETHSW_VLAN_PVID;
+ port_priv->vlans[pvid] |= ETHSW_VLAN_PVID;
+ port_priv->pvid = pvid;
set_tci_error:
if (is_oper) {
@@ -133,13 +141,7 @@ static int ethsw_port_add_vlan(struct ethsw_port_priv *port_priv,
}
if (flags & BRIDGE_VLAN_INFO_PVID) {
- struct dpsw_tci_cfg tci_cfg = {
- .pcp = 0,
- .dei = 0,
- .vlan_id = vid,
- };
-
- err = ethsw_port_set_tci(port_priv, &tci_cfg);
+ err = ethsw_port_set_pvid(port_priv, vid);
if (err)
return err;
}
@@ -819,9 +821,7 @@ static int ethsw_port_del_vlan(struct ethsw_port_priv *port_priv, u16 vid)
return -ENOENT;
if (port_priv->vlans[vid] & ETHSW_VLAN_PVID) {
- struct dpsw_tci_cfg tci_cfg = { 0 };
-
- err = ethsw_port_set_tci(port_priv, &tci_cfg);
+ err = ethsw_port_set_pvid(port_priv, 0);
if (err)
return err;
}
@@ -1254,7 +1254,6 @@ static int ethsw_port_init(struct ethsw_port_priv *port_priv, u16 port)
const char def_mcast[ETH_ALEN] = {0x01, 0x00, 0x5e, 0x00, 0x00, 0x01};
struct net_device *netdev = port_priv->netdev;
struct ethsw_core *ethsw = port_priv->ethsw_data;
- struct dpsw_tci_cfg tci_cfg = {0};
struct dpsw_vlan_if_cfg vcfg;
int err;
@@ -1272,7 +1271,7 @@ static int ethsw_port_init(struct ethsw_port_priv *port_priv, u16 port)
return err;
}
- err = ethsw_port_set_tci(port_priv, &tci_cfg);
+ err = ethsw_port_set_pvid(port_priv, 0);
if (err)
return err;
--
1.9.1
^ permalink raw reply related
* Re: RFC on writel and writel_relaxed
From: Linus Torvalds @ 2018-03-28 6:53 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Arnd Bergmann, linux-rdma@vger.kernel.org, netdev@vger.kernel.org,
Will Deacon, Alexander Duyck, Sinan Kaya, Jason Gunthorpe,
David Laight, Oliver, Paul E. McKenney,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT)
In-Reply-To: <1522219376.7364.109.camel@kernel.crashing.org>
[-- Attachment #1: Type: text/plain, Size: 1086 bytes --]
On Tue, Mar 27, 2018, 20:43 Benjamin Herrenschmidt <benh@kernel.crashing.org>
wrote:
> >
> > Of course, you'd have to be pretty odd to want to start a DMA with a
> > read anyway - partly exactly because it's bad for performance since
> > reads will be synchronous and not buffered like a write).
>
> I have bad memories of old adaptec controllers ...
>
*Old* adaptec controllers were likely to use the in/out instructions for
status and command data.
Those are actually even more ordered than UC reads and writes: the in/out
instructions are not just fully ordered, but are fully *synchronous* on
x86.
So not just doing accesses in order, but actually waiting for everything to
drain before they start executing, but they also wait for the operation
itself to complete (ie "out" will not just queue the write, it will then
wait for the queue to empty and the write data to hit the line).
That's why in/out were *so* slow, and why nobody uses them any more (well,
the address size limitations and the lack of any remapping of the address
obviously also are a reason).
Linus
>
[-- Attachment #2: Type: text/html, Size: 1798 bytes --]
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Benjamin Herrenschmidt @ 2018-03-28 6:56 UTC (permalink / raw)
To: Linus Torvalds
Cc: Alexander Duyck, Will Deacon, Sinan Kaya, Arnd Bergmann,
Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <CA+55aFyaZ=WEXFg4KZ0yCWaR=8Dm_FPK-xcA0EYi3fEzm+KZkA@mail.gmail.com>
On Wed, 2018-03-28 at 06:53 +0000, Linus Torvalds wrote:
>
>
> On Tue, Mar 27, 2018, 20:43 Benjamin Herrenschmidt <benh@kernel.crash
> ing.org> wrote:
> > >
> > > Of course, you'd have to be pretty odd to want to start a DMA
> > with a
> > > read anyway - partly exactly because it's bad for performance
> > since
> > > reads will be synchronous and not buffered like a write).
> >
> > I have bad memories of old adaptec controllers ...
>
> *Old* adaptec controllers were likely to use the in/out instructions
> for status and command data.
>
> Those are actually even more ordered than UC reads and writes: the
> in/out instructions are not just fully ordered, but are fully
> *synchronous* on x86.
>
> So not just doing accesses in order, but actually waiting for
> everything to drain before they start executing, but they also wait
> for the operation itself to complete (ie "out" will not just queue
> the write, it will then wait for the queue to empty and the write
> data to hit the line).
>
> That's why in/out were *so* slow, and why nobody uses them any more
> (well, the address size limitations and the lack of any remapping of
> the address obviously also are a reason).
All true indeed, though a lot of other archs never quite made them
fully synchronous, which was another can of worms ... oh well.
As for Adaptec, you might be right, I do remember having cases of old
stuff triggering DMA on reads, it might have been "Mac" variants of
Adaptec using MMIO or something...
Cheers,
Ben.
^ permalink raw reply
* RE: [PATCH net-next 3/4] qed: Adapter flash update support.
From: Kalluru, Sudarsana @ 2018-03-28 6:56 UTC (permalink / raw)
To: Yuval Mintz; +Cc: davem@davemloft.net, netdev@vger.kernel.org, Elior, Ariel
In-Reply-To: <20180327133635.GB28037@dev-r-vrt-155.mtr.labs.mlnx>
-----Original Message-----
From: Yuval Mintz [mailto:yuvalm@mellanox.com]
Sent: 27 March 2018 19:07
To: Kalluru, Sudarsana <Sudarsana.Kalluru@cavium.com>
Cc: davem@davemloft.net; netdev@vger.kernel.org; Elior, Ariel <Ariel.Elior@cavium.com>
Subject: Re: [PATCH net-next 3/4] qed: Adapter flash update support.
On Mon, Mar 26, 2018 at 03:13:47AM -0700, Sudarsana Reddy Kalluru wrote:
> This patch adds the required driver support for updating the flash or
> non volatile memory of the adapter. At highlevel, flash upgrade
> comprises of reading the flash images from the input file, validating
> the images and writing it to the respective paritions.
s/it/them/
[...]
> + * /----------------------------------------------------------------------\
> + * 0B | 0x4 [command index] |
> + * 4B | image_type | Options | Number of register settings |
> + * 8B | Value |
> + * 12B | Mask |
> + * 16B | Offset |
> + * \----------------------------------------------------------------------/
> + * There can be several Value-Mask-Offset sets as specified by 'Number of...'.
> + * Options - 0'b - Calculate & Update CRC for image */ static int
> +qed_nvm_flash_image_access(struct qed_dev *cdev, const u8 **data,
> + bool *check_resp)
> +{
> + struct qed_nvm_image_att nvm_image;
> + struct qed_hwfn *p_hwfn;
> + bool is_crc = false;
> + u32 image_type;
> + int rc = 0, i;
> + u16 len;
> +
+
> + nvm_image.start_addr = p_hwfn->nvm_info.image_att[i].nvm_start_addr;
> + nvm_image.length = p_hwfn->nvm_info.image_att[i].len;
> +
> + DP_VERBOSE(cdev, NETIF_MSG_DRV,
> + "Read image %02x; type = %08x; NVM [%08x,...,%08x]\n",
> + **data, nvm_image.start_addr, image_type,
> + nvm_image.start_addr + nvm_image.length - 1);
Looks like 3rd and 4th printed parameters are flipped.
> + (*data)++;
> + is_crc = !!(**data);
If you'd actually want to be able to use the reserved bits [forward-compatibility] in the future, you should check bit 0 instead of checking the byte.
> + (*data)++;
> + len = *((u16 *)*data);
> + *data += 2;
[...]
> +
> +/* Binary file format -
> + * /----------------------------------------------------------------------\
> + * 0B | 0x3 [command index] |
> + * 4B | b'0: check_response? | b'1-127 reserved |
This shows there are 128 bits in a 4 byte field.
> + * 8B | File-type | reserved |
> + * \----------------------------------------------------------------------/
> + * Start a new file of the provided type
> + */
> +static int qed_nvm_flash_image_file_start(struct qed_dev *cdev,
> + const u8 **data, bool *check_resp) {
> + int rc;
> +
> + *data += 4;
> + *check_resp = !!(**data);
Like above
> + *data += 4;
> +
> + DP_VERBOSE(cdev, NETIF_MSG_DRV,
> + "About to start a new file of type %02x\n", **data);
> + rc = qed_mcp_nvm_put_file_begin(cdev, **data);
> + *data += 4;
> +
> + return rc;
> +}
> +
> +/* Binary file format -
> + * /----------------------------------------------------------------------\
> + * 0B | 0x2 [command index] |
> + * 4B | Length in bytes |
> + * 8B | b'0: check_response? | b'1-127 reserved |
Same as above
> + * 12B | Offset in bytes |
> + * 16B | Data ... |
> + * \----------------------------------------------------------------------/
> + * Write data as part of a file that was previously started. Data should be
> + * of length equal to that provided in the message
> + */
> +static int qed_nvm_flash_image_file_data(struct qed_dev *cdev,
> + const u8 **data, bool *check_resp) {
> + u32 offset, len;
> + int rc;
> +
> + *data += 4;
> + len = *((u32 *)(*data));
> + *data += 4;
> + *check_resp = !!(**data);
Same as above
> + *data += 4;
> + offset = *((u32 *)(*data));
> + *data += 4;
> +
> + DP_VERBOSE(cdev, NETIF_MSG_DRV,
> + "About to write File-data: %08x bytes to offset %08x\n",
> + len, offset);
> +
> + rc = qed_mcp_nvm_write(cdev, QED_PUT_FILE_DATA, offset,
> + (char *)(*data), len);
> + *data += len;
> +
> + return rc;
> +}
[...]
> +
> +static int qed_nvm_flash(struct qed_dev *cdev, const char *name) {
> + rc = qed_nvm_flash_image_validate(cdev, image, &data);
> + if (rc)
> + goto exit;
> +
> + while (data < data_end) {
> + bool check_resp = false;
> +
> + /* Parse the actual command */
> + cmd_type = *((u32 *)data);
What's the final format of the file? Is it LE?
<Sudarsana> The file contents are in LE format. Thanks for the detailed review. Will re-submit the patch series with the review comments incorporated.
> + switch (cmd_type) {
> + case QED_NVM_FLASH_CMD_FILE_DATA:
> + rc = qed_nvm_flash_image_file_data(cdev, &data,
> + &check_resp);
> + break;
> + case QED_NVM_FLASH_CMD_FILE_START:
> + rc = qed_nvm_flash_image_file_start(cdev, &data,
> + &check_resp);
> + break;
> + case QED_NVM_FLASH_CMD_NVM_CHANGE:
> + rc = qed_nvm_flash_image_access(cdev, &data,
> + &check_resp);
> + break;
> + default:
> + DP_ERR(cdev, "Unknown command %08x\n", cmd_type);
> + rc = -EINVAL;
> + break;
Either goto or drop the print; You're getting from the next condition.
> + }
> +
> + if (rc) {
> + DP_ERR(cdev, "Command %08x failed\n", cmd_type);
> + goto exit;
> + }
> +
> + /* Check response if needed */
> + if (check_resp) {
> + u32 mcp_response = 0;
> +
> + if (qed_mcp_nvm_resp(cdev, (u8 *)&mcp_response)) {
> + DP_ERR(cdev, "Failed getting MCP response\n");
> + rc = -EINVAL;
> + goto exit;
> + }
> +
> + switch (mcp_response & FW_MSG_CODE_MASK) {
> + case FW_MSG_CODE_OK:
> + case FW_MSG_CODE_NVM_OK:
> + case FW_MSG_CODE_NVM_PUT_FILE_FINISH_OK:
> + case FW_MSG_CODE_PHY_OK:
> + break;
> + default:
> + DP_ERR(cdev, "MFW returns error: %08x\n",
> + mcp_response);
> + rc = -EINVAL;
> + goto exit;
> + }
> + }
> + }
> +
> +exit:
> + release_firmware(image);
> +
> + return rc;
> +}
> +
^ permalink raw reply
* Re: [PATCH 5/6] rhashtable: support guaranteed successful insertion.
From: NeilBrown @ 2018-03-28 7:04 UTC (permalink / raw)
To: Herbert Xu; +Cc: Thomas Graf, netdev, linux-kernel
In-Reply-To: <20180328060432.GA16291@gondor.apana.org.au>
[-- Attachment #1: Type: text/plain, Size: 2667 bytes --]
On Wed, Mar 28 2018, Herbert Xu wrote:
> On Wed, Mar 28, 2018 at 08:34:19AM +1100, NeilBrown wrote:
>>
>> It is easy to get an -EBUSY insertion failure when .disable_count is
>> enabled, and I did get that. Blindly propagating that up caused lustre
>> to get terribly confused - not too surprising really.
>
> Right, so this failure mode is specific to your patch 6.
I disagree. My patch 6 only makes it common instead of exceedingly
rare. If any table in the list other than the first has a chain with 16
elements, then trying to insert an element with a hash which matches
that chain will fail with -EBUSY. This is theoretically possible
already, though astronomically unlikely. So that case will never be
tested for.
>
> I think I see the problem. As it currently stands, we never
> need growing when we hit the bucket length limit. We only do
> rehashes.
>
> With your patch, you must change it so that we actually try to
> grow the table if necessary when we hit the bucket length limit.
It is hard to know if it is necessary. And making the new table larger
will make the error less likely, but still won't make it impossible. So
callers will have to handle it - just like they currently have to handle
-ENOMEM even though it is highly unlikely (and not strictly necessary).
Are these errors ever actually useful? I thought I had convinced myself
before that they were (to throttle attacks on the hash function), but
they happen even less often than I thought.
>
> Otherwise it will result in the EBUSY that you're seeing.
>
> I laso think that we shouldn't make this a toggle. If we're going
> to do disable_count then it should be unconditionally done for
> everyone.
>
> However, I personally would prefer a percpu elem count instead of
> disabling it completely. Would that be acceptable to your use-case?
Maybe. Reading a percpu counter isn't cheap. Reading it whenever a hash
chain reaches 16 is reasonable, but I think we would want to read it a
lot more often than that. So probably store the last-sampled time (with
no locking) and only sample the counter if last-sampled is more than
jiffies - 10*HZ (???)
In the case in lustre we also shard the LRU list so that adding to the
LRU causes minimal contention. Keeping a shared counter together with
the lru is trivial and summing them periodically is little burden.
Maybe it makes sense to include that functionality if rhashtables so
that it is there for everyone.
A percpu counter uses a lot more memory than atomic_t. Given that some
callers set nelem_hint to 2 or 3, it seem likely that those callers
don't want to waste memory. Should we force them to?
Thanks,
NeilBrown
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply
* Re: [PATCH] net: fec: set dma_coherent_mask
From: Geert Uytterhoeven @ 2018-03-28 7:06 UTC (permalink / raw)
To: Andy Duan; +Cc: Greg Ungerer, netdev, Linux/m68k
In-Reply-To: <AM4PR0401MB2260558919012AC2D190DCD6FFA30@AM4PR0401MB2260.eurprd04.prod.outlook.com>
Hi Andy,
On Wed, Mar 28, 2018 at 5:04 AM, Andy Duan <fugang.duan@nxp.com> wrote:
> From: Geert Uytterhoeven <geert@linux-m68k.org> Sent: 2018年3月27日 20:59
>> On Mon, Mar 26, 2018 at 3:36 PM, Greg Ungerer <gerg@linux-m68k.org> wrote:
>> > As of commit 205e1b7f51e4 ("dma-mapping: warn when there is no
>> > coherent_dma_mask") the Freescale FEC driver is issuing the following
>> > warning on driver initialization on ColdFire systems:
>> >
>> > WARNING: CPU: 0 PID: 1 at ./include/linux/dma-mapping.h:516 0x40159e20
>> > Modules linked in:
>> > CPU: 0 PID: 1 Comm: swapper Not tainted 4.16.0-rc7-dirty #4 Stack from
>> > 41833dd8:
>> > 41833dd8 40259c53 40025534 40279e26 00000003 00000000
>> 4004e514 41827000
>> > 400255de 40244e42 00000204 40159e20 00000009 00000000
>> 00000000 4024531d
>> > 40159e20 40244e42 00000204 00000000 00000000 00000000
>> 00000007 00000000
>> > 00000000 40279e26 4028d040 40226576 4003ae88 40279e26
>> 418273f6 41833ef8
>> > 7fffffff 418273f2 41867028 4003c9a2 4180ac6c 00000004 41833f8c
>> 4013e71c
>> > 40279e1c 40279e26 40226c16 4013ced2 40279e26 40279e58
>> 4028d040
>> > 00000000 Call Trace:
>> > [<40025534>] 0x40025534
>> > [<4004e514>] 0x4004e514
>> > [<400255de>] 0x400255de
>> > [<40159e20>] 0x40159e20
>> > [<40159e20>] 0x40159e20
>> >
>> > It is not fatal, the driver and the system continue to function normally.
>> >
>> > As per the warning the coherent_dma_mask is not set on this device.
>> > There is nothing special about the DMA memory coherency on this
>> > hardware so we can just set the mask to 32bits during probe.
>> >
>> > Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
>>
>> Thanks for your patch!
>>
>> > ---
>> > drivers/net/ethernet/freescale/fec_main.c | 2 ++
>> > 1 file changed, 2 insertions(+)
>> >
>> > Is this the best way to handle this problem?
>> > Comments welcome...
>> >
>> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
>> > b/drivers/net/ethernet/freescale/fec_main.c
>> > index d4604bc..3cb130a 100644
>> > --- a/drivers/net/ethernet/freescale/fec_main.c
>> > +++ b/drivers/net/ethernet/freescale/fec_main.c
>> > @@ -2702,6 +2702,8 @@ static int fec_enet_alloc_queue(struct net_device
>> *ndev)
>> > int ret = 0;
>> > struct fec_enet_priv_tx_q *txq;
>> >
>> > + dma_set_coherent_mask(&fep->pdev->dev, DMA_BIT_MASK(32));
>> > +
>> > for (i = 0; i < fep->num_tx_queues; i++) {
>> > txq = kzalloc(sizeof(*txq), GFP_KERNEL);
>> > if (!txq) {
>>
>> As per your other email, this does not trigger on iMX systems using DT.
>> Hence I'm wondering if the Coldfire platform code shouldn't just do the same what
>> drivers/of/device.c does, cfr.
>> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwww.s
>> pinics.net%2Flists%2Flinux-m68k%2Fmsg10929.html&data=02%7C01%7Cfugan
>> g.duan%40nxp.com%7C3db2dbadf1154965370608d593e294b2%7C686ea1d3
>> bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636577523756185995&sdata=bsO
>> F%2BnSeXTP7C%2F55JLO3Pv8YybPahMaZDyqB16f9ySA%3D&reserved=0?
>>
>> Gr{oetje,eeting}s,
>>
>> Geert
>>
> It is better to use of_dma_configure(&pdev->dev, np), it iommu support, device also can allocate memory from 64bit space.
At first sight, it indeed looks like of_dma_configure() would do the
right thing,
even on platforms without DT like Coldfire.
Unfortunately of_dma_configure() is replaced by a dummy if !CONFIG_OF.
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: RFC on writel and writel_relaxed
From: Arnd Bergmann @ 2018-03-28 7:11 UTC (permalink / raw)
To: Benjamin Herrenschmidt
Cc: Linus Torvalds, Alexander Duyck, Will Deacon, Sinan Kaya,
Jason Gunthorpe, David Laight, Oliver,
open list:LINUX FOR POWERPC (32-BIT AND 64-BIT),
linux-rdma@vger.kernel.org, Paul E. McKenney,
netdev@vger.kernel.org
In-Reply-To: <1522220165.7364.110.camel@kernel.crashing.org>
On Wed, Mar 28, 2018 at 8:56 AM, Benjamin Herrenschmidt
<benh@kernel.crashing.org> wrote:
> On Wed, 2018-03-28 at 06:53 +0000, Linus Torvalds wrote:
>> On Tue, Mar 27, 2018, 20:43 Benjamin Herrenschmidt <benh@kernel.crashing.org> wrote:
>> That's why in/out were *so* slow, and why nobody uses them any more
>> (well, the address size limitations and the lack of any remapping of
>> the address obviously also are a reason).
>
> All true indeed, though a lot of other archs never quite made them
> fully synchronous, which was another can of worms ... oh well.
Many architectures have no way of providing PCI compliant semantics
for outb, as their instruction set and/or bus interconnect lacks a
method of waiting for completion of an outb.
In practice, it doesn't seem to matter for any of the devices one would
encounter these days: very few use I/O space, and those that do don't
actually rely on the strict ordering. Some architectures (in particular
s390, but I remember seeing the same thing elsewhere) explicitly
disallow I/O space access on PCI because of this. On ARM, the typical
PCI implementations have other problems that are worse than this
one, so most drivers are fine with the almost-working semantics.
Arnd
^ permalink raw reply
* RE: [PATCH] net: fec: set dma_coherent_mask
From: Andy Duan @ 2018-03-28 7:15 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: Greg Ungerer, netdev, Linux/m68k
In-Reply-To: <CAMuHMdWSKxRdWXMOK_iqFShYKXphEUvKRMsoPP5xQAdi1x5NiQ@mail.gmail.com>
From: Geert Uytterhoeven <geert@linux-m68k.org> Sent: 2018年3月28日 15:07
> Hi Andy,
>
> On Wed, Mar 28, 2018 at 5:04 AM, Andy Duan <fugang.duan@nxp.com> wrote:
> > From: Geert Uytterhoeven <geert@linux-m68k.org> Sent: 2018年3月27日
> 20:59
> >> On Mon, Mar 26, 2018 at 3:36 PM, Greg Ungerer <gerg@linux-m68k.org>
> wrote:
> >> > As of commit 205e1b7f51e4 ("dma-mapping: warn when there is no
> >> > coherent_dma_mask") the Freescale FEC driver is issuing the
> >> > following warning on driver initialization on ColdFire systems:
> >> >
> >> > WARNING: CPU: 0 PID: 1 at ./include/linux/dma-mapping.h:516
> >> > 0x40159e20 Modules linked in:
> >> > CPU: 0 PID: 1 Comm: swapper Not tainted 4.16.0-rc7-dirty #4 Stack
> >> > from
> >> > 41833dd8:
> >> > 41833dd8 40259c53 40025534 40279e26 00000003 00000000
> >> 4004e514 41827000
> >> > 400255de 40244e42 00000204 40159e20 00000009 00000000
> >> 00000000 4024531d
> >> > 40159e20 40244e42 00000204 00000000 00000000 00000000
> >> 00000007 00000000
> >> > 00000000 40279e26 4028d040 40226576 4003ae88 40279e26
> >> 418273f6 41833ef8
> >> > 7fffffff 418273f2 41867028 4003c9a2 4180ac6c 00000004
> >> > 41833f8c
> >> 4013e71c
> >> > 40279e1c 40279e26 40226c16 4013ced2 40279e26 40279e58
> >> 4028d040
> >> > 00000000 Call Trace:
> >> > [<40025534>] 0x40025534
> >> > [<4004e514>] 0x4004e514
> >> > [<400255de>] 0x400255de
> >> > [<40159e20>] 0x40159e20
> >> > [<40159e20>] 0x40159e20
> >> >
> >> > It is not fatal, the driver and the system continue to function normally.
> >> >
> >> > As per the warning the coherent_dma_mask is not set on this device.
> >> > There is nothing special about the DMA memory coherency on this
> >> > hardware so we can just set the mask to 32bits during probe.
> >> >
> >> > Signed-off-by: Greg Ungerer <gerg@linux-m68k.org>
> >>
> >> Thanks for your patch!
> >>
> >> > ---
> >> > drivers/net/ethernet/freescale/fec_main.c | 2 ++
> >> > 1 file changed, 2 insertions(+)
> >> >
> >> > Is this the best way to handle this problem?
> >> > Comments welcome...
> >> >
> >> > diff --git a/drivers/net/ethernet/freescale/fec_main.c
> >> > b/drivers/net/ethernet/freescale/fec_main.c
> >> > index d4604bc..3cb130a 100644
> >> > --- a/drivers/net/ethernet/freescale/fec_main.c
> >> > +++ b/drivers/net/ethernet/freescale/fec_main.c
> >> > @@ -2702,6 +2702,8 @@ static int fec_enet_alloc_queue(struct
> >> > net_device
> >> *ndev)
> >> > int ret = 0;
> >> > struct fec_enet_priv_tx_q *txq;
> >> >
> >> > + dma_set_coherent_mask(&fep->pdev->dev, DMA_BIT_MASK(32));
> >> > +
> >> > for (i = 0; i < fep->num_tx_queues; i++) {
> >> > txq = kzalloc(sizeof(*txq), GFP_KERNEL);
> >> > if (!txq) {
> >>
> >> As per your other email, this does not trigger on iMX systems using DT.
> >> Hence I'm wondering if the Coldfire platform code shouldn't just do
> >> the same what drivers/of/device.c does, cfr.
> >> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> >> w.s
> >>
> pinics.net%2Flists%2Flinux-m68k%2Fmsg10929.html&data=02%7C01%7Cfugan
> >>
> g.duan%40nxp.com%7C3db2dbadf1154965370608d593e294b2%7C686ea1d3
> >>
> bc2b4c6fa92cd99c5c301635%7C0%7C0%7C636577523756185995&sdata=bsO
> >> F%2BnSeXTP7C%2F55JLO3Pv8YybPahMaZDyqB16f9ySA%3D&reserved=0?
> >>
> >> Gr{oetje,eeting}s,
> >>
> >> Geert
> >>
> > It is better to use of_dma_configure(&pdev->dev, np), it iommu support, device
> also can allocate memory from 64bit space.
>
> At first sight, it indeed looks like of_dma_configure() would do the right thing,
> even on platforms without DT like Coldfire.
>
> Unfortunately of_dma_configure() is replaced by a dummy if !CONFIG_OF.
>
> Gr{oetje,eeting}s,
>
> Geert
Yes, it only supports DT.
Andy
^ permalink raw reply
* Re: [PATCH 4/6] rhashtable: allow a walk of the hash table without missing objects.
From: NeilBrown @ 2018-03-28 7:17 UTC (permalink / raw)
To: Herbert Xu; +Cc: Thomas Graf, netdev, linux-kernel
In-Reply-To: <20180328060734.GB16291@gondor.apana.org.au>
[-- Attachment #1: Type: text/plain, Size: 2246 bytes --]
On Wed, Mar 28 2018, Herbert Xu wrote:
> On Wed, Mar 28, 2018 at 08:54:41AM +1100, NeilBrown wrote:
>>
>> Possibly.
>> I particularly want the interface to require that you pass the
>> previously returned object to _continue. That makes it easy to see that
>> the object is still being used. If someone changes to code to delete
>> the object before the _continue, there should be a strong hint that it
>> won't work.
>>
>> Maybe it would be better to make it a WARN_ON()
>>
>> if (!obj || WARN_ON(iter->p != obj))
>> iter->p = NULL;
>
> This doesn't really protect against the case where obj is removed.
> All it proves is that the user saved a copy of obj which we already
> did anyway.
True. We ultimately need to trust the user not to do something silly.
We can do little things to help catch obvious errors though. That was
my intent with the WARN_ON.
>
> To detect an actual removal you'd need to traverse the list.
Yes. You could reset ->skip to zero and search for the given object.
That feels a bit like excess hand-holding to me, but probably wouldn't
be too expensive. It only happens when you need to drop out of RCU, and
in that case you are probably already doing something expensive.
>
> I have another idea: we could save insert the walkers into the
> hash table chain at the end, essentially as a hidden list. We
> can mark it with a flag like rht_marker so that normal traversal
> doesn't see it.
>
> That way the removal code can simply traverse that list and inform
> them that the iterator is invalid.
Sounds like over-kill to me.
It might be reasonable to have a CONFIG_DEBUG_RHASHTABLE which enables
extra to code to catch misuse, but I don't see the justification for
always performing these checks.
The DEBUG code could just scan the chain (usually quite short) to see if
the given element is present. Of course it might have already been
rehashed to the next table, so you would to allow for that possibility -
probably check tbl->rehash.
Thanks,
NeilBrown
>
> Cheers,
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page: http://gondor.apana.org.au/~herbert/
> PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ 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