linux-kernel - Re: [PATCH] can: ti_hecc: use timestamp based rx-offloading

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] [day] [month] [year] [list]

Message-ID: <c52cda86-8889-63a7-ce10-e1d10444f6d2@victronenergy.com>
Date:   Fri, 26 Jul 2019 20:10:47 +0000
From:   Jeroen Hofstee <jhofstee@...tronenergy.com>
To:     Marc Kleine-Budde <mkl@...gutronix.de>,
        "linux-can@...r.kernel.org" <linux-can@...r.kernel.org>
CC:     Anant Gole <anantgole@...com>, AnilKumar Ch <anilkumar@...com>,
        Wolfgang Grandegger <wg@...ndegger.com>,
        "David S. Miller" <davem@...emloft.net>,
        "open list:NETWORKING DRIVERS" <netdev@...r.kernel.org>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] can: ti_hecc: use timestamp based rx-offloading

Hello Marc,

On 7/26/19 1:48 PM, Marc Kleine-Budde wrote:
> On 4/29/19 2:03 PM, Jeroen Hofstee wrote:
>
>> @@ -744,8 +652,8 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
>>   	struct net_device *ndev = (struct net_device *)dev_id;
>>   	struct ti_hecc_priv *priv = netdev_priv(ndev);
>>   	struct net_device_stats *stats = &ndev->stats;
>> -	u32 mbxno, mbx_mask, int_status, err_status;
>> -	unsigned long ack, flags;
>> +	u32 mbxno, mbx_mask, int_status, err_status, stamp;
>> +	unsigned long flags, rx_pending;
>>   
>>   	int_status = hecc_read(priv,
>>   		(priv->use_hecc1int) ? HECC_CANGIF1 : HECC_CANGIF0);
>> @@ -769,11 +677,11 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
>>   			spin_lock_irqsave(&priv->mbx_lock, flags);
>>   			hecc_clear_bit(priv, HECC_CANME, mbx_mask);
>>   			spin_unlock_irqrestore(&priv->mbx_lock, flags);
>> -			stats->tx_bytes += hecc_read_mbx(priv, mbxno,
>> -						HECC_CANMCF) & 0xF;
>> +			stamp = hecc_read_stamp(priv, mbxno);
>> +			stats->tx_bytes += can_rx_offload_get_echo_skb(&priv->offload,
>> +									mbxno, stamp);
>>   			stats->tx_packets++;
>>   			can_led_event(ndev, CAN_LED_EVENT_TX);
>> -			can_get_echo_skb(ndev, mbxno);
>>   			--priv->tx_tail;
>>   		}
>>   
>> @@ -784,12 +692,11 @@ static irqreturn_t ti_hecc_interrupt(int irq, void *dev_id)
>>   		((priv->tx_head & HECC_TX_MASK) == HECC_TX_MASK)))
>>   			netif_wake_queue(ndev);
>>   
>> -		/* Disable RX mailbox interrupts and let NAPI reenable them */
>> -		if (hecc_read(priv, HECC_CANRMP)) {
>> -			ack = hecc_read(priv, HECC_CANMIM);
>> -			ack &= BIT(HECC_MAX_TX_MBOX) - 1;
>> -			hecc_write(priv, HECC_CANMIM, ack);
>> -			napi_schedule(&priv->napi);
>> +		/* offload RX mailboxes and let NAPI deliver them */
>> +		while ((rx_pending = hecc_read(priv, HECC_CANRMP))) {
>> +			can_rx_offload_irq_offload_timestamp(&priv->offload,
>> +							     rx_pending);
>> +			hecc_write(priv, HECC_CANRMP, rx_pending);
> Can prepare a patch to move the RMP writing into the mailbox_read()
> function. This makes the mailbox available a bit earlier and works
> better for memory pressure situations, where no skb can be allocated.


For my understanding, is there any other reason for alloc_can_skb,
to fail, besides being out of memory. I couldn't easily find an other
limit enforced on it.

If it is actually _moved_, as you suggested, it does loose the ability to
handle the newly received messages while the messages are read
in the same interrupt, so it needs to interrupt again. That will work,
but seems a bit a silly thing to do.

Perhaps we can do both? Mark the mailbox as available in
mailbox_read, so it is available as soon as possible and clear
them all in the irq (under the assumption that alloc_can_skb
failure means real big trouble, why would we want to keep the
old messages around and eventually ignore the new messages?).

Another question, not related to this patch, but this driver..
Most of the times the irq handles 1 or sometimes 2 messages.
Do you happen to know if it is possible to optionally delay the irq
a bit in the millisecond range, so more work can be done in a single
interrupt? Since there are now 28 rx hardware mailboxes available,
it shouldn't run out easily.

And as last, I guess you want a patch which applies to
linux-can-next/testing, right?

With kind regards,

Jeroen