linux-kernel - Re: [PATCH tty v1 01/74] serial: core: Provide port lock wrappers

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 for Android: free password hash cracker in your pocket

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

Message-ID: <ZQqmEaYeWrurNmGr@alley>
Date:   Wed, 20 Sep 2023 09:58:09 +0200
From:   Petr Mladek <pmladek@...e.com>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     John Ogness <john.ogness@...utronix.de>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Jiri Slaby <jirislaby@...nel.org>,
        linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH tty v1 01/74] serial: core: Provide port lock wrappers

Adding Peter into Cc who also has a big experience with locking APIs.
Well, I think that he would not care ;-)

On Tue 2023-09-19 21:51:12, Thomas Gleixner wrote:
> On Tue, Sep 19 2023 at 16:24, Petr Mladek wrote:
> > On Thu 2023-09-14 20:43:18, John Ogness wrote:
> > IMHO, it would have been better to pass the flags variable directly
> > via a macro as it is done in most *_lock_*_irqsafe() APIs. I mean
> > something like:
> >
> > /**
> >  * uart_port_trylock_irqsave - Try to lock the UART port, save and disable interrupts
> >  * @up:		Pointer to UART port structure
> >  * @flags:	Interrupt flags storage
> >  *
> >  * Returns: True if lock was acquired, false otherwise
> >  */
> > #define uart_port_lock_irqsave(up, flags)		\
> > ({							\
> > 	local_irq_save(flags);				\
> > 	uart_port_lock(lock)				\
> > })
> 
> It's worse.
> 
>      1) Macros are not type safe by themself and rely on type safety
>         of the inner workings.
> 
>      2) Macros are bad for grep as you can't search for a 'struct foo *'
>         argument. Even semantic parsers have their problems with macro
>         constructs. I just learned that again when doing this.
> 
>      3) Macros are just horrible to read
> 
>      4) If you want to out of line the wrapper later, then you still
>         have to keep the macro around because the 'flags' argument is by
>         value and not a pointer.
> 
> >From a code generation point of view it's completely irrelevant whether
> you have a macro or an inline. That was different 25 years ago, but who
> cares about museum compilers today.

I probably was not clear enough. The difference and the motivation
is to pass the "flags" variable instead of pointer "*flags".

Both most common APIs, local_irq_save(flags) and
spin_lock_irqsave(lock, flags) pass the variable. Also most
subsystem specific wrappers do so.

IMHO, some consistency makes the life easier for developers,
especially for frequently used API. And the patchset seems to be
adding >350 users of the uart_port_lock_*irqsave() API.

I do not know. Maybe using macros was bad decision for local_irq*()
and spin_lock*() API. Anyway, we are going to live with
the uart_port_lock*() API for a looong time. And I want to be sure
that we do not create (small) headaches for future generations.

OK, maybe this particular difference is not important enough.
As I said, I do not resist on the change.

Best Regards,
Petr