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
| ||
|
Date: Tue, 13 Oct 2015 04:08:31 +0000 From: Yuval Mintz <Yuval.Mintz@...gic.com> To: Francois Romieu <romieu@...zoreil.com> CC: netdev <netdev@...r.kernel.org>, Ariel Elior <Ariel.Elior@...gic.com> Subject: RE: [PATCH net-next v5 01/10] qed: Add module with basic common support > 500 ko of a basic something is mildly reviewable for mere mortals. Undertood, obviously. Thanks for the time you've put into this. > [...] > > +/* forward */ > > +struct qed_ptt_pool; > > +struct qed_spq; > > +struct qed_sb_info; > > +struct qed_sb_attn_info; > > +struct qed_cxt_mngr; > > +struct qed_sb_sp_info; > > +struct qed_mcp_info; Could have been solved by adding additional header files. But given the high number of files already introduced by this, the thinking was that a couple of forward declerations was better than adding lots of new header files to prevent this sort of dependency. > [...] > > +struct qed_simd_fp_handler { > > + void *token; > > + void (*func)(void *); > > +}; > Use union * ? The token is a cookie to be used by a func, so union isn't appropriate. > [...] > > +static int qed_ilt_shadow_alloc(struct qed_hwfn *p_hwfn) > > +{ > > + struct qed_cxt_mngr *p_mngr = p_hwfn->p_cxt_mngr; > > + struct qed_ilt_client_cfg *clients = p_mngr->clients; > > + struct qed_ilt_cli_blk *p_blk; > > + u32 size, i, j; > > + int rc; > > + > > + size = qed_cxt_ilt_shadow_size(clients); > > + p_mngr->ilt_shadow = kcalloc(size, sizeof(struct qed_dma_mem), > > + GFP_KERNEL); > > + if (!p_mngr->ilt_shadow) { > > + DP_NOTICE(p_hwfn, "Failed to allocate ilt shadow table\n"); > > + rc = -ENOMEM; > > + goto ilt_shadow_fail; > > + } else { > > + DP_VERBOSE(p_hwfn, QED_MSG_ILT, > > + "Allocated 0x%x bytes for ilt shadow\n", > > + (u32)(size * sizeof(struct qed_dma_mem))); > > + } > The "else" branch after the "goto" isn't idiomatic. Not that I mind, but is such a prefernce described in any style-guide? > [...] > > +static int qed_init_qm_info(struct qed_hwfn *p_hwfn) > > +{ > [...] > > + /* PQs will be arranged as follows: First per-TC PQ then pure-LB quete. > > + */ > > + qm_info->qm_pq_params = kzalloc(sizeof(*qm_info->qm_pq_params) * > > + num_pqs, GFP_ATOMIC); > qed_init_qm_info is only used in qed_resc_alloc. qed_resc_alloc performs > GFP_KERNEL alloc and qed_resc_alloc does not use qed_init_qm_info in > a spinlocked section. I would thus expect both to use the same allocation > flag. I know we're wasteful in using GFP_ATOMIC in the driver in many places. We've already revised this in our dev tree, but we're trying to use the same code-base for the initial submission [otherwise it would make a difficult task even more difficult]. Thanks for pointing this out, but unless this is considered crucial for inital submission we'll fix it later on. Thanks, Yuval-- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@...r.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists