From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mailgw1.uni-kl.de ([131.246.120.220]:42608 "EHLO mailgw1.uni-kl.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752083AbbDGM3f (ORCPT ); Tue, 7 Apr 2015 08:29:35 -0400 Received: from itwm2.itwm.fhg.de (itwm2.itwm.fhg.de [131.246.191.3]) by mailgw1.uni-kl.de (8.14.4/8.14.4/Debian-7) with ESMTP id t37CTV39006387 for ; Tue, 7 Apr 2015 14:29:32 +0200 Date: Tue, 7 Apr 2015 14:29:30 +0200 From: Phoebe Buckheister Subject: Re: [PATCHv3 bluetooth-next 2/4] nl802154: add set interface cmd Message-ID: <20150407142930.662c0f92@zoidberg> In-Reply-To: <20150407122149.GA16415@omega> References: <1428407393-16005-1-git-send-email-alex.aring@gmail.com> <1428407393-16005-3-git-send-email-alex.aring@gmail.com> <20150407135902.2b885270@zoidberg> <20150407122149.GA16415@omega> MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-wpan-owner@vger.kernel.org List-ID: To: Alexander Aring Cc: linux-wpan@vger.kernel.org, kernel@pengutronix.de, mkl@pengutronix.de On Tue, 7 Apr 2015 14:21:52 +0200 Alexander Aring 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 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 > > > --- > > > 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, 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. > > If nobody complaint then I will send a v4 and remove the nl802154 > settings, or maybe somebody has a better idea. > > - Alex > > [0] http://www.spinics.net/lists/linux-wpan/msg01568.html