| Message ID | 20240924121603.16006-1-mrgolin@amazon.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | RDMA/efa: Fix node guid compiler warning | expand |
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
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
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.
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
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
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
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
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
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
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;
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(-)