* [net PATCH] net: stmmac: No need to calculate speed divider when offload is disabled
@ 2024-06-14 8:19 Xiaolei Wang
2024-06-15 14:47 ` Simon Horman
0 siblings, 1 reply; 5+ messages in thread
From: Xiaolei Wang @ 2024-06-14 8:19 UTC (permalink / raw)
To: olteanv, linux, alexandre.torgue, andrew, joabreu, davem,
edumazet, kuba, pabeni, mcoquelin.stm32, wojciech.drewek
Cc: netdev, linux-arm-kernel, linux-kernel
commit be27b8965297 ("net: stmmac: replace priv->speed with
the portTransmitRate from the tc-cbs parameters") introduced
a problem. When deleting, it prompts "Invalid portTransmitRate
0 (idleSlope - sendSlope)" and exits. Add judgment on cbs.enable.
Only when offload is enabled, speed divider needs to be calculated.
Fixes: be27b8965297 ("net: stmmac: replace priv->speed with the portTransmitRate from the tc-cbs parameters")
Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
---
.../net/ethernet/stmicro/stmmac/stmmac_tc.c | 38 ++++++++++---------
1 file changed, 20 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
index 1562fbdd0a04..b0fd2d6e525e 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
@@ -358,24 +358,26 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope;
- /* Port Transmit Rate and Speed Divider */
- switch (div_s64(port_transmit_rate_kbps, 1000)) {
- case SPEED_10000:
- case SPEED_5000:
- ptr = 32;
- break;
- case SPEED_2500:
- case SPEED_1000:
- ptr = 8;
- break;
- case SPEED_100:
- ptr = 4;
- break;
- default:
- netdev_err(priv->dev,
- "Invalid portTransmitRate %lld (idleSlope - sendSlope)\n",
- port_transmit_rate_kbps);
- return -EINVAL;
+ if (qopt->enable) {
+ /* Port Transmit Rate and Speed Divider */
+ switch (div_s64(port_transmit_rate_kbps, 1000)) {
+ case SPEED_10000:
+ case SPEED_5000:
+ ptr = 32;
+ break;
+ case SPEED_2500:
+ case SPEED_1000:
+ ptr = 8;
+ break;
+ case SPEED_100:
+ ptr = 4;
+ break;
+ default:
+ netdev_err(priv->dev,
+ "Invalid portTransmitRate %lld (idleSlope - sendSlope)\n",
+ port_transmit_rate_kbps);
+ return -EINVAL;
+ }
}
mode_to_use = priv->plat->tx_queues_cfg[queue].mode_to_use;
--
2.25.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [net PATCH] net: stmmac: No need to calculate speed divider when offload is disabled
2024-06-14 8:19 [net PATCH] net: stmmac: No need to calculate speed divider when offload is disabled Xiaolei Wang
@ 2024-06-15 14:47 ` Simon Horman
2024-06-16 1:15 ` xiaolei wang
0 siblings, 1 reply; 5+ messages in thread
From: Simon Horman @ 2024-06-15 14:47 UTC (permalink / raw)
To: Xiaolei Wang
Cc: olteanv, linux, alexandre.torgue, andrew, joabreu, davem,
edumazet, kuba, pabeni, mcoquelin.stm32, wojciech.drewek, netdev,
linux-arm-kernel, linux-kernel
On Fri, Jun 14, 2024 at 04:19:16PM +0800, Xiaolei Wang wrote:
> commit be27b8965297 ("net: stmmac: replace priv->speed with
> the portTransmitRate from the tc-cbs parameters") introduced
> a problem. When deleting, it prompts "Invalid portTransmitRate
> 0 (idleSlope - sendSlope)" and exits. Add judgment on cbs.enable.
> Only when offload is enabled, speed divider needs to be calculated.
>
> Fixes: be27b8965297 ("net: stmmac: replace priv->speed with the portTransmitRate from the tc-cbs parameters")
> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
> ---
> .../net/ethernet/stmicro/stmmac/stmmac_tc.c | 38 ++++++++++---------
> 1 file changed, 20 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> index 1562fbdd0a04..b0fd2d6e525e 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
> @@ -358,24 +358,26 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
>
> port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope;
>
> - /* Port Transmit Rate and Speed Divider */
> - switch (div_s64(port_transmit_rate_kbps, 1000)) {
> - case SPEED_10000:
> - case SPEED_5000:
> - ptr = 32;
> - break;
> - case SPEED_2500:
> - case SPEED_1000:
> - ptr = 8;
> - break;
> - case SPEED_100:
> - ptr = 4;
> - break;
> - default:
> - netdev_err(priv->dev,
> - "Invalid portTransmitRate %lld (idleSlope - sendSlope)\n",
> - port_transmit_rate_kbps);
> - return -EINVAL;
> + if (qopt->enable) {
> + /* Port Transmit Rate and Speed Divider */
> + switch (div_s64(port_transmit_rate_kbps, 1000)) {
> + case SPEED_10000:
> + case SPEED_5000:
> + ptr = 32;
> + break;
> + case SPEED_2500:
> + case SPEED_1000:
> + ptr = 8;
> + break;
> + case SPEED_100:
> + ptr = 4;
> + break;
> + default:
> + netdev_err(priv->dev,
> + "Invalid portTransmitRate %lld (idleSlope - sendSlope)\n",
> + port_transmit_rate_kbps);
> + return -EINVAL;
> + }
> }
> mode_to_use = priv->plat->tx_queues_cfg[queue].mode_to_use;
Hi Xiaolei Wang,
The code following this function looks like this:
if (mode_to_use == MTL_QUEUE_DCB && qopt->enable) {
ret = stmmac_dma_qmode(priv, priv->ioaddr, queue, MTL_QUEUE_AVB);
if (ret)
return ret;
priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_AVB;
} else if (!qopt->enable) {
ret = stmmac_dma_qmode(priv, priv->ioaddr, queue,
MTL_QUEUE_DCB);
if (ret)
return ret;
priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB;
}
/* Final adjustments for HW */
value = div_s64(qopt->idleslope * 1024ll * ptr, port_transmit_rate_kbps);
priv->plat->tx_queues_cfg[queue].idle_slope = value & GENMASK(31, 0);
value = div_s64(-qopt->sendslope * 1024ll * ptr, port_transmit_rate_kbps);
priv->plat->tx_queues_cfg[queue].send_slope = value & GENMASK(31, 0);
And the div_s64() lines above appear to use
ptr uninitialised in the !qopt->enable case.
Flagged by Smatch.
--
pw-bot: changes-requested
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [net PATCH] net: stmmac: No need to calculate speed divider when offload is disabled
2024-06-15 14:47 ` Simon Horman
@ 2024-06-16 1:15 ` xiaolei wang
2024-06-17 10:29 ` Simon Horman
0 siblings, 1 reply; 5+ messages in thread
From: xiaolei wang @ 2024-06-16 1:15 UTC (permalink / raw)
To: Simon Horman
Cc: olteanv, linux, alexandre.torgue, andrew, joabreu, davem,
edumazet, kuba, pabeni, mcoquelin.stm32, wojciech.drewek, netdev,
linux-arm-kernel, linux-kernel
On 6/15/24 22:47, Simon Horman wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> On Fri, Jun 14, 2024 at 04:19:16PM +0800, Xiaolei Wang wrote:
>> commit be27b8965297 ("net: stmmac: replace priv->speed with
>> the portTransmitRate from the tc-cbs parameters") introduced
>> a problem. When deleting, it prompts "Invalid portTransmitRate
>> 0 (idleSlope - sendSlope)" and exits. Add judgment on cbs.enable.
>> Only when offload is enabled, speed divider needs to be calculated.
>>
>> Fixes: be27b8965297 ("net: stmmac: replace priv->speed with the portTransmitRate from the tc-cbs parameters")
>> Signed-off-by: Xiaolei Wang <xiaolei.wang@windriver.com>
>> ---
>> .../net/ethernet/stmicro/stmmac/stmmac_tc.c | 38 ++++++++++---------
>> 1 file changed, 20 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
>> index 1562fbdd0a04..b0fd2d6e525e 100644
>> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
>> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c
>> @@ -358,24 +358,26 @@ static int tc_setup_cbs(struct stmmac_priv *priv,
>>
>> port_transmit_rate_kbps = qopt->idleslope - qopt->sendslope;
>>
>> - /* Port Transmit Rate and Speed Divider */
>> - switch (div_s64(port_transmit_rate_kbps, 1000)) {
>> - case SPEED_10000:
>> - case SPEED_5000:
>> - ptr = 32;
>> - break;
>> - case SPEED_2500:
>> - case SPEED_1000:
>> - ptr = 8;
>> - break;
>> - case SPEED_100:
>> - ptr = 4;
>> - break;
>> - default:
>> - netdev_err(priv->dev,
>> - "Invalid portTransmitRate %lld (idleSlope - sendSlope)\n",
>> - port_transmit_rate_kbps);
>> - return -EINVAL;
>> + if (qopt->enable) {
>> + /* Port Transmit Rate and Speed Divider */
>> + switch (div_s64(port_transmit_rate_kbps, 1000)) {
>> + case SPEED_10000:
>> + case SPEED_5000:
>> + ptr = 32;
>> + break;
>> + case SPEED_2500:
>> + case SPEED_1000:
>> + ptr = 8;
>> + break;
>> + case SPEED_100:
>> + ptr = 4;
>> + break;
>> + default:
>> + netdev_err(priv->dev,
>> + "Invalid portTransmitRate %lld (idleSlope - sendSlope)\n",
>> + port_transmit_rate_kbps);
>> + return -EINVAL;
>> + }
>> }
>> mode_to_use = priv->plat->tx_queues_cfg[queue].mode_to_use;
> Hi Xiaolei Wang,
>
> The code following this function looks like this:
>
> if (mode_to_use == MTL_QUEUE_DCB && qopt->enable) {
> ret = stmmac_dma_qmode(priv, priv->ioaddr, queue, MTL_QUEUE_AVB);
> if (ret)
> return ret;
> priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_AVB;
> } else if (!qopt->enable) {
> ret = stmmac_dma_qmode(priv, priv->ioaddr, queue,
> MTL_QUEUE_DCB);
> if (ret)
> return ret;
> priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB;
> }
>
> /* Final adjustments for HW */
> value = div_s64(qopt->idleslope * 1024ll * ptr, port_transmit_rate_kbps);
> priv->plat->tx_queues_cfg[queue].idle_slope = value & GENMASK(31, 0);
>
> value = div_s64(-qopt->sendslope * 1024ll * ptr, port_transmit_rate_kbps);
> priv->plat->tx_queues_cfg[queue].send_slope = value & GENMASK(31, 0);
>
> And the div_s64() lines above appear to use
> ptr uninitialised in the !qopt->enable case.
Oh, when deleting the configuration, idleslope and sendslope are both 0,
do you mean we also need to set ptr to 0?
thanks
xiaolei
>
> Flagged by Smatch.
>
> --
> pw-bot: changes-requested
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [net PATCH] net: stmmac: No need to calculate speed divider when offload is disabled
2024-06-16 1:15 ` xiaolei wang
@ 2024-06-17 10:29 ` Simon Horman
2024-06-18 2:31 ` xiaolei wang
0 siblings, 1 reply; 5+ messages in thread
From: Simon Horman @ 2024-06-17 10:29 UTC (permalink / raw)
To: xiaolei wang
Cc: olteanv, linux, alexandre.torgue, andrew, joabreu, davem,
edumazet, kuba, pabeni, mcoquelin.stm32, wojciech.drewek, netdev,
linux-arm-kernel, linux-kernel
On Sun, Jun 16, 2024 at 09:15:05AM +0800, xiaolei wang wrote:
>
> On 6/15/24 22:47, Simon Horman wrote:
> > On Fri, Jun 14, 2024 at 04:19:16PM +0800, Xiaolei Wang wrote:
...
> > /* Final adjustments for HW */
> > value = div_s64(qopt->idleslope * 1024ll * ptr, port_transmit_rate_kbps);
> > priv->plat->tx_queues_cfg[queue].idle_slope = value & GENMASK(31, 0);
> >
> > value = div_s64(-qopt->sendslope * 1024ll * ptr, port_transmit_rate_kbps);
> > priv->plat->tx_queues_cfg[queue].send_slope = value & GENMASK(31, 0);
> >
> > And the div_s64() lines above appear to use
> > ptr uninitialised in the !qopt->enable case.
>
> Oh, when deleting the configuration, idleslope and sendslope are both 0, do
> you mean we also need to set ptr to 0?
Understood, if idleslope and sendslope are 0, then ptr could be set to any
value and the result would be the same. And, based on my limited
understanding, 0 does not seem to be a bad choice.
My point is that ptr shouldn't be uninitialised at this point.
...
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [net PATCH] net: stmmac: No need to calculate speed divider when offload is disabled
2024-06-17 10:29 ` Simon Horman
@ 2024-06-18 2:31 ` xiaolei wang
0 siblings, 0 replies; 5+ messages in thread
From: xiaolei wang @ 2024-06-18 2:31 UTC (permalink / raw)
To: Simon Horman
Cc: olteanv, linux, alexandre.torgue, andrew, joabreu, davem,
edumazet, kuba, pabeni, mcoquelin.stm32, wojciech.drewek, netdev,
linux-arm-kernel, linux-kernel
On 6/17/24 18:29, Simon Horman wrote:
> CAUTION: This email comes from a non Wind River email account!
> Do not click links or open attachments unless you recognize the sender and know the content is safe.
>
> On Sun, Jun 16, 2024 at 09:15:05AM +0800, xiaolei wang wrote:
>> On 6/15/24 22:47, Simon Horman wrote:
>>> On Fri, Jun 14, 2024 at 04:19:16PM +0800, Xiaolei Wang wrote:
> ...
>
>>> /* Final adjustments for HW */
>>> value = div_s64(qopt->idleslope * 1024ll * ptr, port_transmit_rate_kbps);
>>> priv->plat->tx_queues_cfg[queue].idle_slope = value & GENMASK(31, 0);
>>>
>>> value = div_s64(-qopt->sendslope * 1024ll * ptr, port_transmit_rate_kbps);
>>> priv->plat->tx_queues_cfg[queue].send_slope = value & GENMASK(31, 0);
>>>
>>> And the div_s64() lines above appear to use
>>> ptr uninitialised in the !qopt->enable case.
>> Oh, when deleting the configuration, idleslope and sendslope are both 0, do
>> you mean we also need to set ptr to 0?
> Understood, if idleslope and sendslope are 0, then ptr could be set to any
> value and the result would be the same. And, based on my limited
> understanding, 0 does not seem to be a bad choice.
>
> My point is that ptr shouldn't be uninitialised at this point.
OK, I have sent the v2 version
https://patchwork.kernel.org/project/netdevbpf/patch/20240617013922.1035854-1-xiaolei.wang@windriver.com/
thanks
xiaolei
>
> ...
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2024-06-18 2:31 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-06-14 8:19 [net PATCH] net: stmmac: No need to calculate speed divider when offload is disabled Xiaolei Wang
2024-06-15 14:47 ` Simon Horman
2024-06-16 1:15 ` xiaolei wang
2024-06-17 10:29 ` Simon Horman
2024-06-18 2:31 ` xiaolei wang
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).