netdev - Re: [PATCH v3 1/1 net-next] net: fec: add napi support to improve proformance

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHrpEqTcVDBxKdXsaB3xw+RtE13Pj-vX8uuope+8RV0aXdrh+w@mail.gmail.com>
Date:	Fri, 25 Jan 2013 12:51:01 +0800
From:	Frank Li <lznuaa@...il.com>
To:	"Waskiewicz Jr, Peter P" <peter.p.waskiewicz.jr@...el.com>
Cc:	Frank Li <Frank.Li@...escale.com>, shawn.guo@...aro.org,
	B38611@...escale.com, davem@...emloft.net,
	linux-arm-kernel@...ts.infradead.org, netdev@...r.kernel.org,
	s.hauer@...gutronix.de
Subject: Re: [PATCH v3 1/1 net-next] net: fec: add napi support to improve proformance

2013/1/25 Waskiewicz Jr, Peter P <peter.p.waskiewicz.jr@...el.com>:
> On Fri, Jan 25, 2013 at 09:29:03AM +0800, Frank Li wrote:
>
> [...]
>
>>  static void
>> +fec_enet_rx_int_enable(struct net_device *ndev, bool enabled)
>> +{
>> +     struct fec_enet_private *fep = netdev_priv(ndev);
>> +     uint    int_events;
>> +
>> +     int_events = readl(fep->hwp + FEC_IMASK);
>> +     if (enabled)
>> +             int_events |= FEC_ENET_RXF;
>> +     else
>> +             int_events &= ~FEC_ENET_RXF;
>> +     writel(int_events, fep->hwp + FEC_IMASK);
>> +}
>
> You hold your hw_lock when this is called to disable the interrupt, but
> not when you re-enable it.  See below.
>
> [...]
>
>> @@ -813,7 +831,14 @@ fec_enet_interrupt(int irq, void *dev_id)
>>
>>               if (int_events & FEC_ENET_RXF) {
>>                       ret = IRQ_HANDLED;
>> -                     fec_enet_rx(ndev);
>> +
>> +                     spin_lock(&fep->hw_lock);
>> +                     /* Disable the RX interrupt */
>> +                     if (napi_schedule_prep(&fep->napi)) {
>> +                             fec_enet_rx_int_enable(ndev, false);
>> +                             __napi_schedule(&fep->napi);
>> +                     }
>> +                     spin_unlock(&fep->hw_lock);
>>               }
>>
>>               /* Transmit OK, or non-fatal error. Update the buffer
>> @@ -834,7 +859,16 @@ fec_enet_interrupt(int irq, void *dev_id)
>>       return ret;
>>  }
>>
>> -
>> +static int fec_enet_rx_napi(struct napi_struct *napi, int budget)
>> +{
>> +     struct net_device *ndev = napi->dev;
>> +     int pkgs = fec_enet_rx(ndev, budget);
>> +     if (pkgs < budget) {
>> +             napi_complete(napi);
>> +             fec_enet_rx_int_enable(ndev, true);
>
> Here you're not locking when re-enabling, but you do in the disable path.
>
> Also, when you're disabling interrupts above, you're doing that in your
> HW interrupt handler, you should be using spin_lock_irqsave()/irq_restore().

Good catch. Thanks!

>
> Cheers,
> -PJ
--
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