On 05.05.20 23:28, Jakub Kicinski wrote: > On Tue, 5 May 2020 21:57:43 +0200 Julian Wiedmann wrote: >>> This is the comment from the uAPI header: >>> >>> /* The reset() operation must clear the flags for the components which >>> * were actually reset. On successful return, the flags indicate the >>> * components which were not reset, either because they do not exist >>> * in the hardware or because they cannot be reset independently. The >>> * driver must never reset any components that were not requested. >>> */ >>> >>> Now let's take ETH_RESET_PHY as an example. Surely you're not resetting >>> any PHY here, so that bit should not be cleared. Please look at the >>> bits and select the ones which make sense, add whatever is missing. >>> >> >> It's a virtual device, _none_ of them make much sense?! We better not be >> resetting any actual HW components, the other interfaces on the same >> adapter would be quite unhappy about that. > > Well, then, you can't use the API in its current form. You can't say > none of the sub-options are applicable, but the sum of them does. > Agreed, that's my take as well. So we'll basically need a ETH_RESET_FULL bit, for devices that don't fit into the fine-grained component model. >> Sorry for being dense, and I appreciate that the API leaves a lot of room >> for sophisticated partial resets where the driver/HW allows it. >> But it sounds like what you're suggesting is >> (1) we select a rather arbitrary set of components that _might_ represent a >> full "virtual" reset, and then >> (2) expect the user to guess a super-set of these features. And not worry >> when they selected too much, and this obscure PHY thing failed to reset. > > No, please see the code I provided below, and read how the interface > is supposed to work. I posted the code comment in my previous reply. > I don't know what else I can do for you. > > User can still pass "all" but you can't _clear_ all bits, 'cause you > didn't reset any PHY, MAC, etc. > >> So I looked at gve's implementation and thought "yep, looks simple enough". > > Ugh, yeah, gve is not a good example. > >> But if we start asking users to interpret HW bits that hardly make any >> sense to them, we're worse off than with the existing custom sysfs trigger... > > Actually - operationally, how do you expect people to use this reset? > Some user space system detects the NIC is in a bad state? Does the > interface communicate that via some log messages or such? > > The commit message doesn't really explain the "why". > Usually the driver will detect a hung condition itself, and trigger an automatic reset internally (eg. from the TX watchdog). But if that doesn't work, you'll hopefully get enough noisy log warnings to investigate & reset the interface manually. Besides that, it's just an easy way to exercise/test the reset code. Integration with a daemon / management layer definitely sounds like an option, and I'd much rather point those people towards ethtool instead of sysfs. >>> Then my suggestion would be something like: >>> >>> #define QETH_RESET_FLAGS (flag | flag | flag) >>> >>> if ((*flags & QETH_RESET_FLAGS) != QETH_RESET_FLAGS)) >>> return -EINVAL; >>> ... >>> *flags &= ~QETH_RESET_FLAGS;
