* Re: [PATCH] net:appletalk:ddp:fixed coding style issue again relating to
From: David Miller @ 2012-08-07 21:46 UTC (permalink / raw)
To: ahiliation; +Cc: acme, bhutchings, netdev, linux-kernel
In-Reply-To: <1344348020-4966-1-git-send-email-ahiliation@yahoo.co.in>
From: Jeffrin Jose <ahiliation@yahoo.co.in>
Date: Tue, 7 Aug 2012 19:30:20 +0530
> Fixed coding style issue relating to indentation in
> net/appletalk/ddp.c found by checkpatch.pl tool
>
> Signed-off-by: Jeffrin Jose <ahiliation@yahoo.co.in>
Please just stop trying to fix this code up, it's noise and you
keep making mistakes.
> case TIOCOUTQ: {
> - long amount = sk->sk_sndbuf - sk_wmem_alloc_get(sk);
> + long amount = sk->sk_sndbuf - sk_wmem_alloc_get(sk);
>
> - if (amount < 0)
> - amount = 0;
> - rc = put_user(amount, (int __user *)argp);
> - break;
> + if (amount < 0)
> + amount = 0;
> + rc = put_user(amount, (int __user *)argp);
> + break;
> }
This time you didn't adjust the closing brace, as needed.
I'm not applying this and I seriously discourage you from pursing this
further, it's better you spend your time learning how the kernel works
rather than suffering through this coding style cleanup business.
Thanks.
^ permalink raw reply
* Re: [PATCH] net: force dst_default_metrics to const section
From: David Miller @ 2012-08-07 21:55 UTC (permalink / raw)
To: bhutchings; +Cc: eric.dumazet, netdev
In-Reply-To: <1344370527.2688.61.camel@bwh-desktop.uk.solarflarecom.com>
From: Ben Hutchings <bhutchings@solarflare.com>
Date: Tue, 7 Aug 2012 21:15:27 +0100
> Some day the compiler may be smart enough to ignore the different
> between explicit and implicit zero-initialisation, and put it back in
> BSS. Declaring this __cache_aligned_in_smp might be a better option.
I'm surprised it doesn't already do this.
It definitely puts scalar explicit zero initializers into the BSS.
^ permalink raw reply
* [PATCH] cdc-phonet: Don't leak in usbpn_open
From: Jesper Juhl @ 2012-08-07 21:56 UTC (permalink / raw)
To: linux-kernel
Cc: netdev, linux-usb, Greg Kroah-Hartman, Rémi Denis-Courmont,
Remi Denis-Courmont
We allocate memory for 'req' with usb_alloc_urb() and then test
'if (!req || rx_submit(pnd, req, GFP_KERNEL | __GFP_COLD))'.
If we enter that branch due to '!req' then there is no problem. But if
we enter the branch due to 'req' being != 0 and the 'rx_submit()' call
being false, then we'll leak the memory we allocated.
Deal with the leak by always calling 'usb_free_urb(req)' when entering
the branch. If 'req' happens to be 0 then the call is harmless, if it
is not 0 then we free the memory we allocated but don't need.
Signed-off-by: Jesper Juhl <jj@chaosbits.net>
---
drivers/net/usb/cdc-phonet.c | 1 +
1 file changed, 1 insertion(+)
Only compile tested due to lack of hardware.
diff --git a/drivers/net/usb/cdc-phonet.c b/drivers/net/usb/cdc-phonet.c
index 6461004..7d78669 100644
--- a/drivers/net/usb/cdc-phonet.c
+++ b/drivers/net/usb/cdc-phonet.c
@@ -232,6 +232,7 @@ static int usbpn_open(struct net_device *dev)
struct urb *req = usb_alloc_urb(0, GFP_KERNEL);
if (!req || rx_submit(pnd, req, GFP_KERNEL | __GFP_COLD)) {
+ usb_free_urb(req);
usbpn_close(dev);
return -ENOMEM;
}
--
1.7.11.4
--
Jesper Juhl <jj@chaosbits.net> http://www.chaosbits.net/
Don't top-post http://www.catb.org/jargon/html/T/top-post.html
Plain text mails only, please.
^ permalink raw reply related
* Re: [PATCH] net-tcp: TCP/IP stack bypass for loopback connections
From: Eric Dumazet @ 2012-08-07 21:58 UTC (permalink / raw)
To: Bruce "Brutus" Curtis; +Cc: David S. Miller, Eric Dumazet, netdev
In-Reply-To: <1344373683-22945-1-git-send-email-brutus@google.com>
On Tue, 2012-08-07 at 14:08 -0700, Bruce "Brutus" Curtis wrote:
> From: "Bruce \"Brutus\" Curtis" <brutus@google.com>
>
> TCP/IP loopback socket pair stack bypass, based on an idea by, and
> rough upstream patch from, David Miller <davem@davemloft.net> called
> "friends", the data structure modifcations and connection scheme are
> reused with extensive data-path changes.
>
> A new sysctl, net.ipv4.tcp_friends, is added:
> 0: disable friends and use the stock data path.
> 1: enable friends and bypass the stack data path, the default.
>
> Note, when friends is enabled any loopback interpose, e.g. tcpdump,
> will only see the TCP/IP packets during connection establishment and
> finish, all data bypasses the stack and instead is delivered to the
> destination socket directly.
>
> Testing done on a 4 socket 2.2GHz "Quad-Core AMD Opteron(tm) Processor
> 8354 CPU" based system, netperf results for a single connection show
> increased TCP_STREAM throughput, increased TCP_RR and TCP_CRR transaction
> rate for most message sizes vs baseline and comparable to AF_UNIX.
>
> Significant increase (up to 5x) in aggregate throughput for multiple
> netperf runs (STREAM 32KB I/O x N) is seen.
>
> Some results:
>
> Default netperf: netperf
> netperf -t STREAM_STREAM
> netperf -t STREAM_STREAM -- -s 51882 -m 16384 -M 87380
> netperf
>
> Baseline AF_UNIX AF_UNIX Friends
> Mbits/S Mbits/S Mbits/S Mbits/S
> 6860 714 8% 9444 138% 1323% 10576 154% 1481% 112%
>
> Note, for the AF_UNIX (STREAM_STREAM) test 2 results are listed, 1st
> with no options but as the defaults for AF_UNIX sockets are much lower
> performaning a 2nd set of runs with a socket buffer size and send/recv
> buffer sizes equivalent to AF_INET (TCP_STREAM) are done.
>
> Note, all subsequent AF_UNIX (STREAM_STREAM, STREAM_RR) tests are done
> with "-s 51882" such that the same total effective socket buffering is
> used as for the AF_INET runs defaults (16384+NNNNN/2).
>
> STREAM 32KB I/O x N: netperf -l 100 -t TCP_STREAM -- -m 32K -M 32K
> netperf -l 100 -t STREAM_STREAM -- -s 51882 -m 32K -M 32K
> netperf -l 100 -t TCP_STREAM -- -m 32K -M 32K
>
> Baseline AF_UNIX Friends
> N COC Mbits/S Mbits/S Mbits/S
> 1 - 8616 9416 109% 11116 129% 118%
> 2 - 15419 17076 111% 20267 131% 119%
> 16 2 59497 303029 509% 347349 584% 115%
> 32 4 54223 273637 505% 272891 503% 100%
> 256 32 58244 85476 147% 273696 470% 320%
> 512 64 58745 87402 149% 260837 444% 298%
> 1600 200 83161 158915 191% 383947 462% 242%
>
> COC = Cpu Over Commit ratio (16 core platform)
>
> STREAM: netperf -l 100 -t TCP_STREAM
> netperf -l 100 -t STREAM_STREAM -- -s 51882
> netperf -l 100 -t TCP_STREAM
>
> netperf Baseline AF_UNIX Friends
> -m/-M N Mbits/S Mbits/S Mbits/S
> 64 1020 445 44% 515 50% 116%
> 1K 4881 4340 89% 5070 104% 117%
> 8K 5933 8387 141% 9770 165% 116%
> 32K 8168 9538 117% 11067 135% 116%
> 64K 9116 9774 107% 11515 126% 118%
> 128K 9053 10044 111% 13082 145% 130%
> 256K 9642 10351 107% 13470 140% 130%
> 512K 10050 10142 101% 13327 133% 131%
> 1M 8640 9843 114% 12201 141% 124%
> 16M 7179 9619 134% 11316 158% 118%
>
> RR: netperf -l 100 -t TCP_RR
> netperf -l 100 -t STREAM_RR -- -s 51882 -m 16384 -M 87380
> netperf -l 100 -t TCP_RR
>
> netperf Baseline AF_UNIX Friends
> -r N,N Trans./S Trans./S Trans./S
> 64 47913 99681 208% 98225 205% 99%
> 1K 44045 92327 210% 91608 208% 99%
> 8K 26732 33201 124% 33025 124% 99%
> 32K 10903 11972 110% 13574 124% 113%
> 64K 7113 6718 94% 7176 101% 107%
> 128K 4191 3431 82% 3695 88% 108%
> 256K 2324 1937 83% 2147 92% 111%
> 512K 958 1056 110% 1202 125% 114%
> 1M 404 508 126% 497 123% 98%
> 16M 26.1 34.1 131% 32.9 126% 96%
>
> CRR: netperf -l 100 -t TCP_CRR
> netperf -l 100 -t TCP_CRR
>
> netperf Baseline AF_UNIX Friends
> -r N Trans./S Trans./S Trans./S
> 64 14690 - 18191 124% -
> 1K 14258 - 17492 123% -
> 8K 11535 - 14012 121% -
> 32K 7035 - 8974 128% -
> 64K 4312 - 5654 131% -
> 128K 2252 - 3179 141% -
> 256K 1237 - 2008 162% -
> 512K 17.5* - 1079 ? -
> 1M 4.93* - 458 ? -
> 16M 8.29* - 32.5 ? -
>
> Note, "-" denotes test not supported for transport.
> "*" denotes test results reported without statistical confidence.
> "?" denotes results not comparable.
>
> SPLICE 32KB I/O:
>
> Source
> Sink Baseline Friends
> FSFS Mbits/S Mbits/S
> ---- 8042 10810 134%
> Z--- 7071 9773 138%
> --N- 8039 10820 135%
> Z-N- 7902 9796 124%
> -S-- 17326 37496 216%
> ZS-- 9008 9573 106%
> -SN- 16154 36269 225%
> ZSN- 9531 9640 101%
> ---S 8466 9933 117%
> Z--S 8000 9453 118%
> --NS 12783 11379 89%
> Z-NS 11055 9489 86%
> -S-S 12741 24380 191%
> ZS-S 8097 10242 126%
> -SNS 16657 30954 186%
> ZSNS 12108 12763 105%
>
> Note, "Z" source File /dev/zero, "-" source user memory
> "N" sink File /dev/null, "-" sink user memory
> "S" Splice on, "-" Splice off
>
> Signed-off-by: Bruce \"Brutus\" Curtis <brutus@google.com>
> ---
> include/linux/skbuff.h | 2 +
> include/net/request_sock.h | 1 +
> include/net/sock.h | 32 +++-
> include/net/tcp.h | 3 +-
> net/core/skbuff.c | 1 +
> net/core/sock.c | 1 +
> net/core/stream.c | 36 +++
> net/ipv4/inet_connection_sock.c | 20 ++
> net/ipv4/sysctl_net_ipv4.c | 7 +
> net/ipv4/tcp.c | 500 ++++++++++++++++++++++++++++++++++-----
> net/ipv4/tcp_input.c | 22 ++-
> net/ipv4/tcp_ipv4.c | 2 +
> net/ipv4/tcp_minisocks.c | 5 +
> net/ipv4/tcp_output.c | 18 ++-
> net/ipv6/tcp_ipv6.c | 1 +
> 15 files changed, 576 insertions(+), 75 deletions(-)
>
A change in Documentation is welcome (for the sysctl)
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 642cb73..2fbca93 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -332,6 +332,7 @@ typedef unsigned char *sk_buff_data_t;
> * @cb: Control buffer. Free for use by every layer. Put private vars here
> * @_skb_refdst: destination entry (with norefcount bit)
> * @sp: the security path, used for xfrm
> + * @friend: loopback friend socket
> * @len: Length of actual data
> * @data_len: Data length
> * @mac_len: Length of link layer header
> @@ -407,6 +408,7 @@ struct sk_buff {
> #ifdef CONFIG_XFRM
> struct sec_path *sp;
> #endif
> + struct sock *friend;
Is it really needed ?
Since skb wont pass other layers (qdisc, IP, ...) we probably can use
cb[] ?
> unsigned int len,
> data_len;
> __u16 mac_len,
> diff --git a/include/net/request_sock.h b/include/net/request_sock.h
> index 4c0766e..2c74420 100644
> --- a/include/net/request_sock.h
> +++ b/include/net/request_sock.h
> @@ -63,6 +63,7 @@ struct request_sock {
> unsigned long expires;
> const struct request_sock_ops *rsk_ops;
> struct sock *sk;
> + struct sock *friend;
> u32 secid;
> u32 peer_secid;
> };
> diff --git a/include/net/sock.h b/include/net/sock.h
> index dcb54a0..3b371f5 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -197,6 +197,7 @@ struct cg_proto;
> * @sk_userlocks: %SO_SNDBUF and %SO_RCVBUF settings
> * @sk_lock: synchronizer
> * @sk_rcvbuf: size of receive buffer in bytes
> + * @sk_friend: loopback friend socket
> * @sk_wq: sock wait queue and async head
> * @sk_rx_dst: receive input route used by early tcp demux
> * @sk_dst_cache: destination cache
> @@ -286,6 +287,14 @@ struct sock {
> socket_lock_t sk_lock;
> struct sk_buff_head sk_receive_queue;
> /*
> + * If socket has a friend (sk_friend != NULL) then a send skb is
> + * enqueued directly to the friend's sk_receive_queue such that:
> + *
> + * sk_sndbuf -> sk_sndbuf + sk_friend->sk_rcvbuf
> + * sk_wmem_queued -> sk_friend->sk_rmem_alloc
> + */
> + struct sock *sk_friend;
> + /*
> * The backlog queue is special, it is always used with
> * the per-socket spinlock held and requires low latency
> * access. Therefore we special case it's implementation.
> @@ -673,24 +682,40 @@ static inline bool sk_acceptq_is_full(const struct sock *sk)
> return sk->sk_ack_backlog > sk->sk_max_ack_backlog;
> }
>
> +static inline int sk_wmem_queued_get(const struct sock *sk)
> +{
> + if (sk->sk_friend)
I try to convince myself sk->sk_friend cannot be changed to NULL after
this test, (by another cpu)
> + return atomic_read(&sk->sk_friend->sk_rmem_alloc);
> + else
> + return sk->sk_wmem_queued;
> +}
> +
> +static inline int sk_sndbuf_get(const struct sock *sk)
> +{
> + if (sk->sk_friend)
> + return sk->sk_sndbuf + sk->sk_friend->sk_rcvbuf;
> + else
> + return sk->sk_sndbuf;
> +}
> +
> /
Patch doesnt apply on net-next, so its a bit hard to review it
properly ;)
Thanks
^ permalink raw reply
* Re: [PATCH] net: force dst_default_metrics to const section
From: Eric Dumazet @ 2012-08-07 22:09 UTC (permalink / raw)
To: David Miller; +Cc: bhutchings, netdev
In-Reply-To: <20120807.145533.2161378343227639185.davem@davemloft.net>
On Tue, 2012-08-07 at 14:55 -0700, David Miller wrote:
> From: Ben Hutchings <bhutchings@solarflare.com>
> Date: Tue, 7 Aug 2012 21:15:27 +0100
>
> > Some day the compiler may be smart enough to ignore the different
> > between explicit and implicit zero-initialisation, and put it back in
> > BSS. Declaring this __cache_aligned_in_smp might be a better option.
>
> I'm surprised it doesn't already do this.
>
> It definitely puts scalar explicit zero initializers into the BSS.
Not a const :
# cat try.c
const int scalar_value = 0;
const int scalar_value_bss;
int scalar_value = 0;
int scalar_value_bss;
main(int argc, char *argv[])
{
return 0;
}
# gcc -o try try.c && nm -v try|grep scalar_value
00000000004005cc R cscalar_value
0000000000601028 B scalar_value
000000000060102c B scalar_value_bss
0000000000601030 B cscalar_value_bss
gcc 4.6.3
^ permalink raw reply
* Re: [PATCH] net: force dst_default_metrics to const section
From: Eric Dumazet @ 2012-08-07 22:11 UTC (permalink / raw)
To: David Miller; +Cc: bhutchings, netdev
In-Reply-To: <1344377399.28967.198.camel@edumazet-glaptop>
On Wed, 2012-08-08 at 00:10 +0200, Eric Dumazet wrote:
> # gcc -o try try.c && nm -v try|grep scalar_value
> 00000000004005cc R cscalar_value
> 0000000000601028 B scalar_value
> 000000000060102c B scalar_value_bss
> 0000000000601030 B cscalar_value_bss
>
Sorry the source was :
const int cscalar_value = 0;
const int cscalar_value_bss;
int scalar_value = 0;
int scalar_value_bss;
main(int argc, char *argv[])
{
return 0;
}
^ permalink raw reply
* Re: [PATCH] net: force dst_default_metrics to const section
From: Ben Hutchings @ 2012-08-07 22:12 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
In-Reply-To: <1344372945.28967.165.camel@edumazet-glaptop>
On Tue, 2012-08-07 at 22:55 +0200, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
>
>
> > Some day the compiler may be smart enough to ignore the different
> > between explicit and implicit zero-initialisation, and put it back in
> > BSS. Declaring this __cache_aligned_in_smp might be a better option.
>
> __cache_aligned_in_smp aligns start of the structure, but can be
> followed by another var in same cache line. Yes, this is bad.
Oh, that's unexpected.
> By the way we dont care of cache alignment on this structure, only it
> should be const. Its a soft requirement, machine wont crash if it is not
> the case.
Right.
> If compiler is smart one day as you say (it should first be non buggy
> IMHO), then we can add a non zero field like this :
[...]
That would work, but it's ugly! How about defining and using a
meaningfully-named macro that expands to __section(.rodata)?
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] net: force dst_default_metrics to const section
From: Eric Dumazet @ 2012-08-07 22:34 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, netdev
In-Reply-To: <1344377561.2688.66.camel@bwh-desktop.uk.solarflarecom.com>
On Tue, 2012-08-07 at 23:12 +0100, Ben Hutchings wrote:
> On Tue, 2012-08-07 at 22:55 +0200, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> >
> >
> > > Some day the compiler may be smart enough to ignore the different
> > > between explicit and implicit zero-initialisation, and put it back in
> > > BSS. Declaring this __cache_aligned_in_smp might be a better option.
> >
> > __cache_aligned_in_smp aligns start of the structure, but can be
> > followed by another var in same cache line. Yes, this is bad.
>
> Oh, that's unexpected.
>
> > By the way we dont care of cache alignment on this structure, only it
> > should be const. Its a soft requirement, machine wont crash if it is not
> > the case.
>
> Right.
>
> > If compiler is smart one day as you say (it should first be non buggy
> > IMHO), then we can add a non zero field like this :
> [...]
>
> That would work, but it's ugly! How about defining and using a
> meaningfully-named macro that expands to __section(.rodata)?
You are kidding. I prefer plain C and not having to mess with all
arches.
^ permalink raw reply
* Re: [PATCH] net: force dst_default_metrics to const section
From: Ben Hutchings @ 2012-08-07 22:44 UTC (permalink / raw)
To: Eric Dumazet; +Cc: David Miller, netdev
In-Reply-To: <1344378854.28967.201.camel@edumazet-glaptop>
On Wed, 2012-08-08 at 00:34 +0200, Eric Dumazet wrote:
> On Tue, 2012-08-07 at 23:12 +0100, Ben Hutchings wrote:
> > On Tue, 2012-08-07 at 22:55 +0200, Eric Dumazet wrote:
> > > From: Eric Dumazet <edumazet@google.com>
> > >
> > >
> > > > Some day the compiler may be smart enough to ignore the different
> > > > between explicit and implicit zero-initialisation, and put it back in
> > > > BSS. Declaring this __cache_aligned_in_smp might be a better option.
> > >
> > > __cache_aligned_in_smp aligns start of the structure, but can be
> > > followed by another var in same cache line. Yes, this is bad.
> >
> > Oh, that's unexpected.
> >
> > > By the way we dont care of cache alignment on this structure, only it
> > > should be const. Its a soft requirement, machine wont crash if it is not
> > > the case.
> >
> > Right.
> >
> > > If compiler is smart one day as you say (it should first be non buggy
> > > IMHO), then we can add a non zero field like this :
> > [...]
> >
> > That would work, but it's ugly! How about defining and using a
> > meaningfully-named macro that expands to __section(.rodata)?
>
> You are kidding. I prefer plain C and not having to mess with all
> arches.
Any consideration of implementation details like BSS and cache line
sharing is already outside of 'plain C'. And you don't have to 'mess
with all arches'; just look at what <linux/init.h> and <linux/module.h>
do.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH] net: force dst_default_metrics to const section
From: Eric Dumazet @ 2012-08-07 22:56 UTC (permalink / raw)
To: Ben Hutchings; +Cc: David Miller, netdev
In-Reply-To: <1344379474.2688.89.camel@bwh-desktop.uk.solarflarecom.com>
On Tue, 2012-08-07 at 23:44 +0100, Ben Hutchings wrote:
> Any consideration of implementation details like BSS and cache line
> sharing is already outside of 'plain C'. And you don't have to 'mess
> with all arches'; just look at what <linux/init.h> and <linux/module.h>
> do.
"const" is the clean and portable way to express my needs. No magic
section.
All is self-contained in the definition of the metrics, with a nice
comment.
All const are naturally shared by all cpus, without adding extra cache
line boundaries.
Please feel free to send another version, but I wont spend more time
myself.
Thanks
^ permalink raw reply
* Re: [PATCH] net: force dst_default_metrics to const section
From: David Miller @ 2012-08-07 23:17 UTC (permalink / raw)
To: eric.dumazet; +Cc: bhutchings, netdev
In-Reply-To: <1344380200.28967.207.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Wed, 08 Aug 2012 00:56:40 +0200
> On Tue, 2012-08-07 at 23:44 +0100, Ben Hutchings wrote:
>
>> Any consideration of implementation details like BSS and cache line
>> sharing is already outside of 'plain C'. And you don't have to 'mess
>> with all arches'; just look at what <linux/init.h> and <linux/module.h>
>> do.
>
> "const" is the clean and portable way to express my needs. No magic
> section.
>
> All is self-contained in the definition of the metrics, with a nice
> comment.
>
> All const are naturally shared by all cpus, without adding extra cache
> line boundaries.
I agree with Eric and will apply his patch.
^ permalink raw reply
* Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: David Miller @ 2012-08-07 23:21 UTC (permalink / raw)
To: Priyanka.Jain; +Cc: netdev
In-Reply-To: <1344316904-2544-1-git-send-email-Priyanka.Jain@freescale.com>
From: Priyanka Jain <Priyanka.Jain@freescale.com>
Date: Tue, 7 Aug 2012 10:51:44 +0530
> xfrm_policy_afinfo is read mosly data structure.
> Write on xfrm_policy_afinfo is done only at the
> time of configuration.
> So rwlocks can be safely replaced with RCU.
>
> RCUs usage optimizes the performance.
>
> Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
This patch doesn't apply to the net-next tree, please respin.
Also:
> - xfrm_policy_afinfo[afinfo->family] = NULL;
> + rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family],
> + NULL);
Indent that NULL argument properly, it must line up with the first
column after the openning '(' on the previous line.
^ permalink raw reply
* Re: [PATCH 0/2] Add device tree support for davinci_mdio driver and fix cpsw DT binding documentation
From: David Miller @ 2012-08-07 23:25 UTC (permalink / raw)
To: mugunthanvnm; +Cc: netdev, devicetree-discuss, linux-arm-kernel, linux-omap
In-Reply-To: <1344265558-5724-1-git-send-email-mugunthanvnm@ti.com>
From: Mugunthan V N <mugunthanvnm@ti.com>
Date: Mon, 6 Aug 2012 20:35:56 +0530
> This patch set adds support for device tree for Davinci MDIO driver and fixes
> CPSW DT binding documentation to make it copy pastable to dts file.
>
> This patch set is tested with the following git tree on AM335X Beagle Bone
> https://github.com/hvaibhav/am335x-linux/tree/am335x-upstream-staging-cpsw
>
> Mugunthan V N (2):
> drivers: net: ethernet: davince_mdio: device tree implementation
> documentation: dt: bindings: cpsw: fixing the examples for directly
> using it in dts file
Applied to net-next, thanks.
^ permalink raw reply
* Re: [PATCH net-next] net: output path optimizations
From: David Miller @ 2012-08-07 23:26 UTC (permalink / raw)
To: eric.dumazet; +Cc: netdev
In-Reply-To: <1344341996.28967.87.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 07 Aug 2012 14:19:56 +0200
> From: Eric Dumazet <edumazet@google.com>
>
> 1) Avoid dirtying neighbour's confirmed field.
>
> TCP workloads hits this cache line for each incoming ACK.
> Lets write n->confirmed only if there is a jiffie change.
>
> 2) Optimize neigh_hh_output() for the common Ethernet case, were
> hh_len is less than 16 bytes. Replace the memcpy() call
> by two inlined 64bit load/stores on x86_64.
>
> Bench results using udpflood test, with -C option (MSG_CONFIRM flag
> added to sendto(), to reproduce the n->confirmed dirtying on UDP)
>
> 24 threads doing 1.000.000 UDP sendto() on dummy device, 4 runs.
>
> before : 2.247s, 2.235s, 2.247s, 2.318s
> after : 1.884s, 1.905s, 1.891s, 1.895s
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Applied, I'd like to significantly redo how the neigh confirmation
stuff works.
I'm not at all surprised it shows up on TCP workloads.
^ permalink raw reply
* Re: [PATCH net-next] fib: use __fls() on non null argument
From: David Miller @ 2012-08-07 23:27 UTC (permalink / raw)
To: eric.dumazet; +Cc: bhutchings, netdev
In-Reply-To: <1344372347.28967.154.camel@edumazet-glaptop>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 07 Aug 2012 22:45:47 +0200
> [PATCH v2 net-next] fib: use __fls() on non null argument
>
> __fls(x) is a bit faster than fls(x), granted we know x is non null.
>
> As Ben Hutchings pointed out, fls(x) = __fls(x) + 1
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Cc: Ben Hutchings <bhutchings@solarflare.com>
Applied.
This is, btw, the most expensive part of fib_trie on sparc64 since we
really don't have a universal way to do this in a hardware instruction
and therefore we end up with the branch-heavy software implementation :-/
So if anyone can come up with a way to eliminate this fls() entirely,
you will be my hero.
^ permalink raw reply
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: David Miller @ 2012-08-07 23:29 UTC (permalink / raw)
To: ian.molton
Cc: linux-arm-kernel, andrew, thomas.petazzoni, ben.dooks, arnd,
netdev
In-Reply-To: <1344350092-24050-1-git-send-email-ian.molton@codethink.co.uk>
From: Ian Molton <ian.molton@codethink.co.uk>
Date: Tue, 7 Aug 2012 15:34:45 +0100
> Fixed all comments.
>
> * Dropped csb1724 defconfig.
> * Added patch to remove MV643XX_ETH_SHARED_NAME and MV643XX_ETH_NAME
> * Dropped un-necessary D-T irq fixup code
Who is going to take this series?
^ permalink raw reply
* Re: [PATCH v3 0/7] mv643xx.c: Add basic device tree support.
From: Matt Sealey @ 2012-08-08 0:31 UTC (permalink / raw)
To: David Miller
Cc: ian.molton, thomas.petazzoni, andrew, arnd, netdev, ben.dooks,
linux-arm-kernel
In-Reply-To: <20120807.162923.34400427265666163.davem@davemloft.net>
On Tue, Aug 7, 2012 at 6:29 PM, David Miller <davem@davemloft.net> wrote:
> From: Ian Molton <ian.molton@codethink.co.uk>
> Date: Tue, 7 Aug 2012 15:34:45 +0100
>
>> Fixed all comments.
>>
>> * Dropped csb1724 defconfig.
>> * Added patch to remove MV643XX_ETH_SHARED_NAME and MV643XX_ETH_NAME
>> * Dropped un-necessary D-T irq fixup code
>
> Who is going to take this series?
Would anyone mind too much if I *didn't* break out a Pegasos II and
test it? Our platform has a Marvell northbridge (Discovery II)
implementing this, with a Marvell PHY, and it's OpenFirmware (as in,
REAL OpenFirmware) so the device tree isn't about to change to fit new
bindings. But I'm not sure we even have one in the office that boots
anymore.. there may be users out there but they're well beyond
warranty support (early 2005 or so was the last time we sold one).
If anyone needs the original device tree entries to compare and
contrast I may be able to provide them such that any parsing and
initializing of the driver take into account this old
board/northbridge/implementation. I'm just curious if anyone cares
enough..
--
Matt Sealey <matt@genesi-usa.com>
Product Development Analyst, Genesi USA, Inc.
^ permalink raw reply
* RE: [PATCHv2 3/4] modem_shm: u8500-shm: U8500 Shared Memory Driver
From: Arun MURTHY @ 2012-08-08 3:03 UTC (permalink / raw)
To: Alan Cox
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
linux-doc@vger.kernel.org, gregkh@linuxfoundation.org,
Sjur BRENDELAND
In-Reply-To: <20120807110156.4d0c2571@pyramind.ukuu.org.uk>
> > +#define SIZE_OF_FIFO (512*1024)
> > +
> > +static u8 message_fifo[ISA_DEVICES][SIZE_OF_FIFO];
>
> Thats a huge amount of static memory that gets allocated regardless of
> whether the device is open or being used ?
Yes, but all of these will be used, in any system which includes APE-Modem
communication.
>
> > +static int major;
> > +module_param(major, int, 0);
> > +MODULE_PARM_DESC(major, "Major device number");
>
> Is this really needed still - it looks like an escaped debugging aid
Yup, will remove this:-)
>
> > +/* global fops mutex */
> > +static DEFINE_MUTEX(isa_lock);
> > +
> > +/**
> > + * shrm_get_cdev_index() - return the index mapped to l2 header
> > + * @l2_header: L2 header
> > + *
> > + * struct map_device maps the index(count) with the device L2
> header.
> > + * This function returns the index for the provided L2 header in
> case
> > + * of success else -ve value.
> > + */
> > +int shrm_get_cdev_index(u8 l2_header)
> > +{
> > + u8 cnt;
> > + for (cnt = 0; cnt < ISA_DEVICES; cnt++) {
> > + if (map_dev[cnt].l2_header == l2_header)
> > + return map_dev[cnt].idx;
> > + }
> > + return -EINVAL;
> > +}
>
> Would be good to document the locking assumptions on this lot
Sure will add a note on this locking mechanism.
>
> > + /* reset the msg queue pointers */
> > + q->size = SIZE_OF_FIFO;
> > + q->readptr = 0;
> > + q->writeptr = 0;
>
> Is there a reason for not using the existing kfifo layer for this work
> or
> does it need to handle other things kfifo cannot.
Basically it doesn't suit our protocol of having base addr, read/write
pointer, locking etc as the same set of structures and protocol will be
used on the modem side implementation.
>
> > +static u32 isa_select(struct file *filp,
> > + struct poll_table_struct *wait)
> > +{
> > + struct isadev_context *isadev = filp->private_data;
> > + struct shrm_dev *shrm = isadev->dl_queue.shrm;
> > + struct message_queue *q;
> > + u32 mask = 0;
> > + u32 m = iminor(filp->f_path.dentry->d_inode);
> > + u8 idx = shrm_get_cdev_index(m);
> > +
> > + dev_dbg(shrm->dev, "%s IN\n", __func__);
> > +
> > + if (shrm->msr_flag)
> > + return -ENODEV;
>
> This test has no associated locking. Please explain what stop sit
> becoming true during the rest of this function ?
Locking in this file has been used only for protecting the handling
of queues.
When modem is reset, due to some crash or some unknown event, in
that case, this flag will be set and we are not suppose to communicate
any further as modem is in reset state.
>
> + struct shrm_dev *shrm = isadev->dl_queue.shrm;
> > + struct message_queue *q;
> > + u32 msgsize;
> > +
> > + dev_dbg(shrm->dev, "%s IN\n", __func__);
> > +
> > + if (len <= 0)
> > + return -EFAULT;
>
> How can this occur ?
Check for error condition
> > + spin_lock_bh(&q->update_lock);
> > + if (list_empty(&q->msg_list)) {
> > + spin_unlock_bh(&q->update_lock);
> > + dev_dbg(shrm->dev, "Waiting for Data\n");
> > + if (wait_event_interruptible(q->wq_readable,
> > + atomic_read(&q->q_rp) == 1))
> > + return -ERESTARTSYS;
> > + } else
> > + spin_unlock_bh(&q->update_lock);
> > +
> > + if (shrm->msr_flag) {
> > + atomic_set(&q->q_rp, 0);
> > + return -ENODEV;
> > + }
> > +
> > + msgsize = get_size_of_new_msg(q);
> > +
> > + if (len < msgsize)
> > + return -EINVAL;
>
> What happens with two parallel reads - I don't see what prevents
> corruption if that occurs or one racing read freeing the message before
> another has finished processing it.
Two parallel reads for different L2 headers can happen, but within the
same L2 header is out of the scope. Since the client using this in
user space will not know about the message. i.e which msg is for which
client. Hence so that scenario is not considered.
>
> > +
> > + if ((q->readptr+msgsize) >= q->size) {
>
> Minor style comment - you seen to randomly switch between a+b and a + b
> styles 8)
Will correct it.
> > + size = (q->size-q->readptr);
> > + /* Copy First Part of msg */
> > + if (copy_to_user(psrc,
> > + (u8 *)(q->fifo_base+q->readptr),
> > + size)) {
> > + dev_err(shrm->dev, "copy_to_user failed\n");
> > + return -EFAULT;
> > + }
> > + psrc += size;
> > + /* Copy Second Part of msg at the top of fifo */
> > + if (copy_to_user(psrc,
> > + (u8 *)(q->fifo_base),
> > + (msgsize-size))) {
> > + dev_err(shrm->dev, "copy_to_user failed\n");
> > + return -EFAULT;
> > + }
> > + } else {
> > + if (copy_to_user(buf,
> > + (u8 *)(q->fifo_base + q->readptr),
> > + msgsize)) {
> > + dev_err(shrm->dev, "copy_to_user failed\n");
> > + return -EFAULT;
> > + }
>
> Pedantically you should return the number of bytes successfully copied
> if
> you consume data. As this code stands it's not a big deal as far as I
> can
> see since you don't remove the message in this situation. I suspect
> that
> may need to change ?
In error case, if the number of messages copied are returned, there is
no way to communicate the error and hence only that partial message
will be considered and on decoding that message, will be a flaw.
> > +/**
> > + * isa_write() - Write to shrm char device
> > + * @filp: file descriptor
> > + * @buf: user buffer pointer
> > + * @len: size of requested data transfer
> > + * @ppos: not used
> > + *
> > + * It checks if there is space available in queue, and copies the
> message
> > + * inside queue. If there is no space, it blocks until space becomes
> available.
> > + * It also schedules transfer thread to transmit the newly added
> message.
> > + */
> > +ssize_t isa_write(struct file *filp, const char __user *buf,
> > + size_t len, loff_t *ppos)
> > +{
> > + struct isadev_context *isadev = filp->private_data;
> > + struct shrm_dev *shrm = isadev->dl_queue.shrm;
> > + struct message_queue *q;
> > + void *addr = 0;
> > + int err, l2_header;
> > + int ret = 0;
> > +
> > + dev_dbg(shrm->dev, "%s IN\n", __func__);
> > +
> > + if (len <= 0 || buf == NULL)
> > + return -EFAULT;
>
> len < 0 cannot occur, buf == NULL is not an error
Error handling is for what which is not expected.
>
> > + q = &isadev->dl_queue;
> > + l2_header = shrm_get_cdev_l2header(isadev->device_id);
> > + if (l2_header < 0) {
> > + dev_err(shrm->dev, "failed to get L2 header\n");
> > + return l2_header;
> > + }
> > +
> > + switch (l2_header) {
>
> Why keep going through switches and lookup tables - can't you cache
> this
> in the isadev or in some file private data ?
This is a write request coming from user space, hence the identification
has to be done based on the write request.
>
> > + if (copy_from_user(addr, buf, len)) {
> > + dev_err(shrm->dev, "copy_from_user failed\n");
>
> This allows an user to fill the logs with crap - should be dev_dbg
>
> Also what happens on two parallel writers ?
Explained above.
>
>
> > +/**
> > + * isa_ioctl() - To handle different ioctl commands supported by
> driver.
> > + * @inode: structure is used by the kernel internally to
> represent files
> > + * @filp: file descriptor pointer
> > + * @cmd: ioctl command
> > + * @arg: input param
>
> This appears incomplete - it's just debug statements
Yes, will remove this.
>
>
> > +}
> > +/**
> > + * isa_mmap() - Maps kernel queue memory to user space.
> > + * @filp: file descriptor pointer
> > + * @vma: virtual area memory structure.
> > + *
> > + * This function maps kernel FIFO into user space. This function
> > + * shall be called twice to map both uplink and downlink buffers.
>
> Again this appears incomplete
Will work on this.
> > + dev_err(shrm->dev, "Device not opened yet\n");
> > + mutex_unlock(&isa_lock);
> > + return -ENODEV;
> > + }
> > + atomic_set(&isa_context->is_open[idx], 1);
>
> How do you know it will always be one. Also given it's within the mutex
> in all uses I can see why is it an atomic ?
>
As per our assumptions/protocol only one client per L2 header.
> > +
> > + switch (m) {
> > + case RPC_MESSAGING:
> > + dev_info(shrm->dev, "Close RPC_MESSAGING Device\n");
> > + break;
> > + case AUDIO_MESSAGING:
> > + dev_info(shrm->dev, "Close AUDIO_MESSAGING Device\n");
> > + break;
> > + case SECURITY_MESSAGING:
> > + dev_info(shrm->dev, "CLose SECURITY_MESSAGING Device\n");
> > + break;
> > + case COMMON_LOOPBACK_MESSAGING:
> > + kfree(isadev->addr);
> > + dev_info(shrm->dev, "Close COMMON_LOOPBACK_MESSAGING
> Device\n");
> > + break;
> > + case AUDIO_LOOPBACK_MESSAGING:
> > + kfree(isadev->addr);
> > + dev_info(shrm->dev, "Close AUDIO_LOOPBACK_MESSAGING
> Device\n");
> > + break;
> > + case CIQ_MESSAGING:
> > + kfree(isadev->addr);
> > + dev_info(shrm->dev, "Close CIQ_MESSAGING Device\n");
> > + break;
> > + case RTC_CAL_MESSAGING:
> > + dev_info(shrm->dev, "Close RTC_CAL_MESSAGING Device\n");
> > + break;
>
> kfree(NULL) is a no-op and guaranteed so, providing you see isadev-
> >addr
> = NULL properly in the open path you can delete all of this code and
> just
> do
> kfree(isadev->addr);
> mutex_unlock(...)
> return 0;
Ok, sounds good!
>
> > + if (get_boot_state() != BOOT_DONE) {
> > + dev_err(shrm->dev, "Boot is not done\n");
> > + return -EBUSY;
> > + }
>
> Is it guaranteed that this is a one way path - ie a device never goes
> back into BOOT state ?
No, on modem reset, everything happens from first.
>
> > + if (!atomic_dec_and_test(&isa_context->is_open[idx])) {
> > + atomic_inc(&isa_context->is_open[idx]);
> > + dev_err(shrm->dev, "Device already opened\n");
> > + mutex_unlock(&isa_lock);
> > + return -EBUSY;
>
> See comments earlier about the atomics
Replied above
>
> > + }
> > + isadev = &isa_context->isadev[idx];
> > + if (filp != NULL)
> > + filp->private_data = isadev;
>
> How can filp be NULL ?
:-) just a error condition check
>
> > +
> > + switch (m) {
> > + case RPC_MESSAGING:
> > + dev_info(shrm->dev, "Open RPC_MESSAGING Device\n");
> > + break;
> > + case AUDIO_MESSAGING:
> > + dev_info(shrm->dev, "Open AUDIO_MESSAGING Device\n");
> > + break;
> > + case SECURITY_MESSAGING:
> > + dev_info(shrm->dev, "Open SECURITY_MESSAGING Device\n");
> > + break;
> > + case COMMON_LOOPBACK_MESSAGING:
> > + isadev->addr = kzalloc(10 * 1024, GFP_KERNEL);
> > + if (!isadev->addr) {
> > + mutex_unlock(&isa_lock);
> > + return -ENOMEM;
> > + }
> > + dev_info(shrm->dev, "Open COMMON_LOOPBACK_MESSAGING
> Device\n");
> > + break;
> > + case AUDIO_LOOPBACK_MESSAGING:
> > + isadev->addr = kzalloc(10 * 1024, GFP_KERNEL);
> > + if (!isadev->addr) {
> > + mutex_unlock(&isa_lock);
> > + return -ENOMEM;
> > + }
> > + dev_info(shrm->dev, "Open AUDIO_LOOPBACK_MESSAGING
> Device\n");
> > + break;
> > + case CIQ_MESSAGING:
> > + isadev->addr = kzalloc(10 * 1024, GFP_KERNEL);
> > + if (!isadev->addr) {
> > + mutex_unlock(&isa_lock);
> > + return -ENOMEM;
> > + }
> > + dev_info(shrm->dev, "Open CIQ_MESSAGING Device\n");
> > + break;
> > + case RTC_CAL_MESSAGING:
> > + dev_info(shrm->dev, "Open RTC_CAL_MESSAGING Device\n");
> > + break;
> > + };
>
> So all of this could just be
>
> dev_dbg("Open device %d\n", m);
> switch (m) {
> case a
> case b
> case c
> isadev->addr = ....
> break;
> case e
> case f
> isadev->addr = NULL;
> break;
> default:
> error path
>
> also in your error case right now if you run out of memory you don't
> fix
> up the open counters.
Ok sounds good!
>
> > +const struct file_operations isa_fops = {
>
> static
>
>
> > + for (no_dev = 0; no_dev < ISA_DEVICES; no_dev++) {
> > + atomic_set(&isa_context->is_open[no_dev], 1);
> > + device_create(isa_context->shm_class, NULL,
> > + MKDEV(MAJOR(dev_id),
> > + map_dev[no_dev].l2_header), NULL,
> > + map_dev[no_dev].name);
> > + }
>
> What happens if I open the device right here... ?
It can be opened, but nothing thereafter, since modem is not booted.
>
> > +
> > + isa_context->isadev = kzalloc(sizeof
> > + (struct isadev_context)*ISA_DEVICES,
> > + GFP_KERNEL);
> > + if (isa_context->isadev == NULL) {
> > + dev_err(shrm->dev, "Failed to alloc memory\n");
> > + return -ENOMEM;
>
> You just leaked all the other stuff you created, left device pointers
> to
> invalid memory registered.
>
> There is a reason kernel drivers use the goto type error handling - it
> avoids missing these things.
Oops missed it will correct.
>
>
> > diff --git a/drivers/modem_shm/u8500_shm/shrm_fifo.c
> b/drivers/modem_shm/u8500_shm/shrm_fifo.c
> > new file mode 100644
> > index 0000000..ad63cc4
> > --- /dev/null
> > +++ b/drivers/modem_shm/u8500_shm/shrm_fifo.c
> > @@ -0,0 +1,837 @@
> > +/*
> > + * Copyright (C) ST-Ericsson SA 2010
> > + *
> > + * Author: Arun Murthy <arun.murthy@stericsson.com>
> > + * Kumar Sanghvi for ST-Ericsson
> > + *
> > + * License terms: GNU General Public License (GPL) version 2
> > + */
> > +
> > +#include <linux/modem_shm/u8500_shm/shrm.h>
> > +#include <linux/modem_shm/u8500_shm/shrm_driver.h>
> > +#include <linux/modem_shm/u8500_shm/shrm_private.h>
> > +#include <linux/modem_shm/u8500_shm/shrm_net.h>
> > +#include <linux/mfd/dbx500-prcmu.h>
> > +
> > +#define L1_BOOT_INFO_REQ 1
> > +#define L1_BOOT_INFO_RESP 2
> > +#define L1_NORMAL_MSG 3
> > +#define L1_HEADER_MASK 28
> > +#define L1_MAPID_MASK 0xF0000000
> > +#define CONFIG_OFFSET 8
> > +#define COUNTER_OFFSET 20
> > +#define L2_HEADER_SIZE 4
> > +#define L2_HEADER_OFFSET 24
> > +#define MASK_0_15_BIT 0xFF
> > +#define MASK_16_31_BIT 0xFF00
> > +#define MASK_16_27_BIT 0xFFF0000
> > +#define MASK_0_39_BIT 0xFFFFF
> > +#define MASK_40_55_BIT 0xFF00000
> > +#define MASK_8_16_BIT 0x0000FF00
> > +#define MSG_LEN_OFFSET 16
> > +#define SHRM_VER 2
> > +#define ca_ist_inactivity_timer 25 /*25ms */
> > +#define ca_csc_inactivity_timer 25 /*25ms */
> > +
> > +static u8 msg_audio_counter;
> > +static u8 msg_common_counter;
> > +
> > +struct fifo_write_params ape_shm_fifo_0;
> > +struct fifo_write_params ape_shm_fifo_1;
> > +struct fifo_read_params cmt_shm_fifo_0;
> > +struct fifo_read_params cmt_shm_fifo_1;
>
> static or fix the naming
Done!
> > +struct sock *shrm_nl_sk;
>
> Better to make globals visible than bury them in a mass of statics
>
> (actually all of this raises a big question - what happens if you ever
> need to have two modems - you are basically hardcoding one per system.
> That's fine if you are *very* sure it will remain true)
Atleast as per the present protocol, two modem is not supported, and if
so in future, then the protocol will have to be affected. The interrupts
part is the major part.
> > + spin_lock_irqsave(&boot_lock, flags);
> > + boot_state = BOOT_INIT;
>
> Your open method locking earlier assumes this cannot go backwards...
This can go backward, locking is done only while modifying this flag.
>
>
> > + dev_info(shm_dev->dev, "Initiating Modem silent reset\n");
>
> Seems an odd use of dev_info.. if its a bug workaround then perhaps
> dev_warn, and if its not important dev_dbg ?
Would prefer dev_dbg
>
> > +DECLARE_TASKLET(shrm_sw_reset_callback, shrm_modem_reset_callback,
> > + IRQ_PRCMU_MODEM_SW_RESET_REQ);
> > +
>
> static
>
> Lots of other stuff here that should be shrm_ or static...
>
> There also seems to be places where you copy or touch the fifo memory
> which was ioremapped but don't use readb/writeb/memcpy_toio etc, or
> handle endianness. While endianness might be the kind of thing you can
> be
> sure of in some embedded platforms not using readb and the like means
> the
> compiler can do interesting optimisations you won't want and is
> something
> that will break on some architectures and potentially on others in
> future.
Sure will look into that again and implement this in the next version
of the patch.
Thanks and Regards,
Arun R Murthy
--------------
^ permalink raw reply
* RE: [PATCHv2 1/4] modem_shm: Add Modem Access Framework
From: Arun MURTHY @ 2012-08-08 3:36 UTC (permalink / raw)
To: Greg KH
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
linux-doc@vger.kernel.org, Sjur BRENDELAND
In-Reply-To: <20120807183809.GC26990@kroah.com>
> On Tue, Aug 07, 2012 at 12:24:28PM +0530, Arun Murthy wrote:
> > Adds Modem Access Framework, which allows for registering platform
> specific
> > modem access mechanisms. The framework also exposes APIs for client
> drivers
> > for getting and releasing access to modem, regardless of the
> underlying
> > platform specific access mechanism.
>
> The term "modems" here has a lot of legacy connotations. First of
> which
> is, userspace handles this today as tty devices, why aren't you doing
> the same here? Why does this have to be something "special"?
>
The main focus over there the modem IPC. In doing so, there are some
functionality like waking the modem, or releasing the modem etc. These
will be used by the modem IPC drivers and also few others like sim driver
and security drivers.
Since this is a shared call and hence has to be synchronized. Hence so a
small framework like is being done to monitor the modem access related only
operations.
> >
> > Signed-off-by: Arun Murthy <arun.murthy@stericsson.com>
> > Acked-by: Linus Walleij <linus.walleij@stericsson.com>
> > ---
> > drivers/Kconfig | 2 +
> > drivers/Makefile | 1 +
> > drivers/modem_shm/Kconfig | 9 +
> > drivers/modem_shm/Makefile | 1 +
> > drivers/modem_shm/modem_access.c | 419
> ++++++++++++++++++++++++++++++++
>
> Any reason why this can't be under drivers/tty/ ?
No specific reason, other than to have a separate place for the modem
access related drivers. Many of the platforms have these functionality
and hence all of them can reside over here using a common framework(MAF)
>
> > include/linux/modem_shm/modem.h | 64 +++++
> > include/linux/modem_shm/modem_client.h | 55 +++++
>
> Why are both of these "public" like this? Why not just make one file?
> Would someone ever only need to include one of these?
modem.h is a header file used by the MAF, but modem_client.h includes
a structure that will be used by all clients registering to this MAF.
Hence the clients can only include only this header.
>
> > +config MODEM_SHM
> > + bool "Modem Access Framework"
> > + default y
>
> Unless it is needed to boot your machine, NEVER make the default y.
Oops,.. will change this.
>
> > +struct modem {
> > + struct device *dev;
> > + struct list_head list;
> > + char *modem_name;
>
> You already have a name in the struct device, why do you need another
> one?
>
> > + struct device_attribute dev_attr;
>
> Why is this in the structure?
>
> > + struct modem_dev *mdev;
> > + atomic_t use;
>
> What is this variable for?
This is used to monitor the number of requests received. Just to
balance the enable/disable process. Something like the regulator
framework.
>
> Why isn't this a 'struct device' itself?
>
> > +/**
> > + * modem_is_requested - check if modem access is requested
> > + * @modem: modem device
> > + *
> > + * Checks whether modem is accessed or not by querying
> > + * the underlying platform specific modem access
> > + * implementation.
> > + */
> > +int modem_is_requested(struct modem *modem)
> > +{
> > + int ret;
> > +
> > + mutex_lock(&modem->mdev->mutex);
> > + ret = _modem_is_requested(modem->mdev);
> > + mutex_unlock(&modem->mdev->mutex);
> > +
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(modem_is_requested);
>
> EXPORT_SYMBOL_GPL() for this, and the other apis perhaps?
OK.
>
> > +static struct modem *_modem_get(struct device *dev, const char *id,
> > + int exclusive)
> > +{
> > + struct modem_dev *mdev_ptr;
> > + struct modem *modem = ERR_PTR(-ENODEV);
> > + int ret;
> > +
> > + if (id == NULL) {
> > + pr_err("modem_get with no identifier\n");
> > + return modem;
> > + }
> > +
> > + mutex_lock(&modem_list_mutex);
> > + list_for_each_entry(mdev_ptr, &modem_list, modem_list) {
> > + if (strcmp(mdev_get_name(mdev_ptr), id) == 0)
> > + goto found;
> > + }
> > +
> > + goto out;
> > +
> > +found:
> > + if (!try_module_get(mdev_ptr->owner))
> > + goto out;
> > +
> > + modem = create_modem(mdev_ptr, dev, id);
> > + if (modem == NULL) {
> > + modem = ERR_PTR(-ENOMEM);
> > + module_put(mdev_ptr->owner);
> > + }
> > +
> > + mdev_ptr->open_count++;
> > + ret = _modem_is_requested(mdev_ptr);
> > + if (ret)
> > + mdev_ptr->use_count = 1;
> > + else
> > + mdev_ptr->use_count = 0;
> > +
> > +out:
> > + mutex_unlock(&modem_list_mutex);
> > + return modem;
> > +
> > +}
>
> Calling this function does a lot more than just incrementing the
> reference count of an object. It sets it up, and creates things. That
> should be way more documented than this one line says:
Sure will elaborate the documentation.
>
> > +/**
> > + * modem_get - Get reference to a particular platform specific modem
> > + * @dev: device
> > + * @id: modem device name
> > + *
> > + * Get reference to a particular modem device.
> > + */
>
> See, that's really not true.
>
> > +static ssize_t modem_print_state(char *buf, int state)
> > +{
> > + if (state > 0)
> > + return sprintf(buf, "accessed\n");
> > + else if (state == 0)
> > + return sprintf(buf, "released\n");
> > + else
> > + return sprintf(buf, "unknown\n");
> > +}
>
> Why not use an enumerated type for your state, to make it easier to
> understand than 0, -, and +?
Done
>
> > +static ssize_t modem_state_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct modem_dev *mdev = dev_get_drvdata(dev);
> > + ssize_t ret;
> > +
> > + mutex_lock(&mdev->mutex);
> > + ret = modem_print_state(buf, _modem_is_requested(mdev));
>
> Why not just embed the function here? It's only ever called once.
This function will be called by the modem IPC drivers also.
>
> > + mutex_unlock(&mdev->mutex);
> > +
> > + return ret;
> > +}
> > +static DEVICE_ATTR(state, 0444, modem_state_show, NULL);
> > +
> > +static ssize_t modem_use_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct modem_dev *mdev = dev_get_drvdata(dev);
> > + struct modem *mod;
> > + size_t size = 0;
> > +
> > + list_for_each_entry(mod, &mdev->client_list, list) {
> > + if (mod->dev != NULL)
> > + size += sprintf((buf + size), "%s (%d)\n",
> > + dev_name(mod->dev), atomic_read(&mod->use));
> > + else
> > + size += sprintf((buf + size), "unknown (%d)\n",
> > + atomic_read(&mod->use));
> > + }
> > + size += sprintf((buf + size), "\n");
> > +
> > + return size;
> > +}
> > +static DEVICE_ATTR(use, 0444, modem_use_show, NULL);
>
> You have sysfs files with no matching Documentation/ABI entries showing
> how they are to be used, and what they contain, in this patch. Please
> fix this.
>
> And why are you reporting an atomic value, that's 2 values per sysfs
> file, not acceptable.
>
Will change it accordingly.
> > +static ssize_t modem_name_show(struct device *dev,
> > + struct device_attribute *attr, char *buf)
> > +{
> > + struct modem_dev *mdev = dev_get_drvdata(dev);
> > +
> > + return sprintf(buf, "%s\n", mdev_get_name(mdev));
> > +}
> > +static DEVICE_ATTR(name, 0444, modem_name_show, NULL);
>
> The name is in the struct device, which is the directory in sysfs,
> don't
> include it again in a sysfs file, that's redundant.
Ok.
>
> > +static int add_modem_attributes(struct modem_dev *mdev)
> > +{
> > + struct device *dev = &mdev->dev;
> > + struct modem_ops *ops = mdev->desc->ops;
> > + int status = 0;
> > +
> > + status = device_create_file(dev, &dev_attr_use);
> > + if (status < 0)
> > + return status;
> > +
> > + status = device_create_file(dev, &dev_attr_name);
> > + if (status < 0)
> > + return status;
> > +
> > + status = device_create_file(dev, &dev_attr_num_active_users);
> > + if (status < 0)
> > + return status;
> > +
> > + if (ops->is_requested) {
> > + status = device_create_file(dev, &dev_attr_state);
> > + if (status < 0)
> > + return status;
> > + }
> > +
> > + return 0;
> > +}
>
> Please use a default attribute group, as you just raced with userspace,
> and now userspace tried to look for these files when the device was
> created, yet they were not present yet, causing all sorts of problems
> with your tools. This must be fixed.
>
Done. Will implement this in the next version of the patch.
Thanks and Regards,
Arun R Murthy
--------------
^ permalink raw reply
* RE: [PATCHv2 3/4] modem_shm: u8500-shm: U8500 Shared Memory Driver
From: Arun MURTHY @ 2012-08-08 3:36 UTC (permalink / raw)
To: Greg KH
Cc: linux-kernel@vger.kernel.org, netdev@vger.kernel.org,
linux-doc@vger.kernel.org, Sjur BRENDELAND
In-Reply-To: <20120807183925.GD26990@kroah.com>
> > drivers/modem_shm/Kconfig | 2 +
> > drivers/modem_shm/Makefile | 1 +
> > drivers/modem_shm/u8500_shm/Kconfig | 43 +
> > drivers/modem_shm/u8500_shm/Makefile | 7 +
> > drivers/modem_shm/u8500_shm/shrm_char.c | 895 ++++++++++++
> > drivers/modem_shm/u8500_shm/shrm_driver.c | 732 ++++++++++
> > drivers/modem_shm/u8500_shm/shrm_fifo.c | 837 ++++++++++++
> > drivers/modem_shm/u8500_shm/shrm_net.c | 312 +++++
> > drivers/modem_shm/u8500_shm/shrm_protocol.c | 1590
> ++++++++++++++++++++++
> > include/linux/modem_shm/u8500_shm/shrm.h | 23 +
> > include/linux/modem_shm/u8500_shm/shrm_config.h | 114 ++
> > include/linux/modem_shm/u8500_shm/shrm_driver.h | 225 +++
> > include/linux/modem_shm/u8500_shm/shrm_ioctl.h | 43 +
> > include/linux/modem_shm/u8500_shm/shrm_net.h | 46 +
> > include/linux/modem_shm/u8500_shm/shrm_private.h | 183 +++
>
> Why do any of those .h files need to be in include/linux/*? Shouldn't
> they just be in the same directory as the driver itself?
Can be moved to the same dir. Will do that in the next ver of the patch.
Thanks and Regards,
Arun R Murthy
-------------
^ permalink raw reply
* What is the significance of coherent_dma_mask?
From: Venu Byravarasu @ 2012-08-08 3:58 UTC (permalink / raw)
To: gregkh, rdunlap, lars, grant.likely, joe; +Cc: netdev, linux-kernel, swarren
Hi,
Some of the USB drivers are setting coherent_dma_mask & many others are
not doing it.
From the description present in the comment beside this field in
device.h, I could not understand its real significance.
Can some one plz let me know its real need and use case where it is
mandatory to use it?
Thanks,
Venu
^ permalink raw reply
* RE: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: Jain Priyanka-B32167 @ 2012-08-08 4:53 UTC (permalink / raw)
To: David Miller; +Cc: netdev@vger.kernel.org
In-Reply-To: <20120807.162138.5000306557864030.davem@davemloft.net>
-----Original Message-----
From: David Miller [mailto:davem@davemloft.net]
Sent: Wednesday, August 08, 2012 4:52 AM
To: Jain Priyanka-B32167
Cc: netdev@vger.kernel.org
Subject: Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: Priyanka Jain <Priyanka.Jain@freescale.com>
Date: Tue, 7 Aug 2012 10:51:44 +0530
> xfrm_policy_afinfo is read mosly data structure.
> Write on xfrm_policy_afinfo is done only at the time of configuration.
> So rwlocks can be safely replaced with RCU.
>
> RCUs usage optimizes the performance.
>
> Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
This patch doesn't apply to the net-next tree, please respin.
[Priyanka]: I will send v2 after re-spinning against git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git.
Also:
> - xfrm_policy_afinfo[afinfo->family] = NULL;
> + rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family],
> + NULL);
Indent that NULL argument properly, it must line up with the first column after the openning '(' on the previous line.
[Priyanka]: NULL has been pushed to next line to confirm to 80 characters per line rule. If I indent NULL to previous line, it will break 80 characters per line rule.
Please let me know your final say on this. I will make changes accordingly if required.
Thanks
Priyanka
^ permalink raw reply
* Re: [PATCH V2 09/12] net/eipoib: Add main driver functionality
From: Or Gerlitz @ 2012-08-08 5:23 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Eric W. Biederman, Or Gerlitz, davem, roland, netdev, ali,
sean.hefty, Erez Shitrit, Doug Ledford
In-Reply-To: <20120805185031.GA18640@redhat.com>
On Sun, Aug 5, 2012 at 9:50 PM, Michael S. Tsirkin <mst@redhat.com> wrote:
[...]
> So it seems that a sane solution would involve an extra level of
> indirection, with guest addresses being translated to host IB addresses.
> As long as you do this, maybe using an ethernet frame format makes sense.
[...]
Yep, that's among the points we're trying to make, the way you've put
it makes it clearer.
> So far the things that make sense. Here are some that don't, to me:
> - Is a pdf presentation all you have in terms of documentation?
> We are talking communication protocols here - I would expect a
> proper spec, and some effort to standardize, otherwise where's the
> guarantee it won't change in an incompatible way?
To be precise, the solution uses 100% IPoIB wire-protocol, so we don't
see a need
for any spec change / standardization effort. This might go to the 1st
point you've
brought... improve the documentation, will do that. The pdf you looked
at was presented
in a conference.
> Other things that I would expect to be addressed in such a spec is
> interaction with other IPoIB features, such as connected
> mode, checksum offloading etc, and IB features such as multipath etc.
For the eipoib interface, it doesn't really matters if the underlyind
ipoib clones used by it (we call them VIFs) use connected or datagram
mode, what does matter is the MTU and offload features supported by
these VIFs, for which the eipoib interface will have the min among all
these VIFs. Since for a given eipoib nic, all its VIFs must originated
from the same IPoIB PIF (e.g ib0) its easy admin job to make sure they
all have the same mtu / features which are needed for that eipoib nic,
e.g by using the same mode (connected/datagram for all of them), hope
this is clear.
> - The way you encode LID/QPN in the MAC seems questionable. IIRC there's
> more to IB addressing than just the LID. Since everyone on the subnet
> need access to this translation, I think it makes sense to store it in
> the SM. I think this would also obviate some IPv4 specific hacks in kernel.
The idead beyond the encoding was uniqueness, LID/QPN is unique per IB
HCA end-node. I wasn't sure to understand the comment re the IPv4 hacks.
> - IGMP/MAC snooping in a driver is just too hairy.
mmm, any rough idea/direction how to do that otherwise?
> As you point out, bridge currently needs the uplink in promisc mode.
> I don't think a driver should work around that limitation.
> For some setups, it might be interesting to remove the
> promisc mode requirement, failing that,
> I think you could use macvtap passthrough.
That's in the plans, the current code doesn't assume that the eipoib
has bridge on top, for VM networking it works with bridge + tap,
bridge + macvtap, but it would easily work with passthrough when we
allow to create multiple eipoib interfaces on the same ipoib PIF (e.g
today for the ib0 PIF we create eipoib eth0, and then two VIFs ib0.1
and ib0.2 that are enslaved by eth0, but next we will create eth1 and
eth2 which will use ib0.1 and ib0.2
respectively.
> - Currently migration works without host kernel help, would be
> preferable to keep it that way.
OK
^ permalink raw reply
* Re: [PATCH v2] can: kvaser_usb: Add support for Kvaser CAN/USB devices
From: Olivier Sobrie @ 2012-08-08 5:28 UTC (permalink / raw)
To: Oliver Neukum
Cc: Wolfgang Grandegger, Marc Kleine-Budde, linux-can, linux-usb,
netdev
In-Reply-To: <1377311.1NXjxk1Tbx@linux-lqwf.site>
Hi Oliver,
On Mon, Aug 06, 2012 at 10:10:43AM +0200, Oliver Neukum wrote:
> On Monday 06 August 2012 07:21:29 Olivier Sobrie wrote:
> > This driver provides support for several Kvaser CAN/USB devices.
> > Such kind of devices supports up to three can network interfaces.
> >
> > It has been tested with a Kvaser USB Leaf Light (one network interface)
> > connected to a pch_can interface.
> > The firmware version of the Kvaser device was 2.5.205.
>
> > +static netdev_tx_t kvaser_usb_start_xmit(struct sk_buff *skb,
> > + struct net_device *netdev)
> > +{
> > + struct kvaser_usb_net_priv *priv = netdev_priv(netdev);
> > + struct kvaser_usb *dev = priv->dev;
> > + struct net_device_stats *stats = &netdev->stats;
> > + struct can_frame *cf = (struct can_frame *)skb->data;
> > + struct kvaser_usb_tx_urb_context *context = NULL;
> > + struct urb *urb;
> > + void *buf;
> > + struct kvaser_msg *msg;
> > + int i, err;
> > + int ret = NETDEV_TX_OK;
> > +
> > + if (can_dropped_invalid_skb(netdev, skb))
> > + return NETDEV_TX_OK;
> > +
> > + urb = usb_alloc_urb(0, GFP_ATOMIC);
> > + if (!urb) {
> > + netdev_err(netdev, "No memory left for URBs\n");
> > + stats->tx_dropped++;
> > + dev_kfree_skb(skb);
> > + return NETDEV_TX_OK;
> > + }
> > +
> > + buf = usb_alloc_coherent(dev->udev, sizeof(struct kvaser_msg),
> > + GFP_ATOMIC, &urb->transfer_dma);
>
> usb_alloc_coherent() as a rule only makes sense if you reuse the buffer
> and in some cases not even then. Please use a simple kmalloc()
Ok thanks. I'll change it.
>
> > + if (!buf) {
> > + netdev_err(netdev, "No memory left for USB buffer\n");
> > + stats->tx_dropped++;
> > + dev_kfree_skb(skb);
> > + goto nobufmem;
> > + }
> > +
> > + msg = (struct kvaser_msg *)buf;
> > + msg->len = MSG_HEADER_LEN + sizeof(struct kvaser_msg_tx_can);
> > + msg->u.tx_can.flags = 0;
> > + msg->u.tx_can.channel = priv->channel;
> > +
> > + if (cf->can_id & CAN_EFF_FLAG) {
> > + msg->id = CMD_TX_EXT_MESSAGE;
> > + msg->u.tx_can.msg[0] = (cf->can_id >> 24) & 0x1f;
> > + msg->u.tx_can.msg[1] = (cf->can_id >> 18) & 0x3f;
> > + msg->u.tx_can.msg[2] = (cf->can_id >> 14) & 0x0f;
> > + msg->u.tx_can.msg[3] = (cf->can_id >> 6) & 0xff;
> > + msg->u.tx_can.msg[4] = cf->can_id & 0x3f;
> > + } else {
> > + msg->id = CMD_TX_STD_MESSAGE;
> > + msg->u.tx_can.msg[0] = (cf->can_id >> 6) & 0x1f;
> > + msg->u.tx_can.msg[1] = cf->can_id & 0x3f;
> > + }
> > +
> > + msg->u.tx_can.msg[5] = cf->can_dlc;
> > + memcpy(&msg->u.tx_can.msg[6], cf->data, cf->can_dlc);
> > +
> > + if (cf->can_id & CAN_RTR_FLAG)
> > + msg->u.tx_can.flags |= MSG_FLAG_REMOTE_FRAME;
> > +
> > + for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++) {
> > + if (priv->tx_contexts[i].echo_index == MAX_TX_URBS) {
> > + context = &priv->tx_contexts[i];
> > + break;
> > + }
> > + }
> > +
> > + if (!context) {
> > + netdev_warn(netdev, "cannot find free context\n");
> > + ret = NETDEV_TX_BUSY;
> > + goto releasebuf;
> > + }
> > +
> > + context->priv = priv;
> > + context->echo_index = i;
> > + context->dlc = cf->can_dlc;
> > +
> > + msg->u.tx_can.tid = context->echo_index;
> > +
> > + usb_fill_bulk_urb(urb, dev->udev,
> > + usb_sndbulkpipe(dev->udev,
> > + dev->bulk_out->bEndpointAddress),
> > + buf, msg->len,
> > + kvaser_usb_write_bulk_callback, context);
> > + urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP;
> > + usb_anchor_urb(urb, &priv->tx_submitted);
> > +
> > + can_put_echo_skb(skb, netdev, context->echo_index);
> > +
> > + atomic_inc(&priv->active_tx_urbs);
> > +
> > + if (atomic_read(&priv->active_tx_urbs) >= MAX_TX_URBS)
> > + netif_stop_queue(netdev);
> > +
> > + err = usb_submit_urb(urb, GFP_ATOMIC);
> > + if (unlikely(err)) {
> > + can_free_echo_skb(netdev, context->echo_index);
> > +
> > + atomic_dec(&priv->active_tx_urbs);
> > + usb_unanchor_urb(urb);
> > +
> > + stats->tx_dropped++;
> > +
> > + if (err == -ENODEV)
> > + netif_device_detach(netdev);
> > + else
> > + netdev_warn(netdev, "Failed tx_urb %d\n", err);
> > +
> > + goto releasebuf;
> > + }
> > +
> > + netdev->trans_start = jiffies;
> > +
> > + usb_free_urb(urb);
> > +
> > + return NETDEV_TX_OK;
> > +
> > +releasebuf:
> > + usb_free_coherent(dev->udev, sizeof(struct kvaser_msg),
> > + buf, urb->transfer_dma);
> > +nobufmem:
> > + usb_free_urb(urb);
> > + return ret;
> > +}
>
> > +static int kvaser_usb_init_one(struct usb_interface *intf,
> > + const struct usb_device_id *id, int channel)
> > +{
> > + struct kvaser_usb *dev = usb_get_intfdata(intf);
> > + struct net_device *netdev;
> > + struct kvaser_usb_net_priv *priv;
> > + int i, err;
> > +
> > + netdev = alloc_candev(sizeof(*priv), MAX_TX_URBS);
> > + if (!netdev) {
> > + dev_err(&intf->dev, "Cannot alloc candev\n");
> > + return -ENOMEM;
> > + }
> > +
> > + priv = netdev_priv(netdev);
> > +
> > + init_completion(&priv->start_comp);
> > + init_completion(&priv->stop_comp);
> > +
> > + init_usb_anchor(&priv->tx_submitted);
> > + atomic_set(&priv->active_tx_urbs, 0);
> > +
> > + for (i = 0; i < ARRAY_SIZE(priv->tx_contexts); i++)
> > + priv->tx_contexts[i].echo_index = MAX_TX_URBS;
> > +
> > + priv->dev = dev;
> > + priv->netdev = netdev;
> > + priv->channel = channel;
> > +
> > + priv->can.state = CAN_STATE_STOPPED;
> > + priv->can.clock.freq = CAN_USB_CLOCK;
> > + priv->can.bittiming_const = &kvaser_usb_bittiming_const;
> > + priv->can.do_set_bittiming = kvaser_usb_set_bittiming;
> > + priv->can.do_set_mode = kvaser_usb_set_mode;
> > + priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES;
> > + if (id->driver_info & KVASER_HAS_SILENT_MODE)
> > + priv->can.ctrlmode_supported |= CAN_CTRLMODE_LISTENONLY;
> > +
> > + netdev->flags |= IFF_ECHO;
> > +
> > + netdev->netdev_ops = &kvaser_usb_netdev_ops;
> > +
> > + SET_NETDEV_DEV(netdev, &intf->dev);
> > +
> > + err = register_candev(netdev);
> > + if (err) {
> > + dev_err(&intf->dev, "Failed to register can device\n");
> > + free_candev(netdev);
> > + return err;
> > + }
> > +
>
> Here the device is usable.
>
> > + dev->nets[channel] = priv;
>
> And only know you init this field. Looks like a race condition.
Argh... Indeed.
Thanks
>
> > + netdev_dbg(netdev, "device registered\n");
> > +
> > + return 0;
> > +}
> > +
>
> > +static void kvaser_usb_disconnect(struct usb_interface *intf)
> > +{
> > + struct kvaser_usb *dev = usb_get_intfdata(intf);
> > + int i;
> > +
> > + usb_set_intfdata(intf, NULL);
> > +
> > + if (!dev)
> > + return;
> > +
> > + for (i = 0; i < dev->nchannels; i++) {
> > + if (!dev->nets[i])
> > + continue;
> > +
> > + unregister_netdev(dev->nets[i]->netdev);
> > + free_candev(dev->nets[i]->netdev);
> > + }
> > +
> > + kvaser_usb_unlink_all_urbs(dev);
>
> So what happens if an URB completes between freeing the candev
> and unlinking and proceeds to push data to upper layers?
In this case I should avoid using netdev context, i.e. pointer to "struct
kvaser_usb_net_priv" in urb callbacks.
Is that what you mean?
I see it is well done in kvaser_usb_read_bulk_callback()
but not in kvaser_usb_write_bulk_callback().
I'll better protect the callbacks and move the free_candev after the
unlink.
Thank you,
--
Olivier
^ permalink raw reply
* Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
From: David Miller @ 2012-08-08 5:55 UTC (permalink / raw)
To: B32167; +Cc: netdev
In-Reply-To: <AC83832D6324604BB08FAE5A33553F1907A96EA6@039-SN2MPN1-012.039d.mgd.msft.net>
From: Jain Priyanka-B32167 <B32167@freescale.com>
Date: Wed, 8 Aug 2012 04:53:42 +0000
>
>
> -----Original Message-----
> From: David Miller [mailto:davem@davemloft.net]
> Sent: Wednesday, August 08, 2012 4:52 AM
> To: Jain Priyanka-B32167
> Cc: netdev@vger.kernel.org
> Subject: Re: [PATCH][XFRM] Replace rwlock on xfrm_policy_afinfo with rcu
>
> From: Priyanka Jain <Priyanka.Jain@freescale.com>
> Date: Tue, 7 Aug 2012 10:51:44 +0530
>
>> xfrm_policy_afinfo is read mosly data structure.
>> Write on xfrm_policy_afinfo is done only at the time of configuration.
>> So rwlocks can be safely replaced with RCU.
>>
>> RCUs usage optimizes the performance.
>>
>> Signed-off-by: Priyanka Jain <Priyanka.Jain@freescale.com>
>
> This patch doesn't apply to the net-next tree, please respin.
> [Priyanka]: I will send v2 after re-spinning against git://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git.
>
> Also:
>
>> - xfrm_policy_afinfo[afinfo->family] = NULL;
>> + rcu_assign_pointer(xfrm_policy_afinfo[afinfo->family],
>> + NULL);
>
> Indent that NULL argument properly, it must line up with the first column after the openning '(' on the previous line.
> [Priyanka]: NULL has been pushed to next line to confirm to 80 characters per line rule. If I indent NULL to previous line, it will break 80 characters per line rule.
> Please let me know your final say on this. I will make changes accordingly if required.
What in the world are you talking about?
The openning parenthesis of the rcu_assign_pointer() statement is not anywhere
close to the 80th column.
You're doing this:
x(A,
B);
and I want you to do this:
x(A,
B);
^ 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