Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 6 Jun 2023 13:34:34 +0200
From:   Johan Hovold <johan@kernel.org>
To:     Oliver Neukum <oneukum@suse.com>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org,
        Corey Minyard <minyard@acm.org>
Subject: Re: [PATCH v2 1/3] USB: serial: return errors from break handling
Message-ID: <ZH8ZyjG_D1HiNay5@hovoldconsulting.com>
References: <20230604123505.4661-1-johan@kernel.org>
 <20230604123505.4661-2-johan@kernel.org>
 <88472624-cc92-31a7-f35d-a212e61ab959@suse.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <88472624-cc92-31a7-f35d-a212e61ab959@suse.com>
Precedence: bulk

Hi Oliver,

On Tue, Jun 06, 2023 at 01:13:30PM +0200, Oliver Neukum wrote:
> On 04.06.23 14:35, Johan Hovold wrote:
> > @@ -1077,15 +1077,19 @@ static void pl2303_set_break(struct usb_serial_port *port, bool enable)
> >   	result = usb_control_msg(serial->dev, usb_sndctrlpipe(serial->dev, 0),
> >   				 BREAK_REQUEST, BREAK_REQUEST_TYPE, state,
> >   				 0, NULL, 0, 100);
> > -	if (result)
> > +	if (result) {
> >   		dev_err(&port->dev, "error sending break = %d\n", result);
> > +		return result;
> > +	}
> > +
> > +	return 0;
> >   }

And thanks for taking a look.

> this code was always fishy, but I am afraid it worked by accident albeit
> spamming the logs.
> If I may quote from the kerneldoc of usb_control_msg():
> 
>   * usb_control_msg - Builds a control urb, sends it off and waits for completion
> 
> [..]
> 
>   * Return: If successful, the number of bytes transferred. Otherwise, a negative
>   * error number.
>   */
> 
> You need to test for < 0, not != 0

No, the current code is just fine as there is no data stage so
usb_control_msg() returns 0 or negative errno.

Johan