| Message ID | 20240912015541.363600-1-khai.wen.tan@linux.intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [net,1/1] net: stmmac: Fix zero-division error when disabling tc cbs | expand |
On Thu, Sep 12, 2024 at 09:55:41AM +0800, KhaiWenTan wrote: > The commit b8c43360f6e4 ("net: stmmac: No need to calculate speed divider > when offload is disabled") allows the "port_transmit_rate_kbps" to be > set to a value of 0, which is then passed to the "div_s64" function when > tc-cbs is disabled. This leads to a zero-division error. > > When tc-cbs is disabled, the idleslope, sendslope, and credit values the > credit values are not required to be configured. Therefore, adding a return > statement after setting the txQ mode to DCB when tc-cbs is disabled would > prevent a zero-division error. > > Fixes: b8c43360f6e4 ("net: stmmac: No need to calculate speed divider when offload is disabled") > Cc: <stable@vger.kernel.org> > Co-developed-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> > Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> > Signed-off-by: KhaiWenTan <khai.wen.tan@linux.intel.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c > index 996f2bcd07a2..2c3fd9c66d14 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c > @@ -392,10 +392,10 @@ static int tc_setup_cbs(struct stmmac_priv *priv, > } else if (!qopt->enable) { > ret = stmmac_dma_qmode(priv, priv->ioaddr, queue, > MTL_QUEUE_DCB); > - if (ret) > - return ret; > + if (!ret) > + priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB; > > - priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB; > + return ret; > } Thanks, I agree with your analysis. But I think it would be more idomatic to write it such that the main thread of execution is the non-error path (in any case, it makes it easier for me to understand the intent of the code. What I am suggesting is this (extra context provided for clarity): diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c index 996f2bcd07a2..308ef4241768 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c @@ -392,14 +392,15 @@ static int tc_setup_cbs(struct stmmac_priv *priv, } 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; + return 0; } /* 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);
On Thu, Sep 12, 2024 at 04:37:30PM +0100, Simon Horman wrote: > On Thu, Sep 12, 2024 at 09:55:41AM +0800, KhaiWenTan wrote: > > The commit b8c43360f6e4 ("net: stmmac: No need to calculate speed divider > > when offload is disabled") allows the "port_transmit_rate_kbps" to be > > set to a value of 0, which is then passed to the "div_s64" function when > > tc-cbs is disabled. This leads to a zero-division error. > > > > When tc-cbs is disabled, the idleslope, sendslope, and credit values the > > credit values are not required to be configured. Therefore, adding a return > > statement after setting the txQ mode to DCB when tc-cbs is disabled would > > prevent a zero-division error. > > > > Fixes: b8c43360f6e4 ("net: stmmac: No need to calculate speed divider when offload is disabled") > > Cc: <stable@vger.kernel.org> > > Co-developed-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> > > Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> > > Signed-off-by: KhaiWenTan <khai.wen.tan@linux.intel.com> ... One more thing, if you do post an updated patch, please be sure to wait until 24h after the original patch was posted. https://docs.kernel.org/process/maintainer-netdev.html
On 12/9/2024 11:39 pm, Simon Horman wrote: > On Thu, Sep 12, 2024 at 04:37:30PM +0100, Simon Horman wrote: >> On Thu, Sep 12, 2024 at 09:55:41AM +0800, KhaiWenTan wrote: >>> The commit b8c43360f6e4 ("net: stmmac: No need to calculate speed divider >>> when offload is disabled") allows the "port_transmit_rate_kbps" to be >>> set to a value of 0, which is then passed to the "div_s64" function when >>> tc-cbs is disabled. This leads to a zero-division error. >>> >>> When tc-cbs is disabled, the idleslope, sendslope, and credit values the >>> credit values are not required to be configured. Therefore, adding a return >>> statement after setting the txQ mode to DCB when tc-cbs is disabled would >>> prevent a zero-division error. >>> >>> Fixes: b8c43360f6e4 ("net: stmmac: No need to calculate speed divider when offload is disabled") >>> Cc: <stable@vger.kernel.org> >>> Co-developed-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> >>> Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> >>> Signed-off-by: KhaiWenTan <khai.wen.tan@linux.intel.com> > ... > > One more thing, if you do post an updated patch, please > be sure to wait until 24h after the original patch was posted. > > https://docs.kernel.org/process/maintainer-netdev.html Hi Simon, Thanks for the clarification. Will be updating a version 2 for this patch.
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c index 996f2bcd07a2..2c3fd9c66d14 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c @@ -392,10 +392,10 @@ static int tc_setup_cbs(struct stmmac_priv *priv, } else if (!qopt->enable) { ret = stmmac_dma_qmode(priv, priv->ioaddr, queue, MTL_QUEUE_DCB); - if (ret) - return ret; + if (!ret) + priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB; - priv->plat->tx_queues_cfg[queue].mode_to_use = MTL_QUEUE_DCB; + return ret; } /* Final adjustments for HW */
The commit b8c43360f6e4 ("net: stmmac: No need to calculate speed divider when offload is disabled") allows the "port_transmit_rate_kbps" to be set to a value of 0, which is then passed to the "div_s64" function when tc-cbs is disabled. This leads to a zero-division error. When tc-cbs is disabled, the idleslope, sendslope, and credit values the credit values are not required to be configured. Therefore, adding a return statement after setting the txQ mode to DCB when tc-cbs is disabled would prevent a zero-division error. Fixes: b8c43360f6e4 ("net: stmmac: No need to calculate speed divider when offload is disabled") Cc: <stable@vger.kernel.org> Co-developed-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> Signed-off-by: Choong Yong Liang <yong.liang.choong@linux.intel.com> Signed-off-by: KhaiWenTan <khai.wen.tan@linux.intel.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_tc.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)