SCardDisconnect(): avoid unneeded polling stop

[fabled]

If disconnecting last context and the card is powered off (or absent), the powerState does not get changed. In this case there is no need to signal the polling thread.

[LudovicRousseau]

I am not sure about this change.
It adds complexity and I do not see any (real) benefit.

Stopping the polling thread takes around 0.2 ms.

[fabled]

Stopping the polling thread takes around 0.2 ms.

Plus all the CPU time the polling thread spends.

The issue for me is that this happens always when a final disconnect in the direct mode happens. For various reasons this is often for me: many different code pieces we have do interrogate the reader for its features. This is also happening if using OpenSC or its PKCS#11 module - for all the connected readers. Which for me is almost up to the supported limit.

When debugging another issue, I just found this is doing a lot of unnecessary extra work and taking CPU time and causing lot of logging that was misleading me.

So I was thinking its better to not do unneeded CPU work, and came up with this. I think this is a win even if it adds a little complexity by means of one extra if.

[LudovicRousseau]

If you think about "all the CPU time the polling thread spends" then you should find it is very important to STOP the polling thread. Instead of that your change will NOT stop the polling thread.

What reader and reader driver are you using?

[fabled]

If you think about "all the CPU time the polling thread spends" then you should find it is very important to STOP the polling thread. Instead of that your change will NOT stop the polling thread.

Seems the commit message was not sufficient, so apologies for the brevity of description.

So the situation which PR tries to address is the following scenario:

1. There is no card in the reader
2. The polling thread is sleeping in the CCID driver via the call at

   PCSC/src/eventhandler.c

   Line 465 in 3e734d7

   ret = rContext->pthCardEvent(rContext->slot, timeout);

   and is waiting for card events using USB interrupting transfer
3. An application does SCardConnect(SCARD_SHARE_DIRECT) to interrogate the reader functionality, then does SCardDisconnect()

In this SCardDisconnect call, the current code will process the following sequence of events:

1. SCardClose() will call the CCID driver's TAG_IFD_STOP_POLLING_THREAD function at

   PCSC/src/winscard.c

   Line 1033 in 3e734d7

   fct(rContext->slot);

   (this tag is somewhat misleadingly named - it does not stop the thread, but just asynchronously cancels the wait event function)

2. This will wake up the event thread waiting which was sleeping at

   PCSC/src/eventhandler.c

   Line 465 in 3e734d7

   ret = rContext->pthCardEvent(rContext->slot, timeout);

