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
