Serial maintainership

Serial maintainership

[Russell King]

I notice DaveM's taken over serial maintainership.  Please arrange for
serial patches to be sent to davem in future, thanks.  (All ARM serial
drivers are broken as of Tuesday.)

I might take a different view if I at least had a curtious CC: of the
patch, which I had already asked akpm to reject.

Thanks.  That's another subsystem I don't have to care about anymore.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: Serial maintainership

[Alan Cox]

On Iau, 2005-09-08 at 16:52 +0100, Russell King wrote:
> I notice DaveM's taken over serial maintainership.  Please arrange for
> serial patches to be sent to davem in future, thanks.  (All ARM serial
> drivers are broken as of Tuesday.)
> 
> I might take a different view if I at least had a curtious CC: of the
> patch, which I had already asked akpm to reject.
> 
> Thanks.  That's another subsystem I don't have to care about anymore.

Please remember to send Linus a patch updating MAINTAINERS if so.

Re: Serial maintainership

[Russell King]

On Thu, Sep 08, 2005 at 05:38:43PM +0100, Alan Cox wrote:
> On Iau, 2005-09-08 at 16:52 +0100, Russell King wrote:
> > I notice DaveM's taken over serial maintainership.  Please arrange for
> > serial patches to be sent to davem in future, thanks.  (All ARM serial
> > drivers are broken as of Tuesday.)
> > 
> > I might take a different view if I at least had a curtious CC: of the
> > patch, which I had already asked akpm to reject.
> > 
> > Thanks.  That's another subsystem I don't have to care about anymore.
> 
> Please remember to send Linus a patch updating MAINTAINERS if so.

Well, it appears that we're fast approaching meltdown in kernel
land - patches are being applied despite maintainers objection,
maintainers are not being copied with changes in their area, etc.

I might mind less with the occasional slip up if it was occasional,
but it doesn't appear to be anymore - maybe not from my perspective.

This morning Andi Kleen stated:

 "normally he (akpm) asks you before finally sending them off -
  then you can complain again"

I don't appear to be asked by akpm - patches from -mm are sent
to Linus CC'd me, and that occurs during the night.  Come the
morning, they're in Linus tree so unless one is awake reading
email 24 hours a day, it's impossible to "complain again".

Is there a concerted effort to maintainers?  It certainly seems
so from my perspective.

---
Paranoia, n.
   1. A psychotic disorder characterized by delusions of persecution
      with or without grandeur, often strenuously defended with apparent
      logic and reason.
   2. Extreme, irrational distrust of others.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: Serial maintainership

[Andrew Morton]

Russell King <rmk+lkml@arm.linux.org.uk> wrote:
>
> On Thu, Sep 08, 2005 at 05:38:43PM +0100, Alan Cox wrote:
> > On Iau, 2005-09-08 at 16:52 +0100, Russell King wrote:
> > > I notice DaveM's taken over serial maintainership.  Please arrange for
> > > serial patches to be sent to davem in future, thanks.  (All ARM serial
> > > drivers are broken as of Tuesday.)
> > > 
> > > I might take a different view if I at least had a curtious CC: of the
> > > patch, which I had already asked akpm to reject.
> > > 
> > > Thanks.  That's another subsystem I don't have to care about anymore.
> > 
> > Please remember to send Linus a patch updating MAINTAINERS if so.
> 
> Well, it appears that we're fast approaching meltdown in kernel
> land - patches are being applied despite maintainers objection,
> maintainers are not being copied with changes in their area, etc.

Sometimes.  They're mistakes.

> I might mind less with the occasional slip up if it was occasional,
> but it doesn't appear to be anymore - maybe not from my perspective.

The number of times I've been made aware of it happening is quite occasional.

> This morning Andi Kleen stated:
> 
>  "normally he (akpm) asks you before finally sending them off -
>   then you can complain again"
> 
> I don't appear to be asked by akpm

Maintainer is (at least) cc'ed when the patch goes into -mm and when it
goes over to Linus.  Post-facto complaining is appreciated, so we can fix
the kernel and so I can tweak the process or the brain.

