[PATCH] RDMA/efa: Fix node guid compiler warning

[PATCH] RDMA/efa: Fix node guid compiler warning

[Michael Margolin]

The GUID is received in big-endian so align types accordingly to avoid
compiler warnings.

Closes: https://lore.kernel.org/oe-kbuild-all/202409032113.bvyVfsNp-lkp@intel.com/

Fixes: 04e36fd27a2a ("RDMA/efa: Add support for node guid")
Reported-by: kernel test robot <lkp@intel.com>
Reviewed-by: Yehuda Yitschak <yehuday@amazon.com>
Reviewed-by: Yonatan Nachum <ynachum@amazon.com>
Signed-off-by: Michael Margolin <mrgolin@amazon.com>
---
 drivers/infiniband/hw/efa/efa_com_cmd.c | 2 +-
 drivers/infiniband/hw/efa/efa_com_cmd.h | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/infiniband/hw/efa/efa_com_cmd.c b/drivers/infiniband/hw/efa/efa_com_cmd.c
index 5a774925cdea..5754da4e6ff8 100644
--- a/drivers/infiniband/hw/efa/efa_com_cmd.c
+++ b/drivers/infiniband/hw/efa/efa_com_cmd.c
@@ -465,7 +465,7 @@ int efa_com_get_device_attr(struct efa_com_dev *edev,
 	result->db_bar = resp.u.device_attr.db_bar;
 	result->max_rdma_size = resp.u.device_attr.max_rdma_size;
 	result->device_caps = resp.u.device_attr.device_caps;
-	result->guid = resp.u.device_attr.guid;
+	result->guid = (__force __be64)resp.u.device_attr.guid;
 
 	if (result->admin_api_version < 1) {
 		ibdev_err_ratelimited(
diff --git a/drivers/infiniband/hw/efa/efa_com_cmd.h b/drivers/infiniband/hw/efa/efa_com_cmd.h
index 668d033f7477..56382cd1b7c4 100644
--- a/drivers/infiniband/hw/efa/efa_com_cmd.h
+++ b/drivers/infiniband/hw/efa/efa_com_cmd.h
@@ -112,7 +112,7 @@ struct efa_com_get_device_attr_result {
 	u8 addr[EFA_GID_SIZE];
 	u64 page_size_cap;
 	u64 max_mr_pages;
-	u64 guid;
+	__be64 guid;
 	u32 mtu;
 	u32 fw_version;
 	u32 admin_api_version;
-- 
2.40.1

Re: [PATCH] RDMA/efa: Fix node guid compiler warning

[Jason Gunthorpe]

On Tue, Sep 24, 2024 at 12:16:03PM +0000, Michael Margolin wrote:
> The GUID is received in big-endian so align types accordingly to avoid
> compiler warnings.
> 
> Closes: https://lore.kernel.org/oe-kbuild-all/202409032113.bvyVfsNp-lkp@intel.com/
> 
> Fixes: 04e36fd27a2a ("RDMA/efa: Add support for node guid")
> Reported-by: kernel test robot <lkp@intel.com>
> Reviewed-by: Yehuda Yitschak <yehuday@amazon.com>
> Reviewed-by: Yonatan Nachum <ynachum@amazon.com>
> Signed-off-by: Michael Margolin <mrgolin@amazon.com>
> ---
>  drivers/infiniband/hw/efa/efa_com_cmd.c | 2 +-
>  drivers/infiniband/hw/efa/efa_com_cmd.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/efa/efa_com_cmd.c b/drivers/infiniband/hw/efa/efa_com_cmd.c
> index 5a774925cdea..5754da4e6ff8 100644
> --- a/drivers/infiniband/hw/efa/efa_com_cmd.c
> +++ b/drivers/infiniband/hw/efa/efa_com_cmd.c
> @@ -465,7 +465,7 @@ int efa_com_get_device_attr(struct efa_com_dev *edev,
>  	result->db_bar = resp.u.device_attr.db_bar;
>  	result->max_rdma_size = resp.u.device_attr.max_rdma_size;
>  	result->device_caps = resp.u.device_attr.device_caps;
> -	result->guid = resp.u.device_attr.guid;
> +	result->guid = (__force __be64)resp.u.device_attr.guid;

That can't be right, use the proper conversion function, or the proper
type..

Jason

Re: [PATCH] RDMA/efa: Fix node guid compiler warning

[Margolin, Michael]

On 9/24/2024 9:00 PM, Jason Gunthorpe wrote:

> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Tue, Sep 24, 2024 at 12:16:03PM +0000, Michael Margolin wrote:
>> The GUID is received in big-endian so align types accordingly to avoid
>> compiler warnings.
>>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202409032113.bvyVfsNp-lkp@intel.com/
>>
>> Fixes: 04e36fd27a2a ("RDMA/efa: Add support for node guid")
>> Reported-by: kernel test robot <lkp@intel.com>
>> Reviewed-by: Yehuda Yitschak <yehuday@amazon.com>
>> Reviewed-by: Yonatan Nachum <ynachum@amazon.com>
>> Signed-off-by: Michael Margolin <mrgolin@amazon.com>
>> ---
>>   drivers/infiniband/hw/efa/efa_com_cmd.c | 2 +-
>>   drivers/infiniband/hw/efa/efa_com_cmd.h | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/infiniband/hw/efa/efa_com_cmd.c b/drivers/infiniband/hw/efa/efa_com_cmd.c
>> index 5a774925cdea..5754da4e6ff8 100644
>> --- a/drivers/infiniband/hw/efa/efa_com_cmd.c
>> +++ b/drivers/infiniband/hw/efa/efa_com_cmd.c
>> @@ -465,7 +465,7 @@ int efa_com_get_device_attr(struct efa_com_dev *edev,
>>        result->db_bar = resp.u.device_attr.db_bar;
>>        result->max_rdma_size = resp.u.device_attr.max_rdma_size;
>>        result->device_caps = resp.u.device_attr.device_caps;
>> -     result->guid = resp.u.device_attr.guid;
>> +     result->guid = (__force __be64)resp.u.device_attr.guid;
> That can't be right, use the proper conversion function, or the proper
> type..
>
> Jason
The current assumption in the driver is that data from the device 
arrives in a correct byte order for the CPU, while the guid field is in 
reversed order.

Would you suggest doing something like:

>-     result->guid = resp.u.device_attr.guid;
>+     result->guid = __cpu_to_be64(__swab64(resp.u.device_attr.guid));

Michael

Re: [PATCH] RDMA/efa: Fix node guid compiler warning

[Margolin, Michael]

On 9/26/2024 3:56 PM, Margolin, Michael wrote:
> On 9/24/2024 9:00 PM, Jason Gunthorpe wrote:
>
>> CAUTION: This email originated from outside of the organization. Do 
>> not click links or open attachments unless you can confirm the sender 
>> and know the content is safe.
>>
>>
>>
>> On Tue, Sep 24, 2024 at 12:16:03PM +0000, Michael Margolin wrote:
>>> The GUID is received in big-endian so align types accordingly to avoid
>>> compiler warnings.
>>>
>>> Closes: 
>>> https://lore.kernel.org/oe-kbuild-all/202409032113.bvyVfsNp-lkp@intel.com/
>>>
>>> Fixes: 04e36fd27a2a ("RDMA/efa: Add support for node guid")
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Reviewed-by: Yehuda Yitschak <yehuday@amazon.com>
>>> Reviewed-by: Yonatan Nachum <ynachum@amazon.com>
>>> Signed-off-by: Michael Margolin <mrgolin@amazon.com>
>>> ---
>>>   drivers/infiniband/hw/efa/efa_com_cmd.c | 2 +-
>>>   drivers/infiniband/hw/efa/efa_com_cmd.h | 2 +-
>>>   2 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/infiniband/hw/efa/efa_com_cmd.c 
>>> b/drivers/infiniband/hw/efa/efa_com_cmd.c
>>> index 5a774925cdea..5754da4e6ff8 100644
>>> --- a/drivers/infiniband/hw/efa/efa_com_cmd.c
>>> +++ b/drivers/infiniband/hw/efa/efa_com_cmd.c
>>> @@ -465,7 +465,7 @@ int efa_com_get_device_attr(struct efa_com_dev 
>>> *edev,
>>>        result->db_bar = resp.u.device_attr.db_bar;
>>>        result->max_rdma_size = resp.u.device_attr.max_rdma_size;
>>>        result->device_caps = resp.u.device_attr.device_caps;
>>> -     result->guid = resp.u.device_attr.guid;
>>> +     result->guid = (__force __be64)resp.u.device_attr.guid;
>> That can't be right, use the proper conversion function, or the proper
>> type..
>>
>> Jason
> The current assumption in the driver is that data from the device 
> arrives in a correct byte order for the CPU, while the guid field is 
> in reversed order.
>
> Would you suggest doing something like:
>
>> -     result->guid = resp.u.device_attr.guid;
>> +     result->guid = __cpu_to_be64(__swab64(resp.u.device_attr.guid));
>
> Michael
>
Actually that's wrong, the device always sets guid in BE order so no 
swap is needed in the driver in any case.

Re: [PATCH] RDMA/efa: Fix node guid compiler warning

[Jason Gunthorpe]

On Thu, Sep 26, 2024 at 04:25:19PM +0300, Margolin, Michael wrote:

> Actually that's wrong, the device always sets guid in BE order so no
> swap is needed in the driver in any case.

They you just mark it as _be64 in the struct and there is no reason
for the __force ?

Jason

Re: [PATCH] RDMA/efa: Fix node guid compiler warning

[Margolin, Michael]

On 9/26/2024 5:54 PM, Jason Gunthorpe wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Thu, Sep 26, 2024 at 04:25:19PM +0300, Margolin, Michael wrote:
>
>> Actually that's wrong, the device always sets guid in BE order so no
>> swap is needed in the driver in any case.
> They you just mark it as _be64 in the struct and there is no reason
> for the __force ?
>
> Jason

That's probably the most correct way but I prefer to avoid introducing 
kernel specific types in a shared interface file.

Michael

Re: [PATCH] RDMA/efa: Fix node guid compiler warning

[Jason Gunthorpe]

On Thu, Sep 26, 2024 at 11:03:57PM +0300, Margolin, Michael wrote:
> 
> On 9/26/2024 5:54 PM, Jason Gunthorpe wrote:
> > CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
> > 
> > 
> > 
> > On Thu, Sep 26, 2024 at 04:25:19PM +0300, Margolin, Michael wrote:
> > 
> > > Actually that's wrong, the device always sets guid in BE order so no
> > > swap is needed in the driver in any case.
> > They you just mark it as _be64 in the struct and there is no reason
> > for the __force ?
> > 
> > Jason
> 
> That's probably the most correct way but I prefer to avoid introducing
> kernel specific types in a shared interface file.

?

what is a "shared interface file" ?

That doesn't sound like a linux thing

Jason

Re: [PATCH] RDMA/efa: Fix node guid compiler warning

[Margolin, Michael]

On 9/27/2024 1:34 AM, Jason Gunthorpe wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On Thu, Sep 26, 2024 at 11:03:57PM +0300, Margolin, Michael wrote:
>> On 9/26/2024 5:54 PM, Jason Gunthorpe wrote:
>>> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>>>
>>>
>>>
>>> On Thu, Sep 26, 2024 at 04:25:19PM +0300, Margolin, Michael wrote:
>>>
>>>> Actually that's wrong, the device always sets guid in BE order so no
>>>> swap is needed in the driver in any case.
>>> They you just mark it as _be64 in the struct and there is no reason
>>> for the __force ?
>>>
>>> Jason
>> That's probably the most correct way but I prefer to avoid introducing
>> kernel specific types in a shared interface file.
> ?
>
> what is a "shared interface file" ?
>
> That doesn't sound like a linux thing
>
> Jason

Nothing particularly related to linux, just a common practice of having 
the same interface on both sides (driver and device in this case).

Michael

Re: [PATCH] RDMA/efa: Fix node guid compiler warning

[Jason Gunthorpe]

On Wed, Oct 02, 2024 at 03:30:30PM +0300, Margolin, Michael wrote:

> > > > > Actually that's wrong, the device always sets guid in BE order so no
> > > > > swap is needed in the driver in any case.

> > > > They you just mark it as _be64 in the struct and there is no reason
> > > > for the __force ?
> > > > 
> > > That's probably the most correct way but I prefer to avoid introducing
> > > kernel specific types in a shared interface file.
> > ?
> > 
> > what is a "shared interface file" ?
> > 
> > That doesn't sound like a linux thing
> 
> Nothing particularly related to linux, just a common practice of having the
> same interface on both sides (driver and device in this case).

Well, you still have to use correct types in the linux headers and
work that out somehow

Jason

Re: [PATCH] RDMA/efa: Fix node guid compiler warning

[Leon Romanovsky]

On Tue, Sep 24, 2024 at 12:16:03PM +0000, Michael Margolin wrote:
> The GUID is received in big-endian so align types accordingly to avoid
> compiler warnings.
> 
> Closes: https://lore.kernel.org/oe-kbuild-all/202409032113.bvyVfsNp-lkp@intel.com/
> 
> Fixes: 04e36fd27a2a ("RDMA/efa: Add support for node guid")
> Reported-by: kernel test robot <lkp@intel.com>
> Reviewed-by: Yehuda Yitschak <yehuday@amazon.com>
> Reviewed-by: Yonatan Nachum <ynachum@amazon.com>
> Signed-off-by: Michael Margolin <mrgolin@amazon.com>
> ---
>  drivers/infiniband/hw/efa/efa_com_cmd.c | 2 +-
>  drivers/infiniband/hw/efa/efa_com_cmd.h | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/efa/efa_com_cmd.c b/drivers/infiniband/hw/efa/efa_com_cmd.c
> index 5a774925cdea..5754da4e6ff8 100644
> --- a/drivers/infiniband/hw/efa/efa_com_cmd.c
> +++ b/drivers/infiniband/hw/efa/efa_com_cmd.c
> @@ -465,7 +465,7 @@ int efa_com_get_device_attr(struct efa_com_dev *edev,
>  	result->db_bar = resp.u.device_attr.db_bar;
>  	result->max_rdma_size = resp.u.device_attr.max_rdma_size;
>  	result->device_caps = resp.u.device_attr.device_caps;
> -	result->guid = resp.u.device_attr.guid;
> +	result->guid = (__force __be64)resp.u.device_attr.guid;

Based on the discussion, I'm dropping this patch.

Thanks