On 08/20/2014 07:19 AM, Kalle Valo wrote:
Ben Greear <greearb@xxxxxxxxxxxxxxx> writes:
On 08/20/2014 12:29 AM, Kalle Valo wrote:
Ben Greear <greearb@xxxxxxxxxxxxxxx> writes:
+ /* Gather dbg-log */ + dump_tlv = (struct ath10k_tlv_dump_data
*)(buf + sofar); + dump_tlv->type =
cpu_to_le32(ATH10K_FW_CRASH_DUMP_DBGLOG); + dump_tlv->tlv_len =
cpu_to_le32(sizeof(crash_data->dbglog_entry_data));
Hmm should this really be sizeof()? Not next_idx (which
effectively defines number of bytes of the dbglog)?
I haven't tried decoding this yet, but we may need to know the
next_idx to decode this properly.
I really don't like the idea of having untested code in ath10k.
Buggy code is okay (preferably document the known bugs when
submitting code), but untested code is not.
It would be quite a waste of time to keep re-writing a user-space dump
tool while the patch set is in this much churn, I think.
It's not like ath10k is going to blow up, but maybe the decode is less
useful in that we cannot actually get a useful debuglog decode until
the problem is resolved. We get zero firmware dbglog messages
currently, so nothing is lost.
Of course ath10k won't blow up, but every patch and every line of code
increases the maintenance cost. For example, if I had known that the
dbglog dump in your patchset doesn't work at all, I would have dropped
it immediately and saved a lot of time of everyone's time, especially
mine.
So next time you send me untested code PLEASE clearly mention that. I
assume that the developer has tested the submitted code well enough that
it really works. Hoping it to work is not enough! Unless you are
Johannes, Linus or Davem :)
I know that I mentioned I have not actually written the decode tool.
I did test my original patches to make sure they properly dumped
a data file and did not crash the kernel or otherwise muck up the
driver.
And, actually I think I could make a decode tool able to decode the
dbglog as written, especially if the next-idx is included...the messages
are normalized enough that I can just read ints until I find something
that is a legit start of message. Not super clean, but kernel would
remain simpler.
I think we would make more progress if we were slightly less picky on
style issues and details for patches, and just make more
improvements after the initial patches go in. It would certainly
make it easier to do things like write and test and improve the decode
tool.
Thanks,
Ben
--
Ben Greear <greearb@xxxxxxxxxxxxxxx>
Candela Technologies Inc http://www.candelatech.com
--
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
