| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <igtegc4wgi3xiolpbilr3jw7c4xlyrj2d4bqb4b4m2yxcxutjh@4evsl2w6taud>
Date: Wed, 19 Nov 2025 19:24:12 +0530
From: Manivannan Sadhasivam <mani@...nel.org>
To: Bartosz Golaszewski <brgl@...ev.pl>
Cc: manivannan.sadhasivam@....qualcomm.com, Rob Herring <robh@...nel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby <jirislaby@...nel.org>,
Nathan Chancellor <nathan@...nel.org>, Nicolas Schier <nicolas.schier@...ux.dev>,
Hans de Goede <hansg@...nel.org>, Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>,
Mark Pearson <mpearson-lenovo@...ebb.ca>, "Derek J. Clark" <derekjohn.clark@...il.com>,
Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Marcel Holtmann <marcel@...tmann.org>, Luiz Augusto von Dentz <luiz.dentz@...il.com>,
linux-serial@...r.kernel.org, linux-kernel@...r.kernel.org, linux-kbuild@...r.kernel.org,
platform-driver-x86@...r.kernel.org, linux-pci@...r.kernel.org, devicetree@...r.kernel.org,
linux-arm-msm@...r.kernel.org, linux-bluetooth@...r.kernel.org, linux-pm@...r.kernel.org,
Stephan Gerhold <stephan.gerhold@...aro.org>, Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Subject: Re: [PATCH 9/9] power: sequencing: pcie-m2: Add support for PCIe M.2
Key E connectors
On Wed, Nov 19, 2025 at 02:28:00PM +0100, Bartosz Golaszewski wrote:
> On Wed, Nov 12, 2025 at 3:45 PM Manivannan Sadhasivam via B4 Relay
> <devnull+manivannan.sadhasivam.oss.qualcomm.com@...nel.org> wrote:
> >
> > From: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> >
> > Add support for handling the power sequence of the PCIe M.2 Key E
> > connectors. These connectors are used to attach the Wireless Connectivity
> > devices to the host machine including combinations of WiFi, BT, NFC using
> > interfaces such as PCIe/SDIO for WiFi, USB/UART for BT and I2C for NFC.
> >
> > Currently, this driver supports only the PCIe interface for WiFi and UART
> > interface for BT. The driver also only supports driving the 3.3v/1.8v power
> > supplies and W_DISABLE{1/2}# GPIOs. The optional signals of the Key E
> > connectors are not currently supported.
> >
> > For supporting Bluetooth over the non-discoverable UART interface, the
> > driver currently creates the serdev interface after enumerating the PCIe
> > interface. This is mandatory since the device ID is only known after the
> > PCIe enumeration and the ID is used for creating the serdev device.
> >
> > Signed-off-by: Manivannan Sadhasivam <manivannan.sadhasivam@....qualcomm.com>
> > ---
> >
> > +static int pwrseq_pci_m2_e_uart_enable(struct pwrseq_device *pwrseq)
> > +{
> > + struct pwrseq_pcie_m2_ctx *ctx = pwrseq_device_get_drvdata(pwrseq);
> > +
> > + gpiod_set_value_cansleep(ctx->w_disable2_gpio, 0);
>
> Since this is new code and gpiod_set_value_cansleep() now returns an
> integer, can you do
>
Ack
> return gpiod_set_value_cansleep()?
>
> Same elsewhere.
>
> >
> > +static int pwrseq_m2_pcie_notify(struct notifier_block *nb, unsigned long action,
> > + void *data)
> > +{
> > + struct pwrseq_pcie_m2_ctx *ctx = container_of(nb, struct pwrseq_pcie_m2_ctx, nb);
> > + struct pci_dev *pdev = to_pci_dev(data);
> > + struct device_node *remote;
> > + struct serdev_controller *serdev_ctrl;
> > + struct serdev_device *serdev;
> > + struct device *dev = ctx->dev;
> > + int ret;
> > +
> > + /*
> > + * Check whether the PCI device is associated with this M.2 connector or
> > + * not, by comparing the OF node of the PCI device parent and the Port 0
> > + * (PCIe) remote node parent OF node.
> > + */
> > + remote = of_graph_get_remote_node(dev_of_node(ctx->dev), 0, -1);
> > + if (!remote || (remote != pdev->dev.parent->of_node)) {
> > + of_node_put(remote);
>
> You could really use some __free(device_node) here. It would simplify
> the code below quite a bit and make sure you don't miss anything.
>
Ack. I'm still trying to get used to the scope based cleanup :)
> > + return NOTIFY_DONE;
> > + }
> > + of_node_put(remote);
> > +
> > + switch (action) {
> > + case BUS_NOTIFY_ADD_DEVICE:
> > + /* Create serdev device for WCN7850 */
> > + if (pdev->vendor == PCI_VENDOR_ID_QCOM && pdev->device == 0x1107) {
> > + remote = of_graph_get_remote_node(dev_of_node(ctx->dev), 1, -1);
> > + if (!remote) {
> > + of_node_put(remote);
> > + return NOTIFY_DONE;
> > + }
> > +
> > + serdev_ctrl = of_find_serdev_controller_by_node(remote);
> > + of_node_put(remote);
> > + if (!serdev_ctrl)
> > + return NOTIFY_DONE;
> > +
> > + serdev = serdev_device_alloc(serdev_ctrl);
> > + if (!serdev)
> > + return NOTIFY_DONE;
> > +
> > + ret = serdev_device_add(serdev, "WCN7850");
> > + if (ret) {
> > + dev_err(dev, "Failed to add serdev for WCN7850: %d\n", ret);
> > + serdev_device_put(serdev);
> > + return NOTIFY_DONE;
> > + }
> > + }
> > + break;
> > + }
> > +
> > + return NOTIFY_DONE;
> > +}
> > +
> > +static bool pwrseq_pcie_m2_check_remote_node(struct device *dev, u8 port, const char *node)
> > +{
> > + struct device_node *remote;
>
> Same here.
>
> > +
> > + remote = of_graph_get_remote_node(dev_of_node(dev), port, -1);
> > + if (remote && of_node_name_eq(remote, node)) {
> > + of_node_put(remote);
> > + return true;
> > + }
> > +
> > + of_node_put(remote);
> > +
> > + return false;
> > +}
> > +
> > +/*
> > + * If the connector exposes a non-discoverable bus like UART, the respective
> > + * protocol device needs to be created manually with the help of the notifier
> > + * of the discoverable bus like PCIe.
> > + */
>
> I really like this idea BTW!
>
> > +static void pwrseq_pcie_m2_register_notifier(struct pwrseq_pcie_m2_ctx *ctx, struct device *dev)
> > +{
> > + int ret;
> > +
> > + /*
> > + * Register a PCI notifier for Key E connector that has PCIe as Port 0
> > + * interface and Serial as Port 1 interface.
> > + */
> > + if (pwrseq_pcie_m2_check_remote_node(dev, 1, "serial")) {
> > + if (pwrseq_pcie_m2_check_remote_node(dev, 0, "pcie")) {
> > + ctx->dev = dev;
> > + ctx->nb.notifier_call = pwrseq_m2_pcie_notify;
> > + ret = (bus_register_notifier(&pci_bus_type, &ctx->nb));
> > + if (ret) {
> > + dev_err_probe(dev, ret, "Failed to register notifier for serdev\n");
>
> If this is optional and we don't handle the error, should we really
> print it as one? I'd say a dev_dbg() would suffice unless the failure
> here impacts the driver's behavior (which it does, so maybe the
> notifier should not be optional?).
>
Fair enough. I was on double mind tbh. But it makes sense to return failure.
- Mani
--
மணிவண்ணன் சதாசிவம்
Powered by blists - more mailing lists