netdev - 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: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 26 Feb 2014 14:22:04 +0100
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	Appana Durga Kedareswara Rao <appana.durga.rao@...inx.com>,
	"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.

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.

> }
> 
> 
> Are you Ok with the above logic?

No, how can you tell how many frames have been transmitted?

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   |


Download attachment "signature.asc" of type "application/pgp-signature" (243 bytes)