linux-kernel - Re: [PATCH 1/7] staging: vt6655: Rename dwData to reg

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]

Date:   Mon, 18 Jul 2022 21:33:08 +0200
From:   Philipp Hortmann <philipp.g.hortmann@...il.com>
To:     Joe Perches <joe@...ches.com>,
        Forest Bond <forest@...ttletooquiet.net>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        linux-staging@...ts.linux.dev, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/7] staging: vt6655: Rename dwData to reg_value in four
 macros

On 7/18/22 08:07, Joe Perches wrote:
> Please remember that checkpatch is a stupid little scripted tool
> and the actual goal is to have readable code.
Understood.
> 
> Look a bit beyond the code and see if and how you could make the
> code better.
> 
> All of these macros have the same form and logic.
> 
That is the reason why I put them all together in one static function:
static void vt6655_mac_dma_ctl(void __iomem *iobase, u8 reg_index)
{
	unsigned long reg_value;

	reg_value = ioread32(iobase + reg_index);
	if (reg_value & DMACTL_RUN)
		iowrite32(DMACTL_WAKE, iobase + reg_index);
	else
		iowrite32(DMACTL_RUN, iobase + reg_index);
}

> Perhaps it'd be better to use another indirect macro and define
> all of these with that new macro.
> 
> Something like:
> 
> #define mac_v(iobase, reg)						\
> do {									\
> 	void __iomem *addr = (iobase) + (reg);				\
> 	iowrite32(ioread32(addr) & DMACTL_RUN ? DMACTL_WAKE : DMACTL_RUN,\
> 		  addr);						\
> } while (0)
> 
> #define MACvReceive0(iobase)	mac_v(iobase, MAC_REG_RXDMACTL0)
> #define MACvReceive1(iobase)	mac_v(iobase, MAC_REG_RXDMACTL1)
> #define MACvTransmit0(iobase)	mac_v(iobase, MAC_REG_TXDMACTL0)
> #define MACvTransmitAC0(iobase)	mac_v(iobase, MAC_REG_AC0DMACTL)

That is an interesting solution. But for me this code is not as good 
readable as my proposal. Reason is that I struggle with the function in 
function with condition broken into two lines.