On Tue, 7 Apr 2015 14:59:52 +0200
Alexander Aring <alex.aring@xxxxxxxxx> wrote:
> On Tue, Apr 07, 2015 at 02:29:30PM +0200, Phoebe Buckheister wrote:
> > On Tue, 7 Apr 2015 14:21:52 +0200
> > Alexander Aring <alex.aring@xxxxxxxxx> wrote:
> >
> > > Hi Phoebe,
> > >
> > > On Tue, Apr 07, 2015 at 01:59:02PM +0200, Phoebe Buckheister
> > > wrote:
> > > > On Tue, 7 Apr 2015 13:49:51 +0200
> > > > Alexander Aring <alex.aring@xxxxxxxxx> wrote:
> > > >
> > > > > This patch introduce the NL802154_CMD_SET_INTERFACE command
> > > > > which handles setting of all wpan interface mac attributes.
> > > > > his will introduce an easilier wpan mac settings handling in
> > > > > userspace application with nl802154.
> > > > >
> > > > > Signed-off-by: Alexander Aring <alex.aring@xxxxxxxxx>
> > > > > ---
> > > > > net/ieee802154/nl802154.c | 110
> > > > > ++++++++++++++++++++++++++++++++++++++++++++++ 1 file
> > > > > changed, 110 insertions(+)
> > > > >
> > > > > diff --git a/net/ieee802154/nl802154.c
> > > > > b/net/ieee802154/nl802154.c index c12c07f..e2f50ba 100644
> > > > > --- a/net/ieee802154/nl802154.c
> > > > > +++ b/net/ieee802154/nl802154.c
> > > > > @@ -603,6 +603,108 @@ static int nl802154_get_interface(struct
> > > > > sk_buff *skb, struct genl_info *info) return
> > > > > genlmsg_reply(msg, info); }
> > > > >
> > > > > +static int nl802154_set_interface(struct sk_buff *skb, struct
> > > > > genl_info *info) +{
> > > > > + struct cfg802154_registered_device *rdev =
> > > > > info->user_ptr[0];
> > > > > + struct net_device *dev = info->user_ptr[1];
> > > > > + struct wpan_dev *wpan_dev = dev->ieee802154_ptr;
> > > > > + int ret;
> > > > > +
> > > > > + if (info->attrs[NL802154_ATTR_PAN_ID]) {
> > > > > + __le16 pan_id;
> > > > > +
> > > > > + if (wpan_dev->iftype ==
> > > > > NL802154_IFTYPE_MONITOR)
> > > > > + return -EINVAL;
> > > > > +
> > > > > + if (netif_running(dev))
> > > > > + return -EBUSY;
> > > > > +
> > > > > + pan_id =
> > > > > nla_get_le16(info->attrs[NL802154_ATTR_PAN_ID]);
> > > > > + ret = rdev_set_pan_id(rdev, wpan_dev,
> > > > > pan_id);
> > > > > + if (ret < 0)
> > > > > + return ret;
> > > > > + }
> > > > > +
> > > > > + if (info->attrs[NL802154_ATTR_SHORT_ADDR]) {
> > > > > + __le16 short_addr;
> > > > > +
> > > > > + if (wpan_dev->iftype ==
> > > > > NL802154_IFTYPE_MONITOR)
> > > > > + return -EINVAL;
> > > >
> > > > This is not good. You may have partially applied the requested
> > > > settings when you return with an error, leaving the device in
> > > > some intermediate state that is most likely not useful. It'd be
> > > > better to first check whether all settings can be applied, and
> > > > then apply them all at once. But even then we have a problem
> > > > with rolling back some changes if a command fails :/
> > > >
> > >
> > > Yes, we need kind of rollback here if the command failed. I also
> > > stumble over this issue. [0] Mhh, we don't have it when I do each
> > > command in their own CMD SET call. So then I will leave the
> > > current behaviour as it is.
> > >
> > > Some idea:
> > >
> > > Solution would be on MAC settings that we really check if
> > > everything is valid. Then we can set everything in the pib
> > > values, after an interface up this values will be "really" set,
> > > like address filter settings in phy registers.
> > >
> > > This works for interface settings (MAC PIB values). But for phy
> > > values this is more difficult, because it's directly driver-layer
> > > calls. So we can't predigt if an driver layer return some error
> > > then.
> >
> > That sounds good. We could add another driver function that checks a
> > PIB/MIB and returns "valid" or "i'm sorry, dave", where "valid" may
> > only happen when the driver *knows* that it can load the *IB into
> > the hardware. Any possible hardware errors are excluded by these
> > checks,
>
> I think we need such functionality anyway, I wrote in my previous mail
> that the not _all_ valid 802.15.4 mac pib values are supported by real
> transceivers. Like the mrf24j40 [0] which can set the MIN_BE value
> only (I suppose MAX_BE is always 802.15.4 default). But nl802154
> checks only on 802.15.4 constraints only, so you can set everything
> which is allowed by 802.15.4. On a "doing interface up" it will fail
> because the driver layer will told -EINVAL; then and you don't know
> why (okay, experts knows why).
>
> We need to check this value on the SoftMAC layer in
> "net/mac802154/cfg.c".
>
> Now the design question is "a function" or "a array like
> channels_supported which extends the phy pib by us". I would prefer
> the array solution and put simple a u32 backoff_exponents_supported in
> phy_pib [1] and checking over flags if the transceiver supports it's
> or not.
>
> Then we can add more supported arrays for csma retries, frame retries,
> tx_pwr (also design question if all bands or current band only),
> etc...
>
> (For lbt mode, we don't need such functionality... it's only a bool
> and the HW flag is enough).
>
> Currently we don't have this kind of problem, because most settings
> can set on an at86rf230 transceiver only which is fully (and beyond)
> 802.15.4 complaint.
>
> Now the question is again:
>
> It's okay to extend the "supported arrays" in phy pib or we should
> introduce a driver_ops function? I prefer the array solution, because
> we have already such thing which is described by 802.15.4 pib.
Either is fine. The values permitted by the standard are mostly
bitmasks and min/max-pairs, so that actually makes more sense than a
check() function in each driver (until some driver doesn't fit that
model, but we should worry about that then).
>
> > but we'll only see those on the actual load anyway. Then we can load
> > the *IB in bulk and be sure that only the hardware can fail.
> >
>
> Then I would add such functionality to check if valid at first which I
> describe above.
>
> - Alex
>
> [0] http://ww1.microchip.com/downloads/en/DeviceDoc/39776C.pdf
> [1] http://lxr.free-electrons.com/source/include/net/cfg802154.h#L77
--
To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
