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: <EB1619762EAF8B4E97A227FB77B7E0293178EBB5@DBDE01.ent.ti.com> Date: Wed, 22 Feb 2012 16:43:22 +0000 From: "N, Mugunthan V" <mugunthanvnm@...com> To: Joe Perches <joe@...ches.com> CC: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "davem@...emloft.net" <davem@...emloft.net> Subject: RE: [PATCH v2 2/2] netdev: driver: ethernet: Add TI CPSW driver Joe > -----Original Message----- > From: Joe Perches [mailto:joe@...ches.com] > Sent: Tuesday, February 21, 2012 4:34 AM > To: N, Mugunthan V > Cc: netdev@...r.kernel.org; davem@...emloft.net > Subject: Re: [PATCH v2 2/2] netdev: driver: ethernet: Add TI CPSW driver > > On Tue, 2012-02-21 at 00:07 +0530, Mugunthan V N wrote: > > This patch adds support for TI's CPSW driver. > > > > The three port switch gigabit ethernet subsystem provides ethernet > packet > > communication and can be configured as an ethernet switch. Supports > > 10/100/1000 Mbps. > > some more notes. > > > diff --git a/drivers/net/ethernet/ti/cpsw.c > b/drivers/net/ethernet/ti/cpsw.c > [] > > +#define msg(level, type, format, ...) \ > > +do { \ > > + if (netif_msg_##type(priv) && net_ratelimit()) \ > > + dev_##level(priv->dev, format, ## __VA_ARGS__); \ > > +} while (0) > > This hides an always used priv variable. > > I suggest you expand this to multiple #defines like: > > #define cpsw_info(priv, type, format, ...) \ > do { \ > if (netif_msg_##type(priv) && net_ratelimit()) \ > dev_info(priv->dev, format, ##__VA_ARGS__); \ > } while (0) > > and change the uses from > msg(info,...) > to > cpsw_info(priv, type, format, ...) Agreed, will change the define. > > [] > > > +struct cpsw_priv { > > + spinlock_t lock; > > + struct platform_device *pdev; > > + struct net_device *ndev; > > + struct resource *cpsw_res; > > + struct resource *cpsw_ss_res; > > + struct napi_struct napi; > > +#define napi_to_priv(napi) container_of(napi, struct cpsw_priv, napi) > > I still think embedded #defines in structs are hard to read. Agreed, will move defines outside the struct definition. > > [] > > +static int cpsw_poll(struct napi_struct *napi, int budget) > > +{ > > + struct cpsw_priv *priv = napi_to_priv(napi); > > + int num_tx, num_rx; > > + > > + num_tx = cpdma_chan_process(priv->txch, 128); > > + num_rx = cpdma_chan_process(priv->rxch, budget); > > + > > + if (num_rx || num_tx) > > + msg(dbg, intr, "poll %d rx, %d tx pkts\n", num_rx, num_tx); > > cpsw_dbg(priv, intr, etc...) Will change this debug. > [] > > + *link = true; > > + } else { > > + mac_control = 0; > > + /* diable forwarding */ > > disable tyop > > > + if (IS_ERR(slave->phy)) { > > + msg(err, ifup, "phy %s not found on slave %d\n", > > + slave->data->phy_id, slave->slave_num); > > + slave->phy = NULL; > > + } else { > > + printk(KERN_ERR "\nCPSW phy found : id is : 0x%x\n", > > + slave->phy->phy_id); > > Using leading newlines doesn't help much. > I suggest dev_err. Agreed, will change this to dev_err > [] > > > + ndev = alloc_etherdev(sizeof(struct cpsw_priv)); > > + if (!ndev) { > > + pr_err("cpsw: error allocating net_device\n"); > > Add > #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt > before all #includes and remove the embedded prefix > from all pr_<level> uses. Agreed, will make use of pr_fmt and remove embedded prefix in pr_<level> uses > > > + if (is_valid_ether_addr(data->slave_data[0].mac_addr)) { > > + memcpy(priv->mac_addr, data->slave_data[0].mac_addr, > ETH_ALEN); > > + printk(KERN_INFO"Detected MACID=%x:%x:%x:%x:%x:%x\n", > > + priv->mac_addr[0], priv->mac_addr[1], > > + priv->mac_addr[2], priv->mac_addr[3], > > + priv->mac_addr[4], priv->mac_addr[5]); > > Use printf extension %pM to print the MAC address > > pr_info("Detected MAC=%pM\n", priv->mac_addr); Will use %pM to MAC address > > > + priv->slaves = kzalloc(sizeof(struct cpsw_slave) * data->slaves, > > + GFP_KERNEL); > > + if (!priv->slaves) { > > + dev_err(priv->dev, "failed to allocate slave ports\n"); > > No real need for this dev_err Agreed, will remove this dev_err > [] > > + memset(&dma_params, 0, sizeof(dma_params)); > > + dma_params.dev = &pdev->dev; > > + dma_params.dmaregs = (void __iomem *)(((u32)priv->regs) + > > + data->cpdma_reg_ofs); > > + dma_params.rxthresh = (void __iomem *)(((u32)priv->regs) + > > + data->cpdma_reg_ofs + CPDMA_RXTHRESH); > > + dma_params.rxfree = (void __iomem *)(((u32)priv->regs) + > > + data->cpdma_reg_ofs + CPDMA_RXFREE); > > + > > + dma_params.txhdp = (void __iomem *)(((u32)priv->regs) + > > + data->cpdma_sram_ofs + CPDMA_TXHDP); > > + dma_params.rxhdp = (void __iomem *)(((u32)priv->regs) + > > + data->cpdma_sram_ofs + CPDMA_RXHDP); > > + dma_params.txcp = (void __iomem *)(((u32)priv->regs) + > > + data->cpdma_sram_ofs + CPDMA_TXCP); > > + dma_params.rxcp = (void __iomem *)(((u32)priv->regs) + > > + data->cpdma_sram_ofs + CPDMA_RXCP); > > These might be more readable if you didn't try to align it > or maybe add macros or inline functions like: > > #define cpsw_sram_addr(priv, data, offset) \ > (void __iomem *)(((u32)(priv)->regs) + (data)->cpdma_sram_ofs + > offset) Agreed, will make it as macros With regards Mugunthan V N -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists