[PATCH v4] x86, olpc: add debugfs interface for EC commands

[PATCH v4] x86, olpc: add debugfs interface for EC commands

[Daniel Drake]

Add a debugfs interface for sending commands to the OLPC Embedded
Controller (EC) and reading the responses.  The EC provides functionality
for machine identification, battery and AC control, wakeup control, etc.

Having a debugfs interface available is useful for EC development and
debugging.

Based on code by Paul Fox.

Signed-off-by: Daniel Drake <dsd@laptop.org>
Originally-from: Paul Fox <pgf@laptop.org>
Cc: Ingo Molnar <mingo@elte.hu>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Andres Salomon <dilinger@queued.net>
---
 Documentation/ABI/testing/debugfs-olpc |   16 +++++
 arch/x86/platform/olpc/olpc.c          |   97 ++++++++++++++++++++++++++++++++
 2 files changed, 113 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/ABI/testing/debugfs-olpc

v2: incorporate feedback from Andrew Morton (thanks!): documentation in
Documentation/, fixed input checking, more correct command bytes construction

v3: incorporate feedback from Ingo Molnar (thanks!): whitespace/cast fixes,
fix potential overflow when nonsense resp_bytes is passed from userspace,
add mutex to protect data and to protect against concurrent read/write, give
node name 'cmd' which is better than 'generic'

v4: really fix sign-off tags

diff --git a/Documentation/ABI/testing/debugfs-olpc b/Documentation/ABI/testing/debugfs-olpc
new file mode 100644
index 0000000..bd76cc6
--- /dev/null
+++ b/Documentation/ABI/testing/debugfs-olpc
@@ -0,0 +1,16 @@
+What:		/sys/kernel/debug/olpc-ec/cmd
+Date:		Dec 2011
+KernelVersion:	3.4
+Contact:	devel@lists.laptop.org
+Description:
+
+A generic interface for executing OLPC Embedded Controller commands and
+reading their responses.
+
+To execute a command, write data with the format: CC:N A A A A
+CC is the (hex) command, N is the count of expected reply bytes, and A A A A
+are optional (hex) arguments.
+
+To read the response (if any), read from the generic node after executing
+a command. Hex reply bytes will be returned, *whether or not* they came from
+the immediately previous command.
diff --git a/arch/x86/platform/olpc/olpc.c b/arch/x86/platform/olpc/olpc.c
index 7cce722..a4bee53 100644
--- a/arch/x86/platform/olpc/olpc.c
+++ b/arch/x86/platform/olpc/olpc.c
@@ -20,6 +20,8 @@
 #include <linux/platform_device.h>
 #include <linux/of.h>
 #include <linux/syscore_ops.h>
+#include <linux/debugfs.h>
+#include <linux/mutex.h>
 
 #include <asm/geode.h>
 #include <asm/setup.h>
@@ -31,6 +33,15 @@ EXPORT_SYMBOL_GPL(olpc_platform_info);
 
 static DEFINE_SPINLOCK(ec_lock);
 
+/* debugfs interface to EC commands */
+#define EC_MAX_CMD_ARGS (5 + 1)	/* cmd byte + 5 args */
+#define EC_MAX_CMD_REPLY (8)
+
+static struct dentry *ec_debugfs_dir;
+static DEFINE_MUTEX(ec_debugfs_cmd_lock);
+static unsigned char ec_debugfs_resp[EC_MAX_CMD_REPLY];
+static unsigned int ec_debugfs_resp_bytes;
+
 /* EC event mask to be applied during suspend (defining wakeup sources). */
 static u16 ec_wakeup_mask;
 
@@ -269,6 +280,91 @@ int olpc_ec_sci_query(u16 *sci_value)
 }
 EXPORT_SYMBOL_GPL(olpc_ec_sci_query);
 
+static ssize_t ec_debugfs_cmd_write(struct file *file, const char __user *buf,
+				    size_t size, loff_t *ppos)
+{
+	int i, m;
+	unsigned char ec_cmd[EC_MAX_CMD_ARGS];
+	unsigned int ec_cmd_int[EC_MAX_CMD_ARGS];
+	char cmdbuf[64];
+	int ec_cmd_bytes;
+
+	mutex_lock(&ec_debugfs_cmd_lock);
+
+	size = simple_write_to_buffer(cmdbuf, sizeof(cmdbuf), ppos, buf, size);
+
+	m = sscanf(cmdbuf, "%x:%u %x %x %x %x %x", &ec_cmd_int[0],
+		   &ec_debugfs_resp_bytes,
+		   &ec_cmd_int[1], &ec_cmd_int[2], &ec_cmd_int[3],
+		   &ec_cmd_int[4], &ec_cmd_int[5]);
+	if (m < 2 || ec_debugfs_resp_bytes > EC_MAX_CMD_REPLY) {
+		/* reset to prevent overflow on read */
+		ec_debugfs_resp_bytes = 0;
+
+		printk(KERN_DEBUG "olpc-ec: bad ec cmd:  "
+		       "cmd:response-count [arg1 [arg2 ...]]\n");
+		size = -EINVAL;
+		goto out;
+	}
+
+	/* convert scanf'd ints to char */
+	ec_cmd_bytes = m - 2;
+	for (i = 0; i <= ec_cmd_bytes; i++)
+		ec_cmd[i] = ec_cmd_int[i];
+
+	printk(KERN_DEBUG "olpc-ec: debugfs cmd 0x%02x with %d args "
+	       "%02x %02x %02x %02x %02x, want %d returns\n",
+	       ec_cmd[0], ec_cmd_bytes, ec_cmd[1], ec_cmd[2], ec_cmd[3],
+	       ec_cmd[4], ec_cmd[5], ec_debugfs_resp_bytes);
+
+	olpc_ec_cmd(ec_cmd[0], (ec_cmd_bytes == 0) ? NULL : &ec_cmd[1],
+		    ec_cmd_bytes, ec_debugfs_resp, ec_debugfs_resp_bytes);
+
+	printk(KERN_DEBUG "olpc-ec: response "
+	       "%02x %02x %02x %02x %02x %02x %02x %02x (%d bytes expected)\n",
+	       ec_debugfs_resp[0], ec_debugfs_resp[1], ec_debugfs_resp[2],
+	       ec_debugfs_resp[3], ec_debugfs_resp[4], ec_debugfs_resp[5],
+	       ec_debugfs_resp[6], ec_debugfs_resp[7], ec_debugfs_resp_bytes);
+
+out:
+	mutex_unlock(&ec_debugfs_cmd_lock);
+	return size;
+}
+
+static ssize_t ec_debugfs_cmd_read(struct file *file, char __user *buf,
+				   size_t size, loff_t *ppos)
+{
+	unsigned int i, r;
+	char *rp;
+	char respbuf[64];
+
+	mutex_lock(&ec_debugfs_cmd_lock);
+	rp = respbuf;
+	rp += sprintf(rp, "%02x", ec_debugfs_resp[0]);
+	for (i = 1; i < ec_debugfs_resp_bytes; i++)
+		rp += sprintf(rp, ", %02x", ec_debugfs_resp[i]);
+	mutex_unlock(&ec_debugfs_cmd_lock);
+	rp += sprintf(rp, "\n");
+
+	r = rp - respbuf;
+	return simple_read_from_buffer(buf, size, ppos, respbuf, r);
+}
+
+static const struct file_operations ec_debugfs_genops = {
+	.write	 = ec_debugfs_cmd_write,
+	.read	 = ec_debugfs_cmd_read,
+};
+
+static void setup_debugfs(void)
+{
+	ec_debugfs_dir = debugfs_create_dir("olpc-ec", 0);
+	if (ec_debugfs_dir == ERR_PTR(-ENODEV))
+		return;
+
+	debugfs_create_file("cmd", 0600, ec_debugfs_dir, NULL,
+			    &ec_debugfs_genops);
+}
+
 static int olpc_ec_suspend(void)
 {
 	return olpc_ec_mask_write(ec_wakeup_mask);
@@ -372,6 +468,7 @@ static int __init olpc_init(void)
 	}
 
 	register_syscore_ops(&olpc_syscore_ops);
