Message-ID: <20100929063219.GA18733@bnru01.bnr.st.com>
Date:	Wed, 29 Sep 2010 12:02:20 +0530
From:	Kumar SANGHVI <kumar.sanghvi@...ricsson.com>
To:	Rémi Denis-Courmont 
	<remi.denis-courmont@...ia.com>
Cc:	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	STEricsson_nomadik_linux <STEricsson_nomadik_linux@...t.st.com>,
	Sudeep DIVAKARAN <sudeep.divakaran@...ricsson.com>,
	Gulshan KARMANI <gulshan.karmani@...ricsson.com>,
	Linus WALLEIJ <linus.walleij@...ricsson.com>
Subject: Re: [PATCH v2 1/2] Phonet: Implement Pipe Controller to support
 Nokia Slim Modems

Hi Rémi Denis-Courmontt

On Wed, Sep 29, 2010 at 00:25:06 +0200, Rémi Denis-Courmont wrote:
 
> 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.
>
Agreed partially. In fact, we initially followed the same approach.
However, following this approach, we faced problem situations which I
have described in detail in the mail '[PATCH 0/2] Phonet: Implement Pipe
Controller to support Nokia Slim Modems'.

We can introduce corrections in Phonet stack for the PS path which
would be minor. But then those changes look more like a hack. Further,
this change would be required at several places where PS communication
is happening with modem e.g. pep_reply, pipe_skb_send, pipe_snd_status,
etc.

Further, if a new function tomorrow is introduced for PS path
communication with modem then, that hack will have to be added to that
function also.
 
> 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.
>
 
I am not much aware of modem used in Nokia N900 but I guess N900 modem
has pipe controller implemented inside it. So, the pipe 'data' works
fine with kernel.

If we use the same approach for Nokia Slim modems, this approach
introduces the problem of PS data traversing two times the phonet stack.
Again I have described this problem in the mail '[PATCH 0/2] Phonet:
Implement Pipe Controller to support Nokia Slim Modems'.

> 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.

Based on problems described above, we thought it would be better to
handle in kernel. Later on, any user-space can establish Pipe connection
with modem using this approach e.g. Video telephony socket can establish
its own pipe with modem, GPRS/3G socket can establish its own pipe with
modem.

Further, the pipe controller implementation allows the flexibility to
user-space of where it is sending the pipe 'data'.
Currently, the phonet stack by default is sending pipe 'data' to
dst_dev:dst_obj = 0x00:0x00 using the variable struct sockaddr_pn
pipe_srv.
Pipe controller implementation however maintains the remote-pep for any
particular user-space socket and always sends pipe data to that correct
destination remote-pep.

> 
> > @@ -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.

Agreed. I think I should rather use -EAGAIN.
I will correct this in all the rest of places where you have indicated
-EFAULT should not be used.
I will send out a correction patch since phonet pipe controller patch is already merged
by David.

> 
> > @@ -877,11 +1313,11 @@ static int pipe_skb_send(struct sock *sk, struct
> > sk_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 :-(

I apologise here as I accidently lost your commit here.
I will restore your commit here by sending patch.

> 
> -- 
> Rémi Denis-Courmont
> Nokia Devices R&D, Maemo Software, Helsinki

Thanks & regards,
Kumar.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists