Date:	Thu, 2 Apr 2009 11:00:47 +0200
From:	Rolf Eike Beer <eike-kernel@...tec.de>
To:	Krishna Gudipati <kgudipat@...cade.com>
Cc:	James.Bottomley@...senpartnership.com, huangj@...cade.com,
	linux-kernel@...r.kernel.org, linux-scsi@...r.kernel.org,
	nkattang@...cade.com, rvadivel@...cade.com, vravindr@...cade.com
Subject: Re: [PATCH 1/5] bfa: Brocade BFA FC SCSI driver (bfad)

Krishna Gudipati wrote:
> From: Krishna Chaitanya Gudipati <kgudipat@...cade.com>
>
> This patch contains Brocade linux driver specific code that interfaces to
> upper layer linux kernel for PCI, SCSI mid-layer, sysfs related stuff. The
> code handles things such as module load/unload, PCI probe/release, handling
> interrupts, interfacing with sysfs etc. It interacts with the
> firmware/hardware via the code under files with prefix bfa_*.
>
> This patch also fixes the code review comments of Eike from our previous
> submission, we are still considering the suggestion to use devres for our
> driver and this patch does not have those changes.

I think I can find something else in the mean time ;)


> +void
> +bfad_flags_set(struct bfad_s *bfad, u32 flags)
> +{
> +	if (bfad)
> +		bfad->bfad_flags |= flags;
> +}

This is so trivial I doubt it needs to be a function of it's own. Also at one 
place it is already clear that bfad is valid (the other one maybe too).

> +/**
> + *  BFA callbacks
> + */
> +void
> +bfad_hcb_comp(void *arg, bfa_status_t status)
> +{
> +	struct bfad_hal_comp *fcomp = (struct bfad_hal_comp *)arg;

You don't need to cast from a void* in C.

> +	fcomp->status = status;
> +	complete(&fcomp->comp);
> +}
> +
> +/**
> + * bfa_init callback
> + */
> +void
> +bfa_cb_init(void *drv, bfa_status_t init_status)
> +{
> +	struct bfad_s         *bfad = drv;

One space, no tabs.

> +	if (init_status == BFA_STATUS_OK)
> +		bfad->bfad_flags |= BFAD_HAL_INIT_DONE;
> +
> +	complete(&bfad->comp);
> +}
> +
> +/**
> + * bfa_stop callback
> + */
> +void
> +bfa_cb_stop(void *drv, bfa_status_t stop_status)
> +{
> +	struct bfad_s         *bfad = drv;
> +
> +	/* Signal the BFAD stop waiting thread */
> +	complete(&bfad->comp);
> +}
> +
> +/**
> + * bfa_ioc_diable() callback.
> + */
> +void
> +bfa_cb_ioc_disable(void *drv)
> +{
> +	struct bfad_s         *bfad = drv;
> +
> +	complete(&bfad->disable_comp);
> +}
> +
> +void
> +bfa_cb_exit(struct bfad_s *drv)
> +{
> +	complete(&drv->comp);
> +}
> +
> +void
> +bfa_cb_rport_qos_scn_flowid(void *rport,
> +			struct bfa_rport_qos_attr_s old_qos_attr,
> +			struct bfa_rport_qos_attr_s new_qos_attr)
> +{
> +}
> +void
> +bfa_cb_rport_online(void *rport)
> +{
> +}
> +void
> +bfa_cb_rport_offline(void *rport)
> +{
> +}
> +void
> +bfa_cb_rport_qos_scn_prio(void *rport,
> +			struct bfa_rport_qos_attr_s old_qos_attr,
> +			struct bfa_rport_qos_attr_s new_qos_attr)
> +{
> +}
> +
> +void
> +bfa_cb_itnim_sler(void *itnim)
> +{
> +}

Kill those. They are unused anyway (at least the first two that I checked).

> +
> +/**
> + *  bfa callbacks
> + */
> +static struct bfad_port_s *
> +bfad_get_drv_port(struct bfad_s *bfad, struct bfad_vf_s *vf_drv,
> +			struct bfad_vport_s *vp_drv)
> +{
> +	return ((vp_drv) ? (&(vp_drv)->drv_port)
> +		: ((vf_drv) ? (&(vf_drv)->base_port) : (&(bfad)->pport)));
> +}

Some of the braces are superfluous. IMHO it would also be more readable if 
written with if-else if-else.