+	setup_debugfs();
 
 	return 0;
 }
-- 
1.7.7.6

Re: [PATCH v4] x86, olpc: add debugfs interface for EC commands

[Andrew Morton]

On Mon, 26 Mar 2012 19:07:07 +0100 (BST)
Daniel Drake <dsd@laptop.org> wrote:

> Add a debugfs interface for sending commands to the OLPC Embedded
> Controller (EC) and reading the responses.  The EC provides functionality
> for machine identification, battery and AC control, wakeup control, etc.
> 
> Having a debugfs interface available is useful for EC development and
> debugging.
> 
> Based on code by Paul Fox.
> 
> Signed-off-by: Daniel Drake <dsd@laptop.org>
> Originally-from: Paul Fox <pgf@laptop.org>
> Cc: Ingo Molnar <mingo@elte.hu>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: "H. Peter Anvin" <hpa@zytor.com>
> Cc: Andres Salomon <dilinger@queued.net>
> 
> ...
>
> v4: really fix sign-off tags

s/fix/break/?  "Originally-from" is not a recognised tag.  If this code
is based upon an earlier version from Paul then Signed-off-by: is
correct.

What's going on here?  What are you trying to communicate?

Re: [PATCH v4] x86, olpc: add debugfs interface for EC commands

[H. Peter Anvin]

On 03/26/2012 02:29 PM, Andrew Morton wrote:
>>
>> v4: really fix sign-off tags
> 
> s/fix/break/?  "Originally-from" is not a recognised tag.  If this code
> is based upon an earlier version from Paul then Signed-off-by: is
> correct.
> 
> What's going on here?  What are you trying to communicate?

I have been using "Originally-by:" (not Originally-from:) to indicate
content originally written by someone else which has then been
dramatically redone.  It seems to communicate, but probably should be
written into a spec somewhere.

It doesn't make much sense to put that after a Signed-off-by: though
(lines are supposed to be in chronological order.)

	-hpa

Re: [PATCH v4] x86, olpc: add debugfs interface for EC commands

[Andrew Morton]

On Mon, 26 Mar 2012 14:31:46 -0700
"H. Peter Anvin" <hpa@zytor.com> wrote:

> On 03/26/2012 02:29 PM, Andrew Morton wrote:
> >>
> >> v4: really fix sign-off tags
> > 
> > s/fix/break/?  "Originally-from" is not a recognised tag.  If this code
> > is based upon an earlier version from Paul then Signed-off-by: is
> > correct.
> > 
> > What's going on here?  What are you trying to communicate?
> 
> I have been using "Originally-by:" (not Originally-from:) to indicate
> content originally written by someone else which has then been
> dramatically redone.  It seems to communicate, but probably should be
> written into a spec somewhere.
> 

Well, if a person had any contribution at all, we should seek their
Signed-off-by:.  Otherwise they could say "hey, you admitted using my
code but I did not authorise its use", or any other range of bad IANAL
things?

If we want to provide additional details on the authorship trail then
fine, that can be done in English in the changelog.  And it can be done
far more clearly than in an ad-hoc tag whose meaning is unclear.

It's pretty unusual to see a patch which was authored by person A to
have "From: B".  Usually this comes about because B made a mistake.  I
will always query the authorship on such patches.  Sometimes it is
deliberate, but there's no consistent pattern in the reasoning.  For
this reason it is always best to fully explain the authorship
alteration in the changelog text.


If we want to add more tag types then OK, we can discuss that, clearly
define them, raise a patch against Documentation/SubmittingPatches and
start to use them.  But let's not invent new ones without explaining to
anyone else what they mean.


Generally if I see anything other than "Signed-off-by:", "Cc:",
"Tested-by:" "Reported-by:", "Acked-by:" or "Reviewed-by:", I consider
them to be undefined and will zap 'em, sometimes transferring the
intent into the changelog body instead.

Re: [PATCH v4] x86, olpc: add debugfs interface for EC commands

[Paul Fox]

