Clear CCC and transfer status in Cadence driver - #116261
Conversation
|
Hello @mborows2, and thank you very much for your first Pull Request (PR) to the Zephyr Project! All PRs must pass our Continuous Integration (CI) pipeline before merging. When the pipeline run for your PR completes, you are expected to investigate the results, fix any errors, and update your PR for a fresh round of review. Since this is your first contribution, a project community member must manually approve your CI run (this helps us avoid abuse of our CI system). A bot should assign some reviewers who can start the run for you soon. As a heads-up, you will probably have to update your PR to fix CI issues and address review feedback in order to get it ready for merge. Some key rules for updating your PR are:
Also, see:
If you are stuck or need help, you can join us on Discord and ask questions; many community members try to help new contributors there 😊. Try to pick a Discord channel that is associated with the technical details of your request. If you're not sure, use the #general channel. |
|
You have been identified as a likely reviewer for the code this pull request changes, but could not be added to its review request automatically. Please review it if you are able to. |
There was a problem hiding this comment.
Pull request overview
This PR aims to make the Cadence I3C controller driver proactively clear driver-owned transfer status fields (num_xfer, err) for both CCC transactions and private transfers, preventing callers from consuming stale/uninitialized residue when commands are dropped without producing CMDR completion entries.
Changes:
- Add a CCC helper to reset
i3c_ccc_payloadstatus fields before issuing a CCC. - Add a private-transfer helper to reset
i3c_msgstatus fields before issuing I3C transfers. - Invoke these reset helpers in both synchronous and callback-based transfer entry points.
Suppressed comments (3)
drivers/i3c/i3c_cdns.c:1793
- Typo in this comment block ("unitialized"), plus a couple phrasing issues; please correct the text and remove trailing whitespace on the blank line.
*
* The CCC helpers in i3c_ccc.c declare their payloads as unitialized stack
* locals and read @c num_xfer back as length, so that residue gets consumed
* as byte count. Clear the fields up front so an incomplete CCC reports zero
* bytes transferred rather than stack garbage.
drivers/i3c/i3c_cdns.c:2873
- Typos/grammar in this comment block (e.g., "contract a their", "pior", "00 the document") reduce clarity; please correct the wording.
* @c num_xfer and @c err carry the same driver-writes-this contract a their
* CCC counterparts (see @ref i3c_msg) and are populated from the same CMDR
* completion handler, so a message whose command never retires a CMDR entry
* leaves both holding the caller's pior contents. Callers that size a
* subsequent access on @c num_xfer 00 the document way to learn how many
drivers/i3c/i3c_cdns.c:2928
- This call is indented with spaces; the rest of the file uses tabs for indentation.
cdns_i3c_msgs_reset_status(msgs, num_msgs);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| * @c num_xfer and @c err are outputs the controller driver is required to write | ||
| * (see @ref i3c_ccc_payload). This driver only writes them from the CMDR | ||
| * completion handler, which runs once per command response actually retired by | ||
| * the IP. A command that is dropped without producing a CMDR entry therefore | ||
| * never update them, while the transaction as a whole can still comple | ||
| * successfully -- leaving whatevevr the caller happened to have in the payload. |
| static void cdns_i3c_msgs_reset_status(struct i3c)msg *msg, uint8_t num_msgs) | ||
| { | ||
| for (uint8_t i = 0; i < num_msgs; i++) { | ||
| msgs[i].num_xfer = 0; | ||
| msgs[i].err = I3C_ERROR_CE_NONE; | ||
| } | ||
| } |
| static int cdns_i3c_transfer(const struct device *dev, struct i3c_device_desc *target, | ||
| struct i3c_msg *msgs, uint8_t num_msgs) | ||
| { | ||
| cdns_i3c_msgs_reset_status(msgs, num_msgs); |
This comment was marked as outdated.
This comment was marked as outdated.
ec983d1 to
e5cdf34
Compare
|
num_xfer is an output field the controller driver is contractually required to populate - the struct i3c_ccc_payload documentation states "It is expected for the driver to write to this after the transfer". The Cadence RTIO driver does write it, but only from cdns_i3c_rtio_irq_cmdd_emp(), which iterates the command responses the IP actually retired. A command that is dropped without producing a CMDR entry never updates num_xfer, and because sticky is only set from responses that were seen, the transaction can still complete successfully. The caller then reads a field the driver never wrote. Added two helpers to i3c_cdns.c and call them before the transfer is submitted. Signed-off-by: Marek Borowski <marek@borowski.com>
|
Hi @mborows2! Congratulations on getting your very first Zephyr pull request merged 🎉🥳. This is a fantastic achievement, and we're thrilled to have you as part of our community! Now that your first PR is merged, CI will run automatically on subsequent PRs that you send from the same GitHub account. To celebrate this milestone and showcase your contribution, we'd love to award you the Zephyr Technical Contributor badge. If you're interested, please claim your badge by filling out this form: Claim Your Zephyr Badge. Thank you for your valuable input, and we look forward to seeing more of your contributions in the future! 🪁 |
Clear CCC and transfer status in Cadence driver - #116261