Re: [PATCH/RFC v5 net-next] ravb: Add dma queue interrupt support

[Sergei Shtylyov <sergei.shtylyov@xxxxxxxxxxxxxxxxxx>]

Hello.

On 03/02/2016 09:32 PM, Yoshihiro Kaneko wrote:

> > > > > From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@xxxxxxxxxxx>
> > > > >
> > > > > This patch supports the following interrupts.
> > > > >
> > > > > - One interrupt for multiple (error, gPTP)
> > > > > - One interrupt for emac
> > > > > - Four interrupts for dma queue (best effort rx/tx, network control
> > > > > rx/tx)
> > > > >
> > > > > This patch improve efficiency of the interrupt handler by adding the
> > > > > interrupt handler corresponding to each interrupt source described
> > > > > above. Additionally, it reduces the number of times of the access to
> > > > > EthernetAVB IF.
> > > > > Also this patch prevent this driver depends on the whim of a boot
> > > > > loader.
> > > > >
> > > > > [ykaneko0929@xxxxxxxxx: define bit names of registers]
> > > > > [ykaneko0929@xxxxxxxxx: add comment for gen3 only registers]
> > > > > [ykaneko0929@xxxxxxxxx: fix coding style]
> > > > > [ykaneko0929@xxxxxxxxx: update changelog]
> > > > > [ykaneko0929@xxxxxxxxx: gen3: fix initialization of interrupts]
> > > > > [ykaneko0929@xxxxxxxxx: gen3: fix clearing interrupts]
> > > > > [ykaneko0929@xxxxxxxxx: gen3: add helper function for request_irq()]
> > > > > [ykaneko0929@xxxxxxxxx: revert ravb_close() and ravb_ptp_stop()]
> > > > > [ykaneko0929@xxxxxxxxx: avoid calling free_irq() to non-hooked
> > > > > interrupts]
> > > > > [ykaneko0929@xxxxxxxxx: make NC/BE interrupt handler a function]
> > > > > Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@xxxxxxxxxxx>
> > > > > Signed-off-by: Yoshihiro Kaneko <ykaneko0929@xxxxxxxxx>
[...]
> > > > > diff --git a/drivers/net/ethernet/renesas/ravb_main.c
> > > > > b/drivers/net/ethernet/renesas/ravb_main.c
> > > > > index c936682..1bec71e 100644
> > > > > --- a/drivers/net/ethernet/renesas/ravb_main.c
> > > > > +++ b/drivers/net/ethernet/renesas/ravb_main.c
[...]
> > > > > @@ -725,31 +787,15 @@ static irqreturn_t ravb_interrupt(int irq, void
> > > > > *dev_id)
> > > > >
> > > > >                   /* Network control and best effort queue RX/TX */
> > > > >                   for (q = RAVB_NC; q >= RAVB_BE; q--) {
> > > > > -                       if (((ris0 & ric0) & BIT(q)) ||
> > > > > -                           ((tis  & tic)  & BIT(q))) {
> > > > > -                               if (napi_schedule_prep(&priv->napi[q]))
> > > > > {
> > > > > -                                       /* Mask RX and TX interrupts */
> > > > > -                                       ric0 &= ~BIT(q);
> > > > > -                                       tic &= ~BIT(q);
> > > > > -                                       ravb_write(ndev, ric0, RIC0);
> > > > > -                                       ravb_write(ndev, tic, TIC);
> > > > > -                                       __napi_schedule(&priv->napi[q]);
> > > > > -                               } else {
> > > > > -                                       netdev_warn(ndev,
> > > > > -                                                   "ignoring interrupt,
> > > > > rx status 0x%08x, rx mask 0x%08x,\n",
> > > > > -                                                   ris0, ric0);
> > > > > -                                       netdev_warn(ndev,
> > > > > -                                                   "
> > > > > tx status 0x%08x, tx mask 0x%08x.\n",
> > > > > -                                                   tis, tic);
> > > > > -                               }
> > > > > +                       if (ravb_nc_be_interrupt(ndev, q, ris0, &ric0,
> > > > > tis,
> > > > > +                                                &tic))
> > > > >                                   result = IRQ_HANDLED;
> > > > > -                       }
> > > > >                   }
> > > >
> > > >      Unroll this *for* loop please...
> > >
> > > OK.
> >
> >     It was a bad idea actually, sorry...
>
> OK, I will revert this part.
> (But I think it is not that bad...)

   I think the loop will scale better if we ever support the AVB queues...

[...]
> > > > > @@ -767,6 +813,73 @@ static irqreturn_t ravb_interrupt(int irq, void
[...]
> > > > > +static irqreturn_t ravb_dmaq_interrupt(int irq, void *dev_id, int
> > > > > ravb_queue)
> > > >
> > > >      Perhaps, ravb_rx_tx_interrupt()?
> > >
> > > Agreed.
> >
> >     And we still have ravb_dma_interrupt() unused, right?
>
> Do you mean that there is an unused "ravb_dma_interrupt()" in this version?
> I probably be misunderstanding something.

   No, I meant that this name isn't used yet.

> > [...]

> Thanks,
> kaneko

MBR, Sergei