From: Ursula Braun <ubraun@xxxxxxxxxxxxxxxxxx>
Date: Mon, 14 Sep 2015 12:42:46 +0200
> +#ifdef CONFIG_S390
> +#include <asm/ebcdic.h>
> +#endif
Please keep s390'isms out of this code.
> +void smc_link_up(struct smc_link_group *lgr)
> +{
> + struct smc_roce_defs rocdefs;
> + struct smc_link *link = NULL;
> + int i;
> + u8 p1, p2;
Please order local variable declarations from longest to shortest
line (aka: "reverse christimas tree").
Please fix this up for your entire submission, as I'm not going to
point out all of the instances explicitly.
> +
> + /* determine working link */
> + for (i = 0; i <= SMC_MAX_SYM_LINKS; i++) {
> + if (atomic_read(&lgr->lnk[i].state) == SMC_LINK_UP) {
> + link = &lgr->lnk[i];
> + break;
> + }
> + }
Using an atomic_t type and then only performing 'set' and 'read'
operations on that variable is pointless and doesn't provide you
with any real "atomicity".
Please use a plain int, or provide a complete and suitable
justification for using atomic_t in this situation.
> + for (i = 0; i < 2; i++) {
Please change this '2' constant in all of these loops to some
suitable mnenomic. Likewise in the declaration of the 'port_err'
struct member et al.
> + get_random_bytes_arch(&local_peerid[0], 2);
I really don't see any justification for using get_random_bytes_arch()
vs. plain get_random_bytes().
> +void smc_sock_wake_tx(struct sock *sk)
> +{
> + struct smc_sock *smc = smc_sk(sk);
> + struct socket_wq *wq;
> + struct socket *sock = sk->sk_socket;
> +
> +/* similar to sk_stream_write_space */
This comment is imporperly indented.
> + /* cancel close waiting */
> + atomic64_set(&conn->tx_curs_prep.s.acurs,
> + conn->tx_curs_sent.s.lcurs);
> + atomic64_set(&conn->tx_curs_fin.s.acurs,
> + conn->tx_curs_sent.s.lcurs);
Another case of atomic types being used inappropriately. You only use
'set' and 'read' operations on these values, and that makes using
atomic types entirely pointless.
This is a huge and complex submission and I'm already burnt out
reading the changes thus far.
You have a lot to fix up and you can expect many revisions to be
necessary before these changes are ready for integration upstream.
And that's if you are lucky and someone actually continues to review
this work.
--
To unsubscribe from this list: send the line "unsubscribe linux-s390" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
