linux-kernel - Re: [Outreachy kernel] [RESEND PATCH] staging: emxx

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: <20210406195621.GU2531743@casper.infradead.org>
Date:   Tue, 6 Apr 2021 20:56:21 +0100
From:   Matthew Wilcox <willy@...radead.org>
To:     Beatriz Martins de Carvalho <martinsdecarvalhobeatriz@...il.com>
Cc:     linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org,
        outreachy-kernel@...glegroups.com, gregkh@...uxfoundation.org
Subject: Re: [Outreachy kernel] [RESEND PATCH] staging: emxx_udc: Ending line
 with argument

On Tue, Apr 06, 2021 at 08:34:09PM +0100, Beatriz Martins de Carvalho wrote:
> Cleans up check of "Lines should not end with a '('"
> with argument present in next line in file emxx_udc.c

I appreciate that you've removed the checkpatch warning, but this is
still harder to read than the original used to be.

> -				_nbu2ss_writel(
> -					&preg->EP_REGS[ep->epnum - 1].EP_WRITE,
> -					p_buf_32->dw);
> +				_nbu2ss_writel(&preg->EP_REGS[ep->epnum - 1].EP_WRITE,
> +					       p_buf_32->dw);

> -		length = _nbu2ss_readl(
> -			&ep->udc->p_regs->EP_REGS[ep->epnum - 1].EP_LEN_DCNT);
> +		length = _nbu2ss_readl(&ep->udc->p_regs->EP_REGS[ep->epnum - 1].EP_LEN_DCNT);

> -			regdata = _nbu2ss_readl(
> -				&preg->EP_REGS[ep->epnum - 1].EP_STATUS);
> +			regdata = _nbu2ss_readl(&preg->EP_REGS[ep->epnum - 1].EP_STATUS);

The real problem with this driver is that their abstraction layer is
wrong.  For example:

        /* Interrupt Status */
        status = _nbu2ss_readl(&udc->p_regs->EP_REGS[num].EP_STATUS);

        /* Interrupt Clear */
        _nbu2ss_writel(&udc->p_regs->EP_REGS[num].EP_STATUS, ~status);

If instead this were:

	status = nbu2ss_read_ep_status(udc, num);
	nbu2ss_write_ep_status(udc, num, ~status);

that would be a lot shorter and clearer.  Cleanups along these lines
would be a lot more useful, and would fix the 80 column warning.