linux-kernel - RE: [PATCH v4] can: xilinx CAN controller support.

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
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Wed, 26 Feb 2014 14:46:43 +0000
From:	Appana Durga Kedareswara Rao <appana.durga.rao@...inx.com>
To:	Marc Kleine-Budde <mkl@...gutronix.de>,
	"wg@...ndegger.com" <wg@...ndegger.com>,
	Michal Simek <michals@...inx.com>,
	"grant.likely@...aro.org" <grant.likely@...aro.org>,
	"robh+dt@...nel.org" <robh+dt@...nel.org>,
	"linux-can@...r.kernel.org" <linux-can@...r.kernel.org>
CC:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>
Subject: RE: [PATCH v4] can: xilinx CAN controller support.

Hi Marc,


> -----Original Message-----
> From: Marc Kleine-Budde [mailto:mkl@...gutronix.de]
> Sent: Wednesday, February 26, 2014 6:52 PM
> To: Appana Durga Kedareswara Rao; wg@...ndegger.com; Michal Simek;
> grant.likely@...aro.org; robh+dt@...nel.org; linux-can@...r.kernel.org
> Cc: netdev@...r.kernel.org; linux-arm-kernel@...ts.infradead.org; linux-
> kernel@...r.kernel.org; devicetree@...r.kernel.org
> Subject: Re: [PATCH v4] can: xilinx CAN controller support.
>
> On 02/26/2014 02:07 PM, Appana Durga Kedareswara Rao wrote:
> >> This loop looks broken. Can you explain how it works.
> >>
> >> What it shoud do is:
> >> We have put (priv->tx_head - priv->tx_tail) CAN frames into the FIFO.
> >> This means at maximum there could be this amount of CAN frames which
> >> have been successfully transmitted. For every cycle in this while
> >> loop you
> >> should:
> >> a) check if a CAN frame has successfully been transmitted
> >>    (as this CAN core uses a FIFO it should be "oldest")
> >>    A read_reg() of some kind is missing in your loop.
> >> b) if needed, remove this event from the FIFO or
> >>    mark the interrupt as done. Whatever you hardware needs.
> >> c) update your statistics
> >> d) Use can_get_echo_skb to push this frame into the networking stack
> >> e) As a CAN frame has been transmitted successfully, wake the tx_queue.
> >>
> >>> +   while (priv->tx_head - priv->tx_tail > 0) {
> >>> +           if (isr & XCAN_IXR_TXFLL_MASK) {
> >>> +                   priv->write_reg(priv, XCAN_ICR_OFFSET,
> >>> +                                   XCAN_IXR_TXFLL_MASK);
> >>> +                   netif_stop_queue(ndev);
> >>
> >> Why do you stop the queue here? A CAN frame has successfully been
> >> transmitted, there should be room in the FIFO.
> >>
> >>> +                   break;
> >>> +           }
> >>> +           can_get_echo_skb(ndev, priv->tx_tail %
> >>> +                                   priv->xcan_echo_skb_max_tx);
> >>> +           priv->tx_tail++;
> >>> +   }
> >>> +
> >
> > The below are the bit fields available for the Transmit FIFO.
> > 1) In the ISR(interrupt status register)  Tx Ok interrupt and Tx fifo full
> interrupt.
> > 2) in the SR(Status Register) Tx fifo full condition.
> >
> >
> > I am modifying the entire tx interrupt logic to like below.
> >
> > static void xcan_tx_interrupt(struct net_device *ndev, u32 isr) {
> >         struct xcan_priv *priv = netdev_priv(ndev);
> >         struct net_device_stats *stats = &ndev->stats;
> >
> >             while (priv->tx_head - priv->tx_tail > 0) {
> >                 if (isr & XCAN_IXR_TXFLL_MASK) {
> >                         priv->write_reg(priv, XCAN_ICR_OFFSET,
> >                                         XCAN_IXR_TXFLL_MASK);
> >                           break;
> >                 }
> >                 can_get_echo_skb(ndev, priv->tx_tail %
> >                                         priv->xcan_echo_skb_max_tx);
> >                 priv->tx_tail++;
> >          stats->tx_packets++;
> >         netif_wake_queue(ndev);
> >               can_led_event(ndev, CAN_LED_EVENT_TX);
> >
> >         }
>
> You just need to wake the queue once.

Ok
>
> > }
> >
> >
> > Are you Ok with the above logic?
>
> No, how can you tell how many frames have been transmitted?

There is no register to read how many can frames are transmitted.
The only way to know Is by reading this parameter (stats->tx_packets++;) through ip command
ip -d -s link show can
Or using ifconfig command.

At the h/w level it can transmit Max upto 64 packets (Max fifo depth)
We need to monitor the Tx fifo full bit in the ISR(Interrupt Status Register)  or Tx fifo full bit in the SR(Status Register) and if it is full we need to stop the queue that is
I am doing in the _xmit by reading the status register before proceeding the   packet transmission.

Regards,
Kedar.


>
> 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   |



This email and any attachments are intended for the sole use of the named recipient(s) and contain(s) confidential information that may be proprietary, privileged or copyrighted under applicable law. If you are not the intended recipient, do not read, copy, or forward this email message or any attachments. Delete this email message and any attachments immediately.