Hi Matthias,

On 2019-01-12 05:08, Matthias Kaehlcke wrote:

On Fri, Jan 11, 2019 at 08:02:00PM +0530, Balakrishna Godavarthi wrote:

quoted

On 2019-01-11 06:25, Matthias Kaehlcke wrote:

quoted

On Thu, Jan 10, 2019 at 08:18:37PM +0530, Balakrishna Godavarthi wrote:

quoted

Hi Johan,

On 2019-01-09 20:08, Johan Hovold wrote:

quoted

On Fri, Dec 21, 2018 at 05:59:47PM -0800, Matthias Kaehlcke wrote:

quoted

On Thu, Dec 20, 2018 at 08:16:35PM +0530, Balakrishna Godavarthi
wrote:

quoted

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 <redacted>
---
 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);

Again, do you really want to wait indefinitely here?

[Bala]: these commands are mandatory to turn ON or OFF the chip.
        so blocking to the max time is required.
        these commands are sent during the BT chip ON & OFF.
        in the latest series, i have flushed the uart before sending
this
commands
        so the uart FIFO(as just opened the port before calling this
function) or the circular
        buffer will be empty and also i am disabling the flow
control too.
       https://patchwork.kernel.org/patch/10744435/

The commands may be mandatory for switching the chip on or off, but
what is better if there is a problem with sending them (e.g. a buggy
UART driver):

1. wait a reasonable time, report an error
2. wait forever

?

If the single byte command couldn't be sent after a few milliseconds,
it likely never will, waiting forever doesn't fix that. An error
report at least provides some information about the problem and the
driver is in a not-hanging state.

Cheers

Matthias

[Bala]: will update this with a bound TIMEOUT value. But 
wait_until_sent()
is void return
        type how could we know that the data is sent out on the lines.

Good point, I didn't check and expected it to return an error. If you
feel really motivated and have maintainer support you could possibly
change the API, however it seems this would be a somewhat larger
change.

I guess the next best thing to do is to proceed as if all data was
sent and if there was a problem it will likely manifest through
another error (especially for the ON pulse), which still seems better
than a hanging driver.

Cheers

Matthias

[Bala]: sure, will add the timeout to one second and if data didn't sent 
to the lines anyways
         we will get an version command timeouts errors.

-- 
Regards
Balakrishna.