Re: Re: [PATCH net-next v11 1/5] hinic3: Add ethtool queue ops

[Fan Gong <gongfan1@huawei.com>]

> >    Implement following ethtool callback function:
> > .get_ringparam
> > .set_ringparam
> >
> >    These callbacks allow users to utilize ethtool for detailed
> > queue depth configuration and monitoring.
> >
> >    Change port_state_mutex to state_lock as a unified mutex lock
> > in hinic3_nic_dev.
> >
> >    Tightens the success criterion for hinic3_rx_fill_buffers() in
> > hinic3_configure_rxqs().
> >
> >    Changes hinic3_tx_csum()/hinic3_tx_offload() to propagate
> > skb_checksum_help() failures into a TX drop.
> >
> >    Renames hinic3_uninit_qps() to hinic3_get_cur_qps().
> >
> >    Splits hinic3_open_channel() into hinic3_prepare_channel() and
> > hinic3_open_channel().
> >
>
> It may just be me, but I feel like there is a lot going in this single
> patch. Ideally, a patch should carry one logical change. This patch is
> doing 3 things: refactor, new code, bug-fixing in the hinic3_close() path.

Thanks for the following 3 review comments. We are pushing new codes to the
net-next. However, fixing review comments leads to some refactoring and
bug-fixing codes.

We are considering split patch #1 into smaller ones in the next patchset.

> > @@ -330,6 +330,8 @@ static void hinic3_link_status_change(struct net_device *netdev,
> >   		netif_carrier_off(netdev);
> >   		netdev_dbg(netdev, "Link is down\n");
> >   	}
> > +
> > +	return;
>
> Unrelated?

Sorry for our negligence. We will remove this unrelated change in the next
patchset.

> > @@ -493,10 +580,15 @@ static int hinic3_close(struct net_device *netdev)
> >   		return 0;
> >   	}
> >
> > +	mutex_lock(&nic_dev->state_lock);
> >   	hinic3_vport_down(netdev);
> >   	hinic3_close_channel(netdev);
> > -	hinic3_uninit_qps(nic_dev, &qp_params);
> > -	hinic3_free_channel_resources(netdev, &qp_params, &nic_dev->q_params);
> > +	hinic3_get_cur_qps(nic_dev, &qp_params);
> > +	hinic3_free_channel_resources(netdev, &qp_params,
> > +				      &nic_dev->q_params);
> > +	hinic3_free_nicio_res(nic_dev);
> > +	hinic3_destroy_num_qps(netdev);
>
> free_nicio_res(), and destroy_num_qps() looks like fixes to me on the
> clean-up path. Should these go to the net tree?

As mentioned above, we reckon it as a necessary fix to commit new codes, thus
we put it in this patch.