| Message ID | 20220727064321.2953971-7-mw@semihalf.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | DSA: switch to fwnode_/device_ | expand |
On Wed, Jul 27, 2022 at 08:43:19AM +0200, Marcin Wojtas wrote: > A helper function which allows getting the struct net_device pointer > associated with a given device tree node can be more generic and > also support alternative hardware description. Switch to fwnode_ > and update the only existing caller in DSA subsystem. > For that purpose use newly added fwnode_dev_node_match helper routine. > > Signed-off-by: Marcin Wojtas <mw@semihalf.com> > --- > -struct net_device *of_find_net_device_by_node(struct device_node *np) > +struct net_device *fwnode_find_net_device_by_node(struct fwnode_handle *fwnode) > { > struct device *dev; > > - dev = class_find_device(&net_class, NULL, np, of_dev_node_match); > + dev = class_find_device(&net_class, NULL, fwnode, fwnode_find_parent_dev_match); This needs to maintain compatibility with DSA masters that have dev->of_node but don't have dev->fwnode populated. > if (!dev) > return NULL; > > return to_net_dev(dev); > } > -EXPORT_SYMBOL(of_find_net_device_by_node); > -#endif > +EXPORT_SYMBOL(fwnode_find_net_device_by_node);
śr., 27 lip 2022 o 16:31 Vladimir Oltean <olteanv@gmail.com> napisał(a): > > On Wed, Jul 27, 2022 at 08:43:19AM +0200, Marcin Wojtas wrote: > > A helper function which allows getting the struct net_device pointer > > associated with a given device tree node can be more generic and > > also support alternative hardware description. Switch to fwnode_ > > and update the only existing caller in DSA subsystem. > > For that purpose use newly added fwnode_dev_node_match helper routine. > > > > Signed-off-by: Marcin Wojtas <mw@semihalf.com> > > --- > > -struct net_device *of_find_net_device_by_node(struct device_node *np) > > +struct net_device *fwnode_find_net_device_by_node(struct fwnode_handle *fwnode) > > { > > struct device *dev; > > > > - dev = class_find_device(&net_class, NULL, np, of_dev_node_match); > > + dev = class_find_device(&net_class, NULL, fwnode, fwnode_find_parent_dev_match); > > This needs to maintain compatibility with DSA masters that have > dev->of_node but don't have dev->fwnode populated. > Do you mean a situation analogous to what I addressed in: [net-next: PATCH v3 4/8] net: mvpp2: initialize port fwnode pointer ? I found indeed a couple of drivers that may require a similar change (e.g. dpaa2). IMO we have 2 options: - update these drivers - add some kind of fallback? If yes, I am wondering about an elegant solution - maybe add an extra check inside fwnode_find_parent_dev_match? What would you suggest? Best regards, Marcin > > if (!dev) > > return NULL; > > > > return to_net_dev(dev); > > } > > -EXPORT_SYMBOL(of_find_net_device_by_node); > > -#endif > > +EXPORT_SYMBOL(fwnode_find_net_device_by_node);
On Wed, Jul 27, 2022 at 05:18:16PM +0200, Marcin Wojtas wrote: > Do you mean a situation analogous to what I addressed in: > [net-next: PATCH v3 4/8] net: mvpp2: initialize port fwnode pointer > ? Not sure if "analogous" is the right word. My estimation is that the overwhelmingly vast majority of DSA masters can be found by DSA simply due to the SET_NETDEV_DEV() call that the Ethernet drivers need to make anyway. I see that mvpp2 also needed commit c4053ef32208 ("net: mvpp2: initialize port of_node pointer"), but that isn't needed in general, and I can't tell you exactly why it is needed there, I don't know enough about the mvpp2 driver. > I found indeed a couple of drivers that may require a similar change > (e.g. dpaa2). There I can tell you why the dpaa2-mac code mangles with net_dev->dev.of_node, but I'd rather not go into an explanation that essentially doesn't matter. The point is that you'd be mistaken to think that only the drivers which touch the net device's ->dev->of_node are the ones that need updating for your series to not cause regressions. > IMO we have 2 options: > - update these drivers > - add some kind of fallback? If yes, I am wondering about an elegant > solution - maybe add an extra check inside > fwnode_find_parent_dev_match? > > What would you suggest? Fixing fwnode_find_parent_dev_match(), of course. This change broke DSA on my LS1028A system (master in drivers/net/ethernet/freescale/enetc/) and LS1021A (master in drivers/net/ethernet/freescale/gianfar.c).
On Wed, Jul 27, 2022 at 5:24 PM Marcin Wojtas <mw@semihalf.com> wrote: > śr., 27 lip 2022 o 16:31 Vladimir Oltean <olteanv@gmail.com> napisał(a): > > On Wed, Jul 27, 2022 at 08:43:19AM +0200, Marcin Wojtas wrote: ... > > > + dev = class_find_device(&net_class, NULL, fwnode, fwnode_find_parent_dev_match); > > > > This needs to maintain compatibility with DSA masters that have > > dev->of_node but don't have dev->fwnode populated. > > Do you mean a situation analogous to what I addressed in: > [net-next: PATCH v3 4/8] net: mvpp2: initialize port fwnode pointer > ? > > I found indeed a couple of drivers that may require a similar change > (e.g. dpaa2). > > IMO we have 2 options: > - update these drivers Not Vladimir here, but my 2cents that update is best and elegant, it can be done even before this series.
śr., 27 lip 2022 o 18:38 Vladimir Oltean <olteanv@gmail.com> napisał(a): > > On Wed, Jul 27, 2022 at 05:18:16PM +0200, Marcin Wojtas wrote: > > Do you mean a situation analogous to what I addressed in: > > [net-next: PATCH v3 4/8] net: mvpp2: initialize port fwnode pointer > > ? > > Not sure if "analogous" is the right word. My estimation is that the > overwhelmingly vast majority of DSA masters can be found by DSA simply > due to the SET_NETDEV_DEV() call that the Ethernet drivers need to make > anyway. I see that mvpp2 also needed commit c4053ef32208 ("net: mvpp2: > initialize port of_node pointer"), but that isn't needed in general, and > I can't tell you exactly why it is needed there, I don't know enough > about the mvpp2 driver. SET_NETDEV_DEV() fills net_device->dev.parent with &pdev->dev and in most cases it is sufficient apparently it is sufficient for fwnode_find_parent_dev_match (at least tests with mvneta case proves it's fine). We have some corner cases though: * mvpp2 -> single controller can handle up to 3 net_devices and therefore we need device_set_node() to make this work. I think dpaa2 is a similar case * PCIE drivers with extra DT description (I think that's the case of enetc). > > > I found indeed a couple of drivers that may require a similar change > > (e.g. dpaa2). > > There I can tell you why the dpaa2-mac code mangles with net_dev->dev.of_node, > but I'd rather not go into an explanation that essentially doesn't matter. > The point is that you'd be mistaken to think that only the drivers which > touch the net device's ->dev->of_node are the ones that need updating > for your series to not cause regressions. As above - SET_NETDEV_DEV() should be fine in most cases, but we can never be 100% sure untils it's verified. > > > IMO we have 2 options: > > - update these drivers > > - add some kind of fallback? If yes, I am wondering about an elegant > > solution - maybe add an extra check inside > > fwnode_find_parent_dev_match? > > > > What would you suggest? > > Fixing fwnode_find_parent_dev_match(), of course. This change broke DSA > on my LS1028A system (master in drivers/net/ethernet/freescale/enetc/) > and LS1021A (master in drivers/net/ethernet/freescale/gianfar.c). Can you please check applying following diff: --- a/drivers/base/property.c +++ b/drivers/base/property.c @@ -695,20 +695,22 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent); * The routine can be used e.g. as a callback for class_find_device(). * * Returns: %1 - match is found * %0 - match not found */ int fwnode_find_parent_dev_match(struct device *dev, const void *data) { for (; dev; dev = dev->parent) { if (device_match_fwnode(dev, data)) return 1; + else if (device_match_of_node(dev, to_of_node(data)) + return 1; } return 0; } EXPORT_SYMBOL_GPL(fwnode_find_parent_dev_match); Thanks for the review and test. Marcin
śr., 27 lip 2022 o 19:00 Andy Shevchenko <andy.shevchenko@gmail.com> napisał(a): > > On Wed, Jul 27, 2022 at 5:24 PM Marcin Wojtas <mw@semihalf.com> wrote: > > śr., 27 lip 2022 o 16:31 Vladimir Oltean <olteanv@gmail.com> napisał(a): > > > On Wed, Jul 27, 2022 at 08:43:19AM +0200, Marcin Wojtas wrote: > > ... > > > > > + dev = class_find_device(&net_class, NULL, fwnode, fwnode_find_parent_dev_match); > > > > > > This needs to maintain compatibility with DSA masters that have > > > dev->of_node but don't have dev->fwnode populated. > > > > Do you mean a situation analogous to what I addressed in: > > [net-next: PATCH v3 4/8] net: mvpp2: initialize port fwnode pointer > > ? > > > > I found indeed a couple of drivers that may require a similar change > > (e.g. dpaa2). > > > > IMO we have 2 options: > > - update these drivers > > Not Vladimir here, but my 2cents that update is best and elegant, it > can be done even before this series. > In general I agree it's desired, but I'm not sure if we can catch all cases just by reading code or rather base on regression reports later... Best regards, Marcin
On Wed, Jul 27, 2022 at 07:40:00PM +0200, Marcin Wojtas wrote: > SET_NETDEV_DEV() fills net_device->dev.parent with &pdev->dev > and in most cases it is sufficient apparently it is sufficient for > fwnode_find_parent_dev_match (at least tests with mvneta case proves > it's fine). Indeed, mvneta works, which is a plain old platform device that hasn't even been converted to fwnode, so why don't the others? Well, as it turns out, it's one of the cases where I spoke to soon, thinking I knew what was the problem why probing failed, before actually debugging. I thought there was no dmesg output from DSA at all, which would have indicated an eternal -EPROBE_DEFER situation. But there's one tiny line I had overlooked: [ 5.094013] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch This comes from here: static int dsa_port_parse_fw(struct dsa_port *dp, struct fwnode_handle *fwnode) { struct fwnode_handle *ethernet = fwnode_find_reference(fwnode, "ethernet", 0); bool link = fwnode_property_present(fwnode, "link"); const char *name = NULL; int ret; ret = fwnode_property_read_string(fwnode, "label", &name); // if (ret) // return ret; dp->fwnode = fwnode; The 'label' property of a port was optional, you've made it mandatory by accident. It is used only by DSA drivers that register using platform data. (side note, I can't believe you actually have a 'label' property for the CPU port and how many people are in the same situation as you; you know it isn't used for anything, right? how do we stop the cargo cult?) > Can you please check applying following diff: > > --- a/drivers/base/property.c > +++ b/drivers/base/property.c > @@ -695,20 +695,22 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent); > * The routine can be used e.g. as a callback for class_find_device(). > * > * Returns: %1 - match is found > * %0 - match not found > */ > int fwnode_find_parent_dev_match(struct device *dev, const void *data) > { > for (; dev; dev = dev->parent) { > if (device_match_fwnode(dev, data)) > return 1; > + else if (device_match_of_node(dev, to_of_node(data)) > + return 1; > } > > return 0; > } > EXPORT_SYMBOL_GPL(fwnode_find_parent_dev_match); So, nothing to do with device_match_fwnode() failing, that would have been strange, come to think about it. Sorry for the noise here. But looking at the deeper implementation of dev_fwnode() as: struct fwnode_handle *dev_fwnode(struct device *dev) { return IS_ENABLED(CONFIG_OF) && dev->of_node ? of_fwnode_handle(dev->of_node) : dev->fwnode; } I wonder, why did you have to modify mvpp2? It looks at dev->of_node prior to returning dev->fwnode. It should work.
On Thu, Jul 28, 2022 at 12:11:12AM +0300, Vladimir Oltean wrote: > The 'label' property of a port was optional, you've made it mandatory by accident. > It is used only by DSA drivers that register using platform data. I didn't mean to say exactly this, the second phrase was an addition through which I tried to make it clear that the "cpu" label *is* used, but only by the drivers using platform data. With OF it's not, neither that nor label = "dsa". We have other means of recognizing a CPU or DSA port, by the 'ethernet' and/or 'dsa' phandles. Additionally, the label is optional even for user port. One can have udev rules that assign names to Ethernet ports. I think that is even encouraged; some of the things in DSA predate the establishment of some best practices.
śr., 27 lip 2022 o 23:11 Vladimir Oltean <olteanv@gmail.com> napisał(a): > > On Wed, Jul 27, 2022 at 07:40:00PM +0200, Marcin Wojtas wrote: > > SET_NETDEV_DEV() fills net_device->dev.parent with &pdev->dev > > and in most cases it is sufficient apparently it is sufficient for > > fwnode_find_parent_dev_match (at least tests with mvneta case proves > > it's fine). > > Indeed, mvneta works, which is a plain old platform device that hasn't > even been converted to fwnode, so why don't the others? > > Well, as it turns out, it's one of the cases where I spoke to soon, > thinking I knew what was the problem why probing failed, before actually > debugging. > > I thought there was no dmesg output from DSA at all, which would have > indicated an eternal -EPROBE_DEFER situation. But there's one tiny line > I had overlooked: > > [ 5.094013] mscc_felix 0000:00:00.5: error -EINVAL: Failed to register DSA switch > > This comes from here: > > static int dsa_port_parse_fw(struct dsa_port *dp, struct fwnode_handle *fwnode) > { > struct fwnode_handle *ethernet = fwnode_find_reference(fwnode, "ethernet", 0); > bool link = fwnode_property_present(fwnode, "link"); > const char *name = NULL; > int ret; > > ret = fwnode_property_read_string(fwnode, "label", &name); > // if (ret) > // return ret; > > dp->fwnode = fwnode; > > The 'label' property of a port was optional, you've made it mandatory by accident. > It is used only by DSA drivers that register using platform data. Thanks for spotting that, I will make it optional again. > > (side note, I can't believe you actually have a 'label' property for the > CPU port and how many people are in the same situation as you; you know > it isn't used for anything, right? how do we stop the cargo cult?) > Well, given these results: ~/git/linux : git grep 'label = \"cpu\"' arch/arm/boot/dts/ | wc -l 79 ~/git/linux : git grep 'label = \"cpu\"' arch/arm64/boot/dts/ | wc -l 14 It's not a surprise I also have it defined in the platforms I test. I agree it doesn't serve any purpose in terms of creating the devices in DSA with DT, but it IMO increases readability of the description at least. > > Can you please check applying following diff: > > > > --- a/drivers/base/property.c > > +++ b/drivers/base/property.c > > @@ -695,20 +695,22 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent); > > * The routine can be used e.g. as a callback for class_find_device(). > > * > > * Returns: %1 - match is found > > * %0 - match not found > > */ > > int fwnode_find_parent_dev_match(struct device *dev, const void *data) > > { > > for (; dev; dev = dev->parent) { > > if (device_match_fwnode(dev, data)) > > return 1; > > + else if (device_match_of_node(dev, to_of_node(data)) > > + return 1; > > } > > > > return 0; > > } > > EXPORT_SYMBOL_GPL(fwnode_find_parent_dev_match); > > So, nothing to do with device_match_fwnode() failing, that would have > been strange, come to think about it. Sorry for the noise here. > Great, thank you for confirmation. > But looking at the deeper implementation of dev_fwnode() as: > > struct fwnode_handle *dev_fwnode(struct device *dev) > { > return IS_ENABLED(CONFIG_OF) && dev->of_node ? > of_fwnode_handle(dev->of_node) : dev->fwnode; > } > > I wonder, why did you have to modify mvpp2? It looks at dev->of_node > prior to returning dev->fwnode. It should work. When I boot with ACPI, the dev->of_node becomes NULL. With DT it's fine as-is. Best regards, Marcin
śr., 27 lip 2022 o 23:15 Andy Shevchenko <andy.shevchenko@gmail.com> napisał(a): > > > > On Wednesday, July 27, 2022, Marcin Wojtas <mw@semihalf.com> wrote: >> >> śr., 27 lip 2022 o 18:38 Vladimir Oltean <olteanv@gmail.com> napisał(a): >> > >> > On Wed, Jul 27, 2022 at 05:18:16PM +0200, Marcin Wojtas wrote: >> > > Do you mean a situation analogous to what I addressed in: >> > > [net-next: PATCH v3 4/8] net: mvpp2: initialize port fwnode pointer >> > > ? >> > >> > Not sure if "analogous" is the right word. My estimation is that the >> > overwhelmingly vast majority of DSA masters can be found by DSA simply >> > due to the SET_NETDEV_DEV() call that the Ethernet drivers need to make >> > anyway. I see that mvpp2 also needed commit c4053ef32208 ("net: mvpp2: >> > initialize port of_node pointer"), but that isn't needed in general, and >> > I can't tell you exactly why it is needed there, I don't know enough >> > about the mvpp2 driver. >> >> SET_NETDEV_DEV() fills net_device->dev.parent with &pdev->dev >> and in most cases it is sufficient apparently it is sufficient for >> fwnode_find_parent_dev_match (at least tests with mvneta case proves >> it's fine). >> >> We have some corner cases though: >> * mvpp2 -> single controller can handle up to 3 net_devices and >> therefore we need device_set_node() to make this work. I think dpaa2 >> is a similar case >> * PCIE drivers with extra DT description (I think that's the case of enetc). >> >> > >> > > I found indeed a couple of drivers that may require a similar change >> > > (e.g. dpaa2). >> > >> > There I can tell you why the dpaa2-mac code mangles with net_dev->dev.of_node, >> > but I'd rather not go into an explanation that essentially doesn't matter. >> > The point is that you'd be mistaken to think that only the drivers which >> > touch the net device's ->dev->of_node are the ones that need updating >> > for your series to not cause regressions. >> >> As above - SET_NETDEV_DEV() should be fine in most cases, but we can >> never be 100% sure untils it's verified. >> >> > >> > > IMO we have 2 options: >> > > - update these drivers >> > > - add some kind of fallback? If yes, I am wondering about an elegant >> > > solution - maybe add an extra check inside >> > > fwnode_find_parent_dev_match? >> > > >> > > What would you suggest? >> > >> > Fixing fwnode_find_parent_dev_match(), of course. > > > > Fixing how?! > > >> >> >> This change broke DSA >> > on my LS1028A system (master in drivers/net/ethernet/freescale/enetc/) >> > and LS1021A (master in drivers/net/ethernet/freescale/gianfar.c). >> >> Can you please check applying following diff: >> >> --- a/drivers/base/property.c >> +++ b/drivers/base/property.c >> @@ -695,20 +695,22 @@ EXPORT_SYMBOL_GPL(fwnode_get_nth_parent); >> * The routine can be used e.g. as a callback for class_find_device(). >> * >> * Returns: %1 - match is found >> * %0 - match not found >> */ >> int fwnode_find_parent_dev_match(struct device *dev, const void *data) >> { >> for (; dev; dev = dev->parent) { >> if (device_match_fwnode(dev, data)) >> return 1; >> + else if (device_match_of_node(dev, to_of_node(data)) >> + return 1; >> } >> > > This adds a piece of dead code. device_match_fwnode() covers this already. > Yes, indeed. After recent update, I think we can assume the current implementation of fwnode_find_parent_dev_match should work fine with all existing cases. Thank you for all remarks and comments, I'll address them in v4 later today. Best regards, Marcin
On Thu, Jul 28, 2022 at 08:52:04AM +0200, Marcin Wojtas wrote: > Yes, indeed. After recent update, I think we can assume the current > implementation of fwnode_find_parent_dev_match should work fine with > all existing cases. What you should really be fixing is the commit message of patch 4, that's what threw me off: | As a preparation to switch the DSA subsystem from using | of_find_net_device_by_node() to its more generic fwnode_ | equivalent, the port's device structure should be updated | with its fwnode pointer, similarly to of_node - see analogous | commit c4053ef32208 ("net: mvpp2: initialize port of_node pointer"). | | This patch is required to prevent a regression before updating | the DSA API on boards that connect the mvpp2 port to switch, | such as Clearfog GT-8K or CN913x CEx7 Evaluation Board. There's no regression to speak of. DSA didn't work with ACPI before, and fwnode_find_net_device_by_node() still works with the plain dev->of_node assignment.
czw., 28 lip 2022 o 11:16 Vladimir Oltean <olteanv@gmail.com> napisał(a): > > On Thu, Jul 28, 2022 at 08:52:04AM +0200, Marcin Wojtas wrote: > > Yes, indeed. After recent update, I think we can assume the current > > implementation of fwnode_find_parent_dev_match should work fine with > > all existing cases. > > What you should really be fixing is the commit message of patch 4, > that's what threw me off: > > | As a preparation to switch the DSA subsystem from using > | of_find_net_device_by_node() to its more generic fwnode_ > | equivalent, the port's device structure should be updated > | with its fwnode pointer, similarly to of_node - see analogous > | commit c4053ef32208 ("net: mvpp2: initialize port of_node pointer"). > | > | This patch is required to prevent a regression before updating > | the DSA API on boards that connect the mvpp2 port to switch, > | such as Clearfog GT-8K or CN913x CEx7 Evaluation Board. > > There's no regression to speak of. DSA didn't work with ACPI before, and > fwnode_find_net_device_by_node() still works with the plain dev->of_node > assignment. There was a regression even for OF in v1, but after switching to device_match_fwnode() it works indeed. Anyway patch v4 is imo useful, I'll only reword the commit message. Thanks, Marcin
On Thu, Jul 28, 2022 at 06:56:48PM +0200, Marcin Wojtas wrote: > There was a regression even for OF in v1, but after switching to > device_match_fwnode() it works indeed. Anyway patch v4 is imo useful, > I'll only reword the commit message. Do you mean patch 4 or patch v4? If patch 4, of course it's useful, but not for avoiding a regression with OF (case in which I drop all my claims made earlier about fw_find_net_device_by_node), but rather to actually get something working with actual ACPI (although perhaps not in this series, you'll need to add ACPI IDs in the mv88e6xxx driver some time later as well, maybe you could focus this series just on converting DSA to play nice with fwnodes). If you're already thinking about the v4 of this patch set, I'll respond to that in a separate email shortly.
czw., 28 lip 2022 o 21:16 Vladimir Oltean <olteanv@gmail.com> napisał(a): > > On Thu, Jul 28, 2022 at 06:56:48PM +0200, Marcin Wojtas wrote: > > There was a regression even for OF in v1, but after switching to > > device_match_fwnode() it works indeed. Anyway patch v4 is imo useful, > > I'll only reword the commit message. > > Do you mean patch 4 or patch v4? If patch 4, of course it's useful, but Patch 4/8 in v4 :) I'm working on it right now to submit asap. > not for avoiding a regression with OF (case in which I drop all my claims > made earlier about fw_find_net_device_by_node), but rather to actually Change in the mvpp2 driver: - dev->dev.of_node = port_node; + device_set_node(&dev->dev, port_fwnode); is desired and correct anyway, so as a low-cost change I think it can be included in this series (which is in fact preparation-to-ACPI support). I will update the commit message. accordingly. > get something working with actual ACPI (although perhaps not in this > series, you'll need to add ACPI IDs in the mv88e6xxx driver some time v1 added all of this, but we agreed that ACPI-specific bits should be sent separately later, after extending the ACPI Specification. > later as well, maybe you could focus this series just on converting DSA > to play nice with fwnodes). If you're already thinking about the v4 of > this patch set, I'll respond to that in a separate email shortly.
On Thu, Jul 28, 2022 at 08:47:58AM +0200, Marcin Wojtas wrote: > > The 'label' property of a port was optional, you've made it mandatory by accident. > > It is used only by DSA drivers that register using platform data. > > Thanks for spotting that, I will make it optional again. > > > (side note, I can't believe you actually have a 'label' property for the > > CPU port and how many people are in the same situation as you; you know > > it isn't used for anything, right? how do we stop the cargo cult?) > > Well, given these results: > ~/git/linux : git grep 'label = \"cpu\"' arch/arm/boot/dts/ | wc -l > 79 > ~/git/linux : git grep 'label = \"cpu\"' arch/arm64/boot/dts/ | wc -l > 14 > > It's not a surprise I also have it defined in the platforms I test. I > agree it doesn't serve any purpose in terms of creating the devices in > DSA with DT, but it IMO increases readability of the description at > least. We've glided over this way too easily, so I'll repeat this thing I've said: | One can have udev rules that assign names to Ethernet ports. I think | that is even encouraged; some of the things in DSA predate the | establishment of some best practices. I know I'm not exactly "upfront" by saying this at v3 rather than earlier, but I haven't had the time and I still don't have as much as I'd like. Sorry for that. Please don't jump to sending v4 just yet, and please don't expect that this patch set will make it for the upcoming 5.20 release candidates. ACPI is a whole new world and I don't think we want to mass-migrate each and every OF binding that DSA has to the generic fwnode form, at least not without having a serious discussion about it. The 'label' thing is actually one of the things that I'm seriously considering skipping parsing if this is an ACPI system, simply because best practices are different today than they were when the OF bindings were created. There's also the change that validates that phylink has the fwnode properties it needs to work properly: https://patchwork.kernel.org/project/netdevbpf/patch/20220723164635.1621911-1-vladimir.oltean@nxp.com/ Please don't even think that the DSA fwnode conversion will be merged before the validation patch (sorry, I'm not saying this to block you, I'm saying this because I don't want DSA to start with zero-day baggage on ACPI). And even when the validation patch gets merged, you'll need to adapt it to fwnode because that's what will be required syntactically, but we'll only go through the motions of calling of_device_compatible_match() for the OF case. With ACPI, every driver will opt into strict validation, that's non negotiable. And then there are some other issues we've learned about, with the DT bindings that specific drivers such as mv88e6xxx and realtek-smi have. I'll give you more details once we get to the actual mv88e6xxx conversion to ACPI; currently my memory lacks some of the precise details of how come mv88e6xxx came to not observe the issue but realtek-smi did. Anyway, the issue was that fw_devlink causes the internal PHYs to probe with the generic rather than the specific PHY driver, if interrupts are being used (and provided by the switch as 'interrupt-controller'). It can be debated what exactly is at fault there, although one interpretation can be that the DT bindings themselves are to blame, for describing a circular dependency between a parent and a child. I've been suggesting the authors of new drivers to take an alternative approach and describe the switch chip as a MFD, with only the actual switching component being probed by DSA and the rest being separate drivers: https://lore.kernel.org/all/20211204022037.dkipkk42qet4u7go@skbuf/T/ You'll say, ok but don't we have to keep maintaining mv8e6xxx OF bindings functional with fw_devlink too anyway, so what benefit would there be if for ACPI we'd split the exact same monolithic driver into a MFD? And maybe you have a point, I don't know, I haven't actually tried to look at the code and see if it could be restructured cleanly to probe and work in both cases. The bottom line is that if you haven't received too much review for your series until now, I suspect it's because none of the DSA maintainers cropped a large enough chunk of time yet to actually clarify things for themselves. I don't consider the things I've pointed out here to be a 'review' in the proper sense, they're just cases I'm thinking about in the back of my mind where we should learn from past mistakes. I'll revisit when I will have come to some conclusions.
> The 'label' thing is actually one of the things that I'm seriously > considering skipping parsing if this is an ACPI system, simply because > best practices are different today than they were when the OF bindings > were created. Agreed. We want the ACPI binding to learn from what has worked and not worked in DT. We should clean up some of the historical mess. And enforce things we don't in DT simply because there is too much history. So a straight one to one conversion is not going to happen. > It can be debated what exactly is at fault there, although one > interpretation can be that the DT bindings themselves are to blame, > for describing a circular dependency between a parent and a child. DT describes hardware. I'm not sure hardware can have a circular dependency. It is more about how software make use of that hardware description that ends up in circular dependencies. Andrew
czw., 28 lip 2022 o 22:18 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > The 'label' thing is actually one of the things that I'm seriously > > considering skipping parsing if this is an ACPI system, simply because > > best practices are different today than they were when the OF bindings > > were created. > > Agreed. We want the ACPI binding to learn from what has worked and not > worked in DT. We should clean up some of the historical mess. And > enforce things we don't in DT simply because there is too much > history. > > So a straight one to one conversion is not going to happen. I understand your standpoint - there is a long history, possible clean-ups, backward compatibility considerations, etc. that should not be zero-day baggage of ACPI. Otoh, we don't need to be worried about the ACPI binding too much now - as agreed it was removed from this series, beginning from v2. IMO it may be better to return to that once the ACPI Spec is updated with the MDIOSerialBus and the patches are resubmitted on whatever shape of the DSA subsystem is established within the next weeks/months from now. In v1 we discussed also the resubmission of the non-ACPI-related patches, which would pave the way to dropping the explicit OF_ dependency in the DSA and moving to a generic hardware description kernel API - without any functional change. Modifying DT bindings and clean-ups could be done on top this patchset as well. Of course, it is the subsystems' Maintainers call and I'll adjust accordingly - if you wish me to wait and rebase after the 'validation patch' lands in net-next, I'll do that. A side note: I was of course aware that making it for the v5.20 would be extremely hard, but I decided to give it a try anyway - I had to wait for some time, as this series was gated by fate of the eventually abandoned phylink-related changes. Thanks, Marcin
On Thu, Jul 28, 2022 at 11:23:31PM +0200, Marcin Wojtas wrote: > czw., 28 lip 2022 o 22:18 Andrew Lunn <andrew@lunn.ch> napisał(a): > > > > > The 'label' thing is actually one of the things that I'm seriously > > > considering skipping parsing if this is an ACPI system, simply because > > > best practices are different today than they were when the OF bindings > > > were created. > > > > Agreed. We want the ACPI binding to learn from what has worked and not > > worked in DT. We should clean up some of the historical mess. And > > enforce things we don't in DT simply because there is too much > > history. > > > > So a straight one to one conversion is not going to happen. > > I understand your standpoint - there is a long history, possible > clean-ups, backward compatibility considerations, etc. that should not > be zero-day baggage of ACPI. Otoh, we don't need to be worried about > the ACPI binding too much now - as agreed it was removed from this > series, beginning from v2. IMO it may be better to return to that once > the ACPI Spec is updated with the MDIOSerialBus and the patches are > resubmitted on whatever shape of the DSA subsystem is established > within the next weeks/months from now. > > In v1 we discussed also the resubmission of the non-ACPI-related > patches, which would pave the way to dropping the explicit OF_ > dependency in the DSA and moving to a generic hardware description > kernel API - without any functional change. Ideally, we want to keep all the ugly DT stuff in DT. We want to ensure that any "generic hardware description kernel API" does not inherit all the ugly DT stuff. ACPI and DT are different things, so i don't see why they need to share code. Andrew
On Fri, Jul 29, 2022 at 12:29 AM Andrew Lunn <andrew@lunn.ch> wrote: > On Thu, Jul 28, 2022 at 11:23:31PM +0200, Marcin Wojtas wrote: ... > ACPI and DT are different things, so i don't see why they need to > share code. Yes and no. ACPI _DSD extension allows us to share a lot of code when it comes to specific device properties (that are not standardized anyhow by ACPI specification, and for the record many of them even may not, but some are, like MIPI camera). Maybe you want to have something like a property standard for DSA that can be published as MIPI or so and then that part of the code of course may very well be shared. Discussed MDIOSerialBus() resource type is pure ACPI thingy if going to be standardized and indeed, that part can't be shared. Entire exercise with fwnodes allows to make drivers (most of or most of the parts of) to be shared between different resource providers. And this is a cool part about it.
diff --git a/include/linux/etherdevice.h b/include/linux/etherdevice.h index 92b10e67d5f8..a335775af244 100644 --- a/include/linux/etherdevice.h +++ b/include/linux/etherdevice.h @@ -35,6 +35,7 @@ int nvmem_get_mac_address(struct device *dev, void *addrbuf); int device_get_mac_address(struct device *dev, char *addr); int device_get_ethdev_address(struct device *dev, struct net_device *netdev); int fwnode_get_mac_address(struct fwnode_handle *fwnode, char *addr); +struct net_device *fwnode_find_net_device_by_node(struct fwnode_handle *fwnode); u32 eth_get_headlen(const struct net_device *dev, const void *data, u32 len); __be16 eth_type_trans(struct sk_buff *skb, struct net_device *dev); diff --git a/include/linux/of_net.h b/include/linux/of_net.h index 0484b613ca64..f672f831292d 100644 --- a/include/linux/of_net.h +++ b/include/linux/of_net.h @@ -15,7 +15,6 @@ struct net_device; extern int of_get_phy_mode(struct device_node *np, phy_interface_t *interface); extern int of_get_mac_address(struct device_node *np, u8 *mac); int of_get_ethdev_address(struct device_node *np, struct net_device *dev); -extern struct net_device *of_find_net_device_by_node(struct device_node *np); #else static inline int of_get_phy_mode(struct device_node *np, phy_interface_t *interface) @@ -32,11 +31,6 @@ static inline int of_get_ethdev_address(struct device_node *np, struct net_devic { return -ENODEV; } - -static inline struct net_device *of_find_net_device_by_node(struct device_node *np) -{ - return NULL; -} #endif #endif /* __LINUX_OF_NET_H */ diff --git a/net/core/net-sysfs.c b/net/core/net-sysfs.c index d61afd21aab5..fc972545aaea 100644 --- a/net/core/net-sysfs.c +++ b/net/core/net-sysfs.c @@ -6,6 +6,7 @@ */ #include <linux/capability.h> +#include <linux/etherdevice.h> #include <linux/kernel.h> #include <linux/netdevice.h> #include <linux/if_arp.h> @@ -1935,38 +1936,26 @@ static struct class net_class __ro_after_init = { .get_ownership = net_get_ownership, }; -#ifdef CONFIG_OF -static int of_dev_node_match(struct device *dev, const void *data) -{ - for (; dev; dev = dev->parent) { - if (dev->of_node == data) - return 1; - } - - return 0; -} - /* - * of_find_net_device_by_node - lookup the net device for the device node - * @np: OF device node + * fwnode_find_net_device_by_node - lookup the net device for the device fwnode + * @fwnode: firmware node * - * Looks up the net_device structure corresponding with the device node. + * Looks up the net_device structure corresponding with the fwnode. * If successful, returns a pointer to the net_device with the embedded * struct device refcount incremented by one, or NULL on failure. The * refcount must be dropped when done with the net_device. */ -struct net_device *of_find_net_device_by_node(struct device_node *np) +struct net_device *fwnode_find_net_device_by_node(struct fwnode_handle *fwnode) { struct device *dev; - dev = class_find_device(&net_class, NULL, np, of_dev_node_match); + dev = class_find_device(&net_class, NULL, fwnode, fwnode_find_parent_dev_match); if (!dev) return NULL; return to_net_dev(dev); } -EXPORT_SYMBOL(of_find_net_device_by_node); -#endif +EXPORT_SYMBOL(fwnode_find_net_device_by_node); /* Delete sysfs entries but hold kobject reference until after all * netdev references are gone. diff --git a/net/dsa/dsa2.c b/net/dsa/dsa2.c index 82fb3b009fb4..bba416eba9c2 100644 --- a/net/dsa/dsa2.c +++ b/net/dsa/dsa2.c @@ -7,6 +7,7 @@ */ #include <linux/device.h> +#include <linux/etherdevice.h> #include <linux/err.h> #include <linux/list.h> #include <linux/netdevice.h> @@ -1498,7 +1499,7 @@ static int dsa_port_parse_fw(struct dsa_port *dp, struct fwnode_handle *fwnode) struct net_device *master; const char *user_protocol; - master = of_find_net_device_by_node(to_of_node(ethernet)); + master = fwnode_find_net_device_by_node(ethernet); fwnode_handle_put(ethernet); if (!master) return -EPROBE_DEFER;
A helper function which allows getting the struct net_device pointer associated with a given device tree node can be more generic and also support alternative hardware description. Switch to fwnode_ and update the only existing caller in DSA subsystem. For that purpose use newly added fwnode_dev_node_match helper routine. Signed-off-by: Marcin Wojtas <mw@semihalf.com> --- include/linux/etherdevice.h | 1 + include/linux/of_net.h | 6 ----- net/core/net-sysfs.c | 25 ++++++-------------- net/dsa/dsa2.c | 3 ++- 4 files changed, 10 insertions(+), 25 deletions(-)