| 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
| ||
|
Date: Fri, 28 Feb 2014 13:56:56 +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/28/2014 01:44 PM, Appana Durga Kedareswara Rao wrote:
> Hi Marc,
>
>
>> -----Original Message-----
>> From: Marc Kleine-Budde [mailto:mkl@...gutronix.de]
>> Sent: Friday, February 28, 2014 2:02 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/28/2014 06:50 AM, Appana Durga Kedareswara Rao wrote:
>>> Hi Marc,
>>>
>>>
>>>> -----Original Message-----
>>>> From: Marc Kleine-Budde [mailto:mkl@...gutronix.de]
>>>> Sent: Wednesday, February 26, 2014 9:13 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 03:46 PM, Appana Durga Kedareswara Rao wrote:
>>>>> 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
>>>>
>>>> stats->tx_packets is calculated in the above loop and the loop is
>>>> broken. Let me illustrate the problem:
>>>>
>>>> - xmit is called 10 times in a row
>>>> - this means you have 10 CAN frames in the TX FIFO
>>>> - a single CAN frame gets transmitted
>>>> - you get an interrupt
>>>> - you enter the above routine and loop 10 times and echo the CAN frame
>>>> back into the stack
>>>>
>>>> Now every application sees 10 transmitted packages, but there is only
>>>> one transmitted. Every time you loop you have to check if the CAN
>>>> frame has already been transmitted or not. Is that possible with the
>> hardware?
>>>
>>> The only way to know whether the TX packet is transmitted
>> successfully or not is by using the Tx Ok interrupt from the ISR.
>>> This interrupt will come for every Tx Packet.
>>> So I am thinking of there is no loop required in the TX interrupt routine. As
>> it is called for each and every packet.
>>
>> What happens if the interrupt handler is delayed? For example in a RT
>> enabled system the interrupt handler runs as a thread. There might be other
>> threads with higher priority. The hardware will probably send all CAN
>> frames in the FIFO, so you want to reduce the overhead and loop in the tx
>> complete handler.
>>
> Yes I agree with your comment.
> It will be good to have a loop in the Tx interrupt handler
> I am modifying the Tx interrupt handler 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_TXOK_MASK)) {
> break;
> }
> can_get_echo_skb(ndev, priv->tx_tail %
> priv->xcan_echo_skb_max_tx);
> priv->tx_tail++;
> stats->tx_packets++;
> can_led_event(ndev, CAN_LED_EVENT_TX);
> isr = priv->read_reg(priv, XCAN_ISR_OFFSET);
> }
> netif_wake_queue(ndev);
> }
>
> Are you Ok with this?
Do you have to ACK the interrupt, so that TXOK_MASK is finally cleared?
The use case is here: m CAN frames are put into the FIFO, n out of m CAN
frames are transferred, where n < m. In this case you have to leave the
loop after n, but "priv->tx_head - priv->tx_tail == m".
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)
Powered by blists - more mailing lists