RE: [PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
Hi Marcel.
> what happens for network triggered deactivation. Some networks here
> disconnect the GPRS context used MMS. Has some funny behavior that need
> to be taken into account.
Good question! When code reading with this in mind I realize that
we missed to mark the conn_info->interface as available by setting
cid = 0. This is bug, thank you for leading me to it.
FYI this is how CAIF and AT works together:
In order to have payload over CAIF there need to be
both a CAIF channel connected and a context activated with AT*EPPSD.
When Channel-ID configured for CAIF IP Interface and the Channel-Id given
in AT*EPPSD matches, the modem side will connect the CAIF Channel to
the Network Signaling Stack in the modem.
With this new patch CAIF channels are created statically from probe.
Activation and deactivation is controlled by AT*EPPSD, or notified by
+CGEV which will result in a CGACT status query.
If the network disconnects GPRS we should receive a
"+CGEV: NW DEACT", subsequent CGACT status query.
Here the connection (and CAIF Network Interface) should be
marked as unused by setting cid to zero.
> > +#define NLMSG_TAIL(nmsg) \
> > + ((struct rtattr *) (((void *) (nmsg)) + NLMSG_ALIGN((nmsg)-
> >nlmsg_len)))
>
> We have no global define this? You wanted to look into that. What was
> the outcome of it?
I did mention this before - I have looked around, but he did not find
any relevant macro for this unfortunately.
>
> > +struct iplink_req {
> > + struct nlmsghdr n;
> > + struct ifinfomsg i;
> > + char pad[1024];
>
> What is this huge pad for? It looks totally fishy to me.
The pad is there to hold the remaining of the rtnl attributes.
I'm working on restructuring this so that I can separate the
buffer holding the rtnl_message and the iplink_req.
I just have to make it work...
>
> > +static guint rtnl_watch;
>
> Get rid of the double spaces for rtnl_watch.
>
> > +static GIOChannel *channel;
>
> If we follow your convention here and not to overload variables this
> might be better named rtnl_channel.
>
> And yes, I am fine with keeping it since there is no way to connect a
> netlink socket properly to just use send(). You will require to use
> sendto for everything.
>
OK, Done.
>
> > +static void store_newlink_param(struct iplink_req *req, unsigned
> short type,
> > + int index, unsigned flags,
> > + unsigned change, struct ifinfomsg *msg,
> > + int bytes)
> > +{
> > + const char *ifname = NULL;
> > +
> > + get_ifname(msg, bytes, &ifname);
> > + strncpy(req->ifname, ifname,
> > + sizeof(req->ifname));
> > + req->ifname[sizeof(req->ifname)-1] = '\0';
> > + req->ifindex = index;
> > +}
>
> So I think that store_newlink_... and get_ifname can be nicely combined
> into one helper function. And using extract_parameters() function name
> is a bit better.
>
> And please only add parameters to that function that you really need in
> there. And the iplink_req should be last parameter.
>
> As one general is to have input parameters first and then the output
> parameters last. So read this something like extract values from this
> structure to this structure.
Yes, I agree. I have squashed the two functions together and cleaned up the
parameters.
This is much nicer, thank you.
>
> > +static int send_rtnl_req(struct iplink_req *req)
> > +{
> > + struct sockaddr_nl addr;
> > + int sk = g_io_channel_unix_get_fd(channel);
> > +
> > + memset(&addr, 0, sizeof(addr));
> > + addr.nl_family = AF_NETLINK;
> > +
> > + return sendto(sk, req, req->n.nlmsg_len, 0,
> > + (struct sockaddr *) &addr, sizeof(addr));
> > +}
>
> I don't think this should be a separate function. You use it only twice
> and having it close to the code using it would be better.
OK, good I have squashed these to functions as well.
>
> > +static struct iplink_req *find_request(guint32 seq)
> > +{
> > + GSList *list;
> > +
> > + for (list = pending_requests; list; list = list->next) {
> > + struct iplink_req *req = list->data;
> > +
> > + if (req->n.nlmsg_seq == seq)
> > + return req;
> > + }
> > +
> > + return NULL;
> > +}
>
> I would move this function up in this file at the top. It should go
> close to the variable declaration for pending_request.
OK, done.
>
> > +static void rtnl_client_response(struct iplink_req *req, int result)
> > +{
> > +
> > + if (req->callback && req->n.nlmsg_type == RTM_NEWLINK)
> > + req->callback(result, req->user_data,
> > + req->ifname, req->ifindex);
> > +
> > + pending_requests = g_slist_remove(pending_requests, req);
> > + g_free(req);
> > +}
>
> I don't like this split out. You already checked the nlmsg_type and
> here
> you keep checking it again just to reuse some code. This looks like
> waste to me.
I have squashed this into parse_rtnl_message, as you suggest
RE: [PATCH v4 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
Hi Marcel.
> Does anything cleans up the channel when oFono unexpectedly crashes?
No not yet. I suppose the interface will continue up until someone
destroys it. Should we simply destroy all CAIF interfaces
if oFono crashes? I think this would be the simplest approach.
BTW, how does connman behave in this case? Will it do any attempt
to clean up IP interfaces when it notices that oFono APIs has gone missing?
And what about modem state, is it taken back to flight-mode (AT+CFUN=0) in
order to reset everything?
If you don't mind, I would prefer to do restart handling as the next step.
It would be nice to finish this patch review first, what do you think?
>
> I think you can just use a stack buffer when you actually send the
> message. This is damn ugly and from a security point it actually scares
> me.
>
> Please only keep the seqnum in here. Since that is the only thing you
> need to find that pending request. Just get rid of everything that you
> don't need for the callback.
Yes, done.
> > I have squashed this into parse_rtnl_message, as you suggested.
> > I'm not really convinced this was any cleaner though..?
> > I end up with duplicating the code in the two if statements.
>
> We will see. I might have been wrong, but in my mind it just locked
> fine. I will take a second look when you send the updated patch.
OK, tell me what you think about the next patch then.
> > I think that the kernel implementation of rtnl will guarantee the
> > requests to be processed in order. rtnetlink_rcv in
> ..net/core/rtnetlink.c
> > takes the rtnl_lock before processing the netlink messages.
> > I believe this will guarantee that the NEWLINK responses will be
> received
> > in the same order as they are sent.
>
> it might be keeping the order, but I wouldn't count on it. And just
> using your find request function would make this code simpler.
The seqnr is not present in the message header of this response message,
so this will not be possible. I would prefer to stick with the current
implementation. Or do you see another way forward?
> > > > + req = find_request(hdr->nlmsg_seq);
> > > > + if (req == NULL)
> > > > + break;
> > > > +
> > > > + DBG("nlmsg error req");
> > > > + rtnl_client_response(req, -1);
> > > > + return;
> > > > + }
> > >
> > > If I understand this right, then you can always find the pending
> > > request
> > > and remove it here. No need for rtnl_client_response at all.
> >
> > Yes, I guess you are right. I can remove this callback.
> > Currently I am only actually interested in the result when
> > the interface is created successfully. Nothing particularly
> > is done in gprs-context.c if it fails.
>
> That is not what I meant. We should find that request and call the
> callback, but I think this can be done a more generic way.
>
> So whatever message you get, find the request that it belongs to. You
> need that request struct anyway. And then just reply here.
OK, I think I have done so. Have a look when I send the next patch.
> > Actually I think io_channel_read is just fine here. I think connman
> is doing
> > the same thing for rtnl. It is the sending part which is problematic.
>
> We should not be using that in ConnMan either actually. The _read is
> fine, but deprecated. The new _read_chars is doing buffering and a bit
> nasty in that area. So really just use recv() here.
OK, I've changed it to recv.
> > > So if we split out creating the NLMSG header into one helper
> functions,
> > > then you can easily prep the RTNL newlink message here inside the
> > > function.
> >
> > Even with creating this helper function you suggested
> > the prep_rtnl_newlink_req is on 45 lines. I think it makes sense
> > still keep then separate. It is no big deal for me, but I would
> prefer
> > to avoid such a large functions,
>
> As I said above, it looked good in my head. Maybe I was wrong, but in
> general this is pretty simple sequential stuff. So it can be in one
> function and still be easy readable. Lets have a look on how it looks
> and then decide.
OK, I've squashed the two functions.
>
> > > This will also allow you to remove req->loop_enabled variables
> since I
> > > don't see need for storing them.
> >
> > You are right, the loop_enabled variable is not currently needed.
> > However for various testing purposes we frequently want to run the
> CAIF Interface
> > in loopback mode (e.g. testing loopback to the modem). loopback_mode
> will make the
> > interface swap src and destination ip addresses so we actually can do
> ping etc.
> > If you are OK with this, I would prefer to keep this in order to make
> testing easier.
> > But again, I'm easy...
>
> Having loopback mode is fine. I just meant that you don't have to store
> the variable in the request struct since you build the request already.
>
> Please store only things in your request struct that you
RE: [PATCH v4 2/2] stemodem: Update gprs-context to use rtnl to create/remove interfaces.
Hi Marcel.
> Can you please not try to squeeze such change in the same patch. Just
> have a separate one that does this change.
...
> The more I looked through the rest of this patch, the more I thought
> you need to split this patch into a cleanup and caif_rtnl usage patch.
Yes, I have broken this into three patches.
It's a bit hard to find the exactly what to put where, and
tear apart what is already squashed. But have a look and
see what you think.
>
> So I realized now that you are using ifindex as input for
> caif_rtnl_delete_interface, now I am questioning why there is a result
> parameter in the callback for caif_rtnl_create_interface.
OK, agree - done.
> Why are we having this in the first place. CAIF network interfaces are
> point-to-point, right? In that case you should just leave this NULL.
Yes - agree. PtP link should not report GW.
> Please use '\0' instead of just 0. You might need to fix this at other
> places as well.
OK, done at least here..
> > +static void rtnl_callback(int result, gpointer user_data,
> > + char *ifname, int ifindex)
> > +{
> > + struct conn_info *conn = user_data;
> > +
> > + strncpy(conn->interface, ifname, sizeof(conn->interface));
> > + conn->ifindex = ifindex;
> > +
> > + if (result == 0)
> > + conn->created = TRUE;
> > + else {
> > + conn->created = FALSE;
> > + DBG("Could not create CAIF Interface");
> > + }
> > +}
>
> So here is the think. This makes no sense. Either you get an error and
> ifname and index are not valid or you succeed.
>
> So this should be something like
>
> if (ifname < 0) {
> connman_error(...)
> return;
> }
OK, I've done something similar.
Regards,
Sjur
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
ConnectionContext creation for LTE PDN Connection.
Hi all, We have started to investigate LTE impact on oFono, specifically how to integrate LTE and initial PDN connection, and we have some questions regarding this. This raises some questions about the handling of ConnectionContexts. When a terminal attach to LTE network it would normally do an implicit context activation request associated to attach request (+CGATT=1). When the context activation is completed this will result in an event notifying about the activated PDN connection according to 27.007 10.1.19 Packet Domain event reporting +CGEREP. +CGEV: ME PDN ACT At this point we have a "connection context" in the modem. The question is then, how should we model this in oFono? Should we assume/require that a Connection Context already is created by ConnMan and pre-exists in oFono (possibly with a flag indicating that this represents the initial PDN, or perhaps just taking the "internet" ConnectionContext)? Or should oFono automatically create a Connection Context? Furthermore, should oFono automatically proceed and set Active=true, bind the connection to a network interface and set network interface in state UP, or should the ConnectionContext initially set Active=false, waiting for ConnMan to activate it? Regards, Sjur ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
oFono release plan?
Hi Denis and Marcel. Do you have a release plan defined for oFono? When will you release oFono 1.0? Thanks, Regards Sjur ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RTNL sync or async when creating GPRS Context for STE modem.
Hi Marcel. I have finally got around to do some coding in oFono again, and have started to look into creating CAIF Network Interface using RTNL, instead using socket ioctls. I have started out by doing the RTNL handling async, i.e. registering the RTNL socket with g_io_add_watch etc. But I'm not really happy with the code as it feels unnecessary complex, introduces new states in gprs-context.c. etc. So I would like to do the RTNL handling synchronous instead, as this would reduce complexity. But I just wanted your opinion on this before I start refactoring my code. Do you see any issues with doing RTNL request synchronous i.e. sending out RTM_NEWLINK request and a waiting for RTM_NEWLINK response followed by NLMSG_ERROR, (or just NLMSG_ERROR if unsuccessful)? Regards Sjur ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: RTNL sync or async when creating GPRS Context for STE modem.
Rémi Denis-Courmont wrote: > > I have started out by doing the RTNL handling async, > > i.e. registering the RTNL socket with g_io_add_watch etc. > > But I'm not really happy with the code as it feels > > unnecessary complex, introduces new states in gprs-context.c. etc. > > Using an event loop makes sense if, and only if, the process blocks as > far as poll() is concerned. If your ioctl() calls or Netlink requests > do not enter the kernel scheduler at any point, then the glib main > loop will just slows things down - more code, more context switches. Thanks Denis, I think a synchronous implementation should be find then. If I understand RT-Netlink correctly, creating a CAIF Network Interface should not enter the poll() or wait in recvmsg. When sending RTNL newlink message, the whole newlink operation should execute in the senders thread, and response messages be ready in the rtnl sockets receive queue when the send operation completes. Hence read/poll should not block the thread. Regards /Sjur ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: oFono v0.22 test report
Hi Li, Li, Zhigang wrote: > The test will do full light functional validation, mainly focus on > checking implemented functionality of ofono v0.22 ... >Executed total 276 test cases during this round of testing, and among them: >Pass: 266 >Fail: 10 Are these all manual tests or are you using any automated test suite? Regards Sjur Brændeland ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [RFC] AGPS Support
Hi Bastian,
> Bastian, Waldo wrote:
>
> Please find attached a proposal for both a DBUS and Modem API for AGPS
> support. There are some minor changes compared to the proposal from
> May 14, 2010.
>
...
> void SendLCSFrame(string frametype, string framedata)
>
> Send a LCS position protocol frame to the Mobile
> Network. The LCS frame typically represents a
> Position Response.
>
> Valid frametypes are:
> rrlp_measure_position_response
> rrc_measurement_report
>
> The raw frame data is formatted as the concatenated
> sequence of the two digit hexadecimal representation
> of each of its octets. Example: "00FC2345"
> struct ofono_lcs_frame {
> enum ofono_lcs_frame_type lcs_frame_type;
> int frame_length; /* size of raw_frame in bytes */
> unsigned char* raw_frame;
> };
The 27.007 Spec specifies positioning request/responses for AT where
Positioning data are coded in XML data structures.
I think the oFono interface should not use binary data, but rather be aligned
with 27.007
and present decoded data as DBUS typed signals and methods with the same
information
content as specified in the XML data for AT+CPOS, AT+CPOSR.
In this way the vanilla AT driver could handle the CPOS, CPOR AT-commands
and code/decode between XML and explicit data structures, which in turn
oFono Core could code/decode as DBUS data types.
Regards,
Sjur Brændeland
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCHv2 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
> From: Marcel Holtmann [mailto:mar...@holtmann.org]
>
> just to be clear. You want to name it like this? I am just asking here
> since I made a few proposals.
No worries :-)
The file is called caif_rtnl and exported functions are prefixed caif_rtnl,
I think that makes a lot of sense.
>
> > +#define NLMSG_TAIL(nmsg) \
> > + ((struct rtattr *) (((void *) (nmsg)) + NLMSG_ALIGN((nmsg)-
> >nlmsg_len)))
>
> We don't have this one as part of some kernel includes?
I'll check what I can find ;-)
>
> > +static gboolean get_ifname(struct ifinfomsg *msg, int bytes,
> > + const char **ifname)
> > +{
> > + struct rtattr *attr;
> > +
> > + for (attr = IFLA_RTA(msg); RTA_OK(attr, bytes);
> > + attr = RTA_NEXT(attr, bytes))
> > +
> > + if (attr->rta_type == IFLA_IFNAME &&
> > + ifname != NULL) {
> > + *ifname = RTA_DATA(attr);
> > + return TRUE;
> > + }
> > +
> > + return FALSE;
> > +}
>
> After I stared long enough at it, I realized that the code is fine, but
> that was not obvious to me. In this case please use for () { } with the
> curly braces around it. It improves readability.
>
> And I know that other times we don't do { } for single statements, but
> here is does helps my poor human brain ;)
Sure I'll add the braces ;-)
> You don't really have to play this append/remove game. You can just
> append it after send_rtnl_req succeed. The read goes via the mainloop
> anyway and we are a single threaded program. So the order is
> guaranteed.
I'm not sure about this. I need to keep track of what
connection-id I assign to the CAIF Interface in the request and what
interface name and interface index I get for that connection id
in the RTNL response.
This mapping between network interface and connection id is used
when PDP Contexts are activated. The append/remove game provides me
with the request-response mapping.
> And here you should add an extra empty line.
Sorry, I don't know why I keep doing that...
> > +struct rtnl_rsp_param {
> > + int result;
> > + gpointer user_data;
> > + char ifname[IF_NAMESIZE];
> > + int ifindex;
> > +};
> > +
> > +struct rtnl_req_param {
> > + void (*callback)(struct rtnl_rsp_param *param);
> > + gpointer user_data;
> > + int type;
> > + int conn_id;
> > + gboolean loop_enabled;
> > +};
> > +
> > +extern int caif_rtnl_create_interface(struct rtnl_req_param *req);
> > +extern int caif_rtnl_delete_interface(int ifid);
>
> I am not really sold on this param stuct thingy btw. Why do you need
> them? Just proper input params and output params for the callback
> handler would do them same. Won't they?
Yes, I could do that. But in that case I would prefer to typedef the
response function to simplify create function's signature. Otherwise
I think the create function's signature gets too complex.
...
> I do need another look at the RTNL magic and casting. That always
> drives
> my crazy when I look at that. In the meantime, please address these
> details first.
Sure, and thanks for reviewing.
Regards,
Sjur
___
ofono mailing list
ofono@ofono.org
http://lists.ofono.org/listinfo/ofono
RE: [PATCHv2 1/2] stemodem: Add RTNL functionality managing CAIF Network Interfaces.
Hi Marcel. > > +static GSList *pending_requests; > > +static GIOChannel *channel; > > +static guint32 rtnl_seqnr; > > +static guint rtnl_watch; > > To be fair, you don't need both, the GIOChannel and the watch. You can > just add the watch and then unref the GIOChannel and set it to close on > unref. That way when the watch gets removed or you remove it, the > GIOChannel and the underlaying socket will be automatically closed as > well. > As described above. Just unref the GIOChannel and forget about it. And > then you can make the GIOChannel a local variable. I think I have to keep the channel, because I do need the file descriptor derived from the channel when doing sendto. > > > You don't really have to play this append/remove game. You can just > > > append it after send_rtnl_req succeed. The read goes via the > mainloop > > > anyway and we are a single threaded program. So the order is > > > guaranteed. ... > You don't need to append it first, send the message, check error and in > case of an error remove it again. OK, thanks - now I get what your after. I completely misunderstood your previous comment, my bad. > And while we are at it. Essentially you also need to use asynchronous > and non-blocking watch driven sending of your message in the first > place. I did forget to check if you open the RTNL socket non-blocking, > but you should be doing that. So I'll add something like this then: fl = fcntl(sk, F_GETFL); fcntl(sk, F_SETFL, fl | O_NONBLOCK); > > + } > > + > > + g_slist_free(pending_requests); > > + pending_requests = NULL; > > No need to set pending_requests to NULL here. But I will have to move this to the caif_rtnl_init function then, I think pending_requests needs to be NULL before using it. > > > I am not really sold on this param stuct thingy btw. Why do you > need > > > them? Just proper input params and output params for the callback > > > handler would do them same. Won't they? > > > > Yes, I could do that. But in that case I would prefer to typedef the > > response function to simplify create function's signature. Otherwise > > I think the create function's signature gets too complex. > > typedef for function callbacks are just fine. I have no problem with > that at all. I got rid of the structs and typedef'ed response function - I agree this better. typedef void (*rtnl_create_cb_t) (int result, gpointer user_data, char *ifname, int ifindex); extern int caif_rtnl_create_interface(gpointer user_data, int type, int connid, gboolean loop, rtnl_create_cb_t cb); Hoping to get the next patch out later today. Regards, Sjur ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [RFC] plugin/ste: Use D-Bus API from Modem Init Daemon for autoconfig.
Hi Marcel, > > This patch introduces auto discovery of ST-Ericsson modems. > > ST-Ericsson modems are managed by a Modem Init Daemon which > > is responsible for start/stop/restart flashing etc. The > > STE plugin monitors the modem state exposed from the > > Modem Init Damon Dbus API. When the modem is in state "on" > > the STE modem is created and registered. > > > > The reason for not using the standard udev paradigm is that > > the CAIF device is up before the modem is ready to setup AT channels. > > For flashless modems CAIF is used as part of the boot. The Modem Init > > Daemon is managing the flashless boot procedure and sets the State to > > "on" when the modem is available. > > so I don't like this at all. This looks pretty nasty and like an ugly > hack to get something working. > > Please show us what Modem Init Daemon is actually doing. As mentioned above the Modem Init Daemon is responsible for: - Toggling the different GPIOs, powering the modem on/off. - Boot the modem and upload the firmware in several stages. a) Initial Z-Protocol, for handshaking the modem b) PROTROM boot protocol to upload modem firmware. c) CAIF Remote File System protocol for further firmware loading (modules) d) CAIF Remote File System protocol for serving modem file system. - Monitoring the GPIOs for modem restart and act upon this. - Enabling/Disabling the CAIF Link Layer Interface according to the modem state (GPIO). - Monitoring Thermal and Battery Warnings URC over AT and acting upon this. - In case of modem crash, the crash-dump from the modem must be downloaded to host. This needs to be synchronized so that the reboot of the modem is not started before the dump is complete. - Modem Init daemon has to expose an API in for: o triggering restart o signal modem status o initiate upgrade The Modem Init Daemon will be available in product quality under Apache License, and will be in use for several platforms (not just MeeGo) and modem versions. The issue we have is that when the CAIF Link Layer Interface is up it does not necessarily mean that the Modem is fully started. So we need to have a way to synchronize other services and modem clients when the modem is ready. The other services that needs this ready notification are: oFono, Remote File Manager, Logging Daemon. On other platforms than MeeGo there are AT based clients such as Audio, AGPS, RIL as well. We definitely need a synchronization mechanism between Modem Init Daemon and the other services, so we decided to use a D-Bus API for the Modem Init Daemon to expose the modem state and an API for initiating power-off, upgrade and reboot. Regards, Sjur ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
