RE: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
Hi Bo Yu,
> -Original Message-
> From: Bo Yu [mailto:tsu.y...@gmail.com]
> Sent: Monday, June 19, 2017 1:57 AM
> To: Salil Mehta
> Cc: da...@davemloft.net; Zhuangyuzeng (Yisen); huangdaode; lipeng (Y);
> mehta.salil@gmail.com; net...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Linuxarm
> Subject: Re: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3
> Ethernet Driver for hip08 SoC
>
> Hi,
> On Sat, Jun 17, 2017 at 06:24:24PM +0100, Salil Mehta wrote:
> >+struct notifier_block notifier_block;
> >+/* Vxlan/Geneve information */
> >+struct hns3_udp_tunnel udp_tnl[HNS3_UDP_TNL_MAX];
> >+};
> >+
> >+/* the distance between [begin, end) in a ring buffer
> >+ * note: there is a unuse slot between the begin and the end
> >+ */
> >+static inline int ring_dist(struct hns3_enet_ring *ring, int begin,
> int end)
> >+{
> >+return (end - begin + ring->desc_num) % ring->desc_num;
> >+}
> >+
> >+static inline int ring_space(struct hns3_enet_ring *ring)
> >+{
> >+return ring->desc_num -
> >+ring_dist(ring, ring->next_to_clean, ring->next_to_use) -
> 1;
> >+}
> >+
> >+static inline int is_ring_empty(struct hns3_enet_ring *ring)
> >+{
> >+return ring->next_to_use == ring->next_to_clean;
> >+}
> >+
> >+static inline void hns3_write_reg(void __iomem *base, u32 reg, u32
> value)
> >+{
> >+u8 __iomem *reg_addr = READ_ONCE(base);
> >+
> >+writel(value, reg_addr + reg);
> >+}
> >+
> >+#define hns3_write_dev(a, reg, value) \
> >+hns3_write_reg((a)->io_base, (reg), (value))
> >+
> >+#define hnae_queue_xmit(tqp, buf_num) writel_relaxed(buf_num, \
> >+(tqp)->io_base + HNS3_RING_TX_RING_TAIL_REG)
> >+
> >+#define ring_to_dev(ring) (&(ring)->tqp->handle->pdev->dev)
> >+
> >+#define ring_to_dma_dir(ring) (HNAE3_IS_TX_RING(ring) ? \
> >+DMA_TO_DEVICE : DMA_FROM_DEVICE)
> >+
> >+#define tx_ring_data(priv, idx) ((priv)->ring_data[idx])
> >+
> >+#define hnae_buf_size(_ring) ((_ring)->buf_size)
> >+#define hnae_page_order(_ring) (get_order(hnae_buf_size(_ring)))
> >+#define hnae_page_size(_ring) (PAGE_SIZE << hnae_page_order(_ring))
> >+
> >+/* iterator for handling rings in ring group */
> >+#define hns3_for_each_ring(pos, head) \
> >+for (pos = (head).ring; pos != NULL; pos = pos->next)
>
> Only a pos? Comparsion to NULL could be written "pos" noticed by
> checkpatch.
Fixed in patch V4. Thanks!
Salil
>
>
> >+
> >+void hns3_ethtool_set_ops(struct net_device *ndev);
> >+
> >+int hns3_nic_net_xmit_hw(
> >+struct net_device *ndev,
> >+struct sk_buff *skb,
> >+struct hns3_nic_ring_data *ring_data);
> >+int hns3_clean_tx_ring(struct hns3_enet_ring *ring, int budget);
> >+int hns3_clean_rx_ring_ex(
> >+struct hns3_enet_ring *ring,
> >+struct sk_buff **skb_ex,
> >+int budget);
> >+#endif
> >--
> >2.7.4
> >
> >
RE: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
Hi Bo Yu,
> -Original Message-
> From: Bo Yu [mailto:tsu.y...@gmail.com]
> Sent: Monday, June 19, 2017 1:18 AM
> To: Salil Mehta
> Cc: da...@davemloft.net; Zhuangyuzeng (Yisen); huangdaode; lipeng (Y);
> mehta.salil@gmail.com; net...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Linuxarm
> Subject: Re: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3
> Ethernet Driver for hip08 SoC
>
> Hi,
> On Sat, Jun 17, 2017 at 06:24:24PM +0100, Salil Mehta wrote:
> >+static int hns3_fill_desc(struct hns3_enet_ring *ring, void *priv,
> >+ int size, dma_addr_t dma, int frag_end,
> >+ enum hns_desc_type type)
> >+{
> >+struct hns3_desc_cb *desc_cb = >desc_cb[ring->next_to_use];
> >+struct hns3_desc *desc = >desc[ring->next_to_use];
> >+u32 ol_type_vlan_len_msec = 0;
> >+u16 bdtp_fe_sc_vld_ra_ri = 0;
> >+u32 type_cs_vlan_tso = 0;
> >+struct sk_buff *skb;
> >+u32 paylen = 0;
> >+u16 mss = 0;
> >+__be16 protocol;
> >+u8 ol4_proto;
> >+u8 il4_proto;
> >+int ret;
> >+
> >+/* The txbd's baseinfo of DESC_TYPE_PAGE & DESC_TYPE_SKB */
> >+desc_cb->priv = priv;
> >+desc_cb->length = size;
> >+desc_cb->dma = dma;
> >+desc_cb->type = type;
> >+
> >+/* now, fill the descriptor */
> >+desc->addr = cpu_to_le64(dma);
> >+desc->tx.send_size = cpu_to_le16((u16)size);
> >+hns3_set_txbd_baseinfo(_fe_sc_vld_ra_ri, frag_end);
> >+desc->tx.bdtp_fe_sc_vld_ra_ri =
> cpu_to_le16(bdtp_fe_sc_vld_ra_ri);
> >+
> >+if (type == DESC_TYPE_SKB) {
> >+skb = (struct sk_buff *)priv;
> >+paylen = cpu_to_le16(skb->len);
> >+
> >+if (skb->ip_summed == CHECKSUM_PARTIAL) {
> >+skb_reset_mac_len(skb);
> >+protocol = skb->protocol;
> >+
> >+/* vlan packe t*/
>
> Just a spealling: /* vlan packet */
Fixed in V4 patch. Thanks!
Salil
>
> >+if (protocol == htons(ETH_P_8021Q)) {
> >+protocol = vlan_get_protocol(skb);
> >+skb->protocol = protocol;
> >+}
> >+hns3_get_l4_protocol(skb, _proto, _proto);
> >+hns3_set_l2l3l4_len(skb, ol4_proto, il4_proto,
> >+_cs_vlan_tso,
> >+_type_vlan_len_msec);
> >+ret = hns3_set_l3l4_type_csum(skb, ol4_proto,
> il4_proto,
> >+ _cs_vlan_tso,
> >+ _type_vlan_len_msec);
> >+if (ret)
> >+return ret;
> >+
> >+ret = hns3_set_tso(skb, , ,
> >+ _cs_vlan_tso);
> >+if (ret)
> >+return ret;
> >+}
> >+
> >+/* Set txbd */
> >+desc->tx.ol_type_vlan_len_msec =
> >+cpu_to_le32(ol_type_vlan_len_msec);
> >+desc->tx.type_cs_vlan_tso_len =
> >+cpu_to_le32(type_cs_vlan_tso);
> >+desc->tx.paylen = cpu_to_le16(paylen);
> >+desc->tx.mss = cpu_to_le16(mss);
> >+}
> >+
> >+/* move ring pointer to next.*/
> >+ring_ptr_move_fw(ring, next_to_use);
> >+
> >+return 0;
> >+}
> >+
> >+static int hns3_fill_desc_tso(struct hns3_enet_ring *ring, void
> *priv,
> >+ int size, dma_addr_t dma, int frag_end,
> >+ enum hns_desc_type type)
> >+{
> >+int frag_buf_num;
> >+int sizeoflast;
> >+int ret, k;
> >+
> >+frag_buf_num = (size + HNS3_MAX_BD_SIZE - 1) / HNS3_MAX_BD_SIZE;
> >+sizeoflast = size % HNS3_MAX_BD_SIZE;
> >+sizeoflast = sizeoflast ? sizeoflast : HNS3_MAX_BD_SIZE;
> >+
> >+/* When the frag size is bigger than hardware, split this frag */
> >+for (k = 0; k < frag_buf_num; k++) {
> >+ret = hns3_fill_desc(ring, priv,
> >+ (k == frag_buf_num - 1) ?
> >+sizeoflast : HNS3_MAX_BD_SIZE,
> >+dma + HNS3_MAX_BD_SIZE * k,
> >+frag_end && (k == f
RE: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
Hi Bo Yu,
> -Original Message-
> From: Bo Yu [mailto:tsu.y...@gmail.com]
> Sent: Monday, June 19, 2017 1:18 AM
> To: Salil Mehta
> Cc: da...@davemloft.net; Zhuangyuzeng (Yisen); huangdaode; lipeng (Y);
> mehta.salil@gmail.com; net...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Linuxarm
> Subject: Re: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3
> Ethernet Driver for hip08 SoC
>
> Hi,
> On Sat, Jun 17, 2017 at 06:24:24PM +0100, Salil Mehta wrote:
> >+static int hns3_fill_desc(struct hns3_enet_ring *ring, void *priv,
> >+ int size, dma_addr_t dma, int frag_end,
> >+ enum hns_desc_type type)
> >+{
> >+struct hns3_desc_cb *desc_cb = >desc_cb[ring->next_to_use];
> >+struct hns3_desc *desc = >desc[ring->next_to_use];
> >+u32 ol_type_vlan_len_msec = 0;
> >+u16 bdtp_fe_sc_vld_ra_ri = 0;
> >+u32 type_cs_vlan_tso = 0;
> >+struct sk_buff *skb;
> >+u32 paylen = 0;
> >+u16 mss = 0;
> >+__be16 protocol;
> >+u8 ol4_proto;
> >+u8 il4_proto;
> >+int ret;
> >+
> >+/* The txbd's baseinfo of DESC_TYPE_PAGE & DESC_TYPE_SKB */
> >+desc_cb->priv = priv;
> >+desc_cb->length = size;
> >+desc_cb->dma = dma;
> >+desc_cb->type = type;
> >+
> >+/* now, fill the descriptor */
> >+desc->addr = cpu_to_le64(dma);
> >+desc->tx.send_size = cpu_to_le16((u16)size);
> >+hns3_set_txbd_baseinfo(_fe_sc_vld_ra_ri, frag_end);
> >+desc->tx.bdtp_fe_sc_vld_ra_ri =
> cpu_to_le16(bdtp_fe_sc_vld_ra_ri);
> >+
> >+if (type == DESC_TYPE_SKB) {
> >+skb = (struct sk_buff *)priv;
> >+paylen = cpu_to_le16(skb->len);
> >+
> >+if (skb->ip_summed == CHECKSUM_PARTIAL) {
> >+skb_reset_mac_len(skb);
> >+protocol = skb->protocol;
> >+
> >+/* vlan packe t*/
>
> Just a spealling: /* vlan packet */
Fixed in V4 patch. Thanks!
Salil
>
> >+if (protocol == htons(ETH_P_8021Q)) {
> >+protocol = vlan_get_protocol(skb);
> >+skb->protocol = protocol;
> >+}
> >+hns3_get_l4_protocol(skb, _proto, _proto);
> >+hns3_set_l2l3l4_len(skb, ol4_proto, il4_proto,
> >+_cs_vlan_tso,
> >+_type_vlan_len_msec);
> >+ret = hns3_set_l3l4_type_csum(skb, ol4_proto,
> il4_proto,
> >+ _cs_vlan_tso,
> >+ _type_vlan_len_msec);
> >+if (ret)
> >+return ret;
> >+
> >+ret = hns3_set_tso(skb, , ,
> >+ _cs_vlan_tso);
> >+if (ret)
> >+return ret;
> >+}
> >+
> >+/* Set txbd */
> >+desc->tx.ol_type_vlan_len_msec =
> >+cpu_to_le32(ol_type_vlan_len_msec);
> >+desc->tx.type_cs_vlan_tso_len =
> >+cpu_to_le32(type_cs_vlan_tso);
> >+desc->tx.paylen = cpu_to_le16(paylen);
> >+desc->tx.mss = cpu_to_le16(mss);
> >+}
> >+
> >+/* move ring pointer to next.*/
> >+ring_ptr_move_fw(ring, next_to_use);
> >+
> >+return 0;
> >+}
> >+
> >+static int hns3_fill_desc_tso(struct hns3_enet_ring *ring, void
> *priv,
> >+ int size, dma_addr_t dma, int frag_end,
> >+ enum hns_desc_type type)
> >+{
> >+int frag_buf_num;
> >+int sizeoflast;
> >+int ret, k;
> >+
> >+frag_buf_num = (size + HNS3_MAX_BD_SIZE - 1) / HNS3_MAX_BD_SIZE;
> >+sizeoflast = size % HNS3_MAX_BD_SIZE;
> >+sizeoflast = sizeoflast ? sizeoflast : HNS3_MAX_BD_SIZE;
> >+
> >+/* When the frag size is bigger than hardware, split this frag */
> >+for (k = 0; k < frag_buf_num; k++) {
> >+ret = hns3_fill_desc(ring, priv,
> >+ (k == frag_buf_num - 1) ?
> >+sizeoflast : HNS3_MAX_BD_SIZE,
> >+dma + HNS3_MAX_BD_SIZE * k,
> >+frag_end && (k == f
RE: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
Hi Andrew
> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Saturday, June 17, 2017 6:54 PM
> To: Salil Mehta
> Cc: da...@davemloft.net; Zhuangyuzeng (Yisen); huangdaode; lipeng (Y);
> mehta.salil@gmail.com; net...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Linuxarm
> Subject: Re: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3
> Ethernet Driver for hip08 SoC
>
> > +static int hns3_nic_net_up(struct net_device *ndev)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(ndev);
> > + struct hnae3_handle *h = priv->ae_handle;
> > + int i, j;
> > + int ret;
> > +
> > + ret = hns3_nic_init_irq(priv);
> > + if (ret != 0) {
>
> if (ret)
>
> No need to compare with zero.
Sure, changed in V4 patch.
Thanks
Salil
>
> > + netdev_err(ndev, "hns init irq failed! ret=%d\n", ret);
> > + return ret;
>
> > +static int hns3_nic_net_open(struct net_device *ndev)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(ndev);
> > + struct hnae3_handle *h = priv->ae_handle;
> > + int ret;
> > +
> > + netif_carrier_off(ndev);
> > +
> > + ret = netif_set_real_num_tx_queues(ndev, h->kinfo.num_tqps);
> > + if (ret < 0) {
> > + netdev_err(ndev, "netif_set_real_num_tx_queues fail,
> ret=%d!\n",
> > + ret);
> > + return ret;
> > + }
>
> In general, functions return 0 for success, and something else for an
> error. So there is no need to do a comparison. Please remove all
> comparisons, unless it is really needed. It also makes the code look
> consistent. At the moment you sometime have < 0, sometime !=0, and
> sometimes no comparison at all.
Acknowledged, scanned and have changed in V4 patch. Please have a look.
Thanks
Salil
>
> Andrew
RE: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
Hi Andrew
> -Original Message-
> From: Andrew Lunn [mailto:and...@lunn.ch]
> Sent: Saturday, June 17, 2017 6:54 PM
> To: Salil Mehta
> Cc: da...@davemloft.net; Zhuangyuzeng (Yisen); huangdaode; lipeng (Y);
> mehta.salil@gmail.com; net...@vger.kernel.org; linux-
> ker...@vger.kernel.org; Linuxarm
> Subject: Re: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3
> Ethernet Driver for hip08 SoC
>
> > +static int hns3_nic_net_up(struct net_device *ndev)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(ndev);
> > + struct hnae3_handle *h = priv->ae_handle;
> > + int i, j;
> > + int ret;
> > +
> > + ret = hns3_nic_init_irq(priv);
> > + if (ret != 0) {
>
> if (ret)
>
> No need to compare with zero.
Sure, changed in V4 patch.
Thanks
Salil
>
> > + netdev_err(ndev, "hns init irq failed! ret=%d\n", ret);
> > + return ret;
>
> > +static int hns3_nic_net_open(struct net_device *ndev)
> > +{
> > + struct hns3_nic_priv *priv = netdev_priv(ndev);
> > + struct hnae3_handle *h = priv->ae_handle;
> > + int ret;
> > +
> > + netif_carrier_off(ndev);
> > +
> > + ret = netif_set_real_num_tx_queues(ndev, h->kinfo.num_tqps);
> > + if (ret < 0) {
> > + netdev_err(ndev, "netif_set_real_num_tx_queues fail,
> ret=%d!\n",
> > + ret);
> > + return ret;
> > + }
>
> In general, functions return 0 for success, and something else for an
> error. So there is no need to do a comparison. Please remove all
> comparisons, unless it is really needed. It also makes the code look
> consistent. At the moment you sometime have < 0, sometime !=0, and
> sometimes no comparison at all.
Acknowledged, scanned and have changed in V4 patch. Please have a look.
Thanks
Salil
>
> Andrew
Re: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
Hi,
On Sat, Jun 17, 2017 at 06:24:24PM +0100, Salil Mehta wrote:
+ struct notifier_block notifier_block;
+ /* Vxlan/Geneve information */
+ struct hns3_udp_tunnel udp_tnl[HNS3_UDP_TNL_MAX];
+};
+
+/* the distance between [begin, end) in a ring buffer
+ * note: there is a unuse slot between the begin and the end
+ */
+static inline int ring_dist(struct hns3_enet_ring *ring, int begin, int end)
+{
+ return (end - begin + ring->desc_num) % ring->desc_num;
+}
+
+static inline int ring_space(struct hns3_enet_ring *ring)
+{
+ return ring->desc_num -
+ ring_dist(ring, ring->next_to_clean, ring->next_to_use) - 1;
+}
+
+static inline int is_ring_empty(struct hns3_enet_ring *ring)
+{
+ return ring->next_to_use == ring->next_to_clean;
+}
+
+static inline void hns3_write_reg(void __iomem *base, u32 reg, u32 value)
+{
+ u8 __iomem *reg_addr = READ_ONCE(base);
+
+ writel(value, reg_addr + reg);
+}
+
+#define hns3_write_dev(a, reg, value) \
+ hns3_write_reg((a)->io_base, (reg), (value))
+
+#define hnae_queue_xmit(tqp, buf_num) writel_relaxed(buf_num, \
+ (tqp)->io_base + HNS3_RING_TX_RING_TAIL_REG)
+
+#define ring_to_dev(ring) (&(ring)->tqp->handle->pdev->dev)
+
+#define ring_to_dma_dir(ring) (HNAE3_IS_TX_RING(ring) ? \
+ DMA_TO_DEVICE : DMA_FROM_DEVICE)
+
+#define tx_ring_data(priv, idx) ((priv)->ring_data[idx])
+
+#define hnae_buf_size(_ring) ((_ring)->buf_size)
+#define hnae_page_order(_ring) (get_order(hnae_buf_size(_ring)))
+#define hnae_page_size(_ring) (PAGE_SIZE << hnae_page_order(_ring))
+
+/* iterator for handling rings in ring group */
+#define hns3_for_each_ring(pos, head) \
+ for (pos = (head).ring; pos != NULL; pos = pos->next)
Only a pos? Comparsion to NULL could be written "pos" noticed by
checkpatch.
+
+void hns3_ethtool_set_ops(struct net_device *ndev);
+
+int hns3_nic_net_xmit_hw(
+ struct net_device *ndev,
+ struct sk_buff *skb,
+ struct hns3_nic_ring_data *ring_data);
+int hns3_clean_tx_ring(struct hns3_enet_ring *ring, int budget);
+int hns3_clean_rx_ring_ex(
+ struct hns3_enet_ring *ring,
+ struct sk_buff **skb_ex,
+ int budget);
+#endif
--
2.7.4
Re: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
Hi,
On Sat, Jun 17, 2017 at 06:24:24PM +0100, Salil Mehta wrote:
+ struct notifier_block notifier_block;
+ /* Vxlan/Geneve information */
+ struct hns3_udp_tunnel udp_tnl[HNS3_UDP_TNL_MAX];
+};
+
+/* the distance between [begin, end) in a ring buffer
+ * note: there is a unuse slot between the begin and the end
+ */
+static inline int ring_dist(struct hns3_enet_ring *ring, int begin, int end)
+{
+ return (end - begin + ring->desc_num) % ring->desc_num;
+}
+
+static inline int ring_space(struct hns3_enet_ring *ring)
+{
+ return ring->desc_num -
+ ring_dist(ring, ring->next_to_clean, ring->next_to_use) - 1;
+}
+
+static inline int is_ring_empty(struct hns3_enet_ring *ring)
+{
+ return ring->next_to_use == ring->next_to_clean;
+}
+
+static inline void hns3_write_reg(void __iomem *base, u32 reg, u32 value)
+{
+ u8 __iomem *reg_addr = READ_ONCE(base);
+
+ writel(value, reg_addr + reg);
+}
+
+#define hns3_write_dev(a, reg, value) \
+ hns3_write_reg((a)->io_base, (reg), (value))
+
+#define hnae_queue_xmit(tqp, buf_num) writel_relaxed(buf_num, \
+ (tqp)->io_base + HNS3_RING_TX_RING_TAIL_REG)
+
+#define ring_to_dev(ring) (&(ring)->tqp->handle->pdev->dev)
+
+#define ring_to_dma_dir(ring) (HNAE3_IS_TX_RING(ring) ? \
+ DMA_TO_DEVICE : DMA_FROM_DEVICE)
+
+#define tx_ring_data(priv, idx) ((priv)->ring_data[idx])
+
+#define hnae_buf_size(_ring) ((_ring)->buf_size)
+#define hnae_page_order(_ring) (get_order(hnae_buf_size(_ring)))
+#define hnae_page_size(_ring) (PAGE_SIZE << hnae_page_order(_ring))
+
+/* iterator for handling rings in ring group */
+#define hns3_for_each_ring(pos, head) \
+ for (pos = (head).ring; pos != NULL; pos = pos->next)
Only a pos? Comparsion to NULL could be written "pos" noticed by
checkpatch.
+
+void hns3_ethtool_set_ops(struct net_device *ndev);
+
+int hns3_nic_net_xmit_hw(
+ struct net_device *ndev,
+ struct sk_buff *skb,
+ struct hns3_nic_ring_data *ring_data);
+int hns3_clean_tx_ring(struct hns3_enet_ring *ring, int budget);
+int hns3_clean_rx_ring_ex(
+ struct hns3_enet_ring *ring,
+ struct sk_buff **skb_ex,
+ int budget);
+#endif
--
2.7.4
Re: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
Hi,
On Sat, Jun 17, 2017 at 06:24:24PM +0100, Salil Mehta wrote:
+static int hns3_fill_desc(struct hns3_enet_ring *ring, void *priv,
+ int size, dma_addr_t dma, int frag_end,
+ enum hns_desc_type type)
+{
+ struct hns3_desc_cb *desc_cb = >desc_cb[ring->next_to_use];
+ struct hns3_desc *desc = >desc[ring->next_to_use];
+ u32 ol_type_vlan_len_msec = 0;
+ u16 bdtp_fe_sc_vld_ra_ri = 0;
+ u32 type_cs_vlan_tso = 0;
+ struct sk_buff *skb;
+ u32 paylen = 0;
+ u16 mss = 0;
+ __be16 protocol;
+ u8 ol4_proto;
+ u8 il4_proto;
+ int ret;
+
+ /* The txbd's baseinfo of DESC_TYPE_PAGE & DESC_TYPE_SKB */
+ desc_cb->priv = priv;
+ desc_cb->length = size;
+ desc_cb->dma = dma;
+ desc_cb->type = type;
+
+ /* now, fill the descriptor */
+ desc->addr = cpu_to_le64(dma);
+ desc->tx.send_size = cpu_to_le16((u16)size);
+ hns3_set_txbd_baseinfo(_fe_sc_vld_ra_ri, frag_end);
+ desc->tx.bdtp_fe_sc_vld_ra_ri = cpu_to_le16(bdtp_fe_sc_vld_ra_ri);
+
+ if (type == DESC_TYPE_SKB) {
+ skb = (struct sk_buff *)priv;
+ paylen = cpu_to_le16(skb->len);
+
+ if (skb->ip_summed == CHECKSUM_PARTIAL) {
+ skb_reset_mac_len(skb);
+ protocol = skb->protocol;
+
+ /* vlan packe t*/
Just a spealling: /* vlan packet */
+ if (protocol == htons(ETH_P_8021Q)) {
+ protocol = vlan_get_protocol(skb);
+ skb->protocol = protocol;
+ }
+ hns3_get_l4_protocol(skb, _proto, _proto);
+ hns3_set_l2l3l4_len(skb, ol4_proto, il4_proto,
+ _cs_vlan_tso,
+ _type_vlan_len_msec);
+ ret = hns3_set_l3l4_type_csum(skb, ol4_proto, il4_proto,
+ _cs_vlan_tso,
+ _type_vlan_len_msec);
+ if (ret)
+ return ret;
+
+ ret = hns3_set_tso(skb, , ,
+ _cs_vlan_tso);
+ if (ret)
+ return ret;
+ }
+
+ /* Set txbd */
+ desc->tx.ol_type_vlan_len_msec =
+ cpu_to_le32(ol_type_vlan_len_msec);
+ desc->tx.type_cs_vlan_tso_len =
+ cpu_to_le32(type_cs_vlan_tso);
+ desc->tx.paylen = cpu_to_le16(paylen);
+ desc->tx.mss = cpu_to_le16(mss);
+ }
+
+ /* move ring pointer to next.*/
+ ring_ptr_move_fw(ring, next_to_use);
+
+ return 0;
+}
+
+static int hns3_fill_desc_tso(struct hns3_enet_ring *ring, void *priv,
+ int size, dma_addr_t dma, int frag_end,
+ enum hns_desc_type type)
+{
+ int frag_buf_num;
+ int sizeoflast;
+ int ret, k;
+
+ frag_buf_num = (size + HNS3_MAX_BD_SIZE - 1) / HNS3_MAX_BD_SIZE;
+ sizeoflast = size % HNS3_MAX_BD_SIZE;
+ sizeoflast = sizeoflast ? sizeoflast : HNS3_MAX_BD_SIZE;
+
+ /* When the frag size is bigger than hardware, split this frag */
+ for (k = 0; k < frag_buf_num; k++) {
+ ret = hns3_fill_desc(ring, priv,
+(k == frag_buf_num - 1) ?
+ sizeoflast : HNS3_MAX_BD_SIZE,
+ dma + HNS3_MAX_BD_SIZE * k,
+ frag_end && (k == frag_buf_num - 1) ? 1 : 0,
+ (type == DESC_TYPE_SKB && !k) ?
+ DESC_TYPE_SKB : DESC_TYPE_PAGE);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int hns3_nic_maybe_stop_tso(struct sk_buff **out_skb, int *bnum,
+ struct hns3_enet_ring *ring)
+{
+ struct sk_buff *skb = *out_skb;
+ struct skb_frag_struct *frag;
+ int bdnum_for_frag;
+ int frag_num;
+ int buf_num;
+ int size;
+ int i;
+
+ size = skb_headlen(skb);
+ buf_num = (size + HNS3_MAX_BD_SIZE - 1) / HNS3_MAX_BD_SIZE;
+
+ frag_num = skb_shinfo(skb)->nr_frags;
+ for (i = 0; i < frag_num; i++) {
+ frag = _shinfo(skb)->frags[i];
+ size = skb_frag_size(frag);
+ bdnum_for_frag =
+ (size + HNS3_MAX_BD_SIZE - 1) / HNS3_MAX_BD_SIZE;
+ if (bdnum_for_frag > HNS3_MAX_BD_PER_FRAG)
+ return -ENOMEM;
+
+ buf_num += bdnum_for_frag;
+ }
+
+ if (buf_num > ring_space(ring))
+
Re: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
Hi,
On Sat, Jun 17, 2017 at 06:24:24PM +0100, Salil Mehta wrote:
+static int hns3_fill_desc(struct hns3_enet_ring *ring, void *priv,
+ int size, dma_addr_t dma, int frag_end,
+ enum hns_desc_type type)
+{
+ struct hns3_desc_cb *desc_cb = >desc_cb[ring->next_to_use];
+ struct hns3_desc *desc = >desc[ring->next_to_use];
+ u32 ol_type_vlan_len_msec = 0;
+ u16 bdtp_fe_sc_vld_ra_ri = 0;
+ u32 type_cs_vlan_tso = 0;
+ struct sk_buff *skb;
+ u32 paylen = 0;
+ u16 mss = 0;
+ __be16 protocol;
+ u8 ol4_proto;
+ u8 il4_proto;
+ int ret;
+
+ /* The txbd's baseinfo of DESC_TYPE_PAGE & DESC_TYPE_SKB */
+ desc_cb->priv = priv;
+ desc_cb->length = size;
+ desc_cb->dma = dma;
+ desc_cb->type = type;
+
+ /* now, fill the descriptor */
+ desc->addr = cpu_to_le64(dma);
+ desc->tx.send_size = cpu_to_le16((u16)size);
+ hns3_set_txbd_baseinfo(_fe_sc_vld_ra_ri, frag_end);
+ desc->tx.bdtp_fe_sc_vld_ra_ri = cpu_to_le16(bdtp_fe_sc_vld_ra_ri);
+
+ if (type == DESC_TYPE_SKB) {
+ skb = (struct sk_buff *)priv;
+ paylen = cpu_to_le16(skb->len);
+
+ if (skb->ip_summed == CHECKSUM_PARTIAL) {
+ skb_reset_mac_len(skb);
+ protocol = skb->protocol;
+
+ /* vlan packe t*/
Just a spealling: /* vlan packet */
+ if (protocol == htons(ETH_P_8021Q)) {
+ protocol = vlan_get_protocol(skb);
+ skb->protocol = protocol;
+ }
+ hns3_get_l4_protocol(skb, _proto, _proto);
+ hns3_set_l2l3l4_len(skb, ol4_proto, il4_proto,
+ _cs_vlan_tso,
+ _type_vlan_len_msec);
+ ret = hns3_set_l3l4_type_csum(skb, ol4_proto, il4_proto,
+ _cs_vlan_tso,
+ _type_vlan_len_msec);
+ if (ret)
+ return ret;
+
+ ret = hns3_set_tso(skb, , ,
+ _cs_vlan_tso);
+ if (ret)
+ return ret;
+ }
+
+ /* Set txbd */
+ desc->tx.ol_type_vlan_len_msec =
+ cpu_to_le32(ol_type_vlan_len_msec);
+ desc->tx.type_cs_vlan_tso_len =
+ cpu_to_le32(type_cs_vlan_tso);
+ desc->tx.paylen = cpu_to_le16(paylen);
+ desc->tx.mss = cpu_to_le16(mss);
+ }
+
+ /* move ring pointer to next.*/
+ ring_ptr_move_fw(ring, next_to_use);
+
+ return 0;
+}
+
+static int hns3_fill_desc_tso(struct hns3_enet_ring *ring, void *priv,
+ int size, dma_addr_t dma, int frag_end,
+ enum hns_desc_type type)
+{
+ int frag_buf_num;
+ int sizeoflast;
+ int ret, k;
+
+ frag_buf_num = (size + HNS3_MAX_BD_SIZE - 1) / HNS3_MAX_BD_SIZE;
+ sizeoflast = size % HNS3_MAX_BD_SIZE;
+ sizeoflast = sizeoflast ? sizeoflast : HNS3_MAX_BD_SIZE;
+
+ /* When the frag size is bigger than hardware, split this frag */
+ for (k = 0; k < frag_buf_num; k++) {
+ ret = hns3_fill_desc(ring, priv,
+(k == frag_buf_num - 1) ?
+ sizeoflast : HNS3_MAX_BD_SIZE,
+ dma + HNS3_MAX_BD_SIZE * k,
+ frag_end && (k == frag_buf_num - 1) ? 1 : 0,
+ (type == DESC_TYPE_SKB && !k) ?
+ DESC_TYPE_SKB : DESC_TYPE_PAGE);
+ if (ret)
+ return ret;
+ }
+
+ return 0;
+}
+
+static int hns3_nic_maybe_stop_tso(struct sk_buff **out_skb, int *bnum,
+ struct hns3_enet_ring *ring)
+{
+ struct sk_buff *skb = *out_skb;
+ struct skb_frag_struct *frag;
+ int bdnum_for_frag;
+ int frag_num;
+ int buf_num;
+ int size;
+ int i;
+
+ size = skb_headlen(skb);
+ buf_num = (size + HNS3_MAX_BD_SIZE - 1) / HNS3_MAX_BD_SIZE;
+
+ frag_num = skb_shinfo(skb)->nr_frags;
+ for (i = 0; i < frag_num; i++) {
+ frag = _shinfo(skb)->frags[i];
+ size = skb_frag_size(frag);
+ bdnum_for_frag =
+ (size + HNS3_MAX_BD_SIZE - 1) / HNS3_MAX_BD_SIZE;
+ if (bdnum_for_frag > HNS3_MAX_BD_PER_FRAG)
+ return -ENOMEM;
+
+ buf_num += bdnum_for_frag;
+ }
+
+ if (buf_num > ring_space(ring))
+
Re: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
> +
> + for (i = 0; i < priv->vector_num; i++) {
> + tqp_vectors = >tqp_vector[i];
> +
> + if (tqp_vectors->irq_init_flag == HNS3_VEVTOR_INITED)
Should VEVTOR be VECTOR?
> +static void hns3_set_vector_gl(struct hns3_enet_tqp_vector *tqp_vector,
> +u32 gl_value)
> +{
> + writel(gl_value, tqp_vector->mask_addr + HNS3_VECTOR_GL0_OFFSET);
> + writel(gl_value, tqp_vector->mask_addr + HNS3_VECTOR_GL1_OFFSET);
> + writel(gl_value, tqp_vector->mask_addr + HNS3_VECTOR_GL2_OFFSET);
> +}
> +
> +static void hns3_set_vector_rl(struct hns3_enet_tqp_vector *tqp_vector,
> +u32 rl_value)
Could you use more informative names. What does gl and rl mean?
> +{
> + writel(rl_value, tqp_vector->mask_addr + HNS3_VECTOR_RL_OFFSET);
> +}
> +
> +static void hns3_vector_gl_rl_init(struct hns3_enet_tqp_vector *tqp_vector)
> +{
> + /* Default :enable interrupt coalesce */
> + tqp_vector->rx_group.int_gl = HNS3_INT_GL_50K;
> + tqp_vector->tx_group.int_gl = HNS3_INT_GL_50K;
> + hns3_set_vector_gl(tqp_vector, HNS3_INT_GL_50K);
> + hns3_set_vector_rl(tqp_vector, 0);
> + tqp_vector->rx_group.flow_level = HNS3_FLOW_LOW;
> + tqp_vector->tx_group.flow_level = HNS3_FLOW_LOW;
> +}
> +
> +static int hns3_nic_net_up(struct net_device *ndev)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(ndev);
> + struct hnae3_handle *h = priv->ae_handle;
> + int i, j;
> + int ret;
> +
> + ret = hns3_nic_init_irq(priv);
> + if (ret != 0) {
> + netdev_err(ndev, "hns init irq failed! ret=%d\n", ret);
> + return ret;
> + }
> +
> + for (i = 0; i < priv->vector_num; i++)
> + hns3_vector_enable(>tqp_vector[i]);
> +
> + ret = h->ae_algo->ops->start ? h->ae_algo->ops->start(h) : 0;
> + if (ret)
> + goto out_start_err;
> +
> + return 0;
> +
> +out_start_err:
> + netif_stop_queue(ndev);
This seems asymmetric. Where is the netif_start_queue()?
> +
> + for (j = i - 1; j >= 0; j--)
> + hns3_vector_disable(>tqp_vector[j]);
> +
> + return ret;
> +}
> +
> +static int hns3_nic_net_open(struct net_device *ndev)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(ndev);
> + struct hnae3_handle *h = priv->ae_handle;
> + int ret;
> +
> + netif_carrier_off(ndev);
> +
> + ret = netif_set_real_num_tx_queues(ndev, h->kinfo.num_tqps);
> + if (ret < 0) {
> + netdev_err(ndev, "netif_set_real_num_tx_queues fail, ret=%d!\n",
> +ret);
> + return ret;
> + }
> +
> + ret = netif_set_real_num_rx_queues(ndev, h->kinfo.num_tqps);
> + if (ret < 0) {
> + netdev_err(ndev,
> +"netif_set_real_num_rx_queues fail, ret=%d!\n", ret);
> + return ret;
> + }
> +
> + ret = hns3_nic_net_up(ndev);
> + if (ret) {
> + netdev_err(ndev,
> +"hns net up fail, ret=%d!\n", ret);
> + return ret;
> + }
> +
> + netif_carrier_on(ndev);
Carrier on should be performed when the PHY says there is link.
> + netif_tx_wake_all_queues(ndev);
> +
> + return 0;
> +}
> +
> +static void hns3_nic_net_down(struct net_device *ndev)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(ndev);
> + struct hnae3_ae_ops *ops;
> + int i;
> +
> + netif_tx_stop_all_queues(ndev);
> + netif_carrier_off(ndev);
> +
> + ops = priv->ae_handle->ae_algo->ops;
> +
> + if (ops->stop)
> + ops->stop(priv->ae_handle);
> +
> + for (i = 0; i < priv->vector_num; i++)
> + hns3_vector_disable(>tqp_vector[i]);
> +}
> +
> +static int hns3_nic_net_stop(struct net_device *ndev)
> +{
> + hns3_nic_net_down(ndev);
> +
> + return 0;
> +}
> +
> +void hns3_set_multicast_list(struct net_device *ndev)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(ndev);
> + struct hnae3_handle *h = priv->ae_handle;
> + struct netdev_hw_addr *ha = NULL;
> +
> + if (!h) {
> + netdev_err(ndev, "hnae handle is null\n");
> + return;
> + }
> +
> + if (h->ae_algo->ops->set_mc_addr) {
> + netdev_for_each_mc_addr(ha, ndev)
> + if (h->ae_algo->ops->set_mc_addr(h, ha->addr))
> + netdev_err(ndev, "set multicast fail\n");
> + }
> +}
> +
> +static int hns3_nic_uc_sync(struct net_device *netdev,
> + const unsigned char *addr)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(netdev);
> + struct hnae3_handle *h = priv->ae_handle;
> +
> + if (h->ae_algo->ops->add_uc_addr)
> + return h->ae_algo->ops->add_uc_addr(h, addr);
> +
> + return 0;
> +}
> +
> +static int hns3_nic_uc_unsync(struct net_device *netdev,
> + const unsigned char *addr)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(netdev);
> +
Re: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
> +
> + for (i = 0; i < priv->vector_num; i++) {
> + tqp_vectors = >tqp_vector[i];
> +
> + if (tqp_vectors->irq_init_flag == HNS3_VEVTOR_INITED)
Should VEVTOR be VECTOR?
> +static void hns3_set_vector_gl(struct hns3_enet_tqp_vector *tqp_vector,
> +u32 gl_value)
> +{
> + writel(gl_value, tqp_vector->mask_addr + HNS3_VECTOR_GL0_OFFSET);
> + writel(gl_value, tqp_vector->mask_addr + HNS3_VECTOR_GL1_OFFSET);
> + writel(gl_value, tqp_vector->mask_addr + HNS3_VECTOR_GL2_OFFSET);
> +}
> +
> +static void hns3_set_vector_rl(struct hns3_enet_tqp_vector *tqp_vector,
> +u32 rl_value)
Could you use more informative names. What does gl and rl mean?
> +{
> + writel(rl_value, tqp_vector->mask_addr + HNS3_VECTOR_RL_OFFSET);
> +}
> +
> +static void hns3_vector_gl_rl_init(struct hns3_enet_tqp_vector *tqp_vector)
> +{
> + /* Default :enable interrupt coalesce */
> + tqp_vector->rx_group.int_gl = HNS3_INT_GL_50K;
> + tqp_vector->tx_group.int_gl = HNS3_INT_GL_50K;
> + hns3_set_vector_gl(tqp_vector, HNS3_INT_GL_50K);
> + hns3_set_vector_rl(tqp_vector, 0);
> + tqp_vector->rx_group.flow_level = HNS3_FLOW_LOW;
> + tqp_vector->tx_group.flow_level = HNS3_FLOW_LOW;
> +}
> +
> +static int hns3_nic_net_up(struct net_device *ndev)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(ndev);
> + struct hnae3_handle *h = priv->ae_handle;
> + int i, j;
> + int ret;
> +
> + ret = hns3_nic_init_irq(priv);
> + if (ret != 0) {
> + netdev_err(ndev, "hns init irq failed! ret=%d\n", ret);
> + return ret;
> + }
> +
> + for (i = 0; i < priv->vector_num; i++)
> + hns3_vector_enable(>tqp_vector[i]);
> +
> + ret = h->ae_algo->ops->start ? h->ae_algo->ops->start(h) : 0;
> + if (ret)
> + goto out_start_err;
> +
> + return 0;
> +
> +out_start_err:
> + netif_stop_queue(ndev);
This seems asymmetric. Where is the netif_start_queue()?
> +
> + for (j = i - 1; j >= 0; j--)
> + hns3_vector_disable(>tqp_vector[j]);
> +
> + return ret;
> +}
> +
> +static int hns3_nic_net_open(struct net_device *ndev)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(ndev);
> + struct hnae3_handle *h = priv->ae_handle;
> + int ret;
> +
> + netif_carrier_off(ndev);
> +
> + ret = netif_set_real_num_tx_queues(ndev, h->kinfo.num_tqps);
> + if (ret < 0) {
> + netdev_err(ndev, "netif_set_real_num_tx_queues fail, ret=%d!\n",
> +ret);
> + return ret;
> + }
> +
> + ret = netif_set_real_num_rx_queues(ndev, h->kinfo.num_tqps);
> + if (ret < 0) {
> + netdev_err(ndev,
> +"netif_set_real_num_rx_queues fail, ret=%d!\n", ret);
> + return ret;
> + }
> +
> + ret = hns3_nic_net_up(ndev);
> + if (ret) {
> + netdev_err(ndev,
> +"hns net up fail, ret=%d!\n", ret);
> + return ret;
> + }
> +
> + netif_carrier_on(ndev);
Carrier on should be performed when the PHY says there is link.
> + netif_tx_wake_all_queues(ndev);
> +
> + return 0;
> +}
> +
> +static void hns3_nic_net_down(struct net_device *ndev)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(ndev);
> + struct hnae3_ae_ops *ops;
> + int i;
> +
> + netif_tx_stop_all_queues(ndev);
> + netif_carrier_off(ndev);
> +
> + ops = priv->ae_handle->ae_algo->ops;
> +
> + if (ops->stop)
> + ops->stop(priv->ae_handle);
> +
> + for (i = 0; i < priv->vector_num; i++)
> + hns3_vector_disable(>tqp_vector[i]);
> +}
> +
> +static int hns3_nic_net_stop(struct net_device *ndev)
> +{
> + hns3_nic_net_down(ndev);
> +
> + return 0;
> +}
> +
> +void hns3_set_multicast_list(struct net_device *ndev)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(ndev);
> + struct hnae3_handle *h = priv->ae_handle;
> + struct netdev_hw_addr *ha = NULL;
> +
> + if (!h) {
> + netdev_err(ndev, "hnae handle is null\n");
> + return;
> + }
> +
> + if (h->ae_algo->ops->set_mc_addr) {
> + netdev_for_each_mc_addr(ha, ndev)
> + if (h->ae_algo->ops->set_mc_addr(h, ha->addr))
> + netdev_err(ndev, "set multicast fail\n");
> + }
> +}
> +
> +static int hns3_nic_uc_sync(struct net_device *netdev,
> + const unsigned char *addr)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(netdev);
> + struct hnae3_handle *h = priv->ae_handle;
> +
> + if (h->ae_algo->ops->add_uc_addr)
> + return h->ae_algo->ops->add_uc_addr(h, addr);
> +
> + return 0;
> +}
> +
> +static int hns3_nic_uc_unsync(struct net_device *netdev,
> + const unsigned char *addr)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(netdev);
> +
Re: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
> +static int hns3_nic_net_up(struct net_device *ndev)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(ndev);
> + struct hnae3_handle *h = priv->ae_handle;
> + int i, j;
> + int ret;
> +
> + ret = hns3_nic_init_irq(priv);
> + if (ret != 0) {
if (ret)
No need to compare with zero.
> + netdev_err(ndev, "hns init irq failed! ret=%d\n", ret);
> + return ret;
> +static int hns3_nic_net_open(struct net_device *ndev)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(ndev);
> + struct hnae3_handle *h = priv->ae_handle;
> + int ret;
> +
> + netif_carrier_off(ndev);
> +
> + ret = netif_set_real_num_tx_queues(ndev, h->kinfo.num_tqps);
> + if (ret < 0) {
> + netdev_err(ndev, "netif_set_real_num_tx_queues fail, ret=%d!\n",
> +ret);
> + return ret;
> + }
In general, functions return 0 for success, and something else for an
error. So there is no need to do a comparison. Please remove all
comparisons, unless it is really needed. It also makes the code look
consistent. At the moment you sometime have < 0, sometime !=0, and
sometimes no comparison at all.
Andrew
Re: [PATCH V3 net-next 1/8] net: hns3: Add support of HNS3 Ethernet Driver for hip08 SoC
> +static int hns3_nic_net_up(struct net_device *ndev)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(ndev);
> + struct hnae3_handle *h = priv->ae_handle;
> + int i, j;
> + int ret;
> +
> + ret = hns3_nic_init_irq(priv);
> + if (ret != 0) {
if (ret)
No need to compare with zero.
> + netdev_err(ndev, "hns init irq failed! ret=%d\n", ret);
> + return ret;
> +static int hns3_nic_net_open(struct net_device *ndev)
> +{
> + struct hns3_nic_priv *priv = netdev_priv(ndev);
> + struct hnae3_handle *h = priv->ae_handle;
> + int ret;
> +
> + netif_carrier_off(ndev);
> +
> + ret = netif_set_real_num_tx_queues(ndev, h->kinfo.num_tqps);
> + if (ret < 0) {
> + netdev_err(ndev, "netif_set_real_num_tx_queues fail, ret=%d!\n",
> +ret);
> + return ret;
> + }
In general, functions return 0 for success, and something else for an
error. So there is no need to do a comparison. Please remove all
comparisons, unless it is really needed. It also makes the code look
consistent. At the moment you sometime have < 0, sometime !=0, and
sometimes no comparison at all.
Andrew
