linux-kernel - Re: [2.6.19 PATCH 1/7] ehea: interface to network stack

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <44E9C8B6.7030701@de.ibm.com>
Date:	Mon, 21 Aug 2006 16:52:38 +0200
From:	Thomas Klein <osstklei@...ibm.com>
To:	Alexey Dobriyan <adobriyan@...il.com>
CC:	Jan-Bernd Themann <ossthema@...ibm.com>, netdev@...r.kernel.org,
	Christoph Raisch <raisch@...ibm.com>,
	Jan-Bernd Themann <themann@...ibm.com>,
	linux-kernel <linux-kernel@...r.kernel.org>,
	linux-ppc <linuxppc-dev@...abs.org>,
	Marcus Eder <meder@...ibm.com>,
	Thomas Klein <tklein@...ibm.com>
Subject: Re: [2.6.19 PATCH 1/7] ehea: interface to network stack

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 linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/