| 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
| ||
|
Message-ID: <20130419093829.GB660@pengutronix.de>
Date: Fri, 19 Apr 2013 11:38:29 +0200
From: Steffen Trumtrar <s.trumtrar@...gutronix.de>
To: Hein Tibosch <hein_tibosch@...oo.es>
Cc: Nicolas Ferre <nicolas.ferre@...el.com>, netdev@...r.kernel.org,
Ludovic Desroches <ludovic.desroches@...el.com>
Subject: Re: net/macb: clear tx/rx completion flags in ISR
On Fri, Apr 19, 2013 at 05:21:20PM +0800, Hein Tibosch wrote:
> Hi,
>
> On 4/19/2013 3:48 PM, Nicolas Ferre wrote:
> > On 04/19/2013 09:30 AM, Steffen Trumtrar :
> >> Hi Hein,
> >>
> >> On Fri, Apr 19, 2013 at 01:13:26PM +0800, Hein Tibosch wrote:
> >>> Hi Steffen,
> >>>
> >>>> At least in the cadence IP core on the Xilinx Zynq SoC the TCOMP/RCOMP flags
> >>>> are not auto-cleaned. As these flags are evaluated, they need to be cleaned.
> >>> This patch does not work for at least the AVR32 platform. Both RCOMP/RCOMP
> >>> are cleared by *reading* the ISR and writing them would be fatal.
> >>>
> >> :-(
> >>
> >>> Could you tell me the version of the macb of Xilinx Zynq?
> >>>
> >>> u32 version = (macb_readl(bp, MID) & ((1 << MACB_REV_SIZE) - 1))
> >>> | MACB_GREGS_VERSION;
> >>>
> >>> On an AP7000 it reads as 0x0000010D
> >>>
> >> This gives me 0x00000119. The TRM says it is version r1p23.
> >>
> >>> I am thinking of making a patch like:
> >>>
> >>> if (bp->version >= xxx)
> >>> macb_writel(bp, ISR, MACB_BIT(TCOMP));
> >>>
> >>> if (bp->version >= xxx)
> >>> macb_writel(bp, ISR, MACB_BIT(RCOMP));
> >>>
> >>> which would make it work on both platforms.
> > Well, keep in mind that it is the hot path: It can harm the performance
> > if too much tests are performed...
>
> Yes it's in the hot path, both tests will be done within an interrupt.
>
> I just gave it a quick try:
>
> ---
> drivers/net/ethernet/cadence/macb.c | 8 ++++++--
> drivers/net/ethernet/cadence/macb.h | 1 +
> 2 files changed, 7 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/cadence/macb.c b/drivers/net/ethernet/cadence/macb.c
> index ed2cb13..fe31951 100644
> --- a/drivers/net/ethernet/cadence/macb.c
> +++ b/drivers/net/ethernet/cadence/macb.c
> @@ -315,6 +315,8 @@ int macb_mii_init(struct macb *bp)
> struct macb_platform_data *pdata;
> int err = -ENXIO, i;
>
> + bp->ip_version = (macb_readl(bp, MID) & ((1 << MACB_REV_SIZE) - 1))
> + | MACB_GREGS_VERSION;
> /* Enable management port */
> macb_writel(bp, NCR, MACB_BIT(MPE));
> @@ -485,7 +487,8 @@ static void macb_tx_interrupt(struct macb *bp)
> status = macb_readl(bp, TSR);
> macb_writel(bp, TSR, status);
>
> - macb_writel(bp, ISR, MACB_BIT(TCOMP));
> + if (bp->ip_version == 0x00000119)
> + macb_writel(bp, ISR, MACB_BIT(TCOMP));
> netdev_vdbg(bp->dev, "macb_tx_interrupt status = 0x%03lx\n",
> (unsigned long)status);
> @@ -738,7 +741,8 @@ static irqreturn_t macb_interrupt(int irq, void *dev_id)
> * now.
> */
> macb_writel(bp, IDR, MACB_RX_INT_FLAGS);
> - macb_writel(bp, ISR, MACB_BIT(RCOMP));
> + if (bp->ip_version == 0x00000119)
> + macb_writel(bp, ISR, MACB_BIT(RCOMP));
> if (napi_schedule_prep(&bp->napi)) {
> netdev_vdbg(bp->dev, "scheduling RX softirq\n");
> diff --git a/drivers/net/ethernet/cadence/macb.h b/drivers/net/ethernet/cadence/macb.h
> index 993d703..0fbb440 100644
> --- a/drivers/net/ethernet/cadence/macb.h
> +++ b/drivers/net/ethernet/cadence/macb.h
> @@ -575,6 +575,7 @@ struct macb {
> unsigned int duplex;
>
> phy_interface_t phy_interface;
> + unsigned ip_version;
> /* AT91RM9200 transmit */
> struct sk_buff *skb; /* holds skb until xmit interrupt completes */
> --
> 1.7.10
>
> On my AP7000 platforms, on a 100mbit LAN, I could not measure any drop
> of performance using iperf:
>
> Before the patch:
> [ 6] 0.0-10.1 sec 67.4 MBytes 56.1 Mbits/sec
> [ 7] 0.0-10.1 sec 65.5 MBytes 54.6 Mbits/sec
> [ 6] 0.0-10.1 sec 67.8 MBytes 56.4 Mbits/sec
> [ 7] 0.0-10.1 sec 65.1 MBytes 54.3 Mbits/sec
>
> After the patch:
> [ 6] 0.0-10.1 sec 68.1 MBytes 56.8 Mbits/sec
> [ 7] 0.0-10.1 sec 66.5 MBytes 55.4 Mbits/sec
> [ 6] 0.0- 9.9 sec 67.5 MBytes 57.2 Mbits/sec
> [ 8] 0.0-10.1 sec 66.5 MBytes 55.5 Mbits/sec
>
> It looks a bit faster, which is pure coincidence.
> >
> >> The documentation I have is a little bit confusing in that regard.
> >> The cadence datasheet says, this register is R/W, the Xilinx datasheet says,
> >> it is "normaly RO", but the programming guide explicitely mentions clearing
> >> the bit by writing to it.
>
> Steffen, did you really see it happen that TCOMP/RCOMP were not cleared by just
> reading the ISR?
> The Atmel manual says about each ISR field: 'Cleared on read'
>
I just reverted that patch to confirm and the system hangs with
[ 2.680000] Sending DHCP requests .
So, yes. It is needed. Either Cadence changed something or Xilinx is doing
something weird with the core.
> >> It seems, that something like your patch is inevitable.
> > I also had bad feedbacks concerning this patch. Maybe we should take
> > more time to validate this change: event it is in net-next, maybe we
> > should revert it for the moment...
Regards,
Steffen
--
Pengutronix e.K. | |
Industrial Linux Solutions | http://www.pengutronix.de/ |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
--
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