| 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: <20080905021037.GG14128@verge.net.au>
Date: Fri, 5 Sep 2008 12:10:37 +1000
From: Simon Horman <horms@...ge.net.au>
To: Josh Boyer <jwboyer@...ux.vnet.ibm.com>
Cc: benh@...nel.crashing.org, netdev@...r.kernel.org,
linuxppc-dev@...abs.org
Subject: Re: [PATCH 3/3] ibm_newemac: MAL support for PowerPC 405EZ
On Thu, Sep 04, 2008 at 11:02:16AM -0400, Josh Boyer wrote:
> The PowerPC 405EZ SoC has some differences in the interrupt layout and
> handling for the MAL. The SERR, TXDE, and RXDE interrupts are OR'd into
> a single interrupt. Also, due to the possibility for interrupt coalescing,
> the TXEOB and RXEOB interrupts require an interrupt bit to be cleared in
> the ICINTSTAT SDR.
>
> This sets the proper MAL feature bits for 405EZ boards, and adds a common
> shared handler for SERR, TXDE, and RXDE. This has been adapted from code
> originally written by Stefan Roese.
>
> Signed-off-by: Josh Boyer <jwboyer@...ux.vnet.ibm.com>
> ---
> drivers/net/ibm_newemac/mal.c | 98 ++++++++++++++++++++++++++++++++--------
> 1 files changed, 78 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/net/ibm_newemac/mal.c b/drivers/net/ibm_newemac/mal.c
> index 10c267b..3cef534 100644
> --- a/drivers/net/ibm_newemac/mal.c
> +++ b/drivers/net/ibm_newemac/mal.c
> @@ -28,6 +28,7 @@
> #include <linux/delay.h>
>
> #include "core.h"
> +#include <asm/dcr-regs.h>
>
> static int mal_count;
>
> @@ -279,6 +280,9 @@ static irqreturn_t mal_txeob(int irq, void *dev_instance)
> mal_schedule_poll(mal);
> set_mal_dcrn(mal, MAL_TXEOBISR, r);
>
> + if (mal_has_feature(mal, MAL_FTR_CLEAR_ICINTSTAT))
> + mtdcri(SDR0, 0x4510, (mfdcri(SDR0, 0x4510) | 0x60000000));
> +
> return IRQ_HANDLED;
> }
>
> @@ -293,6 +297,9 @@ static irqreturn_t mal_rxeob(int irq, void *dev_instance)
> mal_schedule_poll(mal);
> set_mal_dcrn(mal, MAL_RXEOBISR, r);
>
> + if (mal_has_feature(mal, MAL_FTR_CLEAR_ICINTSTAT))
> + mtdcri(SDR0, 0x4510, (mfdcri(SDR0, 0x4510) | 0x80000000));
> +
> return IRQ_HANDLED;
> }
>
> @@ -336,6 +343,25 @@ static irqreturn_t mal_rxde(int irq, void *dev_instance)
> return IRQ_HANDLED;
> }
>
> +static irqreturn_t mal_int(int irq, void *dev_instance)
> +{
> + struct mal_instance *mal = dev_instance;
> + u32 esr = get_mal_dcrn(mal, MAL_ESR);
> +
> + if (esr & MAL_ESR_EVB) {
> + /* descriptor error */
> + if (esr & MAL_ESR_DE) {
> + if (esr & MAL_ESR_CIDT)
> + return (mal_rxde(irq, dev_instance));
Return statements shouldn't be enlosed in brackets according to
checkpatch.pl. Also in a few other places.
> + else
> + return (mal_txde(irq, dev_instance));
> + } else { /* SERR */
> + return (mal_serr(irq, dev_instance));
> + }
> + }
> + return IRQ_HANDLED;
> +}
> +
> void mal_poll_disable(struct mal_instance *mal, struct mal_commac *commac)
> {
> /* Spinlock-type semantics: only one caller disable poll at a time */
> @@ -542,11 +568,22 @@ static int __devinit mal_probe(struct of_device *ofdev,
> goto fail;
> }
>
> - mal->txeob_irq = irq_of_parse_and_map(ofdev->node, 0);
> - mal->rxeob_irq = irq_of_parse_and_map(ofdev->node, 1);
> - mal->serr_irq = irq_of_parse_and_map(ofdev->node, 2);
> - mal->txde_irq = irq_of_parse_and_map(ofdev->node, 3);
> - mal->rxde_irq = irq_of_parse_and_map(ofdev->node, 4);
> + if (of_device_is_compatible(ofdev->node, "ibm,mcmal-405ez"))
> + mal->features |= (MAL_FTR_CLEAR_ICINTSTAT | MAL_FTR_COMMON_ERR_INT);
The above like is >80 characters wide.
But I'm not sure that anyone cares.
> +
> + if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
> + mal->txeob_irq = irq_of_parse_and_map(ofdev->node, 0);
> + mal->rxeob_irq = irq_of_parse_and_map(ofdev->node, 1);
> + mal->serr_irq = irq_of_parse_and_map(ofdev->node, 2);
> + mal->txde_irq = mal->rxde_irq = mal->serr_irq;
> + } else {
> + mal->txeob_irq = irq_of_parse_and_map(ofdev->node, 0);
> + mal->rxeob_irq = irq_of_parse_and_map(ofdev->node, 1);
> + mal->serr_irq = irq_of_parse_and_map(ofdev->node, 2);
> + mal->txde_irq = irq_of_parse_and_map(ofdev->node, 3);
> + mal->rxde_irq = irq_of_parse_and_map(ofdev->node, 4);
> + }
It seems that that first three calls to irq_of_parse_and_map() could
be moved outside of the if/else clause.
mal->txeob_irq = irq_of_parse_and_map(ofdev->node, 0);
mal->rxeob_irq = irq_of_parse_and_map(ofdev->node, 1);
mal->serr_irq = irq_of_parse_and_map(ofdev->node, 2);
if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
mal->txde_irq = mal->rxde_irq = mal->serr_irq;
} else {
mal->txde_irq = irq_of_parse_and_map(ofdev->node, 3);
mal->rxde_irq = irq_of_parse_and_map(ofdev->node, 4);
}
> +
> if (mal->txeob_irq == NO_IRQ || mal->rxeob_irq == NO_IRQ ||
> mal->serr_irq == NO_IRQ || mal->txde_irq == NO_IRQ ||
> mal->rxde_irq == NO_IRQ) {
> @@ -608,21 +645,42 @@ static int __devinit mal_probe(struct of_device *ofdev,
> sizeof(struct mal_descriptor) *
> mal_rx_bd_offset(mal, i));
>
> - err = request_irq(mal->serr_irq, mal_serr, 0, "MAL SERR", mal);
> - if (err)
> - goto fail2;
> - err = request_irq(mal->txde_irq, mal_txde, 0, "MAL TX DE", mal);
> - if (err)
> - goto fail3;
> - err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
> - if (err)
> - goto fail4;
> - err = request_irq(mal->rxde_irq, mal_rxde, 0, "MAL RX DE", mal);
> - if (err)
> - goto fail5;
> - err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
> - if (err)
> - goto fail6;
> + if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
> + err = request_irq(mal->serr_irq, mal_int, IRQF_SHARED,
> + "MAL SERR", mal);
> + if (err)
> + goto fail2;
> + err = request_irq(mal->txde_irq, mal_int, IRQF_SHARED,
> + "MAL TX DE", mal);
> + if (err)
> + goto fail3;
> + err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
> + if (err)
> + goto fail4;
> + err = request_irq(mal->rxde_irq, mal_int, IRQF_SHARED,
> + "MAL RX DE", mal);
> + if (err)
> + goto fail5;
> + err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
> + if (err)
> + goto fail6;
> + } else {
> + err = request_irq(mal->serr_irq, mal_serr, 0, "MAL SERR", mal);
> + if (err)
> + goto fail2;
> + err = request_irq(mal->txde_irq, mal_txde, 0, "MAL TX DE", mal);
> + if (err)
> + goto fail3;
> + err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
> + if (err)
> + goto fail4;
> + err = request_irq(mal->rxde_irq, mal_rxde, 0, "MAL RX DE", mal);
> + if (err)
> + goto fail5;
> + err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
> + if (err)
> + goto fail6;
> + }
There seems to be a lot of repention in the above if/else clauses.
I wonder if something like this might be nicer.
if (mal_has_feature(mal, MAL_FTR_COMMON_ERR_INT)) {
irqflags = IRQF_SHARED;
hdlr_serr = hdlr_txde = hdlr_rxde = mal_int;
} else {
irqflags = 0;
hdlr_serr = mal_serr;
hdlr_txde = mal_txde;
hdlr_rxde = mal_rxde;
}
err = request_irq(mal->serr_irq, hdlr_serr, irqflags, "MAL SERR", mal);
if (err)
goto fail2;
err = request_irq(mal->txde_irq, hdlr_txde, irqflags, "MAL TX DE", mal);
if (err)
goto fail3;
err = request_irq(mal->txeob_irq, mal_txeob, 0, "MAL TX EOB", mal);
if (err)
goto fail4;
err = request_irq(mal->rxde_irq, hdlr_rxde, irqflags, "MAL RX DE", mal);
if (err)
goto fail5;
err = request_irq(mal->rxeob_irq, mal_rxeob, 0, "MAL RX EOB", mal);
if (err)
goto fail6;
>
> /* Enable all MAL SERR interrupt sources */
> if (mal->version == 2)
> --
> 1.5.5.1
> --
> 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
--
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