| Message ID | 20211226153624.162281-1-marcan@marcan.st |
|---|---|
| Headers | show |
| Series | brcmfmac: Support Apple T2 and M1 platforms | expand |
On Mon, Dec 27, 2021 at 12:36:05AM +0900, Hector Martin wrote: > The Wi-Fi module in Apple machines has a "module-instance" device > property that specifies the platform type and is used for firmware > selection. Its value is a string, so add support for string values in > acpi_extract_apple_properties(). > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Lukas Wunner <lukas@wunner.de>
On Mon, Dec 27, 2021 at 12:35:50AM +0900, Hector Martin wrote: > # On firmware > > As you might expect, the firmware for these machines is not available > under a redistributable license; however, every owner of one of these > machines *is* implicitly licensed to posess the firmware, and the OS > packages containing it are available under well-known URLs on Apple's > CDN with no authentication. Apple's EFI firmware contains a full-fledged network stack for downloading macOS images from osrecovery.apple.com. I suspect that it also contains wifi firmware. You may want to check if it's passed to the OS as an EFI property. Using that would sidestep license issues. There's EDID data, Thunderbolt DROM data and whatnot in those properties, so I wouldn't be surprised if it contained wifi stuff as well. Enable CONFIG_APPLE_PROPERTIES and pass "dump_apple_properties" on the command line to see all EFI properties in dmesg. Alternatively, check "ioreg -l" on macOS. Generally, what's available in the I/O registry should also be available on Linux either as an ACPI or EFI property. Thanks, Lukas
On Sun, Dec 26, 2021 at 4:36 PM Hector Martin <marcan@marcan.st> wrote: > Move one of the declarations from sdio.c to pcie.c, since it makes no > sense in the former (SDIO support is optional), and add missing ones. > > Signed-off-by: Hector Martin <marcan@marcan.st> > Fixes: 75729e110e68 ("brcmfmac: expose firmware config files through modinfo") Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
Hi, On 12/26/21 20:17, Lukas Wunner wrote: > On Mon, Dec 27, 2021 at 12:35:50AM +0900, Hector Martin wrote: >> # On firmware >> >> As you might expect, the firmware for these machines is not available >> under a redistributable license; however, every owner of one of these >> machines *is* implicitly licensed to posess the firmware, and the OS >> packages containing it are available under well-known URLs on Apple's >> CDN with no authentication. > > Apple's EFI firmware contains a full-fledged network stack for > downloading macOS images from osrecovery.apple.com. I suspect > that it also contains wifi firmware. > > You may want to check if it's passed to the OS as an EFI property. > Using that would sidestep license issues. There's EDID data, > Thunderbolt DROM data and whatnot in those properties, so I > wouldn't be surprised if it contained wifi stuff as well. > > Enable CONFIG_APPLE_PROPERTIES and pass "dump_apple_properties" > on the command line to see all EFI properties in dmesg. > Alternatively, check "ioreg -l" on macOS. Generally, what's > available in the I/O registry should also be available on Linux > either as an ACPI or EFI property. Interesting, note that even if the files are not available as a property we also have CONFIG_EFI_EMBEDDED_FIRMWARE, see: drivers/firmware/efi/embedded-firmware.c Documentation/driver-api/firmware/fallback-mechanisms.rst I wrote this to pry/dig out some touchscreen firmwares (where we have been unable to get permission to redistribute) out of EFI boot_services_code mem regions on tablets where the touchsceen is supported under the EFI environment. This may need some tweaks, but if there is an embedded copy of the firmware files in the EFI mem regions somewhere it should be possible to adjust this code to grab it and present it to the firmware-loader mechanism as a fallback option. Refards, Hans
On 2021/12/27 6:42, Hans de Goede wrote: > Hi, > > On 12/26/21 20:17, Lukas Wunner wrote: >> On Mon, Dec 27, 2021 at 12:35:50AM +0900, Hector Martin wrote: >>> # On firmware >>> >>> As you might expect, the firmware for these machines is not available >>> under a redistributable license; however, every owner of one of these >>> machines *is* implicitly licensed to posess the firmware, and the OS >>> packages containing it are available under well-known URLs on Apple's >>> CDN with no authentication. >> >> Apple's EFI firmware contains a full-fledged network stack for >> downloading macOS images from osrecovery.apple.com. I suspect >> that it also contains wifi firmware. >> >> You may want to check if it's passed to the OS as an EFI property. >> Using that would sidestep license issues. There's EDID data, >> Thunderbolt DROM data and whatnot in those properties, so I >> wouldn't be surprised if it contained wifi stuff as well. >> >> Enable CONFIG_APPLE_PROPERTIES and pass "dump_apple_properties" >> on the command line to see all EFI properties in dmesg. >> Alternatively, check "ioreg -l" on macOS. Generally, what's >> available in the I/O registry should also be available on Linux >> either as an ACPI or EFI property. > > Interesting, note that even if the files are not available as > a property we also have CONFIG_EFI_EMBEDDED_FIRMWARE, see: > > drivers/firmware/efi/embedded-firmware.c > Documentation/driver-api/firmware/fallback-mechanisms.rst > > I wrote this to pry/dig out some touchscreen firmwares (where > we have been unable to get permission to redistribute) out of > EFI boot_services_code mem regions on tablets where > the touchsceen is supported under the EFI environment. > > This may need some tweaks, but if there is an embedded copy > of the firmware files in the EFI mem regions somewhere it > should be possible to adjust this code to grab it and present > it to the firmware-loader mechanism as a fallback option. Note that this wouldn't work on M1 Macs anyway, since those don't have EFI (we provide EFI via U-Boot as a chained bootloader on those), and their bootloader doesn't support any networking (it doesn't even do USB or any kind of UI). Quick recap for those not familiar with the M1 boot process: the bootloader is iBoot, which is extremely simple (at least compared to EFI). All it can do is boot kernels from APFS volumes on internal NVMe. The boot selection menu and recovery options are implemented as macOS apps running from a recovery image (~1GB), and "USB boot" is implemented by copying the macOS equivalent of /boot to NVMe. There is a global recovery image as well as per-OS recovery image. The WiFi firmware is present in this image as well as on normal macOS root volumes. Our Linux install script is actually mostly a macOS install script that sets up all the boot components that macOS would normally have, including the recovery image, minus the main root filesystem. This is all required to work properly within Apple's security and multi-boot framework. So, since we're installing the recovery image, we're already in an easy position to pull the firmware out and stick it in the EFI partition for Linux to easily use. The alternative would be for Linux userspace to read it from APFS directly, but that seems unlikely to be practical until linux-apfs is upstreamed. For T2 Macs I'm sure the firmware will be in EFI somewhere, but even if we can get it from there (I wouldn't be surprised if it's e.g. still compressed in the normal boot path that doesn't start network services), I'm not sure it's worth implementing yet another mechanism for those machines. Once we have the vendor-firmware mechanism implemented for M1, it's easy to just run the same script on T2s and get the proper firmware from macOS (which might even be different from the EFI firmware...). macOS definitely doesn't read the firmware from EFI on those machines, so a hack to do it by scanning the code would probably not be something we can rely on to continue working across firmware updates (and they do update WiFi firmware; it's a rather well known source of security issues... so then we'd have to play the update-the-sha256 cat and mouse game). I'm pretty sure there's no property containing the big firmware blob passed explicitly to the OS; it has its own copy.
On Sun, Dec 26, 2021 at 4:37 PM Hector Martin <marcan@marcan.st> wrote: > Apple platforms have firmware and config files identified with multiple > dimensions. We want to be able to find the most specific firmware > available for any given platform, progressively trying more general > firmwares. > > First, add support for having multiple alternate firmware paths. > > Signed-off-by: Hector Martin <marcan@marcan.st> This looks OK to me so FWIW: Acked-by: Linus Walleij <linus.walleij@linaro.org> Make sure Dmitry Osipenko gets to review this though, he has many valuable insights about how the FW is loaded and helped me out a lot when I patched this. Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:37 PM Hector Martin <marcan@marcan.st> wrote: > In order to make use of the multiple alt_path functionality, change > board_type to an array. Bus drivers can pass in a NULL-terminated list > of board type strings to try for the firmware fetch. > > Signed-off-by: Hector Martin <marcan@marcan.st> Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:37 PM Hector Martin <marcan@marcan.st> wrote: > On Apple platforms, the One Time Programmable ROM in the Broadcom chips > contains information about the specific board design (module, vendor, > version) that is required to select the correct NVRAM file. Parse this > OTP ROM and extract the required strings. > > Note that the user OTP offset/size is per-chip. This patch does not add > any chips yet. > > Signed-off-by: Hector Martin <marcan@marcan.st> Overall looks fine! > + const char *chip_params; > + const char *module_params; This variable name "module_params" is a bit confusing since loadable kernel modules have params... Can we think of another name and just put a comment that this refers to the WiFi module building block? Sometimes people talk about SoM:s (system-on-modules), so maybe som_params or brcm_som_params? Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:37 PM Hector Martin <marcan@marcan.st> wrote: > On Apple ARM64 platforms, firmware selection requires two properties > that come from system firmware: the module-instance (aka "island", a > codename representing a given hardware platform) and the antenna-sku. > > The module-instance is hard-coded in per-board DTS files, while the > antenna-sku is forwarded by our bootloader from the Apple Device Tree > into the FDT. Grab them from the DT so firmware selection can use > them. > > The module-instance is used to construct a board_type by prepending it > with "apple,". > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:37 PM Hector Martin <marcan@marcan.st> wrote: > On Apple platforms, firmware selection uses the following elements: > > Property Example Source > ============== ======= ======================== > * Chip name 4378 Device ID > * Chip revision B1 OTP > * Platform shikoku DT (ARM64) or ACPI (x86) > * Module type RASP OTP > * Module vendor m OTP > * Module version 6.11 OTP > * Antenna SKU X3 DT (ARM64) or ??? (x86) > > In macOS, these firmwares are stored using filenames in this format > under /usr/share/firmware/wifi: > > C-4378__s-B1/P-shikoku-X3_M-RASP_V-m__m-6.11.txt > > To prepare firmwares for Linux, we rename these to a scheme following > the existing brcmfmac convention: > > brcmfmac<chip><lower(rev)>-pcie.apple,<platform>-<mod_type>-\ > <mod_vendor>-<mod_version>-<antenna_sku>.txt > > The NVRAM uses all the components, while the firmware and CLM blob only > use the chip/revision/platform/antenna_sku: > > brcmfmac<chip><lower(rev)>-pcie.apple,<platform>-<antenna_sku>.bin > > e.g. > > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-m-6.11-X3.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-X3.bin > > In addition, since there are over 1000 files in total, many of which are > symlinks or outright duplicates, we deduplicate and prune the firmware > tree to reduce firmware filenames to fewer dimensions. For example, the > shikoku platform (MacBook Air M1 2020) simplifies to just 4 files: > > brcm/brcmfmac4378b1-pcie.apple,shikoku.clm_blob > brcm/brcmfmac4378b1-pcie.apple,shikoku.bin > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-m.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-u.txt > > This reduces the total file count to around 170, of which 75 are > symlinks and 95 are regular files: 7 firmware blobs, 27 CLM blobs, and > 61 NVRAM config files. We also slightly process NVRAM files to correct > some formatting issues and add a missing default macaddr= property. > > To handle this, the driver must try the following path formats when > looking for firmware files: > > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-m-6.11-X3.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-m-6.11.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP-m.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-RASP.txt > brcm/brcmfmac4378b1-pcie.apple,shikoku-X3.txt * > brcm/brcmfmac4378b1-pcie.apple,shikoku.txt > > * Not relevant for NVRAM, only for firmware/CLM. > > The chip revision nominally comes from OTP on Apple platforms, but it > can be mapped to the PCI revision number, so we ignore the OTP revision > and continue to use the existing PCI revision mechanism to identify chip > revisions, as the driver already does for other chips. Unfortunately, > the mapping is not consistent between different chip types, so this has > to be determined experimentally. > > Signed-off-by: Hector Martin <marcan@marcan.st> It's a neat hack, and I don't see anyone doing anything smarter so: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:37 PM Hector Martin <marcan@marcan.st> wrote: > On Device Tree platforms, it is customary to be able to set the MAC > address via the Device Tree, as it is often stored in system firmware. > This is particularly relevant for Apple ARM64 platforms, where this > information comes from system configuration and passed through by the > bootloader into the DT. > > Implement support for this by fetching the platform MAC address and > adding or replacing the macaddr= property in nvram. This becomes the > dongle's default MAC address. > > On platforms with an SROM MAC address, this overrides it. On platforms > without one, such as Apple ARM64 devices, this is required for the > firmware to boot (it will fail if it does not have a valid MAC at all). > > Signed-off-by: Hector Martin <marcan@marcan.st> This looks very helpful. > + /* Add space for properties we may add */ > + size += strlen(BRCMF_FW_DEFAULT_BOARDREV) + 1; > + size += BRCMF_FW_MACADDR_LEN + 1; Add some note to the commit log why you also make space for boardrev? (Looks useful.) Is the boardrev spacing in the right patch? With that addressed: Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:38 PM Hector Martin <marcan@marcan.st> wrote: > Newer chips used on Apple platforms have more than max_rxbufpost greater > than 512, which causes warnings when brcmf_msgbuf_rxbuf_data_fill tries > to put more in the ring than fit. Increase the ring sizes to 1024. > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:38 PM Hector Martin <marcan@marcan.st> wrote: > The driver was enabling IRQs before the message processing was > initialized. This could cause IRQs to come in too early and crash the > driver. Instead, move the IRQ enable and hostready to a bus preinit > function, at which point everything is properly initialized. > > Fixes: 9e37f045d5e7 ("brcmfmac: Adding PCIe bus layer support.") > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:38 PM Hector Martin <marcan@marcan.st> wrote: > These newer PCIe core revisions include new sets of registers that must > be used instead of the legacy ones. Introduce a brcmf_pcie_reginfo to > hold the specific register offsets and values to use for a given > platform, and change all the register accesses to indirect through it. > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:38 PM Hector Martin <marcan@marcan.st> wrote: > This chip is present on Apple M1 (t8103) platforms: > > * atlantisb (apple,j274): Mac mini (M1, 2020) > * honshu (apple,j293): MacBook Pro (13-inch, M1, 2020) > * shikoku (apple,j313): MacBook Air (M1, 2020) > * capri (apple,j456): iMac (24-inch, 4x USB-C, M1, 2020) > * santorini (apple,j457): iMac (24-inch, 2x USB-C, M1, 2020) > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:38 PM Hector Martin <marcan@marcan.st> wrote: > On DT platforms, the module-instance and antenna-sku-info properties > are passed in the DT. On ACPI platforms, module-instance is passed via > the analogous Apple device property mechanism, while the antenna SKU > info is instead obtained via an ACPI method that grabs it from > non-volatile storage. > > Add support for this, to allow proper firmware selection on Apple > platforms. > > Signed-off-by: Hector Martin <marcan@marcan.st> If the strings treated here are exactly the same as for the device tree, you should be able to just use "devprops" (firmware node) to handle it abstractly, and then the respective DT and ACPI backend will provide the properties. I don't know if this patch I made recently is enough of an examples: https://lore.kernel.org/linux-hwmon/20211206020423.62402-2-linus.walleij@linaro.org/ If the ACPI and DT differs a lot in format and strings etc it may not be worth it. Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:38 PM Hector Martin <marcan@marcan.st> wrote: > Newer Apple firmwares on chipsets without a hardware RNG require the > host to provide a buffer of 256 random bytes to the device on > initialization. This buffer is present immediately before NVRAM, > suffixed by a footer containing a magic number and the buffer length. > > This won't affect chips/firmwares that do not use this feature, so do it > unconditionally. > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:39 PM Hector Martin <marcan@marcan.st> wrote: > This chip is present on at least these Apple T2 Macs: > > * hawaii: MacBook Air 13" (Late 2018) > * hawaii: MacBook Air 13" (True Tone, 2019) > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:39 PM Hector Martin <marcan@marcan.st> wrote: > This chip is present on at least these Apple T2 Macs: > > * tahiti: MacBook Pro 13" (2020, 2 TB3) > * formosa: MacBook Pro 13" (Touch/2019) > * fiji: MacBook Air 13" (Scissor, 2020) > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:39 PM Hector Martin <marcan@marcan.st> wrote: > This chip exists in two revisions (B2=r3 and B3=r4) on different > platforms, and was added without regard to doing proper firmware > selection or differentiating between them. Fix this to have proper > per-revision firmwares and support Apple NVRAM selection. > > Revision B2 is present on at least these Apple T2 Macs: > > kauai: MacBook Pro 15" (Touch/2018-2019) > maui: MacBook Pro 13" (Touch/2018-2019) > lanai: Mac mini (Late 2018) > ekans: iMac Pro 27" (5K, Late 2017) > > And these non-T2 Macs: > > nihau: iMac 27" (5K, 2019) > > Revision B3 is present on at least these Apple T2 Macs: > > bali: MacBook Pro 16" (2019) > trinidad: MacBook Pro 13" (2020, 4 TB3) > borneo: MacBook Pro 16" (2019, 5600M) > kahana: Mac Pro (2019) > kahana: Mac Pro (2019, Rack) > hanauma: iMac 27" (5K, 2020) > kure: iMac 27" (5K, 2020, 5700/XT) > > Fixes: 24f0bd136264 ("brcmfmac: add the BRCM 4364 found in MacBook Pro 15,2") > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:39 PM Hector Martin <marcan@marcan.st> wrote: > At least on BCM4387, the D11 cores are held in reset on cold startup and > firmware expects to release reset itself. Just assert reset here and let > firmware deassert it. Premature deassertion results in the firmware > failing to initialize properly some of the time, with strange AXI bus > errors. > > Also, BCM4387 has 3 cores, up from 2. The logic for handling that is in > brcmf_chip_ai_resetcore(), but since we aren't using that any more, just > handle it here. > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:39 PM Hector Martin <marcan@marcan.st> wrote: > BCM4387 has trailing odd-sized blocks as part of TCM which have > their size described as a multiple of 1024 instead of 8192. Handle this > so we can compute the TCM size properly. > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:39 PM Hector Martin <marcan@marcan.st> wrote: > The "wlc_ver" iovar returns information on the WLC and EPI versions. > This can be used to determine whether the PMKID_V2 and _V3 features are > supported. > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:39 PM Hector Martin <marcan@marcan.st> wrote: > Add support for the new PMKID_V3 API, which allows performing PMKID > mutations individually, instead of requiring the driver to keep track of > the full list. This new API is required by at least BCM4387. > > Note that PMKID_V2 is not implemented yet. > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:40 PM Hector Martin <marcan@marcan.st> wrote: > Apparently the hex passphrase mechanism does not work on newer > chips/firmware (e.g. BCM4387). It seems there was a simple way of > passing it in binary all along, so use that and avoid the hexification. > > OpenBSD has been doing it like this from the beginning, so this should > work on all chips. > > Also clear the structure before setting the PMK. This was leaking > uninitialized stack contents to the device. > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:40 PM Hector Martin <marcan@marcan.st> wrote: > This chip is present on Apple M1 Pro/Max (t600x) platforms: > > * maldives (apple,j314s): MacBook Pro (14-inch, M1 Pro, 2021) > * maldives (apple,j314c): MacBook Pro (14-inch, M1 Max, 2021) > * madagascar (apple,j316s): MacBook Pro (16-inch, M1 Pro, 2021) > * madagascar (apple,j316c): MacBook Pro (16-inch, M1 Max, 2021) > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:40 PM Hector Martin <marcan@marcan.st> wrote: > The alignment check was wrong (e.g. & 4 instead of & 3), and the logic > was also inefficient if the length was not a multiple of 4, since it > would needlessly fall back to copying the entire buffer bytewise. > > We already have a perfectly good memcpy_toio function, so just call that > instead of rolling our own copy logic here. brcmf_pcie_init_ringbuffers > was already using it anyway. > > Fixes: 9e37f045d5e7 ("brcmfmac: Adding PCIe bus layer support.") > Signed-off-by: Hector Martin <marcan@marcan.st> Excellent patch. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:40 PM Hector Martin <marcan@marcan.st> wrote: > This allows us to get console messages if the firmware crashed during > early init, or if an operation failed and we're about to shut down. > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:40 PM Hector Martin <marcan@marcan.st> wrote: > This avoids leaking memory if brcmf_chip_get_raminfo fails. Note that > the CLM blob is released in the device remove path. > > Fixes: 82f93cf46d60 ("brcmfmac: get chip's default RAM info during PCIe setup") > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:40 PM Hector Martin <marcan@marcan.st> wrote: > Make all the iovar name arguments const char * instead of just char *. > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:40 PM Hector Martin <marcan@marcan.st> wrote: > The TxCap blobs are additional data blobs used on Apple devices, and > are uploaded analogously to CLM blobs. Add core support for doing this. > > Signed-off-by: Hector Martin <marcan@marcan.st> Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:41 PM Hector Martin <marcan@marcan.st> wrote: > These blobs are named .txcap_blob, and exist alongside the existing > .clm_blob files. Use the existing firmware machinery to provide them to > the core. > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:41 PM Hector Martin <marcan@marcan.st> wrote: > The calibration blob for a chip is normally stored in SROM and loaded > internally by the firmware. However, Apple ARM64 platforms instead store > it as part of platform configuration data, and provide it via the Apple > Device Tree. We forward this into the Linux DT in the bootloader. > > Add support for taking this blob from the DT and loading it into the > dongle. The loading mechanism is the same as used for the CLM and TxCap > blobs. > > Signed-off-by: Hector Martin <marcan@marcan.st> Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:38 PM Hector Martin <marcan@marcan.st> wrote: > The Wi-Fi module in Apple machines has a "module-instance" device > property that specifies the platform type and is used for firmware > selection. Its value is a string, so add support for string values in > acpi_extract_apple_properties(). > > Signed-off-by: Hector Martin <marcan@marcan.st> Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:37 PM Hector Martin <marcan@marcan.st> wrote: > Teach brcm_alt_fw_paths to correctly split off variable length > extensions, and enable alt firmware lookups for the CLM blob firmware > requests. > > Apple platforms have per-board CLM blob files. > > Signed-off-by: Hector Martin <marcan@marcan.st> Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:37 PM Hector Martin <marcan@marcan.st> wrote: > Now that the firmware fetcher can handle per-board CLM files, load the > CLM blob alongside the other firmware files and change the bus API to > just return the existing blob, instead of fetching the filename. > > This enables per-board CLM blobs, which are required on Apple platforms. > > Signed-off-by: Hector Martin <marcan@marcan.st> Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:39 PM Hector Martin <marcan@marcan.st> wrote: > This new API version is required for at least the BCM4387 firmware. Add > support for it, with a fallback to the v1 API. > > Signed-off-by: Hector Martin <marcan@marcan.st> Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On Sun, Dec 26, 2021 at 4:36 PM Hector Martin <marcan@marcan.st> wrote: > Merry Christmas! This year Santa brings us a 34-patch series to add > proper support for the Broadcom FullMAC chips used on Apple T2 and M1 > platforms: I tried to review as best I could, when I think I know what I'm doing I state Reviewed-by and when I think it just LooksGoodToMe(TM) I replied Acked-by. If I missed some patch you can assume Acked-by from me on these as well. Thanks for doing this, some really old bugs and code improvements long overdue is in the series, much appreciated. Yours, Linus Walleij
26.12.2021 18:35, Hector Martin пишет: > Apple platforms have firmware and config files identified with multiple > dimensions. We want to be able to find the most specific firmware > available for any given platform, progressively trying more general > firmwares. > > First, add support for having multiple alternate firmware paths. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/firmware.c | 73 ++++++++++++++----- > 1 file changed, 55 insertions(+), 18 deletions(-) ... > -static char *brcm_alt_fw_path(const char *path, const char *board_type) > +static const char **brcm_alt_fw_paths(const char *path, const char *board_type) ... > static int brcmf_fw_request_firmware(const struct firmware **fw, > struct brcmf_fw *fwctx) > { > struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; > - int ret; > + int ret, i; > > /* Files can be board-specific, first try a board-specific path */ > if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) { > - char *alt_path; > + const char **alt_paths = brcm_alt_fw_paths(cur->path, fwctx); The brcm_alt_fw_paths() takes "board_type" argument, while you're passing the "fwctx" to it. This patch doesn't compile. If this code is changed by a further patch, then please use "git rebase --exec" to compile-test all the patches. drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c: In function ‘brcmf_fw_request_firmware’: drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c:642:71: error: passing argument 2 of ‘brcm_alt_fw_paths’ from incompatible pointer type [-Werror=incompatible-pointer-types] 642 | const char **alt_paths = brcm_alt_fw_paths(cur->path, fwctx); | ^~~~~ | | | struct brcmf_fw * drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c:597:69: note: expected ‘const char *’ but argument is of type ‘struct brcmf_fw *’ 597 | static const char **brcm_alt_fw_paths(const char *path, const char *board_type) | ~~~~~~~~~~~~^~~~~~~~~~ drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c: In function ‘brcmf_fw_get_firmwares’: drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c:752:59: error: passing argument 2 of ‘brcm_alt_fw_paths’ from incompatible pointer type [-Werror=incompatible-pointer-types] 752 | fwctx->alt_paths = brcm_alt_fw_paths(first->path, fwctx); | ^~~~~ | | | struct brcmf_fw * drivers/net/wireless/broadcom/brcm80211/brcmfmac/firmware.c:597:69: note: expected ‘const char *’ but argument is of type ‘struct brcmf_fw *’ 597 | static const char **brcm_alt_fw_paths(const char *path, const char *board_type) | ~~~~~~~~~~~~^~~~~~~~~~ cc1: some warnings being treated as errors
26.12.2021 18:35, Hector Martin пишет: > -static char *brcm_alt_fw_path(const char *path, const char *board_type) > +static const char **brcm_alt_fw_paths(const char *path, const char *board_type) > { > char alt_path[BRCMF_FW_NAME_LEN]; > + char **alt_paths; > char suffix[5]; > > strscpy(alt_path, path, BRCMF_FW_NAME_LEN); > @@ -609,27 +612,46 @@ static char *brcm_alt_fw_path(const char *path, const char *board_type) > strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); > strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); > > - return kstrdup(alt_path, GFP_KERNEL); > + alt_paths = kzalloc(sizeof(char *) * 2, GFP_KERNEL); array_size()? > + alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); > + > + return (const char **)alt_paths; Why this casting is needed? > +}
26.12.2021 18:35, Hector Martin пишет: > struct brcmf_fw { > struct device *dev; > struct brcmf_fw_request *req; > + const char **alt_paths; > + int alt_index; ... > +static void brcm_free_alt_fw_paths(const char **alt_paths) > +{ > + int i; ... > static int brcmf_fw_request_firmware(const struct firmware **fw, > struct brcmf_fw *fwctx) > { > struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; > - int ret; > + int ret, i; unsigned int
26.12.2021 18:35, Hector Martin пишет: > +static void brcm_free_alt_fw_paths(const char **alt_paths) > +{ > + int i; > + > + if (!alt_paths) > + return; > + > + for (i = 0; alt_paths[i]; i++) > + kfree(alt_paths[i]); > + > + kfree(alt_paths); > } > > static int brcmf_fw_request_firmware(const struct firmware **fw, > struct brcmf_fw *fwctx) > { > struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; > - int ret; > + int ret, i; > > /* Files can be board-specific, first try a board-specific path */ > if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) { > - char *alt_path; > + const char **alt_paths = brcm_alt_fw_paths(cur->path, fwctx); > > - alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type); > - if (!alt_path) > + if (!alt_paths) > goto fallback; > > - ret = request_firmware(fw, alt_path, fwctx->dev); > - kfree(alt_path); > - if (ret == 0) > - return ret; > + for (i = 0; alt_paths[i]; i++) { > + ret = firmware_request_nowarn(fw, alt_paths[i], fwctx->dev); > + if (ret == 0) { > + brcm_free_alt_fw_paths(alt_paths); > + return ret; > + } > + } > + brcm_free_alt_fw_paths(alt_paths); > } > > fallback: > @@ -641,6 +663,9 @@ static void brcmf_fw_request_done(const struct firmware *fw, void *ctx) > struct brcmf_fw *fwctx = ctx; > int ret; > > + brcm_free_alt_fw_paths(fwctx->alt_paths); > + fwctx->alt_paths = NULL; It looks suspicious that fwctx->alt_paths isn't zero'ed by other code paths. The brcm_free_alt_fw_paths() should take fwctx for the argument and fwctx->alt_paths should be set to NULL there. On the other hand, I'd change the **alt_paths to a fixed-size array. This should simplify the code, making it easier to follow and maintain. - const char **alt_paths; + char *alt_paths[BRCM_MAX_ALT_FW_PATHS]; Then you also won't need to NULL-terminate the array, which is a common source of bugs in kernel.
02.01.2022 08:31, Linus Walleij пишет: > On Sun, Dec 26, 2021 at 4:37 PM Hector Martin <marcan@marcan.st> wrote: > >> Apple platforms have firmware and config files identified with multiple >> dimensions. We want to be able to find the most specific firmware >> available for any given platform, progressively trying more general >> firmwares. >> >> First, add support for having multiple alternate firmware paths. >> >> Signed-off-by: Hector Martin <marcan@marcan.st> > > This looks OK to me so FWIW: > Acked-by: Linus Walleij <linus.walleij@linaro.org> > > Make sure Dmitry Osipenko gets to review this though, he has many > valuable insights about how the FW is loaded and helped me out a > lot when I patched this. Thanks, Linus. I took a brief look at the patch and may give it a test next time, once it will compile without errors and warnings.
02.01.2022 10:08, Dmitry Osipenko пишет: > 26.12.2021 18:35, Hector Martin пишет: >> +static void brcm_free_alt_fw_paths(const char **alt_paths) >> +{ >> + int i; >> + >> + if (!alt_paths) >> + return; >> + >> + for (i = 0; alt_paths[i]; i++) >> + kfree(alt_paths[i]); >> + >> + kfree(alt_paths); >> } >> >> static int brcmf_fw_request_firmware(const struct firmware **fw, >> struct brcmf_fw *fwctx) >> { >> struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; >> - int ret; >> + int ret, i; >> >> /* Files can be board-specific, first try a board-specific path */ >> if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) { >> - char *alt_path; >> + const char **alt_paths = brcm_alt_fw_paths(cur->path, fwctx); >> >> - alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type); >> - if (!alt_path) >> + if (!alt_paths) >> goto fallback; >> >> - ret = request_firmware(fw, alt_path, fwctx->dev); >> - kfree(alt_path); >> - if (ret == 0) >> - return ret; >> + for (i = 0; alt_paths[i]; i++) { >> + ret = firmware_request_nowarn(fw, alt_paths[i], fwctx->dev); >> + if (ret == 0) { >> + brcm_free_alt_fw_paths(alt_paths); >> + return ret; >> + } >> + } >> + brcm_free_alt_fw_paths(alt_paths); >> } >> >> fallback: >> @@ -641,6 +663,9 @@ static void brcmf_fw_request_done(const struct firmware *fw, void *ctx) >> struct brcmf_fw *fwctx = ctx; >> int ret; >> >> + brcm_free_alt_fw_paths(fwctx->alt_paths); >> + fwctx->alt_paths = NULL; > > It looks suspicious that fwctx->alt_paths isn't zero'ed by other code > paths. The brcm_free_alt_fw_paths() should take fwctx for the argument > and fwctx->alt_paths should be set to NULL there. > > On the other hand, I'd change the **alt_paths to a fixed-size array. > This should simplify the code, making it easier to follow and maintain. > > - const char **alt_paths; > + char *alt_paths[BRCM_MAX_ALT_FW_PATHS]; Although, the const should be kept. const char *alt_paths[BRCM_MAX_ALT_FW_PATHS]; > > Then you also won't need to NULL-terminate the array, which is a common > source of bugs in kernel. >
On 2022/01/02 15:45, Dmitry Osipenko wrote: > 26.12.2021 18:35, Hector Martin пишет: >> -static char *brcm_alt_fw_path(const char *path, const char *board_type) >> +static const char **brcm_alt_fw_paths(const char *path, const char *board_type) >> { >> char alt_path[BRCMF_FW_NAME_LEN]; >> + char **alt_paths; >> char suffix[5]; >> >> strscpy(alt_path, path, BRCMF_FW_NAME_LEN); >> @@ -609,27 +612,46 @@ static char *brcm_alt_fw_path(const char *path, const char *board_type) >> strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); >> strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >> >> - return kstrdup(alt_path, GFP_KERNEL); >> + alt_paths = kzalloc(sizeof(char *) * 2, GFP_KERNEL); > > array_size()? Of what array? > >> + alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); >> + >> + return (const char **)alt_paths; > > Why this casting is needed? Because implicit conversion from char ** to const char ** is not legal in C, as that could cause const unsoundness if you do this: char *foo[1]; const char **bar = foo; bar[0] = "constant string"; foo[0][0] = '!'; // clobbers constant string But it's fine in this case since the non-const pointer disappears so nothing can ever write through it again.
On 2022/01/02 16:08, Dmitry Osipenko wrote: > 26.12.2021 18:35, Hector Martin пишет: >> +static void brcm_free_alt_fw_paths(const char **alt_paths) >> +{ >> + int i; >> + >> + if (!alt_paths) >> + return; >> + >> + for (i = 0; alt_paths[i]; i++) >> + kfree(alt_paths[i]); >> + >> + kfree(alt_paths); >> } >> >> static int brcmf_fw_request_firmware(const struct firmware **fw, >> struct brcmf_fw *fwctx) >> { >> struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; >> - int ret; >> + int ret, i; >> >> /* Files can be board-specific, first try a board-specific path */ >> if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) { >> - char *alt_path; >> + const char **alt_paths = brcm_alt_fw_paths(cur->path, fwctx); >> >> - alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type); >> - if (!alt_path) >> + if (!alt_paths) >> goto fallback; >> >> - ret = request_firmware(fw, alt_path, fwctx->dev); >> - kfree(alt_path); >> - if (ret == 0) >> - return ret; >> + for (i = 0; alt_paths[i]; i++) { >> + ret = firmware_request_nowarn(fw, alt_paths[i], fwctx->dev); >> + if (ret == 0) { >> + brcm_free_alt_fw_paths(alt_paths); >> + return ret; >> + } >> + } >> + brcm_free_alt_fw_paths(alt_paths); >> } >> >> fallback: >> @@ -641,6 +663,9 @@ static void brcmf_fw_request_done(const struct firmware *fw, void *ctx) >> struct brcmf_fw *fwctx = ctx; >> int ret; >> >> + brcm_free_alt_fw_paths(fwctx->alt_paths); >> + fwctx->alt_paths = NULL; > > It looks suspicious that fwctx->alt_paths isn't zero'ed by other code > paths. The brcm_free_alt_fw_paths() should take fwctx for the argument > and fwctx->alt_paths should be set to NULL there. There are multiple code paths for alt_paths; the initial firmware lookup uses fwctx->alt_paths, and once we know the firmware load succeeded we use blocking firmware requests for NVRAM/CLM/etc and those do not use the fwctx member, but rather just keep alt_paths in function scope (brcmf_fw_request_firmware). You're right that there was a rebase SNAFU there though, I'll compile test each patch before sending v2. Sorry about that. In this series the code should build again by patch #6. Are you thinking of any particular code paths? As far as I saw when writing this, brcmf_fw_request_done() should always get called whether things succeed or fail. There are no other code paths that free fwctx->alt_paths. > On the other hand, I'd change the **alt_paths to a fixed-size array. > This should simplify the code, making it easier to follow and maintain. > > - const char **alt_paths; > + char *alt_paths[BRCM_MAX_ALT_FW_PATHS]; > > Then you also won't need to NULL-terminate the array, which is a common > source of bugs in kernel. That sounds reasonable, it'll certainly make the code simpler. I'll do that for v2.
02.01.2022 17:18, Hector Martin пишет: > On 2022/01/02 15:45, Dmitry Osipenko wrote: >> 26.12.2021 18:35, Hector Martin пишет: >>> -static char *brcm_alt_fw_path(const char *path, const char *board_type) >>> +static const char **brcm_alt_fw_paths(const char *path, const char *board_type) >>> { >>> char alt_path[BRCMF_FW_NAME_LEN]; >>> + char **alt_paths; >>> char suffix[5]; >>> >>> strscpy(alt_path, path, BRCMF_FW_NAME_LEN); >>> @@ -609,27 +612,46 @@ static char *brcm_alt_fw_path(const char *path, const char *board_type) >>> strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); >>> strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >>> >>> - return kstrdup(alt_path, GFP_KERNEL); >>> + alt_paths = kzalloc(sizeof(char *) * 2, GFP_KERNEL); >> >> array_size()? > > Of what array? array_size(sizeof(*alt_paths), 2) >>> + alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); >>> + >>> + return (const char **)alt_paths; >> >> Why this casting is needed? > > Because implicit conversion from char ** to const char ** is not legal > in C, as that could cause const unsoundness if you do this: > > char *foo[1]; > const char **bar = foo; > > bar[0] = "constant string"; > foo[0][0] = '!'; // clobbers constant string It's up to a programmer to decide what is right to do. C gives you flexibility, meanwhile it's easy to shoot yourself in the foot if you won't be careful. > But it's fine in this case since the non-const pointer disappears so > nothing can ever write through it again. > There is indeed no need for the castings in such cases, it's a typical code pattern in kernel. You would need to do the casting for the other way around, i.e. if char ** was returned and **alt_paths was a const.
02.01.2022 17:25, Hector Martin пишет: > On 2022/01/02 16:08, Dmitry Osipenko wrote: >> 26.12.2021 18:35, Hector Martin пишет: >>> +static void brcm_free_alt_fw_paths(const char **alt_paths) >>> +{ >>> + int i; >>> + >>> + if (!alt_paths) >>> + return; >>> + >>> + for (i = 0; alt_paths[i]; i++) >>> + kfree(alt_paths[i]); >>> + >>> + kfree(alt_paths); >>> } >>> >>> static int brcmf_fw_request_firmware(const struct firmware **fw, >>> struct brcmf_fw *fwctx) >>> { >>> struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; >>> - int ret; >>> + int ret, i; >>> >>> /* Files can be board-specific, first try a board-specific path */ >>> if (cur->type == BRCMF_FW_TYPE_NVRAM && fwctx->req->board_type) { >>> - char *alt_path; >>> + const char **alt_paths = brcm_alt_fw_paths(cur->path, fwctx); >>> >>> - alt_path = brcm_alt_fw_path(cur->path, fwctx->req->board_type); >>> - if (!alt_path) >>> + if (!alt_paths) >>> goto fallback; >>> >>> - ret = request_firmware(fw, alt_path, fwctx->dev); >>> - kfree(alt_path); >>> - if (ret == 0) >>> - return ret; >>> + for (i = 0; alt_paths[i]; i++) { >>> + ret = firmware_request_nowarn(fw, alt_paths[i], fwctx->dev); >>> + if (ret == 0) { >>> + brcm_free_alt_fw_paths(alt_paths); >>> + return ret; >>> + } >>> + } >>> + brcm_free_alt_fw_paths(alt_paths); >>> } >>> >>> fallback: >>> @@ -641,6 +663,9 @@ static void brcmf_fw_request_done(const struct firmware *fw, void *ctx) >>> struct brcmf_fw *fwctx = ctx; >>> int ret; >>> >>> + brcm_free_alt_fw_paths(fwctx->alt_paths); >>> + fwctx->alt_paths = NULL; >> >> It looks suspicious that fwctx->alt_paths isn't zero'ed by other code >> paths. The brcm_free_alt_fw_paths() should take fwctx for the argument >> and fwctx->alt_paths should be set to NULL there. > > There are multiple code paths for alt_paths; the initial firmware lookup > uses fwctx->alt_paths, and once we know the firmware load succeeded we > use blocking firmware requests for NVRAM/CLM/etc and those do not use > the fwctx member, but rather just keep alt_paths in function scope > (brcmf_fw_request_firmware). You're right that there was a rebase SNAFU > there though, I'll compile test each patch before sending v2. Sorry > about that. In this series the code should build again by patch #6. > > Are you thinking of any particular code paths? As far as I saw when > writing this, brcmf_fw_request_done() should always get called whether > things succeed or fail. There are no other code paths that free > fwctx->alt_paths. It should be okay in the particular case then. But this is not obvious without taking a closer look at the code, which is a sign that there is some room for improvement. >> On the other hand, I'd change the **alt_paths to a fixed-size array. >> This should simplify the code, making it easier to follow and maintain. >> >> - const char **alt_paths; >> + char *alt_paths[BRCM_MAX_ALT_FW_PATHS]; >> >> Then you also won't need to NULL-terminate the array, which is a common >> source of bugs in kernel. > > That sounds reasonable, it'll certainly make the code simpler. I'll do > that for v2. Feel free to CC me on v2. I'll take a closer look and give a test to the patches on older hardware, checking for regressions.
On 03/01/2022 05.11, Dmitry Osipenko wrote: > 02.01.2022 17:18, Hector Martin пишет: >> On 2022/01/02 15:45, Dmitry Osipenko wrote: >>> 26.12.2021 18:35, Hector Martin пишет: >>>> -static char *brcm_alt_fw_path(const char *path, const char *board_type) >>>> +static const char **brcm_alt_fw_paths(const char *path, const char *board_type) >>>> { >>>> char alt_path[BRCMF_FW_NAME_LEN]; >>>> + char **alt_paths; >>>> char suffix[5]; >>>> >>>> strscpy(alt_path, path, BRCMF_FW_NAME_LEN); >>>> @@ -609,27 +612,46 @@ static char *brcm_alt_fw_path(const char *path, const char *board_type) >>>> strlcat(alt_path, board_type, BRCMF_FW_NAME_LEN); >>>> strlcat(alt_path, suffix, BRCMF_FW_NAME_LEN); >>>> >>>> - return kstrdup(alt_path, GFP_KERNEL); >>>> + alt_paths = kzalloc(sizeof(char *) * 2, GFP_KERNEL); >>> >>> array_size()? >> >> Of what array? > > array_size(sizeof(*alt_paths), 2) Heh, TIL. I thought you meant ARRAY_SIZE. First time I see the lowercase macro. That's a confusing name collision... >>>> + alt_paths[0] = kstrdup(alt_path, GFP_KERNEL); >>>> + >>>> + return (const char **)alt_paths; >>> >>> Why this casting is needed? >> >> Because implicit conversion from char ** to const char ** is not legal >> in C, as that could cause const unsoundness if you do this: >> >> char *foo[1]; >> const char **bar = foo; >> >> bar[0] = "constant string"; >> foo[0][0] = '!'; // clobbers constant string > > It's up to a programmer to decide what is right to do. C gives you > flexibility, meanwhile it's easy to shoot yourself in the foot if you > won't be careful. Which is why that conversion is illegal without a cast and you need to explicitly choose to shoot yourself in the foot :-) >> But it's fine in this case since the non-const pointer disappears so >> nothing can ever write through it again. >> > > There is indeed no need for the castings in such cases, it's a typical > code pattern in kernel. You would need to do the casting for the other > way around, i.e. if char ** was returned and **alt_paths was a const. You do need to do the cast. Try it. $ cat test.c int main() { char *foo[1]; const char **bar = foo; return 0; } $ gcc test.c test.c: In function ‘main’: test.c:4:28: warning: initialization of ‘const char **’ from incompatible pointer type ‘char **’ [-Wincompatible-pointer-types] 4 | const char **bar = foo; | You can implicitly cast char* to const char*, but you *cannot* impliclicitly cast char** to const char** for the reason I explained. It requires a cast.
03.01.2022 03:41, Hector Martin пишет: >> There is indeed no need for the castings in such cases, it's a typical >> code pattern in kernel. You would need to do the casting for the other >> way around, i.e. if char ** was returned and **alt_paths was a const. > You do need to do the cast. Try it. > > $ cat test.c > int main() { > char *foo[1]; > const char **bar = foo; > > return 0; > } > > $ gcc test.c > test.c: In function ‘main’: > test.c:4:28: warning: initialization of ‘const char **’ from > incompatible pointer type ‘char **’ [-Wincompatible-pointer-types] > 4 | const char **bar = foo; > | > > You can implicitly cast char* to const char*, but you *cannot* > impliclicitly cast char** to const char** for the reason I explained. It > requires a cast. Right, I read it as "char * const *". The "const char **" vs "char * const *" always confuses me. Hence you should've written "const char **alt_paths;" in brcm_alt_fw_paths() in the first place and then casting wouldn't have been needed.
On 2022/01/02 14:50, Linus Walleij wrote: > On Sun, Dec 26, 2021 at 4:37 PM Hector Martin <marcan@marcan.st> wrote: > >> On Device Tree platforms, it is customary to be able to set the MAC >> address via the Device Tree, as it is often stored in system firmware. >> This is particularly relevant for Apple ARM64 platforms, where this >> information comes from system configuration and passed through by the >> bootloader into the DT. >> >> Implement support for this by fetching the platform MAC address and >> adding or replacing the macaddr= property in nvram. This becomes the >> dongle's default MAC address. >> >> On platforms with an SROM MAC address, this overrides it. On platforms >> without one, such as Apple ARM64 devices, this is required for the >> firmware to boot (it will fail if it does not have a valid MAC at all). >> >> Signed-off-by: Hector Martin <marcan@marcan.st> > > This looks very helpful. > >> + /* Add space for properties we may add */ >> + size += strlen(BRCMF_FW_DEFAULT_BOARDREV) + 1; >> + size += BRCMF_FW_MACADDR_LEN + 1; > > Add some note to the commit log why you also make space for > boardrev? (Looks useful.) Is the boardrev spacing in the right > patch? Ah, that was a drive-by fix. While adding the MACADDR space I noticed we weren't allocating space for BOARDREV... not sure if any platforms hit this; it would cause an overflow if there are platforms with no board_rev in the nvram that also don't have enough comments/junk to otherwise make space for it. I'll move it to another patch so it's more evident, and it should get a Fixes: too.
On 2022/01/02 14:38, Linus Walleij wrote: > On Sun, Dec 26, 2021 at 4:37 PM Hector Martin <marcan@marcan.st> wrote: > >> On Apple platforms, the One Time Programmable ROM in the Broadcom chips >> contains information about the specific board design (module, vendor, >> version) that is required to select the correct NVRAM file. Parse this >> OTP ROM and extract the required strings. >> >> Note that the user OTP offset/size is per-chip. This patch does not add >> any chips yet. >> >> Signed-off-by: Hector Martin <marcan@marcan.st> > > Overall looks fine! > >> + const char *chip_params; >> + const char *module_params; > > This variable name "module_params" is a bit confusing since loadable > kernel modules have params... > > Can we think of another name and just put a comment that this > refers to the WiFi module building block? > > Sometimes people talk about SoM:s (system-on-modules), so > maybe som_params or brcm_som_params? > > Yours, > Linus Walleij > How about board_params, since we're already calling those things boards elsewhere in the driver? That could refer to the board of a standalone module, or an integrated board, which should cover all cases.
On 2022/01/02 14:58, Linus Walleij wrote: > On Sun, Dec 26, 2021 at 4:38 PM Hector Martin <marcan@marcan.st> wrote: > >> On DT platforms, the module-instance and antenna-sku-info properties >> are passed in the DT. On ACPI platforms, module-instance is passed via >> the analogous Apple device property mechanism, while the antenna SKU >> info is instead obtained via an ACPI method that grabs it from >> non-volatile storage. >> >> Add support for this, to allow proper firmware selection on Apple >> platforms. >> >> Signed-off-by: Hector Martin <marcan@marcan.st> > > If the strings treated here are exactly the same as for the device tree, > you should be able to just use "devprops" (firmware node) to handle it > abstractly, and then the respective DT and ACPI backend will provide > the properties. > > I don't know if this patch I made recently is enough of an examples: > https://lore.kernel.org/linux-hwmon/20211206020423.62402-2-linus.walleij@linaro.org/ > > If the ACPI and DT differs a lot in format and strings etc it may not > be worth it. It's not quite the same; module-instance is the same from macOS' perspective, but we don't use Apple's device tree directly but rather roll our own DT which uses a different property name in this case. antenna-sku-info uses an ACPI method on x86, so that one is completely different. So in the end nothing is actually shared.
On 2022/01/03 10:26, Dmitry Osipenko wrote: > 03.01.2022 03:41, Hector Martin пишет: >>> There is indeed no need for the castings in such cases, it's a typical >>> code pattern in kernel. You would need to do the casting for the other >>> way around, i.e. if char ** was returned and **alt_paths was a const. >> You do need to do the cast. Try it. >> >> $ cat test.c >> int main() { >> char *foo[1]; >> const char **bar = foo; >> >> return 0; >> } >> >> $ gcc test.c >> test.c: In function ‘main’: >> test.c:4:28: warning: initialization of ‘const char **’ from >> incompatible pointer type ‘char **’ [-Wincompatible-pointer-types] >> 4 | const char **bar = foo; >> | >> >> You can implicitly cast char* to const char*, but you *cannot* >> impliclicitly cast char** to const char** for the reason I explained. It >> requires a cast. > > Right, I read it as "char * const *". The "const char **" vs "char * > const *" always confuses me. > > Hence you should've written "const char **alt_paths;" in > brcm_alt_fw_paths() in the first place and then casting wouldn't have > been needed. Sure, in this case that works since the string is just strduped and never mutated. Either way this will change to an argument instead of a return value, since I'll change it to be statically sized as you said and allocated by the caller (or in the struct).
On 2022/01/02 15:55, Dmitry Osipenko wrote: > 26.12.2021 18:35, Hector Martin пишет: >> struct brcmf_fw { >> struct device *dev; >> struct brcmf_fw_request *req; >> + const char **alt_paths; > >> + int alt_index; > ... >> +static void brcm_free_alt_fw_paths(const char **alt_paths) >> +{ >> + int i; > ... >> static int brcmf_fw_request_firmware(const struct firmware **fw, >> struct brcmf_fw *fwctx) >> { >> struct brcmf_fw_item *cur = &fwctx->req->items[fwctx->curpos]; >> - int ret; >> + int ret, i; > > unsigned int > Thanks, changed for v2!
On 2022/01/02 15:25, Linus Walleij wrote: > On Sun, Dec 26, 2021 at 4:36 PM Hector Martin <marcan@marcan.st> wrote: > >> Merry Christmas! This year Santa brings us a 34-patch series to add >> proper support for the Broadcom FullMAC chips used on Apple T2 and M1 >> platforms: > > I tried to review as best I could, when I think I know what I'm doing I state > Reviewed-by and when I think it just LooksGoodToMe(TM) I replied > Acked-by. If I missed some patch you can assume Acked-by from me > on these as well. > > Thanks for doing this, some really old bugs and code improvements long > overdue is in the series, much appreciated. > > Yours, > Linus Walleij > Thanks for the comprehensive review! I'm glad this all makes some sense and I'm not crazy about the approach :) I'll wait a bit for any other feedback that might come in and then submit a v2 with the fixes/changes mentioned so far. Cheers,
On 1/3/2022 7:27 AM, Hector Martin wrote: > On 2022/01/02 15:25, Linus Walleij wrote: >> On Sun, Dec 26, 2021 at 4:36 PM Hector Martin <marcan@marcan.st> wrote: >> >>> Merry Christmas! This year Santa brings us a 34-patch series to add >>> proper support for the Broadcom FullMAC chips used on Apple T2 and M1 >>> platforms: >> >> I tried to review as best I could, when I think I know what I'm doing I state >> Reviewed-by and when I think it just LooksGoodToMe(TM) I replied >> Acked-by. If I missed some patch you can assume Acked-by from me >> on these as well. >> >> Thanks for doing this, some really old bugs and code improvements long >> overdue is in the series, much appreciated. >> >> Yours, >> Linus Walleij >> > > Thanks for the comprehensive review! I'm glad this all makes some sense > and I'm not crazy about the approach :) > > I'll wait a bit for any other feedback that might come in and then > submit a v2 with the fixes/changes mentioned so far. Happy new year to you all. I wanted to start the new year relaxing by reviewing this series, but with the comments already given I prefer to do that with v2 so don't wait for me :-) Regards, Arend
On Mon, Jan 3, 2022 at 6:52 AM Hector Martin <marcan@marcan.st> wrote: > On 2022/01/02 14:38, Linus Walleij wrote: > > On Sun, Dec 26, 2021 at 4:37 PM Hector Martin <marcan@marcan.st> wrote: > > > >> On Apple platforms, the One Time Programmable ROM in the Broadcom chips > >> contains information about the specific board design (module, vendor, > >> version) that is required to select the correct NVRAM file. Parse this > >> OTP ROM and extract the required strings. > >> > >> Note that the user OTP offset/size is per-chip. This patch does not add > >> any chips yet. > >> > >> Signed-off-by: Hector Martin <marcan@marcan.st> > > > > Overall looks fine! > > > >> + const char *chip_params; > >> + const char *module_params; > > > > This variable name "module_params" is a bit confusing since loadable > > kernel modules have params... > > > > Can we think of another name and just put a comment that this > > refers to the WiFi module building block? > > > > Sometimes people talk about SoM:s (system-on-modules), so > > maybe som_params or brcm_som_params? > > > > Yours, > > Linus Walleij > > > > How about board_params, since we're already calling those things boards > elsewhere in the driver? That could refer to the board of a standalone > module, or an integrated board, which should cover all cases. Fair enough, go for board_params! Yours, Linus Walleij
On Mon, Jan 3, 2022 at 7:03 AM Hector Martin <marcan@marcan.st> wrote: > On 2022/01/02 14:58, Linus Walleij wrote: > > On Sun, Dec 26, 2021 at 4:38 PM Hector Martin <marcan@marcan.st> wrote: > > > >> On DT platforms, the module-instance and antenna-sku-info properties > >> are passed in the DT. On ACPI platforms, module-instance is passed via > >> the analogous Apple device property mechanism, while the antenna SKU > >> info is instead obtained via an ACPI method that grabs it from > >> non-volatile storage. > >> > >> Add support for this, to allow proper firmware selection on Apple > >> platforms. > >> > >> Signed-off-by: Hector Martin <marcan@marcan.st> > > > > If the strings treated here are exactly the same as for the device tree, > > you should be able to just use "devprops" (firmware node) to handle it > > abstractly, and then the respective DT and ACPI backend will provide > > the properties. > > > > I don't know if this patch I made recently is enough of an examples: > > https://lore.kernel.org/linux-hwmon/20211206020423.62402-2-linus.walleij@linaro.org/ > > > > If the ACPI and DT differs a lot in format and strings etc it may not > > be worth it. > > It's not quite the same; module-instance is the same from macOS' > perspective, but we don't use Apple's device tree directly but rather > roll our own DT which uses a different property name in this case. > antenna-sku-info uses an ACPI method on x86, so that one is completely > different. So in the end nothing is actually shared. OK then! Acked-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On 2022/01/04 1:20, Andy Shevchenko wrote: > +void brcmf_acpi_probe(struct device *dev, enum brcmf_bus_type bus_type, > + struct brcmf_mp_device *settings) > +{ > + acpi_status status; > + struct acpi_device *adev = ACPI_COMPANION(dev); > > > Please, move the assignment closer to its first user So... two lines down? :-) > > > + const union acpi_object *o; > + struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL}; > + > + if (!adev) > + return; > + > + if (!ACPI_FAILURE(acpi_dev_get_property(adev, "module-instance", > + ACPI_TYPE_STRING, > &o))) { > + const char *prefix = "apple,"; > + int len = strlen(prefix) + o->string.length + 1; > + char *board_type = devm_kzalloc(dev, len, GFP_KERNEL); > + > + strscpy(board_type, prefix, len); > + strlcat(board_type, o->string.pointer, > > > NIH devm_kasprintf()? That sounds useful, didn't know that existed. Thanks! > > > + brcmf_dbg(INFO, "ACPI module-instance=%s\n", > o->string.pointer); > + settings->board_type = board_type; > + } else { > + brcmf_dbg(INFO, "No ACPI module-instance\n"); > + } > + > + status = acpi_evaluate_object(adev->handle, "RWCV", NULL, &buf); > + o = buf.pointer; > + if (!ACPI_FAILURE(status) && o && o->type == ACPI_TYPE_BUFFER && > + o->buffer.length >= 2) { > + char *antenna_sku = devm_kzalloc(dev, 3, GFP_KERNEL); > + > + memcpy(antenna_sku, o->buffer.pointer, 2); > > > NIH devm_kmemdup()? Not *quite*. I take the first two bytes of the returned buffer and turn them into a null-terminated 3-byte string. kmemdup wouldn't null-terminate or would copy too much, depending on length.
On 2022/01/04 7:50, Andy Shevchenko wrote: > > + status = acpi_evaluate_object(adev->handle, "RWCV", > NULL, &buf); > > + o = buf.pointer; > > + if (!ACPI_FAILURE(status) && o && o->type == > ACPI_TYPE_BUFFER && > > + o->buffer.length >= 2) { > > + char *antenna_sku = devm_kzalloc(dev, 3, > GFP_KERNEL); > > + > > + memcpy(antenna_sku, o->buffer.pointer, 2); > > > > > > NIH devm_kmemdup()? > > Not *quite*. I take the first two bytes of the returned buffer and turn > them into a null-terminated 3-byte string. kmemdup wouldn't > null-terminate or would copy too much, depending on length. > > > > devm_kstrndup() then? > > That doesn't seem to be a thing.
Andy Shevchenko <andy.shevchenko@gmail.com> writes: > On Sunday, December 26, 2021, Hector Martin <marcan@marcan.st> wrote: > > On DT platforms, the module-instance and antenna-sku-info properties > are passed in the DT. On ACPI platforms, module-instance is passed via > the analogous Apple device property mechanism, while the antenna SKU > info is instead obtained via an ACPI method that grabs it from > non-volatile storage. > > Add support for this, to allow proper firmware selection on Apple > platforms. > > Signed-off-by: Hector Martin <marcan@marcan.st> > --- > .../broadcom/brcm80211/brcmfmac/Makefile | 2 + > .../broadcom/brcm80211/brcmfmac/acpi.c | 51 +++++++++++++++++++ > .../broadcom/brcm80211/brcmfmac/common.c | 1 + > .../broadcom/brcm80211/brcmfmac/common.h | 9 ++++ > 4 files changed, 63 insertions(+) > create mode 100644 drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile > index 13c13504a6e8..19009eb9db93 100644 > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/Makefile > @@ -47,3 +47,5 @@ brcmfmac-$(CONFIG_OF) += \ > of.o > brcmfmac-$(CONFIG_DMI) += \ > dmi.o > +brcmfmac-$(CONFIG_ACPI) += \ > + acpi.o > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > new file mode 100644 > index 000000000000..3e56dc7a8db2 > --- /dev/null > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/acpi.c > @@ -0,0 +1,51 @@ > +// SPDX-License-Identifier: ISC > +/* > + * Copyright The Asahi Linux Contributors > + */ > + > +#include <linux/acpi.h> > +#include "debug.h" > +#include "core.h" > +#include "common.h" > + > +void brcmf_acpi_probe(struct device *dev, enum brcmf_bus_type bus_type, > + struct brcmf_mp_device *settings) > +{ > + acpi_status status; > + struct acpi_device *adev = ACPI_COMPANION(dev); > > Please, move the assignment closer to its first user Andy, your email was formatted in HTML. I'm sure you know this already, but our mailing lists drop all HTML emails so other people (and patchwork) don't see your comments.