Re: [PATCH V8 net-next 05/11] net: hibmcge: Implement some .ndo functions

[Jijie Shao]

on 2024/9/9 11:05, Kalesh Anakkur Purayil wrote:
> On Mon, Sep 9, 2024 at 8:11 AM Jijie Shao <shaojijie@xxxxxxxxxx> wrote:
> > +}
> > +
> > +static int hbg_net_open(struct net_device *netdev)
> > +{
> > +       struct hbg_priv *priv = netdev_priv(netdev);
> > +
> > +       if (test_and_set_bit(HBG_NIC_STATE_OPEN, &priv->state))
> > +               return 0;
> [Kalesh] Is there a possibility that dev_open() can be invoked twice?

We want stop NIC when chang_mtu 、self_test or FLR.
So, driver will directly invoke hbg_net_stop() not dev_open() if need.
Therefore, driver must ensure that hbg_net_open() or hbg_net_stop() can not be invoked twice.

> > +
> > +       hbg_all_irq_enable(priv, true);
> > +       hbg_hw_mac_enable(priv, HBG_STATUS_ENABLE);
> > +       netif_start_queue(netdev);
> > +       hbg_phy_start(priv);
> > +
> > +       return 0;
> > +}
> > +
> > +static int hbg_net_stop(struct net_device *netdev)
> > +{
> > +       struct hbg_priv *priv = netdev_priv(netdev);
> > +
> > +       if (!hbg_nic_is_open(priv))
> > +               return 0;
> [Kalesh] Is there any reason to not check HBG_NIC_STATE_OPEN here?

Actually, hbg_nic_is_open() is used to check HBG_NIC_STATE_OPEN.
:
#define hbg_nic_is_open(priv) test_bit(HBG_NIC_STATE_OPEN, &(priv)->state)

> > +
> > +       clear_bit(HBG_NIC_STATE_OPEN, &priv->state);
> > +
> > +       hbg_phy_stop(priv);
> > +       netif_stop_queue(netdev);
> > +       hbg_hw_mac_enable(priv, HBG_STATUS_DISABLE);
> > +       hbg_all_irq_enable(priv, false);
> > +
> > +       return 0;
> > +}
> > +
> > +static int hbg_net_change_mtu(struct net_device *netdev, int new_mtu)
> > +{
> > +       struct hbg_priv *priv = netdev_priv(netdev);
> > +       bool is_opened = hbg_nic_is_open(priv);
> > +
> > +       hbg_net_stop(netdev);
> [Kalesh] Do you still need to call stop when NIC is not opened yet?
> Instead of a new variable, I think you can check netif_running here.

hbg_net_stop() will check HBG_NIC_STATE_OPEN by hbg_nic_is_open(),
So, if  NIC is not opened, hbg_net_stop() will do nothing

Thanks!
	Jijie Shao