| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <yw1xh9l6c23e.fsf@unicorn.mansr.com>
Date: Sat, 31 Oct 2015 18:41:41 +0000
From: Måns Rullgård <mans@...sr.com>
To: Andy Shevchenko <andy.shevchenko@...il.com>
Cc: "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>,
netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH v4] net: ethernet: add driver for Aurora VLSI NB8800 Ethernet controller
Andy Shevchenko <andy.shevchenko@...il.com> writes:
>> +static int nb8800_mdio_read(struct mii_bus *bus, int phy_id, int reg)
>> +{
>> + struct nb8800_priv *priv = bus->priv;
>> + int val;
>> +
>> + if (!nb8800_mdio_wait(bus))
>> + return -ETIMEDOUT;
>> +
>> + val = MIIAR_ADDR(phy_id) | MIIAR_REG(reg);
>> +
>> + nb8800_writel(priv, NB8800_MDIO_CMD, val);
>> + udelay(10);
>
> Why 10? Perhaps add a comment line.
Because it works. The documentation doesn't mention a delay, but it
works unreliably without one.
>> + nb8800_writel(priv, NB8800_MDIO_CMD, val | MDIO_CMD_GO);
>> +
>> + if (!nb8800_mdio_wait(bus))
>> + return -ETIMEDOUT;
>> +
>> + val = nb8800_readl(priv, NB8800_MDIO_STS);
>> + if (val & MDIO_STS_ERR)
>> + return 0xffff;
>
> Can we return an error here?
That breaks the bus scanning in phylib.
>> +
>> + return val & 0xffff;
>> +}
[...]
>> +static int nb8800_poll(struct napi_struct *napi, int budget)
>> +{
>> + struct net_device *dev = napi->dev;
>> + struct nb8800_priv *priv = netdev_priv(dev);
>> + struct nb8800_dma_desc *rx;
>> + int work = 0;
>> + int last = priv->rx_eoc;
>> + int next;
>> +
>> + while (work < budget) {
>> + struct rx_buf *rx_buf;
>> + u32 report;
>> + int len;
>> +
>> + next = (last + 1) & (RX_DESC_COUNT - 1);
>
> Maybe (last + 1) % RX_DESC_COUNT ? It will not prevent to use
> non-power-of-two numbers.
We don't want to be doing divisions anyway, but I can certainly change
it to % if that's preferred.
>> +
>> + rx_buf = &priv->rx_bufs[next];
>> + rx = &priv->rx_descs[next];
>
>> + report = rx->report;
>
> Maybe you can use rx->report directly below.
It's in uncached memory, so didn't want to have gcc accidentally doing
more reads than necessary.
>> +static int nb8800_xmit(struct sk_buff *skb, struct net_device *dev)
>> +{
>> + struct nb8800_priv *priv = netdev_priv(dev);
>> + struct tx_skb_data *skb_data;
>> + struct tx_buf *tx_buf;
>> + dma_addr_t dma_addr;
>> + unsigned int dma_len;
>> + int cpsz, next;
>> + int frags;
>> +
>> + if (atomic_read(&priv->tx_free) <= NB8800_DESC_LOW) {
>> + netif_stop_queue(dev);
>> + return NETDEV_TX_BUSY;
>> + }
>> +
>> + cpsz = (8 - (uintptr_t)skb->data) & 7;
>
> So, cast to uintptr_t looks strange in this driver, since used only
> twice in such expression, why not to use plain unsigned int * ?
Because it's not the same thing. uintptr_t is an integer type the same
size as a pointer. I need to check if the data pointer is 8-byte
aligned as required by the DMA.
>> +static void nb8800_set_rx_mode(struct net_device *dev)
>> +{
>> + struct nb8800_priv *priv = netdev_priv(dev);
>> + struct netdev_hw_addr *ha;
>> + bool af_en;
>> + int i;
>> +
>> + if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI))
>> + af_en = false;
>> + else
>> + af_en = true;
>> +
>> + nb8800_mac_af(dev, af_en);
>> +
>> + if (!af_en)
>> + return;
>
> Would it be
>
> if (dev->flags & (IFF_PROMISC | IFF_ALLMULTI)) {
> nb8800_mac_af(dev, false);
> return;
> }
>
> nb8800_mac_af(dev, true);
>
> ?
Looks equivalent. Maybe that's clearer.
>> +static int nb8800_hw_init(struct net_device *dev)
>> +{
>> + struct nb8800_priv *priv = netdev_priv(dev);
>
>> + unsigned int val = 0;
>
> Useless assignment.
Indeed. Why didn't gcc warn about that?
>> +
>> + nb8800_writeb(priv, NB8800_RANDOM_SEED, 0x08);
>> +
>> + /* TX single deferral params */
>> + nb8800_writeb(priv, NB8800_TX_SDP, 0xc);
>> +
>> + /* Threshold for partial full */
>> + nb8800_writeb(priv, NB8800_PF_THRESHOLD, 0xff);
>> +
>> + /* Pause Quanta */
>> + nb8800_writeb(priv, NB8800_PQ1, 0xff);
>> + nb8800_writeb(priv, NB8800_PQ2, 0xff);
>
> Lot of magic numbers above and below.
Those are from the original driver. Some of them disagree with the
documentation, and the "correct" values don't work.
>> +
>> + /* configure TX DMA Channels */
>> + val = nb8800_readl(priv, NB8800_TXC_CR);
>> + val &= TCR_LE;
>> + val |= TCR_DM | TCR_RS | TCR_TFI(1) | TCR_BTS(2);
>> + nb8800_writel(priv, NB8800_TXC_CR, val);
>> +
>> + /* TX Interrupt Time Register */
>> + nb8800_writel(priv, NB8800_TX_ITR, 1);
>> +
>> + /* configure RX DMA Channels */
>> + val = nb8800_readl(priv, NB8800_RXC_CR);
>> + val &= RCR_LE;
>> + val |= RCR_DM | RCR_RS | RCR_RFI(7) | RCR_BTS(2) | RCR_FL;
>> + nb8800_writel(priv, NB8800_RXC_CR, val);
>> +
>> + /* RX Interrupt Time Register */
>> + nb8800_writel(priv, NB8800_RX_ITR, 1);
>> +
>> + val = TX_RETRY_EN | TX_PAD_EN | TX_APPEND_FCS;
>> + nb8800_writeb(priv, NB8800_TX_CTL1, val);
>> +
>> + /* collision retry count */
>> + nb8800_writeb(priv, NB8800_TX_CTL2, 5);
>> +
>> + val = RX_PAD_STRIP | RX_PAUSE_EN | RX_AF_EN | RX_RUNT;
>> + nb8800_writeb(priv, NB8800_RX_CTL, val);
>> +
>> + nb8800_mc_init(dev, 0);
>> +
>> + nb8800_writeb(priv, NB8800_TX_BUFSIZE, 0xff);
>> +
>> + return 0;
>> +}
>> +
>> +static void nb8800_tangox_init(struct net_device *dev)
>> +{
>> + struct nb8800_priv *priv = netdev_priv(dev);
>> + u32 val;
>> +
>> + val = nb8800_readb(priv, NB8800_TANGOX_PAD_MODE) & 0x78;
>
> Magic 0x78.
That one I can clarify. Will do.
>> +static int nb8800_probe(struct platform_device *pdev)
>> +{
>> + const struct of_device_id *match;
>> + const struct nb8800_ops *ops = NULL;
>> + struct nb8800_priv *priv;
>> + struct resource *res;
>> + struct net_device *dev;
>> + struct mii_bus *bus;
>> + const unsigned char *mac;
>> + void __iomem *base;
>> + int irq;
>> + int ret;
>> +
>> + match = of_match_device(nb8800_dt_ids, &pdev->dev);
>> + if (match)
>> + ops = match->data;
>> +
>> + res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> + if (!res) {
>> + dev_err(&pdev->dev, "No MMIO base\n");
>> + return -EINVAL;
>> + }
>
> Move platform_get_resource() just before devm_ioremap_resource() and
> remove redundant condition with the message.
>
>> + bus = devm_mdiobus_alloc(&pdev->dev);
>> + if (!bus) {
>> + ret = -ENOMEM;
>> + goto err_disable_clk;
>> + }
>> +
>> + bus->name = "nb8800-mii";
>> + bus->read = nb8800_mdio_read;
>> + bus->write = nb8800_mdio_write;
>> + bus->parent = &pdev->dev;
>> + snprintf(bus->id, MII_BUS_ID_SIZE, "%.*s-mii", MII_BUS_ID_SIZE - 5,
>> + pdev->name);
>
> You are not using any IDs here, why not just to strscpy(bus->id,
> bus->name, MII_BUS_ID_SIZE); for now?
I was under the impression the ID should be unique, and there are two of
these devices in the chip.
--
Måns Rullgård
mans@...sr.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists