Re: [PATCH v2 1/2] Phonet: Implement Pipe Controller to support Nokia Slim Modems
From: Rémi Denis-Courmont <hidden>
Date: 2010-09-28 22:24:33
On Monday 27 September 2010 08:07:59 ext Kumar A Sanghvi, you wrote:
quoted hunk ↗ jump to hunk
From: Kumar Sanghvi <redacted> Phonet stack assumes the presence of Pipe Controller, either in Modem or on Application Processing Engine user-space for the Pipe data. Nokia Slim Modems like WG2.5 used in ST-Ericsson U8500 platform do not implement Pipe controller in them. This patch adds Pipe Controller implemenation to Phonet stack to support Pipe data over Phonet stack for Nokia Slim Modems. Signed-off-by: Kumar Sanghvi <redacted> Acked-by: Linus Walleij <redacted> --- Changes: -v2: Correction for header retrieving after pskb_may_pull include/linux/phonet.h | 5 + include/net/phonet/pep.h | 21 +++ net/phonet/Kconfig | 11 ++ net/phonet/pep.c | 448 +++++++++++++++++++++++++++++++++++++++++++++- 4 files changed, 479 insertions(+), 6 deletions(-)diff --git a/include/linux/phonet.h b/include/linux/phonet.h index 85e14a8..96f5625 100644 --- a/include/linux/phonet.h +++ b/include/linux/phonet.h@@ -36,6 +36,11 @@ /* Socket options for SOL_PNPIPE level */ #define PNPIPE_ENCAP 1 #define PNPIPE_IFINDEX 2 +#define PNPIPE_CREATE 3 +#define PNPIPE_ENABLE 4 +#define PNPIPE_DISABLE 5 +#define PNPIPE_DESTROY 6 +#define PNPIPE_INQ 7
As far as I know, you don't need to do that in kernel space. I don't know the
internals of the STE modem. Regardless of having or not having a pipe
controller, Linux userspace can send pipe messages using a plain Phonet
datagram socket. This avoids adding ioctl()'s and protocol stuff in kernel
space. Then, as far as kernel is concerned, only small changes to the data
path would be required.
Both the Nokia modem plugin for oFono, and the (closed-source) Nokia N900 CSD-
GPRS service work that way. In other words, the pipe 'signaling' is done in
userspace, while the pipe 'data' is done in kernel space for optimal
performance.
For some background - Phonet pipes work very much like FTP. There are two
endpoints exchaning data, and one client ('owner') deciding which endpoints
and when to establish a pipe between. In most case the client is also one of
the endpoint, but this is not required. With this patch, the client is tied to
being one of the endpoint. Arguably, this is not a problem for most usecases.
But I am not sure this belongs in *kernel* space. Sticking to the same
comparison: with FTP, you have TCP (data path) in kernel, but not FTP itself
(signaling).
quoted hunk ↗ jump to hunk
@@ -791,6 +1171,48 @@ static int pep_setsockopt(struct sock *sk, int level,int optname, lock_sock(sk); switch (optname) { +#ifdef CONFIG_PHONET_PIPECTRLR + case PNPIPE_CREATE: + if (val) { + if (pn->pipe_state > PIPE_IDLE) { + err = -EFAULT;
Why EFAULT here? I can't see any user-space memory access failure.
+ break;
+ }
+ remote_pep = val & 0xFFFF;
+ pipe_handle = (val >> 16) & 0xFF;
+ pn->remote_pep = remote_pep;
+ err = pipe_handler_create_pipe(sk, pipe_handle,
+ PNPIPE_CREATE);
+ break;
+ }
+
+ case PNPIPE_ENABLE:
+ if (pn->pipe_state != PIPE_DISABLED) {
+ err = -EFAULT;Same here.
+ break;
+ }
+ err = pipe_handler_enable_pipe(sk, PNPIPE_ENABLE);
+ break;
+
+ case PNPIPE_DISABLE:
+ if (pn->pipe_state != PIPE_ENABLED) {
+ err = -EFAULT;Ditto.
+ break;
+ }
+
+ err = pipe_handler_enable_pipe(sk, PNPIPE_DISABLE);
+ break;
+
+ case PNPIPE_DESTROY:
+ if (pn->pipe_state < PIPE_DISABLED) {
+ err = -EFAULT;And here,
quoted hunk ↗ jump to hunk
@@ -877,11 +1313,11 @@ static int pipe_skb_send(struct sock *sk, structsk_buff *skb) } else ph->message_id = PNS_PIPE_DATA; ph->pipe_handle = pn->pipe_handle; - - err = pn_skb_send(sk, skb, &pipe_srv); - if (err && pn_flow_safe(pn->tx_fc)) - atomic_inc(&pn->tx_credits); - return err; +#ifdef CONFIG_PHONET_PIPECTRLR + return pn_skb_send(sk, skb, &spn); +#else + return pn_skb_send(sk, skb, &pipe_srv); +#endif }
This reintroduces the bug that I fixed in 1a98214feef2221cd7c24b17cd688a5a9d85b2ea :-( -- Rémi Denis-Courmont Nokia Devices R&D, Maemo Software, Helsinki