Re: Serial maintainership

[Russell King]

On Thu, Sep 08, 2005 at 01:13:58PM -0700, David S. Miller wrote:
> From: Linus Torvalds <torvalds@xxxxxxxx>
> Date: Thu, 8 Sep 2005 09:27:56 -0700 (PDT)
> 
> > Mistakes happen, and the way you fix them is not to pull a tantrum, but 
> > tell people that they are idiots and they broke something, and get them to 
> > fix it instead.
> 
> In all this noise I still haven't seen what is wrong with
> the build warning fix I made.
> 
> Even as networking maintainer, other people put changes into the
> networking as build or warning fixes, and I have to live with that.
> If I don't like what happened, I call it out and send in a more
> appropriate fix.  This is never something worth peeing my pants in
> public about.
> 
> Anyways, let's discuss the concrete problem here.
> 
> The previous definition of uart_handle_sysrq_char(), when
> SUPPORT_SYSRQ was disabled, was a plain macro define to "(0)" but this
> makes gcc emit empty statement warnings (and rightly so) in cases such
> as:
> 
> 		if (tty == NULL) {
> 			uart_handle_sysrq_char(&up->port, ch, regs);
> 			continue;
> 		}

Actually, it turns out this is a valid warning but not in the way
you're thinking.

In order to handle a sysrq char, you first need to see a break
condition.  You detect break conditions further down in this
function after the above check.

Hence, if tty is NULL, you don't check for any break conditions.
This means that you're never set up to handle a sysrq character,
which in turn means that the above code has no effect and can
actually be removed... which also removes the warning.

> (that example is from drivers/sun/sunsab.c)
> 
> So I changed it so that it was an inline function, borrowing the
> existing code, so that we get the warning erased _and_ we get type
> checking even when SUPPORT_SYSRQ is disabled.  So we end up with:
> 
> static inline int
> uart_handle_sysrq_char(struct uart_port *port, unsigned int ch,
> 		       struct pt_regs *regs)
> {
> #ifdef SUPPORT_SYSRQ
> 	if (port->sysrq) {
> 		if (ch && time_before(jiffies, port->sysrq)) {
> 			handle_sysrq(ch, regs, NULL);
> 			port->sysrq = 0;
> 			return 1;
> 		}
> 		port->sysrq = 0;
> 	}
> #endif
> 	return 0;
> }
> 
> which is what is there now.  I can't see what's so wrong with that.

the "regs" argument may not exist in the parent context in the
!SUPPORT_SYSRQ case.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/