Re: [PATCH v2 1/9] Bluetooth: hci_uart_set_flow_control: Fix NULL deref when using serdev

[Johan Hovold <johan@kernel.org>]

On Mon, Oct 09, 2017 at 04:06:02PM +0200, Hans de Goede wrote:

> On 07-10-17 16:36, Johan Hovold wrote:
> > On Wed, Oct 04, 2017 at 08:43:35PM +0200, Hans de Goede wrote:

> >> @@ -298,6 +299,12 @@ void hci_uart_set_flow_control(struct hci_uart *hu, bool enable)
> >>   	unsigned int set = 0;
> >>   	unsigned int clear = 0;
> >>   
> >> +	if (hu->serdev) {
> >> +		serdev_device_set_flow_control(hu->serdev, !enable);
> >> +		serdev_device_set_rts(hu->serdev, !enable);
> > 
> > The order here may matter; in the non-serdev case, rts is raised before
> > enabling flow control.
> 
> AFAIK it should not matter, if RTS is not set before enabling, then CTS
> may not be set by the other side yet and the hw flow-control will
> wait for CTS to get raised.

That's not necessarily how hardware flow control is implemented. When
using auto-RTS, RTS will simply be asserted whenever there's room in the
receive FIFO.

So in fact it seems the hci_ldisc should be doing this in the reverse
order (e.g. deassert RTS before disabling auto-RTS).

Probably doesn't matter in practice here, but I would at least expect
the enable and disable cases to be each others inverses (and for the
ldisc and serdev implementations to match).

Johan