| 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
| ||
|
Date: Wed, 19 Jun 2013 15:05:54 +0300
From: Andy Shevchenko <andy.shevchenko@...il.com>
To: Alexey Brodkin <Alexey.Brodkin@...opsys.com>
Cc: netdev <netdev@...r.kernel.org>,
Francois Romieu <romieu@...zoreil.com>,
Joe Perches <joe@...ches.com>,
Vineet Gupta <Vineet.Gupta1@...opsys.com>,
Mischa Jonker <Mischa.Jonker@...opsys.com>,
Arnd Bergmann <arnd@...db.de>,
Grant Likely <grant.likely@...aro.org>,
Rob Herring <rob.herring@...xeda.com>,
Paul Gortmaker <paul.gortmaker@...driver.com>,
"David S. Miller" <davem@...emloft.net>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Devicetree Discuss <devicetree-discuss@...ts.ozlabs.org>
Subject: Re: [PATCH v5] ethernet/arc/arc_emac - Add new driver
On Wed, Jun 19, 2013 at 11:12 AM, Alexey Brodkin
<Alexey.Brodkin@...opsys.com> wrote:
> Driver for non-standard on-chip ethernet device ARC EMAC 10/100,
> instantiated in some legacy ARC (Synopsys) FPGA Boards such as
> ARCAngel4/ML50x.
>
> This is based off of current Linus tree.
This line should go away from commit message.
> please take a look at 5th re-spin of the patch.
Few small things I commented below.
> +++ b/drivers/net/ethernet/arc/emac_main.c
> @@ -0,0 +1,828 @@
> +static int arc_emac_rx(struct net_device *ndev, int budget)
> +{
> + struct arc_emac_priv *priv = netdev_priv(ndev);
> + unsigned int work_done;
> +
> + for (work_done = 0; work_done <= budget;) {
> + unsigned int *last_rx_bd = &priv->last_rx_bd;
> + struct net_device_stats *stats = &priv->stats;
> + struct buffer_state *rx_buff = &priv->rx_buff[*last_rx_bd];
> + struct arc_emac_bd *rxbd = &priv->rxbd[*last_rx_bd];
> + unsigned int buflen = EMAC_BUFFER_SIZE;
> + unsigned int pktlen, info = __le32_to_cpu(rxbd->info);
> + struct sk_buff *skb;
> + dma_addr_t addr;
> +
> + if (unlikely((info & OWN_MASK) == FOR_EMAC))
> + break;
> + work_done++;
Why it can't be inside for()?
> + if (unlikely((info & FRST_OR_LAST_MASK) != FRST_OR_LAST_MASK)) {
> + /* Buffer chaining results in a significant
> + * amount of additional bus overhead and thus
> + * a higher CLK frequency or larger FIFOs are required.
> + * Because of that fact we don't support
> + * chaining of receive packets. We pre-allocate
> + * buffers of MTU size so incoming packets
> + * won't be chained.
> + */
Could that comment take less LOC?
> + /* Return ownership to EMAC */
> + rxbd->info = __cpu_to_le32(FOR_EMAC | buflen);
Here and in the whole file. Parhaps cpu_to_le32()?
> + /* Once EMAC sees he is an owner of this BD, EMAC will start to
> + * use it for receiving or transmitting packets,
> + * depending on BD: Tx or Rx.
> + * That's why we need to make sure "data" has proper address
> + * before writing "info" word with owner flag.
> + */
Less LOC?
> + if (status & ERR_MASK) {
> + /* MSER/RXCR/RXFR/RXFL interrupt fires on corresponding
> + * 8-bit error counter overrun.
> + * Because of this fact we add 256 (0x100) items each time
> + * overrun interrupt happens.
> + */
Ditto.
> +static int arc_emac_tx(struct sk_buff *skb, struct net_device *ndev)
> +{
> + len = max_t(unsigned int, ETH_ZLEN, skb->len);
> +
> + /* EMAC still holds this buffer in its possession.
> + * CPU must not modify this buffer descriptor
> + */
> + if (unlikely((__le32_to_cpu(*info) & OWN_MASK) == FOR_EMAC)) {
Perhaps le32_to_cpu() here and in whole file?
> +static int arc_emac_probe(struct platform_device *pdev)
> +{
> + err = of_address_to_resource(pdev->dev.of_node, 0, &res_regs);
> + if (of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> + &clock_frequency)) {
> + err = of_irq_to_resource(pdev->dev.of_node, 0, &res_irq);
You may save few lines if you move those checks before allocation of the netdev.
> + err = arc_mdio_probe(pdev->dev.of_node, priv);
> + if (err) {
> + dev_err(&pdev->dev, "failed to probe MII bus\n");
> + goto out;
> + }
> +
> + priv->phy_dev = of_phy_connect(ndev, phy_node, arc_emac_adjust_link, 0,
> + PHY_INTERFACE_MODE_MII);
> + if (!priv->phy_dev) {
> + netdev_err(ndev, "of_phy_connect() failed\n");
Can we be consistent and use here dev_err()?
> +++ b/drivers/net/ethernet/arc/emac_mdio.c
> @@ -0,0 +1,151 @@
> +static int arc_mdio_complete_wait(struct arc_emac_priv *priv)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < ARC_MDIO_COMPLETE_POLL_COUNT * 40; i++) {
> + msleep(25);
So, in the worst case it takes ARC_MDIO_COMPLETE_POLL_COUNT seconds to
wait. Quite a long time.
Does user expect such a delay?
> +int arc_mdio_probe(struct device_node *dev_node, struct arc_emac_priv *priv)
> +{
> + snprintf(bus->id, MII_BUS_ID_SIZE, "%.8x", (unsigned int)priv->regs);
Is bus->id exposed to user-space somehow?
--
With Best Regards,
Andy Shevchenko
--
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