> +struct bfad_port_s *
> +bfa_cb_port_new(struct bfad_s *bfad, struct bfa_lport_s *port,
> +			enum bfa_port_role roles, struct bfad_vf_s *vf_drv,
> +			struct bfad_vport_s *vp_drv)
> +{
> +	bfa_status_t    rc;
> +	struct bfad_port_s    *port_drv;
> +
> +	if (!vp_drv && !vf_drv) {
> +		port_drv = &bfad->pport;
> +		port_drv->pvb_type = BFAD_PORT_PHYS_BASE;
> +	} else if (!vp_drv && vf_drv) {
> +		port_drv = &vf_drv->base_port;
> +		port_drv->pvb_type = BFAD_PORT_VF_BASE;
> +	} else if (vp_drv && !vf_drv) {
> +		port_drv = &vp_drv->drv_port;
> +		port_drv->pvb_type = BFAD_PORT_PHYS_VPORT;
> +	} else {
> +		port_drv = &vp_drv->drv_port;
> +		port_drv->pvb_type = BFAD_PORT_VF_VPORT;
> +	}
> +	port_drv->bfa_lport = port;
> +	port_drv->roles = roles;
> +	rc = bfad_fc4_port_new(bfad, port_drv, roles);
> +	if (rc != BFA_STATUS_OK) {
> +		bfad_fc4_port_delete(bfad, port_drv, roles);
> +		port_drv = NULL;
> +	}
> +
> +	return port_drv;
> +}
> +
> +void
> +bfa_cb_lport_delete(struct bfad_s *bfad, enum bfa_port_role roles,
> +			struct bfad_vf_s *vf_drv, struct bfad_vport_s *vp_drv)
> +{
> +	struct bfad_port_s    *port_drv;
> +
> +	/* this will be only called from rmmod context */
> +	if (vp_drv && !vp_drv->comp_del) {
> +		port_drv = bfad_get_drv_port(bfad, vf_drv, vp_drv);
> +		bfa_trc(bfad, roles);
> +		bfad_fc4_port_delete(bfad, port_drv, roles);
> +	}
> +}
> +
> +void
> +bfa_cb_lport_online(struct bfad_s *bfad, enum bfa_port_role roles,
> +			struct bfad_vf_s *vf_drv, struct bfad_vport_s *vp_drv)
> +{
> +	struct bfad_port_s *port_drv = bfad_get_drv_port(bfad, vf_drv, vp_drv);
> +
> +	if (roles & BFA_PORT_ROLE_FCP_IM)
> +		bfad_im_port_online(bfad, port_drv);
> +
> +	if (roles & BFA_PORT_ROLE_FCP_TM)
> +		bfad_tm_port_online(bfad, port_drv);
> +
> +	if ((roles & BFA_PORT_ROLE_FCP_IPFC) && ipfc_enable)
> +		bfad_ipfc_port_online(bfad, port_drv);
> +
> +	bfad_flags_set(bfad, BFAD_PORT_ONLINE);
> +}
> +
> +void
> +bfa_cb_lport_offline(struct bfad_s *bfad, enum bfa_port_role roles,
> +			struct bfad_vf_s *vf_drv, struct bfad_vport_s *vp_drv)
> +{
> +	struct bfad_port_s *port_drv = bfad_get_drv_port(bfad, vf_drv, vp_drv);
> +
> +	if (roles & BFA_PORT_ROLE_FCP_IM)
> +		bfad_im_port_offline(bfad, port_drv);
> +
> +	if (roles & BFA_PORT_ROLE_FCP_TM)
> +		bfad_tm_port_offline(bfad, port_drv);
> +
> +	if ((roles & BFA_PORT_ROLE_FCP_IPFC) && ipfc_enable)
> +		bfad_ipfc_port_offline(bfad, port_drv);
> +}
> +
> +void
> +bfa_cb_vf_stop(struct bfad_vf_s *vf_drv)
> +{
> +}
> +
> +void
> +bfa_cb_vport_delete(struct bfad_vport_s *vport_drv)
> +{
> +	bfa_trc(vport_drv->drv_port.bfad, (int)vport_drv->comp_del);
> +	if (vport_drv->comp_del) {
> +		complete(vport_drv->comp_del);
> +		return;
> +	}
> +
> +	kfree(vport_drv);
> +}
> +
> +/**
> + * FCS RPORT alloc callback, after successful PLOGI by FCS
> + */
> +bfa_status_t
> +bfa_cb_rport_alloc(struct bfad_s *bfad, struct bfa_rport_s *rport,
> +		    struct bfad_rport_s **rport_drv)
> +{
> +	bfa_status_t    rc = BFA_STATUS_OK;
> +
> +	*rport_drv = kzalloc(sizeof(struct bfad_rport_s), GFP_ATOMIC);

sizeof(**rport_drv): this will get you the correct size of whatever type this 
variable is.

> +	if (*rport_drv == NULL) {
> +		rc = BFA_STATUS_ENOMEM;
> +		goto ext;
> +	}
> +	(*rport_drv)->rport = rport;
> +ext:
> +	return rc;
> +}

This is a bit few code for that goto style exit.

> +/**
> + * FCS RPORT free callback.
> + */
> +void
> +bfa_cb_rport_free(struct bfad_s *bfad, struct bfad_rport_s **rport_drv)
> +{
> +	kfree(*rport_drv);
> +}

Looks unused.

> +
> +
> +

Too much whitespace.

