[PATCH 1/4] Phonet: add to MAINTAINERS and add myself

[PATCH 1/4] Phonet: add to MAINTAINERS and add myself

[Rémi Denis-Courmont]

From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
 MAINTAINERS |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 9ddb5ac..1fd58ef 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -4541,6 +4541,14 @@ L:	linux-abi-devel@lists.sourceforge.net
 S:	Maintained
 F:	include/linux/personality.h
 
+PHONET PROTOCOL
+M:	Remi Denis-Courmont <remi.denis-courmont@nokia.com>
+S:	Supported
+F:	Documentation/networking/phonet.txt
+F:	include/linux/phonet.h
+F:	include/net/phonet/
+F:	net/phonet/
+
 PHRAM MTD DRIVER
 M:	Joern Engel <joern@lazybastard.org>
 L:	linux-mtd@lists.infradead.org
-- 
1.7.0.4

[PATCH 2/4] Phonet: advise against enabling the pipe controller

[Rémi Denis-Courmont]

From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

As it currently is, the new code path is not compatible with existing
Nokia modems. This would break existing userspace for Nokia modem, such
as the existing oFono ISI driver.

Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
 net/phonet/Kconfig |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/net/phonet/Kconfig b/net/phonet/Kconfig
index 901956a..a4fceb8 100644
--- a/net/phonet/Kconfig
+++ b/net/phonet/Kconfig
@@ -24,4 +24,5 @@ config PHONET_PIPECTRLR
 	  data with Nokia Slim modems like WG2.5 used on ST-Ericsson U8500
 	  platform.
 
-	  If unsure, say N.
+	  This option is incompatible with older Nokia modems.
+	  Say N here unless you really know what you are doing.
-- 
1.7.0.4

[PATCH 3/4] Phonet: cleanup pipe enable socket option

[Rémi Denis-Courmont]

From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

The current code works like this:

  int garbage, status;
  socklen_t len = sizeof(status);

  /* enable pipe */
  setsockopt(fd, SOL_PNPIPE, PNPIPE_ENABLE, &garbage, sizeof(garbage));
  /* disable pipe */
  setsockopt(fd, SOL_PNPIPE, PNPIPE_DISABLE, &garbage, sizeof(garbage));
  /* get status */
  getsockopt(fd, SOL_PNPIPE, PNPIPE_INQ, &status, &len);

...which does not follow the usual socket option pattern. This patch
merges all three "options" into a single gettable&settable option,
before Linux 2.6.37 gets out:

  int status;
  socklen_t len = sizeof(status);

  /* enable pipe */
  status = 1;
  setsockopt(fd, SOL_PNPIPE, PNPIPE_ENABLE, &status, sizeof(status));
  /* disable pipe */
  status = 0;
  setsockopt(fd, SOL_PNPIPE, PNPIPE_ENABLE, &status, sizeof(status));
  /* get status */
  getsockopt(fd, SOL_PNPIPE, PNPIPE_ENABLE, &status, &len);

This also fixes the error code from EFAULT to ENOTCONN.

Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
Cc: Kumar Sanghvi <kumar.sanghvi@stericsson.com>
---
 Documentation/networking/phonet.txt |   15 +------
 include/linux/phonet.h              |    3 +-
 net/phonet/pep.c                    |   72 ++++++++++++++--------------------
 3 files changed, 34 insertions(+), 56 deletions(-)

diff --git a/Documentation/networking/phonet.txt b/Documentation/networking/phonet.txt
index cccf5ff..2d9bc2b 100644
--- a/Documentation/networking/phonet.txt
+++ b/Documentation/networking/phonet.txt
@@ -213,12 +213,9 @@ The implementation adds socket options at SOL_PNPIPE level:
 	It then updates the pipe state associated with the sequenced socket to
 	be PIPE_DISABLED.
 
-  PNPIPE_ENABLE
-	It follows the same sequence as above for enabling a pipe by sending
-	PNS_PEP_ENABLE_REQ initially and then sending PNS_PEP_ENABLED_IND after
-	getting responses from sequenced socket and remote-pep.
-	It will also update the pipe state associated with the sequenced socket
-	to PIPE_ENABLED.
+  PNPIPE_ENABLE accepts one integer value (int). If set to zero, the pipe
+    is disabled. If the value is non-zero, the pipe is enabled. If the pipe
+    is not (yet) connected, ENOTCONN is error is returned.
 
    PNPIPE_DESTROY
 	This will send out PNS_PEP_DISCONNECT_REQ on the sequenced socket and
@@ -226,12 +223,6 @@ The implementation adds socket options at SOL_PNPIPE level:
 	It will also update the pipe state associated with the sequenced socket
 	to PIPE_IDLE
 
