USB: serial: ti_usb_3410_5052: Avoid goto in bulk in callback

[Ladislav Michl <ladis@linux-mips.org>]

On Mon, Dec 11, 2017 at 11:10:35AM +0100, Johan Hovold wrote:
> On Mon, Dec 11, 2017 at 12:09:19AM +0100, Ladislav Michl wrote:
> > Make status handling in bulk in callback function more compact,
> > which renders goto pointless.
> > 
> > Signed-off-by: Ladislav Michl <ladis@linux-mips.org>
> > ---
> >  drivers/usb/serial/ti_usb_3410_5052.c | 36 ++++++++++++++---------------------
> >  1 file changed, 14 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/usb/serial/ti_usb_3410_5052.c b/drivers/usb/serial/ti_usb_3410_5052.c
> > index 6b22857f6e52..2786ec7bbca2 100644
> > --- a/drivers/usb/serial/ti_usb_3410_5052.c
> > +++ b/drivers/usb/serial/ti_usb_3410_5052.c
> > @@ -1219,29 +1219,10 @@ static void ti_bulk_in_callback(struct urb *urb)
> >  
> >  	switch (status) {
> >  	case 0:
> > -		break;
> > -	case -ECONNRESET:
> > -	case -ENOENT:
> > -	case -ESHUTDOWN:
> > -		dev_dbg(dev, "%s - urb shutting down, %d\n", __func__, status);
> > -		return;
> > -	default:
> > -		dev_err(dev, "%s - nonzero urb status, %d\n",
> > -			__func__, status);
> > -	}
> > -
> > -	if (status == -EPIPE)
> > -		goto exit;
> > -
> > -	if (status) {
> > -		dev_err(dev, "%s - stopping read!\n", __func__);
> > -		return;
> > -	}
> > -
> > -	if (urb->actual_length) {
> > +		if (!urb->actual_length)
> > +			break;
> >  		usb_serial_debug_data(dev, __func__, urb->actual_length,
> >  				      urb->transfer_buffer);
> > -
> >  		if (!tport->tp_is_open)
> >  			dev_dbg(dev, "%s - port closed, dropping data\n",
> >  				__func__);
> > @@ -1250,9 +1231,20 @@ static void ti_bulk_in_callback(struct urb *urb)
> >  		spin_lock(&tport->tp_lock);
> >  		port->icount.rx += urb->actual_length;
> >  		spin_unlock(&tport->tp_lock);
> > +		break;
> > +	case -ECONNRESET:
> > +	case -ENOENT:
> > +	case -ESHUTDOWN:
> > +		dev_dbg(dev, "%s - urb shutting down, %d\n", __func__, status);
> > +		return;
> > +	case -EPIPE:
> > +		break;
> > +	default:
> > +		dev_err(dev, "%s - nonzero urb status, %d\n",
> > +			__func__, status);
> > +		return;
> 
> I'm afraid I don't consider this an improvement. I prefer using gotos
> for error paths, while keeping the success path out of the status
> switch.
> 
> Furthermore, this isn't functionally equivalent as we'd not longer log
> an error for -EPIPE.

Yes, you are right... Now, shouldn't we react somehow to stalled endpoint?
Tty side seems to be unaware of it.

> In fact, that -EPIPE is already on my TODO list as we really should not
> be resubmitting URBs for a stalled endpoint in the first place. That in
> turn should allow for some clean up of this callback.

Out of curiosity, how would be stalled endpoint reported?

> Thanks,
> Johan

Thank you,
	ladis
---
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html