linux-kernel - Re: [PATCH v6 4/5] wifi: brcmfmac: Add optional lpo clock enable support

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CABjd4YxiSY0A0iVHGHw9RDey+avxmzUapoLLLyf=80MzVX0yWA@mail.gmail.com>
Date: Wed, 31 Jul 2024 15:01:41 +0300
From: Alexey Charkov <alchark@...il.com>
To: Arend van Spriel <arend.vanspriel@...adcom.com>
Cc: Jacobe Zang <jacobe.zang@...ion.com>, robh@...nel.org, krzk+dt@...nel.org, 
	heiko@...ech.de, kvalo@...nel.org, davem@...emloft.net, edumazet@...gle.com, 
	kuba@...nel.org, pabeni@...hat.com, conor+dt@...nel.org, 
	efectn@...tonmail.com, dsimic@...jaro.org, jagan@...eble.ai, 
	devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, 
	linux-rockchip@...ts.infradead.org, linux-kernel@...r.kernel.org, 
	arend@...adcom.com, linux-wireless@...r.kernel.org, netdev@...r.kernel.org, 
	megi@....cz, duoming@....edu.cn, bhelgaas@...gle.com, minipli@...ecurity.net, 
	brcm80211@...ts.linux.dev, brcm80211-dev-list.pdl@...adcom.com, 
	nick@...das.com
Subject: Re: [PATCH v6 4/5] wifi: brcmfmac: Add optional lpo clock enable support

On Wed, Jul 31, 2024 at 2:15 PM Arend van Spriel
<arend.vanspriel@...adcom.com> wrote:
>
> On 7/31/2024 12:16 PM, Alexey Charkov wrote:
> > Hi Jacobe,
> >
> >
> > On 31/07/2024 9:11 am, Jacobe Zang wrote:
> >  > WiFi modules often require 32kHz clock to function. Add support to
> >  > enable the clock to PCIe driver and move "brcm,bcm4329-fmac" check
> >  > to the top of brcmf_of_probe
> >  >
> >  > Co-developed-by: Ondrej Jirman <megi@....cz>
> >  > Signed-off-by: Ondrej Jirman <megi@....cz>
> >  > Signed-off-by: Jacobe Zang <jacobe.zang@...ion.com>
> >  > ---
> >  >  .../net/wireless/broadcom/brcm80211/brcmfmac/of.c    | 12 +++++++++++-
> >  >  1 file changed, 11 insertions(+), 1 deletion(-)
> >  >
> >  > diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> > b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> >  > index e406e11481a62..7e0a2ad5c7c8a 100644
> >  > --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> >  > +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/of.c
> >  > @@ -6,6 +6,7 @@
> >  >  #include <linux/of.h>
> >  >  #include <linux/of_irq.h>
> >  >  #include <linux/of_net.h>
> >  > +#include <linux/clk.h>
> >  >
> >  >  #include <defs.h>
> >  >  #include "debug.h"
> >  > @@ -70,12 +71,16 @@ void brcmf_of_probe(struct device *dev, enum
> > brcmf_bus_type bus_type,
> >  >  {
> >  >      struct brcmfmac_sdio_pd *sdio = &settings->bus.sdio;
> >  >      struct device_node *root, *np = dev->of_node;
> >  > +    struct clk *clk;
> >  >      const char *prop;
> >  >      int irq;
> >  >      int err;
> >  >      u32 irqf;
> >  >      u32 val;
> >  >
> >  > +    if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac"))
> >  > +        return;
> >
> > Did you test this? The DTS patch you sent as part of this series doesn't
> > list "brcm,bcm4329-fmac" in the compatible, so this will probably return
> > right here, skipping over the rest of your patch.
> >
> > Please test before resending, both with and without the driver for the
> > Bluetooth part of the chip (since it also touches clocks).
> >
> > You are also changing the behavior for other systems by putting this
> > check further up the probe path, which might break things for no reason.
> > Better put your clk-related addition below where this check was
> > originally, rather than reorder stuff you don't have to reorder.
>
> That was upon my suggestion. That check was originally at the top of the
> function, but people added stuff before that. I agree that makes the
> compatible "brcm,brcm4329-fmac" required which is what the textual
> binding stated before the switch to YAML was made:
>
> """
> Broadcom BCM43xx Fullmac wireless SDIO devices
>
> This node provides properties for controlling the Broadcom wireless
> device. The
> node is expected to be specified as a child node to the SDIO controller that
> connects the device to the system.
>
> Required properties:
>
>   - compatible : Should be "brcm,bcm4329-fmac".
> """
>
> Not sure whether this is still true for YAML version (poor YAML reading
> skills ;-) ), but it should as the switch from textual to YAML should
> not have changed the bindings specification.
>
> >  > +
> >  >      /* Apple ARM64 platforms have their own idea of board type,
> > passed in
> >  >       * via the device tree. They also have an antenna SKU parameter
> >  >       */
> >  > @@ -113,8 +118,13 @@ void brcmf_of_probe(struct device *dev, enum
> > brcmf_bus_type bus_type,
> >  >          of_node_put(root);
> >  >      }
> >  >
> >  > -    if (!np || !of_device_is_compatible(np, "brcm,bcm4329-fmac"))
> >  > +    clk = devm_clk_get_optional_enabled(dev, "lpo");
> >  > +    if (!IS_ERR_OR_NULL(clk)) {
> >  > +        brcmf_dbg(INFO, "enabling 32kHz clock\n");
> >  > +        clk_set_rate(clk, 32768);
> >  > +    } else {
> >  >          return;
> >
> > Why return here? If the clock is optional, a lot of systems will not
> > have it - that shouldn't prevent the driver from probing. And you are
> > still not handling the -EPROBE_DEFER case which was mentioned on your
> > previous submission.
>
> Right. The else statement above could/should be:
>
> } else if (clk && PTR_ERR(clk) == -EPROBE_DEFER) {
>          return PTR_ERR(clk);
> }

... plus change the function prototype to return int and propagate
that error code through brcmf_get_module_param to brcmf_pcie_probe's
return value. I guess checking clk for NULL is also redundant in this
case?

Best,
Alexey