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

[Arnd Bergmann <arnd@arndb.de>]

Am Monday 04 September 2006 22:16 schrieb Francois Romieu:
> > +#include "ehea.h"
> > +#include "ehea_qmr.h"
> > +#include "ehea_phyp.h"
>
> Afaik none of those is included in this patch nor in my 2.6.18-git tree.


They are in 5, 3 and 2, respectively

> Happy bissect in sight.

The driver should get merged as a single commit anyway, even
if split diffs are posted for review. Even if it gets merged
like this, bisect will work since the Kconfig option is added
in the final patch.

> > +
> > +static struct net_device_stats *ehea_get_stats(struct net_device *dev)
> > +{
> > +     struct ehea_port *port = netdev_priv(dev);
> > +     struct net_device_stats *stats = &port->stats;
> > +     struct hcp_ehea_port_cb2 *cb2;
> > +     u64 hret, rx_packets;
> > +     int i;
>
> unsigned int ?

does it matter? int as a counter is pretty standard.

> > +
> > +     if (netif_msg_hw(port))
> > +             ehea_dump(cb2, sizeof(*cb2), "net_device_stats");
> > +
> > +     rx_packets = 0;
>
> Could be initialized when it is declared.
>
> > +     for (i = 0; i < port->num_def_qps; i++)
> > +             rx_packets += port->port_res[i].rx_packets;

In one of the previous reviews, we told them to do it this way
instead. Initializing at declaration is error-prone.

> > +
> > +     intreq = ((pr->p_state.ehea_poll & 0xF) == 0xF);
>
> Arguable parenthesis.
>

I'd argue to keep them ;-)

> > +
> > +     hret = ehea_h_modify_ehea_port(port->adapter->handle,
> > +                                    port->logical_port_id,
> > +                                    H_PORT_CB0, mask, cb0);
> > +     if (hret != H_SUCCESS) {
> > +             ret = -EIO;
>
> Why can't ehea_xyz return -EIO/0 directly ?
>

the lowest-level hypercall should return H_* by convention.

Then again, it should also be called plpar_modify_ehea_port()
in that case.

> > +static int ehea_start_xmit(struct sk_buff *skb, struct net_device *dev)
> > +{
> > +     struct ehea_port *port = netdev_priv(dev);
> > +     struct ehea_port_res *pr;
> > +     struct ehea_swqe *swqe;
> > +     unsigned long flags;
> > +     u32 lkey;
> > +     int swqe_index;
> > +
> > +     pr = &port->port_res[0];
>
> Initialization and declaration can happen at the same time.

it's a gray area. In general, I recommend not to combine them
at all. Initialization to NULL or 0 is always bad, this one
is harmless, but I'd still leave it this way, especially after
telling them to clean this up earlier ;-)

	Arnd <><