[PATCH] USB: serial: iuu_phoenix: fix led-activity helpers

[PATCH] USB: serial: iuu_phoenix: fix led-activity helpers

[Johan Hovold]

The set-led command is eight bytes long and starts with a command byte
followed by six bytes of RGB data and ends with a byte encoding a
frequency (see iuu_led() and iuu_rgbf_fill_buffer()).

The led activity helpers had a few long-standing bugs which corrupted
the command packets by inserting a second command byte and thereby
offsetting the RGB data and dropping the frequency in non-xmas mode.

In xmas mode, a related off-by-one error left the frequency field
uninitialised.

Fixes: 60a8fc017103 ("USB: add iuu_phoenix driver")
Reported-by: George Spelvin <lkml@sdf.org>
Signed-off-by: Johan Hovold <johan@kernel.org>
---

George,

Thanks for reporting this issue and sorry for not getting back to you
sooner about this. It was sort buried in that long RFC series of yours
which was about something unrelated (and your patch did two distinct
logical changes in one patch which is generally frowned upon):

	https://lore.kernel.org/r/202003281643.02SGhKg2024958@sdf.org

Let me know if you prefer to respin your fix, and then include also the
fix of the related issue in iuu_led_activity_off().

Otherwise I'll apply this one next week.

Johan



 drivers/usb/serial/iuu_phoenix.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/usb/serial/iuu_phoenix.c b/drivers/usb/serial/iuu_phoenix.c
index 0c2c4aea289c..b4ba79123d9d 100644
--- a/drivers/usb/serial/iuu_phoenix.c
+++ b/drivers/usb/serial/iuu_phoenix.c
@@ -350,10 +350,11 @@ static void iuu_led_activity_on(struct urb *urb)
 {
 	struct usb_serial_port *port = urb->context;
 	char *buf_ptr = port->write_urb->transfer_buffer;
-	*buf_ptr++ = IUU_SET_LED;
+
 	if (xmas) {
-		get_random_bytes(buf_ptr, 6);
-		*(buf_ptr+7) = 1;
+		buf_ptr[0] = IUU_SET_LED;
+		get_random_bytes(buf_ptr + 1, 6);
+		buf_ptr[7] = 1;
 	} else {
 		iuu_rgbf_fill_buffer(buf_ptr, 255, 255, 0, 0, 0, 0, 255);
 	}
@@ -370,13 +371,14 @@ static void iuu_led_activity_off(struct urb *urb)
 {
 	struct usb_serial_port *port = urb->context;
 	char *buf_ptr = port->write_urb->transfer_buffer;
+
 	if (xmas) {
 		iuu_rxcmd(urb);
 		return;
-	} else {
-		*buf_ptr++ = IUU_SET_LED;
-		iuu_rgbf_fill_buffer(buf_ptr, 0, 0, 255, 255, 0, 0, 255);
 	}
+
+	iuu_rgbf_fill_buffer(buf_ptr, 0, 0, 255, 255, 0, 0, 255);
+
 	usb_fill_bulk_urb(port->write_urb, port->serial->dev,
 			  usb_sndbulkpipe(port->serial->dev,
 					  port->bulk_out_endpointAddress),
-- 
2.26.2

Re: [PATCH] USB: serial: iuu_phoenix: fix led-activity helpers

[Greg KH]

On Thu, Jul 16, 2020 at 10:50:55AM +0200, Johan Hovold wrote:
> The set-led command is eight bytes long and starts with a command byte
> followed by six bytes of RGB data and ends with a byte encoding a
> frequency (see iuu_led() and iuu_rgbf_fill_buffer()).
> 
> The led activity helpers had a few long-standing bugs which corrupted
> the command packets by inserting a second command byte and thereby
> offsetting the RGB data and dropping the frequency in non-xmas mode.
> 
> In xmas mode, a related off-by-one error left the frequency field
> uninitialised.
> 
> Fixes: 60a8fc017103 ("USB: add iuu_phoenix driver")
> Reported-by: George Spelvin <lkml@sdf.org>
> Signed-off-by: Johan Hovold <johan@kernel.org>


Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH] USB: serial: iuu_phoenix: fix led-activity helpers

[Johan Hovold]

On Thu, Jul 16, 2020 at 11:05:03AM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 16, 2020 at 10:50:55AM +0200, Johan Hovold wrote:
> > The set-led command is eight bytes long and starts with a command byte
> > followed by six bytes of RGB data and ends with a byte encoding a
> > frequency (see iuu_led() and iuu_rgbf_fill_buffer()).
> > 
> > The led activity helpers had a few long-standing bugs which corrupted
> > the command packets by inserting a second command byte and thereby
> > offsetting the RGB data and dropping the frequency in non-xmas mode.
> > 
> > In xmas mode, a related off-by-one error left the frequency field
> > uninitialised.
> > 
> > Fixes: 60a8fc017103 ("USB: add iuu_phoenix driver")
> > Reported-by: George Spelvin <lkml@sdf.org>
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks for reviewing. Now applied for -next.

Johan