netdev - RE: [PATCH net-next v5 01/10] qed: Add module with basic common support

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <B5657A6538887040AD3A81F1008BEC63E467D6@avmb3.qlogic.org>
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