-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Wolfgang Grandegger wrote:
>>> 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
works
> 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).
[..]
>>>> 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?
Poll/select/... works already even for sending. No work needed.
BTW: I have a hack that puts sending in blocking mode by default, but
that's a different story.
Vladislavs test program found a bug, which is fixed and now it works
with all of the three tested bitrates 125K, 250K and 1Mbit.
>>>> 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.
ACK,
First let's get some drivers into mainline ;)
>>>> 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.
okay, realligned
>>>> +
>>>> +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.
If we make this a library function or inline, IMHO the user of this
function should care about error handling and/or messaging. I increase
rx_dropped when it happens in the normal rx-path.
>>>> + 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.
What about a git tree? The whole svn situation suxs.
cheers, Marc
- --
Pengutronix e.K. | Marc Kleine-Budde |
Linux Solutions for Science and Industry | Phone: +49-231-2826-924 |
Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkquRtkACgkQjTAFq1RaXHPGDwCgk0+WNHkcG2JXK6swVcmCC0Rp
xHYAoI+pl6cyplwJ4s2yBf6WeBkHIjf/
=x7nn
-----END PGP SIGNATURE-----
_______________________________________________
Socketcan-core mailing list
[email protected]
https://lists.berlios.de/mailman/listinfo/socketcan-core
