Krishna Gudipati wrote:
> From: Krishna Chaitanya Gudipati <kgudipat@xxxxxxxxxxx>
>
> 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
Attachment:
signature.asc
Description: This is a digitally signed message part.
