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

[Andrew Lunn]

On Mon, Sep 09, 2024 at 12:04:53PM +0800, Jijie Shao wrote:
> 
> 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.

Generally, we don't want defensive programming. You seem to suggest
hbg_net_open and hbg_net_stop are called in pairs. If this is not
true, you have a bug? Rather than paper over the bug with a return,
let bad things happen so the bug is obvious.

> > > +
> > > +       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)

Which is horrible. Why hide this, when it is in full view in other
places.

Please take a step back. Take time to understand the driver locking
with RTNL, look at what state is already available, and try very hard
to remove priv->state.

> 
> > > +
> > > +       clear_bit(HBG_NIC_STATE_OPEN, &priv->state);
> > > +

While we are at it, why is there not a race condition here between
testing and clearing the bit?

> > > +       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.

Correct. running is used by every driver, it has withstood years of
testing, so is guaranteed to be set/cleared in race free ways. It
should be used instead of this racy priv->state.

	Andrew