From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sergei Shtylyov Date: Sun, 07 Feb 2016 17:09:43 +0000 Subject: Re: [PATCH/RFC v4 net-next] ravb: Add dma queue interrupt support Message-Id: <56B77A57.2080902@cogentembedded.com> List-Id: References: <1453650775-19886-1-git-send-email-ykaneko0929@gmail.com> <56AA4674.6020001@cogentembedded.com> In-Reply-To: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: Yoshihiro Kaneko Cc: netdev@vger.kernel.org, "David S. Miller" , Simon Horman , Magnus Damm , Linux-sh list 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 >>> >>> 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 >>> Signed-off-by: Yoshihiro Kaneko >>> --- >>> >>> 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