Re: [PATCH] USB: kobil_sct: Remove unused transfer buffer allocs

[Johan Hovold <johan@kernel.org>]

Hi Peter,

Forgot to reply to this one.

On Wed, Oct 22, 2014 at 07:40:20AM -0400, Peter Hurley wrote:
> On 10/19/2014 01:12 PM, Johan Hovold wrote:
> > [ +CC: Jiri, Alan, linux-serial ]
> > 
> > On Thu, Oct 16, 2014 at 02:09:29PM -0400, Peter Hurley wrote:
> >> On 10/16/2014 01:59 PM, Peter Hurley wrote:

> >>> @@ -541,10 +531,6 @@ static int kobil_ioctl(struct tty_struct *tty,
> >>>  
> >>>  	switch (cmd) {
> >>>  	case TCFLSH:
> >>> -		transfer_buffer = kmalloc(transfer_buffer_length, GFP_KERNEL);
> >>> -		if (!transfer_buffer)
> >>> -			return -ENOBUFS;
> >>> -
> >>>  		result = usb_control_msg(port->serial->dev,
> >>>  			  usb_sndctrlpipe(port->serial->dev, 0),
> >>>  			  SUSBCRequest_Misc,
> >>> @@ -559,7 +545,6 @@ static int kobil_ioctl(struct tty_struct *tty,
> >>>  		dev_dbg(&port->dev,
> >>>  			"%s - Send reset_all_queues (FLUSH) URB returns: %i\n",
> >>>  			__func__, result);
> >>> -		kfree(transfer_buffer);
> >>>  		return (result < 0) ? -EIO: 0;
> >>                                            ^^^
> >> Returning 0 is almost certainly wrong; no further processing for
> >> TCFLSH is performed.
> > 
> > Indeed.
> > 
> >> Only this driver returns 0 (of all the tty drivers in mainline).
> >>
> >> Returning -ENOIOCTLCMD allows further processing to continue;
> >> especially the line discipline's input flushing, if TCIFLUSH/TCIOFLUSH.
> > 
> > That doesn't seem like a very good idea, and only two *staging* drivers
> > try to play such games (i.e. pretending not to implement the ioctl) as
> > far as I can see.
> 
> Well, returning EIONOCTLCMD is the standard method of ioctl passthrough
> from driver to line discipline.

I disagree with you there. AFAICS only these two staging drivers are
abusing the meaning of EIONOCTLCMD (unrecognised ioctl) to have the
line discipline also act on the ioctl.

> Since driver 'input buffer' flushing is not currently supported by the
> core, this seems the only available workaround.

That is true. But I doubt we should use these two staging drivers as a
model for how this should be handled, if it's at all needed.

> > The only non-staging tty driver which appears to implement TCFLSH,
> > ipwireless, calls tty_perform_flush directly to flush the ldisc buffers.
> > That doesn't seem right either.
> 
> I'm not sure why ipwireless does this; I can only guess that it's a
> workaround for some line discipline that doesn't use n_tty_ioctl_helper().
> 
> > Shouldn't this be fixed by removing TCFLSH from these tty drivers'
> > ioctl callbacks and implementing flush_buffer()?
> >
> > The staging drivers also flush a device input buffer, which could be
> > done in a new callback if at all needed.
> 
> Yeah, that's why the Digi staging drivers are trapping TCFLSH; so they
> can clear input buffers on TCIFLUSH/TCIOFLUSH.
> 
> I'd like to better understand the hardware and driver before extending
> the core interface; this driver may not even run.

Agreed.

> For example, this driver clears its 'input buffer' for
> tcsetattr(TCSADRAIN or TCSAFLUSH). But that doesn't make sense considering
> that the flip buffers could have data in them that isn't flushed; the tty
> core doesn't dump the flip buffers because 'input processing' has not
> happened on that data.
> 
> I think when/if these drivers are promoted is when/if the core interface
> should address this. Just my opinion, though :)

I agree.

Johan