Re: [patch net-next 1/4] net: introduce new priv_flag indicating iface capable of change mac when running

2012-06-29 Thread Jiri Pirko
Fri, Jun 29, 2012 at 12:17:34AM CEST, bhutchi...@solarflare.com wrote:
On Thu, 2012-06-28 at 16:10 +0200, Jiri Pirko wrote:
 Introduce IFF_LIFE_ADDR_CHANGE priv_flag and use it to disable
 netif_running() check in eth_mac_addr()

 Signed-off-by: Jiri Pirko jpi...@redhat.com
 ---
  include/linux/if.h |2 ++
  net/ethernet/eth.c |2 +-
  2 files changed, 3 insertions(+), 1 deletion(-)
 
 diff --git a/include/linux/if.h b/include/linux/if.h
 index f995c66..fd9ee7c 100644
 --- a/include/linux/if.h
 +++ b/include/linux/if.h
 @@ -81,6 +81,8 @@
  #define IFF_UNICAST_FLT 0x2 /* Supports unicast filtering   
 */
  #define IFF_TEAM_PORT   0x4 /* device used as team port */
  #define IFF_SUPP_NOFCS  0x8 /* device supports sending 
 custom FCS */
 +#define IFF_LIFE_ADDR_CHANGE 0x10   /* device supports hardware 
 address
 + * change when it's running */
[...]

Any device that has IFF_UNICAST_FLT can update the unicast MAC filter
while it's running; doesn't that go hand-in-hand with being able to
handle changes to the primary MAC address?  Is the new flag really
necessary at all?

Hmm, this makes sense. But, can you guarantee that all devices behave like this?

Also, there are many devices that does not support unicast filtering
and yet they support updating mac adress while running.

Jirka


Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


[patch net-next 1/4] net: introduce new priv_flag indicating iface capable of change mac when running

2012-06-28 Thread Jiri Pirko
Introduce IFF_LIFE_ADDR_CHANGE priv_flag and use it to disable
netif_running() check in eth_mac_addr()

Signed-off-by: Jiri Pirko jpi...@redhat.com
---
 include/linux/if.h |2 ++
 net/ethernet/eth.c |2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/include/linux/if.h b/include/linux/if.h
index f995c66..fd9ee7c 100644
--- a/include/linux/if.h
+++ b/include/linux/if.h
@@ -81,6 +81,8 @@
 #define IFF_UNICAST_FLT0x2 /* Supports unicast filtering   
*/
 #define IFF_TEAM_PORT  0x4 /* device used as team port */
 #define IFF_SUPP_NOFCS 0x8 /* device supports sending custom FCS */
