| Message ID | 20250116020853.2835521-1-hayashi.kunihiko@socionext.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,1/2] net: stmmac: Limit FIFO size by hardware feature value | expand |
On Thu, 16 Jan 2025 11:08:52 +0900, Kunihiko Hayashi <hayashi.kunihiko@socionext.com> wrote: > Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" from > the platform layer. > > However, these values are constrained by upper limits determined by the > capabilities of each hardware feature. There is a risk that the upper > bits will be truncated due to the calculation, so it's appropriate to > limit them to the upper limit values. > Patch is fine, but the Fixes: tag is required here. And if you like to group this patch and the another patch into one series, it is better to add a cover letter. > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> > --- > drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > index 7bf275f127c9..2d69c3c4b329 100644 > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c > @@ -2375,9 +2375,9 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv) > u32 chan = 0; > u8 qmode = 0; > > - if (rxfifosz == 0) > + if (!rxfifosz || rxfifosz > priv->dma_cap.rx_fifo_size) > rxfifosz = priv->dma_cap.rx_fifo_size; > - if (txfifosz == 0) > + if (!txfifosz || txfifosz > priv->dma_cap.tx_fifo_size) > txfifosz = priv->dma_cap.tx_fifo_size; > > /* Split up the shared Tx/Rx FIFO memory on DW QoS Eth and DW XGMAC */ > @@ -2851,9 +2851,9 @@ static void stmmac_set_dma_operation_mode(struct stmmac_priv *priv, u32 txmode, > int rxfifosz = priv->plat->rx_fifo_size; > int txfifosz = priv->plat->tx_fifo_size; > > - if (rxfifosz == 0) > + if (!rxfifosz || rxfifosz > priv->dma_cap.rx_fifo_size) > rxfifosz = priv->dma_cap.rx_fifo_size; > - if (txfifosz == 0) > + if (!txfifosz || txfifosz > priv->dma_cap.tx_fifo_size) > txfifosz = priv->dma_cap.tx_fifo_size; > > /* Adjust for real per queue fifo size */
Hi Furong, Thank you for your comment. On 2025/01/16 12:04, Furong Xu wrote: > On Thu, 16 Jan 2025 11:08:52 +0900, Kunihiko Hayashi > <hayashi.kunihiko@socionext.com> wrote: > >> Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" from >> the platform layer. >> >> However, these values are constrained by upper limits determined by the >> capabilities of each hardware feature. There is a risk that the upper >> bits will be truncated due to the calculation, so it's appropriate to >> limit them to the upper limit values. >> > > Patch is fine, but the Fixes: tag is required here. I see. I'll find original commit and send the patch with Fixes: tag next. > And if you like to group this patch and the another patch into one series, > it is better to add a cover letter. Yes, I omitted to add a cover letter. However, this patch has no dependency on the other one and needs some consideration, so I'll send it separately. Thank you, --- Best Regards Kunihiko Hayashi
On Thu, Jan 16, 2025 at 11:08:52AM +0900, Kunihiko Hayashi wrote: > Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" from > the platform layer. > > However, these values are constrained by upper limits determined by the > capabilities of each hardware feature. There is a risk that the upper > bits will be truncated due to the calculation, so it's appropriate to > limit them to the upper limit values. Are these values hard coded in the platform layer? Or can they come from userspace? If they are hard coded, we should also fix them. So maybe add a netdev_warn(), and encourage the platform maintainers to fix their platform. If they are coming from userspace, we should consider failing the ethtool call with an -EINVAL, and maybe an extack with the valid range? Andrew
Hi Andrew, On 2025/01/17 5:16, Andrew Lunn wrote: > On Thu, Jan 16, 2025 at 11:08:52AM +0900, Kunihiko Hayashi wrote: >> Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" from >> the platform layer. >> >> However, these values are constrained by upper limits determined by the >> capabilities of each hardware feature. There is a risk that the upper >> bits will be truncated due to the calculation, so it's appropriate to >> limit them to the upper limit values. > > Are these values hard coded in the platform layer? Or can they come > from userspace? My explanation is insufficient and misleading. "From the platform layer" means the common layer of stmmac described in "stmmac_platform.c". > If they are hard coded, we should also fix them. So maybe add a > netdev_warn(), and encourage the platform maintainers to fix their > platform. If they are coming from userspace, we should consider > failing the ethtool call with an -EINVAL, and maybe an extack with the > valid range? These values are derived from the devicetree and stored in the stmmac private structure. They are hardware-specific values, so I think this fix is sufficient. Thank you, --- Best Regards Kunihiko Hayashi
On Mon, Jan 20, 2025 at 02:20:23PM +0900, Kunihiko Hayashi wrote: > Hi Andrew, > > On 2025/01/17 5:16, Andrew Lunn wrote: > > On Thu, Jan 16, 2025 at 11:08:52AM +0900, Kunihiko Hayashi wrote: > > > Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" from > > > the platform layer. > > > > > > However, these values are constrained by upper limits determined by the > > > capabilities of each hardware feature. There is a risk that the upper > > > bits will be truncated due to the calculation, so it's appropriate to > > > limit them to the upper limit values. > > > > Are these values hard coded in the platform layer? Or can they come > > from userspace? > > My explanation is insufficient and misleading. > "From the platform layer" means the common layer of stmmac described in > "stmmac_platform.c". > > > If they are hard coded, we should also fix them. So maybe add a > > netdev_warn(), and encourage the platform maintainers to fix their > > platform. If they are coming from userspace, we should consider > > failing the ethtool call with an -EINVAL, and maybe an extack with the > > valid range? > > These values are derived from the devicetree and stored in the stmmac > private structure. They are hardware-specific values, so I think this > fix is sufficient. But if they are coming from device tree, the device tree developer has made an error, which has been silently ignored. Do we want to leave the device tree broken? Or should we encourage developers to fix them? Printing a warning would facilitate that. Andrew
Hi Andrew, On 2025/01/21 1:29, Andrew Lunn wrote: > On Mon, Jan 20, 2025 at 02:20:23PM +0900, Kunihiko Hayashi wrote: >> Hi Andrew, >> >> On 2025/01/17 5:16, Andrew Lunn wrote: >>> On Thu, Jan 16, 2025 at 11:08:52AM +0900, Kunihiko Hayashi wrote: >>>> Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" > from >>>> the platform layer. >>>> >>>> However, these values are constrained by upper limits determined by > the >>>> capabilities of each hardware feature. There is a risk that the > upper >>>> bits will be truncated due to the calculation, so it's appropriate > to >>>> limit them to the upper limit values. >>> >>> Are these values hard coded in the platform layer? Or can they come >>> from userspace? >> >> My explanation is insufficient and misleading. >> "From the platform layer" means the common layer of stmmac described in >> "stmmac_platform.c". >> >>> If they are hard coded, we should also fix them. So maybe add a >>> netdev_warn(), and encourage the platform maintainers to fix their >>> platform. If they are coming from userspace, we should consider >>> failing the ethtool call with an -EINVAL, and maybe an extack with the >>> valid range? >> >> These values are derived from the devicetree and stored in the stmmac >> private structure. They are hardware-specific values, so I think this >> fix is sufficient. > > But if they are coming from device tree, the device tree developer has > made an error, which has been silently ignored. Do we want to leave > the device tree broken? Or should we encourage developers to fix them? > Printing a warning would facilitate that. I think that developers should fix the devicetree, so I'll add a warning message if the specified value exceeds the hardware capability. Thank you, --- Best Regards Kunihiko Hayashi
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c index 7bf275f127c9..2d69c3c4b329 100644 --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c @@ -2375,9 +2375,9 @@ static void stmmac_dma_operation_mode(struct stmmac_priv *priv) u32 chan = 0; u8 qmode = 0; - if (rxfifosz == 0) + if (!rxfifosz || rxfifosz > priv->dma_cap.rx_fifo_size) rxfifosz = priv->dma_cap.rx_fifo_size; - if (txfifosz == 0) + if (!txfifosz || txfifosz > priv->dma_cap.tx_fifo_size) txfifosz = priv->dma_cap.tx_fifo_size; /* Split up the shared Tx/Rx FIFO memory on DW QoS Eth and DW XGMAC */ @@ -2851,9 +2851,9 @@ static void stmmac_set_dma_operation_mode(struct stmmac_priv *priv, u32 txmode, int rxfifosz = priv->plat->rx_fifo_size; int txfifosz = priv->plat->tx_fifo_size; - if (rxfifosz == 0) + if (!rxfifosz || rxfifosz > priv->dma_cap.rx_fifo_size) rxfifosz = priv->dma_cap.rx_fifo_size; - if (txfifosz == 0) + if (!txfifosz || txfifosz > priv->dma_cap.tx_fifo_size) txfifosz = priv->dma_cap.tx_fifo_size; /* Adjust for real per queue fifo size */
Tx/Rx FIFO size is specified by the parameter "{tx,rx}-fifo-depth" from the platform layer. However, these values are constrained by upper limits determined by the capabilities of each hardware feature. There is a risk that the upper bits will be truncated due to the calculation, so it's appropriate to limit them to the upper limit values. Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@socionext.com> --- drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)