Re: [PATCH v2 4/7] [PATCH 4/8] can: Driver for the SJA1000 CAN controller

[Wolfgang Grandegger <wg@grandegger.com>]

Jonathan Corbet wrote:
> [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.

There is no way to initialize the value returned by netdev_priv() as it
does not point to a member of struct net_device. I already commented here:

  http://marc.info/?l=linux-netdev&m=124120212106891&w=2

Have I missed something?

>> +	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.

OK, will fix.

>> +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?

Yes, that makes more sense.

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

Will fix this return value as well.

>> +
>> +}
>> +
>> +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?

None, because the driver does not (yet) support the sleep mode. For that
reason it's a warning.

>> +		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. 

Right, the missing close_candev() does currently not harm but that might
change in the future. Will change.

>> +	}
>> +
>> +	/* 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. 

Yes, it's up to the lower level.

>> +
>> +	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.

I will add a description.

Wolfgang.