hermes: modem_qrtr: Uniformly check for response errors
Prior to this change, ModemQrtr did not have uniform logic in checking
for errors in QMI responses and logging any relevant messages. This
change unifies this logic and also modified qmi2cpp so that
QmiUimCommand can be used either as a uint16_t or a string.
BUG=chromium:1085825
TEST=All unit tests are passing.
Change-Id: Ia7a140da21f59545ae8582fc419d6ee4d455b322
Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/platform2/+/2281070
Tested-by: Alex Khouderchah <akhouderchah@chromium.org>
Reviewed-by: Eric Caruso <ejcaruso@chromium.org>
Commit-Queue: Pavan Holla <pholla@google.com>
diff --git a/hermes/modem_qrtr.cc b/hermes/modem_qrtr.cc
index fa03eea..af3d6fa 100644
--- a/hermes/modem_qrtr.cc
+++ b/hermes/modem_qrtr.cc
@@ -27,8 +27,14 @@
constexpr uint8_t kEsimSlot = 0x01;
constexpr uint8_t kInvalidChannel = -1;
-bool DecodeSuccessful(const uim_qmi_result& qmi_result) {
- return (qmi_result.result == 0);
+bool CheckMessageSuccess(QmiUimCommand cmd, const uim_qmi_result& qmi_result) {
+ if (qmi_result.result == 0) {
+ return true;
+ }
+
+ LOG(ERROR) << cmd.ToString()
+ << " response contained error: " << qmi_result.error;
+ return false;
}
} // namespace
@@ -364,7 +370,7 @@
}
// TODO(akhouderchah) try to avoid the unnecessary copy from *_resp to vector
- switch (static_cast<QmiUimCommand>(qmi_type)) {
+ switch (qmi_type) {
case QmiUimCommand::kReset:
// TODO(akhouderchah) implement a service reset
break;
@@ -383,27 +389,26 @@
}
void ModemQrtr::ReceiveQmiOpenLogicalChannel(const qrtr_packet& packet) {
+ QmiUimCommand cmd(QmiUimCommand::kOpenLogicalChannel);
+ if (current_state_ != State::kLogicalChannelPending) {
+ LOG(ERROR) << "Received unexpected QMI UIM response: " << cmd.ToString()
+ << " in state " << current_state_;
+ return;
+ }
+
uim_open_logical_channel_resp resp;
unsigned int id;
- if (qmi_decode_message(
- &resp, &id, &packet, QMI_RESPONSE,
- static_cast<uint16_t>(QmiUimCommand::kOpenLogicalChannel),
- uim_open_logical_channel_resp_ei) < 0) {
- LOG(ERROR) << "Failed to decode QMI UIM response kOpenLogicalChannel";
+ if (qmi_decode_message(&resp, &id, &packet, QMI_RESPONSE, cmd,
+ uim_open_logical_channel_resp_ei) < 0) {
+ LOG(ERROR) << "Failed to decode QMI UIM response: " << cmd.ToString();
+ return;
+ } else if (!CheckMessageSuccess(cmd, resp.result)) {
return;
}
- if (current_state_ != State::kLogicalChannelPending) {
- LOG(ERROR) << "Received unexpected QMI UIM response: "
- << "kOpenLogicalChannel in state " << current_state_;
- return;
- }
- if (!DecodeSuccessful(resp.result)) {
- LOG(ERROR) << "kOpenLogicalChannel response indicating error";
- return;
- }
+
if (!resp.channel_id_valid) {
- LOG(ERROR) << "QMI UIM response for kOpenLogicalChannel contained an "
- << "invalid channel id";
+ LOG(ERROR) << "QMI UIM response for " << cmd.ToString()
+ << " contained an invalid channel id";
return;
}
@@ -412,6 +417,7 @@
}
void ModemQrtr::ReceiveQmiSendApdu(const qrtr_packet& packet) {
+ QmiUimCommand cmd(QmiUimCommand::kSendApdu);
CHECK(tx_queue_.size());
// Ensure that the queued element is for a kSendApdu command
TxInfo* base_info = tx_queue_[0].info_.get();
@@ -422,14 +428,11 @@
uim_send_apdu_resp resp;
unsigned int id;
ApduTxInfo* info = static_cast<ApduTxInfo*>(base_info);
- qmi_decode_message(&resp, &id, &packet, QMI_RESPONSE,
- static_cast<uint16_t>(QmiUimCommand::kSendApdu),
- uim_send_apdu_resp_ei);
- if (!DecodeSuccessful(resp.result)) {
- LOG(ERROR) << "Failed to decode received QMI UIM response: kSendApdu";
+ if (!qmi_decode_message(&resp, &id, &packet, QMI_RESPONSE, cmd,
+ uim_send_apdu_resp_ei)) {
+ LOG(ERROR) << "Failed to decode QMI UIM response: " << cmd.ToString();
return;
- } else if (resp.result.error) {
- LOG(ERROR) << "kSendApdu response contained error: " << resp.result.error;
+ } else if (!CheckMessageSuccess(cmd, resp.result)) {
return;
}
diff --git a/hermes/qmi_uim.h b/hermes/qmi_uim.h
index 549b9a5..f361f18 100644
--- a/hermes/qmi_uim.h
+++ b/hermes/qmi_uim.h
@@ -9,13 +9,38 @@
#include <cstdint>
+#include <base/logging.h>
#include <libqrtr.h>
-enum class QmiUimCommand : uint16_t {
- kReset = 0x00,
- kSendApdu = 0x3B,
- kOpenLogicalChannel = 0x42,
+class QmiUimCommand {
+ public:
+ enum Code : uint16_t {
+ kReset = 0x00,
+ kSendApdu = 0x3B,
+ kOpenLogicalChannel = 0x42,
+ };
+
+ QmiUimCommand(Code code) : code_(code) {} // NOLINT(runtime/explicit)
+ operator Code() const { return code_; }
+
+ const char* ToString() {
+ switch (code_) {
+ case kReset:
+ return "Reset";
+ case kSendApdu:
+ return "SendApdu";
+ case kOpenLogicalChannel:
+ return "OpenLogicalChannel";
+ default:
+ CHECK(false) << "Unrecognized value: " << code_;
+ return "";
+ }
+ }
+
+ private:
+ Code code_;
};
+
constexpr int kBufferDataSize = 260;
struct uim_qmi_result {
