Received: by 2002:a05:6358:4e97:b0:b3:742d:4702 with SMTP id ce23csp464380rwb; Thu, 11 Aug 2022 04:56:44 -0700 (PDT) X-Google-Smtp-Source: AA6agR6Tj/ZIvijOKl4/fdl3FEixPWy/6s8Vi9rx4wsHrRWQ1yCRMs6+oBjYFJLnpvbVBlkoGA8x X-Received: by 2002:a17:907:7347:b0:730:ec0f:fd5c with SMTP id dq7-20020a170907734700b00730ec0ffd5cmr22568270ejc.187.1660219004154; Thu, 11 Aug 2022 04:56:44 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1660219004; cv=none; d=google.com; s=arc-20160816; b=jNaOQJVX3WqocldyF5IY2Rli6RtjO0WtkicSb82TuZRtw1y4z1Rw5o2q4hns4Cw1f5 N8hP6VYoGo7xt+4OeAaFyFT+y4XNorwdjlM+KYAjhprnpi3uacbaPrryWBzzUkY6aSo7 ry3v/TwU3BT3jloiGvAMSgNwhq0lK9oifBQeFrhlwq6YU1A/1iSpAqAJqPcI/Ad1HTMZ V1xqmu3G3aWr0edXrZqwJyQYz94lkHXtmXdGkCzZmi3rI6lZGtWK3wiWctI1v7Byaa6C PrWgZMdYOWnm4psidobFRJqPumkkIQ1EQVgJoaTpt1ncSdQwuwny9Zd9zWNDQlxx7f4g yEtg== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:in-reply-to:from :references:cc:to:content-language:subject:user-agent:mime-version :date:message-id:dkim-signature; bh=BHGYpPVOToJJW83BwikwQkKAUWoo3Emiqyb5NZjYFwU=; b=OtSnvVHuLd5+AfAWa5WexHmoIq1FheyF/aW7x0yuaQ/f8qy53EX2hrhO7/it0pILrg YyLJYGpeSldm3y/2drdfM5Xg+3n895rwuUSHDVikOyEoQPLOuyyntRzor9SlII6oqXxX HGHs7wCPrADdZ9JmOwcgN+24w4qPLK+cy/jh0qSIn92qYtVJ53JPjji2P9wT355aN1dZ gi8A43FliZSz4VeXNjmMhHaHuNiaDHJKEkqOAmQeaDOILMfWVFb47AKrcQZ37uwngdx3 g+GBEWFmcToISWdMPUcLeXtaCL1mv9zOj7u6wvBpWg2v2dhycJjNJAOW22yUsdFqURls OzLQ== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=WbJVkwFz; spf=pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-bluetooth-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Return-Path: Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id s10-20020a056402520a00b0044075656bb1si15578561edd.295.2022.08.11.04.55.57; Thu, 11 Aug 2022 04:56:44 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=WbJVkwFz; spf=pass (google.com: domain of linux-bluetooth-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-bluetooth-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S234870AbiHKLwd (ORCPT + 99 others); Thu, 11 Aug 2022 07:52:33 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:56500 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235078AbiHKLwT (ORCPT ); Thu, 11 Aug 2022 07:52:19 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTP id 83AB995E77 for ; Thu, 11 Aug 2022 04:52:18 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1660218737; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=BHGYpPVOToJJW83BwikwQkKAUWoo3Emiqyb5NZjYFwU=; b=WbJVkwFz5rhQ6HGDwzc8ToCsHVlooQrs43OI1xUOFzGkwvfKdthe2lS1MXsCoDrRzWp0xR FwQpRGjffA3IIw5WesU8/Zn2S0OK2HHo7FhZc1KyUxBlZrpTM1KO07K3TExUG15SRUa/bP 6sjFHL9T3gG7ihQQWeRtop7shfoh4e4= Received: from mail-ej1-f72.google.com (mail-ej1-f72.google.com [209.85.218.72]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-651-WovKcL9qM8O3erjNvztQ6w-1; Thu, 11 Aug 2022 07:52:14 -0400 X-MC-Unique: WovKcL9qM8O3erjNvztQ6w-1 Received: by mail-ej1-f72.google.com with SMTP id gb41-20020a170907962900b00730961131a7so5399701ejc.1 for ; Thu, 11 Aug 2022 04:52:13 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc; bh=BHGYpPVOToJJW83BwikwQkKAUWoo3Emiqyb5NZjYFwU=; b=XYSUezFrtSqSETzqLoGLqxXHbbObSTdmyDbxc0vAlMOyz8/hgUfN+/BxnmOrW6orJS Z6LpAi7B/a/j0vZViCaFR590HMCA/HshRG8qYFCtfdBC/4cgTnNKKminCI137v+lBsQ6 A6cINg74C5YKDSw9QKYYHwIhKlYdbgebdwE4zrkzkbTRiFyUwfSh2pNkqwiSp+bwVH1y KrGUrtTFoXp7bNPvdftYjt99fU38kk4RDwlfx5HnKed7v4SiP7Dim0zgadsc2uqcnrZp esV+feLVxLuB8R+roAFGw+OIlFHhD+Z7eUSHePWHgclwI8LzKSknBurjFu9KTbwZrvaR IEsg== X-Gm-Message-State: ACgBeo13YP9UQk/sfDbtXFAequqrNwFLwPgnIMylaq0eRIcDDm0nQEGv kTtBdB+11j0WMa87zVCuY9upodk9Gh2UfWkr89SXko9MRP1UJStjmki+Nm5I8kitotO2N7Jzx7L Hgc4iApRh3LcACCC40b4JOl5LvNZ0 X-Received: by 2002:a17:906:d7a9:b0:731:2189:4f58 with SMTP id pk9-20020a170906d7a900b0073121894f58mr18015573ejb.471.1660218732815; Thu, 11 Aug 2022 04:52:12 -0700 (PDT) X-Received: by 2002:a17:906:d7a9:b0:731:2189:4f58 with SMTP id pk9-20020a170906d7a900b0073121894f58mr18015557ejb.471.1660218732575; Thu, 11 Aug 2022 04:52:12 -0700 (PDT) Received: from ?IPV6:2001:1c00:c1e:bf00:d69d:5353:dba5:ee81? (2001-1c00-0c1e-bf00-d69d-5353-dba5-ee81.cable.dynamic.v6.ziggo.nl. [2001:1c00:c1e:bf00:d69d:5353:dba5:ee81]) by smtp.gmail.com with ESMTPSA id cf6-20020a0564020b8600b00442d5054a51sm1870300edb.87.2022.08.11.04.52.11 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 11 Aug 2022 04:52:11 -0700 (PDT) Message-ID: <065a9f83-e99f-1540-528a-83eb0203e206@redhat.com> Date: Thu, 11 Aug 2022 13:52:11 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.12.0 Subject: Re: [PATCH] Bluetooth: hci_event: Fix vendor (unknown) opcode status handling Content-Language: en-US To: Luiz Augusto von Dentz Cc: Marcel Holtmann , Johan Hedberg , "linux-bluetooth@vger.kernel.org" , Luiz Augusto von Dentz References: <20220807205740.777363-1-hdegoede@redhat.com> From: Hans de Goede In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-3.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A, RCVD_IN_DNSWL_LOW,SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: X-Mailing-List: linux-bluetooth@vger.kernel.org Hi Luiz, On 8/11/22 00:26, Luiz Augusto von Dentz wrote: > Hi Hans, > > On Mon, Aug 8, 2022 at 12:58 PM Luiz Augusto von Dentz > wrote: >> >> Hi Hans, >> >> On Sun, Aug 7, 2022 at 1:57 PM Hans de Goede wrote: >>> >>> Commit c8992cffbe74 ("Bluetooth: hci_event: Use of a function table to >>> handle Command Complete") was (presumably) meant to only refactor things >>> without any functional changes. >>> >>> But it does have one undesirable side-effect, before *status would always >>> be set to skb->data[0] and it might be overridden by some of the opcode >>> specific handling. While now it always set by the opcode specific handlers. >>> This means that if the opcode is not known *status does not get set any >>> more at all! >>> >>> This behavior change has broken bluetooth support for BCM4343A0 HCIs, >>> the hci_bcm.c code tries to configure UART attached HCIs at a higher >>> baudraute using vendor specific opcodes. The BCM4343A0 does not >>> support this and this used to simply fail: >>> >>> [ 25.646442] Bluetooth: hci0: BCM: failed to write clock (-56) >>> [ 25.646481] Bluetooth: hci0: Failed to set baudrate >>> >>> After which things would continue with the initial baudraute. But now >>> that hci_cmd_complete_evt() no longer sets status for unknown opcodes >>> *status is left at 0. This causes the hci_bcm.c code to think the baudraute >>> has been changed on the HCI side and to also adjust the UART baudrate, >>> after which communication with the HCI is broken, leading to: >>> >>> [ 28.579042] Bluetooth: hci0: command 0x0c03 tx timeout >>> [ 36.961601] Bluetooth: hci0: BCM: Reset failed (-110) >>> >>> And non working bluetooth. Fix this by restoring the previous >>> default "*status = skb->data[0]" handling for unknown opcodes. >>> >>> Fixes: c8992cffbe74 ("Bluetooth: hci_event: Use of a function table to handle Command Complete") >>> Cc: Luiz Augusto von Dentz >>> Signed-off-by: Hans de Goede >>> --- >>> net/bluetooth/hci_event.c | 7 +++++++ >>> 1 file changed, 7 insertions(+) >>> >>> diff --git a/net/bluetooth/hci_event.c b/net/bluetooth/hci_event.c >>> index af17dfb20e01..fda31d558ded 100644 >>> --- a/net/bluetooth/hci_event.c >>> +++ b/net/bluetooth/hci_event.c >>> @@ -3996,6 +3996,13 @@ static void hci_cmd_complete_evt(struct hci_dev *hdev, void *data, >>> break; >>> } >>> } >>> + if (i == ARRAY_SIZE(hci_cc_table)) { >>> + /* Unknown opcode, assume byte 0 contains the status, so >>> + * that e.g. __hci_cmd_sync() properly returns errors >>> + * for vendor specific commands send by HCI drivers. >>> + */ >>> + *status = skb->data[0]; >>> + } >> >> The format of return parameters in command is not defined by the spec: >> >> BLUETOOTH CORE SPECIFICATION Version 5.3 | Vol 4, Part E >> page 2189: >> >> Return_Parameters: >> Size: Depends on command >> >> This is the return parameter(s) for the command specified in the >> Command_Opcode event parameter. See each command’s definition for >> the list of return parameters associated with that command. >> >> So assuming the status is the first by is not quite right, although >> for the standard ones that seems to be valid, I think the best way to >> resolve this would have been to check if it a vendor command and then >> have the driver handle it or perhaps have some means for the driver to >> register it vendor_cc_table, we can perhaps have this as a workaround >> for now and only really change how we parse the cc for vendor commands >> if a vendor decide not to have a status as first parameter but Id >> probably leave a comment that quoting the spec that reminds us this >> code may need changing. > > Are you still planning to send updates for this, I consider this quite > urgent given that it can break support with some vendors. Right, sorry for being a bit slow. I will prepare + email a version 2 adding a comment that byte 0 being the status is not guaranteed with vendor commands right away. Regards, Hans > >>> handle_cmd_cnt_and_timer(hdev, ev->ncmd); >>> >>> -- >>> 2.37.1 >>> >> >> >> -- >> Luiz Augusto von Dentz > > >