Marc Kleine-Budde wrote:
> Wolfgang Grandegger wrote:
>>> here "./strip-src -m -i -v 2.6.31" version of the at91 driver,
>>> featuring:
>>> - reworked RX-path
>>> - NAPI for RX and CAN frame errors
>>> TX and CAN state is still managed from irq handler
>>> - CAN chip is turned off after a bus off
>>> the NAPI is stopped via a work_queue, because
>>> NAPI cannot be disabled from IRQ context
>>> (where the bus off is detected).
>> Very nice. Do you really need to stop the NAPI in case of bus-off? You
>> stopped the device and no more message will come in anyhow.
>
> hmmm...I'll try
It would be nice if we could get rid of the work queue. For the MSCAN on
the MPC5200, NAPI is also not disabled on bus-off and the controller
behaves similar (doing automatic bus-off recovery).
>>> I have to figure out if disabling NAPI is possbile from NAPI conext, so
>>> CAN state transition might be moved to NAPI, too.
>
> for the record:
> no, we would get a deadlock then..(waiting in NAPI for NAPI to end)
OK, I see.
>>> Bus off and manual restart via the "ip" utility is working. However the
>>> last remaining bits of the auto recover handling must be removed,
>>> though.
>> Did you follow our recent discussion on how to handle auto recovery? The
>> preferred solution is to *stop* the device if priv->can.restart_ms ==
>> 0. If it's > 0, the hardware should do the recovery and when the state
>> changes to error-active, an RESTARTED error message should be generated.
>
> okay, will change.
>
>>> The driver with NAPI receiving messages with a length of 1 on a 1 Mbit
>>> link without swapping the messages (tested with pengutronix cansequence).
>> Please use Vladislavs' canecho_gen/duts at 125kB/sec, which is more
>> sensitive. Don't forget to fix the unhanded errno ENOBUFS of the write
>> functions.
>
> using select/poll would be better than calling sched_yield
Well, yes, could this easily be achieved? Oliver?
>>> However if I add some "ping -f" load the driver keeps working (tested
>>> with looking at the data bytes in the driver), but the userspace doesn't
>>> get enough cpu cycles so AFAICS the pacakges must be dropped in the
>>> network layer.
>>>
>>> Even on the 1 Mbit link the total number IRQs isn't reduced by NAPI
>>> (less than 1%). But with NAPI I can enable the Acknowledgement Error
>>> even on 1 Mbit, and the system doesn't lock up. It generated ~15K Int/s.
>> With NAPI, message processing is moved from the interrupt to the process
>> context, which efficiently avoids lockups. As longs as CPU resources are
>> available, no messages will be dropped.
>
>>> Adding again "ping -f" load, the CAN interrupts drop to ~11K Int/s,
>>> the Ethernet causes only ~7K Int/s.
>>>
>>> Maybe we can think of some really "slow" polling for such interrupts.
>>> IIRC the SJA1000 has the same problem.
>> I remember some discussion on this topic a long time ago and there it
>> was regarded important to get and inspect all bus errors. Throttling was
>> not accepted.
>
> As we see with the above numbers, we lose about 4000 errors per second
> if ethernet traffic hits the box. I was just thinking about polling this
> particular error if it hits the chip.
This is a general issue and I think we should re-discuss it sooner than
later.
>>> Here's the driver, comments welcome. There are some functions, that
>>> might be moved out of the driver into can/dev.c (alloc_can*frame).
>> There are various general coding style issues:
[snip]
>>> +/* Common registers */
>>> +enum at91_reg {
>>> + AT91_MR = 0x000,
>>> + AT91_IER = 0x004,
>>> + AT91_IDR = 0x008,
>>> + AT91_IMR = 0x00C,
>>> + AT91_SR = 0x010,
>>> + AT91_BR = 0x014,
>>> + AT91_TIM = 0x018,
>>> + AT91_TIMESTP = 0x01C,
>>> + AT91_ECR = 0x020,
>>> + AT91_TCR = 0x024,
>>> + AT91_ACR = 0x028,
>>> +};
>>> +
>>> +/* Mailbox registers (0 <= i <= 15) */
>>> +#define AT91_MMR(i) (enum at91_reg)(0x200 + ((i) * 0x20))
>>> +#define AT91_MAM(i) (enum at91_reg)(0x204 + ((i) * 0x20))
>>> +#define AT91_MID(i) (enum at91_reg)(0x208 + ((i) * 0x20))
>>> +#define AT91_MFID(i) (enum at91_reg)(0x20C + ((i) * 0x20))
>>> +#define AT91_MSR(i) (enum at91_reg)(0x210 + ((i) * 0x20))
>>> +#define AT91_MDL(i) (enum at91_reg)(0x214 + ((i) * 0x20))
>>> +#define AT91_MDH(i) (enum at91_reg)(0x218 + ((i) * 0x20))
>>> +#define AT91_MCR(i) (enum at91_reg)(0x21C + ((i) * 0x20))
>>> +
>>> +/* Register bits */
>>> +#define AT91_MR_AT91EN BIT(0)
>>> +#define AT91_MR_LPM BIT(1)
>>> +#define AT91_MR_ABM BIT(2)
>>> +#define AT91_MR_OVL BIT(3)
>>> +#define AT91_MR_TEOF BIT(4)
>>> +#define AT91_MR_TTM BIT(5)
>>> +#define AT91_MR_TIMFRZ BIT(6)
>>> +#define AT91_MR_DRPT BIT(7)
>>> +
>>> +#define AT91_SR_RBSY BIT(29)
>>> +
>>> +#define AT91_MMR_PRIO_SHIFT (16)
>>> +
>>> +#define AT91_MID_MIDE BIT(29)
>>> +
>>> +#define AT91_MSR_MRTR BIT(20)
>>> +#define AT91_MSR_MABT BIT(22)
>>> +#define AT91_MSR_MRDY BIT(23)
>>> +#define AT91_MSR_MMI BIT(24)
>>> +
>>> +#define AT91_MCR_MRTR BIT(20)
>>> +#define AT91_MCR_MTCR BIT(23)
>>> +
>>> +/* Mailbox Modes */
>>> +enum at91_mb_mode {
>>> + AT91_MB_MODE_DISABLED = 0,
>>> + AT91_MB_MODE_RX = 1,
>>> + AT91_MB_MODE_RX_OVRWR = 2,
>>> + AT91_MB_MODE_TX = 3,
>>> + AT91_MB_MODE_CONSUMER = 4,
>>> + AT91_MB_MODE_PRODUCER = 5,
>>> +};
>>> +
>>> +/* Interrupt mask bits */
>>> +#define AT91_IRQ_MB_RX ((1 << (AT91_MB_RX_LAST + 1)) \
>>> + - (1 << AT91_MB_RX_FIRST))
>>> +#define AT91_IRQ_MB_TX ((1 << (AT91_MB_TX_LAST + 1)) \
>>> + - (1 << AT91_MB_TX_FIRST))
>>> +#define AT91_IRQ_MB_AL (AT91_IRQ_MB_RX | AT91_IRQ_MB_TX)
>>> +
>>> +#define AT91_IRQ_ERRA (1 << 16)
>>> +#define AT91_IRQ_WARN (1 << 17)
>>> +#define AT91_IRQ_ERRP (1 << 18)
>>> +#define AT91_IRQ_BOFF (1 << 19)
>>> +#define AT91_IRQ_SLEEP (1 << 20)
>>> +#define AT91_IRQ_WAKEUP (1 << 21)
>>> +#define AT91_IRQ_TOVF (1 << 22)
>>> +#define AT91_IRQ_TSTP (1 << 23)
>>> +#define AT91_IRQ_CERR (1 << 24)
>>> +#define AT91_IRQ_SERR (1 << 25)
>>> +#define AT91_IRQ_AERR (1 << 26)
>>> +#define AT91_IRQ_FERR (1 << 27)
>>> +#define AT91_IRQ_BERR (1 << 28)
>>> +
>>> +#define AT91_IRQ_ERR_ALL (0x1fff0000)
>>> +#define AT91_IRQ_ERR_FRAME (AT91_IRQ_CERR | AT91_IRQ_SERR | \
>>> + AT91_IRQ_AERR | AT91_IRQ_FERR | AT91_IRQ_BERR)
>>> +#define AT91_IRQ_ERR_LINE (AT91_IRQ_ERRA | AT91_IRQ_WARN | \
>>> + AT91_IRQ_ERRP | AT91_IRQ_BOFF)
>>> +
>>> +#define AT91_IRQ_ALL (0x1fffffff)
>>> +
>>> +struct at91_priv {
>>> + struct can_priv can; /* must be the first member! */
>>> + struct net_device *dev;
>>> + struct napi_struct napi;
>>> +
>>> + void __iomem *reg_base;
>>> +
>>> + u32 reg_sr;
>>> + unsigned int tx_next;
>>> + unsigned int tx_echo;
>>> + unsigned int rx_next;
>>> +
>>> + struct work_struct bus_off_task;
>>> +
>>> + struct clk *clk;
>>> + struct at91_can_data *pdata;
>>> +};
>>> +
>>> +
>>> +static struct can_bittiming_const at91_bittiming_const = {
>>> + .tseg1_min = 4,
>>> + .tseg1_max = 16,
>>> + .tseg2_min = 2,
>>> + .tseg2_max = 8,
>>> + .sjw_max = 4,
>>> + .brp_min = 2,
>>> + .brp_max = 128,
>>> + .brp_inc = 1,
>>> +};
>> Why do you don't use tab here fore aligment of the numbers?
>
> As Wolfram pointed out:
>
>> There are a few of those. Such kind of indentation is ususally
>> rejected upstream, too. If one of the member names ever changes, "="
>> will get misaligned again (or you'd need to alter lines just for
>> beautification which would make git blame less useful). So, one space
>> after the names should do.
I know, but then you probably also want to fix the enums above. In the
file header, I'm more relaxed, especially because defines are usually
aligned as well.
>>> +
>>> +static inline int get_tx_next_mb(struct at91_priv *priv)
>>> +{
>>> + return (priv->tx_next & AT91_NEXT_MB_MASK) + AT91_MB_TX_FIRST;
>>> +}
>>> +
>>> +static inline int get_tx_next_prio(struct at91_priv *priv)
>>> +{
>>> + return (priv->tx_next >> AT91_NEXT_PRIO_SHIFT) & 0xf;
>>> +}
>>> +
>>> +static inline int get_tx_echo_mb(struct at91_priv *priv)
>>> +{
>>> + return (priv->tx_echo & AT91_NEXT_MB_MASK) + AT91_MB_TX_FIRST;
>>> +}
>>> +
>>> +
>>> +static inline u32 at91_read(struct net_device *dev, enum at91_reg reg)
>>> +{
>>> + struct at91_priv *priv = netdev_priv(dev);
>>> + return readl(priv->reg_base + reg);
>>> +}
>>> +
>>> +static inline void
>>> +at91_write(struct net_device *dev, enum at91_reg reg, u32 value)
>>> +{
>>> + struct at91_priv *priv = netdev_priv(dev);
>>> + writel(value, priv->reg_base + reg);
>>> +}
>>> +
>>> +
>>> +static inline void
>>> +set_mb_mode_prio(struct net_device *dev, unsigned int mb,
>>> + enum at91_mb_mode mode, int prio)
>>> +{
>>> + at91_write(dev, AT91_MMR(mb),
>>> + (mode << 24) | (prio << 16));
>> Fits on one line?
>
> it does, fixed
>
>>> +}
>>> +
>>> +static inline void
>>> +set_mb_mode(struct net_device *dev, unsigned int mb, enum at91_mb_mode
>>> mode)
>>> +{
>>> + set_mb_mode_prio(dev, mb, mode, 0);
>>> +}
>>> +
>>> +
>>> +static struct sk_buff *
>>> +alloc_can_frame(struct net_device *dev, struct can_frame **cf)
>>> +{
>>> + struct sk_buff *skb;
>>> +
>>> + skb = netdev_alloc_skb(dev, sizeof(struct can_frame));
>>> + if (unlikely(!skb))
A (rate limited) dev_err would be nice here as well.
>>> + return NULL;
>>> +
>>> + skb->protocol = htons(ETH_P_CAN);
>>> + skb->ip_summed = CHECKSUM_UNNECESSARY;
>>> + *cf = (struct can_frame *)skb_put(skb, sizeof(struct can_frame));
>>> +
>>> + return skb;
>>> +}
>>> +
>>> +
>>> +static struct sk_buff *
>>> +alloc_can_err_frame(struct net_device *dev, struct can_frame **cf)
>>> +{
>>> + struct sk_buff *skb;
>>> +
>>> + skb = alloc_can_frame(dev, cf);
>>> + if (unlikely(!skb))
>>> + return NULL;
>>> +
>>> + memset(*cf, 0, sizeof(struct can_frame));
>>> + (*cf)->can_id = CAN_ERR_FLAG;
>>> + (*cf)->can_dlc = CAN_ERR_DLC;
>>> +
>>> + return skb;
>>> +}
>> Yes, we should put similar functions into dev.[ch]. But I would use the
>> suffix "skb" instead of "frame" because the return value is of type
>> "static struct sk_buff *". Patches are welcome.
>
> fixed. patches against which tree?
First again SVN trunk.
Wolfgang.
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
