On Thu, Apr 21, 2016 at 11:37:34AM +0300, Mathias Nyman wrote:
> >+static void ucsi_acpi_notify(acpi_handle handle, u32 event, void *data)
> >+{
> >+ struct ucsi *ucsi = data;
> >+ struct ucsi_cci *cci;
> >+
> >+ spin_lock(&ucsi->dev_lock);
> >+
> >+ ucsi->status = UCSI_IDLE;
> >+ cci = &ucsi->data->cci;
> >+
> >+ /*
> >+ * REVISIT: This is not documented behavior, but all known PPMs ACK
> >+ * asynchronous events by sending notification with cleared CCI.
> >+ */
> >+ if (!ucsi->data->raw_cci) {
> >+ if (test_bit(EVENT_PENDING, &ucsi->flags))
> >+ complete(&ucsi->complete);
> >+ else
> >+ dev_WARN(ucsi->dev, "spurious notification\n");
> >+ goto out_unlock;
> >+ }
> >+
> >+ if (test_bit(COMMAND_PENDING, &ucsi->flags)) {
> >+ if (cci->busy) {
>
> So if I understood correctly there is no risk of missing a connector change
> here because a cci->busy bit is set there can be no other ones, right?
Exactly. No other bits can be set together bit the busy bit.
> >+ ucsi->status = UCSI_BUSY;
> >+ complete(&ucsi->complete);
> >+
> >+ goto out_unlock;
> >+ } else if (cci->ack_complete || cci->cmd_complete) {
> >+ /* Error Indication is only valid with commands */
> >+ if (cci->error && cci->cmd_complete)
> >+ ucsi->status = UCSI_ERROR;
> >+
> >+ ucsi->data->ctrl.raw_cmd = 0;
> >+ complete(&ucsi->complete);
> >+ }
> >+ }
> >+
> >+ if (cci->connector_change) {
> >+ struct ucsi_connector *con;
> >+
> >+ /*
> >+ * This is workaround for buggy PPMs that create asynchronous
> >+ * event notifications before OPM has enabled them.
> >+ */
> >+ if (!ucsi->connector)
> >+ goto out_unlock;
> >+
> >+ con = ucsi->connector + (cci->connector_change - 1);
> >+
> >+ /*
> >+ * PPM will not clear the connector specific bit in Connector
> >+ * Change Indication field of CCI until the driver has ACK it,
> >+ * and the driver can not ACK it before it has been processed.
> >+ * The PPM will not generate new events before the first has
> >+ * been acknowledged, even if they are for an other connector.
> >+ * So only one event at a time.
> >+ */
> >+ if (!test_and_set_bit(EVENT_PENDING, &ucsi->flags))
> >+ schedule_work(&con->work);
>
> Was there any possibility that we miss fast consecutive connection change, such
> as a new connection change work not being scheduled if the previous is not yet
> ACKed and EVENT_PENDING flag cleared?
> Or does hw/fw make sure no new connection change notification is queued before
> previous is ACKed?
The FW/HW part is called PPM (Platform Policy Manager) in UCSI terms
(straight from USB PD I guess). The OS end, our driver in practice, is
called OPM (OS Policy Manager).
Yes, the PPM will queue an asynchronous event that is received while
OPM is still processing the first one.
> >+ }
> >+out_unlock:
> >+ spin_unlock(&ucsi->dev_lock);
> >+}
> >+
> >+static int ucsi_ack(struct ucsi *ucsi, u8 cmd)
> >+{
> >+ struct ucsi_control ctrl;
> >+ int ret;
> >+
> >+ ctrl.cmd.cmd = UCSI_ACK_CC_CI;
> >+ ctrl.cmd.length = 0;
> >+ ctrl.cmd.data = cmd;
> >+ ret = ucsi_acpi_cmd(ucsi, &ctrl);
> >+ if (ret)
> >+ return ret;
> >+
> >+ /* Waiting for ACK also with ACK CMD for now */
> >+ ret = wait_for_completion_timeout(&ucsi->complete,
> >+ msecs_to_jiffies(UCSI_TIMEOUT_MS));
> >+ if (!ret)
> >+ return -ETIMEDOUT;
> >+ return 0;
> >+}
> >+
> >+static int ucsi_run_cmd(struct ucsi *ucsi, struct ucsi_control *ctrl,
> >+ void *data, size_t size)
> >+{
> >+ int timeout = UCSI_TIMEOUT_MS;
> >+ u16 err_value = 0;
> >+ int ret;
> >+
> >+ while (test_and_set_bit(COMMAND_PENDING, &ucsi->flags) && timeout--)
> >+ usleep_range(1000, 2000);
>
> Basically a home made mutex, but as the flags is needed anyways I guess this
> works as well.
Well, I'll bring back the separate mutex I had before in any case.
> >+
> >+ if (!timeout)
> >+ return -ETIMEDOUT;
> >+
> >+ ret = ucsi_acpi_cmd(ucsi, ctrl);
> >+ if (ret)
> >+ goto err_clear_flag;
> >+
> >+ ret = wait_for_completion_timeout(&ucsi->complete,
> >+ msecs_to_jiffies(UCSI_TIMEOUT_MS));
> >+ if (!ret) {
> >+ ret = -ETIMEDOUT;
> >+ goto err_clear_flag;
> >+ }
> >+
> >+ switch (ucsi->status) {
> >+ case UCSI_IDLE:
> >+ if (data)
> >+ memcpy(data, ucsi->data->message_in, size);
> >+
> >+ ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> >+ break;
> >+ case UCSI_BUSY:
> >+ /* The caller decides whether to cancel or not */
> >+ ret = -EBUSY;
> >+ goto err_clear_flag;
> >+ case UCSI_ERROR:
> >+ ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> >+ if (ret)
> >+ goto err_clear_flag;
> >+
> >+ ctrl->cmd.cmd = UCSI_GET_ERROR_STATUS;
> >+ ctrl->cmd.length = 0;
> >+ ctrl->cmd.data = 0;
> >+ ret = ucsi_acpi_cmd(ucsi, ctrl);
> >+ if (ret)
> >+ goto err_clear_flag;
> >+
> >+ ret = wait_for_completion_timeout(&ucsi->complete,
> >+ msecs_to_jiffies(UCSI_TIMEOUT_MS));
> >+ if (!ret) {
> >+ ret = -ETIMEDOUT;
> >+ goto err_clear_flag;
> >+ }
> >+
> >+ memcpy(&err_value, ucsi->data->message_in, sizeof(err_value));
> >+
> >+ /* Something has really gone wrong */
> >+ if (WARN_ON(ucsi->status == UCSI_ERROR))
> >+ return -ENODEV;
>
> Is it OK to _not_ clear the COMMAND_PENDING flag before return?
> Are we in such a bad state that it doesn't matter?
The PPM is dead in this case so clearing the flag did not feel
important, but I'll jump to the label in any case.
> >+
> >+ ret = ucsi_ack(ucsi, UCSI_ACK_CMD);
> >+ if (ret)
> >+ goto err_clear_flag;
> >+
> >+ switch (err_value) {
> >+ case UCSI_ERROR_INCOMPATIBLE_PARTNER:
> >+ ret = -EOPNOTSUPP;
> >+ break;
> >+ case UCSI_ERROR_CC_COMMUNICATION_ERR:
> >+ ret = -ECOMM;
> >+ break;
> >+ case UCSI_ERROR_CONTRACT_NEGOTIATION_FAIL:
> >+ ret = -EIO;
> >+ break;
> >+ case UCSI_ERROR_DEAD_BATTERY:
> >+ dev_warn(ucsi->dev, "Dead battery condition!\n");
> >+ ret = -EPERM;
> >+ break;
> >+ /* The following mean a bug in this driver */
> >+ case UCSI_ERROR_INVALID_CON_NUM:
> >+ case UCSI_ERROR_UNREGONIZED_CMD:
> >+ case UCSI_ERROR_INVALID_CMD_ARGUMENT:
> >+ default:
> >+ dev_warn(ucsi->dev,
> >+ "%s: possible UCSI driver bug - error %hu\n",
> >+ __func__, err_value);
> >+ ret = -EINVAL;
> >+ break;
> >+ }
> >+ break;
> >+ }
> >+ ctrl->raw_cmd = 0;
> >+err_clear_flag:
> >+ clear_bit(COMMAND_PENDING, &ucsi->flags);
> >+ return ret;
> >+}
> >+
> >+static void ucsi_connector_change(struct work_struct *work)
> >+{
> >+ struct ucsi_connector *con = container_of(work, struct ucsi_connector,
> >+ work);
> >+ struct ucsi_connector_status constat;
> >+ struct ucsi *ucsi = con->ucsi;
> >+ struct ucsi_control ctrl;
> >+ int ret;
> >+
> >+ ctrl.cmd.cmd = UCSI_GET_CONNECTOR_STATUS;
> >+ ctrl.cmd.length = 0;
> >+ ctrl.cmd.data = con->num;
> >+
> >+ ret = ucsi_run_cmd(con->ucsi, &ctrl, &constat, sizeof(constat));
> >+ if (ret) {
> >+ dev_err(ucsi->dev, "%s: failed to read connector status (%d)\n",
> >+ __func__, ret);
> >+ goto out_ack_event;
> >+ }
> >+
>
> ucsi_connector_change() is scheduled when a connector change event is received,
> EVENT_PENDING bit is set, so we prevent new usci_connector_change() handlers from being
> scheduled. COMMAND_PENDIG is anyway cleared in between (must be torun these commands).
> Is there any risk that commands run before this handler is scheduled, and that they
> mess up reading the connector change done here? or is it OK to do other things in between
> getting a connect change event, and actually reading the connector status in here?
There are no operations that would require multiple commands in
sequence..
> >+ /* Ignoring disconnections and Alternate Modes */
> >+ if (!constat.connected || !(constat.change &
> >+ (UCSI_CONSTAT_PARTNER_CHANGE | UCSI_CONSTAT_CONNECT_CHANGE)) ||
> >+ constat.partner_flags & UCSI_CONSTAT_PARTNER_FLAG_ALT_MODE)
> >+ goto out_ack_event;
> >+
> >+ /* If the partner got USB Host role, attempting swap */
> >+ if (constat.partner_type & UCSI_CONSTAT_PARTNER_TYPE_DFP) {
> >+ ctrl.uor.cmd = UCSI_SET_UOR;
> >+ ctrl.uor.con_num = con->num;
> >+ ctrl.uor.role = UCSI_UOR_ROLE_DFP;
> >+
>
> Same here, it it OK to run some completely different command in between this and the
> previous ucsi_run_cmd()
..but since I'm adding the mutex, I'll use it in this notifier handler
as well.
> >+ ret = ucsi_run_cmd(con->ucsi, &ctrl, NULL, 0);
> >+ if (ret)
> >+ dev_err(ucsi->dev, "%s: failed to swap role (%d)\n",
> >+ __func__, ret);
> >+ }
> >+out_ack_event:
> >+ ucsi_ack(ucsi, UCSI_ACK_EVENT);
> >+ clear_bit(EVENT_PENDING, &ucsi->flags);
> >+}
> >+
> >+static int ucsi_reset_ppm(struct ucsi *ucsi)
> >+{
> >+ struct ucsi_control ctrl;
> >+
> >+ memset(&ctrl, 0, sizeof(ctrl));
> >+ ctrl.cmd.cmd = UCSI_PPM_RESET;
> >+ return ucsi_acpi_cmd(ucsi, &ctrl);
>
> Not waiting for any completion, or polling for a bit here like with other commands?
This was a stupid idea for an optimisation. I'll fix it.
> >+}
> >+
> >+static int ucsi_init(struct ucsi *ucsi)
> >+{
> >+ struct ucsi_connector *con;
> >+ int timeout = UCSI_TIMEOUT_MS;
> >+ struct ucsi_control ctrl;
> >+ int ret;
> >+ int i;
> >+
> >+ init_completion(&ucsi->complete);
> >+ spin_lock_init(&ucsi->dev_lock);
> >+
> >+ /* Reset the PPM */
> >+ ret = ucsi_reset_ppm(ucsi);
> >+ if (ret)
> >+ return ret;
> >+
> >+ /* There is no quarantee the PPM will ever set the RESET_COMPLETE bit */
> >+ while (!(ucsi->data->cci.reset_complete) && timeout--)
> >+ usleep_range(1000, 2000);
> >+
> >+ /*
> >+ * REVISIT: Executing second reset to WA an issue seen on some of the
> >+ * Broxton based platforms, where the first reset puts the PPM into a
> >+ * state where it's unable to recognise some of the commands.
> >+ */
> >+ ret = ucsi_reset_ppm(ucsi);
> >+ if (ret)
> >+ return ret;
>
> Not polling for the cci.reset_complete here after ucsi_reset_ppm()?
> Not needed anymore for the second bugfix reset?
Yep, the recovery from the issue did not require it (for whatever
reason). But this of course may not always be the case on other
platforms.
Thanks for the review,
--
heikki
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
