Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

driver: crypto: hisilicon: fix error handling #6648

Merged
merged 1 commit into from
Feb 19, 2024

Conversation

yuzexiyzx
Copy link
Contributor

  1. support num = 0 in qm_set_vt_common().
  2. release qp when qm_xqc_cfg() failed.
  3. add check of msg and qp->num in hisi_qp_recv_sync().

@jenswi-linaro
Copy link
Contributor

There's a typo in the title of the commit, I guess it should be "exception".

Please explain in plain text in the commit message why you're doing this. Most of what you're doing can be seen in the diff, but sometimes it's nice with a summary of that too.

@yuzexiyzx yuzexiyzx changed the title driver: crypto: hisilicon: fix excepton handling driver: crypto: hisilicon: fix exception handling Jan 31, 2024
@yuzexiyzx
Copy link
Contributor Author

@jenswi-linaro comment solved.
changed to

  1. support num = 0 in qm_set_vft_common().
    When qm_set_xqc_set() failed, it will set q_num to zero.
    qm_set_vft_common() need to support this scene.
  2. release qp when qm_xqc_cfg() failed.
  3. add check of msg and qp->num in hisi_qp_recv_sync().
    hisi_qp_recv_sync() is not static function, we need to
    check every input parameter to ensure that it is safe.

@jenswi-linaro
Copy link
Contributor

I'm sorry I think you missed my point. You're in principle describing the diff in detail. In a good commit message you describe why a change is being made, not just what's changing.

If you need to make a list of all things done in a patch it's usually a sign that it should have been multiple patches, but in this case, it seems that it all belongs in one patch.

How about something like:

driver: crypto: hisilicon: fix error handling

When the XXX fails to configure, qm_set_xqc_vft() is called with the num argument
as zero to disable the device (or whatever happens). Update qm_set_xqc_vft() to
handle this error path.

}

qm->qp_in_used++;
mutex_unlock(&qm->qp_lock);
return qp;

qp_release:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since it's an error path, prefer have err_ prefix in the branch label.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

solved

@yuzexiyzx
Copy link
Contributor Author

@jenswi-linaro @etienne-lms All comments have been solved.

@etienne-lms
Copy link
Contributor

Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
It think the IBART test failure is unrelated to this change.

@jenswi-linaro
Copy link
Contributor

The title of the commit says:

driver: crypto: hisilicon: fix exception handling

I think the word "exception" might be misleading since we're not dealing with what we normally call exceptions in this patch. How about using the word "error" instead?

@jenswi-linaro
Copy link
Contributor

With my comment addressed, please apply:
Acked-by: Jens Wiklander <jens.wiklander@linaro.org>

When qm_set_vft_common() fails to configure, qm_set_xqc_vft() is
called with the num argument as zero to disable the device. Update
qm_set_xqc_vft() to handle this error path.

Signed-off-by: Zexi Yu <yuzexi@hisilicon.com>
Acked-by: Jens wiklander <jens.wiklander@linaro.org>
Acked-by: Etienne Carriere <etienne.carriere@foss.st.com>
@yuzexiyzx yuzexiyzx changed the title driver: crypto: hisilicon: fix exception handling driver: crypto: hisilicon: fix error handling Feb 17, 2024
@yuzexiyzx
Copy link
Contributor Author

@jforissier All comments have been solved.

@jforissier
Copy link
Contributor

The IBART error can be ignored (rebase needed). @yuzexiyzx thanks!

@jforissier jforissier merged commit 09c44b0 into OP-TEE:master Feb 19, 2024
6 of 7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants