The variable 'cpu' and 'cpu_sibling' are defined as unsigned int.
However in the for_each_cpu, their values are assigned to -1.
That doesn't make sense and in the cpumask_next() they are implicitly
type conversed to int.
It is universally accepted that the implicit type conversion is
terrible.
Also, having the good programming custom will set an example for
others.
Thus, it might be better to change the definition of 'cpu' and
'cpu_sibling' from unsigned int to int.
Signed-off-by: Jiasheng Jiang <[email protected]>
---
drivers/virt/nitro_enclaves/ne_misc_dev.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
index e21e1e8..2d80879 100644
--- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
+++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
@@ -168,9 +168,9 @@ static bool ne_check_enclaves_created(void)
static int ne_setup_cpu_pool(const char *ne_cpu_list)
{
int core_id = -1;
- unsigned int cpu = 0;
+ int cpu = 0;
cpumask_var_t cpu_pool;
- unsigned int cpu_sibling = 0;
+ int cpu_sibling = 0;
unsigned int i = 0;
int numa_node = -1;
int rc = -EINVAL;
@@ -374,7 +374,7 @@ static int ne_setup_cpu_pool(const char *ne_cpu_list)
*/
static void ne_teardown_cpu_pool(void)
{
- unsigned int cpu = 0;
+ int cpu = 0;
unsigned int i = 0;
int rc = -EINVAL;
@@ -516,7 +516,7 @@ static int ne_get_unused_core_from_cpu_pool(void)
static int ne_set_enclave_threads_per_core(struct ne_enclave *ne_enclave,
int core_id, u32 vcpu_id)
{
- unsigned int cpu = 0;
+ int cpu = 0;
if (core_id < 0 && vcpu_id == 0) {
dev_err_ratelimited(ne_misc_dev.this_device,
@@ -562,7 +562,7 @@ static int ne_set_enclave_threads_per_core(struct ne_enclave *ne_enclave,
static int ne_get_cpu_from_cpu_pool(struct ne_enclave *ne_enclave, u32 *vcpu_id)
{
int core_id = -1;
- unsigned int cpu = 0;
+ int cpu = 0;
unsigned int i = 0;
int rc = -EINVAL;
@@ -1017,7 +1017,7 @@ static int ne_start_enclave_ioctl(struct ne_enclave *ne_enclave,
struct ne_enclave_start_info *enclave_start_info)
{
struct ne_pci_dev_cmd_reply cmd_reply = {};
- unsigned int cpu = 0;
+ int cpu = 0;
struct enclave_start_req enclave_start_req = {};
unsigned int i = 0;
struct pci_dev *pdev = ne_devs.ne_pci_dev->pdev;
@@ -1360,7 +1360,7 @@ static void ne_enclave_remove_all_mem_region_entries(struct ne_enclave *ne_encla
*/
static void ne_enclave_remove_all_vcpu_id_entries(struct ne_enclave *ne_enclave)
{
- unsigned int cpu = 0;
+ int cpu = 0;
unsigned int i = 0;
mutex_lock(&ne_cpu_pool.mutex);
--
2.7.4
On 29/10/2021 04:48, Jiasheng Jiang wrote:
>
> The variable 'cpu' and 'cpu_sibling' are defined as unsigned int.
> However in the for_each_cpu, their values are assigned to -1.
> That doesn't make sense and in the cpumask_next() they are implicitly
> type conversed to int.
> It is universally accepted that the implicit type conversion is
> terrible.
> Also, having the good programming custom will set an example for
> others.
> Thus, it might be better to change the definition of 'cpu' and
> 'cpu_sibling' from unsigned int to int.
>
> Signed-off-by: Jiasheng Jiang <[email protected]>
> ---
> drivers/virt/nitro_enclaves/ne_misc_dev.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/virt/nitro_enclaves/ne_misc_dev.c b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> index e21e1e8..2d80879 100644
> --- a/drivers/virt/nitro_enclaves/ne_misc_dev.c
> +++ b/drivers/virt/nitro_enclaves/ne_misc_dev.c
> @@ -168,9 +168,9 @@ static bool ne_check_enclaves_created(void)
> static int ne_setup_cpu_pool(const char *ne_cpu_list)
> {
> int core_id = -1;
> - unsigned int cpu = 0;
> + int cpu = 0;
> cpumask_var_t cpu_pool;
> - unsigned int cpu_sibling = 0;
> + int cpu_sibling = 0;
> unsigned int i = 0;
> int numa_node = -1;
> int rc = -EINVAL;
> @@ -374,7 +374,7 @@ static int ne_setup_cpu_pool(const char *ne_cpu_list)
> */
> static void ne_teardown_cpu_pool(void)
> {
> - unsigned int cpu = 0;
> + int cpu = 0;
> unsigned int i = 0;
> int rc = -EINVAL;
>
> @@ -516,7 +516,7 @@ static int ne_get_unused_core_from_cpu_pool(void)
> static int ne_set_enclave_threads_per_core(struct ne_enclave *ne_enclave,
> int core_id, u32 vcpu_id)
> {
> - unsigned int cpu = 0;
> + int cpu = 0;
>
> if (core_id < 0 && vcpu_id == 0) {
> dev_err_ratelimited(ne_misc_dev.this_device,
> @@ -562,7 +562,7 @@ static int ne_set_enclave_threads_per_core(struct ne_enclave *ne_enclave,
> static int ne_get_cpu_from_cpu_pool(struct ne_enclave *ne_enclave, u32 *vcpu_id)
> {
> int core_id = -1;
> - unsigned int cpu = 0;
> + int cpu = 0;
> unsigned int i = 0;
> int rc = -EINVAL;
>
> @@ -1017,7 +1017,7 @@ static int ne_start_enclave_ioctl(struct ne_enclave *ne_enclave,
> struct ne_enclave_start_info *enclave_start_info)
> {
> struct ne_pci_dev_cmd_reply cmd_reply = {};
> - unsigned int cpu = 0;
> + int cpu = 0;
> struct enclave_start_req enclave_start_req = {};
> unsigned int i = 0;
> struct pci_dev *pdev = ne_devs.ne_pci_dev->pdev;
> @@ -1360,7 +1360,7 @@ static void ne_enclave_remove_all_mem_region_entries(struct ne_enclave *ne_encla
> */
> static void ne_enclave_remove_all_vcpu_id_entries(struct ne_enclave *ne_enclave)
> {
> - unsigned int cpu = 0;
> + int cpu = 0;
> unsigned int i = 0;
>
> mutex_lock(&ne_cpu_pool.mutex);
> --
> 2.7.4
>
Reviewed-by: Andra Paraschiv <[email protected]>
As a follow-up, in general, when sending new versions of the patch(es)
please also include the changelog after the "---" line. For example:
---
Changelog
v1 -> v2
* Change 1.
* Change 2.
...........
That helps with patch(es) changes tracking.
Thanks,
Andra
Amazon Development Center (Romania) S.R.L. registered office: 27A Sf. Lazar Street, UBC5, floor 2, Iasi, Iasi County, 700045, Romania. Registered in Romania. Registration number J22/2621/2005.
On Fri, Oct 29, 2021 at 01:48:46AM +0000, Jiasheng Jiang wrote:
>The variable 'cpu' and 'cpu_sibling' are defined as unsigned int.
>However in the for_each_cpu, their values are assigned to -1.
>That doesn't make sense and in the cpumask_next() they are implicitly
>type conversed to int.
>It is universally accepted that the implicit type conversion is
>terrible.
>Also, having the good programming custom will set an example for
>others.
>Thus, it might be better to change the definition of 'cpu' and
>'cpu_sibling' from unsigned int to int.
>
>Signed-off-by: Jiasheng Jiang <[email protected]>
>---
> drivers/virt/nitro_enclaves/ne_misc_dev.c | 14 +++++++-------
> 1 file changed, 7 insertions(+), 7 deletions(-)
The for_each_cpu documentation explicitly says that unsigned is
supported and -1 is commonly used to refer to the value before 0 even
for unsigned.
However I have nothing against changing them to int.
Reviewed-by: Stefano Garzarella <[email protected]>
Thanks,
Stefano