Hi Niklas,
On 22-01-2018 16:43, Niklas Cassel wrote:
> Hello stmmac peeps,
>
> I found this smatch error:
>
> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:132
> dwmac4_tx_queue_routing() error:
> buffer overflow 'route_possibilities' 5 <= 254
> drivers/net/ethernet/stmicro/stmmac/dwmac4_core.c:133
> dwmac4_tx_queue_routing() error:
> buffer overflow 'route_possibilities' 5 <= 254
>
> Looking at the code raises some questions:
>
>
> static void dwmac4_tx_queue_routing(struct mac_device_info *hw,
> u8 packet, u32 queue)
> {
> ...
> static const struct stmmac_rx_routing route_possibilities[] = {
> { GMAC_RXQCTRL_AVCPQ_MASK, GMAC_RXQCTRL_AVCPQ_SHIFT },
> { GMAC_RXQCTRL_PTPQ_MASK, GMAC_RXQCTRL_PTPQ_SHIFT },
> { GMAC_RXQCTRL_DCBCPQ_MASK, GMAC_RXQCTRL_DCBCPQ_SHIFT },
> { GMAC_RXQCTRL_UPQ_MASK, GMAC_RXQCTRL_UPQ_SHIFT },
> { GMAC_RXQCTRL_MCBCQ_MASK, GMAC_RXQCTRL_MCBCQ_SHIFT },
> };
>
> value = readl(ioaddr + GMAC_RXQ_CTRL1);
>
> /* routing configuration */
> value &= ~route_possibilities[packet - 1].reg_mask;
> value |= (queue << route_possibilities[packet-1].reg_shift) &
> route_possibilities[packet - 1].reg_mask;
>
>
> Calling the function with e.g. packet == 0 will lead to interesting stuff,
> so the smatch warning is absolutely warranted.
Notice that stmmac_mac_config_rx_queues_routing() checks for
packet == 0x0 and if its true then should never call
rx_queue_routing, check [1] ...
>
>
> Looking where this function is used:
>
> static const struct stmmac_ops dwmac4_ops = {
> ...
> .rx_queue_routing = dwmac4_tx_queue_routing,
>
> Mixing rx and tx.. is this really correct?
It should be dwmac4_rx_queue_routing.
>
>
>
> Looking where the rx_queue_routing function is used:
> git grep rx_queue_routing
> stmmac_main.c: if (rx_queues_count > 1 && priv->hw->mac->rx_queue_routing)
>
> it is just referenced in a single place, and we only check if function is
> non-NULL, we never even call the function, so right now it is just unused
> code.
[1] Another typo. You can see that in function
stmmac_mac_config_rx_queues_routing() we are calling
rx_queue_prio instead of rx_queue_routing ...
Best Regards,
Jose Miguel Abreu
>
>
> Regards,
> Niklas
