Re: [PATCH v3] ethernet/arc/arc_emac - Add new driver

[Andy Shevchenko]

On Thu, Jun 13, 2013 at 5:37 PM, Alexey Brodkin
<Alexey.Brodkin@xxxxxxxxxxxx> 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.

Much better. But still few comments below.

> +++ b/drivers/net/ethernet/arc/arc_emac.h

What the point to keep arc_emac prefix for files? You have separate
folder for them. This particular one can be main.h.

> +struct arc_emac_priv {
> +       struct net_device_stats stats;
> +       unsigned int clock_frequency;
> +       unsigned int max_speed;
> +
> +       /* Pointers to BD rings - CPU side */
> +       struct arc_emac_bd_t *rxbd;
> +       struct arc_emac_bd_t *txbd;

Usually "_t" means "type" that is used in definitions without 'struct' word.
I don't think this is the case here.

> +       /* Pointers to BD rings - Device side */
> +       dma_addr_t rxbd_dma_hdl;
> +       dma_addr_t txbd_dma_hdl;
> +
> +       /* The actual socket buffers */
> +       struct buffer_state rx_buff[RX_BD_NUM];
> +       struct buffer_state tx_buff[TX_BD_NUM];
> +       unsigned int txbd_curr;
> +       unsigned int txbd_dirty;
> +
> +       /* Remember where driver last saw a packet, so next iteration it
> +        * starts from here and not 0
> +        */
> +       unsigned int last_rx_bd;
> +
> +       struct napi_struct napi;
> +
> +       /* Phy saved state */
> +       int duplex;
> +       int speed;
> +
> +       /* Devices */
> +       struct device *dev;
> +       struct device_node *phy_node;
> +       struct phy_device *phy_dev;
> +       struct net_device *ndev;
> +       struct mii_bus *bus;
> +
> +       /* EMAC registers base address */
> +       void __iomem *reg_base_addr;
> +};

It seems you missed my comments against the names of the members. Can
you address them or comment why not?

> +++ b/drivers/net/ethernet/arc/arc_emac_main.c

> +static int arc_emac_get_settings(struct net_device *ndev,
> +                                struct ethtool_cmd *cmd)
> +{
> +       struct arc_emac_priv *priv = netdev_priv(ndev);
> +
> +       if (priv->phy_dev)
> +               return phy_ethtool_gset(priv->phy_dev, cmd);
> +
> +       return -EINVAL;

May be
if (!...)
 return -EINVAL;
return ...;

> +static int arc_emac_set_settings(struct net_device *ndev,
> +                                struct ethtool_cmd *cmd)
> +{
> +       struct arc_emac_priv *priv = netdev_priv(ndev);
> +
> +       if (!capable(CAP_NET_ADMIN))
> +               return -EPERM;
> +
> +       if (priv->phy_dev)
> +               return phy_ethtool_sset(priv->phy_dev, cmd);
> +
> +       return -EINVAL;

Ditto.

> +static int arc_emac_poll(struct napi_struct *napi, int budget)
> +{
> +       struct net_device *ndev = napi->dev;
> +       struct arc_emac_priv *priv = netdev_priv(ndev);
> +       unsigned int i, loop, work_done = 0;
> +       struct sk_buff *skb;

> +       for (loop = 0; loop < RX_BD_NUM; loop++) {
> +               struct net_device_stats *stats = &priv->stats;
> +               struct buffer_state *rx_buff;
> +               struct arc_emac_bd_t *rxbd;
> +               dma_addr_t addr;
> +               unsigned int info, pktlen;
> +               unsigned int buflen = ndev->mtu + EMAC_BUFFER_PAD;
> +
> +               i = (i + 1) % RX_BD_NUM;

I just realize you are using circular buffer here. What about to use
them with linux' native framework? Documentation/circular-buffers.txt

> +               if (unlikely((info & OWN_MASK) == FOR_EMAC)) {
> +                       /* BD is still owned by EMAC */
> +                       continue;
> +               }

Redundant braces.

> +               addr = dma_map_single(&ndev->dev, (void *)rx_buff->skb->data,
> +                                     buflen, DMA_FROM_DEVICE);
> +               if (dma_mapping_error(&ndev->dev, addr)) {
> +                       if (net_ratelimit())
> +                               if (net_ratelimit())

Is it not a typo?

> +                                       netdev_err(ndev, "cannot dma map\n");

> +               work_done++;
> +               if (work_done >= budget)
> +                       break;

Those three could easily go to the for () on the top of this function.

> +static irqreturn_t arc_emac_intr(int irq, void *dev_instance)
> +{

> +       if (status & TXINT_MASK) {
> +               unsigned int i;
> +
> +               for (i = 0; i < TX_BD_NUM; i++) {
> +                       unsigned int *txbd_dirty = &priv->txbd_dirty;
> +                       struct arc_emac_bd_t *txbd = &priv->txbd[*txbd_dirty];
> +                       struct buffer_state *tx_buff = &priv->tx_buff[*txbd_dirty];
> +                       struct sk_buff *skb = tx_buff->skb;
> +                       unsigned int info = txbd->info;

> +                       txbd->data = 0x0;
> +                       txbd->info = 0x0;

Plain 0?

> +                       *txbd_dirty = (*txbd_dirty + 1) % TX_BD_NUM;

Same idea about circular buffers.

> +static int arc_emac_open(struct net_device *ndev)
> +{

> +       /* Set Poll rate so that it polls every 1 ms */
> +       arc_reg_set(priv, R_POLLRATE,
> +                    priv->clock_frequency / 1000000);

I don't understand how you end up with 1ms here. 1000000 is just a
magic number, clock_frequency generally is an arbitrary value.

> +static struct net_device_stats *arc_emac_stats(struct net_device *ndev)
> +{
> +       unsigned long miss, rxerr, rxfram, rxcrc, rxoflow;
> +       struct arc_emac_priv *priv = netdev_priv(ndev);
> +       struct net_device_stats *stats = &priv->stats;
> +
> +       rxerr = arc_reg_get(priv, R_RXERR);
> +       miss = arc_reg_get(priv, R_MISS);
> +
> +       rxcrc = rxerr & 0xff;
> +       rxfram = rxerr >> 8 & 0xff;
> +       rxoflow = rxerr >> 16 & 0xff;

If you declare those three as u8, you can avoid those & 0xff.

> +       ndev = alloc_etherdev(sizeof(struct arc_emac_priv));
> +       if (!ndev) {
> +               err = -ENOMEM;
> +               goto out;
> +       }

Redundant goto. Just return here.

> +       }
> +
> +

Extra line.

--
With Best Regards,
Andy Shevchenko
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/