* Re: [PATCH v3 2/6] dt-bindings: display: mediatek: Add compatibles for MediaTek mt8167
From: CK Hu (胡俊光) @ 2026-02-25 2:23 UTC (permalink / raw)
To: linux-mediatek@lists.infradead.org, l.scorcia@gmail.com
Cc: robh@kernel.org, Chunfeng Yun (云春峰),
krzysztof.kozlowski@oss.qualcomm.com, simona@ffwll.ch,
tzimmermann@suse.de, mripard@kernel.org,
AngeloGioacchino Del Regno, linux-kernel@vger.kernel.org,
maarten.lankhorst@linux.intel.com, conor+dt@kernel.org,
chunkuang.hu@kernel.org, vkoul@kernel.org, krzk+dt@kernel.org,
linux-phy@lists.infradead.org, p.zabel@pengutronix.de,
dri-devel@lists.freedesktop.org, airlied@gmail.com,
linux-arm-kernel@lists.infradead.org, neil.armstrong@linaro.org,
matthias.bgg@gmail.com, devicetree@vger.kernel.org
In-Reply-To: <66eafae30f9fe00b469e79d385c1ddd24d209475.1771863641.git.l.scorcia@gmail.com>
On Mon, 2026-02-23 at 16:22 +0000, Luca Leonardo Scorcia wrote:
> External email : Please do not click links or open attachments until you have verified the sender or the content.
>
>
> Add compatibles for various display-related blocks of MediaTek mt8167.
Reviewed-by: CK Hu <ck.hu@mediatek.com>
>
> Signed-off-by: Luca Leonardo Scorcia <l.scorcia@gmail.com>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>
> ---
> .../devicetree/bindings/display/mediatek/mediatek,aal.yaml | 1 +
> .../devicetree/bindings/display/mediatek/mediatek,ccorr.yaml | 4 +++-
> .../devicetree/bindings/display/mediatek/mediatek,dither.yaml | 1 +
> .../devicetree/bindings/display/mediatek/mediatek,gamma.yaml | 1 +
> .../devicetree/bindings/display/mediatek/mediatek,ovl.yaml | 1 +
> .../devicetree/bindings/display/mediatek/mediatek,rdma.yaml | 1 +
> .../devicetree/bindings/display/mediatek/mediatek,wdma.yaml | 4 +++-
> 7 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> index daf90ebb39bf..4bbea72b292a 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,aal.yaml
> @@ -33,6 +33,7 @@ properties:
> - enum:
> - mediatek,mt2712-disp-aal
> - mediatek,mt6795-disp-aal
> + - mediatek,mt8167-disp-aal
> - const: mediatek,mt8173-disp-aal
> - items:
> - enum:
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> index fca8e7bb0cbc..5c5068128d0c 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ccorr.yaml
> @@ -25,7 +25,9 @@ properties:
> - mediatek,mt8183-disp-ccorr
> - mediatek,mt8192-disp-ccorr
> - items:
> - - const: mediatek,mt8365-disp-ccorr
> + - enum:
> + - mediatek,mt8167-disp-ccorr
> + - mediatek,mt8365-disp-ccorr
> - const: mediatek,mt8183-disp-ccorr
> - items:
> - enum:
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
> index abaf27916d13..891c95be15b9 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,dither.yaml
> @@ -26,6 +26,7 @@ properties:
> - mediatek,mt8183-disp-dither
> - items:
> - enum:
> + - mediatek,mt8167-disp-dither
> - mediatek,mt8186-disp-dither
> - mediatek,mt8188-disp-dither
> - mediatek,mt8192-disp-dither
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
> index 48542dc7e784..ec1054bb06d4 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,gamma.yaml
> @@ -28,6 +28,7 @@ properties:
> - items:
> - enum:
> - mediatek,mt6795-disp-gamma
> + - mediatek,mt8167-disp-gamma
> - const: mediatek,mt8173-disp-gamma
> - items:
> - enum:
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yaml
> index 4f110635afb6..679f731f0f15 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,ovl.yaml
> @@ -23,6 +23,7 @@ properties:
> oneOf:
> - enum:
> - mediatek,mt2701-disp-ovl
> + - mediatek,mt8167-disp-ovl
> - mediatek,mt8173-disp-ovl
> - mediatek,mt8183-disp-ovl
> - mediatek,mt8192-disp-ovl
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,rdma.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,rdma.yaml
> index 878f676b581f..cb187a95c11e 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,rdma.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,rdma.yaml
> @@ -36,6 +36,7 @@ properties:
> - enum:
> - mediatek,mt7623-disp-rdma
> - mediatek,mt2712-disp-rdma
> + - mediatek,mt8167-disp-rdma
> - const: mediatek,mt2701-disp-rdma
> - items:
> - enum:
> diff --git a/Documentation/devicetree/bindings/display/mediatek/mediatek,wdma.yaml b/Documentation/devicetree/bindings/display/mediatek/mediatek,wdma.yaml
> index a3a2b71a4523..816841a96133 100644
> --- a/Documentation/devicetree/bindings/display/mediatek/mediatek,wdma.yaml
> +++ b/Documentation/devicetree/bindings/display/mediatek/mediatek,wdma.yaml
> @@ -24,7 +24,9 @@ properties:
> - enum:
> - mediatek,mt8173-disp-wdma
> - items:
> - - const: mediatek,mt6795-disp-wdma
> + - enum:
> + - mediatek,mt6795-disp-wdma
> + - mediatek,mt8167-disp-wdma
> - const: mediatek,mt8173-disp-wdma
>
> reg:
> --
> 2.43.0
>
>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH] phy: core: fix potential UAF in of_phy_simple_xlate()
From: Dmitry Torokhov @ 2026-02-24 20:38 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Vinod Koul, Kishon Vijay Abraham I, Neil Armstrong,
Rafael J. Wysocki, Geert Uytterhoeven, Johan Hovold,
Claudiu Beznea, Dr. David Alan Gilbert, Peter Griffin,
Dmitry Baryshkov, Krzysztof Kozlowski, Zijun Hu, linux-phy,
linux-kernel
In-Reply-To: <20260224154730.qqnomchkdpxnyf4x@skbuf>
On Tue, Feb 24, 2026 at 05:47:30PM +0200, Vladimir Oltean wrote:
> On Mon, Feb 23, 2026 at 04:37:39PM -0800, Dmitry Torokhov wrote:
> > > You have hit on a weak spot of the Generic PHY framework and I would
> > > like to encourage you to follow through with patches for this finding
> > > (although it won't be exactly trivial, I think it's doable with some
> > > determination).
> >
> > Thank you for the detailed response. I am not sure how much time I can
> > spend on phy rework as this change was an essentially a drive-by. I only
> > happen to look there because I want to remove
> > class_find_device_by_of_node() in favor of class_find_device_by_fwnode().
>
> OK, I believe you can still do that without any dependency.
Right, this is independent, I just happened to look at that function.
>
> > > On your direct question:
> > > It will impact downstream in subtle and unpleasant ways if you change
> > > the of_xlate() semantics w.r.t. reference counting, but the code will
> > > still compile just as before, i.e. when looking just at your downstream
> > > driver (what 99% of developers do) there will be no obvious way of
> > > knowing that something in the API has changed. I would avoid doing that.
> >
> > Hmm, I was not aware that we needed to take particular measures for
> > benefit of downstream trees...
>
> I think you're entirely missing the point.
>
> Obviously it benefits the maintainer/reviewer first and foremost. If you
> don't make the API change unmissable, drivers expecting the old convention
> will eventually creep their way into the mainline kernel. Why put avoidable
> pressure on reviewers watching out for things like this for years to come,
> when you can force downstream to notice and adapt (OR not make the
> change in the first place). Note that downstream means "API consumers
> invisible to you, the API changer", not only "hopelessly un-upstreamable
> drivers".
Well, this is whole "stable kernel API" argument. Rules of lifetime
around objects may change and the kernel needs to adapt. Anyway, this is
your subsystem so you get to decide.
>
> > > The patch above leaves a few loose ends.
> > >
> > > The most obvious is of_phy_simple_xlate(), which has that put_device()
> > > to balance out class_find_device_by_of_node() - which bumps the device
> > > refcount to 2. It "looks" wrong but it is consistent with vendor
> > > implementations of of_xlate(), which also provide a phy->dev refcount of 1.
> > > And as explained, the refcount never _actually_ drops to 0 with the
> > > above patch.
> > >
> > > I would actually address _only_ of_phy_simple_xlate(), for cosmetics sake.
> > > Instead of devm_of_phy_provider_register(..., of_phy_simple_xlate), I
> > > would offer a new helper, devm_of_phy_simple_provider_register(), which
> > > would internally use a callback that doesn't drop the refcount (say
> > > __of_phy_simple_xlate() for lack of imagination). I would convert vendor
> > > PHY drivers one by one to use this (it would be valid at any time to use
> > > either the old or the new method).
> > >
> > > You'd notice that most of the of_phy_simple_xlate() occurrences go away,
> > > except for freescale/phy-fsl-lynx-28g.c which calls this function
> > > directly from its own lynx_28g_xlate(). It will still have to keep
> > > calling it, and for refcount equalization purposes that weird
> > > put_device() will have to continue to exist. Just leave a comment as to
> > > why that is.
> >
> > I this case I would simply add a comment telling why the reference
> > should (and can) be dropped where it is being dropped in
> > of_phy_simple_xlate() and call it a day. There is not much benefit in
> > adding another helper.
>
> OK.
>
> > > But there are more important loose ends still.
> > >
> > > I mentioned that "zombie" device. We've solved the memory safety issue,
> > > but it's possible for consumers to hold onto a phy whose provider has
> > > disappeared. The refcount of &phy->dev hasn't dropped to 0, so
> > > technically it's a valid object, but from PHY API perspective, it's
> > > still possible to call phy_init(), phy_power_on() and friends on this
> > > PHY, and the Generic PHY core will be happy to further call into the
> > > phy->ops->init(), phy->ops->power_on() etc. But the driver has unbound,
> > > so it should really be left alone.
> > >
> > > If we fix the UAF but leave the zombie PHY problem, we've effectively
> > > done nothing but silence static analysis checkers, while the code path
> > > where the PHY provider unbinds effectively remains treated as poorly as
> > > before, just moving the crashes to a different place.
> > >
> > > I suspect what needs to be done here is to introduce a "bool dead" or
> > > similar, which is to be set from phy_destroy() and checked from every
> > > API call. The idea is that API functions on zombie PHYs should fail
> > > without calling into their driver. I **suppose** that
> > > try_module_get(phy->ops->owner) "tried" to avoid this situation, but
> > > it just protects against module removal, not against "echo device >
> > > /sys/bus/.../unbind". So it's absolutely incomplete and easily bypassable.
> >
> > I think this is a problem common to many kernel subsystems, where there
> > are devices that are vital for other devices to function, but are not
> > parents of said devices. Think about regulators or clocks and such.
> > In many such cases even validating at API level is not sufficient,
> > because if you enable a clock you typically keep it running at least for
> > a while, if not for entire duration of device being bound to a driver.
> > If that clock goes away the device will break even though you are not
> > calling any clock APIs at that particular time.
> >
> > We need some kind of revocation mechanism to signal consumers that
> > providers they rely upon are going away.
>
> Yeah, this gave me pause.
>
> When you unbind a Generic PHY you can kind of expect that the data path
> it affects to not work. But you'd sort of expect it to start working
> again when you rebind the PHY driver.
>
> That will not be the case, though.
>
> The problem, simply put, is that the struct phy that the consumer has
> will be different from the second struct phy that the provider registers
> when it rebinds. Using the stale struct phy, we cannot reach the phy_ops
> of the new provider.
>
> If we implement something similar to what Bartosz Golaszewski suggested
> here:
> https://lpc.events/event/17/contributions/1627/
There is also "revocable" from Tzung-Bi Shih.
>
> aka decouple the struct phy given to the consumer from the struct phy
> provided by the provider, and perform a PHY lookup during each
> phy_init() / phy_power_up() / etc etc operation;
>
> we are kinda able to:
> - match the consumer phy to the provider phy and forward the call to
> phy_ops, if the provider is present (or rebound)
> - return -ENODEV instead of calling into phy_ops, if the provider is not
> present
>
> But there's still one big gap.
>
> PHY consumers are driving the PHY through a state machine when they do
> phy_init -> phy_power_up() -> phy_set_mode() -> [ phy_configure()...].
> When the provider unbinds and then rebinds, that state is lost. But the
> consumer has no idea. It knows it is in a state where it called
> phy_power_up(), and next thing it knows, the power_count is 1 and it
> must call phy_power_down().
>
> The Generic PHY core cannot actually replay the entire state in a
> meaningful way so as to make the provider reprobing completely
> transparent (think of phy_calibrate() calls).
>
> So I guess we're looking at what Bartosz refers to as a notification
> side-channel that the PHY provider is going away.
>
> At least, for drivers that care in a meaningful way. For drivers that
> don't care about such complexity, there seems to be a simpler answer:
> device_link_add(DL_FLAG_AUTOREMOVE_CONSUMER).
Does DL_FLAG_AUTOREMOVE_CONSUMER result in forceful unbinding of the
consumer when provider goes away? And if it happens does it happen in
the right order?
>
> I can also answer why device links with "autoremove consumer" are not a
> universal answer. This is because the consumer itself may be a
> multi-port network switch (single device). If you unbind the Generic PHY
> driver (retimer, SerDes PHY) from port 3, you don't want ports 0, 1 and
> 2 to also disappear from the kernel. You need the more complex PHY
> provider disappearance notification which does something more localized
> (calls phylink_stop(), I don't know).
Aren't the ports represented as individual devices with their own state
and lifetime?
>
>
> I can say right off the bat that this is too complicated for me to even
> think about in more detail than that, at the moment. I would be quite
> happy if it's possible to unbind the PHY driver without the possibility
> to rebind it, as a first step.
Yes, this is something that is not phy specific and I think should be
solved at driver core (or at least driver core should provide subsystems
tools to solve this).
Thanks.
--
Dmitry
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH V5 0/3] Add UFS support for x1e80100 SoC
From: Martin K. Petersen @ 2026-02-24 19:40 UTC (permalink / raw)
To: Pradeep P V K
Cc: vkoul, neil.armstrong, robh, krzk+dt, conor+dt, martin.petersen,
andersson, konradybcio, taniya.das, dmitry.baryshkov,
manivannan.sadhasivam, linux-arm-msm, linux-phy, devicetree,
linux-kernel, linux-scsi, nitin.rawat
In-Reply-To: <20260211132926.3716716-1-pradeep.pragallapati@oss.qualcomm.com>
Pradeep,
> Add UFSPHY, UFSHC compatible binding names and UFS devicetree
> enablement changes for Qualcomm x1e80100 SoC.
Applied to 7.1/scsi-staging. Please validate conflict resolution in
qcom,sc7180-ufshc.yaml, thanks!
--
Martin K. Petersen
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v2 2/6] scsi: ufs: qcom,sc7180-ufshc: dt-bindings: Document the Milos UFS Controller
From: Martin K. Petersen @ 2026-02-24 19:19 UTC (permalink / raw)
To: Luca Weiss
Cc: Martin K. Petersen, Herbert Xu, David S. Miller, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Bjorn Andersson, Alim Akhtar,
Avri Altman, Bart Van Assche, Vinod Koul, Neil Armstrong,
Konrad Dybcio, ~postmarketos/upstreaming, phone-devel,
linux-arm-msm, linux-crypto, devicetree, linux-kernel, linux-scsi,
linux-phy, Krzysztof Kozlowski
In-Reply-To: <DGDW69W84LJ1.2GHM2WU31VANR@fairphone.com>
Luca,
> I've added you to this email now since you seem to pick up most patches
> for these files. Could you take this one please to unblock Milos UFS
> dts?
Applied #2, #5, and #6 to 7.1/scsi-staging, thanks!
--
Martin K. Petersen
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v1 1/4] phy: phy-can-transceiver: Convert to use device property API
From: Vladimir Oltean @ 2026-02-24 18:30 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-can, linux-phy, linux-kernel, Marc Kleine-Budde,
Vincent Mailhol, Vinod Koul, Neil Armstrong, Josua Mayer
In-Reply-To: <aZ3X2J1rBq1pMkae@smile.fi.intel.com>
On Tue, Feb 24, 2026 at 06:54:48PM +0200, Andy Shevchenko wrote:
> On Tue, Feb 24, 2026 at 06:26:06PM +0200, Vladimir Oltean wrote:
> > On Thu, Feb 19, 2026 at 09:26:19PM +0100, Andy Shevchenko wrote:
>
> ...
>
> > > - if (!of_property_present(dev->of_node, "mux-states"))
> > > + if (!device_property_present(dev, "mux-states"))
> >
> > There's an entire saga with this function - devm_mux_state_get_optional().
> > Josua Mayer is preparing to move it to the MUX core, which will be a cross-tree series.
> > Would you mind not touching this, to avoid complicating what is already
> > a complicated operation? It is going away anyway, and from what I can
> > see in Josua's last series, its implementation from drivers/mux/core.c
> > is already using device property APIs:
> > https://lore.kernel.org/linux-phy/20260208-rz-sdio-mux-v9-2-9a3be13c1280@solid-run.com/
>
> Basically you ask me to postpone the series until that will be in. Since this
> file is a mess in terms of OF/fwnode API use in exchange I would like whoever
> is doing the other part to speed up a bit if possible.
>
> I prefer to see cleaner solution to be applied sooner and last in a long distance,
> that's why I see either mine first but soon, or that first but also soon should
> be in. Can we try to achieve that?
The idea is that Ulf already expressed the availability to take the phy-can-transceiver
patch through the mmc tree and provide back a tag to be pulled into linux-phy:
https://lore.kernel.org/linux-phy/CAPDyKFrtTaJ5fqqbGrE_K6SAdTZYUfp-BycGjtWs4SabwBysKA@mail.gmail.com/
If linux-phy takes your patch first, there will be a conflict when pulling the
stable branch, and it won't be so fun, plus we can't even build-test Josua's
submission on linux-phy, so that's obviously not great.
So yeah, I'm not requesting you to postpone the entire series, just not
touch devm_mux_state_get_optional() and don't let it appear in your
patch context.
Somebody will have to remove "#include <linux/of.h>" at the end of the
whole process, but that's minor.
> ...
>
> > > - phy = devm_phy_create(dev, dev->of_node, &can_transceiver_phy_ops);
> > > + phy = devm_phy_create(dev, NULL, &can_transceiver_phy_ops);
> >
> > It is not obvious why you replaced dev->of_node with NULL here.
> > It doesn't appear correct. You seem to be breaking OF-based PHY lookups.
>
> It's the default. Yeah, I probably have to explain this in the commit message.
Ah, ok. Found the "phy->dev.of_node = node ?: dev->of_node;" assignment.
Sorry and noted, but please add it to the commit message too.
> Basically all devm_phy_create(dev, dev->of_node, ...) for clarity should be
> converted to that approach. Or even better, a new (agnostic) API should take
> default fwnode from the same device.
>
> phy = devm_phy_create_simple(dev, &..._phy_ops);
>
> // name was quickly chosen and may be not the best we can come up with
I agree in principle. PHY drivers shouldn't be given a function where
they routinely have to set one of the arguments to NULL, but a simpler
function without that argument.
But the phy-core.c doesn't support fwnode at all yet, it uses OF
throughout. I think it would be preferable to leave this change to
somebody who has business in that area.
(are you interested in PHYs with a fwnode for any particular reason, or
just because the API is more "generic" just in case?)
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v6 3/8] phy: Add driver for EyeQ5 Ethernet PHY wrapper
From: Théo Lebrun @ 2026-02-24 17:20 UTC (permalink / raw)
To: Vladimir Oltean, Théo Lebrun
Cc: Vladimir Kondratiev, Grégory Clement, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Vinod Koul,
Kishon Vijay Abraham I, Michael Turquette, Stephen Boyd,
Philipp Zabel, Thomas Bogendoerfer, Neil Armstrong, linux-mips,
devicetree, linux-kernel, linux-phy, linux-clk, Benoît Monin,
Tawfik Bayouk, Thomas Petazzoni, Luca Ceresoli
In-Reply-To: <20260210193516.temrg46yozxma7xb@skbuf>
Hello Vladimir,
On Tue Feb 10, 2026 at 8:35 PM CET, Vladimir Oltean wrote:
> On Tue, Jan 27, 2026 at 06:09:31PM +0100, Théo Lebrun wrote:
>> +static int eq5_phy_init(struct phy *phy)
>> +{
>> + struct eq5_phy_inst *inst = phy_get_drvdata(phy);
>> + struct eq5_phy_private *priv = inst->priv;
>> + struct device *dev = priv->dev;
>> + u32 reg;
>> +
>> + dev_dbg(dev, "phy_init(inst=%td)\n", inst - priv->phys);
>
> Nitpick: can you please remove the debugging prints and maybe add some
> trace points to the PHY core if you feel strongly about having some
> introspection?
Ack!
>> +
>> + writel(0, inst->gp);
>> + writel(0, inst->sgmii);
>> +
>> + udelay(5);
>
> Could you please add a macro or comment hinting at the origin of the
> magic number 5 here? You could also place these 3 lines in a common
> helper, also called from eq5_phy_exit(), to avoid minor code
> duplication.
ACK, something named `eq5_phy_reinit()`.
I don't have precise explanation for the 5µs value; I only know it is
time to let the PHY settle before further register config writes.
Is this enough?
udelay(5); /* settling time */
>> +
>> + reg = readl(inst->gp) | EQ5_GP_TX_SWRST_DIS | EQ5_GP_TX_M_CLKE |
>
> When you write 0 to inst->gp and then read it back, do you expect to
> (a) get back 0 or
> (b) are some fields non-resetting?
>
> I see both as inconsistent, since if (a), you can remove the
> readl(inst->gp) and expect the same result. And if (b), it also
> shouldn't matter if you write zeroes a second time, if it was fine the
> first time?
>
> Shortly said, is readl(inst->gp) really needed?
Some fields are non-resetting (BIT 30).
Will drop. I was trying to play it safe for no good reason.
>
>> + EQ5_GP_SYS_SWRST_DIS | EQ5_GP_SYS_M_CLKE |
>> + FIELD_PREP(EQ5_GP_RGMII_DRV, 0x9);
>
> Quick sanity check on your proposal to use #phy-cells = <1>. This is not
> a request to change anything.
>
> What if you need to customize the RGMII drive strength (or some other
> setting, maybe SGMII polarity if that is available) per lane, for a
> particular board? How would you do that if each PHY does not have its
> own OF node?
I have no knowledge of what that 0x9 stands for, I didn't see the point
exposing it to devicetree. We could plan for the future and add a cell
or create subnodes, but here I kept it simple stupid. Is it OK?
>> + writel(reg, inst->gp);
>> +
>> + return 0;
>> +}
>> +
>> +static int eq5_phy_exit(struct phy *phy)
>> +{
>> + struct eq5_phy_inst *inst = phy_get_drvdata(phy);
>> + struct eq5_phy_private *priv = inst->priv;
>> + struct device *dev = priv->dev;
>> +
>> + dev_dbg(dev, "phy_exit(inst=%td)\n", inst - priv->phys);
>> +
>> + writel(0, inst->gp);
>> + writel(0, inst->sgmii);
>> + udelay(5);
>> +
>> + return 0;
>> +}
>> +
>> +static int eq5_phy_set_mode(struct phy *phy, enum phy_mode mode, int submode)
>> +{
>> + struct eq5_phy_inst *inst = phy_get_drvdata(phy);
>> + struct eq5_phy_private *priv = inst->priv;
>> + struct device *dev = priv->dev;
>> +
>> + dev_dbg(dev, "phy_set_mode(inst=%td, mode=%d, submode=%d)\n",
>> + inst - priv->phys, mode, submode);
>> +
>> + if (mode != PHY_MODE_ETHERNET)
>> + return -EOPNOTSUPP;
>> +
>> + if (!phy_interface_mode_is_rgmii(submode) &&
>> + submode != PHY_INTERFACE_MODE_SGMII)
>> + return -EOPNOTSUPP;
>
> Both PHYs are equal in capabilities, and support both RGMII and SGMII,
> correct? I see the driver is implemented as if they were, but it doesn't
> hurt to ask.
Datasheet indicates 0 can do SGMII/RGMII and 1 can do only RGMII.
Did you imply that the driver code should reject SGMII on PHY 1
if it ever gets asked for?
>> +
>> + inst->phy_interface = submode;
>
> Short story: don't rely on the phy_set_mode_ext() -> phy_power_on() order.
> Implement the driver so that it works the other way around too.
>
> Long story:
> https://lore.kernel.org/netdev/aXzFH09AeIRawCwU@shell.armlinux.org.uk/
I wouldn't mind, but what should phy_power_on() do if no submode has
been provided through phy_set_mode_ext() yet? Guess one? Fail?
Also our PHY will need to be reset to change its mode if we do
power_on() followed by set_mode(), which in practice is never something
we want. Maybe there is a flag to indicate that we require a submode to
power on?
Thanks for the extensive review Vladimir,
--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v1 1/4] phy: phy-can-transceiver: Convert to use device property API
From: Andy Shevchenko @ 2026-02-24 16:54 UTC (permalink / raw)
To: Vladimir Oltean
Cc: linux-can, linux-phy, linux-kernel, Marc Kleine-Budde,
Vincent Mailhol, Vinod Koul, Neil Armstrong, Josua Mayer
In-Reply-To: <20260224162606.spnzzedvmvp2h7xd@skbuf>
On Tue, Feb 24, 2026 at 06:26:06PM +0200, Vladimir Oltean wrote:
> On Thu, Feb 19, 2026 at 09:26:19PM +0100, Andy Shevchenko wrote:
...
> > - if (!of_property_present(dev->of_node, "mux-states"))
> > + if (!device_property_present(dev, "mux-states"))
>
> There's an entire saga with this function - devm_mux_state_get_optional().
> Josua Mayer is preparing to move it to the MUX core, which will be a cross-tree series.
> Would you mind not touching this, to avoid complicating what is already
> a complicated operation? It is going away anyway, and from what I can
> see in Josua's last series, its implementation from drivers/mux/core.c
> is already using device property APIs:
> https://lore.kernel.org/linux-phy/20260208-rz-sdio-mux-v9-2-9a3be13c1280@solid-run.com/
Basically you ask me to postpone the series until that will be in. Since this
file is a mess in terms of OF/fwnode API use in exchange I would like whoever
is doing the other part to speed up a bit if possible.
I prefer to see cleaner solution to be applied sooner and last in a long distance,
that's why I see either mine first but soon, or that first but also soon should
be in. Can we try to achieve that?
...
> > - phy = devm_phy_create(dev, dev->of_node, &can_transceiver_phy_ops);
> > + phy = devm_phy_create(dev, NULL, &can_transceiver_phy_ops);
>
> It is not obvious why you replaced dev->of_node with NULL here.
> It doesn't appear correct. You seem to be breaking OF-based PHY lookups.
It's the default. Yeah, I probably have to explain this in the commit message.
Basically all devm_phy_create(dev, dev->of_node, ...) for clarity should be
converted to that approach. Or even better, a new (agnostic) API should take
default fwnode from the same device.
phy = devm_phy_create_simple(dev, &..._phy_ops);
// name was quickly chosen and may be not the best we can come up with
...
Thanks for the review!
--
With Best Regards,
Andy Shevchenko
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v1 1/4] phy: phy-can-transceiver: Convert to use device property API
From: Vladimir Oltean @ 2026-02-24 16:26 UTC (permalink / raw)
To: Andy Shevchenko
Cc: linux-can, linux-phy, linux-kernel, Marc Kleine-Budde,
Vincent Mailhol, Vinod Koul, Neil Armstrong, Josua Mayer
In-Reply-To: <20260219202910.2304440-2-andriy.shevchenko@linux.intel.com>
Hi Andy,
On Thu, Feb 19, 2026 at 09:26:19PM +0100, Andy Shevchenko wrote:
> It seems the driver is half-moved to use device property APIs.
> Finish that by converting everything to use that.
>
> Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> ---
> drivers/phy/phy-can-transceiver.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/phy/phy-can-transceiver.c b/drivers/phy/phy-can-transceiver.c
> index 330356706ad7..f2259af4af8a 100644
> --- a/drivers/phy/phy-can-transceiver.c
> +++ b/drivers/phy/phy-can-transceiver.c
> @@ -5,9 +5,9 @@
> * Copyright (C) 2021 Texas Instruments Incorporated - https://www.ti.com
> *
> */
> -#include <linux/of.h>
> #include <linux/phy/phy.h>
> #include <linux/platform_device.h>
> +#include <linux/property.h>
> #include <linux/module.h>
> #include <linux/gpio.h>
> #include <linux/gpio/consumer.h>
> @@ -130,7 +130,7 @@ MODULE_DEVICE_TABLE(of, can_transceiver_phy_ids);
> static inline struct mux_state *
> devm_mux_state_get_optional(struct device *dev, const char *mux_name)
> {
> - if (!of_property_present(dev->of_node, "mux-states"))
> + if (!device_property_present(dev, "mux-states"))
There's an entire saga with this function - devm_mux_state_get_optional().
Josua Mayer is preparing to move it to the MUX core, which will be a cross-tree series.
Would you mind not touching this, to avoid complicating what is already
a complicated operation? It is going away anyway, and from what I can
see in Josua's last series, its implementation from drivers/mux/core.c
is already using device property APIs:
https://lore.kernel.org/linux-phy/20260208-rz-sdio-mux-v9-2-9a3be13c1280@solid-run.com/
> return NULL;
>
> return devm_mux_state_get(dev, mux_name);
> @@ -162,7 +162,6 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
> struct can_transceiver_phy *can_transceiver_phy;
> struct can_transceiver_priv *priv;
> const struct can_transceiver_data *drvdata;
> - const struct of_device_id *match;
> struct phy *phy;
> struct gpio_desc *silent_gpio;
> struct gpio_desc *standby_gpio;
> @@ -171,8 +170,7 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
> u32 max_bitrate = 0;
> int err, i, num_ch = 1;
>
> - match = of_match_node(can_transceiver_phy_ids, pdev->dev.of_node);
> - drvdata = match->data;
> + drvdata = device_get_match_data(dev);
> if (drvdata->flags & CAN_TRANSCEIVER_DUAL_CH)
> num_ch = 2;
>
> @@ -197,7 +195,7 @@ static int can_transceiver_phy_probe(struct platform_device *pdev)
> can_transceiver_phy = &priv->can_transceiver_phy[i];
> can_transceiver_phy->priv = priv;
>
> - phy = devm_phy_create(dev, dev->of_node, &can_transceiver_phy_ops);
> + phy = devm_phy_create(dev, NULL, &can_transceiver_phy_ops);
It is not obvious why you replaced dev->of_node with NULL here.
It doesn't appear correct. You seem to be breaking OF-based PHY lookups.
> if (IS_ERR(phy)) {
> dev_err(dev, "failed to create can transceiver phy\n");
> return PTR_ERR(phy);
> --
> 2.50.1
>
>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH] phy: core: fix potential UAF in of_phy_simple_xlate()
From: Vladimir Oltean @ 2026-02-24 15:47 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Vinod Koul, Kishon Vijay Abraham I, Neil Armstrong,
Rafael J. Wysocki, Geert Uytterhoeven, Johan Hovold,
Claudiu Beznea, Dr. David Alan Gilbert, Peter Griffin,
Dmitry Baryshkov, Krzysztof Kozlowski, Zijun Hu, linux-phy,
linux-kernel
In-Reply-To: <aZzmU_oA6mVMsdQT@google.com>
On Mon, Feb 23, 2026 at 04:37:39PM -0800, Dmitry Torokhov wrote:
> > You have hit on a weak spot of the Generic PHY framework and I would
> > like to encourage you to follow through with patches for this finding
> > (although it won't be exactly trivial, I think it's doable with some
> > determination).
>
> Thank you for the detailed response. I am not sure how much time I can
> spend on phy rework as this change was an essentially a drive-by. I only
> happen to look there because I want to remove
> class_find_device_by_of_node() in favor of class_find_device_by_fwnode().
OK, I believe you can still do that without any dependency.
> > On your direct question:
> > It will impact downstream in subtle and unpleasant ways if you change
> > the of_xlate() semantics w.r.t. reference counting, but the code will
> > still compile just as before, i.e. when looking just at your downstream
> > driver (what 99% of developers do) there will be no obvious way of
> > knowing that something in the API has changed. I would avoid doing that.
>
> Hmm, I was not aware that we needed to take particular measures for
> benefit of downstream trees...
I think you're entirely missing the point.
Obviously it benefits the maintainer/reviewer first and foremost. If you
don't make the API change unmissable, drivers expecting the old convention
will eventually creep their way into the mainline kernel. Why put avoidable
pressure on reviewers watching out for things like this for years to come,
when you can force downstream to notice and adapt (OR not make the
change in the first place). Note that downstream means "API consumers
invisible to you, the API changer", not only "hopelessly un-upstreamable
drivers".
> > The patch above leaves a few loose ends.
> >
> > The most obvious is of_phy_simple_xlate(), which has that put_device()
> > to balance out class_find_device_by_of_node() - which bumps the device
> > refcount to 2. It "looks" wrong but it is consistent with vendor
> > implementations of of_xlate(), which also provide a phy->dev refcount of 1.
> > And as explained, the refcount never _actually_ drops to 0 with the
> > above patch.
> >
> > I would actually address _only_ of_phy_simple_xlate(), for cosmetics sake.
> > Instead of devm_of_phy_provider_register(..., of_phy_simple_xlate), I
> > would offer a new helper, devm_of_phy_simple_provider_register(), which
> > would internally use a callback that doesn't drop the refcount (say
> > __of_phy_simple_xlate() for lack of imagination). I would convert vendor
> > PHY drivers one by one to use this (it would be valid at any time to use
> > either the old or the new method).
> >
> > You'd notice that most of the of_phy_simple_xlate() occurrences go away,
> > except for freescale/phy-fsl-lynx-28g.c which calls this function
> > directly from its own lynx_28g_xlate(). It will still have to keep
> > calling it, and for refcount equalization purposes that weird
> > put_device() will have to continue to exist. Just leave a comment as to
> > why that is.
>
> I this case I would simply add a comment telling why the reference
> should (and can) be dropped where it is being dropped in
> of_phy_simple_xlate() and call it a day. There is not much benefit in
> adding another helper.
OK.
> > But there are more important loose ends still.
> >
> > I mentioned that "zombie" device. We've solved the memory safety issue,
> > but it's possible for consumers to hold onto a phy whose provider has
> > disappeared. The refcount of &phy->dev hasn't dropped to 0, so
> > technically it's a valid object, but from PHY API perspective, it's
> > still possible to call phy_init(), phy_power_on() and friends on this
> > PHY, and the Generic PHY core will be happy to further call into the
> > phy->ops->init(), phy->ops->power_on() etc. But the driver has unbound,
> > so it should really be left alone.
> >
> > If we fix the UAF but leave the zombie PHY problem, we've effectively
> > done nothing but silence static analysis checkers, while the code path
> > where the PHY provider unbinds effectively remains treated as poorly as
> > before, just moving the crashes to a different place.
> >
> > I suspect what needs to be done here is to introduce a "bool dead" or
> > similar, which is to be set from phy_destroy() and checked from every
> > API call. The idea is that API functions on zombie PHYs should fail
> > without calling into their driver. I **suppose** that
> > try_module_get(phy->ops->owner) "tried" to avoid this situation, but
> > it just protects against module removal, not against "echo device >
> > /sys/bus/.../unbind". So it's absolutely incomplete and easily bypassable.
>
> I think this is a problem common to many kernel subsystems, where there
> are devices that are vital for other devices to function, but are not
> parents of said devices. Think about regulators or clocks and such.
> In many such cases even validating at API level is not sufficient,
> because if you enable a clock you typically keep it running at least for
> a while, if not for entire duration of device being bound to a driver.
> If that clock goes away the device will break even though you are not
> calling any clock APIs at that particular time.
>
> We need some kind of revocation mechanism to signal consumers that
> providers they rely upon are going away.
Yeah, this gave me pause.
When you unbind a Generic PHY you can kind of expect that the data path
it affects to not work. But you'd sort of expect it to start working
again when you rebind the PHY driver.
That will not be the case, though.
The problem, simply put, is that the struct phy that the consumer has
will be different from the second struct phy that the provider registers
when it rebinds. Using the stale struct phy, we cannot reach the phy_ops
of the new provider.
If we implement something similar to what Bartosz Golaszewski suggested
here:
https://lpc.events/event/17/contributions/1627/
aka decouple the struct phy given to the consumer from the struct phy
provided by the provider, and perform a PHY lookup during each
phy_init() / phy_power_up() / etc etc operation;
we are kinda able to:
- match the consumer phy to the provider phy and forward the call to
phy_ops, if the provider is present (or rebound)
- return -ENODEV instead of calling into phy_ops, if the provider is not
present
But there's still one big gap.
PHY consumers are driving the PHY through a state machine when they do
phy_init -> phy_power_up() -> phy_set_mode() -> [ phy_configure()...].
When the provider unbinds and then rebinds, that state is lost. But the
consumer has no idea. It knows it is in a state where it called
phy_power_up(), and next thing it knows, the power_count is 1 and it
must call phy_power_down().
The Generic PHY core cannot actually replay the entire state in a
meaningful way so as to make the provider reprobing completely
transparent (think of phy_calibrate() calls).
So I guess we're looking at what Bartosz refers to as a notification
side-channel that the PHY provider is going away.
At least, for drivers that care in a meaningful way. For drivers that
don't care about such complexity, there seems to be a simpler answer:
device_link_add(DL_FLAG_AUTOREMOVE_CONSUMER).
I can also answer why device links with "autoremove consumer" are not a
universal answer. This is because the consumer itself may be a
multi-port network switch (single device). If you unbind the Generic PHY
driver (retimer, SerDes PHY) from port 3, you don't want ports 0, 1 and
2 to also disappear from the kernel. You need the more complex PHY
provider disappearance notification which does something more localized
(calls phylink_stop(), I don't know).
I can say right off the bat that this is too complicated for me to even
think about in more detail than that, at the moment. I would be quite
happy if it's possible to unbind the PHY driver without the possibility
to rebind it, as a first step.
> > Finally, I have identified one more loose end still.
> >
> > /**
> > * of_phy_put() - release the PHY
> > * @phy: the phy returned by of_phy_get()
> > *
> > * Releases a refcount the caller received from of_phy_get().
> > */
> > void of_phy_put(struct phy *phy)
> > {
> > if (!phy || IS_ERR(phy))
> > return;
> >
> > mutex_lock(&phy->mutex);
> > if (phy->ops->release)
> > phy->ops->release(phy);
> > mutex_unlock(&phy->mutex);
> >
> > module_put(phy->ops->owner);
> > put_device(&phy->dev);
> > }
> > EXPORT_SYMBOL_GPL(of_phy_put);
> >
> > This function is called by PHY consumers. A PHY provider can have
> > multiple consumers (example in the case of Ethernet: a QSGMII SerDes
> > lane has 4 MAC ports multiplexed onto it).
> >
> > If a single consumer calls of_phy_put() to release its reference to the
> > SerDes lane, the phy->ops->release() method makes absolutely no sense.
> > There are 3 remaining consumers with handles to the lane! But we aren't
> > even telling the PHY which consumer has disappeared! It has nothing
> > useful to do with this information.
> >
> > Looking at actual phy_ops implementations, what they want to know is
> > when _all_ consumers went away, not when individual consumers did.
> > So the phy->ops->release() call needs to be put somewhere which is
> > executed when all consumers disappear. If I were to guess, that would be
> > the phy_release() class callback, but this is completely untested.
>
> Shouldn't phy->ops->release() keep track of users and decide when to
> destroy the resources? The rest seem fine as they simply drop references
> (I assume each user of shared phy will bump up references as needed?)
AFAICS, the only instance is phy/ti/phy-am654-serdes.c: serdes_am654_release().
This undoes the effect of serdes_am654_xlate(), i.e. calls mux_control_deselect(phy->control).
The fact that phy_provider->of_xlate() has side effects on this
platform, rather than just return a struct phy * for a given struct
device_node * (as it's supposed to do), is "interesting".
I don't have extra info why the PHY maintainer didn't add extra phy_ops
for consumer_bind() and consumer_unbind() or something like that, to
make it clear that these calls are supposed to be _per consumer_.
Notice how the first of_xlate() call sets am654_phy->busy = true, and
second of_xlate() call is designed to fail due to am654_phy->busy.
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v7 10/23] scsi: ufs: mediatek: Handle misc host voltage regulators
From: Mark Brown @ 2026-02-24 12:48 UTC (permalink / raw)
To: Peter Wang (王信友)
Cc: chu.stanley@gmail.com, robh@kernel.org,
Chunfeng Yun (云春峰), kishon@kernel.org,
James.Bottomley@HansenPartnership.com, bvanassche@acm.org,
AngeloGioacchino Del Regno, neil.armstrong@linaro.org,
conor+dt@kernel.org, Chaotian Jing (井朝天),
lgirdwood@gmail.com, nicolas.frattaroli@collabora.com,
vkoul@kernel.org, krzk+dt@kernel.org, p.zabel@pengutronix.de,
alim.akhtar@samsung.com, matthias.bgg@gmail.com,
avri.altman@wdc.com, martin.petersen@oracle.com,
linux-scsi@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-phy@lists.infradead.org, linux-mediatek@lists.infradead.org,
Louis-Alexis Eyraud, kernel@collabora.com
In-Reply-To: <2c7c84a2df19624ba9f207fd3fee47a8bdd9d8dc.camel@mediatek.com>
[-- Attachment #1.1: Type: text/plain, Size: 711 bytes --]
On Tue, Feb 24, 2026 at 12:38:50PM +0000, Peter Wang (王信友) wrote:
> On Mon, 2026-02-16 at 14:37 +0100, Nicolas Frattaroli wrote:
> > + if (data->num_reg_names) {
> > + ret = devm_regulator_bulk_get_enable(dev, data-
> > >num_reg_names,
> > + data-
> > >reg_names);
> If these regulators are only acquired and enabled once,
> why not just set regulator-always-on in the device tree?
Drivers should request and enable any regulators they require, they
should not rely on boards happening to enable a supply for them.
Similarly the board should only impose constraints that come from the
system design, it should not assume that drivers will continue to behave
as they do.
[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
[-- Attachment #2: Type: text/plain, Size: 112 bytes --]
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v7 16/23] scsi: ufs: mediatek: Clean up logging prints
From: Peter Wang (王信友) @ 2026-02-24 12:47 UTC (permalink / raw)
To: chu.stanley@gmail.com, robh@kernel.org,
Chunfeng Yun (云春峰), kishon@kernel.org,
James.Bottomley@HansenPartnership.com, bvanassche@acm.org,
AngeloGioacchino Del Regno, neil.armstrong@linaro.org,
conor+dt@kernel.org, Chaotian Jing (井朝天),
lgirdwood@gmail.com, nicolas.frattaroli@collabora.com,
vkoul@kernel.org, krzk+dt@kernel.org, p.zabel@pengutronix.de,
alim.akhtar@samsung.com, matthias.bgg@gmail.com,
avri.altman@wdc.com, martin.petersen@oracle.com,
broonie@kernel.org
Cc: linux-scsi@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-phy@lists.infradead.org, linux-mediatek@lists.infradead.org,
Louis-Alexis Eyraud, kernel@collabora.com
In-Reply-To: <20260216-mt8196-ufs-v7-16-b5f2907c6da7@collabora.com>
On Mon, 2026-02-16 at 14:37 +0100, Nicolas Frattaroli wrote:
> drivers/ufs/host/ufs-mediatek.c | 99 ++++++++++++++++++-------------
> ----------
> 1 file changed, 43 insertions(+), 56 deletions(-)
>
> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-
> mediatek.c
> index ecf16e82a326..2b1f26b55782 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -192,8 +192,8 @@ static void ufs_mtk_crypto_enable(struct ufs_hba
> *hba)
>
> ufs_mtk_crypto_ctrl(res, 1);
> if (res.a0) {
> - dev_info(hba->dev, "%s: crypto enable failed, err:
> %lu\n",
> - __func__, res.a0);
> + dev_err(hba->dev, "%s: crypto enable failed with
> error %lu, disabling\n",
> + __func__, res.a0);
> hba->caps &= ~UFSHCD_CAP_CRYPTO;
> }
> }
> @@ -542,40 +542,38 @@ static void ufs_mtk_boost_crypt(struct ufs_hba
> *hba, bool boost)
>
> ret = clk_prepare_enable(cfg->clk_crypt_mux);
> if (ret) {
> - dev_info(hba->dev, "clk_prepare_enable(): %d\n",
> - ret);
> + dev_err(hba->dev, "%s: Failed to enable
> clk_crypt_mux: %pe\n",
> + __func__, ERR_PTR(ret));
> return;
> }
>
> if (boost) {
> ret = regulator_set_voltage(reg, volt, INT_MAX);
> if (ret) {
> - dev_info(hba->dev,
> - "failed to set vcore to %d\n",
> volt);
> + dev_err(hba->dev, "%s: Failed to set vcore
> to %d: %pe\n",
> + __func__, volt, ERR_PTR(ret));
> goto out;
> }
>
> - ret = clk_set_parent(cfg->clk_crypt_mux,
> - cfg->clk_crypt_perf);
> + ret = clk_set_parent(cfg->clk_crypt_mux, cfg-
> >clk_crypt_perf);
> if (ret) {
> - dev_info(hba->dev,
> - "failed to set clk_crypt_perf\n");
> + dev_err(hba->dev, "%s: Failed to reparent
> clk_crypt_perf: %pe\n",
> + __func__, ERR_PTR(ret));
> regulator_set_voltage(reg, 0, INT_MAX);
> goto out;
> }
> } else {
> - ret = clk_set_parent(cfg->clk_crypt_mux,
> - cfg->clk_crypt_lp);
> + ret = clk_set_parent(cfg->clk_crypt_mux, cfg-
> >clk_crypt_lp);
> if (ret) {
> - dev_info(hba->dev,
> - "failed to set clk_crypt_lp\n");
> + dev_err(hba->dev, "%s: Failed to reparent
> clk_crypt_lp: %pe\n",
> + __func__, ERR_PTR(ret));
> goto out;
> }
>
> ret = regulator_set_voltage(reg, 0, INT_MAX);
> if (ret) {
> - dev_info(hba->dev,
> - "failed to set vcore to MIN\n");
> + dev_err(hba->dev, "%s: Failed to set vcore
> to minimum: %pe\n",
> + __func__, ERR_PTR(ret));
> }
> }
> out:
> @@ -763,10 +761,8 @@ static int ufs_mtk_setup_clocks(struct ufs_hba
> *hba, bool on,
> if (clk_pwr_off) {
> ufs_mtk_pwr_ctrl(hba, false);
> } else {
> - dev_warn(hba->dev, "Clock is not turned off,
> hba->ahit = 0x%x, AHIT = 0x%x\n",
> - hba->ahit,
> - ufshcd_readl(hba,
> -
> REG_AUTO_HIBERNATE_IDLE_TIMER));
> + dev_warn(hba->dev, "Clock isn't off, hba-
> >ahit = 0x%x, AHIT = 0x%x\n",
> + hba->ahit, ufshcd_readl(hba,
> REG_AUTO_HIBERNATE_IDLE_TIMER));
> }
> ufs_mtk_mcq_disable_irq(hba);
> } else if (on && status == POST_CHANGE) {
> @@ -810,11 +806,11 @@ static void ufs_mtk_mcq_set_irq_affinity(struct
> ufs_hba *hba, unsigned int cpu)
> _cpu = (cpu == 0) ? 3 : cpu;
> ret = irq_set_affinity(irq, cpumask_of(_cpu));
> if (ret) {
> - dev_err(hba->dev, "set irq %d affinity to CPU %d
> failed\n",
> + dev_err(hba->dev, "setting irq %d affinity to CPU %d
> failed\n",
> irq, _cpu);
> return;
> }
> - dev_info(hba->dev, "set irq %d affinity to CPU: %d\n", irq,
> _cpu);
> + dev_dbg(hba->dev, "set irq %d affinity to CPU %d\n", irq,
> _cpu);
>
Is it more appropriate to use dev_info for state changes or for setting
changes?
> }
>
> static bool ufs_mtk_is_legacy_chipset(struct ufs_hba *hba, u32
> hw_ip_ver)
> @@ -830,7 +826,8 @@ static bool ufs_mtk_is_legacy_chipset(struct
> ufs_hba *hba, u32 hw_ip_ver)
> default:
> break;
> }
> - dev_info(hba->dev, "legacy IP version - 0x%x, is legacy :
> %d", hw_ip_ver, is_legacy);
> + dev_dbg(hba->dev, "IP version 0x%x, legacy = %s", hw_ip_ver,
> + str_true_false(is_legacy));
>
> return is_legacy;
> }
> @@ -935,15 +932,12 @@ static void ufs_mtk_init_clocks(struct ufs_hba
> *hba)
> }
> }
>
> - list_for_each_entry(clki, head, list) {
> - dev_info(hba->dev, "clk \"%s\" present", clki-
> >name);
> - }
> + list_for_each_entry(clki, head, list)
> + dev_dbg(hba->dev, "clk \"%s\" present", clki->name);
>
> if (!ufs_mtk_is_clk_scale_ready(hba)) {
> hba->caps &= ~UFSHCD_CAP_CLK_SCALING;
> - dev_info(hba->dev,
> - "%s: Clk-scaling not ready. Feature
> disabled.",
> - __func__);
> + dev_info(hba->dev, "%s: Clock scaling unavailable",
> __func__);
> return;
> }
>
> @@ -953,8 +947,8 @@ static void ufs_mtk_init_clocks(struct ufs_hba
> *hba)
> */
> reg = devm_regulator_get_optional(dev, "dvfsrc-vcore");
> if (IS_ERR(reg)) {
> - dev_info(dev, "failed to get dvfsrc-vcore: %ld",
> - PTR_ERR(reg));
> + if (PTR_ERR(reg) != -ENODEV)
> + dev_err(dev, "Failed to get dvfsrc-vcore:
> %pe\n", reg);
> return;
> }
>
> @@ -968,12 +962,9 @@ static void ufs_mtk_init_clocks(struct ufs_hba
> *hba)
> host->mclk.vcore_volt = volt;
>
> /* If default boot is max gear, request vcore */
> - if (reg && volt && host->clk_scale_up) {
> - if (regulator_set_voltage(reg, volt, INT_MAX)) {
> - dev_info(hba->dev,
> - "Failed to set vcore to %d\n",
> volt);
> - }
> - }
> + if (reg && volt && host->clk_scale_up)
> + if (regulator_set_voltage(reg, volt, INT_MAX))
> + dev_err(hba->dev, "Failed to set vcore to
> %d\n", volt);
> }
>
> static void ufs_mtk_setup_clk_gating(struct ufs_hba *hba)
> @@ -1060,7 +1051,7 @@ static void ufs_mtk_init_mcq_irq(struct ufs_hba
> *hba)
> }
> host->mcq_intr_info[i].hba = hba;
> host->mcq_intr_info[i].irq = irq;
> - dev_info(hba->dev, "get platform mcq irq: %d, %d\n",
> i, irq);
> + dev_dbg(hba->dev, "get platform mcq irq: %d, %d\n",
> i, irq);
> }
>
> return;
> @@ -1307,10 +1298,8 @@ static int ufs_mtk_pre_pwr_change(struct
> ufs_hba *hba,
> host_params.desired_working_mode = UFS_PWM_MODE;
>
> ret = ufshcd_negotiate_pwr_params(&host_params,
> dev_max_params, dev_req_params);
> - if (ret) {
> - pr_info("%s: failed to determine capabilities\n",
> - __func__);
> - }
> + if (ret)
> + dev_warn(hba->dev, "%s: failed to determine
> capabilities\n", __func__);
>
> if (ufs_mtk_pmc_via_fastauto(hba, dev_req_params)) {
> ufs_mtk_adjust_sync_length(hba);
> @@ -1356,10 +1345,9 @@ static int ufs_mtk_pre_pwr_change(struct
> ufs_hba *hba,
> ret = ufshcd_uic_change_pwr_mode(hba,
> FASTAUTO_MODE << 4 |
> FASTAUTO_MODE);
>
> - if (ret) {
> - dev_err(hba->dev, "%s: HSG1B FASTAUTO failed
> ret=%d\n",
> - __func__, ret);
> - }
> + if (ret)
> + dev_err(hba->dev, "%s: HSG1B FASTAUTO
> failed: %pe\n",
> + __func__, ERR_PTR(ret));
> }
>
> /* if already configured to the requested pwr_mode, skip
> adapt */
> @@ -1409,7 +1397,7 @@ static int ufs_mtk_auto_hibern8_disable(struct
> ufs_hba *hba)
>
> out:
> if (ret) {
> - dev_warn(hba->dev, "exit h8 state fail, ret=%d\n",
> ret);
> + dev_err(hba->dev, "Failed to exit h8 state: %pe\n",
> ERR_PTR(ret));
>
> ufshcd_force_error_recovery(hba);
>
> @@ -1571,7 +1559,7 @@ static int ufs_mtk_device_reset(struct ufs_hba
> *hba)
> /* Some devices may need time to respond to rst_n */
> usleep_range(10000, 15000);
>
> - dev_info(hba->dev, "device reset done\n");
> + dev_dbg(hba->dev, "device reset done\n");
>
Is it more appropriate to use dev_info for state changes or for setting
changes?
Thanks
Peter
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v7 15/23] scsi: ufs: mediatek: Rework _ufs_mtk_clk_scale error paths
From: Peter Wang (王信友) @ 2026-02-24 12:43 UTC (permalink / raw)
To: chu.stanley@gmail.com, robh@kernel.org,
Chunfeng Yun (云春峰), kishon@kernel.org,
James.Bottomley@HansenPartnership.com, bvanassche@acm.org,
AngeloGioacchino Del Regno, neil.armstrong@linaro.org,
conor+dt@kernel.org, Chaotian Jing (井朝天),
lgirdwood@gmail.com, nicolas.frattaroli@collabora.com,
vkoul@kernel.org, krzk+dt@kernel.org, p.zabel@pengutronix.de,
alim.akhtar@samsung.com, matthias.bgg@gmail.com,
avri.altman@wdc.com, martin.petersen@oracle.com,
broonie@kernel.org
Cc: linux-scsi@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-phy@lists.infradead.org, linux-mediatek@lists.infradead.org,
Louis-Alexis Eyraud, kernel@collabora.com
In-Reply-To: <20260216-mt8196-ufs-v7-15-b5f2907c6da7@collabora.com>
On Mon, 2026-02-16 at 14:37 +0100, Nicolas Frattaroli wrote:
> Errors should be printed at the correct log level. Additionally, it
> looks like some "goto out"'s were omitted in the scale up case, which
> looks like a mistake, as the scale down branch of the code does use
> them.
>
> Rework the error messages to make them nicer and at the correct
> verbosity, and add the missing gotos.
>
> Reviewed-by: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
Reviewed-by: Peter Wang <peter.wang@mediatek.com>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v7 14/23] scsi: ufs: mediatek: Remove mediatek,ufs-broken-rtc property
From: Peter Wang (王信友) @ 2026-02-24 12:43 UTC (permalink / raw)
To: chu.stanley@gmail.com, robh@kernel.org,
Chunfeng Yun (云春峰), kishon@kernel.org,
James.Bottomley@HansenPartnership.com, bvanassche@acm.org,
AngeloGioacchino Del Regno, neil.armstrong@linaro.org,
conor+dt@kernel.org, Chaotian Jing (井朝天),
lgirdwood@gmail.com, nicolas.frattaroli@collabora.com,
vkoul@kernel.org, krzk+dt@kernel.org, p.zabel@pengutronix.de,
alim.akhtar@samsung.com, matthias.bgg@gmail.com,
avri.altman@wdc.com, martin.petersen@oracle.com,
broonie@kernel.org
Cc: linux-scsi@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-phy@lists.infradead.org, linux-mediatek@lists.infradead.org,
Louis-Alexis Eyraud, kernel@collabora.com
In-Reply-To: <20260216-mt8196-ufs-v7-14-b5f2907c6da7@collabora.com>
On Mon, 2026-02-16 at 14:37 +0100, Nicolas Frattaroli wrote:
> diff --git a/drivers/ufs/host/ufs-mediatek.c b/drivers/ufs/host/ufs-
> mediatek.c
> index 230e11533eac..424533538b90 100644
> --- a/drivers/ufs/host/ufs-mediatek.c
> +++ b/drivers/ufs/host/ufs-mediatek.c
> @@ -655,9 +655,6 @@ static void ufs_mtk_init_host_caps(struct ufs_hba
> *hba)
> if (of_property_read_bool(np, "mediatek,ufs-rtff-mtcmos"))
> host->caps |= UFS_MTK_CAP_RTFF_MTCMOS;
>
> - if (of_property_read_bool(np, "mediatek,ufs-broken-rtc"))
> - host->caps |= UFS_MTK_CAP_MCQ_BROKEN_RTC;
> -
> dev_info(hba->dev, "caps: 0x%x", host->caps);
> }
>
> @@ -1185,8 +1182,6 @@ static int ufs_mtk_init(struct ufs_hba *hba)
> hba->quirks |= UFSHCI_QUIRK_SKIP_MANUAL_WB_FLUSH_CTRL;
>
> hba->quirks |= UFSHCD_QUIRK_MCQ_BROKEN_INTR;
> - if (host->caps & UFS_MTK_CAP_MCQ_BROKEN_RTC)
> - hba->quirks |= UFSHCD_QUIRK_MCQ_BROKEN_RTC;
>
Maybe check the hardware version (host->ip_ver)
and set UFSHCD_QUIRK_MCQ_BROKEN_RTC instead of removing it?
Thanks
Peter
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v7 10/23] scsi: ufs: mediatek: Handle misc host voltage regulators
From: AngeloGioacchino Del Regno @ 2026-02-24 12:41 UTC (permalink / raw)
To: Peter Wang (王信友), chu.stanley@gmail.com,
robh@kernel.org, Chunfeng Yun (云春峰),
kishon@kernel.org, James.Bottomley@HansenPartnership.com,
bvanassche@acm.org, neil.armstrong@linaro.org,
conor+dt@kernel.org, Chaotian Jing (井朝天),
lgirdwood@gmail.com, nicolas.frattaroli@collabora.com,
vkoul@kernel.org, krzk+dt@kernel.org, p.zabel@pengutronix.de,
alim.akhtar@samsung.com, matthias.bgg@gmail.com,
avri.altman@wdc.com, martin.petersen@oracle.com,
broonie@kernel.org
Cc: linux-scsi@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-phy@lists.infradead.org, linux-mediatek@lists.infradead.org,
Louis-Alexis Eyraud, kernel@collabora.com
In-Reply-To: <2c7c84a2df19624ba9f207fd3fee47a8bdd9d8dc.camel@mediatek.com>
Il 24/02/26 13:38, Peter Wang (王信友) ha scritto:
> On Mon, 2026-02-16 at 14:37 +0100, Nicolas Frattaroli wrote:
>> @@ -1188,8 +1190,21 @@ static int ufs_mtk_get_supplies(struct
>> ufs_mtk_host *host)
>> {
>> struct device *dev = host->hba->dev;
>> const struct ufs_mtk_soc_data *data =
>> of_device_get_match_data(dev);
>> + int ret;
>> +
>> + if (!data)
>> + return 0;
>> +
>> + if (data->num_reg_names) {
>> + ret = devm_regulator_bulk_get_enable(dev, data-
>>> num_reg_names,
>> + data-
>>> reg_names);
>
> Hi Nicolas,
>
> If these regulators are only acquired and enabled once,
> why not just set regulator-always-on in the device tree?
>
Because:
1. The UFS driver has to acquire the correct supplies regardless of them
being on, guaranteeing both a readable power tree (sysfs/debugfs) and
a correct hardware description in devicetree.
2. A future update might get those regulators disabled during deep sleep
Regards,
Angelo
> Thanks
> Peter
>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v7 13/23] scsi: ufs: mediatek: Use the common PHY framework
From: Peter Wang (王信友) @ 2026-02-24 12:41 UTC (permalink / raw)
To: chu.stanley@gmail.com, robh@kernel.org,
Chunfeng Yun (云春峰), kishon@kernel.org,
James.Bottomley@HansenPartnership.com, bvanassche@acm.org,
AngeloGioacchino Del Regno, neil.armstrong@linaro.org,
conor+dt@kernel.org, Chaotian Jing (井朝天),
lgirdwood@gmail.com, nicolas.frattaroli@collabora.com,
vkoul@kernel.org, krzk+dt@kernel.org, p.zabel@pengutronix.de,
alim.akhtar@samsung.com, matthias.bgg@gmail.com,
avri.altman@wdc.com, martin.petersen@oracle.com,
broonie@kernel.org
Cc: linux-scsi@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-phy@lists.infradead.org, linux-mediatek@lists.infradead.org,
Louis-Alexis Eyraud, kernel@collabora.com
In-Reply-To: <20260216-mt8196-ufs-v7-13-b5f2907c6da7@collabora.com>
On Mon, 2026-02-16 at 14:37 +0100, Nicolas Frattaroli wrote:
> There is no need to reinvent the PHY framework, especially not its OF
> parsing.
>
> Change the code to simply use the PHY framework to acquire the
> device's
> PHY in the ufshcd init, so that it's device linked to the right
> device.
>
> Reviewed-by: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
> ---
Could you move the PHY reorganization patch (11/23) into this patch?
Thanks
Peter
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v7 12/23] scsi: ufs: mediatek: Remove vendor kernel quirks cruft
From: Peter Wang (王信友) @ 2026-02-24 12:40 UTC (permalink / raw)
To: chu.stanley@gmail.com, robh@kernel.org,
Chunfeng Yun (云春峰), kishon@kernel.org,
James.Bottomley@HansenPartnership.com, bvanassche@acm.org,
AngeloGioacchino Del Regno, neil.armstrong@linaro.org,
conor+dt@kernel.org, Chaotian Jing (井朝天),
lgirdwood@gmail.com, nicolas.frattaroli@collabora.com,
vkoul@kernel.org, krzk+dt@kernel.org, p.zabel@pengutronix.de,
alim.akhtar@samsung.com, matthias.bgg@gmail.com,
avri.altman@wdc.com, martin.petersen@oracle.com,
broonie@kernel.org
Cc: krzysztof.kozlowski@oss.qualcomm.com, linux-scsi@vger.kernel.org,
devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-phy@lists.infradead.org, linux-mediatek@lists.infradead.org,
Louis-Alexis Eyraud, kernel@collabora.com
In-Reply-To: <20260216-mt8196-ufs-v7-12-b5f2907c6da7@collabora.com>
On Mon, 2026-02-16 at 14:37 +0100, Nicolas Frattaroli wrote:
> Both ufs_mtk_vreg_fix_vcc and ufs_mtk_vreg_fix_vccqx look like they
> are
> vendor kernel hacks to work around existing downstream device trees.
> Mainline does not need or want them, so remove them.
>
> Reviewed-by: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com>
> Reviewed-by: Krzysztof Kozlowski
> <krzysztof.kozlowski@oss.qualcomm.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
Reviewed-by: Peter Wang <peter.wang@mediatek.com>
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v7 11/23] scsi: ufs: mediatek: Rework probe function
From: Peter Wang (王信友) @ 2026-02-24 12:40 UTC (permalink / raw)
To: chu.stanley@gmail.com, robh@kernel.org,
Chunfeng Yun (云春峰), kishon@kernel.org,
James.Bottomley@HansenPartnership.com, bvanassche@acm.org,
AngeloGioacchino Del Regno, neil.armstrong@linaro.org,
conor+dt@kernel.org, Chaotian Jing (井朝天),
lgirdwood@gmail.com, nicolas.frattaroli@collabora.com,
vkoul@kernel.org, krzk+dt@kernel.org, p.zabel@pengutronix.de,
alim.akhtar@samsung.com, matthias.bgg@gmail.com,
avri.altman@wdc.com, martin.petersen@oracle.com,
broonie@kernel.org
Cc: linux-scsi@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-phy@lists.infradead.org, linux-mediatek@lists.infradead.org,
Louis-Alexis Eyraud, kernel@collabora.com
In-Reply-To: <20260216-mt8196-ufs-v7-11-b5f2907c6da7@collabora.com>
On Mon, 2026-02-16 at 14:37 +0100, Nicolas Frattaroli wrote:
> Remove the ti,syscon-reset cruft, as it was never documented in the
> binding, and is not modelling the hardware correctly.
>
> Make PHY mandatory. All the compatibles supported by the binding make
> it
> mandatory.
>
> Entertain this driver's insistence on playing with the PHY's RPM, but
> at
> least fix the part where it doesn't increase the reference count,
> which
> would lead to use-after-free.
>
> Reviewed-by: AngeloGioacchino Del Regno
> <angelogioacchino.delregno@collabora.com>
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@collabora.com>
Could you separate this patch to "remove reset" and "reorganize PHY"?
The PHY reorganization can be merged with patch (13/23).
Thanks
Peter
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH v7 10/23] scsi: ufs: mediatek: Handle misc host voltage regulators
From: Peter Wang (王信友) @ 2026-02-24 12:38 UTC (permalink / raw)
To: chu.stanley@gmail.com, robh@kernel.org,
Chunfeng Yun (云春峰), kishon@kernel.org,
James.Bottomley@HansenPartnership.com, bvanassche@acm.org,
AngeloGioacchino Del Regno, neil.armstrong@linaro.org,
conor+dt@kernel.org, Chaotian Jing (井朝天),
lgirdwood@gmail.com, nicolas.frattaroli@collabora.com,
vkoul@kernel.org, krzk+dt@kernel.org, p.zabel@pengutronix.de,
alim.akhtar@samsung.com, matthias.bgg@gmail.com,
avri.altman@wdc.com, martin.petersen@oracle.com,
broonie@kernel.org
Cc: linux-scsi@vger.kernel.org, devicetree@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-phy@lists.infradead.org, linux-mediatek@lists.infradead.org,
Louis-Alexis Eyraud, kernel@collabora.com
In-Reply-To: <20260216-mt8196-ufs-v7-10-b5f2907c6da7@collabora.com>
On Mon, 2026-02-16 at 14:37 +0100, Nicolas Frattaroli wrote:
> @@ -1188,8 +1190,21 @@ static int ufs_mtk_get_supplies(struct
> ufs_mtk_host *host)
> {
> struct device *dev = host->hba->dev;
> const struct ufs_mtk_soc_data *data =
> of_device_get_match_data(dev);
> + int ret;
> +
> + if (!data)
> + return 0;
> +
> + if (data->num_reg_names) {
> + ret = devm_regulator_bulk_get_enable(dev, data-
> >num_reg_names,
> + data-
> >reg_names);
Hi Nicolas,
If these regulators are only acquired and enabled once,
why not just set regulator-always-on in the device tree?
Thanks
Peter
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: (subset) [PATCH v3 0/6] Add support for mt8167 display blocks
From: AngeloGioacchino Del Regno @ 2026-02-24 9:34 UTC (permalink / raw)
To: linux-mediatek, Luca Leonardo Scorcia
Cc: Chun-Kuang Hu, Philipp Zabel, Maarten Lankhorst, Maxime Ripard,
Thomas Zimmermann, David Airlie, Simona Vetter, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, Chunfeng Yun, Vinod Koul,
Neil Armstrong, Matthias Brugger, dri-devel, devicetree,
linux-kernel, linux-arm-kernel, linux-phy
In-Reply-To: <cover.1771863641.git.l.scorcia@gmail.com>
On Mon, 23 Feb 2026 16:22:44 +0000, Luca Leonardo Scorcia wrote:
> This series adds support for the display blocks on MediaTek mt8167.
> Tested on Xiaomi Mi Smart Clock x04g.
>
> The first patch just does some reordering of dts nodes with no other
> changes as this makes later patches cleaner and easier to follow.
>
> v3:
> - Added mt8167-dsi compatible to driver instead of changing the binding;
> - Resolved patch formatting issues.
>
> [...]
Applied to v7.0-next/dts64, thanks!
[1/6] arm64: dts: mt8167: Reorder nodes according to mmio address
commit: d51b7191f2072e11259edd2bff88385891d0ae56
Cheers,
Angelo
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 1/3] phy: apple: atc: Make atcphy_dwc3_reset_ops variable static
From: Janne Grunau @ 2026-02-24 7:57 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Sven Peter, Neal Gompa, Vinod Koul, Neil Armstrong, Philipp Zabel,
asahi, linux-arm-kernel, linux-phy, linux-kernel, linux-arm-msm
In-Reply-To: <20260216110413.159994-4-krzysztof.kozlowski@oss.qualcomm.com>
On Mon, Feb 16, 2026 at 12:04:14PM +0100, Krzysztof Kozlowski wrote:
> File-scope 'atcphy_dwc3_reset_ops' is not used outside of this unit, so
> make it static to silence sparse warning:
>
> atc.c:2026:32: warning: symbol 'atcphy_dwc3_reset_ops' was not declared. Should it be static?
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> ---
> drivers/phy/apple/atc.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/phy/apple/atc.c b/drivers/phy/apple/atc.c
> index dc867f368b68..32d97226e926 100644
> --- a/drivers/phy/apple/atc.c
> +++ b/drivers/phy/apple/atc.c
> @@ -2023,7 +2023,7 @@ static int atcphy_dwc3_reset_deassert(struct reset_controller_dev *rcdev, unsign
> return 0;
> }
>
> -const struct reset_control_ops atcphy_dwc3_reset_ops = {
> +static const struct reset_control_ops atcphy_dwc3_reset_ops = {
> .assert = atcphy_dwc3_reset_assert,
> .deassert = atcphy_dwc3_reset_deassert,
> };
Reviewed-by: Janne Grunau <j@jannau.net>
thanks
Janne
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 3/3] phy: qcom: qmp-usbc: Simplify check for non-NULL pointer
From: Dmitry Baryshkov @ 2026-02-24 7:47 UTC (permalink / raw)
To: Krzysztof Kozlowski
Cc: Sven Peter, Janne Grunau, Neal Gompa, Vinod Koul, Neil Armstrong,
Philipp Zabel, asahi, linux-arm-kernel, linux-phy, linux-kernel,
linux-arm-msm
In-Reply-To: <20260216110413.159994-6-krzysztof.kozlowski@oss.qualcomm.com>
On Mon, Feb 16, 2026 at 12:04:16PM +0100, Krzysztof Kozlowski wrote:
> Pointers should not use explicit '0' comparison, so just use standard
> evaluation as non-NULL:
>
> phy-qcom-qmp-usbc.c:1682:31: warning: Using plain integer as NULL pointer
>
> Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
> ---
> drivers/phy/qualcomm/phy-qcom-qmp-usbc.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
--
With best wishes
Dmitry
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 2/2] phy: qcom: qmp-pcie: Add vdda-refgen supplies for Glymur
From: Qiang Yu @ 2026-02-24 6:42 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Vinod Koul, Neil Armstrong, Rob Herring, Krzysztof Kozlowski,
Conor Dooley, linux-arm-msm, linux-phy, devicetree, linux-kernel
In-Reply-To: <hi2znbyo5tunwi6du5ifxjci4xmjvfz5wrbikqs7yagpt2rhki@il4g5olkhkvw>
On Fri, Feb 13, 2026 at 07:48:23PM +0200, Dmitry Baryshkov wrote:
> On Wed, Feb 11, 2026 at 07:33:28PM -0800, Qiang Yu wrote:
> > On Mon, Feb 09, 2026 at 03:15:24PM +0200, Dmitry Baryshkov wrote:
> > > On Sun, Feb 08, 2026 at 08:49:40PM -0800, Qiang Yu wrote:
> > > > The refgen providing reference voltage for PCIe QMP PHY on Glymur requires
> > > > two power supplies independent from the PHY's core and qref rails. Add
> > > > support for vdda-refgen0p9 and vdda-refgen1p2 supplies with a dedicated
> > > > glymur_qmp_phy_vreg_l list.
> > > >
> > > > Update both Gen5x4 and Gen4x2 configurations to use the new supply list.
> > >
> > > I'd ask for the DTSI patch too...
> > >
> > I will post dtsi patch after we get agreement on how to descibe refgen in
> > dt-bindings.
>
> Please include DT patches in future series, they help us understand your
> changes.
>
Okay, will include DT patches in future series.
- Qiang Yu
> >
> > - Qiang Yu
> > > >
> > > > Signed-off-by: Qiang Yu <qiang.yu@oss.qualcomm.com>
> > > > ---
> > > > drivers/phy/qualcomm/phy-qcom-qmp-pcie.c | 12 ++++++++----
> > > > 1 file changed, 8 insertions(+), 4 deletions(-)
> > > >
> > >
> > > --
> > > With best wishes
> > > Dmitry
>
> --
> With best wishes
> Dmitry
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH 1/2] dt-bindings: phy: qcom,sc8280xp-qmp-pcie-phy: Add vdda-refgen supply for Glymur
From: Qiang Yu @ 2026-02-24 6:41 UTC (permalink / raw)
To: Dmitry Baryshkov
Cc: Krzysztof Kozlowski, Vinod Koul, Neil Armstrong, Rob Herring,
Krzysztof Kozlowski, Conor Dooley, linux-arm-msm, linux-phy,
devicetree, linux-kernel
In-Reply-To: <hcslao22elcihjw56ltu4yo54lotheqpikzsrq6tia33di4fs4@2ygrbwhcfx2a>
On Fri, Feb 13, 2026 at 07:47:36PM +0200, Dmitry Baryshkov wrote:
> On Wed, Feb 11, 2026 at 07:32:23PM -0800, Qiang Yu wrote:
> > On Mon, Feb 09, 2026 at 09:06:23AM +0100, Krzysztof Kozlowski wrote:
> > > On Sun, Feb 08, 2026 at 08:49:39PM -0800, Qiang Yu wrote:
> > > > The PCIe QMP PHYs on Glymur require stable reference voltage provided by
> > > > refgen. The refgen itself requires two separate power supplies:
> > > > vdda-refgen0p9 and vdda-refgen1p2.
> > > >
> > > > Since there is no dedicated driver for REFGEN, add vdda-refgen0p9-supply
> > >
> > > How does the driver matter for the bindings? If I add dedicated driver
> > > for refgen, then I change the bindings?
> >
> > Yeah, I know that dt-bindings should describe hardware, not software. But
> > what I meant to say is that the refgen is different from qref which is
> > controlled via TCSR registers and its LDOs are requested to vote in
> > tcsrcc driver. The refgen doesn't required register setting and it doesn't
> > have dedicated driver, so we vote its LDOs in phy driver. I will avoid
> > this statement in next version.
>
> I must admit, I could not find references to the refgen in any of Glymur
> PCIe-related HPGs.
We can find it on ipcatalog. On Glymur, there are 6 refgen instances:
u_cm_phy_refgen_0_west supplies to pcie3/4/6.
u_cm_phy_refgen_3_east supplies to pcie5.
This two refgen also supply to other modules like usb and qref which
is required by PCIe phy for stable reference clk.
In previous targets, refgen was often ignored because it shares LDOs with
phy or qref. We typically only vote for vdda-phy, vdda-pll, and vdda-qref
supplies. However, on Glymur, there are more instances and less LDO
sharing between phy/qref/refgen.
>
> >
> > >
> > > There is qcom,sc8280xp-refgen-regulator so why there cannot be
> > > qcom,x1e-refgen-regulator?
> >
> > I think we can and it seems better because the refgen for pcie phy also
> > supplies reference voltage to other modules like usb. But I checked the
> > qcom-refgen-regulator.c, it contains some register settings and there is
> > no LDOs voting. I'm not sure what does those register do, maybe Konrad
> > can provide some backgroud.
>
> Those regs provide voting for refgen, because on those platforms DSI
> block doesn't have a hardware vote for refgen.
>
Okay, for PCIe PHY, we don't need register settings for refgen.
- Qiang Yu
> > But on Glymur, we only need to vote LDOs. So
> > what if we use a fixed regulator in the device tree to represent refgen?
> > We could set refgen0p9 and refgen1p2 as its input supplies, then the PCIe
> > PHY would just need one refgen supply reference.
> >
> > - Qiang Yu
> > >
> > >
> > > > and vdda-refgen1p2-supply properties to the PCIe PHY dt-bindings. Use
> > > > conditional schema to restrict these properties to only Glymur PCIe QMP
> > > > PHYs.
> > >
> > > Best regards,
> > > Krzysztof
> > >
>
> --
> With best wishes
> Dmitry
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH] phy: core: fix potential UAF in of_phy_simple_xlate()
From: Dmitry Torokhov @ 2026-02-24 0:37 UTC (permalink / raw)
To: Vladimir Oltean
Cc: Vinod Koul, Kishon Vijay Abraham I, Neil Armstrong,
Rafael J. Wysocki, Geert Uytterhoeven, Johan Hovold,
Claudiu Beznea, Dr. David Alan Gilbert, Peter Griffin,
Dmitry Baryshkov, Krzysztof Kozlowski, Zijun Hu, linux-phy,
linux-kernel
In-Reply-To: <20260223231500.zeffezslctqamhp7@skbuf>
Hi Vladimir,
On Tue, Feb 24, 2026 at 01:15:00AM +0200, Vladimir Oltean wrote:
> Hi Dmitry,
>
> On Fri, Feb 20, 2026 at 05:01:50PM -0800, Dmitry Torokhov wrote:
> > On Thu, Feb 19, 2026 at 04:11:37PM -0800, Dmitry Torokhov wrote:
> > > On Thu, Feb 19, 2026 at 03:57:11PM -0800, Dmitry Torokhov wrote:
> > > > The implementation put_device()s located device and then uses
> > > > container_of() on the pointer. The device may disappear by that time,
> > > > resulting in UAF.
> > > >
> > > > Fix the problem by keeping the reference to the framer device,
> > > > avoiding getting an extra reference to it in framer_get(), and making
> > > > sure to drop the reference in error path when we fail to get the module.
> > >
> > > Hmm, I was too rash. There are bunch of other xlate functions that need
> > > to be updated to take the reference.
> >
> > So I am convinced that xlate functions need to bump up the reference to
> > phy devices they return. The question is how to deal with the ones that
> > do not. I can either convert them in the same patch (the changes are
> > quite mechanical) or we can do the whole song and dance, introduce a
> > flag, set it up in converted xlate functions, have the core respect it,
> > and then remove it from xlates and from the core when it is all done.
> >
> > Please let me know.
>
> You have hit on a weak spot of the Generic PHY framework and I would
> like to encourage you to follow through with patches for this finding
> (although it won't be exactly trivial, I think it's doable with some
> determination).
Thank you for the detailed response. I am not sure how much time I can
spend on phy rework as this change was an essentially a drive-by. I only
happen to look there because I want to remove
class_find_device_by_of_node() in favor of class_find_device_by_fwnode().
>
> On your direct question:
> It will impact downstream in subtle and unpleasant ways if you change
> the of_xlate() semantics w.r.t. reference counting, but the code will
> still compile just as before, i.e. when looking just at your downstream
> driver (what 99% of developers do) there will be no obvious way of
> knowing that something in the API has changed. I would avoid doing that.
Hmm, I was not aware that we needed to take particular measures for
benefit of downstream trees...
>
> But first the problem. Too little has been said about the problem, and
> too much about the solution. We can't find a good solution if we don't
> call out the problem properly first.
>
> The phy_get() function follows a "lookup-then-get" approach for PHY
> providers, rather than "atomic-lookup-and-get".
>
> Namely, the phy_provider->of_xlate() caller, which is _of_phy_get(),
> returns a struct phy * with its underlying device reference count
> nominally at 1. All callers of _of_phy_get() do later call get_device()
> to bump this refcount for each PHY consumer, but it is "too late" in the
> sense that a window has been opened where the PHY provider driver can
> unbind, and the reference count of &phy->dev can drop to 0 and it can be
> freed.
Yes.
>
> Immediately after being created by phy_create(), the &phy->dev has a
> refcount of 1, and only the action of its provider driver (calling
> phy_destroy() directly or through devres, on driver unbind) can drop
> that refcount to 0. As long as the driver doesn't do that, the &phy->dev
> refcount is not in danger.
Right.
>
> Then the PHY is exposed to the outside world using of_phy_provider_register(),
> where the fact that the provider driver can concurrently unregister
> starts being a problem.
>
> Therefore, I would say that the problem is that consumers, aka phy_get()
> callers, can get a phy with a &phy->dev refcount that is not already bumped
> by the time they are handed over that PHY.
>
> Mechanically, PHY lookup happens under &phy_provider_mutex, and I
> believe it would be sufficient to call get_device() under this lock, in
> order for consumers to get PHYs with their reference bumped.
<skip the explanation and a patch>
Yes, given that there is 1:1 mapping between device and provider (not
phy instance but its parent) it looks like we can drop the reference and
then bump it up again as long as we hold that mutex.
>
> The patch above leaves a few loose ends.
>
> The most obvious is of_phy_simple_xlate(), which has that put_device()
> to balance out class_find_device_by_of_node() - which bumps the device
> refcount to 2. It "looks" wrong but it is consistent with vendor
> implementations of of_xlate(), which also provide a phy->dev refcount of 1.
> And as explained, the refcount never _actually_ drops to 0 with the
> above patch.
>
> I would actually address _only_ of_phy_simple_xlate(), for cosmetics sake.
> Instead of devm_of_phy_provider_register(..., of_phy_simple_xlate), I
> would offer a new helper, devm_of_phy_simple_provider_register(), which
> would internally use a callback that doesn't drop the refcount (say
> __of_phy_simple_xlate() for lack of imagination). I would convert vendor
> PHY drivers one by one to use this (it would be valid at any time to use
> either the old or the new method).
>
> You'd notice that most of the of_phy_simple_xlate() occurrences go away,
> except for freescale/phy-fsl-lynx-28g.c which calls this function
> directly from its own lynx_28g_xlate(). It will still have to keep
> calling it, and for refcount equalization purposes that weird
> put_device() will have to continue to exist. Just leave a comment as to
> why that is.
I this case I would simply add a comment telling why the reference
should (and can) be dropped where it is being dropped in
of_phy_simple_xlate() and call it a day. There is not much benefit in
adding another helper.
>
>
> But there are more important loose ends still.
>
> I mentioned that "zombie" device. We've solved the memory safety issue,
> but it's possible for consumers to hold onto a phy whose provider has
> disappeared. The refcount of &phy->dev hasn't dropped to 0, so
> technically it's a valid object, but from PHY API perspective, it's
> still possible to call phy_init(), phy_power_on() and friends on this
> PHY, and the Generic PHY core will be happy to further call into the
> phy->ops->init(), phy->ops->power_on() etc. But the driver has unbound,
> so it should really be left alone.
>
> If we fix the UAF but leave the zombie PHY problem, we've effectively
> done nothing but silence static analysis checkers, while the code path
> where the PHY provider unbinds effectively remains treated as poorly as
> before, just moving the crashes to a different place.
>
> I suspect what needs to be done here is to introduce a "bool dead" or
> similar, which is to be set from phy_destroy() and checked from every
> API call. The idea is that API functions on zombie PHYs should fail
> without calling into their driver. I **suppose** that
> try_module_get(phy->ops->owner) "tried" to avoid this situation, but
> it just protects against module removal, not against "echo device >
> /sys/bus/.../unbind". So it's absolutely incomplete and easily bypassable.
I think this is a problem common to many kernel subsystems, where there
are devices that are vital for other devices to function, but are not
parents of said devices. Think about regulators or clocks and such.
In many such cases even validating at API level is not sufficient,
because if you enable a clock you typically keep it running at least for
a while, if not for entire duration of device being bound to a driver.
If that clock goes away the device will break even though you are not
calling any clock APIs at that particular time.
We need some kind of revocation mechanism to signal consumers that
providers they rely upon are going away.
>
>
> Finally, I have identified one more loose end still.
>
> /**
> * of_phy_put() - release the PHY
> * @phy: the phy returned by of_phy_get()
> *
> * Releases a refcount the caller received from of_phy_get().
> */
> void of_phy_put(struct phy *phy)
> {
> if (!phy || IS_ERR(phy))
> return;
>
> mutex_lock(&phy->mutex);
> if (phy->ops->release)
> phy->ops->release(phy);
> mutex_unlock(&phy->mutex);
>
> module_put(phy->ops->owner);
> put_device(&phy->dev);
> }
> EXPORT_SYMBOL_GPL(of_phy_put);
>
> This function is called by PHY consumers. A PHY provider can have
> multiple consumers (example in the case of Ethernet: a QSGMII SerDes
> lane has 4 MAC ports multiplexed onto it).
>
> If a single consumer calls of_phy_put() to release its reference to the
> SerDes lane, the phy->ops->release() method makes absolutely no sense.
> There are 3 remaining consumers with handles to the lane! But we aren't
> even telling the PHY which consumer has disappeared! It has nothing
> useful to do with this information.
>
> Looking at actual phy_ops implementations, what they want to know is
> when _all_ consumers went away, not when individual consumers did.
> So the phy->ops->release() call needs to be put somewhere which is
> executed when all consumers disappear. If I were to guess, that would be
> the phy_release() class callback, but this is completely untested.
Shouldn't phy->ops->release() keep track of users and decide when to
destroy the resources? The rest seem fine as they simply drop references
(I assume each user of shared phy will bump up references as needed?)
>
> Sorry that this email is a bit long, it got a bit late writing it and I
> don't have a lot of energy left to trim it down.
> In summary I found 4 highly related problems:
> - use after free
> - misleading of_phy_simple_xlate() API pattern (not a functional issue)
> - zombie PHYs
> - premature release call
Thanks.
--
Dmitry
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply
* Re: [PATCH] phy: core: fix potential UAF in of_phy_simple_xlate()
From: Vladimir Oltean @ 2026-02-23 23:15 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Vinod Koul, Kishon Vijay Abraham I, Neil Armstrong,
Rafael J. Wysocki, Geert Uytterhoeven, Johan Hovold,
Claudiu Beznea, Dr. David Alan Gilbert, Peter Griffin,
Dmitry Baryshkov, Krzysztof Kozlowski, Zijun Hu, linux-phy,
linux-kernel
In-Reply-To: <aZkCyQ1oizPXplyU@google.com>
Hi Dmitry,
On Fri, Feb 20, 2026 at 05:01:50PM -0800, Dmitry Torokhov wrote:
> On Thu, Feb 19, 2026 at 04:11:37PM -0800, Dmitry Torokhov wrote:
> > On Thu, Feb 19, 2026 at 03:57:11PM -0800, Dmitry Torokhov wrote:
> > > The implementation put_device()s located device and then uses
> > > container_of() on the pointer. The device may disappear by that time,
> > > resulting in UAF.
> > >
> > > Fix the problem by keeping the reference to the framer device,
> > > avoiding getting an extra reference to it in framer_get(), and making
> > > sure to drop the reference in error path when we fail to get the module.
> >
> > Hmm, I was too rash. There are bunch of other xlate functions that need
> > to be updated to take the reference.
>
> So I am convinced that xlate functions need to bump up the reference to
> phy devices they return. The question is how to deal with the ones that
> do not. I can either convert them in the same patch (the changes are
> quite mechanical) or we can do the whole song and dance, introduce a
> flag, set it up in converted xlate functions, have the core respect it,
> and then remove it from xlates and from the core when it is all done.
>
> Please let me know.
You have hit on a weak spot of the Generic PHY framework and I would
like to encourage you to follow through with patches for this finding
(although it won't be exactly trivial, I think it's doable with some
determination).
On your direct question:
It will impact downstream in subtle and unpleasant ways if you change
the of_xlate() semantics w.r.t. reference counting, but the code will
still compile just as before, i.e. when looking just at your downstream
driver (what 99% of developers do) there will be no obvious way of
knowing that something in the API has changed. I would avoid doing that.
But first the problem. Too little has been said about the problem, and
too much about the solution. We can't find a good solution if we don't
call out the problem properly first.
The phy_get() function follows a "lookup-then-get" approach for PHY
providers, rather than "atomic-lookup-and-get".
Namely, the phy_provider->of_xlate() caller, which is _of_phy_get(),
returns a struct phy * with its underlying device reference count
nominally at 1. All callers of _of_phy_get() do later call get_device()
to bump this refcount for each PHY consumer, but it is "too late" in the
sense that a window has been opened where the PHY provider driver can
unbind, and the reference count of &phy->dev can drop to 0 and it can be
freed.
Immediately after being created by phy_create(), the &phy->dev has a
refcount of 1, and only the action of its provider driver (calling
phy_destroy() directly or through devres, on driver unbind) can drop
that refcount to 0. As long as the driver doesn't do that, the &phy->dev
refcount is not in danger.
Then the PHY is exposed to the outside world using of_phy_provider_register(),
where the fact that the provider driver can concurrently unregister
starts being a problem.
Therefore, I would say that the problem is that consumers, aka phy_get()
callers, can get a phy with a &phy->dev refcount that is not already bumped
by the time they are handed over that PHY.
Mechanically, PHY lookup happens under &phy_provider_mutex, and I
believe it would be sufficient to call get_device() under this lock, in
order for consumers to get PHYs with their reference bumped.
Why?
Let's consider what a PHY provider does when unbinding. It runs the PHY
registration and creation processes in reverse, i.e.
- it calls of_phy_provider_unregister() directly or through devres
- it calls phy_destroy() directly or through devres
Since of_phy_provider_unregister() also acquires &phy_provider_mutex,
this acts like a synchronization barrier. There are 2 cases:
Consumer Provider
phy_get()
-> mutex_lock(&phy_provider_mutex)
-> get_device(&phy->dev) -> 2
-> mutex_unlock(&phy_provider_mutex)
of_phy_provider_unregister()
-> mutex_lock(&phy_provider_mutex)
-> removes phy from list
-> mutex_unlock(&phy_provider_mutex)
phy_destroy()
-> device_unregister(&phy->dev)
-> device_del(dev);
-> put_device(dev); // -> 1
and the consumer remains with a "zombie" PHY device (more later)
Or
Consumer Provider
phy_get()
of_phy_provider_unregister()
-> mutex_lock(&phy_provider_mutex)
-> removes phy from list
-> mutex_unlock(&phy_provider_mutex)
-> mutex_lock(&phy_provider_mutex)
-> doesn't find the PHY device!
It was unpublished even if not freed
-> mutex_unlock(&phy_provider_mutex)
phy_destroy()
-> device_unregister(&phy->dev)
-> device_del(dev);
-> put_device(dev); // -> 1
and the consumer won't be allowed to even find the PHY device in this case.
So this is why get_device() under phy_provider_mutex actually helps.
Now, it is much less important whether you push the get_device() to the
actual of_xlate() PHY vendor implementation or not. For simplicity sake,
I suggest you don't do that, but instead keep it in the core, to
preserve driver API semantics (patch not even compile-tested):
-- >8 --
diff --git a/drivers/phy/phy-core.c b/drivers/phy/phy-core.c
index 21aaf2f76e53..c50e38f057a8 100644
--- a/drivers/phy/phy-core.c
+++ b/drivers/phy/phy-core.c
@@ -122,17 +122,19 @@ EXPORT_SYMBOL_GPL(phy_remove_lookup);
static struct phy *phy_find(struct device *dev, const char *con_id)
{
const char *dev_id = dev_name(dev);
- struct phy_lookup *p, *pl = NULL;
+ struct phy *phy = NULL;
+ struct phy_lookup *p;
mutex_lock(&phy_provider_mutex);
list_for_each_entry(p, &phys, node)
if (!strcmp(p->dev_id, dev_id) && !strcmp(p->con_id, con_id)) {
- pl = p;
+ phy = p->phy;
+ get_device(&phy->dev);
break;
}
mutex_unlock(&phy_provider_mutex);
- return pl ? pl->phy : ERR_PTR(-ENODEV);
+ return phy ? phy : ERR_PTR(-ENODEV);
}
static struct phy_provider *of_phy_provider_lookup(struct device_node *node)
@@ -649,6 +651,7 @@ static struct phy *_of_phy_get(struct device_node *np, int index)
}
phy = phy_provider->of_xlate(phy_provider->dev, &args);
+ get_device(&phy->dev);
out_put_module:
module_put(phy_provider->owner);
@@ -682,10 +685,10 @@ struct phy *of_phy_get(struct device_node *np, const char *con_id)
if (IS_ERR(phy))
return phy;
- if (!try_module_get(phy->ops->owner))
+ if (!try_module_get(phy->ops->owner)) {
+ put_device(&phy->dev);
return ERR_PTR(-EPROBE_DEFER);
-
- get_device(&phy->dev);
+ }
return phy;
}
@@ -803,10 +806,10 @@ struct phy *phy_get(struct device *dev, const char *string)
if (IS_ERR(phy))
return phy;
- if (!try_module_get(phy->ops->owner))
+ if (!try_module_get(phy->ops->owner)) {
+ put_device(&phy->dev);
return ERR_PTR(-EPROBE_DEFER);
-
- get_device(&phy->dev);
+ }
link = device_link_add(dev, &phy->dev, DL_FLAG_STATELESS);
if (!link)
@@ -969,11 +972,10 @@ struct phy *devm_of_phy_get_by_index(struct device *dev, struct device_node *np,
if (!try_module_get(phy->ops->owner)) {
devres_free(ptr);
+ put_device(&phy->dev);
return ERR_PTR(-EPROBE_DEFER);
}
- get_device(&phy->dev);
-
*ptr = phy;
devres_add(dev, ptr);
-- >8 --
The patch above leaves a few loose ends.
The most obvious is of_phy_simple_xlate(), which has that put_device()
to balance out class_find_device_by_of_node() - which bumps the device
refcount to 2. It "looks" wrong but it is consistent with vendor
implementations of of_xlate(), which also provide a phy->dev refcount of 1.
And as explained, the refcount never _actually_ drops to 0 with the
above patch.
I would actually address _only_ of_phy_simple_xlate(), for cosmetics sake.
Instead of devm_of_phy_provider_register(..., of_phy_simple_xlate), I
would offer a new helper, devm_of_phy_simple_provider_register(), which
would internally use a callback that doesn't drop the refcount (say
__of_phy_simple_xlate() for lack of imagination). I would convert vendor
PHY drivers one by one to use this (it would be valid at any time to use
either the old or the new method).
You'd notice that most of the of_phy_simple_xlate() occurrences go away,
except for freescale/phy-fsl-lynx-28g.c which calls this function
directly from its own lynx_28g_xlate(). It will still have to keep
calling it, and for refcount equalization purposes that weird
put_device() will have to continue to exist. Just leave a comment as to
why that is.
But there are more important loose ends still.
I mentioned that "zombie" device. We've solved the memory safety issue,
but it's possible for consumers to hold onto a phy whose provider has
disappeared. The refcount of &phy->dev hasn't dropped to 0, so
technically it's a valid object, but from PHY API perspective, it's
still possible to call phy_init(), phy_power_on() and friends on this
PHY, and the Generic PHY core will be happy to further call into the
phy->ops->init(), phy->ops->power_on() etc. But the driver has unbound,
so it should really be left alone.
If we fix the UAF but leave the zombie PHY problem, we've effectively
done nothing but silence static analysis checkers, while the code path
where the PHY provider unbinds effectively remains treated as poorly as
before, just moving the crashes to a different place.
I suspect what needs to be done here is to introduce a "bool dead" or
similar, which is to be set from phy_destroy() and checked from every
API call. The idea is that API functions on zombie PHYs should fail
without calling into their driver. I **suppose** that
try_module_get(phy->ops->owner) "tried" to avoid this situation, but
it just protects against module removal, not against "echo device >
/sys/bus/.../unbind". So it's absolutely incomplete and easily bypassable.
Finally, I have identified one more loose end still.
/**
* of_phy_put() - release the PHY
* @phy: the phy returned by of_phy_get()
*
* Releases a refcount the caller received from of_phy_get().
*/
void of_phy_put(struct phy *phy)
{
if (!phy || IS_ERR(phy))
return;
mutex_lock(&phy->mutex);
if (phy->ops->release)
phy->ops->release(phy);
mutex_unlock(&phy->mutex);
module_put(phy->ops->owner);
put_device(&phy->dev);
}
EXPORT_SYMBOL_GPL(of_phy_put);
This function is called by PHY consumers. A PHY provider can have
multiple consumers (example in the case of Ethernet: a QSGMII SerDes
lane has 4 MAC ports multiplexed onto it).
If a single consumer calls of_phy_put() to release its reference to the
SerDes lane, the phy->ops->release() method makes absolutely no sense.
There are 3 remaining consumers with handles to the lane! But we aren't
even telling the PHY which consumer has disappeared! It has nothing
useful to do with this information.
Looking at actual phy_ops implementations, what they want to know is
when _all_ consumers went away, not when individual consumers did.
So the phy->ops->release() call needs to be put somewhere which is
executed when all consumers disappear. If I were to guess, that would be
the phy_release() class callback, but this is completely untested.
Sorry that this email is a bit long, it got a bit late writing it and I
don't have a lot of energy left to trim it down.
In summary I found 4 highly related problems:
- use after free
- misleading of_phy_simple_xlate() API pattern (not a functional issue)
- zombie PHYs
- premature release call
--
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox