| Message ID | 20220629035434.1891-1-luizluca@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: dsa: realtek: drop custom slave MII | expand |
On 29.06.2022 06:54, Luiz Angelo Daros de Luca wrote: > The last patch cleans all the deprecated code while keeping the kernel > messages. However, if there is no "mdio" node but there is a node with > the old compatible stings "realtek,smi-mdio", it will show an error. It > should still work but it will use polling instead of interruptions. > > My idea, if accepted, is to submit patches 1 and 2 now. After a > reasonable period, submit patch 3. > > I don't have an SMI-connected device and I'm asking for testers. It > would be nice to test the first 2 patches with: I'd love to test this on an Asus RT-AC88U which has got the smi-connected RTL8365MB switch but modifying the OpenWrt SDK to build for latest kernels is a really painful process. I know it's not related to this patch series but, does anyone know a more efficient way of building the kernel with rootfs with sufficent userspace tools? Like, am I supposed to use Buildroot, Yocto? Arınç
This RFC patch series cleans realtek-smi custom slave mii bus. Since fe7324b932, dsa generic code provides everything needed for realtek-smi driver. For extra caution, this series should be applied in two steps: the first 2 patches introduce the new code path that uses dsa generic code. It will show a warning message if the tree contains deprecated references. It will still fall back to the old code path if an "mdio" is not found. > > The last patch cleans all the deprecated code while keeping the kernel > messages. However, if there is no "mdio" node but there is a node with > the old compatible stings "realtek,smi-mdio", it will show an error. It > should still work but it will use polling instead of interruptions. > > My idea, if accepted, is to submit patches 1 and 2 now. After a > reasonable period, submit patch 3. > > I don't have an SMI-connected device and I'm asking for testers. It > would be nice to test the first 2 patches with: > 1) "mdio" without a compatible string. It should work without warnings. > 2) "mdio" with a compatible string. It should work with a warning asking > to remove the compatible string > 3) "xxx" node with compatible string. It should work with a warning > asking to rename "xxx" to "mdio" and remove the compatible string > > In all those cases, the switch should still keep using interruptions. > > After that, the last patch can be applied. The same tests can be > performed: > 1) "mdio" without a compatible string. It should work without warnings. > 2) "mdio" with a compatible string. It should work with a warning asking > to remove the compatible string > 3) "xxx" node with compatible string. It should work with an error > asking to rename "xxx" to "mdio" and remove the compatible string. The > switch will use polling instead of interruptions. > > This series might inspire other drivers as well. Currently, most dsa > driver implements a custom slave mii, normally only defining a > phy_{read,write} and loading properties from an "mdio" OF node. Since > fe7324b932, dsa generic code can do all that if the mdio node is named > "mdio". I believe most drivers could simply drop their slave mii > implementations and add phy_{read,write} to the dsa_switch_ops. For > drivers that look for an "mdio-like" node using a compatible string, it > might need some type of transition to let vendors update their OF tree. > > Regards, > > Luiz > I might have forgotten to add a new line after the subject. It ate the first paragraph. I'm top-posting it. Regards, Luiz
Em qua., 29 de jun. de 2022 às 09:30, Arınç ÜNAL <arinc.unal@arinc9.com> escreveu: > > On 29.06.2022 06:54, Luiz Angelo Daros de Luca wrote: > > The last patch cleans all the deprecated code while keeping the kernel > > messages. However, if there is no "mdio" node but there is a node with > > the old compatible stings "realtek,smi-mdio", it will show an error. It > > should still work but it will use polling instead of interruptions. > > > > My idea, if accepted, is to submit patches 1 and 2 now. After a > > reasonable period, submit patch 3. > > > > I don't have an SMI-connected device and I'm asking for testers. It > > would be nice to test the first 2 patches with: > > I'd love to test this on an Asus RT-AC88U which has got the > smi-connected RTL8365MB switch but modifying the OpenWrt SDK to build > for latest kernels is a really painful process. I know it's not related > to this patch series but, does anyone know a more efficient way of > building the kernel with rootfs with sufficent userspace tools? Like, am > I supposed to use Buildroot, Yocto? Hello Arinç, You can backport those patches to mostly any device already using rtl8365mb. The code it changes is mostly the same since the files migrated to the realtek directory. However, you do need to backport fe7324b932 (which is also easily applicable). I believe only the last patch will conflict as some new functions were added to the dsa_switch_ops that is being removed. Regards, Luiz
Hi Luiz, On Wed, Jun 29, 2022 at 01:43:45PM -0300, Luiz Angelo Daros de Luca wrote: > This RFC patch series cleans realtek-smi custom slave mii bus. Since > fe7324b932, dsa generic code provides everything needed for > realtek-smi driver. For extra caution, this series should be applied > in two steps: the first 2 patches introduce the new code path that > uses dsa generic code. It will show a warning message if the tree > contains deprecated references. It will still fall back to the old > code path if an "mdio" > is not found. In principle I like your changes, but I'm not sure if what you are doing is allowed, since DT is ABI. The fact that you have to split this into two steps, with the first step warning about old "incompatible" DTs (your point 3 below) before the second step breaks that compatibility, suggests that you are aware that you could be breaking old DTs. I'm not going to argue with you if you say "but the node with compatible realtek,smi-mdio was also called mdio in the bindings, so it shouldn't break old DTs", which is a valid point. But if that is your rationale, then there's no need to split the series at all, right? If you want to avoid that debate, what you could do instead is add a const char *slave_mii_compatible; member to struct dsa_switch, and try searching in dsa_switch_setup() for a child node with that compatible if the lookup of a node named "mdio" fails. I don't know if this would help you do the same thing with other drivers. Btw, I think the first patch in the series is kind of pointless. You can just do the rename of ds_ops_mdio to ds_ops in the last patch, adding your justification in the commit message: "while we're at it, rename ds_ops_mdio etc...". Kind regards, Alvin > > > > > The last patch cleans all the deprecated code while keeping the kernel > > messages. However, if there is no "mdio" node but there is a node with > > the old compatible stings "realtek,smi-mdio", it will show an error. It > > should still work but it will use polling instead of interruptions. > > > > My idea, if accepted, is to submit patches 1 and 2 now. After a > > reasonable period, submit patch 3. > > > > I don't have an SMI-connected device and I'm asking for testers. It > > would be nice to test the first 2 patches with: > > 1) "mdio" without a compatible string. It should work without warnings. > > 2) "mdio" with a compatible string. It should work with a warning asking > > to remove the compatible string > > 3) "xxx" node with compatible string. It should work with a warning > > asking to rename "xxx" to "mdio" and remove the compatible string > > > > In all those cases, the switch should still keep using interruptions. > > > > After that, the last patch can be applied. The same tests can be > > performed: > > 1) "mdio" without a compatible string. It should work without warnings. > > 2) "mdio" with a compatible string. It should work with a warning asking > > to remove the compatible string > > 3) "xxx" node with compatible string. It should work with an error > > asking to rename "xxx" to "mdio" and remove the compatible string. The > > switch will use polling instead of interruptions. > > > > This series might inspire other drivers as well. Currently, most dsa > > driver implements a custom slave mii, normally only defining a > > phy_{read,write} and loading properties from an "mdio" OF node. Since > > fe7324b932, dsa generic code can do all that if the mdio node is named > > "mdio". I believe most drivers could simply drop their slave mii > > implementations and add phy_{read,write} to the dsa_switch_ops. For > > drivers that look for an "mdio-like" node using a compatible string, it > > might need some type of transition to let vendors update their OF tree. > > > > Regards, > > > > Luiz > > > > I might have forgotten to add a new line after the subject. It ate the > first paragraph. I'm top-posting it. > > Regards, > > Luiz
On Wed, Jun 29, 2022 at 06:14:56PM +0000, Alvin Šipraga wrote: > If you want to avoid that debate, what you could do instead is add a > const char *slave_mii_compatible; member to struct dsa_switch, and try > searching in dsa_switch_setup() for a child node with that compatible if > the lookup of a node named "mdio" fails. I don't know if this would help > you do the same thing with other drivers. Forgot to make it clear, but here I meant that a DSA driver could then set ds->slave_mii_compatible = "realtek,smi-mdio" in its probe function before calling dsa_register_switch(ds).
> Hi Luiz, > > On Wed, Jun 29, 2022 at 01:43:45PM -0300, Luiz Angelo Daros de Luca wrote: > > This RFC patch series cleans realtek-smi custom slave mii bus. Since > > fe7324b932, dsa generic code provides everything needed for > > realtek-smi driver. For extra caution, this series should be applied > > in two steps: the first 2 patches introduce the new code path that > > uses dsa generic code. It will show a warning message if the tree > > contains deprecated references. It will still fall back to the old > > code path if an "mdio" > > is not found. > > In principle I like your changes, but I'm not sure if what you are doing > is allowed, since DT is ABI. The fact that you have to split this into > two steps, with the first step warning about old "incompatible" DTs > (your point 3 below) before the second step breaks that compatibility, > suggests that you are aware that you could be breaking old DTs. Thanks Alvin for your review. Yes, that is a good question for the ML. I don't know at what level we can break compatibility (DT and driver). That's why it is a RFC. > I'm not going to argue with you if you say "but the node with compatible > realtek,smi-mdio was also called mdio in the bindings, so it shouldn't > break old DTs", which is a valid point. But if that is your rationale, > then there's no need to split the series at all, right? The DT requires "realtek,smi-mdio" but also mentions the "mdio" name, not a generic name as "mdioX". If we agree that the name "mdio" is already required by the DT bindings, it is the driver implementation that is not compliant. Even if we are not violating the DT bindings, we are changing the driver behavior. That's why I suggested the transition process. I do believe that it would be very, very rare to name that mdio as anything other than "mdio" and even the driver itself is too fresh to be widespread. In a non-RFC series, I would also drop the "realtek,smi-mdio" compatible string from the bindings (as it is back compatible). > If you want to avoid that debate, what you could do instead is add a > const char *slave_mii_compatible; member to struct dsa_switch, and try > searching in dsa_switch_setup() for a child node with that compatible if > the lookup of a node named "mdio" fails. I don't know if this would help > you do the same thing with other drivers. The DSA change to accept "mdio" was an improvement to avoid adding a custom slave mdio when you already have a single mdio and just need to point to a DT node. Adding compatible strings for that situation does not make much sense as a compatible string is not necessary when you are already restricting your case to a single mdio. For more complex setups, you still need to create your own slave mdio implementation. Some drivers already depend on the "mdio" name and this series is also a suggestion for them to try their drivers dropping their custom slave mdio implementations. > Btw, I think the first patch in the series is kind of pointless. You can > just do the rename of ds_ops_mdio to ds_ops in the last patch, adding > your justification in the commit message: "while we're at it, rename > ds_ops_mdio etc...". As a RFC, I'm trying to split each change in such a way they can be merged individually. I believe that the new names make it clearer why we have two structures. Even if the idea behind this series did not get accepted, that first patch might be useful for someone reading the driver for the first time. Regards, > > Kind regards, > Alvin > > > > > > > > > The last patch cleans all the deprecated code while keeping the kernel > > > messages. However, if there is no "mdio" node but there is a node with > > > the old compatible stings "realtek,smi-mdio", it will show an error. It > > > should still work but it will use polling instead of interruptions. > > > > > > My idea, if accepted, is to submit patches 1 and 2 now. After a > > > reasonable period, submit patch 3. > > > > > > I don't have an SMI-connected device and I'm asking for testers. It > > > would be nice to test the first 2 patches with: > > > 1) "mdio" without a compatible string. It should work without warnings. > > > 2) "mdio" with a compatible string. It should work with a warning asking > > > to remove the compatible string > > > 3) "xxx" node with compatible string. It should work with a warning > > > asking to rename "xxx" to "mdio" and remove the compatible string > > > > > > In all those cases, the switch should still keep using interruptions. > > > > > > After that, the last patch can be applied. The same tests can be > > > performed: > > > 1) "mdio" without a compatible string. It should work without warnings. > > > 2) "mdio" with a compatible string. It should work with a warning asking > > > to remove the compatible string > > > 3) "xxx" node with compatible string. It should work with an error > > > asking to rename "xxx" to "mdio" and remove the compatible string. The > > > switch will use polling instead of interruptions. > > > > > > This series might inspire other drivers as well. Currently, most dsa > > > driver implements a custom slave mii, normally only defining a > > > phy_{read,write} and loading properties from an "mdio" OF node. Since > > > fe7324b932, dsa generic code can do all that if the mdio node is named > > > "mdio". I believe most drivers could simply drop their slave mii > > > implementations and add phy_{read,write} to the dsa_switch_ops. For > > > drivers that look for an "mdio-like" node using a compatible string, it > > > might need some type of transition to let vendors update their OF tree. > > > > > > Regards, > > > > > > Luiz > > > > > > > I might have forgotten to add a new line after the subject. It ate the > > first paragraph. I'm top-posting it. > > > > Regards, > > > > Luiz
On Thu, Jun 30, 2022 at 02:05:39PM -0300, Luiz Angelo Daros de Luca wrote: > > If you want to avoid that debate, what you could do instead is add a > > const char *slave_mii_compatible; member to struct dsa_switch, and try > > searching in dsa_switch_setup() for a child node with that compatible if > > the lookup of a node named "mdio" fails. I don't know if this would help > > you do the same thing with other drivers. > > The DSA change to accept "mdio" was an improvement to avoid adding a > custom slave mdio when you already have a single mdio and just need to > point to a DT node. Adding compatible strings for that situation does > not make much sense as a compatible string is not necessary when you > are already restricting your case to a single mdio. For more complex > setups, you still need to create your own slave mdio implementation. > Some drivers already depend on the "mdio" name and this series is also > a suggestion for them to try their drivers dropping their custom slave > mdio implementations. It was just a suggestion for how to achieve what you want (less code in the driver) without breaking backward compatibility. Obviously newly submitted drivers shouldn't use the interface for the reasons you just gave. Whether it is a good idea to add such an interface to the DSA core, idk. Personally I think it's fair enough, but I also don't mind the status quo. The code you're removing is not exactly doing any harm. I think my suggestion only makes sense if you can show that other drivers can benefit as well. Kind regards, Alvin
On Thu, Jun 30, 2022 at 02:05:39PM -0300, Luiz Angelo Daros de Luca wrote: > > Hi Luiz, > > > > On Wed, Jun 29, 2022 at 01:43:45PM -0300, Luiz Angelo Daros de Luca wrote: > > > This RFC patch series cleans realtek-smi custom slave mii bus. Since > > > fe7324b932, dsa generic code provides everything needed for > > > realtek-smi driver. For extra caution, this series should be applied > > > in two steps: the first 2 patches introduce the new code path that > > > uses dsa generic code. It will show a warning message if the tree > > > contains deprecated references. It will still fall back to the old > > > code path if an "mdio" > > > is not found. > > > > In principle I like your changes, but I'm not sure if what you are doing > > is allowed, since DT is ABI. The fact that you have to split this into > > two steps, with the first step warning about old "incompatible" DTs > > (your point 3 below) before the second step breaks that compatibility, > > suggests that you are aware that you could be breaking old DTs. > > Thanks Alvin for your review. Yes, that is a good question for the ML. > I don't know at what level we can break compatibility (DT and driver). > That's why it is a RFC. DT bindings are only extended in backwards-compatible ways. Only in the case where you can prove that there is no DT user of a certain binding, and that none should appear either, is when you can consider breaking the backward compatibility. The idea here is that old DT blobs may live forever and be provided by fixed firmware such as U-Boot, you can't really force anyone to update them. > > I'm not going to argue with you if you say "but the node with compatible > > realtek,smi-mdio was also called mdio in the bindings, so it shouldn't > > break old DTs", which is a valid point. But if that is your rationale, > > then there's no need to split the series at all, right? > > The DT requires "realtek,smi-mdio" but also mentions the "mdio" name, > not a generic name as "mdioX". If we agree that the name "mdio" is > already required by the DT bindings, it is the driver implementation > that is not compliant. Even if we are not violating the DT bindings, > we are changing the driver behavior. That's why I suggested the > transition process. I do believe that it would be very, very rare to > name that mdio as anything other than "mdio" and even the driver > itself is too fresh to be widespread. In a non-RFC series, I would > also drop the "realtek,smi-mdio" compatible string from the bindings > (as it is back compatible). Technically the MDIO node should be named "mdio", at least that's what Documentation/devicetree/bindings/net/mdio.yaml says, that it should be "mdio" or "mdio@something". And since the #address-cells of the DSA switch OF node itself (parent of MDIO node) is 0, it can't really be "mdio@something", so that leaves us with "mdio". However I don't think there is any validation for the node name. > > If you want to avoid that debate, what you could do instead is add a > > const char *slave_mii_compatible; member to struct dsa_switch, and try > > searching in dsa_switch_setup() for a child node with that compatible if > > the lookup of a node named "mdio" fails. I don't know if this would help > > you do the same thing with other drivers. > > The DSA change to accept "mdio" was an improvement to avoid adding a > custom slave mdio when you already have a single mdio and just need to > point to a DT node. Adding compatible strings for that situation does > not make much sense as a compatible string is not necessary when you > are already restricting your case to a single mdio. For more complex > setups, you still need to create your own slave mdio implementation. > Some drivers already depend on the "mdio" name and this series is also > a suggestion for them to try their drivers dropping their custom slave > mdio implementations. I think we are going down a slippery slope here. DSA providing generic code to register an MDIO bus is not something I'd consider core functionality, and I don't consider any duplication of concerns or logic if drivers register their own MDIO buses. Here you are effectively proposing to gratuitously break the DT binding, the obvious question is, what is there to even gain from this? And why do you even need to remove the compatible string from the MDIO node, can't you just ignore it, does it bother you in any way?
On Wed, Jul 6, 2022 at 5:29 PM Vladimir Oltean <olteanv@gmail.com> wrote: > On Thu, Jun 30, 2022 at 02:05:39PM -0300, Luiz Angelo Daros de Luca wrote: > > > In principle I like your changes, but I'm not sure if what you are doing > > > is allowed, since DT is ABI. The fact that you have to split this into > > > two steps, with the first step warning about old "incompatible" DTs > > > (your point 3 below) before the second step breaks that compatibility, > > > suggests that you are aware that you could be breaking old DTs. > > > > Thanks Alvin for your review. Yes, that is a good question for the ML. > > I don't know at what level we can break compatibility (DT and driver). > > That's why it is a RFC. > > DT bindings are only extended in backwards-compatible ways. Only in the > case where you can prove that there is no DT user of a certain binding, > and that none should appear either, is when you can consider breaking > the backward compatibility. The idea here is that old DT blobs may live > forever and be provided by fixed firmware such as U-Boot, you can't > really force anyone to update them. We break it when it makes sense. The central question is to ascertain if there are actually binary DTBs deployed with these bindings, in mass-market products, and these are not upgraded in tandem with the kernel. A mistake (IMO) in the early days of DT was to assume that it was used with Open Firmware (OF) which is like ACPI, a kind of BIOS. Most users of DT do not use OF, the only thing we ever see relating to it is the of_* prefix. People actually using open firmware would embed the DTB with the open firmware and flash it into a (desktop) computer as a blob, pretty much like how the ACPI BIOS works now. It turns out the majority of contemporary users of DT don't use DTBs like this at all: instead they compile the kernel and the DTB, then flash both into the platform at the same time. There is even the FIT format for U-Boot which is a package of both kernel and DT and whatnot. Actually very few people flash their DTB in such a way that it cannot get upgraded, and in fact most flash both at the same time, after building both from source. In that case it doesn't matter if we break compatibility. While we strive to keep DT schemas strict and compatible (it is a good ambition) I would reverse the burden of proof for backward compatibility: if it can not be proven that irrevocable DTBs have been deployed, and that kernels may get upgraded independently of the DTB, using this specific binding, then it is fine to change it in incompatible ways if we need to. It could also be that the DT bindings started to get used with another operating system. But these things have to be demonstrated, they are the rare cases and should not be the assumption, as if a DT binding is immediately used in a myriad of places the second we merge it to Torvald's tree. Such adoption in the real world happens much later. If the only specimens are inside a company that has not yet released any products we can certainly change it. What we don't want is the general public running into these incompatibilities. Notice that as we discuss this, I see some people being requested to reflash their (ACPI) bioses rather than put fixes in the kernel for erroneous ACPI DSDT:s. Not for end users, but for people working with prototypes still in development. "Go fix your DSDT BIOS tables". Yours, Linus Walleij
The last patch cleans all the deprecated code while keeping the kernel messages. However, if there is no "mdio" node but there is a node with the old compatible stings "realtek,smi-mdio", it will show an error. It should still work but it will use polling instead of interruptions. My idea, if accepted, is to submit patches 1 and 2 now. After a reasonable period, submit patch 3. I don't have an SMI-connected device and I'm asking for testers. It would be nice to test the first 2 patches with: 1) "mdio" without a compatible string. It should work without warnings. 2) "mdio" with a compatible string. It should work with a warning asking to remove the compatible string 3) "xxx" node with compatible string. It should work with a warning asking to rename "xxx" to "mdio" and remove the compatible string In all those cases, the switch should still keep using interruptions. After that, the last patch can be applied. The same tests can be performed: 1) "mdio" without a compatible string. It should work without warnings. 2) "mdio" with a compatible string. It should work with a warning asking to remove the compatible string 3) "xxx" node with compatible string. It should work with an error asking to rename "xxx" to "mdio" and remove the compatible string. The switch will use polling instead of interruptions. This series might inspire other drivers as well. Currently, most dsa driver implements a custom slave mii, normally only defining a phy_{read,write} and loading properties from an "mdio" OF node. Since fe7324b932, dsa generic code can do all that if the mdio node is named "mdio". I believe most drivers could simply drop their slave mii implementations and add phy_{read,write} to the dsa_switch_ops. For drivers that look for an "mdio-like" node using a compatible string, it might need some type of transition to let vendors update their OF tree. Regards, Luiz