> +void
> +bfad_hal_mem_release(struct bfad_s *bfad)
> +{
> +	int             i;
> +	struct bfa_meminfo_s *hal_meminfo = &bfad->meminfo;
> +	struct bfa_mem_elem_s *meminfo_elem;
> +
> +	for (i = 0; i < BFA_MEM_TYPE_MAX; i++) {

i < ARRAY_SIZE(hal_meminfo->meminfo)

> +		meminfo_elem = &hal_meminfo->meminfo[i];
> +		if (meminfo_elem->kva != NULL) {
> +			switch (meminfo_elem->mem_type) {
> +			case BFA_MEM_TYPE_KVA:
> +				vfree(meminfo_elem->kva);
> +				break;
> +			case BFA_MEM_TYPE_DMA:
> +				bfad_os_dma_free(bfad, meminfo_elem);
> +				break;
> +			default:
> +				bfa_assert(0);
> +				break;
> +			}
> +		}
> +	}
> +
> +	memset(hal_meminfo, 0, sizeof(struct bfa_meminfo_s));

sizeof(*hal_meminfo)

> +}
> +
> +void
> +bfad_update_hal_cfg(struct bfa_iocfc_cfg_s *bfa_cfg)
> +{
> +	if (num_rports > 0)
> +		bfa_cfg->fwcfg.num_rports = num_rports;
> +	if (num_ios > 0)
> +		bfa_cfg->fwcfg.num_ioim_reqs = num_ios;
> +	if (num_tms > 0)
> +		bfa_cfg->fwcfg.num_tskim_reqs = num_tms;
> +	if (num_fcxps > 0)
> +		bfa_cfg->fwcfg.num_fcxp_reqs = num_fcxps;
> +	if (num_ufbufs > 0)
> +		bfa_cfg->fwcfg.num_uf_bufs = num_ufbufs;
> +	if (reqq_size > 0)
> +		bfa_cfg->drvcfg.num_reqq_elems = reqq_size;
> +	if (rspq_size > 0)
> +		bfa_cfg->drvcfg.num_rspq_elems = rspq_size;
> +	if (num_sgpgs > 0)
> +		bfa_cfg->drvcfg.num_sgpgs = num_sgpgs;

What happens if num_* is 0? No need to update that value then? I mean, if 
there were more ports and then the next scan gives 0, shouldn't they be reset 
to 0? I must confess I have not checked when this is called ...

> +	/* TODO read role from config file FCS_BRINGUP*/
> +	bfa_cfg->drvcfg.bport_role = BFA_PORT_ROLE_FCP_IM;
> +
> +}
> +
> +bfa_status_t
> +bfad_hal_mem_alloc(struct bfad_s *bfad)
> +{
> +	int             i;
> +	struct bfa_meminfo_s *hal_meminfo = &bfad->meminfo;
> +	struct bfa_mem_elem_s *meminfo_elem;
> +	dma_addr_t      phys_addr;
> +	void           *kva;
> +	bfa_status_t    rc = BFA_STATUS_OK;
> +
> +	bfa_cfg_get_default(&bfad->ioc_cfg);
> +	bfad_update_hal_cfg(&bfad->ioc_cfg);
> +	bfad->cfg_data.ioc_queue_depth = bfad->ioc_cfg.fwcfg.num_ioim_reqs ;
> +	bfa_cfg_get_meminfo(&bfad->ioc_cfg, hal_meminfo);
> +
> +	for (i = 0; i < BFA_MEM_TYPE_MAX; i++) {
> +		meminfo_elem = &hal_meminfo->meminfo[i];
> +		switch (meminfo_elem->mem_type) {
> +		case BFA_MEM_TYPE_KVA:
> +			kva = vmalloc(meminfo_elem->mem_len);
> +			if (kva == NULL) {
> +				bfad_hal_mem_release(bfad);
> +				rc = BFA_STATUS_ENOMEM;
> +				goto ext;

If you goto ext here instead of just "return BFA_STATUS_ENOMEM" then you 
should add an error goto target to share with the next case. And why not use 
ENOMEM anyway?

I'm stopping here because of lacking time. What I found hard is that you split 
the headers into a different patch. In fact you need not to split that into 
pieces at all as all patches touch different code areas but are only usable 
together, noone will ever have a chance to end up in the middle of this 
patches searching for a problem in this driver. Ok, could be to reduce the 
size of the mail itself, which is ok but for the final submission.

I see people doing this all the time but I think it does harm: these commits 
add extra noise when doing git-bisect to find something and one commit by 
itself can not work as the other parts are essential to make them work at 
all.

One other thing that I find personally disturbing: you do not send your "PATCH 
n/5" mails as replies to the "PATCH 0/5" mail. This would keep the 
discussions together in peoples mailers so they are easier to follow (or 
ignore, YMMV *g*).

Anyway, thanks for your work ;) No pun intended.

Eike

Download attachment "signature.asc " of type "application/pgp-signature" (198 bytes)

Powered by blists - more mailing lists