Re: [PATCH 1/6] ehea: interface to network stack
On Monday 14 August 2006 17:43, Jan-Bernd Themann wrote:
> as our queue size is always a power of 2, we simply use:
> i++;
> i &= (ringbufferlength - 1)
>
> So we can get along without the if.
>
The recommended (by Linus) way for dealing with ring buffers
like that is to always read the counter through an accessor
and don't care about the overflow when updating it.
You can write small access functions for that:
struct my_struct {
...
unsigned rbuf_index;
unsigned rbuf_mask;
...
};
static inline unsigned int my_index(struct my_struct *p)
{
return p->rb_index & p->rb_mask;
}
static inline unsigned int my_index_next(struct my_struct *p)
{
return (++p->rb_index) & p->rb_mask;
}
Arnd <><
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] ehea: interface to network stack
Hi, Anton Blanchard wrote: Is a conditional cheaper than a divide? In case of a misprediction I would assume it to be significantly slower and I don't know the ratio of mispredictions for this branch. A quick scan of the web shows 40 cycles for athlon64 idiv, and its similarly slow on many other cpus. Even assuming you mispredict every branch its going to be a win. Anton as our queue size is always a power of 2, we simply use: i++; i &= (ringbufferlength - 1) So we can get along without the if. Jan-Bernd - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] ehea: interface to network stack
> Is a conditional cheaper than a divide? In case of a misprediction I > would assume it to be significantly slower and I don't know the ratio > of mispredictions for this branch. A quick scan of the web shows 40 cycles for athlon64 idiv, and its similarly slow on many other cpus. Even assuming you mispredict every branch its going to be a win. Anton - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] ehea: interface to network stack
On Sat, 12 August 2006 06:56:24 +1000, Anton Blanchard wrote: > > > + > > + skb_index = ((index - i > > + + port_res->skb_arr_sq_len) > > +% port_res->skb_arr_sq_len); > > This is going to force an expensive divide. Its much better to change > this to the simpler and quicker: > > i++; > if (i > max) > i = 0; Is a conditional cheaper than a divide? In case of a misprediction I would assume it to be significantly slower and I don't know the ratio of mispredictions for this branch. Jörn -- Victory in war is not repetitious. -- Sun Tzu - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] ehea: interface to network stack
Hi, > --- linux-2.6.18-rc4-orig/drivers/net/ehea/ehea_main.c1969-12-31 > +#define DEB_PREFIX "main" Doesnt appear to be used. > +static struct net_device_stats *ehea_get_stats(struct net_device *dev) ... > + cb2 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL); I cant see where this gets freed. > + > + skb_index = ((index - i > + + port_res->skb_arr_sq_len) > + % port_res->skb_arr_sq_len); This is going to force an expensive divide. Its much better to change this to the simpler and quicker: i++; if (i > max) i = 0; There are a few places in the driver can be changed to do this. > +static int ehea_setup_single_port(struct ehea_adapter *adapter,A > + int portnum, struct device_node *dn) ... > + cb4 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL); I cant see where this is freed. Anton - To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] ehea: interface to network stack
Hi Christian,
thanks for your comments, we'll send an updated patch set soon.
Jan-Bernd
Christian Borntraeger wrote:
Hi Jan-Bernd,
I had some minutes, here are some finding after a quick look.
On Wednesday 09 August 2006 10:38, you wrote:
+static struct net_device_stats *ehea_get_stats(struct net_device *dev)
+{
+ int i;
+ u64 hret = H_HARDWARE;
+ u64 rx_packets = 0;
+ struct ehea_port *port = (struct ehea_port*)dev->priv;
dev->priv is a void pointer, this cast is unnecessary. When we are at it, have
you considered the netdev_priv macro? This will require some prep in
alloc_netdev and might not always pe possible.
good point, we'll use alloc_etherdev / netdev_priv
+
+ EDEB_DMP(7, (u8*)cb2,
+sizeof(struct hcp_query_ehea_port_cb_2), "After HCALL");
+
+ for (i = 0; i < port->num_def_qps; i++) {
+ rx_packets += port->port_res[i].rx_packets;
+ }
+
+ stats->tx_packets = cb2->txucp + cb2->txmcp + cb2->txbcp;
+ stats->multicast = cb2->rxmcp;
+ stats->rx_errors = cb2->rxuerr;
+ stats->rx_bytes = cb2->rxo;
+ stats->tx_bytes = cb2->txo;
+ stats->rx_packets = rx_packets;
+
+get_stat_exit:
+ EDEB_EX(7, "");
+ return stats;
+}
again, cb2 is not freed.
[...]
yep, done
+static inline u64 get_swqe_addr(u64 tmp_addr, int addr_seg)
+{
+ u64 addr;
+ addr = tmp_addr;
+ return addr;
+}
This is suppsed to change in the future? If not you can get rid of it.
+
+static inline u64 get_rwqe_addr(u64 tmp_addr)
+{
+ return tmp_addr;
+}
same here.
removed
+ ehea_poll()
The poll function seems too long and therefore hard to review. Please consider
splitting it.
done
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] ehea: interface to network stack
Hi,
thanks for your comments!
We'll post a modified patch very soon.
Jan-Bernd
Alexey Dobriyan wrote:
On Wed, Aug 09, 2006 at 10:38:20AM +0200, Jan-Bernd Themann wrote:
--- linux-2.6.18-rc4-orig/drivers/net/ehea/ehea_main.c
+++ kernel/drivers/net/ehea/ehea_main.c
+static inline u64 get_swqe_addr(u64 tmp_addr, int addr_seg)
+{
+ u64 addr;
+ addr = tmp_addr;
+ return addr;
+}
+
+static inline u64 get_rwqe_addr(u64 tmp_addr)
+{
+ return tmp_addr;
+}
The point of this exercise?
has been removed
+static inline int ehea_refill_rq3_def(struct ehea_port_res *pr, int nr_of_wqes)
Way too big to be inline function.
+{
+ int i;
+ int ret = 0;
+ struct ehea_qp *qp;
+ struct ehea_rwqe *rwqe;
+ int skb_arr_rq3_len = pr->skb_arr_rq3_len;
+ struct sk_buff **skb_arr_rq3 = pr->skb_arr_rq3;
+ EDEB_EN(8, "pr=%p, nr_of_wqes=%d", pr, nr_of_wqes);
+ if (nr_of_wqes == 0)
+ return -EINVAL;
+ qp = pr->qp;
+ for (i = 0; i < nr_of_wqes; i++) {
+ int index = pr->skb_rq3_index++;
+ struct sk_buff *skb = dev_alloc_skb(EHEA_MAX_PACKET_SIZE
+ + NET_IP_ALIGN);
+
+ if (!skb) {
+ EDEB_ERR(4, "No memory for skb. Only %d rwqe
filled.",
+i);
+ ret = -ENOMEM;
+ break;
+ }
+ skb_reserve(skb, NET_IP_ALIGN);
+
+ rwqe = ehea_get_next_rwqe(qp, 3);
+ pr->skb_rq3_index %= skb_arr_rq3_len;
+ skb_arr_rq3[index] = skb;
+ rwqe->wr_id = EHEA_BMASK_SET(EHEA_WR_ID_TYPE,
EHEA_RWQE3_TYPE)
+ | EHEA_BMASK_SET(EHEA_WR_ID_INDEX, index);
+ rwqe->sg_list[0].l_key = ehea_get_recv_lkey(pr);
+ rwqe->sg_list[0].vaddr = get_rwqe_addr((u64)skb->data);
+ rwqe->sg_list[0].len = EHEA_MAX_PACKET_SIZE;
+ rwqe->data_segments = 1;
+ }
+
+ /* Ring doorbell */
+ iosync();
+ ehea_update_rq3a(qp, i);
+ EDEB_EX(8, "");
+ return ret;
+}
+
+
+static inline int ehea_refill_rq3(struct ehea_port_res *pr, int nr_of_wqes)
+{
+ return ehea_refill_rq3_def(pr, nr_of_wqes);
+}
ehea_refill_rq3[123] appears to be 1:1 wrappers around
ehea_refill_rq3[123]_def. Any idea behind them?
introduced for near future features
+ init_attr = (struct ehea_qp_init_attr*)
+ kzalloc(sizeof(struct ehea_qp_init_attr), GFP_KERNEL);
Useless cast.
removed
+ pr->skb_arr_sq = (struct sk_buff**)vmalloc(sizeof(struct sk_buff*)
+ * (max_rq_entries + 1));
Useless cast
removed
+ pr->skb_arr_rq1 = (struct sk_buff**)vmalloc(sizeof(struct sk_buff*)
+* (max_rq_entries + 1));
+ pr->skb_arr_rq2 = (struct sk_buff**)vmalloc(sizeof(struct sk_buff*)
+* (max_rq_entries + 1));
+ pr->skb_arr_rq3 = (struct sk_buff**)vmalloc(sizeof(struct sk_buff*)
+* (max_rq_entries + 1));
+static int ehea_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
+{
+ EDEB_ERR(4, "ioctl not supported: dev=%s cmd=%d", dev->name, cmd);
Then copy NULL into ->do_ioctl!
done
+ return -EOPNOTSUPP;
+}
+ ehea_port_cb_0 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL);
+
+ if (!ehea_port_cb_0) {
+ EDEB_ERR(4, "No memory for ehea_port control block");
+ ret = -ENOMEM;
+ goto kzalloc_failed;
+ }
+
+ memcpy((u8*)(&(ehea_port_cb_0->port_mac_addr)),
+ (u8*)&(mac_addr->sa_data[0]), 6);
No casts on memcpy arguments.
done
+ memcpy((u8*)&ehea_mcl_entry->macaddr, mc_mac_addr, ETH_ALEN);
+static inline void ehea_xmit2(struct sk_buff *skb,
+ struct net_device *dev, struct ehea_swqe *swqe,
+ u32 lkey)
+{
+ int nfrags;
+ unsigned short skb_protocol = skb->protocol;
Useless variable. And it should be __be16, FYI.
changed
+ nfrags = skb_shinfo(skb)->nr_frags;
+ EDEB_EN(7, "skb->nfrags=%d (0x%X)", nfrags, nfrags);
+
+ if (skb_protocol == ETH_P_IP) {
ITYM, htons(ETH_P_IP).
good point, thx
+static inline void ehea_xmit3(struct sk_buff *skb,
+ struct net_device *dev, struct ehea_swqe *swqe)
+{
+ int i;
+ skb_frag_t *frag;
+ int nfrags = skb_shinfo(skb)->nr_frags;
+ u8 *imm_data = &swqe->u.immdata_nodesc.immediate_data[0];
+ u64 skb_protocol = skb->protocol;
Useless var.
removed
+
+ EDEB_EN(7, "");
+ if (likely(skb_protocol == ETH_P_IP)) {
htons(ETH_P_IP)
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
Re: [PATCH 1/6] ehea: interface to network stack
Hi Michael,
thanks for your very helpful comments so far, we'll provide a patch with these
and other fixes
very soon.
See comments below.
Jan-Bernd
Michael Ellerman wrote:
> Hi Jan-Bernd,
>
> Comments below the code they refer to.
>
> On Wed, 2006-08-09 at 10:38 +0200, Jan-Bernd Themann wrote:
>> Signed-off-by: Jan-Bernd Themann <[EMAIL PROTECTED]>
>
>> drivers/net/ehea/ehea_main.c | 2738
+++
>> 1 file changed, 2738 insertions(+)
>
>> --- linux-2.6.18-rc4-orig/drivers/net/ehea/ehea_main.c 1969-12-31
16:00:00.0 -0800
>> +++ kernel/drivers/net/ehea/ehea_main.c2006-08-08 23:59:39.683357016
-0700
>> @@ -0,0 +1,2738 @@
>> +/*
>> + * linux/drivers/net/ehea/ehea_main.c
>
> Putting the file name in the file is fairly redundant IMHO.
>
>> + * eHEA ethernet device driver for IBM eServer System p
>
> What's the actual hardware that this is for? System p covers a whole
> range of machines, do they really all support this driver?
>
>> + * (C) Copyright IBM Corp. 2006
>> + *
>> + * Authors:
>> + * Christoph Raisch <[EMAIL PROTECTED]>
>> + * Jan-Bernd Themann <[EMAIL PROTECTED]>
>> + * Heiko-Joerg Schick <[EMAIL PROTECTED]>
>> + * Thomas Klein <[EMAIL PROTECTED]>
>> + *
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2, or (at your option)
>> + * any later version.
>> + *
>> + * This program is distributed in the hope that it will be useful,
>> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
>> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.See the
>> + * GNU General Public License for more details.
>> + *
>> + * You should have received a copy of the GNU General Public License
>> + * along with this program; if not, write to the Free Software
>> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
>> + */
>> +
>> +#define DEB_PREFIX "main"
>> +
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +#include
>> +
>> +#include "ehea.h"
>> +#include "ehea_qmr.h"
>> +#include "ehea_phyp.h"
>> +
>> +
>> +MODULE_LICENSE("GPL");
>> +MODULE_AUTHOR("Christoph Raisch <[EMAIL PROTECTED]>");
>> +MODULE_DESCRIPTION("IBM eServer HEA Driver");
>> +MODULE_VERSION(EHEA_DRIVER_VERSION);
>> +
>> +static int __devinit ehea_probe(struct ibmebus_dev *dev,
>> + const struct of_device_id *id);
>> +static int __devexit ehea_remove(struct ibmebus_dev *dev);
>> +static int ehea_sense_port_attr(struct ehea_adapter *adapter, int portnum);
>
> I haven't looked closely, but can you rearrange the functions so you
> don't need these forward declarations?
yes, rearrangement is possible. Done.
>
>> +int ehea_trace_level = 5;
>> +
>> +static struct net_device_stats *ehea_get_stats(struct net_device *dev)
>> +{
>> + int i;
>> + u64 hret = H_HARDWARE;
>
> You unconditionally assign to hret below.
>
>> + u64 rx_packets = 0;
>
> Why not just update stats->rx_packets directly?
>
>> + struct ehea_port *port = (struct ehea_port*)dev->priv;
>> + struct ehea_adapter *adapter = port->adapter;
>
> I don't think you need adapter, you only use it in one place, just
> access it through port->adapter->handle (below).
done
>
>> + struct hcp_query_ehea_port_cb_2 *cb2 = NULL;
>> + struct net_device_stats *stats = &port->stats;
>> +
>> + EDEB_EN(7, "net_device=%p", dev);
>> +
>> + cb2 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL);
>> + if (!cb2) {
>> + EDEB_ERR(4, "No memory for cb2");
>> + goto get_stat_exit;
>
> You leak cb2 here.
>
done
>> + }
>> +
>> + hret = ehea_h_query_ehea_port(adapter->handle,
>> +port->logical_port_id,
>> +H_PORT_CB2,
>> +H_PORT_CB2_ALL,
>> +cb2);
>> +
>> + if (hret != H_SUCCESS) {
>> + EDEB_ERR(4, "query_ehea_port failed for cb2");
>> + goto get_stat_exit;
>> + }
>> +
>> + EDEB_DMP(7, (u8*)cb2,
>> + sizeof(struct hcp_query_ehea_port_cb_2), "After HCALL");
>> +
>> + for (i = 0; i < port->num_def_qps; i++) {
>> + rx_packets += port->port_res[i].rx_packets;
>> + }
>> +
>> + stats->tx_packets = cb2->txucp + cb2->txmcp + cb2->txbcp;
>> + stats->multicast = cb2->rxmcp;
>> + stats->rx_errors = cb2->rxuerr;
>> + stats->rx_bytes = cb2->rxo;
>> + stats->tx_bytes = cb2->txo;
>> + stats->rx_packets = rx_packets;
>> +
>> +get_stat_exit:
>> + EDEB_EX(7, "");
>> + return stats;
>> +}
>> +
>> +static inline u32 ehea_get_send_lkey(struct ehea_port_res *pr)
>> +{
>> + return pr->send_mr.lkey;
>> +}
>
> Get rid of this, it's only used once.
>
done
>> +static
Re: [PATCH 1/6] ehea: interface to network stack
On Thu, 2006-08-10 at 16:15 +1000, Michael Ellerman wrote:
> > + struct hcp_query_ehea_port_cb_2 *cb2 = NULL;
> > + struct net_device_stats *stats = &port->stats;
> > +
> > + EDEB_EN(7, "net_device=%p", dev);
> > +
> > + cb2 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL);
> > + if (!cb2) {
> > + EDEB_ERR(4, "No memory for cb2");
> > + goto get_stat_exit;
>
> You leak cb2 here.
>
> > + }
> > +
> > + hret = ehea_h_query_ehea_port(adapter->handle,
> > + port->logical_port_id,
> > + H_PORT_CB2,
> > + H_PORT_CB2_ALL,
> > + cb2);
> > +
> > + if (hret != H_SUCCESS) {
> > + EDEB_ERR(4, "query_ehea_port failed for cb2");
> > + goto get_stat_exit;
Sorry, here.
cheers
--
Michael Ellerman
IBM OzLabs
wwweb: http://michael.ellerman.id.au
phone: +61 2 6212 1183 (tie line 70 21183)
We do not inherit the earth from our ancestors,
we borrow it from our children. - S.M.A.R.T Person
signature.asc
Description: This is a digitally signed message part
Re: [PATCH 1/6] ehea: interface to network stack
Hi Jan-Bernd,
Comments below the code they refer to.
On Wed, 2006-08-09 at 10:38 +0200, Jan-Bernd Themann wrote:
> Signed-off-by: Jan-Bernd Themann <[EMAIL PROTECTED]>
> drivers/net/ehea/ehea_main.c | 2738
> +++
> 1 file changed, 2738 insertions(+)
> --- linux-2.6.18-rc4-orig/drivers/net/ehea/ehea_main.c1969-12-31
> 16:00:00.0 -0800
> +++ kernel/drivers/net/ehea/ehea_main.c 2006-08-08 23:59:39.683357016
> -0700
> @@ -0,0 +1,2738 @@
> +/*
> + * linux/drivers/net/ehea/ehea_main.c
Putting the file name in the file is fairly redundant IMHO.
> + * eHEA ethernet device driver for IBM eServer System p
What's the actual hardware that this is for? System p covers a whole
range of machines, do they really all support this driver?
> + * (C) Copyright IBM Corp. 2006
> + *
> + * Authors:
> + * Christoph Raisch <[EMAIL PROTECTED]>
> + * Jan-Bernd Themann <[EMAIL PROTECTED]>
> + * Heiko-Joerg Schick <[EMAIL PROTECTED]>
> + * Thomas Klein <[EMAIL PROTECTED]>
> + *
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2, or (at your option)
> + * any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> + */
> +
> +#define DEB_PREFIX "main"
> +
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +#include "ehea.h"
> +#include "ehea_qmr.h"
> +#include "ehea_phyp.h"
> +
> +
> +MODULE_LICENSE("GPL");
> +MODULE_AUTHOR("Christoph Raisch <[EMAIL PROTECTED]>");
> +MODULE_DESCRIPTION("IBM eServer HEA Driver");
> +MODULE_VERSION(EHEA_DRIVER_VERSION);
> +
> +static int __devinit ehea_probe(struct ibmebus_dev *dev,
> + const struct of_device_id *id);
> +static int __devexit ehea_remove(struct ibmebus_dev *dev);
> +static int ehea_sense_port_attr(struct ehea_adapter *adapter, int portnum);
I haven't looked closely, but can you rearrange the functions so you
don't need these forward declarations?
> +int ehea_trace_level = 5;
> +
> +static struct net_device_stats *ehea_get_stats(struct net_device *dev)
> +{
> + int i;
> + u64 hret = H_HARDWARE;
You unconditionally assign to hret below.
> + u64 rx_packets = 0;
Why not just update stats->rx_packets directly?
> + struct ehea_port *port = (struct ehea_port*)dev->priv;
> + struct ehea_adapter *adapter = port->adapter;
I don't think you need adapter, you only use it in one place, just
access it through port->adapter->handle (below).
> + struct hcp_query_ehea_port_cb_2 *cb2 = NULL;
> + struct net_device_stats *stats = &port->stats;
> +
> + EDEB_EN(7, "net_device=%p", dev);
> +
> + cb2 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL);
> + if (!cb2) {
> + EDEB_ERR(4, "No memory for cb2");
> + goto get_stat_exit;
You leak cb2 here.
> + }
> +
> + hret = ehea_h_query_ehea_port(adapter->handle,
> + port->logical_port_id,
> + H_PORT_CB2,
> + H_PORT_CB2_ALL,
> + cb2);
> +
> + if (hret != H_SUCCESS) {
> + EDEB_ERR(4, "query_ehea_port failed for cb2");
> + goto get_stat_exit;
> + }
> +
> + EDEB_DMP(7, (u8*)cb2,
> + sizeof(struct hcp_query_ehea_port_cb_2), "After HCALL");
> +
> + for (i = 0; i < port->num_def_qps; i++) {
> + rx_packets += port->port_res[i].rx_packets;
> + }
> +
> + stats->tx_packets = cb2->txucp + cb2->txmcp + cb2->txbcp;
> + stats->multicast = cb2->rxmcp;
> + stats->rx_errors = cb2->rxuerr;
> + stats->rx_bytes = cb2->rxo;
> + stats->tx_bytes = cb2->txo;
> + stats->rx_packets = rx_packets;
> +
> +get_stat_exit:
> + EDEB_EX(7, "");
> + return stats;
> +}
> +
> +static inline u32 ehea_get_send_lkey(struct ehea_port_res *pr)
> +{
> + return pr->send_mr.lkey;
> +}
Get rid of this, it's only used once.
> +static inline u32 ehea_get_recv_lkey(struct ehea_port_res *pr)
> +{
> + return pr->recv_mr.lkey;
> +}
And this one only twice? Is it really useful?
> +
> +#define EHEA_OD_ADDR(address, segment) (((address) & (PAGE_SIZE - 1)) \
> + | ((segment) << 12));
> +
> +static inline u64 get_swqe_addr(u64 tmp_addr, int addr_seg)
> +{
> + u64 addr;
> + addr = tmp_addr;
> + return addr;
Re: [PATCH 1/6] ehea: interface to network stack
On Wed, Aug 09, 2006 at 10:38:20AM +0200, Jan-Bernd Themann wrote:
> --- linux-2.6.18-rc4-orig/drivers/net/ehea/ehea_main.c
> +++ kernel/drivers/net/ehea/ehea_main.c
> +static inline u64 get_swqe_addr(u64 tmp_addr, int addr_seg)
> +{
> + u64 addr;
> + addr = tmp_addr;
> + return addr;
> +}
> +
> +static inline u64 get_rwqe_addr(u64 tmp_addr)
> +{
> + return tmp_addr;
> +}
The point of this exercise?
> +static inline int ehea_refill_rq3_def(struct ehea_port_res *pr, int
> nr_of_wqes)
Way too big to be inline function.
> +{
> + int i;
> + int ret = 0;
> + struct ehea_qp *qp;
> + struct ehea_rwqe *rwqe;
> + int skb_arr_rq3_len = pr->skb_arr_rq3_len;
> + struct sk_buff **skb_arr_rq3 = pr->skb_arr_rq3;
> + EDEB_EN(8, "pr=%p, nr_of_wqes=%d", pr, nr_of_wqes);
> + if (nr_of_wqes == 0)
> + return -EINVAL;
> + qp = pr->qp;
> + for (i = 0; i < nr_of_wqes; i++) {
> + int index = pr->skb_rq3_index++;
> + struct sk_buff *skb = dev_alloc_skb(EHEA_MAX_PACKET_SIZE
> + + NET_IP_ALIGN);
> +
> + if (!skb) {
> + EDEB_ERR(4, "No memory for skb. Only %d rwqe
> filled.",
> + i);
> + ret = -ENOMEM;
> + break;
> + }
> + skb_reserve(skb, NET_IP_ALIGN);
> +
> + rwqe = ehea_get_next_rwqe(qp, 3);
> + pr->skb_rq3_index %= skb_arr_rq3_len;
> + skb_arr_rq3[index] = skb;
> + rwqe->wr_id = EHEA_BMASK_SET(EHEA_WR_ID_TYPE,
> EHEA_RWQE3_TYPE)
> + | EHEA_BMASK_SET(EHEA_WR_ID_INDEX, index);
> + rwqe->sg_list[0].l_key = ehea_get_recv_lkey(pr);
> + rwqe->sg_list[0].vaddr = get_rwqe_addr((u64)skb->data);
> + rwqe->sg_list[0].len = EHEA_MAX_PACKET_SIZE;
> + rwqe->data_segments = 1;
> + }
> +
> + /* Ring doorbell */
> + iosync();
> + ehea_update_rq3a(qp, i);
> + EDEB_EX(8, "");
> + return ret;
> +}
> +
> +
> +static inline int ehea_refill_rq3(struct ehea_port_res *pr, int nr_of_wqes)
> +{
> + return ehea_refill_rq3_def(pr, nr_of_wqes);
> +}
ehea_refill_rq3[123] appears to be 1:1 wrappers around
ehea_refill_rq3[123]_def. Any idea behind them?
> + init_attr = (struct ehea_qp_init_attr*)
> + kzalloc(sizeof(struct ehea_qp_init_attr), GFP_KERNEL);
Useless cast.
> + pr->skb_arr_sq = (struct sk_buff**)vmalloc(sizeof(struct sk_buff*)
> + * (max_rq_entries + 1));
Useless cast
> + pr->skb_arr_rq1 = (struct sk_buff**)vmalloc(sizeof(struct sk_buff*)
> + * (max_rq_entries + 1));
> + pr->skb_arr_rq2 = (struct sk_buff**)vmalloc(sizeof(struct sk_buff*)
> + * (max_rq_entries + 1));
> + pr->skb_arr_rq3 = (struct sk_buff**)vmalloc(sizeof(struct sk_buff*)
> + * (max_rq_entries + 1));
> +static int ehea_ioctl(struct net_device *dev, struct ifreq *ifr, int cmd)
> +{
> + EDEB_ERR(4, "ioctl not supported: dev=%s cmd=%d", dev->name, cmd);
Then copy NULL into ->do_ioctl!
> + return -EOPNOTSUPP;
> +}
> + ehea_port_cb_0 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL);
> +
> + if (!ehea_port_cb_0) {
> + EDEB_ERR(4, "No memory for ehea_port control block");
> + ret = -ENOMEM;
> + goto kzalloc_failed;
> + }
> +
> + memcpy((u8*)(&(ehea_port_cb_0->port_mac_addr)),
> +(u8*)&(mac_addr->sa_data[0]), 6);
No casts on memcpy arguments.
> + memcpy((u8*)&ehea_mcl_entry->macaddr, mc_mac_addr, ETH_ALEN);
> +static inline void ehea_xmit2(struct sk_buff *skb,
> + struct net_device *dev, struct ehea_swqe *swqe,
> + u32 lkey)
> +{
> + int nfrags;
> + unsigned short skb_protocol = skb->protocol;
Useless variable. And it should be __be16, FYI.
> + nfrags = skb_shinfo(skb)->nr_frags;
> + EDEB_EN(7, "skb->nfrags=%d (0x%X)", nfrags, nfrags);
> +
> + if (skb_protocol == ETH_P_IP) {
ITYM, htons(ETH_P_IP).
> +static inline void ehea_xmit3(struct sk_buff *skb,
> + struct net_device *dev, struct ehea_swqe *swqe)
> +{
> + int i;
> + skb_frag_t *frag;
> + int nfrags = skb_shinfo(skb)->nr_frags;
> + u8 *imm_data = &swqe->u.immdata_nodesc.immediate_data[0];
> + u64 skb_protocol = skb->protocol;
Useless var.
> +
> + EDEB_EN(7, "");
> + if (likely(skb_protocol == ETH_P_IP)) {
htons(ETH_P_IP)
-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] ehea: interface to network stack
Hi Jan-Bernd,
I had some minutes, here are some finding after a quick look.
On Wednesday 09 August 2006 10:38, you wrote:
> +static struct net_device_stats *ehea_get_stats(struct net_device *dev)
> +{
> + int i;
> + u64 hret = H_HARDWARE;
> + u64 rx_packets = 0;
> + struct ehea_port *port = (struct ehea_port*)dev->priv;
dev->priv is a void pointer, this cast is unnecessary. When we are at it, have
you considered the netdev_priv macro? This will require some prep in
alloc_netdev and might not always pe possible.
> + struct ehea_adapter *adapter = port->adapter;
> + struct hcp_query_ehea_port_cb_2 *cb2 = NULL;
> + struct net_device_stats *stats = &port->stats;
> +
> + EDEB_EN(7, "net_device=%p", dev);
> +
> + cb2 = kzalloc(H_CB_ALIGNMENT, GFP_KERNEL);
> + if (!cb2) {
> + EDEB_ERR(4, "No memory for cb2");
> + goto get_stat_exit;
> + }
> +
> + hret = ehea_h_query_ehea_port(adapter->handle,
> + port->logical_port_id,
> + H_PORT_CB2,
> + H_PORT_CB2_ALL,
> + cb2);
> +
> + if (hret != H_SUCCESS) {
> + EDEB_ERR(4, "query_ehea_port failed for cb2");
> + goto get_stat_exit;
> + }
You leak memory here, dont you? (cb2 points to allocated memory and you are in
an error path.)
> +
> + EDEB_DMP(7, (u8*)cb2,
> + sizeof(struct hcp_query_ehea_port_cb_2), "After HCALL");
> +
> + for (i = 0; i < port->num_def_qps; i++) {
> + rx_packets += port->port_res[i].rx_packets;
> + }
> +
> + stats->tx_packets = cb2->txucp + cb2->txmcp + cb2->txbcp;
> + stats->multicast = cb2->rxmcp;
> + stats->rx_errors = cb2->rxuerr;
> + stats->rx_bytes = cb2->rxo;
> + stats->tx_bytes = cb2->txo;
> + stats->rx_packets = rx_packets;
> +
> +get_stat_exit:
> + EDEB_EX(7, "");
> + return stats;
> +}
again, cb2 is not freed.
[...]
> +static inline u64 get_swqe_addr(u64 tmp_addr, int addr_seg)
> +{
> + u64 addr;
> + addr = tmp_addr;
> + return addr;
> +}
This is suppsed to change in the future? If not you can get rid of it.
> +
> +static inline u64 get_rwqe_addr(u64 tmp_addr)
> +{
> + return tmp_addr;
> +}
same here.
> +
> +static int ehea_poll(struct net_device *dev, int *budget)
> +{
> + struct ehea_port *port = (struct ehea_port*)dev->priv;
Again. no cast, maybe netdev_priv macro.
> + struct ehea_port_res *port_res = &port->port_res[0];
> + struct ehea_cqe *cqe;
> + struct ehea_qp *qp = port_res->qp;
> + int wqe_index = 0;
> + int last_wqe_index = 0;
> + int x = 0;
> + int processed = 0;
> + int processed_RQ1 = 0;
> + int processed_RQ2 = 0;
> + int processed_RQ3 = 0;
> + int rq;
> + int intreq;
> + struct sk_buff **skb_arr_rq1 = port_res->skb_arr_rq1;
> + struct sk_buff **skb_arr_rq2 = port_res->skb_arr_rq2;
> + struct sk_buff **skb_arr_rq3 = port_res->skb_arr_rq3;
> + int skb_arr_rq1_len = port_res->skb_arr_rq1_len;
> + int my_quota = min(*budget, dev->quota);
> +
> + EDEB_EN(7, "dev=%p, port_res=%p, budget=%d, quota=%d, qp_nr=%x",
> + dev, port_res, *budget, dev->quota,
> + port_res->qp->init_attr.qp_nr);
> + my_quota = min(my_quota, EHEA_MAX_RWQE);
> +
> + /* rq0 is low latency RQ */
> + cqe = ehea_poll_rq1(qp, &wqe_index);
> + while ((my_quota > 0) && cqe) {
> + ehea_inc_rq1(qp);
> + processed_RQ1++;
> + processed++;
> + my_quota--;
> +
> + EDEB_DMP(6, (u8*)cqe, 4 * 16, "CQE");
> + last_wqe_index = wqe_index;
> + rmb();
> + if (!ehea_check_cqe(cqe, &rq)) {
> + struct sk_buff *skb;
> + if (rq == 1) { /* LL RQ1 */
> + void *pref;
> +
> + x = (wqe_index + 1) % skb_arr_rq1_len;
> + pref = (void*)skb_arr_rq1[x];
> + prefetchw(pref);
> + prefetchw(pref + EHEA_CACHE_LINE);
> +
> + x = (wqe_index + 1) % skb_arr_rq1_len;
> + pref = (void*)(skb_arr_rq1[x]->data);
> + prefetchw(pref);
> + prefetchw(pref + EHEA_CACHE_LINE);
> +
> + skb = skb_arr_rq1[wqe_index];
> + if (unlikely(!skb)) {
> + EDEB_ERR(4, "LL SBK=NULL, wqe_index=%d",
> + wqe_index);
> + skb = dev_alloc_skb(EHEA_LL_PKT_SIZE);
> + if (!skb)
> + panic("Alloc SKB failed");
> + }
> +
