| 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: <4A0BDE79.4060606@grandegger.com>
Date: Thu, 14 May 2009 11:03:53 +0200
From: Wolfgang Grandegger <wg@...ndegger.com>
To: Jonathan Corbet <corbet@....net>
CC: netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
Oliver Hartkopp <oliver.hartkopp@...kswagen.de>
Subject: Re: [PATCH v2 4/7] [PATCH 4/8] can: Driver for the SJA1000 CAN controller
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.
--
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