diff mbox series

[V4,net-next,3/9] net: ena: add explicit casting to variables

Message ID 1607083875-32134-4-git-send-email-akiyano@amazon.com (mailing list archive)
State Changes Requested
Delegated to: Netdev Maintainers
Headers show
Series XDP Redirect implementation for ENA driver | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count success Link
netdev/tree_selection success Clearly marked for net-next
netdev/subject_prefix success Link
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch success total: 0 errors, 0 warnings, 0 checks, 40 lines checked
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link
netdev/stable success Stable not CCed

Commit Message

Kiyanovski, Arthur Dec. 4, 2020, 12:11 p.m. UTC 
From: Arthur Kiyanovski <akiyano@amazon.com>

This patch adds explicit casting to some implicit conversions in the ena
driver. The implicit conversions fail some of our static checkers that
search for accidental conversions in our driver.
Adding this cast won't affect the end results, and would sooth the
checkers.

Signed-off-by: Ido Segev <idose@amazon.com>
Signed-off-by: Igor Chauskin <igorch@amazon.com>
Signed-off-by: Shay Agroskin <shayagr@amazon.com>
Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
---
 drivers/net/ethernet/amazon/ena/ena_com.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Alexander Duyck Dec. 7, 2020, 7 p.m. UTC | #1 
On Fri, Dec 4, 2020 at 4:15 AM <akiyano@amazon.com> wrote:
>
> From: Arthur Kiyanovski <akiyano@amazon.com>
>
> This patch adds explicit casting to some implicit conversions in the ena
> driver. The implicit conversions fail some of our static checkers that
> search for accidental conversions in our driver.
> Adding this cast won't affect the end results, and would sooth the
> checkers.
>
> Signed-off-by: Ido Segev <idose@amazon.com>
> Signed-off-by: Igor Chauskin <igorch@amazon.com>
> Signed-off-by: Shay Agroskin <shayagr@amazon.com>
> Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
> ---
>  drivers/net/ethernet/amazon/ena/ena_com.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
> index e168edf3c930..7910d8e68a99 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_com.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
> @@ -1369,7 +1369,7 @@ int ena_com_execute_admin_command(struct ena_com_admin_queue *admin_queue,
>                                    "Failed to submit command [%ld]\n",
>                                    PTR_ERR(comp_ctx));
>
> -               return PTR_ERR(comp_ctx);
> +               return (int)PTR_ERR(comp_ctx);
>         }
>
>         ret = ena_com_wait_and_process_admin_cq(comp_ctx, admin_queue);

I'm not a big fan of resolving it this way as we are going to end up
with the pattern throughout the kernel if this is really needed. It
might make more sense to either come up with a new define that returns
int instead of long, or to tweak the existing PTR_ERR define so that
it returns an int instead of a long.

An alternative here would be to just pass PTR_ERR into ret and then
process it that way within this if block. As it stands the comparison
to ERR_PTR(-ENODEV) doesn't read very well anyway.

