Re: [PATCH 05/18] tty: serial: samsung_tty: add support for Apple UARTs

[Hector Martin 'marcan']

On 06/02/2021 22.15, Marc Zyngier wrote:
> > -static int s3c24xx_serial_has_interrupt_mask(struct uart_port *port)
> > +static int s3c24xx_irq_type(struct uart_port *port)
> >   {
> > -	return to_ourport(port)->info->type == PORT_S3C6400;
> > +	switch (to_ourport(port)->info->type) {
> > +	case PORT_S3C6400:
> > +		return IRQ_S3C6400;
> > +	case PORT_APPLE:
> > +		return IRQ_APPLE;
> > +	default:
> > +		return IRQ_DISCRETE;
> > +	}
> > +
>
> nit: For ease of reviewing, it'd be good if you could split this patch
> with introducing the S3C6400 and "discrete" support initially, and
> only then add the new stuff.

Good idea, will do for v2.

> > +	if (s3c24xx_irq_type(port) == IRQ_APPLE)
> > +		s3c24xx_serial_tx_chars(NO_IRQ, ourport);
>
> Instead of directly calling into the handler (which has its own
> problems, see below), could you just tickle the interrupt status
> register to make an interrupt pending and trigger an actual interrupt?
> I have no idea whether the HW supports this kind of trick though.

I thought of that, but I tried really hard to find such a feature with 
no success. The best I can do is unmask and trigger the *RX* timeout 
interrupt which will eventually fire but... this doesn't work so well in 
practice. There is no way to trigger IRQ flags directly (as those bits 
are write-1-to-clear).

> > -	spin_lock_irqsave(&port->lock, flags);
> > +	/* Only lock if called from IRQ context */
> > +	if (irq != NO_IRQ)
> > +		spin_lock_irqsave(&port->lock, flags);
>
> Isn't that actually dangerous? What prevents the interrupt from firing
> right in the middle of this sequence and create havoc when called from
> enable_tx_pio()? I fail to see what you gain with sidestepping the
> locking.

The callpath here is:

uart_start -> __uart_start -> (uart_ops.start_tx) 
s3c24xx_serial_start_tx -> s3c24xx_serial_start_tx_pio -> enable_tx_pio 
-> s3c24xx_serial_tx_chars

And uart_start takes the uart_port lock. None of the serial functions 
take the lock because the serial core already does, but obviously the 
IRQ handler needs to, *if* it's called as an IRQ handler only.

> The default should be IRQ_NONE, otherwise the kernel cannot detect a
> screaming spurious interrupt.

Good point, and this needs fixing in s3c64xx_serial_handle_irq too then 
(which is what I based mine off of).

> > +	ret = request_irq(port->irq, apple_serial_handle_irq, IRQF_SHARED,
> > +			  s3c24xx_serial_portname(port), ourport);
>
> Why IRQF_SHARED? Do you expect any other device sharing the same line
> with this UART?

This also came from s3c64xx_serial_startup and... now I wonder why that 
one needs it. Maybe on some SoCs it does get shared? Certainly not for 
discrete rx/tx irq chips (and indeed those don't set the flag)...

CCing Thomas, who added the S3C64xx support (and should probably review 
this patch); is there a reason for IRQF_SHARED there? NB: v1 breaks the 
build on arm or with CONFIG_PM_SLEEP, those will be fixed for v2.

Either way, certainly not for Apple SoCs; I'll get rid of IRQF_SHARED 
for v2.

> > diff --git a/include/uapi/linux/serial_core.h b/include/uapi/linux/serial_core.h
> > index 62c22045fe65..59d102b674db 100644
> > --- a/include/uapi/linux/serial_core.h
> > +++ b/include/uapi/linux/serial_core.h
> > @@ -277,4 +277,7 @@
> >   /* Freescale LINFlexD UART */
> >   #define PORT_LINFLEXUART	122
> >   
> > +/* Apple Silicon (M1/T8103) UART (Samsung variant) */
> > +#define PORT_APPLE	123
> > +
>
> Do you actually need a new port type here? Looking at the driver
> itself, it is mainly used to work out the IRQ model. Maybe introducing
> a new irq_type field in the port structure would be better than
> exposing this to userspace (which should see something that is exactly
> the same as a S3C UART).

Well... every S3C variant already has its own port type here.

#define PORT_S3C2410    55
#define PORT_S3C2440    61
#define PORT_S3C2400    67
#define PORT_S3C2412    73
#define PORT_S3C6400    84

If we don't introduce a new one, which one should we pretend to be? :)

I agree that it might make sense to merge all of these into one, though; 
I don't know what the original reason for splitting them out is. But now 
that they're part of the userspace API, this might not be a good idea. 
Though, unsurprisingly, some googling suggests there are zero users of 
these defines in userspace.

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