[PATCH for-rc] RDMA/vmw_pvrdma: Fix the active_speed and phys_state value

[PATCH for-rc] RDMA/vmw_pvrdma: Fix the active_speed and phys_state value

[Adit Ranadive]

The PVRDMA device still reports the active_speed in u8.
Lets use the ib_eth_get_speed to report the speed and
width. Unfortunately, phys_state gets stored as msb of
the new u16 active_speed.

Fixes: 376ceb31ff87 ("RDMA: Fix link active_speed size")
Reviewed-by: Vishnu Dasa <vdasa@vmware.com>
Signed-off-by: Adit Ranadive <aditr@vmware.com>
---
 drivers/infiniband/hw/vmw_pvrdma/pvrdma.h      | 18 ------------------
 .../infiniband/hw/vmw_pvrdma/pvrdma_verbs.c    |  8 ++++----
 .../infiniband/hw/vmw_pvrdma/pvrdma_verbs.h    |  2 +-
 3 files changed, 5 insertions(+), 23 deletions(-)

diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
index c142f5e7f25f..17f20506575f 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma.h
@@ -360,24 +360,6 @@ static inline enum pvrdma_port_width ib_port_width_to_pvrdma(
 	return (enum pvrdma_port_width)width;
 }
 
-static inline enum ib_port_width pvrdma_port_width_to_ib(
-					enum pvrdma_port_width width)
-{
-	return (enum ib_port_width)width;
-}
-
-static inline enum pvrdma_port_speed ib_port_speed_to_pvrdma(
-					enum ib_port_speed speed)
-{
-	return (enum pvrdma_port_speed)speed;
-}
-
-static inline enum ib_port_speed pvrdma_port_speed_to_ib(
-					enum pvrdma_port_speed speed)
-{
-	return (enum ib_port_speed)speed;
-}
-
 static inline int ib_qp_attr_mask_to_pvrdma(int attr_mask)
 {
 	return attr_mask & PVRDMA_MASK(PVRDMA_QP_ATTR_MASK_MAX);
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c
index fc412cbfd042..221705837d78 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.c
@@ -167,11 +167,11 @@ int pvrdma_query_port(struct ib_device *ibdev, u8 port,
 	props->sm_sl = resp->attrs.sm_sl;
 	props->subnet_timeout = resp->attrs.subnet_timeout;
 	props->init_type_reply = resp->attrs.init_type_reply;
-	props->active_width = pvrdma_port_width_to_ib(resp->attrs.active_width);
-	props->active_speed = pvrdma_port_speed_to_ib(resp->attrs.active_speed);
-	props->phys_state = resp->attrs.phys_state;
+	err = ib_get_eth_speed(ibdev, 1, &props->active_speed,
+			       &props->active_width);
+	props->phys_state = resp->attrs.active_speed >> 8;
 
-	return 0;
+	return err;
 }
 
 /**
diff --git a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.h b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.h
index f0e5ffba2d51..715416902992 100644
--- a/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.h
+++ b/drivers/infiniband/hw/vmw_pvrdma/pvrdma_verbs.h
@@ -178,7 +178,7 @@ struct pvrdma_port_attr {
 	u8			active_width;
 	u16			active_speed;
 	u8			phys_state;
-	u8			reserved[2];
+	u8			reserved;
 };
 
 struct pvrdma_global_route {
-- 
2.18.1

Re: [PATCH for-rc] RDMA/vmw_pvrdma: Fix the active_speed and phys_state value

[Jason Gunthorpe]

On Wed, Oct 28, 2020 at 11:19:45PM +0000, Adit Ranadive wrote:
> The PVRDMA device still reports the active_speed in u8.
> Lets use the ib_eth_get_speed to report the speed and
> width. Unfortunately, phys_state gets stored as msb of
> the new u16 active_speed.

This explanation is not clear, I have no idea what this is fixing

Jason

Re: [Suspected Spam] Re: [PATCH for-rc] RDMA/vmw_pvrdma: Fix the active_speed and phys_state value

[Adit Ranadive]

On 10/29/20 4:57 AM, Jason Gunthorpe wrote:
> On Wed, Oct 28, 2020 at 11:19:45PM +0000, Adit Ranadive wrote:
>> The PVRDMA device still reports the active_speed in u8.
>> Lets use the ib_eth_get_speed to report the speed and
>> width. Unfortunately, phys_state gets stored as msb of
>> the new u16 active_speed.
> 
> This explanation is not clear, I have no idea what this is fixing

It seemed more clear to me in my head, I guess :).

After commit 376ceb31ff87 changed the active_speed attribute to
u16, both the active_speed and phys_state attributes in the
pvrdma_port_attr struct are getting stored in this u16. As a 
result, these show up as invalid values in ibv_devinfo.

Our device still gives us back a u8 active_speed so both these
are getting stored in the u16. This fix I proposed simply gets 
the active_speed from the netdev while the phys_state still 
needs to come from the pvrdma device, i.e. the msb the of the
u16. I also removed some unused functions as a result.

Alternatively, I could change the u8 active_width and u16 
active_speed to reserved now that we're getting the active_speed
and active_width from the ib_get_eth_speed function.

> 
> Jason
> 

Re: Re: Re: [PATCH for-rc] RDMA/vmw_pvrdma: Fix the active_speed and phys_state value

[Adit Ranadive]

On 10/29/20 9:16 AM, Adit Ranadive wrote:
> On 10/29/20 4:57 AM, Jason Gunthorpe wrote:
>> On Wed, Oct 28, 2020 at 11:19:45PM +0000, Adit Ranadive wrote:
>>> The PVRDMA device still reports the active_speed in u8.
>>> Lets use the ib_eth_get_speed to report the speed and
>>> width. Unfortunately, phys_state gets stored as msb of
>>> the new u16 active_speed.
>>
>> This explanation is not clear, I have no idea what this is fixing
> 
> It seemed more clear to me in my head, I guess :).
> 
> After commit 376ceb31ff87 changed the active_speed attribute to
> u16, both the active_speed and phys_state attributes in the
> pvrdma_port_attr struct are getting stored in this u16. As a 
> result, these show up as invalid values in ibv_devinfo.
> 
> Our device still gives us back a u8 active_speed so both these
> are getting stored in the u16. This fix I proposed simply gets 
> the active_speed from the netdev while the phys_state still 
> needs to come from the pvrdma device, i.e. the msb the of the
> u16. I also removed some unused functions as a result.
> 
> Alternatively, I could change the u8 active_width and u16 
> active_speed to reserved now that we're getting the active_speed
> and active_width from the ib_get_eth_speed function.
> 

Jason, did you have any comments on this or did you want me
to just send v1 with an updated description?

>>
>> Jason
>>

Re: Re: Re: [PATCH for-rc] RDMA/vmw_pvrdma: Fix the active_speed and phys_state value

[Jason Gunthorpe]

On Mon, Nov 02, 2020 at 09:55:25AM -0800, Adit Ranadive wrote:
> On 10/29/20 9:16 AM, Adit Ranadive wrote:
> > On 10/29/20 4:57 AM, Jason Gunthorpe wrote:
> >> On Wed, Oct 28, 2020 at 11:19:45PM +0000, Adit Ranadive wrote:
> >>> The PVRDMA device still reports the active_speed in u8.
> >>> Lets use the ib_eth_get_speed to report the speed and
> >>> width. Unfortunately, phys_state gets stored as msb of
> >>> the new u16 active_speed.
> >>
> >> This explanation is not clear, I have no idea what this is fixing
> > 
> > It seemed more clear to me in my head, I guess :).
> > 
> > After commit 376ceb31ff87 changed the active_speed attribute to
> > u16, both the active_speed and phys_state attributes in the
> > pvrdma_port_attr struct are getting stored in this u16. As a 
> > result, these show up as invalid values in ibv_devinfo.
> > 
> > Our device still gives us back a u8 active_speed so both these
> > are getting stored in the u16. This fix I proposed simply gets 
> > the active_speed from the netdev while the phys_state still 
> > needs to come from the pvrdma device, i.e. the msb the of the
> > u16. I also removed some unused functions as a result.
> > 
> > Alternatively, I could change the u8 active_width and u16 
> > active_speed to reserved now that we're getting the active_speed
> > and active_width from the ib_get_eth_speed function.
> 
> Jason, did you have any comments on this or did you want me
> to just send v1 with an updated description?

I still haven't figured out what this is fixing.

Is 'struct pvrdma_port_attr' some kind of ABI? If so why isn't the fix
to revert the type?

Jason

Re: [PATCH for-rc] RDMA/vmw_pvrdma: Fix the active_speed and phys_state value

[Adit Ranadive]

On 11/2/20 10:02 AM, Jason Gunthorpe wrote:
> On Mon, Nov 02, 2020 at 09:55:25AM -0800, Adit Ranadive wrote:
>> On 10/29/20 9:16 AM, Adit Ranadive wrote:
>>> On 10/29/20 4:57 AM, Jason Gunthorpe wrote:
>>>> On Wed, Oct 28, 2020 at 11:19:45PM +0000, Adit Ranadive wrote:
>>>>> The PVRDMA device still reports the active_speed in u8.
>>>>> Lets use the ib_eth_get_speed to report the speed and
>>>>> width. Unfortunately, phys_state gets stored as msb of
>>>>> the new u16 active_speed.
>>>>
>>>> This explanation is not clear, I have no idea what this is fixing
>>>
>>> It seemed more clear to me in my head, I guess :).
>>>
>>> After commit 376ceb31ff87 changed the active_speed attribute to
>>> u16, both the active_speed and phys_state attributes in the
>>> pvrdma_port_attr struct are getting stored in this u16. As a 
>>> result, these show up as invalid values in ibv_devinfo.
>>>
>>> Our device still gives us back a u8 active_speed so both these
>>> are getting stored in the u16. This fix I proposed simply gets 
>>> the active_speed from the netdev while the phys_state still 
>>> needs to come from the pvrdma device, i.e. the msb the of the
>>> u16. I also removed some unused functions as a result.
>>>
>>> Alternatively, I could change the u8 active_width and u16 
>>> active_speed to reserved now that we're getting the active_speed
>>> and active_width from the ib_get_eth_speed function.
>>
>> Jason, did you have any comments on this or did you want me
>> to just send v1 with an updated description?
> 
> I still haven't figured out what this is fixing.
> 
> Is 'struct pvrdma_port_attr' some kind of ABI? If so why isn't the fix
> to revert the type?

I can revert it but I thought that it had to a u16 based on the IBTA, no?
Or does that not apply to device-level stuff?
Also, instead of reverting it seemed better to use the ib_get_eth_speed
function to get the active_speed based on the netdev.

Re: [PATCH for-rc] RDMA/vmw_pvrdma: Fix the active_speed and phys_state value

[Jason Gunthorpe]

On Mon, Nov 02, 2020 at 10:21:21AM -0800, Adit Ranadive wrote:
> On 11/2/20 10:02 AM, Jason Gunthorpe wrote:
> > On Mon, Nov 02, 2020 at 09:55:25AM -0800, Adit Ranadive wrote:
> >> On 10/29/20 9:16 AM, Adit Ranadive wrote:
> >>> On 10/29/20 4:57 AM, Jason Gunthorpe wrote:
> >>>> On Wed, Oct 28, 2020 at 11:19:45PM +0000, Adit Ranadive wrote:
> >>>>> The PVRDMA device still reports the active_speed in u8.
> >>>>> Lets use the ib_eth_get_speed to report the speed and
> >>>>> width. Unfortunately, phys_state gets stored as msb of
> >>>>> the new u16 active_speed.
> >>>>
> >>>> This explanation is not clear, I have no idea what this is fixing
> >>>
> >>> It seemed more clear to me in my head, I guess :).
> >>>
> >>> After commit 376ceb31ff87 changed the active_speed attribute to
> >>> u16, both the active_speed and phys_state attributes in the
> >>> pvrdma_port_attr struct are getting stored in this u16. As a 
> >>> result, these show up as invalid values in ibv_devinfo.
> >>>
> >>> Our device still gives us back a u8 active_speed so both these
> >>> are getting stored in the u16. This fix I proposed simply gets 
> >>> the active_speed from the netdev while the phys_state still 
> >>> needs to come from the pvrdma device, i.e. the msb the of the
> >>> u16. I also removed some unused functions as a result.
> >>>
> >>> Alternatively, I could change the u8 active_width and u16 
> >>> active_speed to reserved now that we're getting the active_speed
> >>> and active_width from the ib_get_eth_speed function.
> >>
> >> Jason, did you have any comments on this or did you want me
> >> to just send v1 with an updated description?
> > 
> > I still haven't figured out what this is fixing.
> > 
> > Is 'struct pvrdma_port_attr' some kind of ABI? If so why isn't the fix
> > to revert the type?
> 
> I can revert it but I thought that it had to a u16 based on the IBTA, no?
> Or does that not apply to device-level stuff?

You didn't answer the question, it it ABI to some kind of FW interface
or something?

*HOW* did two fields get overlapped onto a single u16?? The compiler
won't do this..

Jason

Re: [Suspected Spam] Re: [PATCH for-rc] RDMA/vmw_pvrdma: Fix the active_speed and phys_state value

[Adit Ranadive]

On 11/2/20 10:27 AM, Jason Gunthorpe wrote:
> On Mon, Nov 02, 2020 at 10:21:21AM -0800, Adit Ranadive wrote:
>> On 11/2/20 10:02 AM, Jason Gunthorpe wrote:
>>> On Mon, Nov 02, 2020 at 09:55:25AM -0800, Adit Ranadive wrote:
>>>> On 10/29/20 9:16 AM, Adit Ranadive wrote:
>>>>> On 10/29/20 4:57 AM, Jason Gunthorpe wrote:
>>>>>> On Wed, Oct 28, 2020 at 11:19:45PM +0000, Adit Ranadive wrote:
>>>>>>> The PVRDMA device still reports the active_speed in u8.
>>>>>>> Lets use the ib_eth_get_speed to report the speed and
>>>>>>> width. Unfortunately, phys_state gets stored as msb of
>>>>>>> the new u16 active_speed.
>>>>>>
>>>>>> This explanation is not clear, I have no idea what this is fixing
>>>>>
>>>>> It seemed more clear to me in my head, I guess :).
>>>>>
>>>>> After commit 376ceb31ff87 changed the active_speed attribute to
>>>>> u16, both the active_speed and phys_state attributes in the
>>>>> pvrdma_port_attr struct are getting stored in this u16. As a 
>>>>> result, these show up as invalid values in ibv_devinfo.
>>>>>
>>>>> Our device still gives us back a u8 active_speed so both these
>>>>> are getting stored in the u16. This fix I proposed simply gets 
>>>>> the active_speed from the netdev while the phys_state still 
>>>>> needs to come from the pvrdma device, i.e. the msb the of the
>>>>> u16. I also removed some unused functions as a result.
>>>>>
>>>>> Alternatively, I could change the u8 active_width and u16 
>>>>> active_speed to reserved now that we're getting the active_speed
>>>>> and active_width from the ib_get_eth_speed function.
>>>>
>>>> Jason, did you have any comments on this or did you want me
>>>> to just send v1 with an updated description?
>>>
>>> I still haven't figured out what this is fixing.
>>>
>>> Is 'struct pvrdma_port_attr' some kind of ABI? If so why isn't the fix
>>> to revert the type?
>>
>> I can revert it but I thought that it had to a u16 based on the IBTA, no?
>> Or does that not apply to device-level stuff?
> 
> You didn't answer the question, it it ABI to some kind of FW interface
> or something?
> 
> *HOW* did two fields get overlapped onto a single u16?? The compiler
> won't do this..
> 

It is an ABI to the device for port attributes. The device gives us back
this structure for query port verb. The response from the device is
memcopied into this pvrdma_port_attr structure. So both the bytes 
representing active_speed and phys_state from the device are copied 
into the single u16 in this structure.

Re: [Suspected Spam] Re: [PATCH for-rc] RDMA/vmw_pvrdma: Fix the active_speed and phys_state value

[Jason Gunthorpe]

On Mon, Nov 02, 2020 at 10:38:19AM -0800, Adit Ranadive wrote:
> On 11/2/20 10:27 AM, Jason Gunthorpe wrote:
> > On Mon, Nov 02, 2020 at 10:21:21AM -0800, Adit Ranadive wrote:
> >> On 11/2/20 10:02 AM, Jason Gunthorpe wrote:
> >>> On Mon, Nov 02, 2020 at 09:55:25AM -0800, Adit Ranadive wrote:
> >>>> On 10/29/20 9:16 AM, Adit Ranadive wrote:
> >>>>> On 10/29/20 4:57 AM, Jason Gunthorpe wrote:
> >>>>>> On Wed, Oct 28, 2020 at 11:19:45PM +0000, Adit Ranadive wrote:
> >>>>>>> The PVRDMA device still reports the active_speed in u8.
> >>>>>>> Lets use the ib_eth_get_speed to report the speed and
> >>>>>>> width. Unfortunately, phys_state gets stored as msb of
> >>>>>>> the new u16 active_speed.
> >>>>>>
> >>>>>> This explanation is not clear, I have no idea what this is fixing
> >>>>>
> >>>>> It seemed more clear to me in my head, I guess :).
> >>>>>
> >>>>> After commit 376ceb31ff87 changed the active_speed attribute to
> >>>>> u16, both the active_speed and phys_state attributes in the
> >>>>> pvrdma_port_attr struct are getting stored in this u16. As a 
> >>>>> result, these show up as invalid values in ibv_devinfo.
> >>>>>
> >>>>> Our device still gives us back a u8 active_speed so both these
> >>>>> are getting stored in the u16. This fix I proposed simply gets 
> >>>>> the active_speed from the netdev while the phys_state still 
> >>>>> needs to come from the pvrdma device, i.e. the msb the of the
> >>>>> u16. I also removed some unused functions as a result.
> >>>>>
> >>>>> Alternatively, I could change the u8 active_width and u16 
> >>>>> active_speed to reserved now that we're getting the active_speed
> >>>>> and active_width from the ib_get_eth_speed function.
> >>>>
> >>>> Jason, did you have any comments on this or did you want me
> >>>> to just send v1 with an updated description?
> >>>
> >>> I still haven't figured out what this is fixing.
> >>>
> >>> Is 'struct pvrdma_port_attr' some kind of ABI? If so why isn't the fix
> >>> to revert the type?
> >>
> >> I can revert it but I thought that it had to a u16 based on the IBTA, no?
> >> Or does that not apply to device-level stuff?
> > 
> > You didn't answer the question, it it ABI to some kind of FW interface
> > or something?
> > 
> > *HOW* did two fields get overlapped onto a single u16?? The compiler
> > won't do this..
> > 
> 
> It is an ABI to the device for port attributes. The device gives us back
> this structure for query port verb. The response from the device is
> memcopied into this pvrdma_port_attr structure. So both the bytes 
> representing active_speed and phys_state from the device are copied 
> into the single u16 in this structure.

So it is ABI and it shouldn't have been changed, point at the stuff
that made it ABI and revert the structure layout change..

Jason

Re: [Suspected Spam] Re: [PATCH for-rc] RDMA/vmw_pvrdma: Fix the active_speed and phys_state value

[Leon Romanovsky]

On Mon, Nov 02, 2020 at 02:46:40PM -0400, Jason Gunthorpe wrote:
> On Mon, Nov 02, 2020 at 10:38:19AM -0800, Adit Ranadive wrote:
> > On 11/2/20 10:27 AM, Jason Gunthorpe wrote:
> > > On Mon, Nov 02, 2020 at 10:21:21AM -0800, Adit Ranadive wrote:
> > >> On 11/2/20 10:02 AM, Jason Gunthorpe wrote:
> > >>> On Mon, Nov 02, 2020 at 09:55:25AM -0800, Adit Ranadive wrote:
> > >>>> On 10/29/20 9:16 AM, Adit Ranadive wrote:
> > >>>>> On 10/29/20 4:57 AM, Jason Gunthorpe wrote:
> > >>>>>> On Wed, Oct 28, 2020 at 11:19:45PM +0000, Adit Ranadive wrote:
> > >>>>>>> The PVRDMA device still reports the active_speed in u8.
> > >>>>>>> Lets use the ib_eth_get_speed to report the speed and
> > >>>>>>> width. Unfortunately, phys_state gets stored as msb of
> > >>>>>>> the new u16 active_speed.
> > >>>>>>
> > >>>>>> This explanation is not clear, I have no idea what this is fixing
> > >>>>>
> > >>>>> It seemed more clear to me in my head, I guess :).
> > >>>>>
> > >>>>> After commit 376ceb31ff87 changed the active_speed attribute to
> > >>>>> u16, both the active_speed and phys_state attributes in the
> > >>>>> pvrdma_port_attr struct are getting stored in this u16. As a
> > >>>>> result, these show up as invalid values in ibv_devinfo.
> > >>>>>
> > >>>>> Our device still gives us back a u8 active_speed so both these
> > >>>>> are getting stored in the u16. This fix I proposed simply gets
> > >>>>> the active_speed from the netdev while the phys_state still
> > >>>>> needs to come from the pvrdma device, i.e. the msb the of the
> > >>>>> u16. I also removed some unused functions as a result.
> > >>>>>
> > >>>>> Alternatively, I could change the u8 active_width and u16
> > >>>>> active_speed to reserved now that we're getting the active_speed
> > >>>>> and active_width from the ib_get_eth_speed function.
> > >>>>
> > >>>> Jason, did you have any comments on this or did you want me
> > >>>> to just send v1 with an updated description?
> > >>>
> > >>> I still haven't figured out what this is fixing.
> > >>>
> > >>> Is 'struct pvrdma_port_attr' some kind of ABI? If so why isn't the fix
> > >>> to revert the type?
> > >>
> > >> I can revert it but I thought that it had to a u16 based on the IBTA, no?
> > >> Or does that not apply to device-level stuff?
> > >
> > > You didn't answer the question, it it ABI to some kind of FW interface
> > > or something?
> > >
> > > *HOW* did two fields get overlapped onto a single u16?? The compiler
> > > won't do this..
> > >
> >
> > It is an ABI to the device for port attributes. The device gives us back
> > this structure for query port verb. The response from the device is
> > memcopied into this pvrdma_port_attr structure. So both the bytes
> > representing active_speed and phys_state from the device are copied
> > into the single u16 in this structure.
>
> So it is ABI and it shouldn't have been changed, point at the stuff
> that made it ABI and revert the structure layout change..

How will it work for the new IBTA speed?

Thanks

>
> Jason

Re: [Suspected Spam] Re: [PATCH for-rc] RDMA/vmw_pvrdma: Fix the active_speed and phys_state value

[Adit Ranadive]

On 11/2/20 10:56 PM, Leon Romanovsky wrote:
> On Mon, Nov 02, 2020 at 02:46:40PM -0400, Jason Gunthorpe wrote:
>> On Mon, Nov 02, 2020 at 10:38:19AM -0800, Adit Ranadive wrote:
>>> On 11/2/20 10:27 AM, Jason Gunthorpe wrote:
>>>> On Mon, Nov 02, 2020 at 10:21:21AM -0800, Adit Ranadive wrote:
>>>>> On 11/2/20 10:02 AM, Jason Gunthorpe wrote:
>>>>>> On Mon, Nov 02, 2020 at 09:55:25AM -0800, Adit Ranadive wrote:
>>>>>>> On 10/29/20 9:16 AM, Adit Ranadive wrote:
>>>>>>>> On 10/29/20 4:57 AM, Jason Gunthorpe wrote:
>>>>>>>>> On Wed, Oct 28, 2020 at 11:19:45PM +0000, Adit Ranadive wrote:
>>>>>>>>>> The PVRDMA device still reports the active_speed in u8.
>>>>>>>>>> Lets use the ib_eth_get_speed to report the speed and
>>>>>>>>>> width. Unfortunately, phys_state gets stored as msb of
>>>>>>>>>> the new u16 active_speed.
>>>>>>>>>
>>>>>>>>> This explanation is not clear, I have no idea what this is fixing
>>>>>>>>
>>>>>>>> It seemed more clear to me in my head, I guess :).
>>>>>>>>
>>>>>>>> After commit 376ceb31ff87 changed the active_speed attribute to
>>>>>>>> u16, both the active_speed and phys_state attributes in the
>>>>>>>> pvrdma_port_attr struct are getting stored in this u16. As a
>>>>>>>> result, these show up as invalid values in ibv_devinfo.
>>>>>>>>
>>>>>>>> Our device still gives us back a u8 active_speed so both these
>>>>>>>> are getting stored in the u16. This fix I proposed simply gets
>>>>>>>> the active_speed from the netdev while the phys_state still
>>>>>>>> needs to come from the pvrdma device, i.e. the msb the of the
>>>>>>>> u16. I also removed some unused functions as a result.
>>>>>>>>
>>>>>>>> Alternatively, I could change the u8 active_width and u16
>>>>>>>> active_speed to reserved now that we're getting the active_speed
>>>>>>>> and active_width from the ib_get_eth_speed function.
>>>>>>>
>>>>>>> Jason, did you have any comments on this or did you want me
>>>>>>> to just send v1 with an updated description?
>>>>>>
>>>>>> I still haven't figured out what this is fixing.
>>>>>>
>>>>>> Is 'struct pvrdma_port_attr' some kind of ABI? If so why isn't the fix
>>>>>> to revert the type?
>>>>>
>>>>> I can revert it but I thought that it had to a u16 based on the IBTA, no?
>>>>> Or does that not apply to device-level stuff?
>>>>
>>>> You didn't answer the question, it it ABI to some kind of FW interface
>>>> or something?
>>>>
>>>> *HOW* did two fields get overlapped onto a single u16?? The compiler
>>>> won't do this..
>>>>
>>>
>>> It is an ABI to the device for port attributes. The device gives us back
>>> this structure for query port verb. The response from the device is
>>> memcopied into this pvrdma_port_attr structure. So both the bytes
>>> representing active_speed and phys_state from the device are copied
>>> into the single u16 in this structure.
>>
>> So it is ABI and it shouldn't have been changed, point at the stuff
>> that made it ABI and revert the structure layout change..
> 
> How will it work for the new IBTA speed?

Hopefully that should be addressed in another patch I'll send out
that uses the ib_get_eth_speed api?

> 
> Thanks
> 
>>
>> Jason

Re: [Suspected Spam] Re: [PATCH for-rc] RDMA/vmw_pvrdma: Fix the active_speed and phys_state value

[Leon Romanovsky]

On Tue, Nov 03, 2020 at 02:41:06PM -0800, Adit Ranadive wrote:
> On 11/2/20 10:56 PM, Leon Romanovsky wrote:
> > On Mon, Nov 02, 2020 at 02:46:40PM -0400, Jason Gunthorpe wrote:
> >> On Mon, Nov 02, 2020 at 10:38:19AM -0800, Adit Ranadive wrote:
> >>> On 11/2/20 10:27 AM, Jason Gunthorpe wrote:
> >>>> On Mon, Nov 02, 2020 at 10:21:21AM -0800, Adit Ranadive wrote:
> >>>>> On 11/2/20 10:02 AM, Jason Gunthorpe wrote:
> >>>>>> On Mon, Nov 02, 2020 at 09:55:25AM -0800, Adit Ranadive wrote:
> >>>>>>> On 10/29/20 9:16 AM, Adit Ranadive wrote:
> >>>>>>>> On 10/29/20 4:57 AM, Jason Gunthorpe wrote:
> >>>>>>>>> On Wed, Oct 28, 2020 at 11:19:45PM +0000, Adit Ranadive wrote:
> >>>>>>>>>> The PVRDMA device still reports the active_speed in u8.
> >>>>>>>>>> Lets use the ib_eth_get_speed to report the speed and
> >>>>>>>>>> width. Unfortunately, phys_state gets stored as msb of
> >>>>>>>>>> the new u16 active_speed.
> >>>>>>>>>
> >>>>>>>>> This explanation is not clear, I have no idea what this is fixing
> >>>>>>>>
> >>>>>>>> It seemed more clear to me in my head, I guess :).
> >>>>>>>>
> >>>>>>>> After commit 376ceb31ff87 changed the active_speed attribute to
> >>>>>>>> u16, both the active_speed and phys_state attributes in the
> >>>>>>>> pvrdma_port_attr struct are getting stored in this u16. As a
> >>>>>>>> result, these show up as invalid values in ibv_devinfo.
> >>>>>>>>
> >>>>>>>> Our device still gives us back a u8 active_speed so both these
> >>>>>>>> are getting stored in the u16. This fix I proposed simply gets
> >>>>>>>> the active_speed from the netdev while the phys_state still
> >>>>>>>> needs to come from the pvrdma device, i.e. the msb the of the
> >>>>>>>> u16. I also removed some unused functions as a result.
> >>>>>>>>
> >>>>>>>> Alternatively, I could change the u8 active_width and u16
> >>>>>>>> active_speed to reserved now that we're getting the active_speed
> >>>>>>>> and active_width from the ib_get_eth_speed function.
> >>>>>>>
> >>>>>>> Jason, did you have any comments on this or did you want me
> >>>>>>> to just send v1 with an updated description?
> >>>>>>
> >>>>>> I still haven't figured out what this is fixing.
> >>>>>>
> >>>>>> Is 'struct pvrdma_port_attr' some kind of ABI? If so why isn't the fix
> >>>>>> to revert the type?
> >>>>>
> >>>>> I can revert it but I thought that it had to a u16 based on the IBTA, no?
> >>>>> Or does that not apply to device-level stuff?
> >>>>
> >>>> You didn't answer the question, it it ABI to some kind of FW interface
> >>>> or something?
> >>>>
> >>>> *HOW* did two fields get overlapped onto a single u16?? The compiler
> >>>> won't do this..
> >>>>
> >>>
> >>> It is an ABI to the device for port attributes. The device gives us back
> >>> this structure for query port verb. The response from the device is
> >>> memcopied into this pvrdma_port_attr structure. So both the bytes
> >>> representing active_speed and phys_state from the device are copied
> >>> into the single u16 in this structure.
> >>
> >> So it is ABI and it shouldn't have been changed, point at the stuff
> >> that made it ABI and revert the structure layout change..
> >
> > How will it work for the new IBTA speed?
>
> Hopefully that should be addressed in another patch I'll send out
> that uses the ib_get_eth_speed api?

It will work for now, but any new speed above NDR will break pvrdma.

Thanks

>
> >
> > Thanks
> >
> >>
> >> Jason