andrew wrote:
 > On Mon, 26 Mar 2012 19:07:07 +0100 (BST)
 > Daniel Drake <dsd@laptop.org> wrote:
 > 
 > > Add a debugfs interface for sending commands to the OLPC Embedded
 > > Controller (EC) and reading the responses.  The EC provides functionality
 > > for machine identification, battery and AC control, wakeup control, etc.
 > > 
 > > Having a debugfs interface available is useful for EC development and
 > > debugging.
 > > 
 > > Based on code by Paul Fox.
 > > 
 > > Signed-off-by: Daniel Drake <dsd@laptop.org>
 > > Originally-from: Paul Fox <pgf@laptop.org>
 > > Cc: Ingo Molnar <mingo@elte.hu>
 > > Cc: Thomas Gleixner <tglx@linutronix.de>
 > > Cc: "H. Peter Anvin" <hpa@zytor.com>
 > > Cc: Andres Salomon <dilinger@queued.net>
 > > 
 > > ...
 > >
 > > v4: really fix sign-off tags
 > 
 > s/fix/break/?  "Originally-from" is not a recognised tag.  If this code
 > is based upon an earlier version from Paul then Signed-off-by: is
 > correct.
 > 
 > What's going on here?  What are you trying to communicate?


i think the way daniel had it before was likely correct.

i wrote the original code, but it looked pretty different back then. 
daniel has brought it to its current state.  i _then_ added my
Signed-off-by.

so the code is both "Based on code by Paul Fox", and my Signed-off-by
should come after daniel's (assuming chronological order).

paul
=---------------------
 paul fox, pgf@laptop.org

Re: [PATCH v4] x86, olpc: add debugfs interface for EC commands

[Daniel Drake]

On Mon, Mar 26, 2012 at 3:29 PM, Andrew Morton
<akpm@linux-foundation.org> wrote:
> s/fix/break/?  "Originally-from" is not a recognised tag.  If this code
> is based upon an earlier version from Paul then Signed-off-by: is
> correct.
>
> What's going on here?  What are you trying to communicate?

I'm trying to take Ingo's suggestion, in the thread "[patch 1/8] x86,
olpc: add debugfs interface for EC commands" he wrote:

====
This is not a valid signoff sequence - the 'From: ' author of
the patch must be the first SOB line.

The way to do this is either to have a:

 From: Paul Fox <pgf@laptop.org>

or to covert Paul Fox's SOB to a credit line, such as:

 Originally-from: Paul Fox <pgf@laptop.org>
====

The original code was from Paul Fox. I changed it somewhat
significantly, and Paul approves of the end result.
Can someone suggest a way of expressing this, including tag ordering,
that will be accepted by all parties?  :)

Thanks
Daniel

Re: [PATCH v4] x86, olpc: add debugfs interface for EC commands

[H. Peter Anvin]

On 03/26/2012 03:45 PM, Daniel Drake wrote:
> On Mon, Mar 26, 2012 at 3:29 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
>> s/fix/break/?  "Originally-from" is not a recognised tag.  If this code
>> is based upon an earlier version from Paul then Signed-off-by: is
>> correct.
>>
>> What's going on here?  What are you trying to communicate?
> 
> I'm trying to take Ingo's suggestion, in the thread "[patch 1/8] x86,
> olpc: add debugfs interface for EC commands" he wrote:
> 
> ====
> This is not a valid signoff sequence - the 'From: ' author of
> the patch must be the first SOB line.
> 
> The way to do this is either to have a:
> 
>  From: Paul Fox <pgf@laptop.org>
> 
> or to covert Paul Fox's SOB to a credit line, such as:
> 
>  Originally-from: Paul Fox <pgf@laptop.org>
> ====
> 
> The original code was from Paul Fox. I changed it somewhat
> significantly, and Paul approves of the end result.
> Can someone suggest a way of expressing this, including tag ordering,
> that will be accepted by all parties?  :)
> 

My recommendation is:

Originally-by: Paul Fox <pgf@laptop.org>

... followed by Signed-off-by: in the order of patch flow.  Approving a
patch, when not passing through, is indicated by Acked-by: or Reviewed-by:

	-hpa

