lkml.org 
[lkml]   [2006]   [Aug]   [11]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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 linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

\
 
 \ /
  Last update: 2006-08-11 13:43    [W:0.149 / U:0.380 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site