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.