> - patches from -mm are sent
> to Linus CC'd me, and that occurs during the night.  Come the
> morning, they're in Linus tree so unless one is awake reading
> email 24 hours a day, it's impossible to "complain again".

It shouldn't be necessary to complain more than once.  Sometimes I'll hang
on to a complained-about patch a) to remember that something needs to
happen and b) because an update is expected.  Once or twice I've
accidentally submitted such patches.

This particular patch ([SERIAL]: Avoid 'statement with no effect'
warnings.) didn't ever go into -mm.

Re: Serial maintainership

[Linus Torvalds]

On Thu, 8 Sep 2005, Alan Cox wrote:
>
> On Iau, 2005-09-08 at 16:52 +0100, Russell King wrote:
> > I notice DaveM's taken over serial maintainership.  Please arrange for
> > serial patches to be sent to davem in future, thanks.  (All ARM serial
> > drivers are broken as of Tuesday.)
> > 
> > I might take a different view if I at least had a curtious CC: of the
> > patch, which I had already asked akpm to reject.
> > 
> > Thanks.  That's another subsystem I don't have to care about anymore.
> 
> Please remember to send Linus a patch updating MAINTAINERS if so.

Guys, stop being stupid about things. I already sent rmk an email in 
private. And Alan, there's absolutely no point in making things even 
worse.

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.

You don't have to be polite about it, and swearing is fine. So instead of
saying "I don't want to play any more because Davem made a mistake", say
something like "Davem is a f*cking clueless moron, here's what he did and
here's why it's wrong".

Notice? In both cases you get to vent your unhappiness. In the second
case, you make the person who made a mistake look bad. But in the first
case, it's just yourself that looks bad.

		Linus

Re: Serial maintainership

[David S. Miller]

From: Linus Torvalds <torvalds@osdl.org>
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;
		}

(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.

Re: Serial maintainership

[Russell King]

On Thu, Sep 08, 2005 at 01:13:58PM -0700, David S. Miller wrote:
> From: Linus Torvalds <torvalds@osdl.org>
> 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

Re: Serial maintainership

[David S. Miller]

From: Russell King <rmk+lkml@arm.linux.org.uk>
Date: Thu, 8 Sep 2005 21:22:36 +0100

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

Then pass in a NULL in the ARM serial drivers instead of this ugly
dependency upon the macro not using the argument.

Re: Serial maintainership

[Linus Torvalds]

On Thu, 8 Sep 2005, David S. Miller wrote:
> 
> > the "regs" argument may not exist in the parent context in the
> > !SUPPORT_SYSRQ case.
> 
> Then pass in a NULL in the ARM serial drivers instead of this ugly
> dependency upon the macro not using the argument.

No, the ARM driver -does- want to pass in "regs" for the SYSRQ case, it's 
just that "regs" doesn't even exist when SYSRQ is not enabled. Look into 
drivers/serial/amba-pl010.c as an example.

Yes, it's a bit ugly, but we've had similar cases in other places. And
it's likely a valid optimization, and the old code worked beautifully by
just not even caring when "regs" wasn't used due to SYSRQ not being
enabled.

We've had somewhat similar cases where optimizations depended on macros 
not even expanding their arguments when they aren't used (ie the arguments 
might be expensive to expand: function calls or inline asm that the 
compiler can't remove even if the result isn't used).

So it's certainly a valid optimization to know that the arguments aren't
even evaluated, and thus it's sometimes really wrong to change a macro
into an inline function.

		Linus

Re: Serial maintainership

[David S. Miller]

From: Linus Torvalds <torvalds@osdl.org>
Date: Thu, 8 Sep 2005 13:39:35 -0700 (PDT)

> So it's certainly a valid optimization to know that the arguments aren't
> even evaluated, and thus it's sometimes really wrong to change a macro
> into an inline function.

Ok, I'll revert the patch and fix the sunsab.c driver as
Russell indicated.  So much for type checking...

Re: Serial maintainership