3. Since the thread was not cancelled nor there was any actual change in the reader state, it will continue to execute the lines at

   PCSC/src/eventhandler.c

   Lines 472 to 505 in 3e734d7

   	#ifndef DISABLE_ON_DEMAND_POWER_ON
   	/* the card is powered but not used */
   	(void)pthread_mutex_lock(&rContext->powerState_lock);
   	if (POWER_STATE_POWERED == rContext->powerState)
   	{
   	/* power down */
   	IFDPowerICC(rContext, IFD_POWER_DOWN, NULL, NULL);
   	rContext->powerState = POWER_STATE_UNPOWERED;
   	Log1(PCSC_LOG_DEBUG, "powerState: POWER_STATE_UNPOWERED");
   	/* the protocol is unset after a power down */
   	rContext->readerState->cardProtocol = SCARD_PROTOCOL_UNDEFINED;
   	}
   	/* the card was in use */
   	if (POWER_STATE_GRACE_PERIOD == rContext->powerState)
   	{
   	/* the next state should be UNPOWERED unless the
   	* card is used again */
   	rContext->powerState = POWER_STATE_POWERED;
   	Log1(PCSC_LOG_DEBUG, "powerState: POWER_STATE_POWERED");
   	}
   	(void)pthread_mutex_unlock(&rContext->powerState_lock);
   	#endif
   	if (rContext->hLockId == 0xFFFF)
   	{
   	/*
   	* Exit and notify the caller
   	*/
   	Log1(PCSC_LOG_INFO, "Die");
   	rContext->hLockId = 0;
   	(void)pthread_exit(NULL);
   	}

   and

   PCSC/src/eventhandler.c

   Lines 328 to 465 in 3e734d7

   	dwStatus = 0;
   	rv = IFDStatusICC(rContext, &dwStatus);
   	if (rv != SCARD_S_SUCCESS)
   	{
   	Log2(PCSC_LOG_ERROR, "Error communicating to: %s", readerName);
   	/*
   	* Set error status on this reader while errors occur
   	*/
   	rContext->readerState->readerState = SCARD_UNKNOWN;
   	rContext->readerState->cardAtrLength = 0;
   	rContext->readerState->cardProtocol = SCARD_PROTOCOL_UNDEFINED;
   	dwCurrentState = SCARD_UNKNOWN;
   	EHSignalEventToClients();
   	}
   	if (dwStatus & SCARD_ABSENT)
   	{
   	if (dwCurrentState == SCARD_PRESENT ||
   	dwCurrentState == SCARD_UNKNOWN)
   	{
   	/*
   	* Change the status structure
   	*/
   	Log2(PCSC_LOG_INFO, "Card Removed From %s", readerName);
   	/*
   	* Notify the card has been removed
   	*/
   	(void)RFSetReaderEventState(rContext, SCARD_REMOVED);
   	rContext->readerState->cardAtrLength = 0;
   	rContext->readerState->cardProtocol = SCARD_PROTOCOL_UNDEFINED;
   	rContext->readerState->readerState = SCARD_ABSENT;
   	dwCurrentState = SCARD_ABSENT;
   	rContext->readerState->eventCounter++;
   	if (rContext->readerState->eventCounter > 0xFFFF)
   	rContext->readerState->eventCounter = 0;
   	EHSignalEventToClients();
   	}
   	}
   	else if (dwStatus & SCARD_PRESENT)
   	{
   	if (dwCurrentState == SCARD_ABSENT ||
   	dwCurrentState == SCARD_UNKNOWN)
   	{
   	#ifdef DISABLE_AUTO_POWER_ON
   	rContext->readerState->cardAtrLength = 0;
   	rContext->readerState->cardProtocol = SCARD_PROTOCOL_UNDEFINED;
   	rContext->readerState->readerState = SCARD_PRESENT;
   	RFSetPowerState(rContext, POWER_STATE_UNPOWERED);
   	Log1(PCSC_LOG_DEBUG, "powerState: POWER_STATE_UNPOWERED");
   	rv = IFD_SUCCESS;
   	Log1(PCSC_LOG_INFO, "Skip card power on");
   	#else
   	/*
   	* Power and reset the card
   	*/
   	dwAtrLen = sizeof(rContext->readerState->cardAtr);
   	rv = IFDPowerICC(rContext, IFD_POWER_UP,
   	rContext->readerState->cardAtr, &dwAtrLen);
   	rContext->readerState->cardAtrLength = dwAtrLen;
   	/* the protocol is unset after a power on */
   	rContext->readerState->cardProtocol = SCARD_PROTOCOL_UNDEFINED;
   	if (rv == IFD_SUCCESS)
   	{
   	rContext->readerState->readerState = SCARD_PRESENT | SCARD_POWERED | SCARD_NEGOTIABLE;
   	RFSetPowerState(rContext, POWER_STATE_POWERED);
   	Log1(PCSC_LOG_DEBUG, "powerState: POWER_STATE_POWERED");
   	}
   	else
   	{
   	rContext->readerState->readerState = SCARD_PRESENT | SCARD_SWALLOWED;
   	RFSetPowerState(rContext, POWER_STATE_UNPOWERED);
   	Log1(PCSC_LOG_DEBUG, "powerState: POWER_STATE_UNPOWERED");
   	rContext->readerState->cardAtrLength = 0;
   	}
   	#endif
   	dwCurrentState = SCARD_PRESENT;
   	rContext->readerState->eventCounter++;
   	if (rContext->readerState->eventCounter > 0xFFFF)
   	rContext->readerState->eventCounter = 0;
   	Log2(PCSC_LOG_INFO, "Card inserted into %s", readerName);
   	EHSignalEventToClients();
   	if (rv == IFD_SUCCESS)
   	{
   	if (rContext->readerState->cardAtrLength > 0)
   	{
   	LogXxd(PCSC_LOG_INFO, "Card ATR: ",
   	rContext->readerState->cardAtr,
   	rContext->readerState->cardAtrLength);
   	}
   	else
   	Log1(PCSC_LOG_INFO, "Card ATR: (NULL)");
   	}
   	else
   	Log1(PCSC_LOG_ERROR,"Error powering up card.");
   	}
   	}
   	/*
   	* Sharing may change w/o an event pass it on
   	*/
   	if (readerSharing != rContext->contexts)
   	{
   	readerSharing = rContext->contexts;
   	rContext->readerState->readerSharing = readerSharing;
   	EHSignalEventToClients();
   	}
   	if (rContext->pthCardEvent)
   	{
   	int ret;
   	int timeout;
   	#ifndef DISABLE_ON_DEMAND_POWER_ON
   	if (POWER_STATE_POWERED == RFGetPowerState(rContext))
   	/* The card is powered but not yet used */
   	timeout = PCSCLITE_POWER_OFF_GRACE_PERIOD;
   	else
   	/* The card is already in use or not used at all */
   	#endif
   	timeout = PCSCLITE_STATUS_EVENT_TIMEOUT;
   	ret = rContext->pthCardEvent(rContext->slot, timeout);

   including:
   • potentially sleeping 400ms depending on what the driver returned (fixed in latest CCID driver, thanks for merging that PR)
   • sending and receiving status query message via USB, which returns same state and doing nothing additional
   • re-entring sleep state to wait again for interrupts

In context of the CCID driver, both the cancellation of the USB interrupt transfer and then re-arming it are somewhat non-trivial operations as they involve quite a bit of kernel involvement to configure the USB controller accordingly.

This suggested PR will avoid all of the above code execution because the event poller is not waken up after disconnecting SCARD_SHARE_DIRECT context.

Hopefully this explains the intent in good enough detail?

What reader and reader driver are you using?

Various different reader hardware - all of them on the CCID driver.

[LudovicRousseau]

I understand the scenario.
I don't understand why this change is needed. Just because it "involve quite a bit of kernel involvement to configure the USB controller"?

What are you doing with the reader so that it is a problem for you?
Your application keeps doing SCardConnect/SCardDisconnect with no card in the reader?

[fabled]

I don't understand why this change is needed.

It is not needed for correctness. This is purely to reduce CPU cycles used and debug log chatter. Nothing else.