| Message ID | 1263403629-18827-1-git-send-email-chripell@fsfe.org |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
Hi Christian, Christian Pellegrin wrote: > This patch adds error checking of ctrlmode values for CAN devices. As > an example all availabe bits are implemented in the mcp251x driver. > > Signed-off-by: Christian Pellegrin <chripell@fsfe.org> > --- > drivers/net/can/at91_can.c | 1 + > drivers/net/can/bfin_can.c | 1 + > drivers/net/can/dev.c | 13 +++++++++++-- > drivers/net/can/mcp251x.c | 32 ++++++++++++++++++++++++++++++-- > drivers/net/can/mscan/mscan.c | 1 + > drivers/net/can/sja1000/sja1000.c | 1 + > drivers/net/can/ti_hecc.c | 1 + > drivers/net/can/usb/ems_usb.c | 1 + > include/linux/can/dev.h | 2 ++ > 9 files changed, 49 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c > index f728749..a2f29a3 100644 > --- a/drivers/net/can/at91_can.c > +++ b/drivers/net/can/at91_can.c > @@ -1073,6 +1073,7 @@ static int __init at91_can_probe(struct platform_device *pdev) > priv->can.bittiming_const = &at91_bittiming_const; > priv->can.do_set_bittiming = at91_set_bittiming; > priv->can.do_set_mode = at91_set_mode; > + priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES; > priv->reg_base = addr; > priv->dev = dev; > priv->clk = clk; > diff --git a/drivers/net/can/bfin_can.c b/drivers/net/can/bfin_can.c > index 7e1926e..bf7f9ba 100644 > --- a/drivers/net/can/bfin_can.c > +++ b/drivers/net/can/bfin_can.c > @@ -603,6 +603,7 @@ struct net_device *alloc_bfin_candev(void) > priv->can.bittiming_const = &bfin_can_bittiming_const; > priv->can.do_set_bittiming = bfin_can_set_bittiming; > priv->can.do_set_mode = bfin_can_set_mode; > + priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES; > > return dev; > } > diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c > index c1bb29f..e25ab98 100644 > --- a/drivers/net/can/dev.c > +++ b/drivers/net/can/dev.c > @@ -587,13 +587,22 @@ static int can_changelink(struct net_device *dev, > > if (data[IFLA_CAN_CTRLMODE]) { > struct can_ctrlmode *cm; > + u32 ctrlmode; > > /* Do not allow changing controller mode while running */ > if (dev->flags & IFF_UP) > return -EBUSY; > cm = nla_data(data[IFLA_CAN_CTRLMODE]); > - priv->ctrlmode &= ~cm->mask; > - priv->ctrlmode |= cm->flags; > + if (cm->flags & ~priv->ctrlmode_supported) > + return -EOPNOTSUPP; > + ctrlmode = priv->ctrlmode & ~cm->mask; > + ctrlmode |= cm->flags; > + if (priv->check_ctrlmode) { > + err = priv->check_ctrlmode(ctrlmode); > + if (err < 0) > + return err; > + } Could you please explain, what the "check_ctrlmode" callback is good for. For me it seems useless, at a first glance. Without, also the variable ctrlmode is not necessary. > + priv->ctrlmode = ctrlmode; > } > > if (data[IFLA_CAN_BITTIMING]) { > diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c > index afa2fa4..0ad8786 100644 > --- a/drivers/net/can/mcp251x.c > +++ b/drivers/net/can/mcp251x.c > @@ -538,10 +538,14 @@ static void mcp251x_set_normal_mode(struct spi_device *spi) > > if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) { > /* Put device into loopback mode */ > - mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_LOOPBACK); > + } else if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) { > + /* Put device into liste-only mode */ > + mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_LISTEN_ONLY); > } else { > /* Put device into normal mode */ > - mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_NORMAL); > + mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_NORMAL | > + (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT ? > + CANCTRL_OSM : 0)); > > /* Wait for the device to enter normal mode */ > timeout = jiffies + HZ; > @@ -917,6 +921,25 @@ static void mcp251x_irq_work_handler(struct work_struct *ws) > } > } > > +static inline int mcp251x_match_bits(u32 arg, u32 mask) > +{ > + if ((arg & mask) == mask) > + return 1; > + return 0; > +} > + > +static int mcp251x_check_ctrlmode(u32 ctrlmode) > +{ > + if (mcp251x_match_bits(ctrlmode, CAN_CTRLMODE_LOOPBACK | > + CAN_CTRLMODE_LISTENONLY) || > + mcp251x_match_bits(ctrlmode, CAN_CTRLMODE_LOOPBACK | > + CAN_CTRLMODE_ONE_SHOT) || > + mcp251x_match_bits(ctrlmode, CAN_CTRLMODE_ONE_SHOT | > + CAN_CTRLMODE_LISTENONLY)) > + return -EOPNOTSUPP; In another mail you mentioned, that "ENOTSUPP" does not result in a useful user space error message. I checked "errno.h" of my Linux distribution and there ENOTSUPP is not even defined, in contrast to "EOPNOTSUPP". Hm, ENOTSUPP is used in may places in the kernel and also in some CAN source files and I think we should fix that. > + return 0; > +} For me this check never fails if "priv->can.ctrlmode_supported" is set properly. Or have I missed something? The rest looks ok. Thanks, Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jan 13, 2010 at 8:56 PM, Wolfgang Grandegger <wg@grandegger.com> wrote: > Hi Christian, Hi, > Could you please explain, what the "check_ctrlmode" callback is good > for. For me it seems useless, at a first glance. Without, also the > variable ctrlmode is not necessary. It's needed to avoid unmeaningful combinations like loop-back + listen-only (it's quite sure you won't hear nothing and this mode isn't even programmable on the mcp251x for example; other could be more subtle, like having one-shot mode on or off doesn't make any difference both with loop-back or listen-only). Of course I can hard-code this but if we add some other fancy options with controller-specific behavior I'm not sure all the possible cases could be catch. On the other hand it's supposed that people who set ctrlmode more or less know what are they doing, so this test may be superflous. If you think so I can just eliminate it. >> + return -EOPNOTSUPP; > > In another mail you mentioned, that "ENOTSUPP" does not result in a > useful user space error message. I checked "errno.h" of my Linux > distribution and there ENOTSUPP is not even defined, in contrast to > "EOPNOTSUPP". Hm, ENOTSUPP is used in may places in the kernel and also > in some CAN source files and I think we should fix that. > I agree, perhaps this should be pointed out on LKML too (even if we risk to ignite a flame war between kernel and glibc folks ;-) ) >> + return 0; >> +} > > For me this check never fails if "priv->can.ctrlmode_supported" is set > properly. Or have I missed something? > as I said above it catches the case when the device is put in loop-back and listen-only at the same time.
christian pellegrin wrote: > On Wed, Jan 13, 2010 at 8:56 PM, Wolfgang Grandegger <wg@grandegger.com> wrote: >> Hi Christian, > > Hi, > >> Could you please explain, what the "check_ctrlmode" callback is good >> for. For me it seems useless, at a first glance. Without, also the >> variable ctrlmode is not necessary. > > It's needed to avoid unmeaningful combinations like loop-back + > listen-only (it's quite sure you won't hear nothing and this mode > isn't even programmable on the mcp251x for example; other could be > more subtle, like having one-shot mode on or off doesn't make any > difference both with loop-back or listen-only). Of course I can > hard-code this but if we add some other fancy options with > controller-specific behavior I'm not sure all the possible cases could > be catch. On the other hand it's supposed that people who set ctrlmode > more or less know what are they doing, so this test may be superflous. > If you think so I can just eliminate it. I see. I really don't like the extra callback. Currently, it seems overkill to me. In principle, we could also do some checks in the device open function, if needed. >>> + return -EOPNOTSUPP; >> In another mail you mentioned, that "ENOTSUPP" does not result in a >> useful user space error message. I checked "errno.h" of my Linux >> distribution and there ENOTSUPP is not even defined, in contrast to >> "EOPNOTSUPP". Hm, ENOTSUPP is used in may places in the kernel and also >> in some CAN source files and I think we should fix that. >> > > I agree, perhaps this should be pointed out on LKML too (even if we > risk to ignite a flame war between kernel and glibc folks ;-) ) I found some links on that subject. Obviously, there is ENOTSUP and EOPNOTSUPP in the glibc, which are equal, but no ENOTSUPP. I tend to replace ENOTSUPP with EOPNOTSUPP, EINVAL or ENOSYS, what ever is more appropriate. Do you feel that EINVAL is more appropriate for the case above? >>> + return 0; >>> +} >> For me this check never fails if "priv->can.ctrlmode_supported" is set >> properly. Or have I missed something? >> > > as I said above it catches the case when the device is put in > loop-back and listen-only at the same time. Let's keep it simple for the moment. In future, we may need something more sophisticated for feature and capability handling anyhow. Wolfgang. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jan 14, 2010 at 3:36 PM, Wolfgang Grandegger <wg@grandegger.com> wrote: > > I see. I really don't like the extra callback. Currently, it seems > overkill to me. In principle, we could also do some checks in the device > open function, if needed. > ack, I'll prepare a v2 patch soon. > > I found some links on that subject. Obviously, there is ENOTSUP and > EOPNOTSUPP in the glibc, which are equal, but no ENOTSUPP. I tend to > replace ENOTSUPP with EOPNOTSUPP, EINVAL or ENOSYS, what ever is more > appropriate. Do you feel that EINVAL is more appropriate for the case above? > I'm perfectly ok with EOPNOTSUPP as you suggested.
diff --git a/drivers/net/can/at91_can.c b/drivers/net/can/at91_can.c index f728749..a2f29a3 100644 --- a/drivers/net/can/at91_can.c +++ b/drivers/net/can/at91_can.c @@ -1073,6 +1073,7 @@ static int __init at91_can_probe(struct platform_device *pdev) priv->can.bittiming_const = &at91_bittiming_const; priv->can.do_set_bittiming = at91_set_bittiming; priv->can.do_set_mode = at91_set_mode; + priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES; priv->reg_base = addr; priv->dev = dev; priv->clk = clk; diff --git a/drivers/net/can/bfin_can.c b/drivers/net/can/bfin_can.c index 7e1926e..bf7f9ba 100644 --- a/drivers/net/can/bfin_can.c +++ b/drivers/net/can/bfin_can.c @@ -603,6 +603,7 @@ struct net_device *alloc_bfin_candev(void) priv->can.bittiming_const = &bfin_can_bittiming_const; priv->can.do_set_bittiming = bfin_can_set_bittiming; priv->can.do_set_mode = bfin_can_set_mode; + priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES; return dev; } diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c index c1bb29f..e25ab98 100644 --- a/drivers/net/can/dev.c +++ b/drivers/net/can/dev.c @@ -587,13 +587,22 @@ static int can_changelink(struct net_device *dev, if (data[IFLA_CAN_CTRLMODE]) { struct can_ctrlmode *cm; + u32 ctrlmode; /* Do not allow changing controller mode while running */ if (dev->flags & IFF_UP) return -EBUSY; cm = nla_data(data[IFLA_CAN_CTRLMODE]); - priv->ctrlmode &= ~cm->mask; - priv->ctrlmode |= cm->flags; + if (cm->flags & ~priv->ctrlmode_supported) + return -EOPNOTSUPP; + ctrlmode = priv->ctrlmode & ~cm->mask; + ctrlmode |= cm->flags; + if (priv->check_ctrlmode) { + err = priv->check_ctrlmode(ctrlmode); + if (err < 0) + return err; + } + priv->ctrlmode = ctrlmode; } if (data[IFLA_CAN_BITTIMING]) { diff --git a/drivers/net/can/mcp251x.c b/drivers/net/can/mcp251x.c index afa2fa4..0ad8786 100644 --- a/drivers/net/can/mcp251x.c +++ b/drivers/net/can/mcp251x.c @@ -538,10 +538,14 @@ static void mcp251x_set_normal_mode(struct spi_device *spi) if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) { /* Put device into loopback mode */ - mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_LOOPBACK); + } else if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) { + /* Put device into liste-only mode */ + mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_LISTEN_ONLY); } else { /* Put device into normal mode */ - mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_NORMAL); + mcp251x_write_reg(spi, CANCTRL, CANCTRL_REQOP_NORMAL | + (priv->can.ctrlmode & CAN_CTRLMODE_ONE_SHOT ? + CANCTRL_OSM : 0)); /* Wait for the device to enter normal mode */ timeout = jiffies + HZ; @@ -917,6 +921,25 @@ static void mcp251x_irq_work_handler(struct work_struct *ws) } } +static inline int mcp251x_match_bits(u32 arg, u32 mask) +{ + if ((arg & mask) == mask) + return 1; + return 0; +} + +static int mcp251x_check_ctrlmode(u32 ctrlmode) +{ + if (mcp251x_match_bits(ctrlmode, CAN_CTRLMODE_LOOPBACK | + CAN_CTRLMODE_LISTENONLY) || + mcp251x_match_bits(ctrlmode, CAN_CTRLMODE_LOOPBACK | + CAN_CTRLMODE_ONE_SHOT) || + mcp251x_match_bits(ctrlmode, CAN_CTRLMODE_ONE_SHOT | + CAN_CTRLMODE_LISTENONLY)) + return -EOPNOTSUPP; + return 0; +} + static const struct net_device_ops mcp251x_netdev_ops = { .ndo_open = mcp251x_open, .ndo_stop = mcp251x_stop, @@ -948,6 +971,11 @@ static int __devinit mcp251x_can_probe(struct spi_device *spi) priv->can.bittiming_const = &mcp251x_bittiming_const; priv->can.do_set_mode = mcp251x_do_set_mode; priv->can.clock.freq = pdata->oscillator_frequency / 2; + priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES | + CAN_CTRLMODE_LOOPBACK | CAN_CTRLMODE_LISTENONLY; + if (pdata->model == CAN_MCP251X_MCP2515) + priv->can.ctrlmode_supported |= CAN_CTRLMODE_ONE_SHOT; + priv->can.check_ctrlmode = mcp251x_check_ctrlmode; priv->net = net; dev_set_drvdata(&spi->dev, priv); diff --git a/drivers/net/can/mscan/mscan.c b/drivers/net/can/mscan/mscan.c index 40827c1..6b7dd57 100644 --- a/drivers/net/can/mscan/mscan.c +++ b/drivers/net/can/mscan/mscan.c @@ -686,6 +686,7 @@ struct net_device *alloc_mscandev(void) priv->can.bittiming_const = &mscan_bittiming_const; priv->can.do_set_bittiming = mscan_do_set_bittiming; priv->can.do_set_mode = mscan_do_set_mode; + priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES; for (i = 0; i < TX_QUEUE_SIZE; i++) { priv->tx_queue[i].id = i; diff --git a/drivers/net/can/sja1000/sja1000.c b/drivers/net/can/sja1000/sja1000.c index 345304d..ace103a 100644 --- a/drivers/net/can/sja1000/sja1000.c +++ b/drivers/net/can/sja1000/sja1000.c @@ -567,6 +567,7 @@ struct net_device *alloc_sja1000dev(int sizeof_priv) priv->can.bittiming_const = &sja1000_bittiming_const; priv->can.do_set_bittiming = sja1000_set_bittiming; priv->can.do_set_mode = sja1000_set_mode; + priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES; if (sizeof_priv) priv->priv = (void *)priv + sizeof(struct sja1000_priv); diff --git a/drivers/net/can/ti_hecc.c b/drivers/net/can/ti_hecc.c index 7d370e3..8332e24 100644 --- a/drivers/net/can/ti_hecc.c +++ b/drivers/net/can/ti_hecc.c @@ -909,6 +909,7 @@ static int ti_hecc_probe(struct platform_device *pdev) priv->can.bittiming_const = &ti_hecc_bittiming_const; priv->can.do_set_mode = ti_hecc_do_set_mode; priv->can.do_get_state = ti_hecc_get_state; + priv->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES; ndev->irq = irq->start; ndev->flags |= IFF_ECHO; diff --git a/drivers/net/can/usb/ems_usb.c b/drivers/net/can/usb/ems_usb.c index ddb17e2..bfab283 100644 --- a/drivers/net/can/usb/ems_usb.c +++ b/drivers/net/can/usb/ems_usb.c @@ -1022,6 +1022,7 @@ static int ems_usb_probe(struct usb_interface *intf, dev->can.bittiming_const = &ems_usb_bittiming_const; dev->can.do_set_bittiming = ems_usb_set_bittiming; dev->can.do_set_mode = ems_usb_set_mode; + dev->can.ctrlmode_supported = CAN_CTRLMODE_3_SAMPLES; netdev->flags |= IFF_ECHO; /* we support local echo */ diff --git a/include/linux/can/dev.h b/include/linux/can/dev.h index 7e7c98a..6bc2993 100644 --- a/include/linux/can/dev.h +++ b/include/linux/can/dev.h @@ -38,6 +38,7 @@ struct can_priv { enum can_state state; u32 ctrlmode; + u32 ctrlmode_supported; int restart_ms; struct timer_list restart_timer; @@ -46,6 +47,7 @@ struct can_priv { int (*do_set_mode)(struct net_device *dev, enum can_mode mode); int (*do_get_state)(const struct net_device *dev, enum can_state *state); + int (*check_ctrlmode) (u32 ctrlmode); unsigned int echo_skb_max; struct sk_buff **echo_skb;
This patch adds error checking of ctrlmode values for CAN devices. As an example all availabe bits are implemented in the mcp251x driver. Signed-off-by: Christian Pellegrin <chripell@fsfe.org> --- drivers/net/can/at91_can.c | 1 + drivers/net/can/bfin_can.c | 1 + drivers/net/can/dev.c | 13 +++++++++++-- drivers/net/can/mcp251x.c | 32 ++++++++++++++++++++++++++++++-- drivers/net/can/mscan/mscan.c | 1 + drivers/net/can/sja1000/sja1000.c | 1 + drivers/net/can/ti_hecc.c | 1 + drivers/net/can/usb/ems_usb.c | 1 + include/linux/can/dev.h | 2 ++ 9 files changed, 49 insertions(+), 4 deletions(-)