On Wed, Jul 02, 2014 at 09:57:50AM +0200, Marc Kleine-Budde wrote:
> On 07/02/2014 08:20 AM, Dong Aisheng wrote:
> [...]
>
> >>> +static int m_can_do_rx_poll(struct net_device *dev, int quota)
> >>> +{
> >>> + struct m_can_priv *priv = netdev_priv(dev);
> >>> + struct net_device_stats *stats = &dev->stats;
> >>> + struct sk_buff *skb;
> >>> + struct can_frame *frame;
> >>> + u32 rxfs, flags, fgi;
> >>> + u32 num_rx_pkts = 0;
> >>> +
> >>> + rxfs = m_can_read(priv, M_CAN_RXF0S);
> >>> + if (!(rxfs & RXFS_FFL_MASK)) {
> >>> + netdev_dbg(dev, "no messages in fifo0\n");
> >>> + return 0;
> >>> + }
> >>> +
> >>> + while ((rxfs & RXFS_FFL_MASK) && (quota > 0)) {
> >>> + netdev_dbg(dev, "fifo0 status 0x%x\n", rxfs);
> >>
> >> Please remove the netdev_dbg(), once the driver is stable it should be
> >> of no use.
> >>
> >
> > Got it.
> >
> >>> + if (rxfs & RXFS_RFL)
> >>> + netdev_warn(dev, "Rx FIFO 0 Message Lost\n");
> >>
> >> What does that mean? Can you still rx the message if it's lost?
> >>
> >
> > It just warns that there's a message lost, but there are still other
> > message in fifo to receive.
> >
> >>> +
> >>> + skb = alloc_can_skb(dev, &frame);
> >>> + if (!skb) {
> >>> + stats->rx_dropped++;
> >>> + return -ENOMEM;
> >>
> >> Have a look at the user of m_can_do_rx_poll() and how it makes use of
> >> the return value.
> >>
> >
> > Right, thanks for spotting it.
> >
> >>> + }
> >>> +
> >>> + fgi = (rxfs & RXFS_FGI_MASK) >> RXFS_FGI_OFF;
> >>
> >> BTW: Is this a _real_ fifo? Or evolution of the c_can/d_can interface
> >> where it's not a fifo at all.
> >>
> >
> > Yes, it is real fifo in the message ram.
> >
> >>> + flags = readl(priv->mram_base + priv->rxf0_off + fgi * 16);
> >>> + if (flags & RX_BUF_XTD)
> >>> + frame->can_id = (flags & CAN_EFF_MASK) | CAN_EFF_FLAG;
> >>> + else
> >>> + frame->can_id = (flags >> 18) & CAN_SFF_MASK;
> >>> + netdev_dbg(dev, "R0 0x%x\n", flags);
> >>
> >> please remove dbg
> >>> +
> >>> + if (flags & RX_BUF_RTR) {
> >>> + frame->can_id |= CAN_RTR_FLAG;
> >>> + } else {
> >>> + flags = readl(priv->mram_base +
> >>> + priv->rxf0_off + fgi * 16 + 0x4);
> >>> + frame->can_dlc = get_can_dlc((flags >> 16) & 0x0F);
> >>> + netdev_dbg(dev, "R1 0x%x\n", flags);
> >>
> >> please remove
> >>
> >>> +
> >>> + *(u32 *)(frame->data + 0) = readl(priv->mram_base +
> >>> + priv->rxf0_off + fgi * 16 + 0x8);
> >>> + *(u32 *)(frame->data + 4) = readl(priv->mram_base +
> >>> + priv->rxf0_off + fgi * 16 + 0xC);
> >>
> >>
> >> can you create a wrapper function to hide the pointer arithmetics here?
> >> Somethig like m_can_read_fifo()
> >>
> >
> > Yes, i could make a wrapper function for it.
> >
> >>> + netdev_dbg(dev, "R2 0x%x\n", *(u32 *)(frame->data + 0));
> >>> + netdev_dbg(dev, "R3 0x%x\n", *(u32 *)(frame->data + 4));
> >>> + }
> >>> +
> >>> + /* acknowledge rx fifo 0 */
> >>> + m_can_write(priv, M_CAN_RXF0A, fgi);
> >>> +
> >>> + netif_receive_skb(skb);
> >>> + netdev_dbg(dev, "new packet received\n");
> >>> +
> >>> + stats->rx_packets++;
> >>> + stats->rx_bytes += frame->can_dlc;
> >>
> >> Please move the stats handling in front of netif_receive_skb() as the
> >> skb and thus frame is not a valid pointer anymore.
> >>
> >
> > Good catch!
> > Will change it.
> >
> >>> +
> >>> + can_led_event(dev, CAN_LED_EVENT_RX);
> >>
> >> Please move out of the loop so that it is just called once (if a CAN
> >> frame is rx'ed) per m_can_do_rx_poll().
>
> > Why that?
> > The purpose is calling it for each new packet received.
>
> It will only trigger LED blinking, and tglx pointed out, that we don't
> need the overhead of calling it for every CAN frame.
>
Okay, got it, thanks for this information.
Regards
Dong Aisheng
> Marc
>
> --
> Pengutronix e.K. | Marc Kleine-Budde |
> Industrial Linux Solutions | Phone: +49-231-2826-924 |
> Vertretung West/Dortmund | Fax: +49-5121-206917-5555 |
> Amtsgericht Hildesheim, HRA 2686 | http://www.pengutronix.de |
>
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
