linux-kernel - Re: [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Thu, 27 Dec 2018 08:53:56 +0530
From:   Balakrishna Godavarthi <bgodavar@...eaurora.org>
To:     Matthias Kaehlcke <mka@...omium.org>
Cc:     marcel@...tmann.org, johan.hedberg@...il.com, johan@...nel.org,
        linux-kernel@...r.kernel.org, linux-bluetooth@...r.kernel.org,
        hemantg@...eaurora.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH v5 1/5] Bluetooth: hci_qca: use wait_until_sent() for
 power pulses

Hi Matthias,

On 2018-12-27 03:51, Matthias Kaehlcke wrote:
> On Wed, Dec 26, 2018 at 12:01:51PM +0530, Balakrishna Godavarthi wrote:
>> Hi Matthias,
>> 
>> On 2018-12-22 07:29, Matthias Kaehlcke wrote:
>> > On Thu, Dec 20, 2018 at 08:16:35PM +0530, Balakrishna Godavarthi wrote:
>> > > wcn3990 requires a power pulse to turn ON/OFF along with
>> > > regulators. Sometimes we are observing the power pulses are sent
>> > > out with some time delay, due to queuing these commands. This is
>> > > causing synchronization issues with chip, which intern delay the
>> > > chip setup or may end up with communication issues.
>> > >
>> > > Signed-off-by: Balakrishna Godavarthi <bgodavar@...eaurora.org>
>> > > ---
>> > >  drivers/bluetooth/hci_qca.c | 38
>> > > ++++++++++++++-----------------------
>> > >  1 file changed, 14 insertions(+), 24 deletions(-)
>> > >
>> > > diff --git a/drivers/bluetooth/hci_qca.c b/drivers/bluetooth/hci_qca.c
>> > > index f036c8f98ea3..5a07c2370289 100644
>> > > --- a/drivers/bluetooth/hci_qca.c
>> > > +++ b/drivers/bluetooth/hci_qca.c
>> > > @@ -1013,11 +1013,9 @@ static inline void host_set_baudrate(struct
>> > > hci_uart *hu, unsigned int speed)
>> > >  		hci_uart_set_baudrate(hu, speed);
>> > >  }
>> > >
>> > > -static int qca_send_power_pulse(struct hci_dev *hdev, u8 cmd)
>> > > +static int qca_send_power_pulse(struct hci_uart *hu, u8 cmd)
>> > >  {
>> > > -	struct hci_uart *hu = hci_get_drvdata(hdev);
>> > > -	struct qca_data *qca = hu->priv;
>> > > -	struct sk_buff *skb;
>> > > +	int ret;
>> > >
>> > >  	/* These power pulses are single byte command which are sent
>> > >  	 * at required baudrate to wcn3990. On wcn3990, we have an external
>> > > @@ -1029,19 +1027,16 @@ static int qca_send_power_pulse(struct
>> > > hci_dev *hdev, u8 cmd)
>> > >  	 * save power. Disabling hardware flow control is mandatory while
>> > >  	 * sending power pulses to SoC.
>> > >  	 */
>> > > -	bt_dev_dbg(hdev, "sending power pulse %02x to SoC", cmd);
>> > > -
>> > > -	skb = bt_skb_alloc(sizeof(cmd), GFP_KERNEL);
>> > > -	if (!skb)
>> > > -		return -ENOMEM;
>> > > -
>> > > +	bt_dev_dbg(hu->hdev, "sending power pulse %02x to SoC", cmd);
>> > >  	hci_uart_set_flow_control(hu, true);
>> > > +	ret = serdev_device_write_buf(hu->serdev, &cmd, sizeof(cmd));
>> > > +	if (ret < 0) {
>> > > +		bt_dev_err(hu->hdev, "failed to send power pulse %02x to SoC",
>> > > +			   cmd);
>> > > +		return ret;
>> > > +	}
>> > >
>> > > -	skb_put_u8(skb, cmd);
>> > > -	hci_skb_pkt_type(skb) = HCI_COMMAND_PKT;
>> > > -
>> > > -	skb_queue_tail(&qca->txq, skb);
>> > > -	hci_uart_tx_wakeup(hu);
>> > > +	serdev_device_wait_until_sent(hu->serdev, 0);
>> >
>> > serdev_device_wait_until_sent() might only guarantee that the UART
>> > circular buffer is empty (see
>> > https://elixir.bootlin.com/linux/v4.19/source/drivers/tty/tty_ioctl.c#L225),
>> > not that the data has actually sent (e.g. it might be sitting in
>> > the UART FIFO). However with this:
>> >
>> > >  	/* Wait for 100 uS for SoC to settle down */
>> > >  	usleep_range(100, 200);
>> >
>> > we should probably be fine, unless there's tons of data in the
>> > FIFO.
>> >
>> > You currently call serdev_device_write_flush() in
>> > qca_power_shutdown(), I wonder if it would make sense to call it in
>> > qca_send_power_pulse(), regardless of whether it's an on or off
>> 
>> [Bala]: during sending the ON pulse we will not have any data in the
>>         UART circular buffer as this is the first command to send and 
>> we are
>> sending it as soon as we open the port.
>>         so i taught why should be flush the circular as it is already 
>> empty.
> 
>> > pulse. In any case we don't care if the chip receives any 'pending'
>> > data when we switch it on or off, right?
>> >
>> 
>> [Bala]: during on we freshly open port and i see that flush() called 
>> while
>> port opening.
>> 
>> https://elixir.bootlin.com/linux/latest/source/drivers/tty/serial/serial_core.c#L207
> 
> I would argue that the serdev_device_write_flush() call in
> qca_power_shutdown() is related with/needed for sending the power off
> pulse, hence it should be part of qca_send_power_pulse(), unless it
> adds a significant overhead and we really want to call it only in the
> shutdown case.
> 
> Flushing the buffer should be fairly lightweight and power pulses are
> only sent when the device is switched on or off, so the overhead
> should be negligible. You *could* restrict the flush to the power off
> pulse, assuming that the driver always re-opens the port in
> qca_wcn3990_init() (tests with this patch set suggest this might not
> be needed) and that serdev_device_open() flushes the buffer (which
> seems a sane assumption). Yet given the minimal overhead I'd suggest
> to not make assumptions about what happened previously in other code
> and avoid the clutter of a condition that doesn't add much value.
> 
[Bala]: will call the flush() while sending the power pulses 
irrespective of the pulse type.

> Cheers
> 
> Matthias

-- 
Regards
Balakrishna.