Re: [PATCH v4] x86, olpc: add debugfs interface for EC commands

[Andrew Morton]

On Mon, 26 Mar 2012 16:45:10 -0600
Daniel Drake <dsd@laptop.org> wrote:

> On Mon, Mar 26, 2012 at 3:29 PM, Andrew Morton
> <akpm@linux-foundation.org> wrote:
> > s/fix/break/? __"Originally-from" is not a recognised tag. __If this code
> > is based upon an earlier version from Paul then Signed-off-by: is
> > correct.
> >
> > What's going on here? __What are you trying to communicate?
> 
> I'm trying to take Ingo's suggestion, in the thread "[patch 1/8] x86,
> olpc: add debugfs interface for EC commands" he wrote:
> 
> ====
> This is not a valid signoff sequence - the 'From: ' author of
> the patch must be the first SOB line.
> 
> The way to do this is either to have a:
> 
>  From: Paul Fox <pgf@laptop.org>
> 
> or to covert Paul Fox's SOB to a credit line, such as:
> 
>  Originally-from: Paul Fox <pgf@laptop.org>
> ====
> 
> The original code was from Paul Fox. I changed it somewhat
> significantly, and Paul approves of the end result.
> Can someone suggest a way of expressing this, including tag ordering,
> that will be accepted by all parties?  :)

From: Daniel Drake <dsd@laptop.org>

....

The original code was from Paul Fox.  I changed it somewhat
significantly, and Paul approves of the end result.

Signed-off-by: Paul Fox <pgf@laptop.org>
Signed-off-by: Daniel Drake <dsd@laptop.org>


If I see a changelog like that, I will assume that Paul is OK with you
having the primary authorship (although some suspicion remains ;)). 
Especially when both individuals are from the same organization.

You could swap the order of the Signed-off-by: lines, but the ordering
of those lines isn't at all a reliable indication of anything.  So
nobody should actually *use* the ordering for any purpose.  It is best
to explicitly clarify these things in the changelog text.

Re: [PATCH v4] x86, olpc: add debugfs interface for EC commands

[Andrew Morton]

On Mon, 26 Mar 2012 15:47:08 -0700
"H. Peter Anvin" <hpa@zytor.com> wrote:

> On 03/26/2012 03:45 PM, Daniel Drake wrote:
> > On Mon, Mar 26, 2012 at 3:29 PM, Andrew Morton
> > <akpm@linux-foundation.org> wrote:
> >> s/fix/break/?  "Originally-from" is not a recognised tag.  If this code
> >> is based upon an earlier version from Paul then Signed-off-by: is
> >> correct.
> >>
> >> What's going on here?  What are you trying to communicate?
> > 
> > I'm trying to take Ingo's suggestion, in the thread "[patch 1/8] x86,
> > olpc: add debugfs interface for EC commands" he wrote:
> > 
> > ====
> > This is not a valid signoff sequence - the 'From: ' author of
> > the patch must be the first SOB line.
> > 
> > The way to do this is either to have a:
> > 
> >  From: Paul Fox <pgf@laptop.org>
> > 
> > or to covert Paul Fox's SOB to a credit line, such as:
> > 
> >  Originally-from: Paul Fox <pgf@laptop.org>
> > ====
> > 
> > The original code was from Paul Fox. I changed it somewhat
> > significantly, and Paul approves of the end result.
> > Can someone suggest a way of expressing this, including tag ordering,
> > that will be accepted by all parties?  :)
> > 
> 
> My recommendation is:
> 
> Originally-by: Paul Fox <pgf@laptop.org>
> 
> ... followed by Signed-off-by: in the order of patch flow.  Approving a
> patch, when not passing through, is indicated by Acked-by: or Reviewed-by:

I'd be OK with that, but first we should define what "Originally-by:"
means!  I'd want it to mean that a) Paul was the original author and b)
Paul approves the recording of Daniel as the primary author.

