> On 23 Mar 2016, at 12:36 AM, Masao Uebayashi <[email protected]> wrote:
>
> On Tue, Mar 22, 2016 at 09:36:18PM +1000, David Gwynne wrote:
>> this basically makes the code use if_get instead of carrying a
>> pointer around. this is as mechanical as i can make it.
>>
>> ok?
>>
>> Index: if_vlan_var.h
>> ===================================================================
>> RCS file: /cvs/src/sys/net/if_vlan_var.h,v
>> retrieving revision 1.33
>> diff -u -p -r1.33 if_vlan_var.h
>> --- if_vlan_var.h 14 Mar 2016 03:48:47 -0000 1.33
>> +++ if_vlan_var.h 22 Mar 2016 11:34:45 -0000
>> @@ -61,9 +61,8 @@ struct vlan_mc_entry {
>>
>> struct ifvlan {
>> struct arpcom ifv_ac; /* make this an interface */
>> - struct ifnet *ifv_p; /* parent interface of this vlan */
>> + unsigned int ifv_p; /* parent interface of this vlan */
>> struct ifv_linkmib {
>> - int ifvm_parent;
>> u_int16_t ifvm_proto; /* encapsulation ethertype */
>> u_int16_t ifvm_tag; /* tag to apply on packets leaving if */
>> u_int16_t ifvm_prio; /* prio to apply on packet leaving if */
>> Index: if_vlan.c
>> ===================================================================
>> RCS file: /cvs/src/sys/net/if_vlan.c,v
>> retrieving revision 1.155
>> diff -u -p -r1.155 if_vlan.c
>> --- if_vlan.c 18 Mar 2016 02:40:04 -0000 1.155
>> +++ if_vlan.c 22 Mar 2016 11:34:45 -0000
>> @@ -235,7 +235,12 @@ vlan_start(struct ifnet *ifp)
>> uint8_t prio;
>>
>> ifv = ifp->if_softc;
>> - ifp0 = ifv->ifv_p;
>> + ifp0 = if_get(ifv->ifv_p);
>> + if (ifp0 == NULL || (ifp0->if_flags & (IFF_UP|IFF_RUNNING)) !=
>> + (IFF_UP|IFF_RUNNING)) {
>> + ifq_purge(&ifp->if_snd);
>
> You can't dereference ifp if (ifp0 == NULL)?
ifp is a pointer to the vlan interface, and ifp0 is supposed to be its parent.
the vlan interface can exist after its parent is detached.
>
>> + goto leave;
>> + }
>>
>> for (;;) {
>> IFQ_DEQUEUE(&ifp->if_snd, m);
>> @@ -247,12 +252,6 @@ vlan_start(struct ifnet *ifp)
>> bpf_mtap_ether(ifp->if_bpf, m, BPF_DIRECTION_OUT);
>> #endif /* NBPFILTER > 0 */
>>
>> - if ((ifp0->if_flags & (IFF_UP|IFF_RUNNING)) !=
>> - (IFF_UP|IFF_RUNNING)) {
>> - ifp->if_oerrors++;
>> - m_freem(m);
>> - continue;
>> - }
>>
>> /* IEEE 802.1p has prio 0 and 1 swapped */
>> prio = m->m_pkthdr.pf.prio;
>> @@ -290,6 +289,9 @@ vlan_start(struct ifnet *ifp)
>> }
>> ifp->if_opackets++;
>> }
>> +
>> +leave:
>> + if_put(ifp0);
>> }
>>
>> struct mbuf *
>> @@ -358,7 +360,7 @@ vlan_input(struct ifnet *ifp0, struct mb
>>
>> list = &tagh[TAG_HASH(tag)];
>> SRPL_FOREACH(ifv, list, &i, ifv_list) {
>> - if (ifp0 == ifv->ifv_p && tag == ifv->ifv_tag &&
>> + if (ifp0->if_index == ifv->ifv_p && tag == ifv->ifv_tag &&
>> etype == ifv->ifv_type)
>> break;
>> }
>> @@ -417,13 +419,13 @@ vlan_config(struct ifvlan *ifv, struct i
>>
>> if (ifp0->if_type != IFT_ETHER)
>> return EPROTONOSUPPORT;
>> - if (ifv->ifv_p == ifp0 && ifv->ifv_tag == tag) /* noop */
>> + if (ifp0->if_index == ifv->ifv_p && ifv->ifv_tag == tag) /* noop */
>> return (0);
>>
>> /* Remember existing interface flags and reset the interface */
>> flags = ifv->ifv_flags;
>> vlan_unconfig(&ifv->ifv_if, ifp0);
>> - ifv->ifv_p = ifp0;
>> + ifv->ifv_p = ifp0->if_index;
>> ifv->ifv_if.if_baudrate = ifp0->if_baudrate;
>>
>> if (ifp0->if_capabilities & IFCAP_VLAN_MTU) {
>> @@ -509,8 +511,9 @@ vlan_unconfig(struct ifnet *ifp, struct
>> struct ifnet *ifp0;
>>
>> ifv = ifp->if_softc;
>> - if ((ifp0 = ifv->ifv_p) == NULL)
>> - return 0;
>> + ifp0 = if_get(ifv->ifv_p);
>> + if (ifp0 == NULL)
>> + goto disconnect;
>>
>> /* Unset promisc mode on the interface and its parent */
>> if (ifv->ifv_flags & IFVF_PROMISC) {
>> @@ -544,8 +547,9 @@ vlan_unconfig(struct ifnet *ifp, struct
>> */
>> vlan_multi_apply(ifv, ifp0, SIOCDELMULTI);
>>
>> +disconnect:
>> /* Disconnect from parent. */
>> - ifv->ifv_p = NULL;
>> + ifv->ifv_p = 0;
>> ifv->ifv_if.if_mtu = ETHERMTU;
>> ifv->ifv_if.if_hardmtu = ETHERMTU;
>> ifv->ifv_flags = 0;
>
> So you're changing error code path here? I'm not sure this correctness
> but I'd change this separately.
its not really an error path, its cleanup. again, if the parent has detached
then this is where we need to reset the vlan interface state.
>
>> @@ -557,6 +561,8 @@ vlan_unconfig(struct ifnet *ifp, struct
>> bzero(LLADDR(sdl), ETHER_ADDR_LEN);
>> bzero(ifv->ifv_ac.ac_enaddr, ETHER_ADDR_LEN);
>>
>> + if_put(ifp0);
>> +
>> return (0);
>> }
>>
>> @@ -564,12 +570,22 @@ void
>> vlan_vlandev_state(void *v)
>> {
>> struct ifvlan *ifv = v;
>> + struct ifnet *ifp0;
>> + int link_state = LINK_STATE_DOWN;
>> + uint64_t baudrate = 0;
>>
>> - if (ifv->ifv_if.if_link_state == ifv->ifv_p->if_link_state)
>> + ifp0 = if_get(ifv->ifv_p);
>> + if (ifp0 != NULL) {
>> + link_state = ifp0->if_link_state;
>> + baudrate = ifp0->if_baudrate;
>> + }
>> + if_put(ifp0);
>> +
>> + if (ifv->ifv_if.if_link_state == link_state)
>> return;
>>
>> - ifv->ifv_if.if_link_state = ifv->ifv_p->if_link_state;
>> - ifv->ifv_if.if_baudrate = ifv->ifv_p->if_baudrate;
>> + ifv->ifv_if.if_link_state = link_state;
>> + ifv->ifv_if.if_baudrate = baudrate;
>> if_link_state_change(&ifv->ifv_if);
>> }
>>
>
> I don't follow the logic here...
the vlans link state is reported as down unless it is attached to a valid
parent, in which case it shadows the parents state. we dont report changes
unless they're different to the current state.
dlg
