Re: [PATCHv3 bluetooth-next 2/4] nl802154: add set interface cmd

[Varka Bhadram <varkabhadram@gmail.com>]

On 04/07/2015 05:51 PM, 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 <alex.aring@gmail.com> 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@gmail.com>
>>> ---
>>>  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.
>
>
> 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
> --
> To unsubscribe from this list: send the line "unsubscribe linux-wpan" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Instead of this its better to leave the current behavior as it is.

Why do we need to complicate the process...?

-- 
Varka Bhadram