Re: [PATCH v4] x86, olpc: add debugfs interface for EC commands

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> > Signed-off-by: Daniel Drake <dsd@laptop.org>
> > Originally-from: Paul Fox <pgf@laptop.org>
> > Cc: Ingo Molnar <mingo@elte.hu>
> > Cc: Thomas Gleixner <tglx@linutronix.de>
> > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > Cc: Andres Salomon <dilinger@queued.net>
> > 
> > ...
> >
> > v4: really fix sign-off tags
> 
> s/fix/break/?  "Originally-from" is not a recognised tag.  If this code
> is based upon an earlier version from Paul then Signed-off-by: is
> correct.

No, the original ordering was *not* correct:

  From: Daniel Drake <dsd@laptop.org>

  [...]

  Signed-off-by: Daniel Drake <dsd@laptop.org>
  Signed-off-by: Paul Fox <pgf@laptop.org>

In the previous discussion we had I explained what the rules for 
signoffs are. Let me quote Linus as well:

  " The sign-off chain should be very simple: the first person 
    to sign off should be the author, and the last person to 
    sign off should be the committer. "

  https://lkml.org/lkml/2012/3/22/489
  
This is not true for this patch, because the first signoff does 
not match the 'From:' line (author).

Nor is the last signoff the committer - i.e. the person sending 
me this patch to apply. Every maintainer along the route adds a 
signoff to the tail if it's propagated via email, or does a 
merge commit if it's a pull.

If Daniel sends me a patch he should be the last signoff. If he 
authored the patch then he should also be the first (and, by 
implication, only) signoff. Signed-off-by does not recognize 
multiple authorship - that has to be written into the changelog, 
added via another type of tag - either approach is fine to me. 

What I cannot do is to apply patches that have visibly broken 
signoff chains.

Thanks,

	Ingo

Re: [PATCH v4] x86, olpc: add debugfs interface for EC commands

[Andrew Morton]

On Tue, 27 Mar 2012 01:14:08 +0200
Ingo Molnar <mingo@kernel.org> wrote:

> 
> * Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > > Signed-off-by: Daniel Drake <dsd@laptop.org>
> > > Originally-from: Paul Fox <pgf@laptop.org>
> > > Cc: Ingo Molnar <mingo@elte.hu>
> > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > > Cc: Andres Salomon <dilinger@queued.net>
> > > 
> > > ...
> > >
> > > v4: really fix sign-off tags
> > 
> > s/fix/break/?  "Originally-from" is not a recognised tag.  If this code
> > is based upon an earlier version from Paul then Signed-off-by: is
> > correct.
> 
> No, the original ordering was *not* correct:
> 
>   From: Daniel Drake <dsd@laptop.org>
> 
>   [...]
> 
>   Signed-off-by: Daniel Drake <dsd@laptop.org>
>   Signed-off-by: Paul Fox <pgf@laptop.org>
> 
> In the previous discussion we had I explained what the rules for 
> signoffs are. Let me quote Linus as well:
> 
>   " The sign-off chain should be very simple: the first person 
>     to sign off should be the author, and the last person to 
>     sign off should be the committer. "
> 
>   https://lkml.org/lkml/2012/3/22/489
>   
> This is not true for this patch, because the first signoff does 
> not match the 'From:' line (author).
> 
> Nor is the last signoff the committer - i.e. the person sending 
> me this patch to apply. Every maintainer along the route adds a 
> signoff to the tail if it's propagated via email, or does a 
> merge commit if it's a pull.
> 
> If Daniel sends me a patch he should be the last signoff. If he 
> authored the patch then he should also be the first (and, by 
> implication, only) signoff. Signed-off-by does not recognize 
> multiple authorship - that has to be written into the changelog, 
> added via another type of tag - either approach is fine to me. 

That's a bunch of stuff which you and Linus apparently cooked up and
didn't tell anyone about and didn't document anywhere.  I'd never heard
about it before and I doubt if many other people knew about it.  And if
anyone should have known about it, I should have!

So we have an unknown but probably large number of patches in the tree
now which do not follow this rule.  So nobody can depend on
Signed-off-by: ordering in the tree as it stands.

So if we want to implement this (new!) rule then let's write the damn
thing down (in Documentation/SubmittingPatches) and tell people about
it!  And, if poss, add a checkpatch rule to detect possible violations.

