Message-ID: <20090513155205.4bf06c25@bike.lwn.net>
Date:	Wed, 13 May 2009 15:52:05 -0600
From:	Jonathan Corbet <corbet@....net>
To:	Wolfgang Grandegger <wg@...ndegger.com>
Cc:	netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
	Wolfgang Grandegger <wg@...ndegger.com>,
	Oliver Hartkopp <oliver.hartkopp@...kswagen.de>
Subject: Re: [PATCH v2 4/7] [PATCH 4/8] can: Driver for the SJA1000 CAN
 controller

[Quick drive-by review continues...]

> +
> +static int sja1000_probe_chip(struct net_device *dev)
> +{
> +	struct sja1000_priv *priv = netdev_priv(dev);

Looking down toward the bottom, I see:

> +struct sja1000_priv {
> +	struct can_priv can;

So you're still using the "put the higher-level structure at the top so we
can treat it like either kind of pointer" trick.  I'd still recommend
against that.  Far better to do something like:

	struct can_priv *canpriv = netdev_priv(dev);
	struct sja_1000_priv *priv = container_of(canpriv, struct sja_1000_priv, can);

Of course, you can put that dance into a helper function.

> +	if (dev->base_addr && (priv->read_reg(dev, 0) == 0xFF)) {
> +		printk(KERN_INFO "%s: probing @0x%lX failed\n",
> +		       DRV_NAME, dev->base_addr);
> +		return 0;
> +	}
> +	return 1;
> +}

So zero is an error return?  That's contrary to usual practice.

> +static int set_reset_mode(struct net_device *dev)
> +{
> +	struct sja1000_priv *priv = netdev_priv(dev);
> +	unsigned char status = priv->read_reg(dev, REG_MOD);
> +	int i;
> +
> +	/* disable interrupts */
> +	priv->write_reg(dev, REG_IER, IRQ_OFF);
> +
> +	for (i = 0; i < 100; i++) {
> +		/* check reset bit */
> +		if (status & MOD_RM) {
> +			priv->can.state = CAN_STATE_STOPPED;
> +			return 0;
> +		}
> +
> +		priv->write_reg(dev, REG_MOD, MOD_RM);	/* reset chip */
> +		status = priv->read_reg(dev, REG_MOD);
> +		udelay(10);

Wouldn't you want to read the new state *after* the delay?

> +	}
> +
> +	dev_err(dev->dev.parent, "setting SJA1000 into reset mode failed!\n");
> +	return 1;
> +
> +}
> +
> +static int set_normal_mode(struct net_device *dev)
> +{
> +	struct sja1000_priv *priv = netdev_priv(dev);
> +	unsigned char status = priv->read_reg(dev, REG_MOD);
> +	int i;
> +
> +	for (i = 0; i < 100; i++) {
> +		/* check reset bit */
> +		if ((status & MOD_RM) == 0) {
> +			priv->can.state = CAN_STATE_ERROR_ACTIVE;
> +			/* enable all interrupts */
> +			priv->write_reg(dev, REG_IER, IRQ_ALL);
> +
> +			return 0;
> +		}
> +
> +		/* set chip to normal mode */
> +		priv->write_reg(dev, REG_MOD, 0x00);
> +		status = priv->read_reg(dev, REG_MOD);
> +		udelay(10);

Here too?

> +	}
> +
> +	dev_err(dev->dev.parent, "setting SJA1000 into normal mode failed!\n");
> +	return 1;
> +
> +}
> +

[...]

> +irqreturn_t sja1000_interrupt(int irq, void *dev_id)
> +{
> +	struct net_device *dev = (struct net_device *)dev_id;
> +	struct sja1000_priv *priv = netdev_priv(dev);
> +	struct net_device_stats *stats = &dev->stats;
> +	uint8_t isrc, status;
> +	int n = 0;
> +
> +	/* Shared interrupts and IRQ off? */
> +	if (priv->read_reg(dev, REG_IER) == IRQ_OFF)
> +		return IRQ_NONE;
> +
> +	if (priv->pre_irq)
> +		priv->pre_irq(dev);
> +
> +	while ((isrc = priv->read_reg(dev, REG_IR)) && (n < SJA1000_MAX_IRQ)) {
> +		n++;
> +		status = priv->read_reg(dev, REG_SR);
> +
> +		if (isrc & IRQ_WUI)
> +			dev_warn(dev->dev.parent, "wakeup interrupt\n");

How many of these might you get?  Should this be rate limited?

> +		if (isrc & IRQ_TI) {
> +			/* transmission complete interrupt */
> +			stats->tx_packets++;
> +			can_get_echo_skb(dev, 0);
> +			netif_wake_queue(dev);
> +		}
> +		if (isrc & IRQ_RI) {
> +			/* receive interrupt */
> +			while (status & SR_RBS) {
> +				sja1000_rx(dev);
> +				status = priv->read_reg(dev, REG_SR);
> +			}
> +		}
> +		if (isrc & (IRQ_DOI | IRQ_EI | IRQ_BEI | IRQ_EPI | IRQ_ALI)) {
> +			/* error interrupt */
> +			if (sja1000_err(dev, isrc, status))
> +				break;
> +		}
> +	}
> +
> +	if (priv->post_irq)
> +		priv->post_irq(dev);
> +
> +	if (n >= SJA1000_MAX_IRQ)
> +		dev_dbg(dev->dev.parent, "%d messages handled in ISR", n);
> +
> +	return (n) ? IRQ_HANDLED : IRQ_NONE;
> +}
> +EXPORT_SYMBOL_GPL(sja1000_interrupt);
> +
> +static int sja1000_open(struct net_device *dev)
> +{
> +	struct sja1000_priv *priv = netdev_priv(dev);
> +	int err;
> +
> +	/* set chip into reset mode */
> +	set_reset_mode(dev);
> +
> +	/* common open */
> +	err = open_candev(dev);
> +	if (err)
> +		return err;
> +
> +	/* register interrupt handler, if not done by the device driver */
> +	if (!(priv->flags & SJA1000_CUSTOM_IRQ_HANDLER)) {
> +		err = request_irq(dev->irq, &sja1000_interrupt, IRQF_SHARED,
> +				  dev->name, (void *)dev);
> +		if (err)
> +			return -EAGAIN;

If you return here you fail, but you've not undone open_candev().  Looking
there, it seems no harm will be done - until somebody changes open_candev()
someday. 

> +	}
> +
> +	/* init and start chi */
> +	sja1000_start(dev);
> +	priv->open_time = jiffies;
> +
> +	netif_start_queue(dev);
> +
> +	return 0;
> +}
> +

[...]

> +/*
> + * SJA1000 private data structure
> + */
> +struct sja1000_priv {
> +	struct can_priv can;
> +	long open_time;
> +	struct sk_buff *echo_skb;
> +
> +	u8 (*read_reg) (const struct net_device *dev, int reg);
> +	void (*write_reg) (const struct net_device *dev, int reg, u8 val);
> +	void (*pre_irq) (const struct net_device *dev);
> +	void (*post_irq) (const struct net_device *dev);

What are the locking rules for functions like ->read_reg() now?  Entirely
up to the lower level?  Would be good to document that near the structure
definition. 

> +
> +	void *priv;		/* for board-specific data */
> +	struct net_device *dev;
> +
> +	u8 ocr;
> +	u8 cdr;
> +	u32 flags;

The meaning of these fields is not exactly clear.

> +};

jon
--
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