| Message ID | 20200929161307.542-1-irusskikh@marvell.com |
|---|---|
| Headers | show |
| Series | net: atlantic: phy tunables from mac driver | expand |
On Tue, Sep 29, 2020 at 07:13:04PM +0300, Igor Russkikh wrote: > This series implements phy tunables settings via MAC driver callbacks. > > AQC 10G devices use integrated MAC+PHY solution, where PHY is fully controlled > by MAC firmware. Therefore, it is not possible to implement separate phy driver > for these. > > We use ethtool ops callbacks to implement downshift and EDPC tunables. Hi Michal Do you have any code to implement tunables via netlink? This code is defining new ethtool calls. It seems like now would be a good time to plumb in extack, so the driver can report back the valid range of a tunable when given an unsupported value. Andrew
On Tue, 29 Sep 2020 19:04:13 +0200 Andrew Lunn wrote: > On Tue, Sep 29, 2020 at 07:13:04PM +0300, Igor Russkikh wrote: > > This series implements phy tunables settings via MAC driver callbacks. > > > > AQC 10G devices use integrated MAC+PHY solution, where PHY is fully controlled > > by MAC firmware. Therefore, it is not possible to implement separate phy driver > > for these. > > > > We use ethtool ops callbacks to implement downshift and EDPC tunables. > > Hi Michal > > Do you have any code to implement tunables via netlink? > > This code is defining new ethtool calls. It seems like now would be a > good time to plumb in extack, so the driver can report back the valid > range of a tunable when given an unsupported value. Do you mean report supported range via extack? Perhaps we should have a real API for that kind of info? We can plumb it through to the core for now and expose to user space once netlink comes.
> Do you mean report supported range via extack?
Yes.
811ac400ea33 ("net: phy: dp83869: Add speed optimization feature")
was merged recently. It has:
+ default:
+ phydev_err(phydev,
+ "Downshift count must be 1, 2, 4 or 8\n");
+ return -EINVAL;
and there are more examples in PHY drivers where it would be good to
tell the uses what the valid values are. I guess most won't see this
kernel message, but if netlink ethtool printed:
Invalid Argument: Downshift count must be 1, 2, 4 or 8
it would be a lot more user friendly.
Andrew
On Tue, 29 Sep 2020 20:47:23 +0200 Andrew Lunn wrote: > > Do you mean report supported range via extack? > > Yes. > > 811ac400ea33 ("net: phy: dp83869: Add speed optimization feature") > > was merged recently. It has: > > + default: > + phydev_err(phydev, > + "Downshift count must be 1, 2, 4 or 8\n"); > + return -EINVAL; > > and there are more examples in PHY drivers where it would be good to > tell the uses what the valid values are. I guess most won't see this > kernel message, but if netlink ethtool printed: > > Invalid Argument: Downshift count must be 1, 2, 4 or 8 > > it would be a lot more user friendly. Ah, now I recall, we already discussed this. FWIW we could provision for the extack and just pass NULL for now? Would that be too ugly?
On Tue, Sep 29, 2020 at 05:09:48PM -0700, Jakub Kicinski wrote: > On Tue, 29 Sep 2020 20:47:23 +0200 Andrew Lunn wrote: > > > Do you mean report supported range via extack? > > > > Yes. > > > > 811ac400ea33 ("net: phy: dp83869: Add speed optimization feature") > > > > was merged recently. It has: > > > > + default: > > + phydev_err(phydev, > > + "Downshift count must be 1, 2, 4 or 8\n"); > > + return -EINVAL; > > > > and there are more examples in PHY drivers where it would be good to > > tell the uses what the valid values are. I guess most won't see this > > kernel message, but if netlink ethtool printed: > > > > Invalid Argument: Downshift count must be 1, 2, 4 or 8 > > > > it would be a lot more user friendly. > > Ah, now I recall, we already discussed this. > > FWIW we could provision for the extack and just pass NULL for now? > Would that be too ugly? If Michal does not have any code lying around in a drawer, what might be a good idea. For the old IOCTL we will need to pass a NULL anyway. Andrew
On Tue, Sep 29, 2020 at 07:04:13PM +0200, Andrew Lunn wrote: > On Tue, Sep 29, 2020 at 07:13:04PM +0300, Igor Russkikh wrote: > > This series implements phy tunables settings via MAC driver callbacks. > > > > AQC 10G devices use integrated MAC+PHY solution, where PHY is fully controlled > > by MAC firmware. Therefore, it is not possible to implement separate phy driver > > for these. > > > > We use ethtool ops callbacks to implement downshift and EDPC tunables. > > Hi Michal > > Do you have any code to implement tunables via netlink? I already tried to open a discussion about tunables once but it somehow died before we got somewhere and I'm not sure I was fully understood then. My view is that tunables (both the "ethtool" ones and PHY tunables) were a workaround for lack of extensibility of the ioctl interface where adding a new parameter to existing command was often impossible while adding a tunable was relatively easy. But the implementation doesn't really scale well so a rework would be necessary if the number of tunables were to grow to the order of tens. Moreover, recently it surfaced that while we have type id for string tunables, nobody seems to know how exactly are they supposed to work. With netlink, we can add new attributes easily and I don't see much advantage in adding more tunables (assorted heap of unrelated values) over adding either attributes to existing commands or new commands (for new types of information or settings). PHY tunables could be probably grouped into one command. Rx and Tx copybreak could belong together as "skb handling parameters" (?) and I've seen someone proposing another parameter (IIRC related to head split) which would also belong there. I'm not sure about ETHTOOL_PFC_PREVENTION_TOUT. What would IMHO justify a similar concept to current tunables would be device (driver) specific tunables, i.e. generalization of private flags to other data types. But as I said before, I'm not sure if we want such feature as it would be probably too tempting to abuse by NIC vendors. > This code is defining new ethtool calls. It seems like now would be a > good time to plumb in extack, so the driver can report back the valid > range of a tunable when given an unsupported value. Adding an extack pointer to new ethtool ops seems like the most natural solution. For existing ethtool ops, David Miller suggested that as all of them are called under RTNL lock (and we cannot easily git rid of it after many years of such guarantee), we could in fact use a global variable. Or maybe rather a helper function. Another question is how to allow ethtool ops setting not only the text message but also the offending attribute. So far the idea I was able to come with is that the ethtool_ops callback could set one (or two in case of nested requests, we probably won't need more) integers to identify the attribute (or top level and nested) and caller would translate them to a pointer into the request message. Michal
> Another question is how to allow ethtool ops setting not only the text > message but also the offending attribute. For PHY tunables, i don't think it is needed. The current API only allows a single value to be passed. That seems to be enough, and it forces us to keep tunables simple. If need be, the core could set the attribute, since there should only be one containing the value. Andrew
On Wed, Sep 30, 2020 at 05:16:56PM +0200, Andrew Lunn wrote: > > Another question is how to allow ethtool ops setting not only the text > > message but also the offending attribute. > > For PHY tunables, i don't think it is needed. The current API only > allows a single value to be passed. That seems to be enough, and it > forces us to keep tunables simple. If need be, the core could set the > attribute, since there should only be one containing the value. It probably wasn't obvious but I mean the two parts of my e-mail as independent, i.e. the second part was rather meant as a general thought on allowing drivers to report errors/warnings via extack, not specific to PHY tunables. Michal