Message-ID: <Z8scADVM/g/669Uz@lizhi-Precision-Tower-5810>
Date: Fri, 7 Mar 2025 11:17:04 -0500
From: Frank Li <Frank.li@....com>
To: Bjorn Andersson <andersson@...nel.org>
Cc: Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
	Bjorn Andersson <bjorn.andersson@....qualcomm.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Rob Herring <robh@...nel.org>,
	Krzysztof Kozlowski <krzk+dt@...nel.org>,
	Conor Dooley <conor+dt@...nel.org>, Felipe Balbi <balbi@...nel.org>,
	Wesley Cheng <quic_wcheng@...cinc.com>,
	Saravana Kannan <saravanak@...gle.com>,
	Philipp Zabel <p.zabel@...gutronix.de>,
	Konrad Dybcio <konradybcio@...nel.org>,
	"linux-arm-msm@...r.kernel.org" <linux-arm-msm@...r.kernel.org>,
	"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
	"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 5/7] usb: dwc3: qcom: Snapshot driver for backwards
 compatibilty

On Thu, Mar 06, 2025 at 04:49:28PM -0600, Bjorn Andersson wrote:
> On Wed, Mar 05, 2025 at 12:31:49AM +0000, Thinh Nguyen wrote:
> > On Mon, Mar 03, 2025, Bjorn Andersson wrote:
> > > On Tue, Mar 04, 2025 at 12:39:12AM +0000, Thinh Nguyen wrote:
> > > > On Tue, Mar 04, 2025, Thinh Nguyen wrote:
> > > > > On Wed, Feb 26, 2025, Bjorn Andersson wrote:
> > > > > > In order to more tightly integrate the Qualcomm glue driver with the
> > > > > > dwc3 core the driver is redesigned to avoid splitting the implementation
> > > > > > using the driver model. But due to the strong coupling to the Devicetree
> > > > > > binding needs to be updated as well.
> > > > > >
> > > > > > Various ways to provide backwards compatibility with existing Devicetree
> > > > > > blobs has been explored, but migrating the Devicetree information
> > > > > > between the old and the new binding is non-trivial.
> > > > > >
> > > > > > For the vast majority of boards out there, the kernel and Devicetree are
> > > > > > generated and handled together, which in practice means that backwards
> > > > > > compatibility needs to be managed across about 1 kernel release.
> > > > > >
> > > > > > For some though, such as the various Snapdragon laptops, the Devicetree
> > > > > > blobs live a life separate of the kernel. In each one of these, with the
> > > > > > continued extension of new features, it's recommended that users would
> > > > > > upgrade their Devicetree somewhat frequently.
> > > > > >
> > > > > > With this in mind, simply carrying a snapshot/copy of the current driver
> > > > > > is simpler than creating and maintaining the migration code.
> > > > > >
> > > > > > The driver is kept under the same Kconfig option, to ensure that Linux
> > > > > > distributions doesn't drop USB support on these platforms.
> > > > > >
> > > > > > The driver, which is going to be refactored to handle the newly
> > > > > > introduced qcom,snps-dwc3 compatible, is updated to temporarily not
> > > > > > match against any compatible.
> > > > > >
> > > > > > This driver should be removed after 2 LTS releases.
> > > > > >
> > > > > > Signed-off-by: Bjorn Andersson <bjorn.andersson@....qualcomm.com>
> > > > > > ---
> > > > > >  drivers/usb/dwc3/Makefile           |   1 +
> > > > > >  drivers/usb/dwc3/dwc3-qcom-legacy.c | 934 ++++++++++++++++++++++++++++++++++++
> > > > > >  drivers/usb/dwc3/dwc3-qcom.c        |   1 -
> > > > > >  3 files changed, 935 insertions(+), 1 deletion(-)
> > > > > >
> > > > >
> > > > > This is a bit concerning if there's no matching compatible string. ie.
> > > > > we don't have user for the new driver without downstream dependencies
> > > > > (or some workaround in the driver binding).
> > > >
> > > > Ignore the comment above, I missed the "temporarily" in your log
> > > > above. However, the comment below still stands.
> > > >
> > > > >
> > > > > While I understand the intention, I'm afraid we may have to support and
> > > > > maintain this much longer than the proposed 2 LTS releases (as seen with
> > > > > anything tagged with "legacy" in the upstream kernel).
> > >
> > > There are no products shipping today using dwc3-qcom where Devicetree is
> > > considered firmware. The primary audience for a longer transition is
> > > users of the various laptops with Qualcomm-chip in them. But given the
> > > rapid development in a variety of functional areas, these users will be
> > > highly compelled to update their DTBs within 2 years.
> > >
> > > The other obvious user group is to make sure us upstream developers
> > > don't loose USB when things get out of sync.
> > >
> > >
> > > That said, if the model defined here is to be followed in other cases
> > > (or my other vendors) where Devicetree is treated as firmware, your
> > > concerns are valid - and it might be worth taking the cost of managing
> > > the live-migration code.
> > >
> > > > > If possible, I'd
> > > > > prefer the complications of maintenance of the migration code be handled
> > > > > downstream.
> > > > >
> > >
> > > I'm sorry, but here it sounds like you're saying that you don't want any
> > > migration code upstream at all? This is not possible, as this will break
> > > USB for developers and users short term. We can of course discuss the 2
> > > LTS though, if you want a shorter life span for this migration.
> > >
> >
> > My first concern is now we have a legacy driver that should not be
> > continued to be developed while we also need to address any
> > regression/fixes found in the future from the legacy driver. While I
> > would encourage users to start migrating to the new driver, I won't
> > reject fixes to the legacy driver either. In the next 2 years+, my
> > other concern is that I'm not confident that we can easily remove the
> > legacy driver and the DTS then.
> >
>
> The problem at hand is that the driver _needs_ a bunch of work.
> Role-switching only works sometimes, extcon is (for older platforms)
> duplicated in both glue and core - with the hope that each part does its
> thing in a suitable fashion, the layering violations can trigger
> NULL-pointer dereferences or use-after-free, PM runtime is marked
> forbidden...
>
> We've looked at these problems for a few years now, without coming up
> with any solution to address these issues within the current design.
>
> Following this refactor, we will be able to work on these improvements.
> For this to happen, I intend to transition all the
> arch/*/boot/dts/qcom/* platforms to the new binding as soon as possible.
>
>
> Looking ahead, when we hit the point of deprecating the dwc3-qcom-legacy
> driver:
>
> The upstream-based product we have today do ship Devicetree in
> combination with the kernel, so they would upgrade both together and get
> the new driver.
>
> The other group would be kernel developers, enthusiasts, specific users
> who for some reason is upgrading their kernel but not their Devicetree.
> These users will want the new features and stability we're bringing.
>
> > Code can break, and that's not unexpected. If 2 LTS releases later and
> > we remove the dwc3-qcom-legacy, things can break then too. This may just
> > as be painful if we need fixes to the legacy driver due to some previous
> > regression. Also, I'm sure your team did a fair share of testing the new
> > driver right? Is there some major concern in the new driver that we
> > haven't addressed?
> >
>
> The new and old drivers are mostly identical at this point, and expected
> to diverge from here.
>
> The one thing I have identified to differ is that the "legacy" driver
> supports 2 extcon handles in the glue, but this is not considered
> acceptable by the binding so I haven't found anyone actually exercising
> this code path - then again extcon and usb_role_switch is one of the
> things this enables us to clean up.
>
>
> That said, while this model seems suitable for Qualcomm, due to the
> current state of things, I don't know if the same is true for Frank Li,
> perhaps NXP has a broader user base and need the migration logic.

So far, we use only one extcon.

Frank
>
> > >
> > > In my view, setting a flag date when the dwc3-qcom-legacy.c will be
> > > removed will provide upstream users a transition period, at a very low
> > > additional cost (934 lines of already tested code). If someone
> > > downstream after that flag date wants to retain support for qcom,dwc3
> > > they can just revert the removal of dwc3-qcom-legacy.c.
> >
> > The same can be said that they can revert the update (or apply fixes)
> > should they found issue with the new change.
> >
>
> We're changing the Devicetree binding, which gives us two problems:
> 1) Devicetree source code and DWC3 driver code are merged through
> different trees.
>
> 2) The compiled Devicetree (.dtb) and kernel image are in some cases
> separate software deliverables.
>
> So we absolutely need some migration mechanism to not just break USB for
> everyone for the coming 1-2 releases - at least.
>
> That said, the "2 LTS" is completely arbitrary. If you prefer to limit
> that, we can certainly have that discussion! E.g. I wouldn't argue
> against setting the flag-date by the end of this year.
>
> > >
> > > The alternative is that I try to get the migration code suggested in v3
> > > to a state where it can be merged (right now it's 6x larger) and then
> > > keep investing indefinitely in making sure it's not bit-rotting
> > > (although Rob Herring did request a flag date of the migration code in
> > > v3 as well...).
> > >
> >
> > All that said, if you believe that this transition will be quite
> > disruptive without preserving the legacy driver/dts, then we will do so.
> >
>
> We absolutely need a transition period, per above reasons. The length of
> it is an open question.
>
> > Can I request that you make this snapshot as one of the first patches in
> > the series so reverts/git-blames can easily be traced?
> >
>
> Absolutely.
>
> > BR,
> > Thinh
> >
> > Side question: for Snapdragon laptops, without the corresponding kernel
> > and DTS updates, don't things break easily?
>
> It certainly happens, but maintaining backwards compatibility is
> something we're striving for. As the Devicetree bindings mature, the
> easier this is though.
>
> One example where this is a problem will be clear here, that users
> attempting to boot today's kernel with tomorrows Devicetree blobs will
> not get USB - because today's kernel doesn't know how to make of the
> information in that description.
>
> This is true for any hardware or firmware interface though, so there's
> only so much one can do about that (and whatever that is, we're trying
> to do - for the sake of user friendliness).
>
> Regards,
> Bjorn

Powered by blists - more mailing lists