| 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
| ||
|
Message-ID: <75a8b12023f38a8af5e281ea1902cb73@codeaurora.org>
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.
Powered by blists - more mailing lists