Re: [PATCH v2 6/6] serial: 8250: make port operations work on struct uart_8250_port

[Alejandro Colomar <alx+linux-hardening@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 6345 bytes --]

Hi Jiri,

> Date: 2026-10-13 11:39:00+0200
> From: Jiri Slaby <jirislaby@kernel.org>
>
> On 13. 10. 26, 11:09, Andy Shevchenko wrote:
> > On Tue, Oct 13, 2026 at 09:53:26AM +0200, Jiri Slaby wrote:
> > > On 13. 10. 26, 9:33, Andy Shevchenko wrote:
> > > > On Tue, Oct 13, 2026 at 08:48:51AM +0200, Jiri Slaby (SUSE) wrote:
> > 
> > ...
> > 
> > > > > -	rc = serial8250_register_8250_port(&port);
> > > > > -	if (rc < 0)
> > > > > +	struct uart_8250_port *uport = serial8250_register_8250_port(&port);
> > > > > +	if (IS_ERR(uport)) {
> > > > > +		rc = PTR_ERR(uport);
> > > > >    		goto err_sysfs_remove;
> > > > > +	}
> > > 
> > > > Also do not define
> > > > variables in the middle of the code (it's only allowed for auto cleanup
> > > > variables and for-loop iterators).
> > > 
> > > Why, actually?
> > 
> > Because it's the style.
> 
> There is no hard style though. It varies across subsystems.
> 
> > Linus also told it many times
> 
> Which is neither an argument. Linus is wrong at times ("Link:" use as an
> example).
> 
> All in all, I do not insist on this at all. It's easier to read and follow
> the code for me. Not necessarily the case for others...
> 
> > > First, it is used in the kernel quite a few times already
> > > [1]. Second, while it is sort of c++ism (and I like to use modern c++), it
> > > makes the life of the variable a lot more obvious.
> > > 
> > > [1] eg. see output of: rg -U -C 5 '}\n+\s+struct .*\*.*=' drivers/
> > 
> > (you made me to install something I haven't heard about, it's better to use
> >   `git grep` though)
> 
> rg is order of magnitude faster (searches in parallel). More importantly in
> this case, supports multiline matches. (And respects .git ignores.)

Is that really true?

	alx@debian:~/src/linux/linux/master$ time \
			find * -type f \
			| grep '\.[ch]' \
			| sort \
			| xargs grep -n ARRAY_SIZE /dev/null \
			| wc -l;
	58852

	real	0m1.022s
	user	0m0.726s
	sys	0m0.358s
	alx@debian:~/src/linux/linux/master$ time \
			git ls-files \
			| grep '\.[ch]' \
			| xargs grep -n ARRAY_SIZE /dev/null \
			| wc -l;
	58854

	real	0m0.923s
	user	0m0.650s
	sys	0m0.333s