+#define IFF_LIFE_ADDR_CHANGE 0x10  /* device supports hardware address
+* change when it's running */
 
 
 #define IF_GET_IFACE   0x0001  /* for querying only */
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index 36e5880..8f8ded4 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -283,7 +283,7 @@ int eth_mac_addr(struct net_device *dev, void *p)
 {
struct sockaddr *addr = p;
 
-   if (netif_running(dev))
+   if (!(dev-priv_flags  IFF_LIFE_ADDR_CHANGE)  netif_running(dev))
return -EBUSY;
if (!is_valid_ether_addr(addr-sa_data))
return -EADDRNOTAVAIL;
-- 
1.7.10.4

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [patch net-next 1/4] net: introduce new priv_flag indicating iface capable of change mac when running

2012-06-28 Thread Eric Dumazet
On Thu, 2012-06-28 at 16:10 +0200, Jiri Pirko wrote:
 Introduce IFF_LIFE_ADDR_CHANGE priv_flag and use it to disable
 netif_running() check in eth_mac_addr()
 
 Signed-off-by: Jiri Pirko jpi...@redhat.com
 ---
  include/linux/if.h |2 ++
  net/ethernet/eth.c |2 +-
  2 files changed, 3 insertions(+), 1 deletion(-)
 
 diff --git a/include/linux/if.h b/include/linux/if.h
 index f995c66..fd9ee7c 100644
 --- a/include/linux/if.h
 +++ b/include/linux/if.h
 @@ -81,6 +81,8 @@
  #define IFF_UNICAST_FLT  0x2 /* Supports unicast filtering   
 */
  #define IFF_TEAM_PORT0x4 /* device used as team port */
  #define IFF_SUPP_NOFCS   0x8 /* device supports sending 
 custom FCS */
 +#define IFF_LIFE_ADDR_CHANGE 0x10/* device supports hardware 
 address
 +  * change when it's running */
  
 
  #define IF_GET_IFACE 0x0001  /* for querying only */
 diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
 index 36e5880..8f8ded4 100644
 --- a/net/ethernet/eth.c
 +++ b/net/ethernet/eth.c
 @@ -283,7 +283,7 @@ int eth_mac_addr(struct net_device *dev, void *p)
  {
   struct sockaddr *addr = p;
  
 - if (netif_running(dev))
 + if (!(dev-priv_flags  IFF_LIFE_ADDR_CHANGE)  netif_running(dev))
   return -EBUSY;
   if (!is_valid_ether_addr(addr-sa_data))
   return -EADDRNOTAVAIL;

Since the memcpy() is not atomic, there is a small window where a reader
could get a half-changed mac address. I guess its a detail.



___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [patch net-next 1/4] net: introduce new priv_flag indicating iface capable of change mac when running

2012-06-28 Thread Michael S. Tsirkin
On Thu, Jun 28, 2012 at 05:24:52PM +0200, Eric Dumazet wrote:
 On Thu, 2012-06-28 at 16:10 +0200, Jiri Pirko wrote:
  Introduce IFF_LIFE_ADDR_CHANGE priv_flag and use it to disable
  netif_running() check in eth_mac_addr()
  
  Signed-off-by: Jiri Pirko jpi...@redhat.com
  ---
   include/linux/if.h |2 ++
   net/ethernet/eth.c |2 +-
   2 files changed, 3 insertions(+), 1 deletion(-)
  
  diff --git a/include/linux/if.h b/include/linux/if.h
  index f995c66..fd9ee7c 100644
  --- a/include/linux/if.h
  +++ b/include/linux/if.h
  @@ -81,6 +81,8 @@
   #define IFF_UNICAST_FLT0x2 /* Supports unicast filtering   
  */
   #define IFF_TEAM_PORT  0x4 /* device used as team port */
   #define IFF_SUPP_NOFCS 0x8 /* device supports sending 
  custom FCS */
  +#define IFF_LIFE_ADDR_CHANGE 0x10  /* device supports hardware 
  address
  +* change when it's running */
   
  
   #define IF_GET_IFACE   0x0001  /* for querying only */
  diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
  index 36e5880..8f8ded4 100644
  --- a/net/ethernet/eth.c
  +++ b/net/ethernet/eth.c
  @@ -283,7 +283,7 @@ int eth_mac_addr(struct net_device *dev, void *p)
   {
  struct sockaddr *addr = p;
   
  -   if (netif_running(dev))
  +   if (!(dev-priv_flags  IFF_LIFE_ADDR_CHANGE)  netif_running(dev))
  return -EBUSY;
  if (!is_valid_ether_addr(addr-sa_data))
  return -EADDRNOTAVAIL;
 
 Since the memcpy() is not atomic, there is a small window where a reader
 could get a half-changed mac address. I guess its a detail.
 

At least for virtio nothing changes - we had this bug forever.
How'd you fix this?

-- 
MST
___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization


Re: [patch net-next 1/4] net: introduce new priv_flag indicating iface capable of change mac when running

2012-06-28 Thread Ben Hutchings
On Thu, 2012-06-28 at 16:10 +0200, Jiri Pirko wrote:
 Introduce IFF_LIFE_ADDR_CHANGE priv_flag and use it to disable
 netif_running() check in eth_mac_addr()

 Signed-off-by: Jiri Pirko jpi...@redhat.com
 ---
  include/linux/if.h |2 ++
  net/ethernet/eth.c |2 +-
  2 files changed, 3 insertions(+), 1 deletion(-)
 
 diff --git a/include/linux/if.h b/include/linux/if.h
 index f995c66..fd9ee7c 100644
 --- a/include/linux/if.h
 +++ b/include/linux/if.h
 @@ -81,6 +81,8 @@
  #define IFF_UNICAST_FLT  0x2 /* Supports unicast filtering   
 */
  #define IFF_TEAM_PORT0x4 /* device used as team port */
  #define IFF_SUPP_NOFCS   0x8 /* device supports sending 
 custom FCS */
 +#define IFF_LIFE_ADDR_CHANGE 0x10/* device supports hardware 
 address
 +  * change when it's running */
[...]

Any device that has IFF_UNICAST_FLT can update the unicast MAC filter
while it's running; doesn't that go hand-in-hand with being able to
handle changes to the primary MAC address?  Is the new flag really
necessary at all?

Ben.

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

___
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization