linux-kernel - Re: [PATCH 05/18] tty: serial: samsung

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:   Tue, 9 Feb 2021 01:10:02 +0900
From:   Hector Martin <marcan@...can.st>
To:     Krzysztof Kozlowski <krzk@...nel.org>
Cc:     soc@...nel.org, linux-arm-kernel@...ts.infradead.org,
        Marc Zyngier <maz@...nel.org>, robh+dt@...nel.org,
        Arnd Bergmann <arnd@...nel.org>, linux-kernel@...r.kernel.org,
        devicetree@...r.kernel.org, Olof Johansson <olof@...om.net>
Subject: Re: [PATCH 05/18] tty: serial: samsung_tty: add support for Apple
 UARTs

On 08/02/2021 19.54, Krzysztof Kozlowski wrote:
>> +enum s3c24xx_irq_type {
>> +	IRQ_DISCRETE = 0,
>> +	IRQ_S3C6400 = 1,
>> +	IRQ_APPLE = 2,
> 
> It seems you add the third structure to differentiate type of UART.
> There is already port type and s3c24xx_serial_drv_data, no need for
> third structure (kind of similar to tries of Tamseel Shams in recent
> patches). It's too much. Instead, differentiate by port type or prepare
> own set of uart_ops if it's really different (like you did with startup
> op).

This ties into little changes in a bunch of places, so separate uart_ops 
  callbacks for every one of those would end up duplicating a lot of code :(

That list is just used to map the port type to something that only 
represents the type of IRQs, but its only real purpose for the 
indirection is so I can do "== IRQ_DISCRETE" in some codepaths to mean 
"not apple or S3C6400".

e.g.

     if (s3c24xx_irq_type(port) == IRQ_DISCRETE)
         free_irq(ourport->rx_irq, ourport);

Would have to become

     if (port->type != IRQ_S3C6400 && port->type != IRQ_APPLE)
         free_irq(ourport->rx_irq, ourport);

or

     switch (port->type) {
     case IRQ_S3C6400:
     case IRQ_APPLE:
         break;
     default:
         free_irq(ourport->rx_irq, ourport);
     }

Which one do you prefer?

Aside: Marc didn't like introducing new port types into uapi, but if we 
don't do that then we need a "real" port type in drv_data that isn't the 
uapi-exposed port or something along those lines, with a separate enum 
containing all the true port type values for that.

>>   	/* Startup sequence is different for s3c64xx and higher SoC's */
>> -	if (s3c24xx_serial_has_interrupt_mask(port))
>> +	case IRQ_S3C6400:
>>   		s3c24xx_serial_ops.startup = s3c64xx_serial_startup;
> 
> Don't overwrite specific ops. It's difficult to see then which ops are
> being used. Instead create a new set of uart_ops matching the needs.

s3c24xx_serial_ops was already doing that here, but I can move that to a 
a separate uart_ops too.

Ack on all the other comments, I'll make the changes for v2.

-- 
Hector Martin (marcan@...can.st)
Public Key: https://mrcn.st/pub