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;
MIME-Version: 1.0
References: <20220315095129.15254-1-tangmeng@uniontech.com>
In-Reply-To: <20220315095129.15254-1-tangmeng@uniontech.com>
From:   Martin Blumenstingl <martin.blumenstingl@googlemail.com>
Date:   Tue, 15 Mar 2022 19:49:59 +0100
Message-ID: <CAFBinCBQzs6zY1p8E3gn9jhm8EhiH5YpZpzyBc0C5c6ziOpgFA@mail.gmail.com>
Subject: Re: [PATCH v3 1/2] Bluetooth: btrtl: btmrvl: Fix firmware filename
 for rtl8723bs chipset
To:     Meng Tang <tangmeng@uniontech.com>
Cc:     marcel@holtmann.org, johan.hedberg@gmail.com, luiz.dentz@gmail.com,
        linux-bluetooth@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

Hello,

On Tue, Mar 15, 2022 at 10:51 AM Meng Tang <tangmeng@uniontech.com> wrote:
>
> Firmware for rtl8723bs chipset is as part of the linux-firmware
> repository in filename rtl8723bs_config-OBDA8723.bin.
>
> This patch fixes mwifiex driver to load correct firmware file for
> rtl8723bs.
This raises some serious questions for me:
- the subject states that this patch is for the btmrvl driver
- mwifiex is for Marvell based chips which to my knowledge are not
related to any Realtek chips, so mwifiex should never try to load
Realtek firmware files

[...]
> @@ -88,7 +88,7 @@ static const struct id_table ic_id_table[] = {
>           .config_needed = true,
>           .has_rom_version = true,
>           .fw_name  = "rtl_bt/rtl8723bs_fw.bin",
> -         .cfg_name = "rtl_bt/rtl8723bs_config" },
> +         .cfg_name = "rtl_bt/rtl8723bs_config-OBDA8723" },
This change is incorrect, see btrtl_initialize():
  if (btrtl_dev->ic_info->cfg_name) {
    if (postfix) {
       snprintf(cfg_name, sizeof(cfg_name), "%s-%s.bin",
btrtl_dev->ic_info->cfg_name, postfix);
     } else {
       snprintf(cfg_name, sizeof(cfg_name), "%s.bin",
btrtl_dev->ic_info->cfg_name);

This suffix (postfix) can be either OBDA8723, OBDA0623 or just be
absent - see hci_h5.c (h5_acpi_match, h5_serdev_probe() which sets
h5->id and h5_btrtl_setup() which uses h5->id).

[...]
> -MODULE_FIRMWARE("rtl_bt/rtl8723bs_config.bin");
> +MODULE_FIRMWARE("rtl_bt/rtl8723bs_config-OBDA8723.bin");
According to the driver code "rtl_bt/rtl8723bs_config.bin" is a valid
value so I think it should not be removed.
Adding "rtl_bt/rtl8723bs_config-OBDA8723.bin" is fine for me - if
that's done then "rtl_bt/rtl8723bs_config-OBDA0623.bin" should be
added as well.


Best regards,
Martin