linux-kernel - Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support

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: <1158055669.3032.11.camel@laptopd505.fenrus.org>
Date:	Tue, 12 Sep 2006 12:07:49 +0200
From:	Arjan van de Ven <arjan@...radead.org>
To:	Zang Roy-r61911 <tie-fei.zang@...escale.com>
Cc:	Andrew Morton <akpm@...l.org>, jgarzik <jgarzik@...ox.com>,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [patch 3/3] Add tsi108 On Chip Ethernet device driver support

On Tue, 2006-09-12 at 16:55 +0800, Zang Roy-r61911 wrote:
> The driver for tsi108/9 on chip Ethernet port

Hi,

I have some review comments about your driver; please consider them for
fixing....

> +
> +#undef DEBUG
> +#ifdef DEBUG
> +#define DBG(fmt...) do { printk(fmt); } while(0)
> +#else
> +#define DBG(fmt...) do { } while(0)
> +#endif

please don't do this, there is pr_debug and dev_dbg() for a reason
already. No reason to add a copy.

> +
> +typedef struct net_device net_device;
> +typedef struct sk_buff sk_buff;

Please don't do this...



> +	TSI108_ETH_WRITE_PHYREG(TSI108_MAC_MII_ADDR,
> +				(data->phy << TSI108_MAC_MII_ADDR_PHY) |
> +				(reg << TSI108_MAC_MII_ADDR_REG));
> +	mb();
> +	TSI108_ETH_WRITE_PHYREG(TSI108_MAC_MII_CMD, 0);
> +	mb();
> +	TSI108_ETH_WRITE_PHYREG(TSI108_MAC_MII_CMD, TSI108_MAC_MII_CMD_READ);
> +	mb();

what is this mb() for? do you want them to be an io memory barrier?

> +	while (TSI108_ETH_READ_REG(TSI108_MAC_MII_IND) &
> +	       TSI108_MAC_MII_IND_BUSY) ;

do you want this to be an infinate loop? At minimum the ";" needs to be
a cpu_relax() to avoid the cpu from burning up but it'd be a lot nicer
if this loop wasn't infinite




> +
> +static irqreturn_t tsi108_irq(int irq, void *dev_id, struct pt_regs *regs)
> +{
> +	if (irq == NO_IRQ)
> +		return IRQ_NONE;	/* Not our interrupt */
> +
> +	return tsi108_irq_one(dev_id);
> +}

hi this IRQ_NONE just looks odd, was this really needed?



> +	mb();
> +	while (tsi108_read_mii(data, PHY_CTRL, NULL) & PHY_CTRL_AUTONEG_START)
> +		;

another infinite loop that wants cpu_relax() for sure

> +		spin_unlock_irq(&phy_lock);
> +		msleep(10);
> +		spin_lock_irq(&phy_lock);
> +	}

hmm some places take phy_lock with disabling interrupts, while others
don't. I sort of fear "the others" may be buggy.... are you sure those
are ok?



Greetings,
   Arjan van de Ven

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/