[PATCH 1/8] ath10k: cleanup ath10k_pci_wait_for_target_init()

[Michal Kazior]

On 26 March 2014 10:28, Kalle Valo <kvalo at qca.qualcomm.com> wrote:
> Michal Kazior <michal.kazior at tieto.com> writes:

[...]

>>> -       while (wait_limit-- &&
>>> -              !(ioread32(ar_pci->mem + FW_INDICATOR_ADDRESS) &
>>> -                FW_IND_INITIALIZED)) {
>>> +       timeout = jiffies + msecs_to_jiffies(wait);
>>> +
>>> +       do {
>>> +               val = ath10k_pci_read32(ar, FW_INDICATOR_ADDRESS);
>>> +               if (val == FW_IND_INITIALIZED)
>>> +                       break;

Ah, for some reason I've missed this earlier. You seem to replace &
with ==. I wonder if that's okay?

FW indicator may contain FW_IND_EVENT_PENDING (i.e. firmware crashed
bit) too. It might be a good idea to check for that too and return a
different errno?


>>> +
>>
>> It might be worth to add:
>>
>>  if (val == 0xFFFFFFFF)
>>    return -EIO;
>
> What does receiving 0xFFFFFFFF mean here? PCI bus kaput?

A simple grepping (find drivers/net/ethernet/ -type f -name '*.[ch]' |
xargs grep -nC5 ioread32 | grep -C5 -i 0xffffffff) turns out results
like this:

 drivers/net/ethernet/cisco/enic/vnic_rq.c-200-  }
 drivers/net/ethernet/cisco/enic/vnic_rq.c-201-
 drivers/net/ethernet/cisco/enic/vnic_rq.c-202-  /* Use current
fetch_index as the ring starting point */
 drivers/net/ethernet/cisco/enic/vnic_rq.c:203:  fetch_index =
ioread32(&rq->ctrl->fetch_index);
 drivers/net/ethernet/cisco/enic/vnic_rq.c-204-
 drivers/net/ethernet/cisco/enic/vnic_rq.c-205-  if (fetch_index ==
0xFFFFFFFF) { /* check for hardware gone  */
 drivers/net/ethernet/cisco/enic/vnic_rq.c-206-          /* Hardware
surprise removal: reset fetch_index */
 drivers/net/ethernet/cisco/enic/vnic_rq.c-207-          fetch_index = 0;
 drivers/net/ethernet/cisco/enic/vnic_rq.c-208-  }


> Do we really want to stop trying after receiving that? What harm would
> it cause to keep on trying? We would return an error anyway after the
> timeout, right?

Yes. It's harmless but having the check allows to discern a real
timeout (target doesn't wake up for some reason) and a pci-e link
issue.


Michał