There seems to be a difference of two occurences, which is due to
symlinks:

	alx@debian:~/src/linux/linux/master$ diff -u \
			<(find * -type f \
				| grep '\.[ch]' \
				| sort \
				| xargs grep -n ARRAY_SIZE /dev/null) \
			<(git ls-files \
				| grep '\.[ch]' \
				| xargs grep -n ARRAY_SIZE /dev/null);
	--- /dev/fd/63	2026-10-13 12:40:11.675302047 +0200
	+++ /dev/fd/62	2026-10-13 12:40:11.675302047 +0200
	@@ -57769,6 +57769,7 @@
	 tools/testing/selftests/bpf/benchs/bench_sockmap.c:128:	for (i = 0; i < ARRAY_SIZE(ctx.fds); i++) {
	 tools/testing/selftests/bpf/bpf_util.h:69:#ifndef ARRAY_SIZE
	 tools/testing/selftests/bpf/bpf_util.h:70:# define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
	+tools/testing/selftests/bpf/disasm.c:23:	BUILD_BUG_ON(ARRAY_SIZE(func_id_str) != __BPF_FUNC_MAX_ID);
	 tools/testing/selftests/bpf/map_tests/lpm_trie_map_basic_ops.c:1006:	const char *sorted_keys[ARRAY_SIZE(keys)];
	 tools/testing/selftests/bpf/map_tests/lpm_trie_map_basic_ops.c:1012:	fd = lpm_trie_create(sizeof(key), sizeof(value), ARRAY_SIZE(keys));
	 tools/testing/selftests/bpf/map_tests/lpm_trie_map_basic_ops.c:1014:	for (i = 0; i < ARRAY_SIZE(keys); i++) {
	@@ -58786,6 +58787,7 @@
	 tools/testing/selftests/vfio/lib/drivers/dsa/dsa.c:215:	dsa->max_copies_per_batch = min(dsa->max_copies_per_batch, ARRAY_SIZE(dsa->copy));
	 tools/testing/selftests/vfio/lib/drivers/dsa/dsa.c:340:	for (i = 0; i < ARRAY_SIZE(dsa->copy); i++) {
	 tools/testing/selftests/vfio/lib/drivers/dsa/dsa.c:349:	for (i = 0; i < ARRAY_SIZE(dsa->batch) && count; i++) {
	+tools/testing/selftests/vfio/lib/drivers/igb/e1000_82575.h:240:#define E1000_VLVF_ARRAY_SIZE     32
	 tools/testing/selftests/vfio/lib/drivers/nv_falcon/hw.h:249:#define VERIFIED_GPU_MAP_SIZE ARRAY_SIZE(verified_gpu_map)
	 tools/testing/selftests/vfio/lib/iommu.c:59:	for (i = 0; i < ARRAY_SIZE(iommu_modes); i++) {
	 tools/testing/selftests/vfio/lib/sysfs.c:48:	VFIO_ASSERT_GT(read(fd, buf, ARRAY_SIZE(buf)), 0);

And here goes rg(1):

	alx@debian:~/src/linux/linux/master$ time rg -n ARRAY_SIZE | wc -l
	58957

	real	0m0.512s
	user	0m0.490s
	sys	0m0.974s

It's around 50% faster, which I wouldn't call orders of magnitude.
rg(1) inherits the bogus design of find(1) in trying to filter the file
names with multiple subtly different options.  grep(1) can do that much
better, and by using a pipe, you have a dedicated process (often in a
dedicated CPU) for that task, which is likely more efficient.

And here's plain grep(1):

	alx@debian:~/src/linux/linux/master$ time grep -r -n ARRAY_SIZE \
			| wc -l
	58957

	real	0m1.140s
	user	0m0.663s
	sys	0m0.489s

Multiline matches are also supported by pcre2grep(1), BTW, which uses
PCRE2, which are way more useful than that.

pcre2grep(1) is itself a bit slower than grep(1) --especially if you try
very complex regexes--:

	alx@debian:~/src/linux/linux/master$ time \
			find * -type f \
			| grep '\.[ch]' \
			| sort \
			| xargs pcre2grep -n ARRAY_SIZE /dev/null \
			| wc -l;
	58852

	real	0m1.857s
	user	0m1.408s
	sys	0m0.516s

But if you pre-filter the files with a simpler regex, then it comes back
to usual time:

	alx@debian:~/src/linux/linux/master$ time \
			find * -type f \
			| grep '\.[ch]' \
			| sort \
			| xargs grep -l ARRAY_SIZE \
			| xargs pcre2grep -n ARRAY_SIZE /dev/null \
			| wc -l;
	58852

	real	0m0.919s
	user	0m0.830s
	sys	0m0.462s


Have a lovely day!
Alex

> > Note excluded __free() if I get regexp correctly
> > 
> > $ rg -U -l '}\n+\s+struct .*\*[^)]*=' drivers/ | less
> > drivers/ufs/host/ufs-qcom.c
> ...
> > drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
> > 
> > So, less than dozen of drivers doing wrong is not an argument, try something
> > better. But thanks, I will check and address this in some of the above.
> 
> Well, it's only an example of a declaration after a previous opening brace
> "{" -- this one turned out to be easy to search for. There is a lot of more,
> of course. kernel/ included.
> 
> thanks,
> -- 
> js
> suse labs
> 

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]