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

[Adit Ranadive <aditr@vmware.com>]

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