Merge branch 'dsa_to_port-loops'
Vladimir Oltean says: ==================== Remove the "dsa_to_port in a loop" antipattern v1->v2: more patches v2->v3: less patches As opposed to previous series, I would now like to first refactor the DSA core, since that sees fewer patches than drivers, and make the helpers available. Since the refactoring is fairly noisy, I don't want to force it on driver maintainers right away, patches can be submitted independently. The original cover letter is below: The DSA core and drivers currently iterate too much through the port list of a switch. For example, this snippet: for (port = 0; port < ds->num_ports; port++) { if (!dsa_is_cpu_port(ds, port)) continue; ds->ops->change_tag_protocol(ds, port, tag_ops->proto); } iterates through ds->num_ports once, and then calls dsa_is_cpu_port to filter out the other types of ports. But that function has a hidden call to dsa_to_port() in it, which contains: list_for_each_entry(dp, &dst->ports, list) if (dp->ds == ds && dp->index == p) return dp; where the only thing we wanted to know in the first place was whether dp->type == DSA_PORT_TYPE_CPU or not. So it seems that the problem is that we are not iterating with the right variable. We have an "int port" but in fact need a "struct dsa_port *dp". This has started being an issue since this patch series: https://patchwork.ozlabs.org/project/netdev/cover/20191020031941.3805884-1-vivien.didelot@gmail.com/ The currently proposed set of changes iterates like this: dsa_switch_for_each_cpu_port(cpu_dp, ds) err = ds->ops->change_tag_protocol(ds, cpu_dp->index, tag_ops->proto); which iterates directly over ds->dst->ports, which is a list of struct dsa_port *dp. This makes it much easier and more efficient to check dp->type. As a nice side effect, with the proposed driver API, driver writers are now encouraged to use more efficient patterns, and not only due to less iterations through the port list. For example, something like this: for (port = 0; port < ds->num_ports; port++) do_something(); probably does not need to do_something() for the ports that are disabled in the device tree. But adding extra code for that would look like this: for (port = 0; port < ds->num_ports; port++) { if (!dsa_is_unused_port(ds, port)) continue; do_something(); } and therefore, it is understandable that some driver writers may decide to not bother. This patch series introduces a "dsa_switch_for_each_available_port" macro which comes at no extra cost in terms of lines of code / number of braces to the driver writer, but it has the "dsa_is_unused_port" check embedded within it. ==================== Signed-off-by: David S. Miller <davem@davemloft.net>
Showing
Please register or sign in to comment