Re: [PATCH RFC] power: sequencing: pcie-m2: stop requiring subsystem IDs for WCN6855/WCN7850

[Manivannan Sadhasivam <mani@kernel.org>]

On Fri, Nov 06, 2026 at 07:30:06PM +0100, Jens Glathe wrote:
> Hi sashiko-bot,
> 
> On 11/6/26 12:26, sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 1 potential
> > issue to consider:
> > - [High] pwrseq: pcie-m2: bare PCI_DEVICE() creates bogus serdev on
> >    USB-BT cards
> > --
> > 
> > commit 651d16502b81077112cba212f5fb84dc03306321
> > Author: Jens Glathe <jens.glathe@oldschoolsolutions.biz>
> > 
> > power: sequencing: pcie-m2: stop requiring subsystem IDs for WCN6855/WCN7850
> > 
> > This commit replaces the subsystem-specific PCI_DEVICE_SUB() entries for
> > Qualcomm WCN6855 (0x1103) and WCN7850 (0x1107) in pwrseq_m2_pci_ids[] with
> > bare PCI_DEVICE() entries.
> > 
> > > diff --git a/drivers/power/sequencing/pwrseq-pcie-m2.c b/drivers/power/sequencing/pwrseq-pcie-m2.c
> > > index 56c5d51293c10..72c721149dfae 100644
> > > --- a/drivers/power/sequencing/pwrseq-pcie-m2.c
> > > +++ b/drivers/power/sequencing/pwrseq-pcie-m2.c
> > > @@ -217,15 +217,9 @@ static const struct pci_device_id pwrseq_m2_pci_ids[] = {
> > >   	  .driver_data = (kernel_ulong_t)"nxp,88w8987-bt" },
> > >   	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_QCOM, 0x1103, PCI_VENDOR_ID_QCOM, 0x0108),
> > >   	  .driver_data = (kernel_ulong_t)"qcom,qca2066-bt" },
> > > -	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_QCOM, 0x1103, PCI_VENDOR_ID_FOXCONN, 0xe105),
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_QCOM, 0x1103),
> > >   	  .driver_data = (kernel_ulong_t)"qcom,wcn6855-bt" },
> > > -	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_QCOM, 0x1103, PCI_VENDOR_ID_QCOM, 0x337e),
> > > -	  .driver_data = (kernel_ulong_t)"qcom,wcn6855-bt" },
> > > -	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_QCOM, 0x1107, PCI_VENDOR_ID_QCOM, 0x337c),
> > > -	  .driver_data = (kernel_ulong_t)"qcom,wcn7850-bt" },
> > > -	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_QCOM, 0x1107, PCI_VENDOR_ID_LENOVO, 0xe0e9),
> > > -	  .driver_data = (kernel_ulong_t)"qcom,wcn7850-bt" },
> > > -	{ PCI_DEVICE_SUB(PCI_VENDOR_ID_QCOM, 0x1107, PCI_VENDOR_ID_FOXCONN, 0xe0ee),
> > > +	{ PCI_DEVICE(PCI_VENDOR_ID_QCOM, 0x1107),
> > >   	  .driver_data = (kernel_ulong_t)"qcom,wcn7850-bt" },
> > >   	{ } /* Sentinel */
> > >   };
> > [Severity: High]
> > Won't matching bare 0x1103 and 0x1107 device IDs reintroduce the regression
> > fixed by a39ac4651e3b ("power: sequencing: pcie-m2: Match WCN6855 and
> > WCN7851 UART BT variants by subdevice ID") whenever a USB-BT M.2 card is
> > plugged into a socketed M.2 Key E slot?
> > 
> > The pcie-m2-e-connector node describes the physical M.2 slot wiring on the
> > host board, not the card installed in it.  Boards with a general-purpose M.2
> > Key E socket (such as arch/arm64/boot/dts/qcom/hamoa-iot-evk.dts) wire the
> > SoC UART to the slot and define port@3 unconditionally in their static DT.
> > 
> > When a USB-BT WCN6855 or WCN7850/WCN7851 card is inserted into such a slot,
> > pwrseq_pcie_m2_create_serdev() -> pwrseq_pcie_m2_create_serdev_one() sees
> > port@3 on ctx->dev and matches 0x1103 or 0x1107 in pwrseq_m2_pci_ids[].  It
> > then creates a bogus bluetooth OF node and UART serdev device, causing
> > hci_qca to bind alongside btusb and fail with -ETIMEDOUT in qca_setup() on
> > the unconnected UART lines.
> > 
> > Also, the commit message notes that Lenovo T14s Gen 6 (17aa:e0e9) is missing
> > from the table, but 58a003302481 ("power: sequencing: pcie-m2: Add Lenovo
> > ThinkPad T14s gen6 WCN7850 subsystem PCI ids") already added that entry
> > above.
> Well. It was missing upstream when I made the experience on my T14s G6.
> Having a whitelist for every subsystem that is allowed in such a generic
> driver is not the best design decision, IMO.
> I took another look, this driver is loaded when it is a compatible in a
> device tree. Usually this means the hardware got described, and you can draw
> conclusions from the device tree on what is supported. For the conflicting
> case, the device tree would need to describe the USB and the serial port on
> the e-key connector. When you have a connector wired up this way, the driver
> would need directions on what to choose, for instance by providing the
> subsystem IDs as base for a decision. These are the special cases, though.
> For most boards describing the uart on the connector would suffice to choose
> the right, mostly generic driver for WCN6855 and WCN7850. So it would be
> better to have the driver detect the "both ports are defined in the dt" case
> and  not create a serdev:
> 
> static bool pwrseq_pcie_m2_bt_ambiguous(struct device *dev)
> {
>     bool has_usb  = pwrseq_pcie_m2_check_remote_node(dev, 2, 0, "usb");
>     bool has_uart = pwrseq_pcie_m2_check_remote_node(dev, 3, 0, "serial");
> 
>     return has_usb && has_uart;
> }
> 

This is wrong. A generic slot can define both 'usb' and 'serial' endpoint
nodes and users can connect any key E cards to the slot. So having both nodes
doesn't mean that the user is only going to connect BT-UART cards.

While I agree with you that populating the ID table for each module is a pain,
we can't magically detect the card type before probing the device.

- Mani

-- 
மணிவண்ணன் சதாசிவம்