James Tavares has uploaded this change for review. ( https://gerrit.osmocom.org/c/simtrace2/+/26865 )
Change subject: firmware: bugfix: disable cardemu comms in local SIM mode ......................................................................
firmware: bugfix: disable cardemu comms in local SIM mode
This change prevents contention on the ISO7816 bus by disabling the card emulation state machine when the SIM switch is in the local mode. Without this change, the card emulation firmware can clobber ISO7816 communications and cause contention with certain (but not all) SIM cards.
Changes: - Add 'enabled' flag to cardemu instance that is set/cleared by usb_command_sim_select() (the only place where sim switch occurs). - Flag is initialized as false (disabled) by default, to match local SIM mode default. - When card emulation is disabled, force SIM VCC to be "OFF", SIM RESET as "not in RESET", and drop bytes bytes received on the ISO7816 interface (but do service buffers).
Change-Id: I4010f988712eac4a6af8568ccd60062f9de62449 --- M firmware/libcommon/source/mode_cardemu.c 1 file changed, 31 insertions(+), 13 deletions(-)
git pull ssh://gerrit.osmocom.org:29418/simtrace2 refs/changes/65/26865/1
diff --git a/firmware/libcommon/source/mode_cardemu.c b/firmware/libcommon/source/mode_cardemu.c index 8e4acf9..00a062e 100644 --- a/firmware/libcommon/source/mode_cardemu.c +++ b/firmware/libcommon/source/mode_cardemu.c @@ -83,10 +83,21 @@ #ifdef DETECT_VCC_BY_ADC uint32_t vcc_uv; #endif + + /*! real-time state of VCC I/O line, irrespective of enabled flag */ bool vcc_active; + + /*! last VCC state we reported to the card emu state machine (conditioned by enabled flag) */ bool vcc_active_last; + + /*! real-time state of RST I/O line, irrespective of enabled flag */ bool rst_active; + + /*! last RST state we reported to the card emu state machine (conditioned by enabled flag) */ bool rst_active_last; + + /*! flag indicating whether this instance should perform card emulation, or not */ + bool enabled; };
struct cardem_inst cardem_inst[] = { @@ -505,19 +516,25 @@ #endif /* DETECT_VCC_BY_ADC */
-/* called from main loop; dispatches card I/O state changes to card_emu from main loop */ +/** + * called from main loop; dispatches card I/O state changes to card_emu from main loop. + * NOTE: conditions I/O state on the ci->enabled flag; if the instance is disabled, we assume VCC is + * disabled and the device is not in reset + */ static void process_io_statechg(struct cardem_inst *ci) { - if (ci->vcc_active != ci->vcc_active_last) { - card_emu_io_statechg(ci->ch, CARD_IO_VCC, ci->vcc_active); + const bool vcc_active = ci->vcc_active && ci->enabled; + if (vcc_active != ci->vcc_active_last) { + card_emu_io_statechg(ci->ch, CARD_IO_VCC, vcc_active); /* FIXME do this for real */ - card_emu_io_statechg(ci->ch, CARD_IO_CLK, ci->vcc_active); - ci->vcc_active_last = ci->vcc_active; + card_emu_io_statechg(ci->ch, CARD_IO_CLK, vcc_active); + ci->vcc_active_last = vcc_active; }
- if (ci->rst_active != ci->rst_active_last) { - card_emu_io_statechg(ci->ch, CARD_IO_RST, ci->rst_active); - ci->rst_active_last = ci->rst_active; + const bool rst_active = ci->rst_active && ci->enabled; + if (rst_active != ci->rst_active_last) { + card_emu_io_statechg(ci->ch, CARD_IO_RST, rst_active); + ci->rst_active_last = rst_active; } }
@@ -764,10 +781,8 @@ if (msgb_l2len(msg) < sizeof(*mss)) return -1;
- if (mss->remote_sim) - sim_switch_use_physical(ci->num, 0); - else - sim_switch_use_physical(ci->num, 1); + ci->enabled = mss->remote_sim ? true : false; + sim_switch_use_physical(ci->num, !ci->enabled);
return 0; } @@ -911,7 +926,10 @@ } uint8_t byte = rbuf_read(&ci->rb); __enable_irq(); - card_emu_process_rx_byte(ci->ch, byte); + + if (ci->enabled) { + card_emu_process_rx_byte(ci->ch, byte); + } //TRACE_ERROR("%uRx%02x\r\n", i, byte); }
laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/simtrace2/+/26865 )
Change subject: firmware: bugfix: disable cardemu comms in local SIM mode ......................................................................
Patch Set 1: Code-Review+1
(2 comments)
nice catch, I think some minor cleanup would be appreciated.
https://gerrit.osmocom.org/c/simtrace2/+/26865/1/firmware/libcommon/source/m... File firmware/libcommon/source/mode_cardemu.c:
https://gerrit.osmocom.org/c/simtrace2/+/26865/1/firmware/libcommon/source/m... PS1, Line 87: /*! real-time state of VCC I/O line, irrespective of enabled flag */ normally we would split this in a separate cosmetic patch only adding the comments, to keep it separate from actual functional changes.
https://gerrit.osmocom.org/c/simtrace2/+/26865/1/firmware/libcommon/source/m... PS1, Line 100: ; indeed, it is hard to see that an actual flag was added, because the reviewer (at least I in my first iteration) assumed that all added here were just some comments.
James Tavares has posted comments on this change. ( https://gerrit.osmocom.org/c/simtrace2/+/26865 )
Change subject: firmware: bugfix: disable cardemu comms in local SIM mode ......................................................................
Patch Set 1:
(2 comments)
Patch Set 1: Code-Review+1
(2 comments)
nice catch, I think some minor cleanup would be appreciated.
Thank you! Please let me know specifically what you would like changed.
https://gerrit.osmocom.org/c/simtrace2/+/26865/1/firmware/libcommon/source/m... File firmware/libcommon/source/mode_cardemu.c:
https://gerrit.osmocom.org/c/simtrace2/+/26865/1/firmware/libcommon/source/m... PS1, Line 87: /*! real-time state of VCC I/O line, irrespective of enabled flag */
normally we would split this in a separate cosmetic patch only adding the comments, to keep it separ […]
Understood, however in this case this commit changes the previously understood usage of some of these variables, and how they interact with each another, hence why I am documenting them. It would not make sense to submit a separate change request for just documentation.
https://gerrit.osmocom.org/c/simtrace2/+/26865/1/firmware/libcommon/source/m... PS1, Line 100: ;
indeed, it is hard to see that an actual flag was added, because the reviewer (at least I in my firs […]
Per comment above, I believe these changes should be reviewed as one change request.
Attention is currently required from: jtavares. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/simtrace2/+/26865 )
Change subject: firmware: bugfix: disable cardemu comms in local SIM mode ......................................................................
Patch Set 1:
(1 comment)
Patchset:
PS1: sorry for the delay, now getting back to having a look at this. I now realize we haven't seen this problem as it only shows up on the actual simtrace2 device, since QMOD/OWHW/... have switches in front of the UART: if set to "local' SIM mode, the SAM3 UART is not connected. Only the SIMtrace2 device has the "older" architecture.
Attention is currently required from: laforge. jtavares has posted comments on this change. ( https://gerrit.osmocom.org/c/simtrace2/+/26865 )
Change subject: firmware: bugfix: disable cardemu comms in local SIM mode ......................................................................
Patch Set 1:
(4 comments)
Patchset:
PS1:
sorry for the delay, now getting back to having a look at this. […]
Thanks! simtrace2 has the additional complexity of being able to run sniff firmware, as well, and so that might be why the hardware does not disconnect the SAM3 UART.
In any evident, are you OK with merging this solution for the simtrace2?
Also, you have two comments on the change that are still marked "Unresolved". I believe they are addressed, but I wasn't sure what the etiquette was here with respect to marking other people's comments as "resolved", and so I have left them as unresolved.
PS1: Hello,
Looking for a merge of this important fix for simtrace2. I had posted comments back in January, but I guess I neglected to press the "REPLY" button. Doing so now. Sorry for the delay.
Regards, James
File firmware/libcommon/source/mode_cardemu.c:
https://gerrit.osmocom.org/c/simtrace2/+/26865/comment/73fef2e9_e71e05dc PS1, Line 87: /*! real-time state of VCC I/O line, irrespective of enabled flag */
Understood, however in this case this commit changes the previously understood usage of some of thes […]
Done
https://gerrit.osmocom.org/c/simtrace2/+/26865/comment/3e7cfd82_c7e36a86 PS1, Line 100: ;
Per comment above, I believe these changes should be reviewed as one change request.
Done
Attention is currently required from: jtavares. laforge has posted comments on this change. ( https://gerrit.osmocom.org/c/simtrace2/+/26865 )
Change subject: firmware: bugfix: disable cardemu comms in local SIM mode ......................................................................
Patch Set 2: Code-Review+2
laforge has submitted this change. ( https://gerrit.osmocom.org/c/simtrace2/+/26865 )
Change subject: firmware: bugfix: disable cardemu comms in local SIM mode ......................................................................
firmware: bugfix: disable cardemu comms in local SIM mode
This change prevents contention on the ISO7816 bus by disabling the card emulation state machine when the SIM switch is in the local mode. Without this change, the card emulation firmware can clobber ISO7816 communications and cause contention with certain (but not all) SIM cards.
Changes: - Add 'enabled' flag to cardemu instance that is set/cleared by usb_command_sim_select() (the only place where sim switch occurs). - Flag is initialized as false (disabled) by default, to match local SIM mode default. - When card emulation is disabled, force SIM VCC to be "OFF", SIM RESET as "not in RESET", and drop bytes bytes received on the ISO7816 interface (but do service buffers).
Change-Id: I4010f988712eac4a6af8568ccd60062f9de62449 --- M firmware/libcommon/source/mode_cardemu.c 1 file changed, 31 insertions(+), 13 deletions(-)
Approvals: Jenkins Builder: Verified laforge: Looks good to me, approved
diff --git a/firmware/libcommon/source/mode_cardemu.c b/firmware/libcommon/source/mode_cardemu.c index 7fd069a..7f52647 100644 --- a/firmware/libcommon/source/mode_cardemu.c +++ b/firmware/libcommon/source/mode_cardemu.c @@ -83,10 +83,21 @@ #ifdef DETECT_VCC_BY_ADC uint32_t vcc_uv; #endif + + /*! real-time state of VCC I/O line, irrespective of enabled flag */ bool vcc_active; + + /*! last VCC state we reported to the card emu state machine (conditioned by enabled flag) */ bool vcc_active_last; + + /*! real-time state of RST I/O line, irrespective of enabled flag */ bool rst_active; + + /*! last RST state we reported to the card emu state machine (conditioned by enabled flag) */ bool rst_active_last; + + /*! flag indicating whether this instance should perform card emulation, or not */ + bool enabled; };
struct cardem_inst cardem_inst[] = { @@ -515,19 +526,25 @@ #endif /* DETECT_VCC_BY_ADC */
-/* called from main loop; dispatches card I/O state changes to card_emu from main loop */ +/** + * called from main loop; dispatches card I/O state changes to card_emu from main loop. + * NOTE: conditions I/O state on the ci->enabled flag; if the instance is disabled, we assume VCC is + * disabled and the device is not in reset + */ static void process_io_statechg(struct cardem_inst *ci) { - if (ci->vcc_active != ci->vcc_active_last) { - card_emu_io_statechg(ci->ch, CARD_IO_VCC, ci->vcc_active); + const bool vcc_active = ci->vcc_active && ci->enabled; + if (vcc_active != ci->vcc_active_last) { + card_emu_io_statechg(ci->ch, CARD_IO_VCC, vcc_active); /* FIXME do this for real */ - card_emu_io_statechg(ci->ch, CARD_IO_CLK, ci->vcc_active); - ci->vcc_active_last = ci->vcc_active; + card_emu_io_statechg(ci->ch, CARD_IO_CLK, vcc_active); + ci->vcc_active_last = vcc_active; }
- if (ci->rst_active != ci->rst_active_last) { - card_emu_io_statechg(ci->ch, CARD_IO_RST, ci->rst_active); - ci->rst_active_last = ci->rst_active; + const bool rst_active = ci->rst_active && ci->enabled; + if (rst_active != ci->rst_active_last) { + card_emu_io_statechg(ci->ch, CARD_IO_RST, rst_active); + ci->rst_active_last = rst_active; } }
@@ -778,10 +795,8 @@ if (msgb_l2len(msg) < sizeof(*mss)) return -1;
- if (mss->remote_sim) - sim_switch_use_physical(ci->num, 0); - else - sim_switch_use_physical(ci->num, 1); + ci->enabled = mss->remote_sim ? true : false; + sim_switch_use_physical(ci->num, !ci->enabled);
return 0; } @@ -925,7 +940,10 @@ } uint8_t byte = rbuf_read(&ci->rb); __enable_irq(); - card_emu_process_rx_byte(ci->ch, byte); + + if (ci->enabled) { + card_emu_process_rx_byte(ci->ch, byte); + } //TRACE_ERROR("%uRx%02x\r\n", i, byte); }