On 8/17/20 5:33 PM, Eric Dumazet wrote:
>
>
> On 8/17/20 2:17 PM, Hauke Mehrtens wrote:
>> On 8/16/20 8:07 PM, Eric Dumazet wrote:
>>>
>>>
>>> On 8/15/20 11:33 AM, Hauke Mehrtens wrote:
>>>> Use napi_complete_done() and activate the interrupts when this function
>>>> returns true. This way the generic NAPI code can take care of activating
>>>> the interrupts.
>>>>
>>>> Signed-off-by: Hauke Mehrtens <ha...@hauke-m.de>
>>>> ---
>>>> drivers/net/ethernet/lantiq_xrx200.c | 8 ++------
>>>> 1 file changed, 2 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/drivers/net/ethernet/lantiq_xrx200.c
>>>> b/drivers/net/ethernet/lantiq_xrx200.c
>>>> index f34e4dc8c661..674ffb2ecd9a 100644
>>>> --- a/drivers/net/ethernet/lantiq_xrx200.c
>>>> +++ b/drivers/net/ethernet/lantiq_xrx200.c
>>>> @@ -229,10 +229,8 @@ static int xrx200_poll_rx(struct napi_struct *napi,
>>>> int budget)
>>>> }
>>>> }
>>>>
>>>> - if (rx < budget) {
>>>> - napi_complete(&ch->napi);
>>>> + if (napi_complete_done(&ch->napi, rx))
>>>> ltq_dma_enable_irq(&ch->dma);
>>>> - }
>>>>
>>>> return rx;
>>>> }
>>>> @@ -271,10 +269,8 @@ static int xrx200_tx_housekeeping(struct napi_struct
>>>> *napi, int budget)
>>>> if (netif_queue_stopped(net_dev))
>>>> netif_wake_queue(net_dev);
>>>>
>>>> - if (pkts < budget) {
>>>> - napi_complete(&ch->napi);
>>>> + if (napi_complete_done(&ch->napi, pkts))
>>>> ltq_dma_enable_irq(&ch->dma);
>>>> - }
>>>>
>>>> return pkts;
>>>> }
>>>>
>>>
>>>
>>> This looks buggy to me.
>>
>> Hi Eric,
>>
>> Thanks for looking at the patch.
>>
>> What exactly looks buggy to you?
>
> You removed the " if (rx < budget) "
>
> But you must not.
>
> Drivers have to keep the test.
>
> Something like that seems more correct :
>
> diff --git a/drivers/net/ethernet/lantiq_xrx200.c
> b/drivers/net/ethernet/lantiq_xrx200.c
> index
> 1645e4e7ebdbb3c7abff8fe4207273df20f123d4..e3d617d387ed0f5593c3ba81d1d531d463bb5a6e
> 100644
> --- a/drivers/net/ethernet/lantiq_xrx200.c
> +++ b/drivers/net/ethernet/lantiq_xrx200.c
> @@ -230,8 +230,8 @@ static int xrx200_poll_rx(struct napi_struct *napi, int
> budget)
> }
>
> if (rx < budget) {
> - napi_complete(&ch->napi);
> - ltq_dma_enable_irq(&ch->dma);
> + if (napi_complete(&ch->napi, rx))
Obviously : s/napi_complete/napi_complete_done/
> + ltq_dma_enable_irq(&ch->dma);
> }
>
> return rx;
> @@ -269,8 +269,8 @@ static int xrx200_tx_housekeeping(struct napi_struct
> *napi, int budget)
> netdev_completed_queue(ch->priv->net_dev, pkts, bytes);
>
> if (pkts < budget) {
> - napi_complete(&ch->napi);
> - ltq_dma_enable_irq(&ch->dma);
> + if (napi_complete(&ch->napi, pkts))
Same : s/napi_complete/napi_complete_done/
> + ltq_dma_enable_irq(&ch->dma);
> }
>
> return pkts;
>