> @@ -1595,7 +1595,7 @@ int ena_com_set_aenq_config(struct ena_com_dev *ena_dev, u32 groups_flag)
>  int ena_com_get_dma_width(struct ena_com_dev *ena_dev)
>  {
>         u32 caps = ena_com_reg_bar_read32(ena_dev, ENA_REGS_CAPS_OFF);
> -       int width;
> +       u32 width;
>
>         if (unlikely(caps == ENA_MMIO_READ_TIMEOUT)) {
>                 netdev_err(ena_dev->net_device, "Reg read timeout occurred\n");
> @@ -2266,7 +2266,7 @@ int ena_com_set_dev_mtu(struct ena_com_dev *ena_dev, int mtu)
>         cmd.aq_common_descriptor.opcode = ENA_ADMIN_SET_FEATURE;
>         cmd.aq_common_descriptor.flags = 0;
>         cmd.feat_common.feature_id = ENA_ADMIN_MTU;
> -       cmd.u.mtu.mtu = mtu;
> +       cmd.u.mtu.mtu = (u32)mtu;
>
>         ret = ena_com_execute_admin_command(admin_queue,
>                                             (struct ena_admin_aq_entry *)&cmd,

Wouldn't it make more sense to define mtu as a u32 in the first place
and address this in the function that calls this rather than doing the
cast at the last minute?

> @@ -2689,7 +2689,7 @@ int ena_com_indirect_table_set(struct ena_com_dev *ena_dev)
>                 return ret;
>         }
>
> -       cmd.control_buffer.length = (1ULL << rss->tbl_log_size) *
> +       cmd.control_buffer.length = (u32)(1ULL << rss->tbl_log_size) *
>                 sizeof(struct ena_admin_rss_ind_table_entry);
>
>         ret = ena_com_execute_admin_command(admin_queue,
> @@ -2712,7 +2712,7 @@ int ena_com_indirect_table_get(struct ena_com_dev *ena_dev, u32 *ind_tbl)
>         u32 tbl_size;
>         int i, rc;
>
> -       tbl_size = (1ULL << rss->tbl_log_size) *
> +       tbl_size = (u32)(1ULL << rss->tbl_log_size) *
>                 sizeof(struct ena_admin_rss_ind_table_entry);
>
>         rc = ena_com_get_feature_ex(ena_dev, &get_resp,

For these last two why not make it 1u instead of 1ull for the bit
being shifted? At least that way you are not implying possible
truncation in the conversion.
Shay Agroskin Dec. 7, 2020, 8:19 p.m. UTC | #2 
Alexander Duyck <alexander.duyck@gmail.com> writes:

> On Fri, Dec 4, 2020 at 4:15 AM <akiyano@amazon.com> wrote:
>>
>> From: Arthur Kiyanovski <akiyano@amazon.com>
>>
>> This patch adds explicit casting to some implicit conversions 
>> in the ena
>> driver. The implicit conversions fail some of our static 
>> checkers that
>> search for accidental conversions in our driver.
>> Adding this cast won't affect the end results, and would sooth 
>> the
>> checkers.
>>
>> Signed-off-by: Ido Segev <idose@amazon.com>
>> Signed-off-by: Igor Chauskin <igorch@amazon.com>
>> Signed-off-by: Shay Agroskin <shayagr@amazon.com>
>> Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
>> ---
>>  drivers/net/ethernet/amazon/ena/ena_com.c | 10 +++++-----
>>  1 file changed, 5 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c 
>> b/drivers/net/ethernet/amazon/ena/ena_com.c
>> index e168edf3c930..7910d8e68a99 100644
>> --- a/drivers/net/ethernet/amazon/ena/ena_com.c
>> +++ b/drivers/net/ethernet/amazon/ena/ena_com.c
>> @@ -1369,7 +1369,7 @@ int ena_com_execute_admin_command(struct 
>> ena_com_admin_queue *admin_queue,
>>                                    "Failed to submit command 
>>                                    [%ld]\n",
>>                                    PTR_ERR(comp_ctx));
>>
>> -               return PTR_ERR(comp_ctx);
>> +               return (int)PTR_ERR(comp_ctx);
>>         }
>>
>>         ret = ena_com_wait_and_process_admin_cq(comp_ctx, 
>>         admin_queue);
>
> I'm not a big fan of resolving it this way as we are going to 
> end up
> with the pattern throughout the kernel if this is really 
> needed. It
> might make more sense to either come up with a new define that 
> returns
> int instead of long, or to tweak the existing PTR_ERR define so 
> that
> it returns an int instead of a long.
>
> An alternative here would be to just pass PTR_ERR into ret and 
> then
> process it that way within this if block. As it stands the 
> comparison
> to ERR_PTR(-ENODEV) doesn't read very well anyway.
>

Hi, thanks for reviewing the code. Looking at it I agree it looks 
rather ugly. I'll try to rework it to something less hideous.

Regarding the idea to make it more generic (kernel wide change), 
our static checkers test the ena_com code on various platforms and 
the 'implicit cast' warning might not repeat for other drivers

>> @@ -1595,7 +1595,7 @@ int ena_com_set_aenq_config(struct 
>> ena_com_dev *ena_dev, u32 groups_flag)
>>  int ena_com_get_dma_width(struct ena_com_dev *ena_dev)
>>  {
>>         u32 caps = ena_com_reg_bar_read32(ena_dev, 
>>         ENA_REGS_CAPS_OFF);
>> -       int width;
>> +       u32 width;
>>
>>         if (unlikely(caps == ENA_MMIO_READ_TIMEOUT)) {
>>                 netdev_err(ena_dev->net_device, "Reg read 
>>                 timeout occurred\n");
>> @@ -2266,7 +2266,7 @@ int ena_com_set_dev_mtu(struct 
>> ena_com_dev *ena_dev, int mtu)
>>         cmd.aq_common_descriptor.opcode = 
>>         ENA_ADMIN_SET_FEATURE;
>>         cmd.aq_common_descriptor.flags = 0;
>>         cmd.feat_common.feature_id = ENA_ADMIN_MTU;
>> -       cmd.u.mtu.mtu = mtu;
>> +       cmd.u.mtu.mtu = (u32)mtu;
>>
>>         ret = ena_com_execute_admin_command(admin_queue,
>>                                             (struct 
>>                                             ena_admin_aq_entry 
>>                                             *)&cmd,
>
> Wouldn't it make more sense to define mtu as a u32 in the first 
> place
> and address this in the function that calls this rather than 
> doing the
> cast at the last minute?
>

It would make the code prettier at the very least. I'll try to 
tweak this a little

>> @@ -2689,7 +2689,7 @@ int ena_com_indirect_table_set(struct 
>> ena_com_dev *ena_dev)
>>                 return ret;
>>         }
>>
>> -       cmd.control_buffer.length = (1ULL << rss->tbl_log_size) 
>> *
>> +       cmd.control_buffer.length = (u32)(1ULL << 
>> rss->tbl_log_size) *
>>                 sizeof(struct ena_admin_rss_ind_table_entry);
>>
>>         ret = ena_com_execute_admin_command(admin_queue,
>> @@ -2712,7 +2712,7 @@ int ena_com_indirect_table_get(struct 
>> ena_com_dev *ena_dev, u32 *ind_tbl)
>>         u32 tbl_size;
>>         int i, rc;
>>
>> -       tbl_size = (1ULL << rss->tbl_log_size) *
>> +       tbl_size = (u32)(1ULL << rss->tbl_log_size) *
>>                 sizeof(struct ena_admin_rss_ind_table_entry);
>>
>>         rc = ena_com_get_feature_ex(ena_dev, &get_resp,
>
> For these last two why not make it 1u instead of 1ull for the 
> bit
> being shifted? At least that way you are not implying possible
> truncation in the conversion.

This sounds correct, I'll give it a try. Thanks
Shay Agroskin Dec. 8, 2020, 6:11 p.m. UTC | #3 
Alexander Duyck <alexander.duyck@gmail.com> writes:

> On Fri, Dec 4, 2020 at 4:15 AM <akiyano@amazon.com> wrote:
>>
>> From: Arthur Kiyanovski <akiyano@amazon.com>
>>
>> This patch adds explicit casting to some implicit conversions 
>> in the ena
>> driver. The implicit conversions fail some of our static 
>> checkers that
>> search for accidental conversions in our driver.
>> Adding this cast won't affect the end results, and would sooth 
>> the
>> checkers.
>>
>> Signed-off-by: Ido Segev <idose@amazon.com>
>> Signed-off-by: Igor Chauskin <igorch@amazon.com>
>> Signed-off-by: Shay Agroskin <shayagr@amazon.com>
>> Signed-off-by: Arthur Kiyanovski <akiyano@amazon.com>
>> ---
>> ...
>> @@ -2712,7 +2712,7 @@ int ena_com_indirect_table_get(struct 
>> ena_com_dev *ena_dev, u32 *ind_tbl)
>>         u32 tbl_size;
>>         int i, rc;
>>
>> -       tbl_size = (1ULL << rss->tbl_log_size) *
>> +       tbl_size = (u32)(1ULL << rss->tbl_log_size) *
>>                 sizeof(struct ena_admin_rss_ind_table_entry);
>>
>>         rc = ena_com_get_feature_ex(ena_dev, &get_resp,
>
> For these last two why not make it 1u instead of 1ull for the 
> bit
> being shifted? At least that way you are not implying possible
> truncation in the conversion.

We decided to remove this conversion from the patch altogether. We 
might do something different in the future to achieve the same 
result. Thanks for your comment
diff mbox series

Patch

diff --git a/drivers/net/ethernet/amazon/ena/ena_com.c b/drivers/net/ethernet/amazon/ena/ena_com.c
index e168edf3c930..7910d8e68a99 100644
--- a/drivers/net/ethernet/amazon/ena/ena_com.c
+++ b/drivers/net/ethernet/amazon/ena/ena_com.c
@@ -1369,7 +1369,7 @@  int ena_com_execute_admin_command(struct ena_com_admin_queue *admin_queue,
 				   "Failed to submit command [%ld]\n",
 				   PTR_ERR(comp_ctx));
 
-		return PTR_ERR(comp_ctx);
+		return (int)PTR_ERR(comp_ctx);
 	}
 
 	ret = ena_com_wait_and_process_admin_cq(comp_ctx, admin_queue);
@@ -1595,7 +1595,7 @@  int ena_com_set_aenq_config(struct ena_com_dev *ena_dev, u32 groups_flag)
 int ena_com_get_dma_width(struct ena_com_dev *ena_dev)
 {
 	u32 caps = ena_com_reg_bar_read32(ena_dev, ENA_REGS_CAPS_OFF);
-	int width;
+	u32 width;
 
 	if (unlikely(caps == ENA_MMIO_READ_TIMEOUT)) {
 		netdev_err(ena_dev->net_device, "Reg read timeout occurred\n");
@@ -2266,7 +2266,7 @@  int ena_com_set_dev_mtu(struct ena_com_dev *ena_dev, int mtu)
 	cmd.aq_common_descriptor.opcode = ENA_ADMIN_SET_FEATURE;
 	cmd.aq_common_descriptor.flags = 0;
 	cmd.feat_common.feature_id = ENA_ADMIN_MTU;
-	cmd.u.mtu.mtu = mtu;
+	cmd.u.mtu.mtu = (u32)mtu;
 
 	ret = ena_com_execute_admin_command(admin_queue,
 					    (struct ena_admin_aq_entry *)&cmd,
@@ -2689,7 +2689,7 @@  int ena_com_indirect_table_set(struct ena_com_dev *ena_dev)
 		return ret;
 	}
 
-	cmd.control_buffer.length = (1ULL << rss->tbl_log_size) *
+	cmd.control_buffer.length = (u32)(1ULL << rss->tbl_log_size) *
 		sizeof(struct ena_admin_rss_ind_table_entry);
 
 	ret = ena_com_execute_admin_command(admin_queue,
@@ -2712,7 +2712,7 @@  int ena_com_indirect_table_get(struct ena_com_dev *ena_dev, u32 *ind_tbl)
 	u32 tbl_size;
 	int i, rc;
 
-	tbl_size = (1ULL << rss->tbl_log_size) *
+	tbl_size = (u32)(1ULL << rss->tbl_log_size) *
 		sizeof(struct ena_admin_rss_ind_table_entry);
 
 	rc = ena_com_get_feature_ex(ena_dev, &get_resp,