| 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: <000e01cf40ac$08c19290$1a44b7b0$@samsung.com>
Date: Sat, 15 Mar 2014 17:09:40 -0700
From: "Andrew.an" <bh74.an@...sung.com>
To: 'Francois Romieu' <romieu@...zoreil.com>
Cc: netdev@...r.kernel.org, linux-samsung-soc@...r.kernel.org,
davem@...emloft.net, ilho215.lee@...sung.com,
siva.kallam@...sung.com, vipul.pandya@...sung.com,
ks.giri@...sung.com, 'Joe Perches' <joe@...ches.com>
Subject: RE: [PATCH V2 RE-SEND 1/7] net: sxgbe: add basic framework for Samsung
10Gb ethernet driver
Francois Romieu <romieu@...zoreil.com>
> Byungho An <bh74.an@...sung.com> :
> > From: Siva Reddy <siva.kallam@...sung.com>
> >
> > This patch adds support for Samsung 10Gb ethernet driver(sxgbe).
> > - sxgbe core initialization
> > - Tx and Rx support
> > - MDIO support
> > - ISRs for Tx and Rx
> > - ifconfig support to driver
>
> You'll find a partial review below.
>
> [...]
> > diff --git a/drivers/net/ethernet/samsung/sxgbe_common.h
> b/drivers/net/ethernet/samsung/sxgbe_common.h
> > new file mode 100644
> > index 0000000..3f16220
> > --- /dev/null
> > +++ b/drivers/net/ethernet/samsung/sxgbe_common.h
> [...]
> > +enum dma_irq_status {
> > + tx_hard_error = BIT(0),
> > + tx_bump_tc = BIT(1),
> > + handle_tx = BIT(2),
> > + rx_hard_error = BIT(3),
> > + rx_bump_tc = BIT(4),
> > + handle_rx = BIT(5),
>
> Please use tabs before "=" to line things up.
OK.
>
> [...]
> > +struct sxgbe_hwtimestamp {
> > + void (*config_hw_tstamping)(void __iomem *ioaddr, u32 data);
> > + void (*config_sub_second_increment)(void __iomem *ioaddr);
> > + int (*init_systime)(void __iomem *ioaddr, u32 sec, u32 nsec);
> > + int (*config_addend)(void __iomem *ioaddr, u32 addend);
> > + int (*adjust_systime)(void __iomem *ioaddr, u32 sec, u32 nsec,
> > + int add_sub);
> > + u64 (*get_systime)(void __iomem *ioaddr);
> > +};
>
> None of these method is ever used.
OK. Those methods will be posted later.
>
> Even annotated with __iomem, I'd rather keep the void * to a minimum and
> push the device driver pointer through the call chain. Your call.
I think either will be fine.
>
> [...]
> > +struct sxgbe_core_ops {
> > + /* MAC core initialization */
> > + void (*core_init)(void __iomem *ioaddr);
> > + /* Dump MAC registers */
> > + void (*dump_regs)(void __iomem *ioaddr);
> > + /* Handle extra events on specific interrupts hw dependent */
> > + int (*host_irq_status)(void __iomem *ioaddr,
> > + struct sxgbe_extra_stats *x);
> > + /* Set power management mode (e.g. magic frame) */
> > + void (*pmt)(void __iomem *ioaddr, unsigned long mode);
> > + /* Set/Get Unicast MAC addresses */
> > + void (*set_umac_addr)(void __iomem *ioaddr, unsigned char *addr,
> > + unsigned int reg_n);
> > + void (*get_umac_addr)(void __iomem *ioaddr, unsigned char *addr,
> > + unsigned int reg_n);
> > + void (*enable_rx)(void __iomem *ioaddr, bool enable);
> > + void (*enable_tx)(void __iomem *ioaddr, bool enable);
> > +
> > + /* controller version specific operations */
> > + int (*get_controller_version)(void __iomem *ioaddr);
> > +
> > + /* If supported then get the optional core features */
> > + unsigned int (*get_hw_feature)(void __iomem *ioaddr,
> > + unsigned char feature_index);
> > + /* adjust SXGBE speed */
> > + void (*set_speed)(void __iomem *ioaddr, unsigned char speed);
> > +};
>
> This indirection level is never used.
Those are used, can you give more detail?
>
> > +
> > +const struct sxgbe_core_ops *sxgbe_get_core_ops(void);
> > +
> > +struct sxgbe_ops {
> > + const struct sxgbe_core_ops *mac;
> > + const struct sxgbe_desc_ops *desc;
> > + const struct sxgbe_dma_ops *dma;
> > + const struct sxgbe_mtl_ops *mtl;
>
> Will these indirection levels ever be used ?
Those are used, can you give more detail?
>
> > + const struct sxgbe_hwtimestamp *ptp;
> > + struct mii_regs mii; /* MII register Addresses */
> > + struct mac_link link;
> > + unsigned int ctrl_uid;
> > + unsigned int ctrl_id;
> > +};
> > +
> > +/* SXGBE private data structures */
> > +struct sxgbe_tx_queue {
> > + u8 queue_no;
> > + unsigned int irq_no;
> > + struct sxgbe_priv_data *priv_ptr;
> > + struct sxgbe_tx_norm_desc *dma_tx;
>
> You may lay things a bit differently.
can you give more detail?
>
> [...]
> > +/* SXGBE HW capabilities */
> > +struct sxgbe_hw_features {
> > + /****** CAP [0] *******/
> > + unsigned int gmii_1000mbps;
>
> This field is never read.
It will be used later and will be removed in the next post.
>
> > + unsigned int vlan_hfilter;
>
> This field is never read.
Same above.
>
> > + unsigned int sma_mdio;
>
> This field is never read.
Same above.
>
> > + unsigned int pmt_remote_wake_up;
>
> This field *is* read.
>
> > + unsigned int pmt_magic_frame;
>
> So is this one.
>
> > + unsigned int rmon;
>
> But this one isn't :o/
>
> > + unsigned int arp_offload;
>
> Sic.
>
> The storage is a bit expensive. You may pack some boolean into a single
> unsigned int.
It can be packed but not for all.
>
> [...]
> > +struct sxgbe_priv_data {
> > + /* DMA descriptos */
> > + struct sxgbe_tx_queue *txq[SXGBE_TX_QUEUES];
> > + struct sxgbe_rx_queue *rxq[SXGBE_RX_QUEUES];
> > + u8 cur_rx_qnum;
> > +
> > + unsigned int dma_tx_size;
> > + unsigned int dma_rx_size;
> > + unsigned int dma_buf_sz;
> > + u32 rx_riwt;
> > +
> > + struct napi_struct napi;
> > +
> > + void __iomem *ioaddr;
> > + struct net_device *dev;
> > + struct device *device;
> > + struct sxgbe_ops *hw;/* sxgbe specific ops */
> > + int no_csum_insertion;
> > + spinlock_t lock;
>
> There is no spin_lock_init for this field.
OK. Need to cleanup.
>
> OTOH it isn't really used at all.
>
> [...]
> > + int oldlink;
> > + int speed;
> > + int oldduplex;
> > + unsigned int flow_ctrl;
> > + unsigned int pause;
>
> You may add some extra inner struct when fields are related.
>
> [...]
> > diff --git a/drivers/net/ethernet/samsung/sxgbe_desc.c
> b/drivers/net/ethernet/samsung/sxgbe_desc.c
> > new file mode 100644
> > index 0000000..9a93553
> > --- /dev/null
> > +++ b/drivers/net/ethernet/samsung/sxgbe_desc.c
> [...]
> > +static u64 sxgbe_get_rx_timestamp(struct sxgbe_rx_ctxt_desc *p)
> > +{
> > + u64 ns;
> > + ns = p->tstamp_lo;
>
> Please insert an empty line between the declaration and the body of the
> function.
OK.
>
> [...]
> > diff --git a/drivers/net/ethernet/samsung/sxgbe_desc.h
> b/drivers/net/ethernet/samsung/sxgbe_desc.h
> > new file mode 100644
> > index 0000000..761b521
> > --- /dev/null
> > +++ b/drivers/net/ethernet/samsung/sxgbe_desc.h
> [...]
> > +struct sxgbe_tx_norm_desc {
> > + u64 tdes01; /* buf1 address */
> > + union {
> > + /* TX Read-Format Desc 2,3 */
> > + struct {
> > + /* TDES2 */
> > + u32 buf1_size:14;
> > + u32 vlan_tag_ctl:2;
> > + u32 buf2_size:14;
> > + u32 timestmp_enable:1;
> > + u32 int_on_com:1;
>
> Is there a device endianness control bit to make it safe ?
>
> It's quite common to use __{le / be}32 and the relevant cpu_to_leXY
> helpers.
I'll consider it.
>
> [...]
> > diff --git a/drivers/net/ethernet/samsung/sxgbe_dma.c
> b/drivers/net/ethernet/samsung/sxgbe_dma.c
> > new file mode 100644
> > index 0000000..9ee4a3c
> > --- /dev/null
> > +++ b/drivers/net/ethernet/samsung/sxgbe_dma.c
> [...]
> > +static int sxgbe_dma_init(void __iomem *ioaddr, int fix_burst,
> > + int burst_map, int adv_addr_mode)
> > +{
> > + int retry_count = 10;
> > + u32 reg_val;
> > +
> > + /* reset the DMA */
> > + writel(SXGBE_DMA_SOFT_RESET, ioaddr +
> SXGBE_DMA_MODE_REG);
> > + while (retry_count--) {
> > + if (!(readl(ioaddr + SXGBE_DMA_MODE_REG) &
> > + SXGBE_DMA_SOFT_RESET))
>
> if (!(readl(ioaddr + SXGBE_DMA_MODE_REG) &
> SXGBE_DMA_SOFT_RESET))
>
> Btw some ad-hoc helpers may shorten the code, for instance:
>
> if (!(sx_r32(sp, SXGBE_DMA_MODE_REG) &
> SXGBE_DMA_SOFT_RESET))
>
> [...]
> > +static void sxgbe_dma_channel_init(void __iomem *ioaddr, int cha_num,
> > + int fix_burst, int pbl, dma_addr_t
dma_tx,
> > + dma_addr_t dma_rx, int t_rsize, int
r_rsize)
> > +{
> > + u32 reg_val;
> > + dma_addr_t dma_addr;
> > +
> > + reg_val = readl(ioaddr + SXGBE_DMA_CHA_CTL_REG(cha_num));
> > + /* set the pbl */
> > + if (fix_burst) {
> > + reg_val |= SXGBE_DMA_PBL_X8MODE;
> > + writel(reg_val, ioaddr +
> SXGBE_DMA_CHA_CTL_REG(cha_num));
> > + /* program the TX pbl */
> > + reg_val = readl(ioaddr +
> SXGBE_DMA_CHA_TXCTL_REG(cha_num));
> > + reg_val |= (pbl << SXGBE_DMA_TXPBL_LSHIFT);
> > + writel(reg_val, ioaddr +
> SXGBE_DMA_CHA_TXCTL_REG(cha_num));
> > + /* program the RX pbl */
> > + reg_val = readl(ioaddr +
> SXGBE_DMA_CHA_RXCTL_REG(cha_num));
> > + reg_val |= (pbl << SXGBE_DMA_RXPBL_LSHIFT);
> > + writel(reg_val, ioaddr +
> SXGBE_DMA_CHA_RXCTL_REG(cha_num));
> > + }
> > +
> > + /* program desc registers */
> > + writel((dma_tx >> 32),
>
> Excess parenthesis.
OK.
>
> > + ioaddr + SXGBE_DMA_CHA_TXDESC_HADD_REG(cha_num));
> > + writel((dma_tx & 0xFFFFFFFF),
> > + ioaddr + SXGBE_DMA_CHA_TXDESC_LADD_REG(cha_num));
> > +
> > + writel((dma_rx >> 32),
> > + ioaddr + SXGBE_DMA_CHA_RXDESC_HADD_REG(cha_num));
> > + writel((dma_rx & 0xFFFFFFFF),
> > + ioaddr + SXGBE_DMA_CHA_RXDESC_LADD_REG(cha_num));
> > +
> > + /* program tail pointers */
> > + /* assumption: upper 32 bits are constant and
> > + * same as TX/RX desc list
> > + */
> > + dma_addr = dma_tx + ((t_rsize-1) * SXGBE_DESC_SIZE_BYTES);
>
> dma_addr = dma_tx + ((t_rsize - 1) * SXGBE_DESC_SIZE_BYTES);
OK.
>
> [...]
> > diff --git a/drivers/net/ethernet/samsung/sxgbe_main.c
> b/drivers/net/ethernet/samsung/sxgbe_main.c
> > new file mode 100644
> > index 0000000..7b5a6bd
> > --- /dev/null
> > +++ b/drivers/net/ethernet/samsung/sxgbe_main.c
> [...]
> > +struct sxgbe_priv_data *sxgbe_dvr_probe(struct device *device,
> > + struct sxgbe_plat_data *plat_dat,
> > + void __iomem *addr)
> > +{
> [...]
> > + /* Rx Watchdog is available, enable depend on platform data */
> > + if (!priv->plat->riwt_off) {
> > + priv->use_riwt = 1;
> > + pr_info("Enable RX Mitigation via HW Watchdog Timer\n");
> > + }
> > +
> > + netif_napi_add(ndev, &priv->napi, sxgbe_poll, 64);
>
> There's no balancing netif_napi_del in sxgbe_dvr_remove.
OK.
Thanks.
>
> --
> Ueimor
> --
> 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
--
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