On Wed, 24 Jan 2018 13:54:16 +0000, Stachura, Mariusz wrote: > >On Tue, 23 Jan 2018 10:55:33 +0000, Stachura, Mariusz wrote: > >> As for review: > >> 1) nit: reverse christmas tree > >> I'm fixing it. > >> 2) Are abilities guaranteed to be filled in by this function? Otherwise > >> you will use uninitialized stack memory. > >> Good point, I'm changing dev_dbg to dev_err and returning > >> immediately after printing it > >> 3) nit: looks like a GENMASK_ULL() coded as a loop > >> You are right, but I just realized this is not what I wanted :). I > >> wanted to include all enum values, however not all values between enums. > >> In example, there is a gap between: > >> I40E_PHY_TYPE_UNSUPPORTED = 0xF, > >> I40E_PHY_TYPE_100BASE_TX = 0x11, > >> And I did not want to include 0x10. I am adding #define that inludes > >> all ORed enums (in i40e_adminq_cmd.h, just after enum i40e_aq_phy_type > >> definition). I will assign this define to the mask, instead of using > >> GENMASK_ULL. > > > >What about the error handling? That was my main worry. That the > >communication with FW will fail on open and the link will remain disabled. > > Hi again, > > As I checked, this > ( > https://sourceforge.net/projects/e1000/files/i40e%20stable/2.4.3/i40e-2.4.3.tar.gz/download > ) > version had this patch applied, feel free to give it a shot.
Cool thanks, I tried that and the link does go down as expected (y) > Error handling was also addressed. i40e_open checks > i40e_force_link_state and returns EBUSY if get/set capabilities was > not successful.
