On 13 August 2014 16:09, Kalle Valo <kvalo@xxxxxxxxxxxxxxxx> wrote:
> Michal Kazior <michal.kazior@xxxxxxxxx> writes:
>
>> It's not really necessary to have a dedicated irq
>> handler just for the sake of catching early fw
>> crashes. It is now safe to use one handler even
>> during early stages of device boot up.
>>
>> Signed-off-by: Michal Kazior <michal.kazior@xxxxxxxxx>
>
> Nice, this really needs to be cleaned up.
>
>> drivers/net/wireless/ath/ath10k/pci.c | 160 ++++++++--------------------------
>> 1 file changed, 36 insertions(+), 124 deletions(-)
>
> Shouldn't you also remove early_irq_tasklet from struct ath10k_pci?
Oops. Good catch.
>> -static void ath10k_pci_fw_interrupt_handler(struct ath10k *ar)
>> +static bool ath10k_pci_fw_crashed(struct ath10k *ar)
>> {
>> - struct ath10k_pci *ar_pci = ath10k_pci_priv(ar);
>> - u32 fw_indicator;
>> -
>> - fw_indicator = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
>> -
>> - if (fw_indicator & FW_IND_EVENT_PENDING) {
>> - /* ACK: clear Target-side pending event */
>> - ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
>> - fw_indicator & ~FW_IND_EVENT_PENDING);
>> + return ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS) &
>> + FW_IND_EVENT_PENDING;
>> +}
>
> Hehe, in Ben's firmware crash dump patches I renamed
> ath10k_pci_hif_dump_area() to ath10k_pci_firmware_crashed(). So we now
> have two very similarly named functions doing very different :)
>
> I propose that you rename this function to ath10k_pci_is_fw_crashed() or
> ath10k_pci_has_fw_crashed() and I rename ath10k_pci_hif_dump_area() to
> ath10k_pci_fw_crashed_dump(). I think that we have pretty consistent
> naming, no?
I'm okay with that. WIll fix.
>> +static void ath10k_pci_fw_crashed_clear(struct ath10k *ar)
>> +{
>> + ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
>> + (ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS) &
>> + ~FW_IND_EVENT_PENDING));
>
> Please don't embed calls like this, with a temporary variable you get it
> more readable and the resulting code should be the same anyway.
Will fix.
>
>> @@ -2412,14 +2322,6 @@ static int ath10k_pci_wait_for_target_init(struct ath10k *ar)
>> return -EIO;
>> }
>>
>> - if (val & FW_IND_EVENT_PENDING) {
>> - ath10k_warn("device has crashed during init\n");
>> - ath10k_pci_write32(ar, FW_INDICATOR_ADDRESS,
>> - val & ~FW_IND_EVENT_PENDING);
>> - ath10k_pci_hif_dump_area(ar);
>> - return -ECOMM;
>> - }
>
> I'm a bit worried about this. Are you relying now that the target will
> always trigger an interrupt or what? If yes, how can we sure it will
> happen on all possible cases? I would prefer to have some kind of safety
> net anyway, even if it's just a simple warning.
Hmm, I'll keep it. I just remembered it's possible for a device to
fail to fully setup pci link/config and crash without asserting an
interrupt while still providing a trace in the host interest area.
Michał
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
