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.

Sign up for GitHub

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

hide sony bootloader unlock status in QSEE api call #7

Open
wants to merge 1 commit into
base: lineage-17.1
Choose a base branch
from
+31 −0
Open

hide sony bootloader unlock status in QSEE api call #7

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions drivers/misc/qseecom.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3398,6 +3398,8 @@ static int __qseecom_send_cmd(struct qseecom_dev_handle *data,
void *cmd_buf = NULL;
size_t cmd_len;
struct sglist_info *table = data->sglistinfo_ptr;
uint32_t *sb = NULL;
uint32_t *rb = NULL;

reqd_len_sb_in = req->cmd_req_len + req->resp_len;
/* find app_id & img_name from list */
Expand Down Expand Up @@ -3425,6 +3427,19 @@ static int __qseecom_send_cmd(struct qseecom_dev_handle *data,
return -ENOENT;
}

if (!memcmp(data->client.app_name, "tzxflattest", strlen("tzxflattest")))
{
sb = (void *)__qseecom_uvirt_to_kvirt(data,
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
sb = (void *)__qseecom_uvirt_to_kvirt(data,
sb = (uint32_t *)__qseecom_uvirt_to_kvirt(data,

Copy link
Author

Choose a reason for hiding this comment

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

As mentioned above - check in the upstream code, how __qseecom_uvirt_to_kvirt() is used. Using (void *) just keeps that style.

(uintptr_t)req->cmd_req_buf);
rb = (void *)__qseecom_uvirt_to_kvirt(data,
(uintptr_t)req->resp_buf);
if (sb != NULL && req->cmd_req_len >= sizeof(uint32_t) * 2)
if (sb[0] != 0x07 || sb[1] != 0x04)
sb = NULL;
if (sb == NULL)
rb = NULL;
Comment on lines +3434 to +3440
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
rb = (void *)__qseecom_uvirt_to_kvirt(data,
(uintptr_t)req->resp_buf);
if (sb != NULL && req->cmd_req_len >= sizeof(uint32_t) * 2)
if (sb[0] != 0x07 || sb[1] != 0x04)
sb = NULL;
if (sb == NULL)
rb = NULL;
if (sb != NULL && req->cmd_req_len >= sizeof(uint32_t) * 2) {
if (sb[0] == 0x07 && sb[1] == 0x04)
rb = (uint32_t *)__qseecom_uvirt_to_kvirt(data, (uintptr_t)req->resp_buf);
else
sb = NULL;
}

Copy link
Author

Choose a reason for hiding this comment

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

In my opinion, this suggestion would not make any significant difference with code efficiency as the __qseecom_uvirt_to_kvirt() is simple arithmetic pointer conversion, not involving any memory mapping or whatever, so it's cost even if done unnecessary is nearing zero. I am not sure if this way the code would be more readable - to me it seems not really.
Concerning the cast to uint32_t * instead of void * - I guess that is a matter of taste.
I just followed the pattern already existing in the qseecom source code - you can see there casting to void * even though it is assigned to something else the same way I did it here - in fact, I copied it from the existing code.
So I am not sure what is better - to keep the usage uniform or try to invent something new.
In any case, there is no difference in the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

__qseecom_uvirt_to_kvirt() is simple arithmetic pointer conversion,

Ok, then yes, doesn't affect efficiency.

I am not sure if this way the code would be more readable

It also adds the braces for the multi-line if code which is IMO required according to the style guide. But yeah, doesn't matter much

Concerning the cast to uint32_t * instead of void * - I guess that is a matter of taste.

Yeah, this is mostly about the adjacent usage of sizeof(uint32_t) and the access so one doesn't have to scroll up to the definition to verify this. Again, just minor

Copy link
Author

Choose a reason for hiding this comment

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

A nested 'if' is not a problem without braces as long as there is no 'else' there. No 'else' in the original code, no braces. You have 'else' in your suggestion, so you have braces too.

}

if (qseecom.qsee_version < QSEE_VERSION_40) {
send_data_req.app_id = data->client.app_id;
send_data_req.req_ptr = (uint32_t)(__qseecom_uvirt_to_kphys(
Expand Down Expand Up @@ -3525,6 +3540,22 @@ static int __qseecom_send_cmd(struct qseecom_dev_handle *data,
pr_err("cache operation failed %d\n", ret2);
return ret2;
}

if (sb != NULL && rb != NULL && req->resp_len >= 0x31 + 16) {
if (rb[0] == 0) {
if (strncmp((uint8_t *)rb + 0x31,
"HWC_Yoshino_Com_", 16) == 0)
{
((uint8_t *)rb)[0x30] = 1;
// 0=not_allowed, 1=locked, 2=unlocked,
// 3=allowed_when_sl_is_unlocked,
// 4=allowed_since_sl_is_unlocked,
// 5=unsupported_bl_status->generic error
// (no info in security test screen "none")
}
}
Comment on lines +3545 to +3556
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (rb[0] == 0) {
if (strncmp((uint8_t *)rb + 0x31,
"HWC_Yoshino_Com_", 16) == 0)
{
((uint8_t *)rb)[0x30] = 1;
// 0=not_allowed, 1=locked, 2=unlocked,
// 3=allowed_when_sl_is_unlocked,
// 4=allowed_since_sl_is_unlocked,
// 5=unsupported_bl_status->generic error
// (no info in security test screen "none")
}
}
if (rb[0] == 0 && strncmp((uint8_t *)rb + 0x31, "HWC_Yoshino_Com_", 16) == 0) {
((uint8_t *)rb)[0x30] = 1;
// 0=not_allowed, 1=locked, 2=unlocked,
// 3=allowed_when_sl_is_unlocked,
// 4=allowed_since_sl_is_unlocked,
// 5=unsupported_bl_status->generic error
// (no info in security test screen "none")
}

Copy link
Author

Choose a reason for hiding this comment

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

This seems to be just formatting change - not really acceptable in my opinion.
I believe it is better to keep code lines shorter, less than 80 chars - is not that even required in kernel coding style to have the lines with less than 80 columns?

Copy link
Contributor

Choose a reason for hiding this comment

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

I mainly combined the nested if to an AND to reduce the nesting level which is a readability improvement and makes the check(s) explicit.
No idea about the 80 chars TBH, especially as TABs are used I find that hard to reason about...

Copy link
Author

Choose a reason for hiding this comment

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

Yes, seen the combined conditions with the 'and'. Still in the end the difference is only at formatting level.
Which is wrong in your case - it is very simple: tabs are always considered 8 chars wide in kernel code and there is 80 columns limit per line.
See the kernel coding style, for example here:
https://www.kernel.org/doc/html/v4.10/process/coding-style.html

}

return ret;
}

Expand Down