[Linus Torvalds]

On Thu, 8 Sep 2005, David S. Miller wrote:
> 
> Ok, I'll revert the patch and fix the sunsab.c driver as
> Russell indicated.  So much for type checking...

Actually, I think there's a simpler fix. Instead of reverting, how about 
something like this?

(You might even remove the #ifdef inside the function by then, since "ch" 
being a constant zero will make 90% of it go away anyway).

rmk? Davem?

		Linus

---
diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
--- a/include/linux/serial_core.h
+++ b/include/linux/serial_core.h
@@ -401,6 +401,9 @@ uart_handle_sysrq_char(struct uart_port 
 #endif
 	return 0;
 }
+#ifndef SUPPORT_SYSRQ
+#define uart_handle_sysrq_char(port,ch,regs) uart_handle_sysrq_char(port, 0, NULL)
+#endif
 
 /*
  * We do the SysRQ and SAK checking like this...

Re: Serial maintainership

[David S. Miller]

From: Linus Torvalds <torvalds@osdl.org>
Date: Thu, 8 Sep 2005 14:22:37 -0700 (PDT)

> On Thu, 8 Sep 2005, David S. Miller wrote:
> > 
> > Ok, I'll revert the patch and fix the sunsab.c driver as
> > Russell indicated.  So much for type checking...
> 
> Actually, I think there's a simpler fix. Instead of reverting, how about 
> something like this?
> 
> (You might even remove the #ifdef inside the function by then, since "ch" 
> being a constant zero will make 90% of it go away anyway).
> 
> rmk? Davem?

I'm fine with this.

Re: Serial maintainership

[Russell King]

On Thu, Sep 08, 2005 at 02:22:37PM -0700, Linus Torvalds wrote:
> On Thu, 8 Sep 2005, David S. Miller wrote:
> > Ok, I'll revert the patch and fix the sunsab.c driver as
> > Russell indicated.  So much for type checking...
> 
> Actually, I think there's a simpler fix. Instead of reverting, how about 
> something like this?
> 
> (You might even remove the #ifdef inside the function by then, since "ch" 
> being a constant zero will make 90% of it go away anyway).
> 
> rmk? Davem?

Ok, I'll settle for this.

> ---
> diff --git a/include/linux/serial_core.h b/include/linux/serial_core.h
> --- a/include/linux/serial_core.h
> +++ b/include/linux/serial_core.h
> @@ -401,6 +401,9 @@ uart_handle_sysrq_char(struct uart_port 
>  #endif
>  	return 0;
>  }
> +#ifndef SUPPORT_SYSRQ
> +#define uart_handle_sysrq_char(port,ch,regs) uart_handle_sysrq_char(port, 0, NULL)
> +#endif
>  
>  /*
>   * We do the SysRQ and SAK checking like this...

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:  2.6 Serial core

Re: Serial maintainership

[Linus Torvalds]

On Thu, 8 Sep 2005, Linus Torvalds wrote:
> 
> (You might even remove the #ifdef inside the function by then, since "ch" 
> being a constant zero will make 90% of it go away anyway).

Sadly, the remaining part checks "port->sysrq", which doesn't even exist 
unless CONFIG_SERIAL_CORE_CONSOLE is set, so that doesn't work out.

Oh, well. That simple three-liner should work fine, I was just hoping we 
could do the rest more cleanly..

		Linus

Re: Serial maintainership

[viro]

On Thu, Sep 08, 2005 at 01:13:58PM -0700, David S. Miller wrote:
> From: Linus Torvalds <torvalds@osdl.org>
> 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.

The fact that it's called regardless of SUPPORT_SYSRQ and some callers
look like

#ifdef SUPPORT_SYSRQ
int foo(blah, struct pt_regs *regs)
#else
int foo(blah)
#endif
{
	...
	uart_handle_sysrq_char(..., regs);
	...
}

which works with old definition (without SUPPORT_SYSRQ the last argument of
uart_handle_sysrq_char() is never seen by parser) and obviously dies with
the new one.

And yes, it's sick...