re: crypto: qat - Intel(R) QAT transport code

re: crypto: qat - Intel(R) QAT transport code

[Dan Carpenter]

Hello Tadeusz Struk,

The patch a672a9dc872e: "crypto: qat - Intel(R) QAT transport code"
from Jun 5, 2014, leads to the following static checker warning:

	drivers/crypto/qat/qat_common/adf_transport.c:412 adf_init_bank()
	error: potentially using uninitialized 'coalesc_enabled'.

drivers/crypto/qat/qat_common/adf_transport.c
   407          /* Enable IRQ coalescing always. This will allow to use
   408           * the optimised flag and coalesc register.
   409           * If it is disabled in the config file just use min time value */
   410          if (adf_get_cfg_int(accel_dev, "Accelerator0",
   411                              ADF_ETRMGR_COALESCING_ENABLED_FORMAT,
   412                              bank_num, &coalesc_enabled) && coalesc_enabled)
                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
This condition is reversed, so it only enables coalescing on error.

Probably this feature has not been tested yet.

   413                  adf_enable_coalesc(bank, "Accelerator0", bank_num);
   414          else
   415                  bank->irq_coalesc_timer = ADF_COALESCING_MIN_TIME;
   416  

regards,
dan carpenter

Re: crypto: qat - Intel(R) QAT transport code

[Tadeusz Struk]

On 12/02/2014 04:21 AM, Dan Carpenter wrote:
> drivers/crypto/qat/qat_common/adf_transport.c
>    407          /* Enable IRQ coalescing always. This will allow to use
>    408           * the optimised flag and coalesc register.
>    409           * If it is disabled in the config file just use min time value */
>    410          if (adf_get_cfg_int(accel_dev, "Accelerator0",
>    411                              ADF_ETRMGR_COALESCING_ENABLED_FORMAT,
>    412                              bank_num, &coalesc_enabled) && coalesc_enabled)
>                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This condition is reversed, so it only enables coalescing on error.

Hello Dan,
Yes, you are correct. Looks like we never enable interrupt coalescing.
Thanks for reporting the issue. Patch fixing this will be out soon.
Regards,
Tadeusz

Re: crypto: qat - Intel(R) QAT transport code

[Tadeusz Struk]

On 12/02/2014 08:49 AM, Tadeusz Struk wrote:
> On 12/02/2014 04:21 AM, Dan Carpenter wrote:
>> drivers/crypto/qat/qat_common/adf_transport.c
>>    407          /* Enable IRQ coalescing always. This will allow to use
>>    408           * the optimised flag and coalesc register.
>>    409           * If it is disabled in the config file just use min time value */
>>    410          if (adf_get_cfg_int(accel_dev, "Accelerator0",
>>    411                              ADF_ETRMGR_COALESCING_ENABLED_FORMAT,
>>    412                              bank_num, &coalesc_enabled) && coalesc_enabled)
>>                     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>> This condition is reversed, so it only enables coalescing on error.
> 
> Hello Dan,
> Yes, you are correct. Looks like we never enable interrupt coalescing.
> Thanks for reporting the issue. Patch fixing this will be out soon.
> Regards,
> Tadeusz
> 
Hi,
I had to take a closer look to see what's going on there.
We do enable coalescing always, just as the comment states (in function
adf_enable_ring_irq(), line 105). The adf_enable_coalesc() function only
reads the coalescing timer from the configuration and stores it in the
bank->irq_coalesc_timer.

Your point is still valid - the condition is reversed so we have always
used ADF_COALESCING_MIN_TIME.
What's also missing is initialization of coalesc_enabled and the
adf_enable_coalesc() function should really be called
adf_get_coalesc_timer()
I'll send a patch to fix it.
Thanks again.
Regards,
Tadeusz