Re: [PATCH net-next v4 09/12] net: ethernet: aquantia: Call netdev_register after all initialized.

[Lino Sanfilippo]

On 20.02.2017 20:36, Pavel Belous wrote:
> From: Pavel Belous 
> 
> netdev_register should be called when everything is initialized.
> Also we should use net_device->reg_state field instead own
> "is_ndev_registered" flag to avoid any race.
> 
> Signed-off-by: Pavel Belous 
> ---
>  drivers/net/ethernet/aquantia/atlantic/aq_nic.c  | 10 +-
>  drivers/net/ethernet/aquantia/atlantic/aq_nic_internal.h |  1 -
>  2 files changed, 5 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c 
> b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> index a8a27c5..bce312a 100644
> --- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> +++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
> @@ -261,16 +261,16 @@ int aq_nic_ndev_register(struct aq_nic_s *self)
>   ether_addr_copy(self->ndev->dev_addr, mac_addr_permanent);
>   }
>  #endif
> - err = register_netdev(self->ndev);
> - if (err < 0)
> - goto err_exit;
>  
> - self->is_ndev_registered = true;
>   netif_carrier_off(self->ndev);
>  
>   for (i = AQ_CFG_VECS_MAX; i--;)
>   aq_nic_ndev_queue_stop(self, i);
>  
> + err = register_netdev(self->ndev);
> + if (err < 0)
> + goto err_exit;
> +
>  err_exit:
>   return err;
>  }
> @@ -293,7 +293,7 @@ void aq_nic_ndev_free(struct aq_nic_s *self)
>   if (!self->ndev)
>   goto err_exit;
>  
> - if (self->is_ndev_registered)
> + if (self->ndev->reg_state == NETREG_REGISTERED)
>   unregister_netdev(self->ndev);

Reviewed-by: Lino Sanfilippo 

A suggestion for a future patch (not necessarily part of this series):
Get rid of the above check at all. AFAICS aq_nic_ndev_free() is only
called in remove() and in probe(). In remove() the check is not needed, since it
always evaluates to true (device is always registered). In probe() it is only
needed for a clean unwind in case of error. The latter can also be done without
 the check in aq_nic_ndev_free().

Regards,
Lino

[PATCH net-next v4 09/12] net: ethernet: aquantia: Call netdev_register after all initialized.

[Pavel Belous]

From: Pavel Belous 

netdev_register should be called when everything is initialized.
Also we should use net_device->reg_state field instead own
"is_ndev_registered" flag to avoid any race.

Signed-off-by: Pavel Belous 
---
 drivers/net/ethernet/aquantia/atlantic/aq_nic.c  | 10 +-
 drivers/net/ethernet/aquantia/atlantic/aq_nic_internal.h |  1 -
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c 
b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
index a8a27c5..bce312a 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic.c
@@ -261,16 +261,16 @@ int aq_nic_ndev_register(struct aq_nic_s *self)
ether_addr_copy(self->ndev->dev_addr, mac_addr_permanent);
}
 #endif
-   err = register_netdev(self->ndev);
-   if (err < 0)
-   goto err_exit;
 
-   self->is_ndev_registered = true;
netif_carrier_off(self->ndev);
 
for (i = AQ_CFG_VECS_MAX; i--;)
aq_nic_ndev_queue_stop(self, i);
 
+   err = register_netdev(self->ndev);
+   if (err < 0)
+   goto err_exit;
+
 err_exit:
return err;
 }
@@ -293,7 +293,7 @@ void aq_nic_ndev_free(struct aq_nic_s *self)
if (!self->ndev)
goto err_exit;
 
-   if (self->is_ndev_registered)
+   if (self->ndev->reg_state == NETREG_REGISTERED)
unregister_netdev(self->ndev);
 
if (self->aq_hw)
diff --git a/drivers/net/ethernet/aquantia/atlantic/aq_nic_internal.h 
b/drivers/net/ethernet/aquantia/atlantic/aq_nic_internal.h
index f81738a..e7d2711 100644
--- a/drivers/net/ethernet/aquantia/atlantic/aq_nic_internal.h
+++ b/drivers/net/ethernet/aquantia/atlantic/aq_nic_internal.h
@@ -22,7 +22,6 @@ struct aq_nic_s {
unsigned int aq_vecs;
unsigned int packet_filter;
unsigned int power_state;
-   bool is_ndev_registered;
u8 port;
struct aq_hw_ops aq_hw_ops;
struct aq_hw_caps_s aq_hw_caps;
-- 
2.7.4