[ipxe-devel] [PATCH 1/2] Prevent a netdevice (VLAN) to open/close twice.

[Wissam Shoukair]

I wanted to do this fix (that you attached), but I saw in the patch 8ab2f51997db80c88d098844ad5a9af5736d1c9e (Mark devices as open only if opening succeeds) that this fixed some issue in the netdev_close, so I wanted to go more on the safe side and add 2 more states also for the following reason:

In the close flow:
call netdev_close(vlan_dev) ->
     mark vlan_dev as closed
     call vlan_close(vlan_dev) ->
        call netdev_notify(vlan dev)
             call vlan_sync(trunk_dev) ->
                   Open vlan because the trunk is open (not yet closed).

In netdev_close() the notify comes before the close operation of the VLAN. (correct me if I'm wrong)

...
        /* Mark as closed */
        netdev->state &= ~NETDEV_OPEN;

        /* Notify drivers of device state change */
        netdev_notify ( netdev );

        /* Close the device */
        netdev->op->close ( netdev );
...


Do you think that if the notify comes after the close operation it won't be an issue?

Thanks,
Wissam

-----Original Message-----
From: Michael Brown [mailto:mcb30 at ipxe.org] 
Sent: יום ד 05 מרץ 2014 16:02
To: Wissam Shoukair; ipxe-devel at ipxe.org
Subject: Re: [PATCH 1/2] Prevent a netdevice (VLAN) to open/close twice.

On 05/03/14 13:33, Wissam Shoukair wrote:
> The reason for this fix is, when opening a VLAN, the current code 
> checks if the trunk interface in also open. If it's not opened, then 
> it will open the trunk and notify all other drivers, which the VLAN 
> driver is one of them, and when the VLAN driver is notified it will 
> try to sync with the opened trunk, so it will be opened again. (same 
> for the close flow)

Does the attached patch fix your problem?  This marks the device as open before calling the ->open() method, and so prevents vlan_sync() from attempting to (harmlessly) reopen the child VLAN device.

I don't think the same problem exists on the close path:

   call netdev_close(vlan_dev) ->
     mark vlan_dev as closed
     call vlan_close(vlan_dev) ->
       call netdev_close(trunk_dev)
       mark trunk_dev as closed
       call netdev_notify(trunk_dev) ->
         call vlan_sync(trunk_dev) ->
           both devices are marked as closed, so no action taken

   call netdev_close(trunk_dev) ->
     mark trunk_dev as closed
     call netdev_notify(trunk_dev) ->
       call vlan_sync(trunk_dev) ->
         call netdev_close(vlan_dev) ->
           call netdev_close(trunk_dev) ->
             trunk_dev is already marked as closed, so no action taken

Michael