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