-   PNPIPE_INQ
-	This getsocktopt allows the user-space running on the sequenced socket
-	to examine the pipe state associated with that socket ie. whether the
-	pipe is created (PIPE_DISABLED) or enabled (PIPE_ENABLED) or disabled
-	(PIPE_DISABLED) or no pipe exists (PIPE_IDLE).
-
 After a pipe has been created and enabled successfully, the Pipe data can be
 exchanged between the host-pep and remote-pep (modem).
 
diff --git a/include/linux/phonet.h b/include/linux/phonet.h
index 96f5625..e27cbf9 100644
--- a/include/linux/phonet.h
+++ b/include/linux/phonet.h
@@ -38,9 +38,8 @@
 #define PNPIPE_IFINDEX		2
 #define PNPIPE_CREATE           3
 #define PNPIPE_ENABLE           4
-#define PNPIPE_DISABLE          5
+/* unused slot */
 #define PNPIPE_DESTROY          6
-#define PNPIPE_INQ              7
 
 #define PNADDR_ANY		0
 #define PNADDR_BROADCAST	0xFC
diff --git a/net/phonet/pep.c b/net/phonet/pep.c
index aa3d870..f818f76 100644
--- a/net/phonet/pep.c
+++ b/net/phonet/pep.c
@@ -327,29 +327,20 @@ static int pipe_handler_send_ind(struct sock *sk, u16 dobj, u8 utid,
 	return pn_skb_send(sk, skb, &spn);
 }
 