Re: [PATCH v4] x86, olpc: add debugfs interface for EC commands

[Ingo Molnar]

* Andrew Morton <akpm@linux-foundation.org> wrote:

> On Tue, 27 Mar 2012 01:14:08 +0200
> Ingo Molnar <mingo@kernel.org> wrote:
> 
> > 
> > * Andrew Morton <akpm@linux-foundation.org> wrote:
> > 
> > > > Signed-off-by: Daniel Drake <dsd@laptop.org>
> > > > Originally-from: Paul Fox <pgf@laptop.org>
> > > > Cc: Ingo Molnar <mingo@elte.hu>
> > > > Cc: Thomas Gleixner <tglx@linutronix.de>
> > > > Cc: "H. Peter Anvin" <hpa@zytor.com>
> > > > Cc: Andres Salomon <dilinger@queued.net>
> > > > 
> > > > ...
> > > >
> > > > v4: really fix sign-off tags
> > > 
> > > s/fix/break/?  "Originally-from" is not a recognised tag.  If this code
> > > is based upon an earlier version from Paul then Signed-off-by: is
> > > correct.
> > 
> > No, the original ordering was *not* correct:
> > 
> >   From: Daniel Drake <dsd@laptop.org>
> > 
> >   [...]
> > 
> >   Signed-off-by: Daniel Drake <dsd@laptop.org>
> >   Signed-off-by: Paul Fox <pgf@laptop.org>
> > 
> > In the previous discussion we had I explained what the rules for 
> > signoffs are. Let me quote Linus as well:
> > 
> >   " The sign-off chain should be very simple: the first person 
> >     to sign off should be the author, and the last person to 
> >     sign off should be the committer. "
> > 
> >   https://lkml.org/lkml/2012/3/22/489
> >   
> > This is not true for this patch, because the first signoff does 
> > not match the 'From:' line (author).
> > 
> > Nor is the last signoff the committer - i.e. the person sending 
> > me this patch to apply. Every maintainer along the route adds a 
> > signoff to the tail if it's propagated via email, or does a 
> > merge commit if it's a pull.
> > 
> > If Daniel sends me a patch he should be the last signoff. If he 
> > authored the patch then he should also be the first (and, by 
> > implication, only) signoff. Signed-off-by does not recognize 
> > multiple authorship - that has to be written into the changelog, 
> > added via another type of tag - either approach is fine to me. 
> 
> That's a bunch of stuff which you and Linus apparently cooked 
> up and didn't tell anyone about and didn't document anywhere. 
> [...]

I certainly didn't cook it up with Linus, and I think it's 
pretty clearly written down in SubmittingPatches:

        Signed-off-by: Random J Developer <random@developer.example.org>
        [lucky@maintainer.example.org: struct foo moved from foo.c to foo.h]
        Signed-off-by: Lucky K Maintainer <lucky@maintainer.example.org>

It even describes how maintainers append themselves to after the 
existing Signed-off-by chain - and describes how the author of 
the patch adds the first Signed-off-by.

So I think it's pretty clearly defined.

> [...]  I'd never heard about it before and I doubt if many 
> other people knew about it.  And if anyone should have known 
> about it, I should have!
>
> So we have an unknown but probably large number of patches in 
> the tree now which do not follow this rule. [...]

I think all the major Git trees are doing it the way described 
in SubmittingPatches: first signoff is author and maintainers 
append a Signed-off-by to the end.

In practice there's a large number of noise in the tree through, 
as with any information created by humans, to be read by humans 
and not checked by computers. But this does not justify the 
intentional creation of noise I think.

Thanks,

	Ingo

Re: [PATCH v4] x86, olpc: add debugfs interface for EC commands

[Andres Salomon]

