Alexey Dobriyan wrote:
> On Fri, Aug 18, 2006 at 01:29:01PM +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 int ehea_refill_rq3_def(struct ehea_port_res *pr, int
nr_of_wqes)
>
> This one looks too big to be inlined as well as other similalry named
> functions.
Agreed. Inlining avoided where possible.
>> +static int ehea_clean_port_res(struct ehea_port *port, struct ehea_port_res
*pr)
>
> Freeing/deallocating/cleaning functions usually return void. The fact
> that you always return -EINVAL only reaffirmes my belief. ;-)
>
Fixed.
>> +static inline void write_swqe2_data(struct sk_buff *skb,
>> + struct net_device *dev,
>> + struct ehea_swqe *swqe,
>> + u32 lkey)
>
> Way too big.
Function split.
>> +static inline void ehea_xmit2(struct sk_buff *skb,
>> + struct net_device *dev, struct ehea_swqe *swqe,
>> + u32 lkey)
>> +
>> +static inline void ehea_xmit3(struct sk_buff *skb,
>> + struct net_device *dev, struct ehea_swqe *swqe)
>
> Ditto.
These functions are on a performance-critical path and they are called
exactly once - so the object's size isn't affected and having them inline
seems appropriate to me.
>> + if (grp)
>> + memset(cb1->vlan_filter, 0, sizeof(cb1->vlan_filter));
>> + else
>> + memset(cb1->vlan_filter, 1, sizeof(cb1->vlan_filter));
>
> Just to be sure, this should be 1 not 0xff?
>
Will be checked.
>> +void ehea_clean_all_port_res(struct ehea_port *port)
>> +{
>> + int ret;
>> + int i;
>> + for(i = 0; i < port->num_def_qps + port->num_tx_qps; i++)
>> + ehea_clean_port_res(port, &port->port_res[i]);
>> +
>> + ret = ehea_destroy_eq(port->qp_eq);
>> +}
>
> ret is entirely useless.
Correct. It's gone.
>
>> +int __init ehea_module_init(void)
> static
>
>> +{
>> + int ret = -EINVAL;
>> +
>> + printk("IBM eHEA Ethernet Device Driver (Release %s)\n", DRV_VERSION);
>> +
>> + ret = ibmebus_register_driver(&ehea_driver);
>> + if (ret) {
>> + ehea_error("failed registering eHEA device driver on ebus");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>
> Pass ret to upper layer. Simplest way is:
>
> static int __init ehea_module_init(void)
> {
> return ibmebus_register_driver(&ehea_driver);
> }
Agreed to pass ret to upper layer, but we want to keep the error message.
Code modified accordingly.
Regards
Thomas
-
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
