[PATCH] FIXUP: CHROMIUM: fix transposed param settings

[PATCH] FIXUP: CHROMIUM: fix transposed param settings

[Nick Vaccaro]

The __cros_ec_pwm_get_duty() routine was transposing the insize and
outsize fields when calling cros_ec_cmd_xfer_status().

The original code worked without error due to size of the two particular
parameter blocks passed to cros_ec_cmd_xfer_status(), so this change is
not fixing an actual runtime problem, just correcting the calling usage.

Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org>
---
 drivers/pwm/pwm-cros-ec.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
index 2e4ab20cfb83..de5b7c9860b6 100644
--- a/drivers/pwm/pwm-cros-ec.c
+++ b/drivers/pwm/pwm-cros-ec.c
@@ -75,8 +75,8 @@ static int __cros_ec_pwm_get_duty(struct cros_ec_device *ec, u8 index,
 
 	msg->version = 0;
 	msg->command = EC_CMD_PWM_GET_DUTY;
-	msg->insize = sizeof(*params);
-	msg->outsize = sizeof(*resp);
+	msg->insize = sizeof(*resp);
+	msg->outsize = sizeof(*params);
 
 	params->pwm_type = EC_PWM_TYPE_GENERIC;
 	params->index = index;
-- 
2.12.2

Re: [PATCH] FIXUP: CHROMIUM: fix transposed param settings

[Brian Norris]

Hi Nick,

When sending patches to kernel mailing lists, we don't use prefixes like
"CHROMIUM" -- those only apply to Chrome OS kernel trees, to indicate
patches that should be specific to the Chromium (OS) project and not
necessarily upstream Linux.

Here, you want to follow the patterns used by the subsystem. This is
sort of covered in the Documentation/process/ directory:

https://www.kernel.org/doc/html/latest/process/submitting-patches.html#the-canonical-patch-format

Or you might look at 'git log drivers/pwm/ here.

i.e., this might have a subject:

[PATCH] pwm: cros-ec: fix transposed param settings

On Thu, Jun 22, 2017 at 10:54:39AM -0700, Nick Vaccaro wrote:
> The __cros_ec_pwm_get_duty() routine was transposing the insize and
> outsize fields when calling cros_ec_cmd_xfer_status().
> 
> The original code worked without error due to size of the two particular
> parameter blocks passed to cros_ec_cmd_xfer_status(), so this change is
> not fixing an actual runtime problem, just correcting the calling usage.
> 
> Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org>
> ---

Patch looks good:

Reviewed-by: Brian Norris <briannorris@chromium.org>

>  drivers/pwm/pwm-cros-ec.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
> index 2e4ab20cfb83..de5b7c9860b6 100644
> --- a/drivers/pwm/pwm-cros-ec.c
> +++ b/drivers/pwm/pwm-cros-ec.c
> @@ -75,8 +75,8 @@ static int __cros_ec_pwm_get_duty(struct cros_ec_device *ec, u8 index,
>  
>  	msg->version = 0;
>  	msg->command = EC_CMD_PWM_GET_DUTY;
> -	msg->insize = sizeof(*params);
> -	msg->outsize = sizeof(*resp);
> +	msg->insize = sizeof(*resp);
> +	msg->outsize = sizeof(*params);
>  
>  	params->pwm_type = EC_PWM_TYPE_GENERIC;
>  	params->index = index;

Re: [PATCH] FIXUP: CHROMIUM: fix transposed param settings

[Nick Vaccaro]

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

Hi Brian,
Thanks for the info, I'm new to this process.

I was asked to create a FIXUP CL and send it upstream.  Not knowing format
of a "FIXUP" cl, I looked at other FIXUP cl's in the chromeos kernel and
followed suit.
I emailed it upstream with a "FIXUP: CHROMIUM:" prefix. Should I change the
checkin comment title line to "[PATCH] pwm: cros-ec: fix transposed param
settings" and resend the email?
Please advise, thanks.

On Fri, Jun 23, 2017 at 1:23 PM, Brian Norris <briannorris@chromium.org>
wrote:

> Hi Nick,
>
> When sending patches to kernel mailing lists, we don't use prefixes like
> "CHROMIUM" -- those only apply to Chrome OS kernel trees, to indicate
> patches that should be specific to the Chromium (OS) project and not
> necessarily upstream Linux.
>
> Here, you want to follow the patterns used by the subsystem. This is
> sort of covered in the Documentation/process/ directory:
>
> https://www.kernel.org/doc/html/latest/process/
> submitting-patches.html#the-canonical-patch-format
>
> Or you might look at 'git log drivers/pwm/ here.
>
> i.e., this might have a subject:
>
> [PATCH] pwm: cros-ec: fix transposed param settings
>
> On Thu, Jun 22, 2017 at 10:54:39AM -0700, Nick Vaccaro wrote:
> > The __cros_ec_pwm_get_duty() routine was transposing the insize and
> > outsize fields when calling cros_ec_cmd_xfer_status().
> >
> > The original code worked without error due to size of the two particular
> > parameter blocks passed to cros_ec_cmd_xfer_status(), so this change is
> > not fixing an actual runtime problem, just correcting the calling usage.
> >
> > Signed-off-by: Nick Vaccaro <nvaccaro@chromium.org>
> > ---
>
> Patch looks good:
>
> Reviewed-by: Brian Norris <briannorris@chromium.org>
>
> >  drivers/pwm/pwm-cros-ec.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/pwm/pwm-cros-ec.c b/drivers/pwm/pwm-cros-ec.c
> > index 2e4ab20cfb83..de5b7c9860b6 100644
> > --- a/drivers/pwm/pwm-cros-ec.c
> > +++ b/drivers/pwm/pwm-cros-ec.c
> > @@ -75,8 +75,8 @@ static int __cros_ec_pwm_get_duty(struct
> cros_ec_device *ec, u8 index,
> >
> >       msg->version = 0;
> >       msg->command = EC_CMD_PWM_GET_DUTY;
> > -     msg->insize = sizeof(*params);
> > -     msg->outsize = sizeof(*resp);
> > +     msg->insize = sizeof(*resp);
> > +     msg->outsize = sizeof(*params);
> >
> >       params->pwm_type = EC_PWM_TYPE_GENERIC;
> >       params->index = index;
>

[-- Attachment #2: Type: text/html, Size: 3435 bytes --]

Re: [PATCH] FIXUP: CHROMIUM: fix transposed param settings

[Brian Norris]

(Kernel mailing lists don't usually like HTML mail. Gmail web interface
can get plain text if you really try, in one of the 'compose' options.
But it'll always screw up patch formatting, so it's only worth light
use)

On Fri, Jun 23, 2017 at 02:04:44PM -0700, Nick Vaccaro wrote:
>    Hi Brian,
>    Thanks for the info, I'm new to this process.

No problem, and understood.

>    I was asked to create a FIXUP CL and send it upstream.  Not knowing
>    format of a "FIXUP" cl, I looked at other FIXUP cl's in the chromeos
>    kernel and followed suit.

There are various reasons we might use a "FIXUP" prefix within the
Chrome OS kernel; among them: a patch might diverge upstream, as it gets
reviewed, but we might already have applied the patch to our tree.

In this case, you have a totally new patch.

[If you wanted to represent the "FIXUP" concept here, you might use a
tag like this, before the Signed-off-by:

Fixes: 1f0d3bb02785 ("pwm: Add ChromeOS EC PWM driver")
]

If in doubt, check the git log for examples (as you did already), but
note that there are different practices for mainline Linux and for the
Chromium OS, as Chromium OS is a downstream project.

>    I emailed it upstream with a "FIXUP: CHROMIUM:" prefix. Should I change
>    the checkin comment title line to "[PATCH] pwm: cros-ec: fix transposed
>    param settings" and resend the email?

Yes, that'd be all.

(And you can include my 'Reviewed-by:' tag when you resend.)

>    Please advise, thanks.

Brian