Re: [PATCH] ieee802154: add rx LQI from userspace

[ClÃment PÃron]

Hi Alexander, Stefan,

Thanks for your feedbacks,

On Mon, 9 Jul 2018 at 10:49, Stefan Schmidt <stefan@xxxxxxxxxxxxxxxxxx> wrote:
>
> Hello Clement.
>
> Finally coming to review the patch. Sorry for the delay.
>
> On 07.06.2018 16:08, ClÃment PÃron wrote:
> > From: Romuald CARI <romuald.cari@xxxxxxxxxxxx>
> >
> > The Link Quality Indication data exposed by drivers could not be accessed from
> > userspace. Since this data is per-datagram received, it makes sense to make it
> > available to userspace application through the ancillary data mechanism in
> > recvmsg rather than through ioctls. This can be activated using the socket
> > option WPAN_WANTLQI under SOL_IEEE802154 protocol.
>
> I can see that it makes the application life a lot easier to have data
> send out and LQI value synced up by using the socket approach instead of
> dealing with socket and ioctl's. I am good with this patch in general.
>
> So you have some public code that uses this approach? I would be
> interesting in the userspace part of yours. A demo would be fine. If the
> network handling part of your application is public anyway that would be
> even better. :-)

I'm sorry but the userspace code that use this isn't open.
I will check if I can share this part.
Just the idea is to compute an average LQI and when it reach a
threshold we allow the remote to control the device.

>
> As Alex mentiwe oned have some socket examples in the wpa-tools package
> to give people a head start when wanting to use the subsystem. Having
> such a simple example for the LQI feature would really give you bonus
> points. :-)

Indeed, add an example for this feature will be really interesting.

>
> https://github.com/linux-wpan/wpan-tools/tree/master/examples
>
> >
> > This LQI data is available in the ancillary data buffer under the SOL_IEEE802154
> > level as the type WPAN_LQI. The value is an unsigned byte indicating the link
> > quality with values ranging 0-255.
> >
> > Signed-off-by: Romuald Cari <romuald.cari@xxxxxxxxxxxx>
> > Signed-off-by: ClÃment Peron <clement.peron@xxxxxxxxxxxx>
> > ---
> >  include/net/af_ieee802154.h |  1 +
> >  net/ieee802154/socket.c     | 17 +++++++++++++++++
> >  2 files changed, 18 insertions(+)
> >
> > diff --git a/include/net/af_ieee802154.h b/include/net/af_ieee802154.h
> > index a5563d27a3eb..8003a9f6eb43 100644
> > --- a/include/net/af_ieee802154.h
> > +++ b/include/net/af_ieee802154.h
> > @@ -56,6 +56,7 @@ struct sockaddr_ieee802154 {
> >  #define WPAN_WANTACK         0
> >  #define WPAN_SECURITY                1
> >  #define WPAN_SECURITY_LEVEL  2
> > +#define WPAN_WANTLQI         3
> >
> >  #define WPAN_SECURITY_DEFAULT        0
> >  #define WPAN_SECURITY_OFF    1
> > diff --git a/net/ieee802154/socket.c b/net/ieee802154/socket.c
> > index a60658c85a9a..bc6b912603f1 100644
> > --- a/net/ieee802154/socket.c
> > +++ b/net/ieee802154/socket.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/termios.h>   /* For TIOCOUTQ/INQ */
> >  #include <linux/list.h>
> >  #include <linux/slab.h>
> > +#include <linux/socket.h>
> >  #include <net/datalink.h>
> >  #include <net/psnap.h>
> >  #include <net/sock.h>
> > @@ -452,6 +453,7 @@ struct dgram_sock {
> >       unsigned int bound:1;
> >       unsigned int connected:1;
> >       unsigned int want_ack:1;
> > +     unsigned int want_lqi:1;
> >       unsigned int secen:1;
> >       unsigned int secen_override:1;
> >       unsigned int seclevel:3;
> > @@ -486,6 +488,7 @@ static int dgram_init(struct sock *sk)
> >       struct dgram_sock *ro = dgram_sk(sk);
> >
> >       ro->want_ack = 1;
> > +     ro->want_lqi = 0;
> >       return 0;
> >  }
> >
> > @@ -713,6 +716,7 @@ static int dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> >       size_t copied = 0;
> >       int err = -EOPNOTSUPP;
> >       struct sk_buff *skb;
> > +     struct dgram_sock *ro = dgram_sk(sk);
> >       DECLARE_SOCKADDR(struct sockaddr_ieee802154 *, saddr, msg->msg_name);
> >
> >       skb = skb_recv_datagram(sk, flags, noblock, &err);
> > @@ -744,6 +748,13 @@ static int dgram_recvmsg(struct sock *sk, struct msghdr *msg, size_t len,
> >               *addr_len = sizeof(*saddr);
> >       }
> >
> > +     if (ro->want_lqi) {
> > +             err = put_cmsg(msg, SOL_IEEE802154, WPAN_WANTLQI,
> > +                            sizeof(uint8_t), &(mac_cb(skb)->lqi));
> > +             if (err)
> > +                     goto done;
> > +     }
> > +
>
> I am wondering a bit about the LQI you get back here. Maybe Alex can
> also shed some lights on it. The LQI value stored here is always from
> the last frame send (to any peer)? Or is it the last frame send to this
> specific peer?
>
> I have not put much thoughts into the LQI thing so far.Just thinking we
> need to be careful what we are providing to not let userspace make bad
> decisions (e.g. wrong routing changes due to link values given for a
> different peer connection).
>
> >       if (flags & MSG_TRUNC)
> >               copied = skb->len;
> >  done:
> > @@ -847,6 +858,9 @@ static int dgram_getsockopt(struct sock *sk, int level, int optname,
> >       case WPAN_WANTACK:
> >               val = ro->want_ack;
> >               break;
> > +     case WPAN_WANTLQI:
> > +             val = ro->want_lqi;
> > +             break;
> >       case WPAN_SECURITY:
> >               if (!ro->secen_override)
> >                       val = WPAN_SECURITY_DEFAULT;
> > @@ -892,6 +906,9 @@ static int dgram_setsockopt(struct sock *sk, int level, int optname,
> >       case WPAN_WANTACK:
> >               ro->want_ack = !!val;
> >               break;
> > +     case WPAN_WANTLQI:
> > +             ro->want_lqi = !!val;
> > +             break;
> >       case WPAN_SECURITY:
> >               if (!ns_capable(net->user_ns, CAP_NET_ADMIN) &&
> >                   !ns_capable(net->user_ns, CAP_NET_RAW)) {
> >
>
> Review wise I am happy with the patch. I will give it a test.

Thanks, for the review,
Clement

>
> regards
> Stefan Schmidt