-static int pipe_handler_enable_pipe(struct sock *sk, int cmd)
+static int pipe_handler_enable_pipe(struct sock *sk, int enable)
 {
-	int ret;
 	struct pep_sock *pn = pep_sk(sk);
-
-	switch (cmd) {
-	case PNPIPE_ENABLE:
-		ret = pipe_handler_send_req(sk, pn->pn_sk.sobject,
-				PNS_PIPE_ENABLE_UTID, PNS_PEP_ENABLE_REQ,
-				pn->pipe_handle, GFP_ATOMIC);
-		break;
-
-	case PNPIPE_DISABLE:
-		ret = pipe_handler_send_req(sk, pn->pn_sk.sobject,
-				PNS_PIPE_DISABLE_UTID, PNS_PEP_DISABLE_REQ,
-				pn->pipe_handle, GFP_ATOMIC);
-		break;
-
-	default:
-		ret = -EINVAL;
+	int utid, req;
+
+	if (enable) {
+		utid = PNS_PIPE_ENABLE_UTID;
+		req = PNS_PEP_ENABLE_REQ;
+	} else {
+		utid = PNS_PIPE_DISABLE_UTID;
+		req = PNS_PEP_DISABLE_REQ;
 	}
-
-	return ret;
+	return pipe_handler_send_req(sk, pn->pn_sk.sobject, utid, req,
+			pn->pipe_handle, GFP_ATOMIC);
 }
 
 static int pipe_handler_create_pipe(struct sock *sk, int pipe_handle, int cmd)
@@ -1187,23 +1178,6 @@ static int pep_setsockopt(struct sock *sk, int level, int optname,
 			break;
 		}
 
-	case PNPIPE_ENABLE:
-		if (pn->pipe_state != PIPE_DISABLED) {
-			err = -EFAULT;
-			break;
-		}
-		err = pipe_handler_enable_pipe(sk, PNPIPE_ENABLE);
-		break;
-
-	case PNPIPE_DISABLE:
-		if (pn->pipe_state != PIPE_ENABLED) {
-			err = -EFAULT;
-			break;
-		}
-
-		err = pipe_handler_enable_pipe(sk, PNPIPE_DISABLE);
-		break;
-
 	case PNPIPE_DESTROY:
 		if (pn->pipe_state < PIPE_DISABLED) {
 			err = -EFAULT;
@@ -1239,6 +1213,17 @@ static int pep_setsockopt(struct sock *sk, int level, int optname,
 			err = 0;
 		}
 		goto out_norel;
+
+#ifdef CONFIG_PHONET_PIPECTRLR
+	case PNPIPE_ENABLE:
+		if (pn->pipe_state <= PIPE_IDLE) {
+			err = -ENOTCONN;
+			break;
+		}
+		err = pipe_handler_enable_pipe(sk, val);
+		break;
+#endif
+
 	default:
 		err = -ENOPROTOOPT;
 	}
@@ -1264,15 +1249,18 @@ static int pep_getsockopt(struct sock *sk, int level, int optname,
 		val = pn->ifindex ? PNPIPE_ENCAP_IP : PNPIPE_ENCAP_NONE;
 		break;
 
+	case PNPIPE_IFINDEX:
+		val = pn->ifindex;
+		break;
+
 #ifdef CONFIG_PHONET_PIPECTRLR
-	case PNPIPE_INQ:
-		val = pn->pipe_state;
+	case PNPIPE_ENABLE:
+		if (pn->pipe_state <= PIPE_IDLE)
+			return -ENOTCONN;
+		val = pn->pipe_state != PIPE_DISABLED;
 		break;
 #endif
 
-	case PNPIPE_IFINDEX:
-		val = pn->ifindex;
-		break;
 	default:
 		return -ENOPROTOOPT;
 	}
-- 
1.7.0.4

[PATCH 4/4] Phonet: mark the pipe controller as EXPERIMENTAL

[Rémi Denis-Courmont]

From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

There are a bunch of issues that need to be fixed, including:
 - GFP_KERNEL allocations from atomic context
   (and GFP_ATOMIC in process context),
 - abuse of the setsockopt() call convention,
 - unprotected/unlocked static variables...

IMHO, we will need to alter the userspace ABI when we fix it. So mark
the configuration option as EXPERIMENTAL for the time being (or should
it be BROKEN instead?).

Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
---
 net/phonet/Kconfig |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/net/phonet/Kconfig b/net/phonet/Kconfig
index a4fceb8..0d9b8a2 100644
--- a/net/phonet/Kconfig
+++ b/net/phonet/Kconfig
@@ -16,8 +16,8 @@ config PHONET
 	  will be called phonet. If unsure, say N.
 
 config PHONET_PIPECTRLR
-	bool "Phonet Pipe Controller"
-	depends on PHONET
+	bool "Phonet Pipe Controller (EXPERIMENTAL)"
+	depends on PHONET && EXPERIMENTAL
 	default N
 	help
 	  The Pipe Controller implementation in Phonet stack to support Pipe
-- 
1.7.0.4

Re: [PATCH 4/4] Phonet: mark the pipe controller as EXPERIMENTAL

[David Miller]

From: Rémi Denis-Courmont <remi@remlab.net>
Date: Fri,  8 Oct 2010 17:02:03 +0300

> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> 
> There are a bunch of issues that need to be fixed, including:
>  - GFP_KERNEL allocations from atomic context
>    (and GFP_ATOMIC in process context),
>  - abuse of the setsockopt() call convention,
>  - unprotected/unlocked static variables...
> 
> IMHO, we will need to alter the userspace ABI when we fix it. So mark
> the configuration option as EXPERIMENTAL for the time being (or should
> it be BROKEN instead?).
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

Applied.

Re: [PATCH 4/4] Phonet: mark the pipe controller as EXPERIMENTAL

[Kumar SANGHVI]

On Fri, Oct 08, 2010 at 16:02:03 +0200, Rémi Denis-Courmont wrote:
> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> 
> There are a bunch of issues that need to be fixed, including:
>  - GFP_KERNEL allocations from atomic context
>    (and GFP_ATOMIC in process context),
>  - abuse of the setsockopt() call convention,
>  - unprotected/unlocked static variables...
> 
> IMHO, we will need to alter the userspace ABI when we fix it. So mark
> the configuration option as EXPERIMENTAL for the time being (or should
> it be BROKEN instead?).
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

Thank you for this patch.
I have already started implementing 'connect()' socket call for Pipe
controller logic, as per advise of Rémi Denis-Courmontt. I will upload
this patch for review once I have validated GPRS/3G path (control/data)
to be working fine.

Hopefully, with all the reviews and suggestions shared on the mailing
list, existing abuse can be removed and new abuses can be avoided.

The only intention of submitting is to make Linux work smoothly with
newer devices. If there is some improper code getting introduced
unknowingly, then I will surely work on the review comments to fix it.

Thank you all!

Best regards,
Kumar.

Re: [PATCH 4/4] Phonet: mark the pipe controller as EXPERIMENTAL

[Rémi Denis-Courmont]

On Mon, 11 Oct 2010 12:39:29 +0530, Kumar SANGHVI
<kumar.sanghvi@stericsson.com> wrote:
> I have already started implementing 'connect()' socket call for Pipe
> controller logic, as per advise of Rémi Denis-Courmontt. I will upload
> this patch for review once I have validated GPRS/3G path (control/data)
> to be working fine.
> 
> Hopefully, with all the reviews and suggestions shared on the mailing
> list, existing abuse can be removed and new abuses can be avoided.
> 
> The only intention of submitting is to make Linux work smoothly with
> newer devices. If there is some improper code getting introduced
> unknowingly, then I will surely work on the review comments to fix it.

I am sure everybody wants to get things right eventually. That's not much
of a concern to me.

However, I am concerned that Linux 2.6.37 ships with an unfinished
implementation, or worse, an unfinished user-space interface.

-- 
Rémi Denis-Courmont
http://www.remlab.net
http://fi.linkedin.com/in/remidenis

Re: [PATCH 3/4] Phonet: cleanup pipe enable socket option

[David Miller]

From: Rémi Denis-Courmont <remi@remlab.net>
Date: Fri,  8 Oct 2010 17:02:02 +0300

> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> 
> The current code works like this:
 ...
> ...which does not follow the usual socket option pattern. This patch
> merges all three "options" into a single gettable&settable option,
> before Linux 2.6.37 gets out:
 ...
> This also fixes the error code from EFAULT to ENOTCONN.
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> Cc: Kumar Sanghvi <kumar.sanghvi@stericsson.com>

Applied.

Re: [PATCH 3/4] Phonet: cleanup pipe enable socket option

[Kumar SANGHVI]

Hi,

On Fri, Oct 08, 2010 at 16:02:02 +0200, Rémi Denis-Courmont wrote:
> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> 
> The current code works like this:
> 
>   int garbage, status;
>   socklen_t len = sizeof(status);
> 
>   /* enable pipe */
>   setsockopt(fd, SOL_PNPIPE, PNPIPE_ENABLE, &garbage, sizeof(garbage));
>   /* disable pipe */
>   setsockopt(fd, SOL_PNPIPE, PNPIPE_DISABLE, &garbage, sizeof(garbage));
>   /* get status */
>   getsockopt(fd, SOL_PNPIPE, PNPIPE_INQ, &status, &len);
> 
> ...which does not follow the usual socket option pattern. This patch
> merges all three "options" into a single gettable&settable option,
> before Linux 2.6.37 gets out:
> 
>   int status;
>   socklen_t len = sizeof(status);
> 
>   /* enable pipe */
>   status = 1;
>   setsockopt(fd, SOL_PNPIPE, PNPIPE_ENABLE, &status, sizeof(status));
>   /* disable pipe */
>   status = 0;
>   setsockopt(fd, SOL_PNPIPE, PNPIPE_ENABLE, &status, sizeof(status));
>   /* get status */
>   getsockopt(fd, SOL_PNPIPE, PNPIPE_ENABLE, &status, &len);
> 
> This also fixes the error code from EFAULT to ENOTCONN.
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> Cc: Kumar Sanghvi <kumar.sanghvi@stericsson.com>
 
Thank you!

BR,
Kumar.

Re: [PATCH 2/4] Phonet: advise against enabling the pipe controller

[David Miller]

From: Rémi Denis-Courmont <remi@remlab.net>
Date: Fri,  8 Oct 2010 17:02:01 +0300

> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> 
> As it currently is, the new code path is not compatible with existing
> Nokia modems. This would break existing userspace for Nokia modem, such
> as the existing oFono ISI driver.
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

This really needs to be designed such that it can co-exist with current
userland technology.

Applied.

Re: [PATCH 2/4] Phonet: advise against enabling the pipe controller

[Rémi Denis-Courmont]

Le samedi 9 octobre 2010 00:17:30 David Miller, vous avez écrit :
> From: Rémi Denis-Courmont <remi@remlab.net>
> Date: Fri,  8 Oct 2010 17:02:01 +0300
> 
> > From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> > 
> > As it currently is, the new code path is not compatible with existing
> > Nokia modems. This would break existing userspace for Nokia modem, such
> > as the existing oFono ISI driver.
> > 
> > Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> 
> This really needs to be designed such that it can co-exist with current
> userland technology.

Yeah, don't tell me. This kinda static option works for Linux kernel on an 
embedded device like my employer. But it does not fly on the desktop.

This big patch came as a surprise, so did its merging. I don't when it can be 
improved.

-- 
Rémi Denis-Courmont
http://www.remlab.net/
http://fi.linkedin.com/in/remidenis

Re: [PATCH 2/4] Phonet: advise against enabling the pipe controller

[Kumar SANGHVI]

Hi,

On Fri, Oct 08, 2010 at 16:02:01 +0200, Rémi Denis-Courmont wrote:
> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> 
> As it currently is, the new code path is not compatible with existing
> Nokia modems. This would break existing userspace for Nokia modem, such
> as the existing oFono ISI driver.
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

Yes, the pipe controller logic is not intended for existing Nokia modems
that already implement pipe controller inside them.
Thank you for putting correct warning message.

Re: [PATCH 1/4] Phonet: add to MAINTAINERS and add myself

[David Miller]

From: Rémi Denis-Courmont <remi@remlab.net>
Date: Fri,  8 Oct 2010 17:02:00 +0300

> From: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>
> 
> Signed-off-by: Rémi Denis-Courmont <remi.denis-courmont@nokia.com>

Applied.