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

[Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>]

Hello.

On 02/07/2016 07:50 PM, Yoshihiro Kaneko wrote:

> I apologize for not responding to you earlier.

    Absolutely no problem, these reviews/tests take time from my main tasks 
anyway. :-)

>>> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>>>
>>> This patch supports the following interrupts.
>>>
>>> - One interrupt for multiple (descriptor, error, management)
>>> - 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.
>>>
>>> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>>> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@gmail.com>
>>> ---
>>>
>>> This patch is based on the master branch of David Miller's next networking
>>> tree.
>>>
>>> v4 [Yoshihiro Kaneko]
>>> * compile tested only
>>> * As suggested by Sergei Shtylyov
>>>     drivers/net/ethernet/renesas/ravb.h:
>>>       - make two lines of comment into one line.
>>>       - remove unused definition of xxx_ALL.
>>>     drivers/net/ethernet/renesas/ravb_main.c:
>>>       - remove unrelated change (fix indentation).
>>>       - output warning messages when napi_schedule_prep() fails in
>>> ravb_dmaq_
>>>         interrupt() like ravb_interrupt().
>>>       - change the function name from req_irq to hook_irq.
>>>       - fix programming error in hook_irq().
>>>       - do free_irq() for rx_irqs[] and tx_irqs[] for only gen3 in
>>> out_free_
>>>         irq label in ravb_open().
>>>
>>> v3 [Yoshihiro Kaneko]
>>> * compile tested only
>>> * As suggested by Sergei Shtylyov
>>>     - update changelog
>>>     drivers/net/ethernet/renesas/ravb.h:
>>>       - add comments to the additional registers like CIE
>>>     drivers/net/ethernet/renesas/ravb_main.c:
>>>       - fix the initialization of the interrupt in ravb_dmac_init()
>>>       - revert ravb_error_interrupt() because gen3 code is wrong
>>>       - change the comment "Management" in ravb_multi_interrupt()
>>>       - add a helper function for request_irq() in ravb_open()
>>>       - revert ravb_close() because atomicity is not necessary here
>>>     drivers/net/ethernet/renesas/ravb_ptp.c:
>>>       - revert ravb_ptp_stop() because atomicity is not necessary here
>>>
>>> v2 [Yoshihiro Kaneko]
>>> * compile tested only
>>> * As suggested by Sergei Shtylyov
>>>     - add comment to CIE
>>>     - remove comments from CIE bits
>>>     - fix value of TIx_ALL
>>>     - define each bits for CIE, GIE, GID, RIE0, RID0, RIE2, RID2, TIE, TID
>>>     - reversed Christmas tree declaration ordered
>>>     - rename _ravb_emac_interrupt() to ravb_emac_interrupt_unlocked()
>>>     - remove unnecessary clearing of CIE
>>>     - use a bit name corresponding to the target register, RIE0, RIE2, TIE,
>>>       TID, RID2, GID, GIE
>>
>>
>>     As I already noted, the changes made to the original patch are supposed
>> to be documented above --- (no need to separate diff versions there though).
>>     Either that, or just say that it's your patch, based on Mizuguchi-san's
>> work (the amount of changes makes that possible, I think).
>
> I will record that I made a change to this patch in the commit log of
> the next version.
> I don't think that I changed the essence of this patch. I changed
> various trivial things, or fixed bugs you pointed out.

    OK, as you wish. But in case this gets too tedious, I'll understand if you 
change the authorship.

>> [...]
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c
>>> b/drivers/net/ethernet/renesas/ravb_main.c
>>> index ac43ed9..076f25f 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c

[...]

>>
>>> +
>>> +       spin_lock(&priv->lock);
>>> +
>>> +       ris0 = ravb_read(ndev, RIS0);
>>> +       ric0 = ravb_read(ndev, RIC0);
>>> +       tis  = ravb_read(ndev, TIS);
>>> +       tic  = ravb_read(ndev, TIC);
>>> +
>>> +       /* Timestamp updated */
>>> +       if (tis & TIS_TFUF) {
>>> +               ravb_write(ndev, TID_TFUD, TID);
>>
>>
>>     Wait, you're supposed to clear the TFUF interrupt, not to disable!
>
> Thanks for finding this bug.
>
>> And with that fixed, this interrupt's handler could get factored out into a
>> function...
>
> Is this not too small to make a function?

    I wouldn't say so. But need to count the summary LoCs, of course... 
perhaps indeed not worth it...

[...]

>>> @@ -1215,29 +1332,64 @@ static const struct ethtool_ops ravb_ethtool_ops >>> {
>>>          .get_ts_info            = ravb_get_ts_info,
>>>    };
>>>
>>> +static inline int hook_irq(unsigned int irq, irq_handler_t handler,

>>     Namespacing this function with 'ravb_' prefix would be preferable, I did
>> that for all functions, even those that didn't have this prefix in sh_eth...

    Didn't have 'sh_eth_' prefix, I meant.

> Got it.

    OK.

>>
>>> +                          struct net_device *ndev, struct device *dev,
>>> +                          const char *ch)
>>> +{
>>> +       char *name;
>>> +       int error;
>>> +
>>> +       name = devm_kasprintf(dev, GFP_KERNEL, "%s:%s", ndev->name, ch);
>>> +       error = request_irq(irq, handler, IRQF_SHARED, name, ndev);
>>
>>
>>     Not sure if we need IRQF_SHARED on those IRQs, they're not really
>> shareable...
>
> I don't know whether this causes something bad.
> I think this controller is supporting a shared IRQ.

    Based on the high-level trigger, I'd rather suspect not. Anyway, all the 
SoC IRQs are dedicated to a certain (single) source.

[...]

[...]
>>
>>     OK, I'll now proceed to sanity-testing this patch on the gen2 hardware.

    I'm afraid this will have to wait until I have a gen2 board with fully 
working AVB... :-(

> Thanks,
> kaneko

MBR, Sergei