[OpenWrt-Devel] [PATCH 2/2] netifd: Don't call set_state for external device in device_claim

[Felix Fietkau]

On 2015-09-02 13:27, Hans Dedecker wrote:
> 
> 
> On Wed, Sep 2, 2015 at 10:31 AM, Felix Fietkau <nbd at openwrt.org
> <mailto:nbd at openwrt.org>> wrote:
> 
>     On 2015-09-01 15:53, Hans Dedecker wrote:
>     >
>     >
>     > On Tue, Sep 1, 2015 at 2:49 PM, Felix Fietkau <nbd at openwrt.org
>     <mailto:nbd at openwrt.org>
>     > <mailto:nbd at openwrt.org <mailto:nbd at openwrt.org>>> wrote:
>     >
>     >     On 2015-09-01 14:43, Hans Dedecker wrote:
>     >     > The function set_state disable is not called for external
>     devices
>     >     in device_release
>     >     > which means for external vlan/macvlan devices they won't be
>     deleted.
>     >     > As a result of this the set_state enable call for external
>     devices
>     >     by device_claim fails
>     >     > as vlan/macvlan devices cannot be created since the device
>     already
>     >     exists in the kernel.
>     >     > Therefore move the external device check from
>     device_set_state to
>     >     device_claim so
>     >     > external vlan/macvlan devices are not created again and can also
>     >     be external.
>     >     Why/how do vlan/macvlan devices become external?
>     >
>     > This use case is driven by an external application which is adding a
>     > vlan device to the bridge.
>     > Initially the vlan device is created as an internal device but not
>     added
>     > to the bridge; later added by an external application via ubus to the
>     > bridge. In this scenario we hit the issue when doing network
>     reload the
>     > vlan device is not anymore an active member of the bridge as vlan
>     > set_state enable was called which failed.
>     > This is a bit similar to a wireless device which has uci device
>     config;
>     > initially it's considered as an internal device by netifd when loading
>     > the config but becomes an external device when the wireless logic adds
>     > it to the bridge.
>     The main point of dev->external is to declare that a device is fully
>     managed externally, and netifd is not supposed to bring it up or down.
>     Maybe a better fix would be to allow devices to be added dynamically
>     without forcing dev->external on them (or just prevent that flag from
>     being added for vlan devices).
> 
> Devices can be added dynamically without being forced external via the
> function interface_handle_link; so that is covered. 
> Regarding the external flag being set for vlan devices; would it be
> acceptable the external flag can only be set  for simple devices in
> device_get ? This is the only place where non simple devices can be set
> external.
> My intention with this patch was also to line up the external flag check
> when the state is altered; in device_release the external flag is
> checked although the same check is done in set_device_state (which would
> eventually be called by the set_state callback function). So I thought
> to add the same external flag check in device_claim to line up both
> functions.
Okay, seems I didn't look at the context closely enough. I think this
patch makes sense after all, and I will apply it.

- Felix
_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/cgi-bin/mailman/listinfo/openwrt-devel