[linux-6.6.y] LoongArch: Add LoongArch security engine driver support #621
Conversation
Signed-off-by: Hongchen Zhang <[email protected]> Signed-off-by: Binbin Zhou <[email protected]>
Signed-off-by: Zhao Qunqin <[email protected]> Signed-off-by: Binbin Zhou <[email protected]>
Reviewer's Guide by SourceryThis pull request adds support for the LoongArch security engine driver by introducing new driver files, updating the Makefile and Kconfig, and adding device tree bindings. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Hi @AaronDot. Thanks for your PR. I'm waiting for a deepin-community member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @AaronDot - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a comment block at the beginning of
drivers/char/loongson_se.cto describe the purpose of the driver. - It might be helpful to add a comment explaining the purpose of the
lsse_sdf_cdev.cdriver.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 2 issues found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| spin_unlock_irqrestore(&se->dev_lock, flag); | ||
| } | ||
|
|
||
| static int se_send_requeset(struct loongson_se *se, |
There was a problem hiding this comment.
suggestion (typo): Typo in function name 'se_send_requeset'.
Consider renaming the function to 'se_send_request' (and updating all its call sites) to avoid confusion.
Suggested implementation:
static int se_send_request(struct loongson_se *se,Make sure to search for all occurrences of "se_send_requeset" in the file and/or project and update them to "se_send_request" to ensure consistency and avoid linker errors.
| int msg_first, msg_nr; | ||
|
|
||
| if (!se) { | ||
| pr_err("SE has bot been initialized\n"); |
There was a problem hiding this comment.
nitpick (typo): Typo in error message string.
Replace 'bot' with 'not' for clarity.
| pr_err("SE has bot been initialized\n"); | |
| pr_err("SE has not been initialized\n"); |
|
|
||
| se->dev = &pdev->dev; | ||
| platform_set_drvdata(pdev, se); | ||
| dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); |
There was a problem hiding this comment.
suggestion (bug_risk): Ignoring return value of dma_set_mask_and_coherent.
Consider checking the return value to handle potential failures, ensuring the DMA configuration succeeds.
| dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); | |
| err = dma_set_mask_and_coherent(&pdev->dev, DMA_BIT_MASK(64)); | |
| if (err) | |
| return err; |
| return err; | ||
| } | ||
|
|
||
| static int se_send_genl_cmd(struct loongson_se *se, struct se_mailbox_data *req, |
There was a problem hiding this comment.
issue (complexity): Consider refactoring the retry logic in se_send_genl_cmd to use a for-loop instead of goto statements.
Consider refactoring the retry logic in se_send_genl_cmd into a simple loop. For example, you could replace the goto-based retries with a for-loop as shown below:
static int se_send_genl_cmd(struct loongson_se *se, struct se_mailbox_data *req,
struct se_mailbox_data *res, int retry)
{
int err = 0, cnt;
for (cnt = 0; cnt < retry; cnt++) {
dev_dbg(se->dev, "%d time send cmd 0x%x\n", cnt + 1, req->u.gcmd.cmd);
err = se_send_requeset(se, req);
if (err)
continue;
if (!wait_for_completion_timeout(&se->cmd_completion,
msecs_to_jiffies(0x1000))) {
se_enable_int(se, req->int_bit);
continue;
}
err = loongson_se_get_res(se, req->int_bit, req->u.gcmd.cmd, res);
if (err || res->u.res.cmd_ret) {
se_enable_int(se, req->int_bit);
continue;
}
break;
}
if (cnt >= retry)
err = -ETIMEDOUT;
se_enable_int(se, req->int_bit);
return err;
}This approach reduces nesting and improves readability while keeping the functionality intact.
| wake_up(&se->wq); | ||
| } | ||
|
|
||
| static int se_send_sdf_cmd(struct lsse_sdf_dev *se, int len, int retry) |
There was a problem hiding this comment.
issue (complexity): Consider abstracting repeated logic into helper functions to simplify the code flow.
Consider abstracting repeated error–cleanup and user/kernel copy logic into helper routines to simplify the intertwined flow. For example, you can encapsulate the copy logic into an inline function rather than duplicating copy/ memcpy decisions:
static inline int copy_data(void *dest, const void *src, size_t count, bool is_user)
{
if (is_user)
return copy_from_user(dest, src, count) ? -EFAULT : 0;
memcpy(dest, src, count);
return 0;
}Then in your send/recv functions, replace:
if (user) {
ret = copy_from_user(se->se_ch->data_buffer, buf, count);
if (ret) {
ret = -EFAULT;
goto out_unlock;
}
/* ... */
} else
memcpy(se->se_ch->data_buffer, buf, count);with:
ret = copy_data(se->se_ch->data_buffer, buf, count, user);
if (ret)
goto out_unlock;Similarly, consolidate error paths by abstracting common cleanup work into a single helper function rather than interleaving goto labels. For example, an inline unlock-and-return function:
static inline int unlock_and_return(struct mutex *lock, int ret)
{
mutex_unlock(lock);
return ret;
}Then replace patterns like:
if (ret) {
mutex_unlock(&se->data_lock);
return ret;
}with:
if (ret)
return unlock_and_return(&se->data_lock, ret);These small focused changes reduce nesting and interleaved goto statements while keeping all functionality intact.
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: sourcery-ai[bot] The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Add LoongArch security engine driver support
Summary by Sourcery
Add support for the LoongArch security engine by introducing a new driver and updating the device tree bindings to include the Loongson SE module. This enables cryptographic operations and secure communication channels on LoongArch platforms.
New Features:
Enhancements:
Documentation: