| 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: <20080904234145.17f55cc2@zod.rchland.ibm.com>
Date: Thu, 4 Sep 2008 23:41:45 -0400
From: Josh Boyer <jwboyer@...ux.vnet.ibm.com>
To: Simon Horman <horms@...ge.net.au>
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 Fri, 5 Sep 2008 12:10:37 +1000
Simon Horman <horms@...ge.net.au> wrote:
> > +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.
I hate checkpatch, but that's easy enough to fix. Though I don't see
what other places I add with that mistake.
> > + 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.
I don't. If Ben complains I'll change it.
> > +
> > + 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);
> }
Indeed they could. Good catch.
> > +
> > 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;
I like it. Much cleaner. I'll fix that up too.
josh
--
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