| Message ID | 20220322014506.27872-2-ansuelsmth@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Reduce qca8k_priv space usage | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/subject_prefix | success | Link |
| netdev/cover_letter | success | Series has a cover letter |
| netdev/patch_count | success | Link |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/cc_maintainers | success | CCed 8 of 8 maintainers |
| netdev/build_clang | success | Errors and warnings before: 0 this patch: 0 |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 59 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, Mar 22, 2022 at 02:45:03AM +0100, Ansuel Smith wrote: > Drop the MTU array from qca8k_priv and use slave net dev to get the max > MTU across all user port. CPU port can be skipped as DSA already make > sure CPU port are set to the max MTU across all ports. > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > --- I hardly find this to be an improvement and I would rather not see such unjustified complexity in a device driver. What are the concrete benefits, size wise? > drivers/net/dsa/qca8k.c | 38 +++++++++++++++++++++++--------------- > drivers/net/dsa/qca8k.h | 1 - > 2 files changed, 23 insertions(+), 16 deletions(-) > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > index d3ed0a7f8077..4366d87b4bbd 100644 > --- a/drivers/net/dsa/qca8k.c > +++ b/drivers/net/dsa/qca8k.c > @@ -2367,13 +2367,31 @@ static int > qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu) > { > struct qca8k_priv *priv = ds->priv; > - int i, mtu = 0; > + struct dsa_port *dp; > + int mtu = new_mtu; > > - priv->port_mtu[port] = new_mtu; > + /* We have only have a general MTU setting. So check > + * every port and set the max across all port. > + */ > + list_for_each_entry(dp, &ds->dst->ports, list) { > + /* We can ignore cpu port, DSA will itself chose > + * the max MTU across all port > + */ > + if (!dsa_port_is_user(dp)) > + continue; > > - for (i = 0; i < QCA8K_NUM_PORTS; i++) > - if (priv->port_mtu[i] > mtu) > - mtu = priv->port_mtu[i]; > + if (dp->index == port) > + continue; > + > + /* Address init phase where not every port have > + * a slave device > + */ > + if (!dp->slave) > + continue; > + > + if (mtu < dp->slave->mtu) > + mtu = dp->slave->mtu; > + } > > /* Include L2 header / FCS length */ > return qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN); > @@ -3033,16 +3051,6 @@ qca8k_setup(struct dsa_switch *ds) > QCA8K_PORT_HOL_CTRL1_WRED_EN, > mask); > } > - > - /* Set initial MTU for every port. > - * We have only have a general MTU setting. So track > - * every port and set the max across all port. > - * Set per port MTU to 1500 as the MTU change function > - * will add the overhead and if its set to 1518 then it > - * will apply the overhead again and we will end up with > - * MTU of 1536 instead of 1518 > - */ > - priv->port_mtu[i] = ETH_DATA_LEN; > } > > /* Special GLOBAL_FC_THRESH value are needed for ar8327 switch */ > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h > index f375627174c8..562d75997e55 100644 > --- a/drivers/net/dsa/qca8k.h > +++ b/drivers/net/dsa/qca8k.h > @@ -398,7 +398,6 @@ struct qca8k_priv { > struct device *dev; > struct dsa_switch_ops ops; > struct gpio_desc *reset_gpio; > - unsigned int port_mtu[QCA8K_NUM_PORTS]; > struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */ > struct qca8k_mgmt_eth_data mgmt_eth_data; > struct qca8k_mib_eth_data mib_eth_data; > -- > 2.34.1 >
On Tue, Mar 22, 2022 at 01:58:12PM +0200, Vladimir Oltean wrote: > On Tue, Mar 22, 2022 at 02:45:03AM +0100, Ansuel Smith wrote: > > Drop the MTU array from qca8k_priv and use slave net dev to get the max > > MTU across all user port. CPU port can be skipped as DSA already make > > sure CPU port are set to the max MTU across all ports. > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > --- > > I hardly find this to be an improvement and I would rather not see such > unjustified complexity in a device driver. What are the concrete > benefits, size wise? > The main idea here is, if the value is already present and accessible, why should we duplicate it? Tracking the MTU in this custom way already caused some bugs (check the comment i'm removing). We both use standard way to track ports MTU and we save some additional space. At the cost of 2 additional checks are are not that much of a problem. Also from this I discovered that (at least on ipq806x that use stmmac) when master needs to change MTU, stmmac complains that the interface is up and it must be put down. Wonder if that's common across other drivers or it's only specific to stmmac. > > drivers/net/dsa/qca8k.c | 38 +++++++++++++++++++++++--------------- > > drivers/net/dsa/qca8k.h | 1 - > > 2 files changed, 23 insertions(+), 16 deletions(-) > > > > diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c > > index d3ed0a7f8077..4366d87b4bbd 100644 > > --- a/drivers/net/dsa/qca8k.c > > +++ b/drivers/net/dsa/qca8k.c > > @@ -2367,13 +2367,31 @@ static int > > qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu) > > { > > struct qca8k_priv *priv = ds->priv; > > - int i, mtu = 0; > > + struct dsa_port *dp; > > + int mtu = new_mtu; > > > > - priv->port_mtu[port] = new_mtu; > > + /* We have only have a general MTU setting. So check > > + * every port and set the max across all port. > > + */ > > + list_for_each_entry(dp, &ds->dst->ports, list) { > > + /* We can ignore cpu port, DSA will itself chose > > + * the max MTU across all port > > + */ > > + if (!dsa_port_is_user(dp)) > > + continue; > > > > - for (i = 0; i < QCA8K_NUM_PORTS; i++) > > - if (priv->port_mtu[i] > mtu) > > - mtu = priv->port_mtu[i]; > > + if (dp->index == port) > > + continue; > > + > > + /* Address init phase where not every port have > > + * a slave device > > + */ > > + if (!dp->slave) > > + continue; > > + > > + if (mtu < dp->slave->mtu) > > + mtu = dp->slave->mtu; > > + } > > > > /* Include L2 header / FCS length */ > > return qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN); > > @@ -3033,16 +3051,6 @@ qca8k_setup(struct dsa_switch *ds) > > QCA8K_PORT_HOL_CTRL1_WRED_EN, > > mask); > > } > > - > > - /* Set initial MTU for every port. > > - * We have only have a general MTU setting. So track > > - * every port and set the max across all port. > > - * Set per port MTU to 1500 as the MTU change function > > - * will add the overhead and if its set to 1518 then it > > - * will apply the overhead again and we will end up with > > - * MTU of 1536 instead of 1518 > > - */ > > - priv->port_mtu[i] = ETH_DATA_LEN; > > } > > > > /* Special GLOBAL_FC_THRESH value are needed for ar8327 switch */ > > diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h > > index f375627174c8..562d75997e55 100644 > > --- a/drivers/net/dsa/qca8k.h > > +++ b/drivers/net/dsa/qca8k.h > > @@ -398,7 +398,6 @@ struct qca8k_priv { > > struct device *dev; > > struct dsa_switch_ops ops; > > struct gpio_desc *reset_gpio; > > - unsigned int port_mtu[QCA8K_NUM_PORTS]; > > struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */ > > struct qca8k_mgmt_eth_data mgmt_eth_data; > > struct qca8k_mib_eth_data mib_eth_data; > > -- > > 2.34.1 > > >
On Tue, Mar 22, 2022 at 02:38:08PM +0100, Ansuel Smith wrote: > On Tue, Mar 22, 2022 at 01:58:12PM +0200, Vladimir Oltean wrote: > > On Tue, Mar 22, 2022 at 02:45:03AM +0100, Ansuel Smith wrote: > > > Drop the MTU array from qca8k_priv and use slave net dev to get the max > > > MTU across all user port. CPU port can be skipped as DSA already make > > > sure CPU port are set to the max MTU across all ports. > > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > --- > > > > I hardly find this to be an improvement and I would rather not see such > > unjustified complexity in a device driver. What are the concrete > > benefits, size wise? > > > > The main idea here is, if the value is already present and accessible, > why should we duplicate it? Tracking the MTU in this custom way already > caused some bugs (check the comment i'm removing). We both use standard > way to track ports MTU and we save some additional space. At the cost of > 2 additional checks are are not that much of a problem. Where is the bug? > Also from this I discovered that (at least on ipq806x that use stmmac) > when master needs to change MTU, stmmac complains that the interface is > up and it must be put down. Wonder if that's common across other drivers > or it's only specific to stmmac. I never had the pleasure of dealing with such DSA masters. I wonder why can't stmmac_change_mtu() check if netif_running(), call dev_close and set a bool, and at the end, if the bool was set, call dev_open back?
On Tue, Mar 22, 2022 at 03:55:35PM +0200, Vladimir Oltean wrote: > On Tue, Mar 22, 2022 at 02:38:08PM +0100, Ansuel Smith wrote: > > On Tue, Mar 22, 2022 at 01:58:12PM +0200, Vladimir Oltean wrote: > > > On Tue, Mar 22, 2022 at 02:45:03AM +0100, Ansuel Smith wrote: > > > > Drop the MTU array from qca8k_priv and use slave net dev to get the max > > > > MTU across all user port. CPU port can be skipped as DSA already make > > > > sure CPU port are set to the max MTU across all ports. > > > > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > > --- > > > > > > I hardly find this to be an improvement and I would rather not see such > > > unjustified complexity in a device driver. What are the concrete > > > benefits, size wise? > > > > > > > The main idea here is, if the value is already present and accessible, > > why should we duplicate it? Tracking the MTU in this custom way already > > caused some bugs (check the comment i'm removing). We both use standard > > way to track ports MTU and we save some additional space. At the cost of > > 2 additional checks are are not that much of a problem. > > Where is the bug? > There was a bug where we tracked the MTU with the FCS and L2 added and then in the change_mtu code we added another time the FCS and L2 header just because we used this custom way and nobody notice that we were adding 2 times the same headers. (it's now fixed but still it's a reason why using standard way to track MTU would have prevented that) > > Also from this I discovered that (at least on ipq806x that use stmmac) > > when master needs to change MTU, stmmac complains that the interface is > > up and it must be put down. Wonder if that's common across other drivers > > or it's only specific to stmmac. > > I never had the pleasure of dealing with such DSA masters. I wonder why > can't stmmac_change_mtu() check if netif_running(), call dev_close and > set a bool, and at the end, if the bool was set, call dev_open back? Oh ok so it's not standard that stmmac_change_mtu() just refuse to change the MTU instead of put the interface down, change MTU and reopen it... Fun stuff... From system side to change MTU to a new value (so lower MTU on any port or set MTU to a higher value for one pot) I have to: 1. ifconfig eth0 down 2. ifconfig lan1 mtu 1600 up 3. ifconfig eth up If I just ifconfig lan1 mtu 1600 up it's just rejected with stmmac complaining.
On Tue, Mar 22, 2022 at 03:03:36PM +0100, Ansuel Smith wrote: > On Tue, Mar 22, 2022 at 03:55:35PM +0200, Vladimir Oltean wrote: > > On Tue, Mar 22, 2022 at 02:38:08PM +0100, Ansuel Smith wrote: > > > On Tue, Mar 22, 2022 at 01:58:12PM +0200, Vladimir Oltean wrote: > > > > On Tue, Mar 22, 2022 at 02:45:03AM +0100, Ansuel Smith wrote: > > > > > Drop the MTU array from qca8k_priv and use slave net dev to get the max > > > > > MTU across all user port. CPU port can be skipped as DSA already make > > > > > sure CPU port are set to the max MTU across all ports. > > > > > > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > > > --- > > > > > > > > I hardly find this to be an improvement and I would rather not see such > > > > unjustified complexity in a device driver. What are the concrete > > > > benefits, size wise? > > > > > > > > > > The main idea here is, if the value is already present and accessible, > > > why should we duplicate it? Tracking the MTU in this custom way already > > > caused some bugs (check the comment i'm removing). We both use standard > > > way to track ports MTU and we save some additional space. At the cost of > > > 2 additional checks are are not that much of a problem. > > > > Where is the bug? > > There was a bug where we tracked the MTU with the FCS and L2 added and > then in the change_mtu code we added another time the FCS and L2 header > just because we used this custom way and nobody notice that we were adding > 2 times the same headers. (it's now fixed but still it's a reason why > using standard way to track MTU would have prevented that) No, I'm sorry, this is completely unjustified complexity - not to mention it's buggy, too. Does qca8k support cascaded setups? Because if it does: /* We have only have a general MTU setting. So check * every port and set the max across all port. */ list_for_each_entry(dp, &ds->dst->ports, list) { /* We can ignore cpu port, DSA will itself chose * the max MTU across all port */ if (!dsa_port_is_user(dp)) continue; if (dp->index == port) // <- this will exclude from the max MTU calculation the ports in other switches that are numerically equal to @port. continue; /* Address init phase where not every port have * a slave device */ if (!dp->slave) continue; if (mtu < dp->slave->mtu) mtu = dp->slave->mtu; } Not to mention it's missing the blatantly obvious. DSA calls ->port_change_mtu() on the CPU port with the max MTU, every time that changes. You need the max MTU. Why calculate it again? Why don't you do what mt7530 does, which has a similar restriction, and just program the hardware when the CPU port MTU is updated? You may think - does this work with multiple CPU ports? Well, yes it does, since DSA calculates the largest MTU across the entire tree, and not just across the user ports affine to a certain CPU port. If it wasn't for this possibility, I would have been in favor of introducing a dsa_tree_largest_mtu(dst) helper in the DSA core, but I can't find it justifiable. > > > Also from this I discovered that (at least on ipq806x that use stmmac) > > > when master needs to change MTU, stmmac complains that the interface is > > > up and it must be put down. Wonder if that's common across other drivers > > > or it's only specific to stmmac. > > > > I never had the pleasure of dealing with such DSA masters. I wonder why > > can't stmmac_change_mtu() check if netif_running(), call dev_close and > > set a bool, and at the end, if the bool was set, call dev_open back? > > Oh ok so it's not standard that stmmac_change_mtu() just refuse to > change the MTU instead of put the interface down, change MTU and reopen > it... Fun stuff... > > From system side to change MTU to a new value (so lower MTU on any port > or set MTU to a higher value for one pot) I have to: > 1. ifconfig eth0 down > 2. ifconfig lan1 mtu 1600 up > 3. ifconfig eth up > > If I just ifconfig lan1 mtu 1600 up it's just rejected with stmmac > complaining. Not sure if there is any hard line on this. But I made a poll, and the crushing majority of drivers in drivers/net/ethernet/ do not require !netif_running() in ndo_change_mtu. The ones that do are: nixge macb altera tse axienet renesas sh ksz884x bcm63xx_enet sundance stmmac (compared to more than 100 that don't, and even have a dedicated code path for live changes) By the way, an an interesting aside - I've found the xgene, atl1c and xgmac drivers to be obviously odd (meaning that more drivers might be odd in the same way, but in more subtle ways I haven't noticed): when netif_running() is false, they simply return 0, but they don't change dev->mtu either, they just ignore the request. So on one hand you have drivers that _want_ to be down to change the MTU, and on the other you have drivers that silently ignore MTU changes when they're down. Hard for DSA to do something reasonable to handle both cases...
On Thu, Mar 24, 2022 at 12:45:24PM +0200, Vladimir Oltean wrote: > On Tue, Mar 22, 2022 at 03:03:36PM +0100, Ansuel Smith wrote: > > On Tue, Mar 22, 2022 at 03:55:35PM +0200, Vladimir Oltean wrote: > > > On Tue, Mar 22, 2022 at 02:38:08PM +0100, Ansuel Smith wrote: > > > > On Tue, Mar 22, 2022 at 01:58:12PM +0200, Vladimir Oltean wrote: > > > > > On Tue, Mar 22, 2022 at 02:45:03AM +0100, Ansuel Smith wrote: > > > > > > Drop the MTU array from qca8k_priv and use slave net dev to get the max > > > > > > MTU across all user port. CPU port can be skipped as DSA already make > > > > > > sure CPU port are set to the max MTU across all ports. > > > > > > > > > > > > Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> > > > > > > --- > > > > > > > > > > I hardly find this to be an improvement and I would rather not see such > > > > > unjustified complexity in a device driver. What are the concrete > > > > > benefits, size wise? > > > > > > > > > > > > > The main idea here is, if the value is already present and accessible, > > > > why should we duplicate it? Tracking the MTU in this custom way already > > > > caused some bugs (check the comment i'm removing). We both use standard > > > > way to track ports MTU and we save some additional space. At the cost of > > > > 2 additional checks are are not that much of a problem. > > > > > > Where is the bug? > > > > There was a bug where we tracked the MTU with the FCS and L2 added and > > then in the change_mtu code we added another time the FCS and L2 header > > just because we used this custom way and nobody notice that we were adding > > 2 times the same headers. (it's now fixed but still it's a reason why > > using standard way to track MTU would have prevented that) > > No, I'm sorry, this is completely unjustified complexity - not to > mention it's buggy, too. Does qca8k support cascaded setups? Because if > it does: > > /* We have only have a general MTU setting. So check > * every port and set the max across all port. > */ > list_for_each_entry(dp, &ds->dst->ports, list) { > /* We can ignore cpu port, DSA will itself chose > * the max MTU across all port > */ > if (!dsa_port_is_user(dp)) > continue; > > if (dp->index == port) // <- this will exclude from the max MTU calculation the ports in other switches that are numerically equal to @port. > continue; > > /* Address init phase where not every port have > * a slave device > */ > if (!dp->slave) > continue; > > if (mtu < dp->slave->mtu) > mtu = dp->slave->mtu; > } > > Not to mention it's missing the blatantly obvious. DSA calls > ->port_change_mtu() on the CPU port with the max MTU, every time that > changes. > > You need the max MTU. > > Why calculate it again? Why don't you do what mt7530 does, which has a > similar restriction, and just program the hardware when the CPU port MTU > is updated? > I just checked and wow it was that easy... Also wonder if I should add some check for jumbo frame... (I should check what is the max MTU for the switch and if it can accept jumbo frame+fcs+l2) > You may think - does this work with multiple CPU ports? Well, yes it > does, since DSA calculates the largest MTU across the entire tree, and > not just across the user ports affine to a certain CPU port. > > If it wasn't for this possibility, I would have been in favor of > introducing a dsa_tree_largest_mtu(dst) helper in the DSA core, but I > can't find it justifiable. > > > > > Also from this I discovered that (at least on ipq806x that use stmmac) > > > > when master needs to change MTU, stmmac complains that the interface is > > > > up and it must be put down. Wonder if that's common across other drivers > > > > or it's only specific to stmmac. > > > > > > I never had the pleasure of dealing with such DSA masters. I wonder why > > > can't stmmac_change_mtu() check if netif_running(), call dev_close and > > > set a bool, and at the end, if the bool was set, call dev_open back? > > > > Oh ok so it's not standard that stmmac_change_mtu() just refuse to > > change the MTU instead of put the interface down, change MTU and reopen > > it... Fun stuff... > > > > From system side to change MTU to a new value (so lower MTU on any port > > or set MTU to a higher value for one pot) I have to: > > 1. ifconfig eth0 down > > 2. ifconfig lan1 mtu 1600 up > > 3. ifconfig eth up > > > > If I just ifconfig lan1 mtu 1600 up it's just rejected with stmmac > > complaining. > > Not sure if there is any hard line on this. But I made a poll, and the > crushing majority of drivers in drivers/net/ethernet/ do not require > !netif_running() in ndo_change_mtu. The ones that do are: > > nixge > macb > altera tse > axienet > renesas sh > ksz884x > bcm63xx_enet > sundance > stmmac > > (compared to more than 100 that don't, and even have a dedicated code > path for live changes) > > By the way, an an interesting aside - I've found the xgene, atl1c and > xgmac drivers to be obviously odd (meaning that more drivers might be > odd in the same way, but in more subtle ways I haven't noticed): > when netif_running() is false, they simply return 0, but they don't > change dev->mtu either, they just ignore the request. > > So on one hand you have drivers that _want_ to be down to change the > MTU, and on the other you have drivers that silently ignore MTU changes > when they're down. Hard for DSA to do something reasonable to handle > both cases... Wonder if I should propose a change for stmmac and just drop the interface and restart it when the change is down.
On Thu, Mar 24, 2022 at 09:44:27PM +0100, Ansuel Smith wrote: > On Thu, Mar 24, 2022 at 12:45:24PM +0200, Vladimir Oltean wrote: > > You need the max MTU. > > > > Why calculate it again? Why don't you do what mt7530 does, which has a > > similar restriction, and just program the hardware when the CPU port MTU > > is updated? > > > > I just checked and wow it was that easy... > Also wonder if I should add some check for jumbo frame... (I should > check what is the max MTU for the switch and if it can accept jumbo > frame+fcs+l2) I'm not following you, sorry. What is your definition of "jumbo frame", and what check is there to add? > Wonder if I should propose a change for stmmac and just drop the > interface and restart it when the change is down. In the case of stmmac, dev->mtu is considered from stmmac_fix_features() and from init_dma_rx_desc_rings(), so yes, putting the interface down before updating the MTU and the device features, and then putting it back up if it was previously up, should do the trick. Other drivers like gianfar and many others do this too.
On Thu, Mar 24, 2022 at 11:05:08PM +0200, Vladimir Oltean wrote: > On Thu, Mar 24, 2022 at 09:44:27PM +0100, Ansuel Smith wrote: > > On Thu, Mar 24, 2022 at 12:45:24PM +0200, Vladimir Oltean wrote: > > > You need the max MTU. > > > > > > Why calculate it again? Why don't you do what mt7530 does, which has a > > > similar restriction, and just program the hardware when the CPU port MTU > > > is updated? > > > > > > > I just checked and wow it was that easy... > > Also wonder if I should add some check for jumbo frame... (I should > > check what is the max MTU for the switch and if it can accept jumbo > > frame+fcs+l2) > > I'm not following you, sorry. What is your definition of "jumbo frame", > and what check is there to add? > With jumbo frame i mean frame with MTU of 9000. This is supported by this switch. Anyway i just checked and the reg can totally accept a 9000 MTU + fcs + l2 > > Wonder if I should propose a change for stmmac and just drop the > > interface and restart it when the change is down. > > In the case of stmmac, dev->mtu is considered from stmmac_fix_features() > and from init_dma_rx_desc_rings(), so yes, putting the interface down > before updating the MTU and the device features, and then putting it > back up if it was previously up, should do the trick. Other drivers like > gianfar and many others do this too. Ok i'm reworking this in v2 with the stmmac change and the change to change mtu following what is done for mt7530. Thx a lot for the suggestion. Happy that the additional space can be dropped and still use a more correct and simple approach.
On Fri, Mar 25, 2022 at 12:10:19AM +0100, Ansuel Smith wrote: > Ok i'm reworking this in v2 with the stmmac change and the change to > change mtu following what is done for mt7530. Thx a lot for the > suggestion. Happy that the additional space can be dropped and still use > a more correct and simple approach. That's all fine, but if you read the news you'll notice that net-next is currently closed and will probably be so for around 2 weeks. The pull request for 5.18 was sent by Jakub yesterday: https://patchwork.kernel.org/project/netdevbpf/patch/20220323180738.3978487-1-kuba@kernel.org/ Not sure why the status page says it's still open, it'll probably be updated eventually: http://vger.kernel.org/~davem/net-next.html
On Fri, Mar 25, 2022 at 01:14:23AM +0200, Vladimir Oltean wrote: > On Fri, Mar 25, 2022 at 12:10:19AM +0100, Ansuel Smith wrote: > > Ok i'm reworking this in v2 with the stmmac change and the change to > > change mtu following what is done for mt7530. Thx a lot for the > > suggestion. Happy that the additional space can be dropped and still use > > a more correct and simple approach. > > That's all fine, but if you read the news you'll notice that net-next is > currently closed and will probably be so for around 2 weeks. > The pull request for 5.18 was sent by Jakub yesterday: > https://patchwork.kernel.org/project/netdevbpf/patch/20220323180738.3978487-1-kuba@kernel.org/ > Not sure why the status page says it's still open, it'll probably be > updated eventually: > http://vger.kernel.org/~davem/net-next.html Thanks for the alert! I will send an RFC then hoping to get some review tag. About the html page, I was also confused some times ago where net-next was closed and the page was with the "We are open" image. Wonder if it's a CDN problem? No idea but to me that page looks to be a quicker way than checking net-next last commit or checking the mailing list.
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c index d3ed0a7f8077..4366d87b4bbd 100644 --- a/drivers/net/dsa/qca8k.c +++ b/drivers/net/dsa/qca8k.c @@ -2367,13 +2367,31 @@ static int qca8k_port_change_mtu(struct dsa_switch *ds, int port, int new_mtu) { struct qca8k_priv *priv = ds->priv; - int i, mtu = 0; + struct dsa_port *dp; + int mtu = new_mtu; - priv->port_mtu[port] = new_mtu; + /* We have only have a general MTU setting. So check + * every port and set the max across all port. + */ + list_for_each_entry(dp, &ds->dst->ports, list) { + /* We can ignore cpu port, DSA will itself chose + * the max MTU across all port + */ + if (!dsa_port_is_user(dp)) + continue; - for (i = 0; i < QCA8K_NUM_PORTS; i++) - if (priv->port_mtu[i] > mtu) - mtu = priv->port_mtu[i]; + if (dp->index == port) + continue; + + /* Address init phase where not every port have + * a slave device + */ + if (!dp->slave) + continue; + + if (mtu < dp->slave->mtu) + mtu = dp->slave->mtu; + } /* Include L2 header / FCS length */ return qca8k_write(priv, QCA8K_MAX_FRAME_SIZE, mtu + ETH_HLEN + ETH_FCS_LEN); @@ -3033,16 +3051,6 @@ qca8k_setup(struct dsa_switch *ds) QCA8K_PORT_HOL_CTRL1_WRED_EN, mask); } - - /* Set initial MTU for every port. - * We have only have a general MTU setting. So track - * every port and set the max across all port. - * Set per port MTU to 1500 as the MTU change function - * will add the overhead and if its set to 1518 then it - * will apply the overhead again and we will end up with - * MTU of 1536 instead of 1518 - */ - priv->port_mtu[i] = ETH_DATA_LEN; } /* Special GLOBAL_FC_THRESH value are needed for ar8327 switch */ diff --git a/drivers/net/dsa/qca8k.h b/drivers/net/dsa/qca8k.h index f375627174c8..562d75997e55 100644 --- a/drivers/net/dsa/qca8k.h +++ b/drivers/net/dsa/qca8k.h @@ -398,7 +398,6 @@ struct qca8k_priv { struct device *dev; struct dsa_switch_ops ops; struct gpio_desc *reset_gpio; - unsigned int port_mtu[QCA8K_NUM_PORTS]; struct net_device *mgmt_master; /* Track if mdio/mib Ethernet is available */ struct qca8k_mgmt_eth_data mgmt_eth_data; struct qca8k_mib_eth_data mib_eth_data;
Drop the MTU array from qca8k_priv and use slave net dev to get the max MTU across all user port. CPU port can be skipped as DSA already make sure CPU port are set to the max MTU across all ports. Signed-off-by: Ansuel Smith <ansuelsmth@gmail.com> --- drivers/net/dsa/qca8k.c | 38 +++++++++++++++++++++++--------------- drivers/net/dsa/qca8k.h | 1 - 2 files changed, 23 insertions(+), 16 deletions(-)