Re: Re: [PATCH net-next v12 2/8] hinic3: Fix loose success check in rx buffer filling

[Fan Gong]

> >    Tightens the success criterion for hinic3_rx_fill_buffers() in
> > hinic3_configure_rxqs().
> >
> > Co-developed-by: Wu Di <wudi234@xxxxxxxxxx>
> > Signed-off-by: Wu Di <wudi234@xxxxxxxxxx>
> > Co-developed-by: Teng Peisen <tengpeisen@xxxxxxxxxx>
> > Signed-off-by: Teng Peisen <tengpeisen@xxxxxxxxxx>
> > Signed-off-by: Fan Gong <gongfan1@xxxxxxxxxx>
> > ---
> >   drivers/net/ethernet/huawei/hinic3/hinic3_rx.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c b/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
> > index 309ab5901379..389b1c2158be 100644
> > --- a/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
> > +++ b/drivers/net/ethernet/huawei/hinic3/hinic3_rx.c
> > @@ -541,7 +541,7 @@ int hinic3_configure_rxqs(struct net_device *netdev, u16 num_rq,
> >   		rq_associate_cqes(rxq);
> >
> >   		pkts = hinic3_rx_fill_buffers(rxq);
> > -		if (!pkts) {
> > +		if (pkts < rxq->q_mask) {
> >   			netdev_err(netdev, "Failed to fill Rx buffer\n");
> >   			return -ENOMEM;
> >   		}
>
> Given the subject, curious if a partially populated ring result in any
> functional failure? if so, then perhaps failure mode can be explained
> and this specific change be sent to net tree?

Thanks for the comment. It fixes a previous review comment of the patchset
as the following quotes. To see the full review, visit
https://lore.kernel.org/netdev/20260413171817.1e1eca30@xxxxxxxxxx/

> > +int
> > +hinic3_change_channel_settings(struct net_device *netdev,
> > +			       struct hinic3_dyna_txrxq_params *trxq_params)
> > +{
> > +	struct hinic3_nic_dev *nic_dev = netdev_priv(netdev);
> > +	struct hinic3_dyna_txrxq_params old_qp_params = {};
> > +	struct hinic3_dyna_qp_params new_qp_params = {};
> > +	struct hinic3_dyna_qp_params cur_qp_params = {};
> > +	bool need_teardown = false;
> > +	unsigned long flags;
> > +	int err;
> > +
> > +	mutex_lock(&nic_dev->channel_cfg_lock);
> > +
> > +	hinic3_config_num_qps(netdev, trxq_params);
> > +
> > +	err = hinic3_alloc_channel_resources(netdev, &new_qp_params,
> > +					     trxq_params);
> > +	if (err) {
> > +		netdev_err(netdev, "Failed to alloc channel resources\n");
> > +		mutex_unlock(&nic_dev->channel_cfg_lock);
> > +		return err;
> > +	}
> > +
> > +	spin_lock_irqsave(&nic_dev->channel_res_lock, flags);
> > +	if (!test_and_set_bit(HINIC3_CHANGE_RES_INVALID, &nic_dev->flags))
> > +		need_teardown = true;
> > +	spin_unlock_irqrestore(&nic_dev->channel_res_lock, flags);
> > +
> > +	if (need_teardown) {
> > +		hinic3_vport_down(netdev);
> > +		hinic3_close_channel(netdev);
> > +		hinic3_uninit_qps(nic_dev, &cur_qp_params);
> > +		hinic3_free_channel_resources(netdev, &cur_qp_params,
> > +					      &nic_dev->q_params);
> > +	}
> > +
> > +	if (nic_dev->num_qp_irq > trxq_params->num_qps)
> > +		hinic3_qp_irq_change(netdev, trxq_params->num_qps);
> > +
> > +	spin_lock_irqsave(&nic_dev->channel_res_lock, flags);
> > +	old_qp_params = nic_dev->q_params;
> > +	nic_dev->q_params = *trxq_params;
> > +	spin_unlock_irqrestore(&nic_dev->channel_res_lock, flags);
> > +
> > +	hinic3_init_qps(nic_dev, &new_qp_params);
> > +
> > +	err = hinic3_open_channel(netdev);
>
> This "open" function allocates Rx buffers, and fails if it couldn't get
> even one. That's no good.