On Mon, 26 Mar 2012 15:53:59 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Mon, 26 Mar 2012 16:45:10 -0600
> Daniel Drake <dsd@laptop.org> wrote:
> 
> > On Mon, Mar 26, 2012 at 3:29 PM, Andrew Morton
> > <akpm@linux-foundation.org> wrote:
> > > s/fix/break/? __"Originally-from" is not a recognised tag. __If
> > > this code is based upon an earlier version from Paul then
> > > Signed-off-by: is correct.
> > >
> > > What's going on here? __What are you trying to communicate?
> > 
> > I'm trying to take Ingo's suggestion, in the thread "[patch 1/8]
> > x86, olpc: add debugfs interface for EC commands" he wrote:
> > 
> > ====
> > This is not a valid signoff sequence - the 'From: ' author of
> > the patch must be the first SOB line.
> > 
> > The way to do this is either to have a:
> > 
> >  From: Paul Fox <pgf@laptop.org>
> > 
> > or to covert Paul Fox's SOB to a credit line, such as:
> > 
> >  Originally-from: Paul Fox <pgf@laptop.org>
> > ====
> > 
> > The original code was from Paul Fox. I changed it somewhat
> > significantly, and Paul approves of the end result.
> > Can someone suggest a way of expressing this, including tag
> > ordering, that will be accepted by all parties?  :)
> 
> From: Daniel Drake <dsd@laptop.org>
> 
> ....
> 
> The original code was from Paul Fox.  I changed it somewhat
> significantly, and Paul approves of the end result.
> 
> Signed-off-by: Paul Fox <pgf@laptop.org>
> Signed-off-by: Daniel Drake <dsd@laptop.org>


This makes a lot more sense to me (and has been used in the past by
both myself and dsd for OLPC submissions) rather than having a plethora
of machine-parsable tags that no one cares about.  Minus the "approves
of the end result" bit, though, as that's implicit in the SOB line.
There are innumerable possible variants of this (Originally-by,
Co-authored-with, Based-on-code-in-another-project-by,
Written-while-being-whipped-by, and so on).

Re: [PATCH v4] x86, olpc: add debugfs interface for EC commands

[Joe Perches]

On Mon, 2012-03-26 at 16:23 -0700, Andrew Morton wrote:
> if poss, add a checkpatch rule to detect possible violations.

I don't see how that's possible.

Re: [PATCH v4] x86, olpc: add debugfs interface for EC commands

[Matthew Garrett]

On Mon, Mar 26, 2012 at 02:51:46PM -0700, Andrew Morton wrote:

> Well, if a person had any contribution at all, we should seek their
> Signed-off-by:.  Otherwise they could say "hey, you admitted using my
> code but I did not authorise its use", or any other range of bad IANAL
> things?

The Certificate of Origin says:

        (b) The contribution is based upon previous work that, to the best
            of my knowledge, is covered under an appropriate open source
            license and I have the right under that license to submit that
            work with modifications, whether created in whole or in part
            by me, under the same open source license (unless I am
            permitted to submit under a different license), as indicated
            in the file; or

If a previous author has given a Signed-off-by then they've clearly 
indicated that their code is under an appropriate license and may be 
submitted to the kernel. If someone else then takes that code, modifies 
it and submits it then there's no obvious reason why we still need the 
original Signed-off-by. Giving credit to the original author is 
obviously appropriate, but I don't see why we need any more than that.

-- 
Matthew Garrett | mjg59@srcf.ucam.org

Re: [PATCH v4] x86, olpc: add debugfs interface for EC commands

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 817 bytes --]

On Tue, 27 Mar 2012 17:07:14 +0100, Matthew Garrett said:

> If a previous author has given a Signed-off-by then they've clearly
> indicated that their code is under an appropriate license and may be
> submitted to the kernel. If someone else then takes that code, modifies
> it and submits it then there's no obvious reason why we still need the
> original Signed-off-by. Giving credit to the original author is
> obviously appropriate, but I don't see why we need any more than that.

Conversely, if a maintainer does fixups on a patch while commiting
it, we *don't* discard the original Signed-off-by.   I'd say if somebody's
submitting somebody else's patch, the chain should be:

signed-off-by: original author
signed-off-by: the person who did the updates
signed-off-by: <chain of maintainers heading upstream>

[-- Attachment #2: Type: application/pgp-signature, Size: 865 bytes --]