On 4.10.2019 16.54, Marc Kleine-Budde wrote:
> On 10/4/19 11:02 AM, Anssi Hannula wrote:
>> Userspace can signal with CAN_CTRLMODE_BERR_REPORTING whether they need
>> reporting of bus errors (CAN_ERR_BUSERROR) or not.
>>
>> However, xilinx_can driver currently always sends CAN_ERR_BUSERROR
>> frames to userspace on bus errors.
>>
>> To improve performance on error conditions when bus error reporting is
>> not needed, avoid sending CAN_ERR_BUSERROR frames unless requested via
>> CAN_CTRLMODE_BERR_REPORTING.
>>
>> The error interrupt is still kept enabled as there is no dedicated state
>> transition interrupt, but just disabling error frame submission still
>> yields a significant performance improvement. In a simple test with
>> continuous bus errors and no userspace programs reading/writing CAN I
>> saw system CPU load reduced by 1/3.
>>
>> Tested on a ZynqMP board with CAN-FD v1.0.
>>
>> Signed-off-by: Anssi Hannula <anssi.hannula@xxxxxxxxxx>
>> ---
>> drivers/net/can/xilinx_can.c | 84 +++++++++++++++++++-----------------
>> 1 file changed, 45 insertions(+), 39 deletions(-)
>>
>> diff --git a/drivers/net/can/xilinx_can.c b/drivers/net/can/xilinx_can.c
>> index 911b34316c9d..9b9ec07f7e5b 100644
>> --- a/drivers/net/can/xilinx_can.c
>> +++ b/drivers/net/can/xilinx_can.c
>> @@ -471,6 +471,10 @@ static int xcan_chip_start(struct net_device *ndev)
>> return err;
>>
>> /* Enable interrupts */
>> + /* We enable the ERROR interrupt even with CAN_CTRLMODE_BERR_REPORTING
>> + * disabled as there is no dedicated interrupt for a state change to
>> + * ERROR_WARNING/ERROR_PASSIVE.
>> + */
>> ier = XCAN_IXR_TXOK_MASK | XCAN_IXR_BSOFF_MASK |
>> XCAN_IXR_WKUP_MASK | XCAN_IXR_SLP_MASK |
>> XCAN_IXR_ERROR_MASK | XCAN_IXR_RXOFLW_MASK |
>> @@ -981,11 +985,10 @@ static void xcan_err_interrupt(struct net_device *ndev, u32 isr)
>> {
>> struct xcan_priv *priv = netdev_priv(ndev);
>> struct net_device_stats *stats = &ndev->stats;
>> - struct can_frame *cf;
>> - struct sk_buff *skb;
>> + struct can_frame cf;
>> u32 err_status;
>>
>> - skb = alloc_can_err_skb(ndev, &cf);
>> + memset(&cf, 0, sizeof(cf));
> This change is unrelated to the one described. Please move these into a
> seperate patch.
>
It is related - the SKB allocation has to be delayed as we no longer
know at this point whether the SKB is needed or not.
So a temporary can_frame is used instead.
Or should I still move it to a separate patch along lines of "delay CAN
error frame SKB allocation to accommodate a following commit where we do
not want to always allocate it"?
I'd side on keeping it together, but your call :)
--
Anssi Hannula / Bitwise Oy
+358 503803997
