| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Tue, 22 Dec 2015 18:02:24 +0300
From: Sergei Shtylyov <sergei.shtylyov@...entembedded.com>
To: Yoshihiro Kaneko <ykaneko0929@...il.com>, netdev@...r.kernel.org
Cc: "David S. Miller" <davem@...emloft.net>,
Simon Horman <horms@...ge.net.au>,
Magnus Damm <magnus.damm@...il.com>, linux-sh@...r.kernel.org
Subject: Re: [PATCH/RFC v2 net-next] ravb: Add dma queue interrupt support
Hello.
On 12/20/2015 12:15 PM, Yoshihiro Kaneko wrote:
> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@...esas.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)
You still don't say why it's better than the current scheme...
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@...esas.com>
> Signed-off-by: Yoshihiro Kaneko <ykaneko0929@...il.com>
> ---
>
> This patch is based on the master branch of David Miller's next networking
> tree.
>
> 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
>
> drivers/net/ethernet/renesas/ravb.h | 213 ++++++++++++++++++++++++++
> drivers/net/ethernet/renesas/ravb_main.c | 247 +++++++++++++++++++++++++++----
> drivers/net/ethernet/renesas/ravb_ptp.c | 45 ++++--
> 3 files changed, 464 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 9fbe92a..71badd6d 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -157,6 +157,7 @@ enum ravb_reg {
> TIC = 0x0378,
> TIS = 0x037C,
> ISS = 0x0380,
> + CIE = 0x0384, /* R-Car Gen3 only */
> GCCR = 0x0390,
> GMTT = 0x0394,
> GPTC = 0x0398,
> @@ -170,6 +171,15 @@ enum ravb_reg {
> GCT0 = 0x03B8,
> GCT1 = 0x03BC,
> GCT2 = 0x03C0,
> + GIE = 0x03CC,
> + GID = 0x03D0,
> + DIL = 0x0440,
> + RIE0 = 0x0460,
> + RID0 = 0x0464,
> + RIE2 = 0x0470,
> + RID2 = 0x0474,
> + TIE = 0x0478,
> + TID = 0x047c,
So you only commented on CIE and considered it done? :-)
[...]
> @@ -411,14 +422,27 @@ static int ravb_dmac_init(struct net_device *ndev)
> ravb_write(ndev, TCCR_TFEN, TCCR);
>
> /* Interrupt init: */
> - /* Frame receive */
> - ravb_write(ndev, RIC0_FRE0 | RIC0_FRE1, RIC0);
> - /* Disable FIFO full warning */
> - ravb_write(ndev, 0, RIC1);
> - /* Receive FIFO full error, descriptor empty */
> - ravb_write(ndev, RIC2_QFE0 | RIC2_QFE1 | RIC2_RFFE, RIC2);
> - /* Frame transmitted, timestamp FIFO updated */
> - ravb_write(ndev, TIC_FTE0 | TIC_FTE1 | TIC_TFUE, TIC);
> + if (priv->chip_id == RCAR_GEN2) {
> + /* Frame receive */
> + ravb_write(ndev, RIC0_FRE0 | RIC0_FRE1, RIC0);
> + /* Disable FIFO full warning */
> + ravb_write(ndev, 0, RIC1);
> + /* Receive FIFO full error, descriptor empty */
> + ravb_write(ndev, RIC2_QFE0 | RIC2_QFE1 | RIC2_RFFE, RIC2);
> + /* Frame transmitted, timestamp FIFO updated */
> + ravb_write(ndev, TIC_FTE0 | TIC_FTE1 | TIC_TFUE, TIC);
> + } else {
> + /* Clear DIL.DPLx */
> + ravb_write(ndev, 0, DIL);
> + /* Set queue specific interrupt */
> + ravb_write(ndev, CIE_CRIE | CIE_CTIE | CIE_CL0M, CIE);
> + /* Frame receive */
> + ravb_write(ndev, RIE0_FRS0 | RIE0_FRS1, RIE0);
> + /* Receive FIFO full error, descriptor empty */
> + ravb_write(ndev, RIE2_QFS0 | RIE2_QFS1 | RIE2_RFFS, RIE2);
> + /* Frame transmitted, timestamp FIFO updated */
> + ravb_write(ndev, TIE_FTS0 | TIE_FTS1 | TIE_TFUS, TIE);
> + }
So in this case for gen3 we enable interrupts we need in addition to already
enabled (by a boot loader perhaps)? I don't think you actually want it...
[...]
> @@ -690,7 +726,10 @@ static void ravb_error_interrupt(struct net_device *ndev)
> ravb_write(ndev, ~EIS_QFS, EIS);
> if (eis & EIS_QFS) {
> ris2 = ravb_read(ndev, RIS2);
> - ravb_write(ndev, ~(RIS2_QFF0 | RIS2_RFFF), RIS2);
> + if (priv->chip_id == RCAR_GEN2)
> + ravb_write(ndev, ~(RIS2_QFF0 | RIS2_RFFF), RIS2);
> + else
> + ravb_write(ndev, RID2_QFD0 | RID2_RFFD, RID2);
Err, aren't you doing 2 different things for gen2 and gen3 here. For gen2
you're clearing the QFF0/RFFF interrupts, for gen3 you're disabling them, no?
[...]
> @@ -758,16 +797,43 @@ static irqreturn_t ravb_interrupt(int irq, void *dev_id)
[...]
> +/* Descriptor IRQ/Error/Management interrupt handler */
> +static irqreturn_t ravb_multi_interrupt(int irq, void *dev_id)
> +{
> + struct net_device *ndev = dev_id;
> + struct ravb_private *priv = netdev_priv(ndev);
> + irqreturn_t result = IRQ_NONE;
> + u32 iss;
> +
> + spin_lock(&priv->lock);
> + /* Get interrupt status */
> + iss = ravb_read(ndev, ISS);
> +
> /* Error status summary */
> if (iss & ISS_ES) {
> ravb_error_interrupt(ndev);
> result = IRQ_HANDLED;
> }
>
> + /* Management */
I still doubt this comment is valid...
> if (iss & ISS_CGIS)
> result = ravb_ptp_interrupt(ndev);
>
[...]
> @@ -1223,23 +1343,68 @@ static const struct ethtool_ops ravb_ethtool_ops = {
> static int ravb_open(struct net_device *ndev)
> {
> struct ravb_private *priv = netdev_priv(ndev);
> - int error;
> + struct platform_device *pdev = priv->pdev;
> + struct device *dev = &pdev->dev;
> + int error, i;
> + char *name;
>
> napi_enable(&priv->napi[RAVB_BE]);
> napi_enable(&priv->napi[RAVB_NC]);
>
> - error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED, ndev->name,
> - ndev);
> - if (error) {
> - netdev_err(ndev, "cannot request IRQ\n");
> - goto out_napi_off;
> - }
> -
> - if (priv->chip_id == RCAR_GEN3) {
> - error = request_irq(priv->emac_irq, ravb_interrupt,
> - IRQF_SHARED, ndev->name, ndev);
> + if (priv->chip_id == RCAR_GEN2) {
> + error = request_irq(ndev->irq, ravb_interrupt, IRQF_SHARED,
> + ndev->name, ndev);
> if (error) {
> netdev_err(ndev, "cannot request IRQ\n");
> + goto out_napi_off;
> + }
> + } else {
> + name = devm_kasprintf(dev, GFP_KERNEL, "%s:ch22:multi",
> + ndev->name);
> + error = request_irq(ndev->irq, ravb_multi_interrupt,
> + IRQF_SHARED, name, ndev);
> + if (error) {
> + netdev_err(ndev, "cannot request IRQ %s\n", name);
> + goto out_napi_off;
> + }
> + name = devm_kasprintf(dev, GFP_KERNEL, "%s:ch24:emac",
> + ndev->name);
> + error = request_irq(priv->emac_irq, ravb_emac_interrupt,
> + IRQF_SHARED, name, ndev);
> + if (error) {
> + netdev_err(ndev, "cannot request IRQ %s\n", name);
> + goto out_free_irq;
> + }
> + name = devm_kasprintf(dev, GFP_KERNEL, "%s:ch0:rx_be",
> + ndev->name);
> + error = request_irq(priv->rx_irqs[RAVB_BE], ravb_be_interrupt,
> + IRQF_SHARED, name, ndev);
> + if (error) {
> + netdev_err(ndev, "cannot request IRQ %s\n", name);
> + goto out_free_irq;
> + }
> + name = devm_kasprintf(dev, GFP_KERNEL, "%s:ch18:tx_be",
> + ndev->name);
> + error = request_irq(priv->tx_irqs[RAVB_BE], ravb_be_interrupt,
> + IRQF_SHARED, name, ndev);
> + if (error) {
> + netdev_err(ndev, "cannot request IRQ %s\n", name);
> + goto out_free_irq;
> + }
> + name = devm_kasprintf(dev, GFP_KERNEL, "%s:ch1:rx_nc",
> + ndev->name);
> + error = request_irq(priv->rx_irqs[RAVB_NC], ravb_nc_interrupt,
> + IRQF_SHARED, name, ndev);
> + if (error) {
> + netdev_err(ndev, "cannot request IRQ %s\n", name);
> + goto out_free_irq;
> + }
> + name = devm_kasprintf(dev, GFP_KERNEL, "%s:ch19:tx_nc",
> + ndev->name);
> + error = request_irq(priv->tx_irqs[RAVB_NC], ravb_nc_interrupt,
> + IRQF_SHARED, name, ndev);
> + if (error) {
> + netdev_err(ndev, "cannot request IRQ %s\n", name);
> goto out_free_irq;
> }
This error case is repetitive, couldn't you consolidate it somehow?
[...]
> @@ -1494,10 +1663,15 @@ static int ravb_close(struct net_device *ndev)
> netif_tx_stop_all_queues(ndev);
>
> /* Disable interrupts by clearing the interrupt masks. */
> - ravb_write(ndev, 0, RIC0);
> - ravb_write(ndev, 0, RIC2);
> - ravb_write(ndev, 0, TIC);
> -
> + if (priv->chip_id == RCAR_GEN2) {
> + ravb_write(ndev, 0, RIC0);
> + ravb_write(ndev, 0, RIC2);
> + ravb_write(ndev, 0, TIC);
> + } else {
> + ravb_write(ndev, RID0_ALL, RID0);
> + ravb_write(ndev, RID2_ALL, RID2);
> + ravb_write(ndev, TID_ALL, TID);
> + }
Don't see why this is better than the old code. We don't care about
atomicity here AFAIU.
[...]
> @@ -1798,6 +1973,22 @@ static int ravb_probe(struct platform_device *pdev)
> goto out_release;
> }
> priv->emac_irq = irq;
> + for (i = 0; i < NUM_RX_QUEUE; i++) {
> + irq = platform_get_irq_byname(pdev, ravb_rx_irqs[i]);
> + if (irq < 0) {
> + error = irq;
> + goto out_release;
> + }
> + priv->rx_irqs[i] = irq;
> + }
> + for (i = 0; i < NUM_TX_QUEUE; i++) {
> + irq = platform_get_irq_byname(pdev, ravb_tx_irqs[i]);
> + if (irq < 0) {
> + error = irq;
> + goto out_release;
> + }
> + priv->tx_irqs[i] = irq;
> + }
Perhaps it would better to use sprintf() here, in both loops...
[...]
> diff --git a/drivers/net/ethernet/renesas/ravb_ptp.c b/drivers/net/ethernet/renesas/ravb_ptp.c
> index 7a8ce92..870d7b7 100644
> --- a/drivers/net/ethernet/renesas/ravb_ptp.c
> +++ b/drivers/net/ethernet/renesas/ravb_ptp.c
[...]
> @@ -352,7 +367,11 @@ void ravb_ptp_stop(struct net_device *ndev)
> {
> struct ravb_private *priv = netdev_priv(ndev);
>
> - ravb_write(ndev, 0, GIC);
> + if (priv->chip_id == RCAR_GEN2)
> + ravb_write(ndev, 0, GIC);
> + else
> + ravb_write(ndev, GID_ALL, GID);
Again, don't see why it's better than the old code.
[...]
MBR, Sergei
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists