Message-ID: <1f2f46ee-40c0-4967-9f39-fab791eff76d@quicinc.com>
Date: Tue, 28 Oct 2025 22:33:56 +0530
From: Pavan Kondeti <pavan.kondeti@....qualcomm.com>
To: Neil Armstrong <neil.armstrong@...aro.org>
Cc: Pavan Kondeti <pavan.kondeti@....qualcomm.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Hrishabh Rajput <hrishabh.rajput@....qualcomm.com>,
        Bjorn Andersson <andersson@...nel.org>,
        Konrad Dybcio <konradybcio@...nel.org>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Guenter Roeck <linux@...ck-us.net>, Rob Herring <robh@...nel.org>,
        Krzysztof Kozlowski <krzk+dt@...nel.org>,
        Conor Dooley <conor+dt@...nel.org>, linux-arm-msm@...r.kernel.org,
        linux-watchdog@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3] watchdog: Add driver for Gunyah Watchdog

On Tue, Oct 28, 2025 at 05:39:26PM +0100, Neil Armstrong wrote:
> On 10/28/25 17:33, Pavan Kondeti wrote:
> > On Tue, Oct 28, 2025 at 05:17:44PM +0100, Krzysztof Kozlowski wrote:
> > > On 28/10/2025 13:27, Pavan Kondeti wrote:
> > > > On Tue, Oct 28, 2025 at 12:07:40PM +0100, Krzysztof Kozlowski wrote:
> > > > > On 28/10/2025 12:04, Krzysztof Kozlowski wrote:
> > > > > > On 28/10/2025 11:58, Hrishabh Rajput wrote:
> > > > > > > 
> > > > > > > On 10/28/2025 3:10 PM, Krzysztof Kozlowski wrote:
> > > > > > > > On 28/10/2025 10:35, Hrishabh Rajput via B4 Relay wrote:
> > > > > > > > > +
> > > > > > > > > +static int __init gunyah_wdt_init(void)
> > > > > > > > > +{
> > > > > > > > > +	struct arm_smccc_res res;
> > > > > > > > > +	struct device_node *np;
> > > > > > > > > +	int ret;
> > > > > > > > > +
> > > > > > > > > +	/* Check if we're running on a Qualcomm device */
> > > > > > > > > +	np = of_find_compatible_node(NULL, NULL, "qcom,smem");
> > > > > > > > I don't think you implemented my feedback. This again is executed on
> > > > > > > > every platform, e.g. on Samsung, pointlessly.
> > > > > > > > 
> > > > > > > > Implement previous feedback.
> > > > > > > 
> > > > > > > Do you want us to add platform device from another driver which is
> > > > > > > probed only on Qualcomm devices (like socinfo from previous discussion)
> > > > > > > and get rid of the module init function entirely? As keeping anything in
> > > > > > > the module init will get it executed on all platforms.
> > > > > > 
> > > > > > Instead of asking the same can you read previous discussion? What is
> > > > > > unclear here:
> > > > > > https://lore.kernel.org/all/3b901f9d-dbfa-4f93-a8d2-3e89bd9783c9@kernel.org/
> > > > > > ?
> > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > With this patch version, we have tried to reduce the code execution on
> > > > > > > non-Qualcomm devices (also tried the alternative as mentioned in the
> > > > > > > cover letter). Adding platform device from another driver as described
> > > > > > > above would eliminate it entirely, please let us know if you want us to
> > > > > > > do that.
> > > > > > 
> > > > > > Why do I need to repeat the same as last time?
> > > > > 
> > > > > 
> > > > > Now I see that you completely ignored previous discussion and sent THE
> > > > > SAME approach.
> > > > 
> > > > Our intention is not to waste reviewers time at all. It is just a
> > > > misunderstanding on what your comment is about. Let me elaborate further
> > > > not to defend our approach here but to get a clarity so that we don't
> > > > end up in the same situation when v4 is posted.
> > > > 
> > > > https://lore.kernel.org/all/b94d8ca3-af58-4a78-9a5a-12e3db0bf75f@kernel.org/
> > > > 
> > > > You mentioned here
> > > > 
> > > > ```
> > > > To me socinfo feels even better. That way only, really only qcom devices
> > > > will execute this SMC.
> > > > ```
> > > > 
> > > > We interpreted this comment as `avoid executing this SMC on non qcom
> > > > devices`. That is exactly what we have done in the current patch. since
> > > 
> > > 
> > > So where did you use socinfo? Point me to the code.
> > > 
> > 
> > Okay, lets go a bit deep into the socinfo part. we have used
> > `soc_device_match()` API to detect if the device is qcom (`family =
> > Snapdragon`). It works. However, when we built both `socinfo` and
> > `gunyah-wdt` as modules, we do see that `gunyah-wdt` gets probed before
> > `socinfo` because the driver that registers socinfo as platform device
> > which is `smem` probe is getting delayed. As you may know `smem` device
> > gets registered by `OF` core directly before the whole platform devices
> > are populated. To make sure that any configuration works, we went with
> > `qcom,smem` based detection. This is mentioned in the cover letter, sure
> > it is a detail that can easily be lost. Now one might just say go and
> > fix probe deferral problems. The problem here is that `smem` platform
> > device creation happens differently to other devices which is leading to
> > probe deferral. I can enumerate the problem in much detail, if that
> > interests you.
> > 
> > Please help us understand what is the real concern here? we don't want
> > to call `of_find_compatible_node()` API on non qcom devices but it is
> > okay to register drivers. It is still not clear why would non qcom
> > devices/platform which care about performance load all modules that gets
> > compiled with ARM64. Needless to say it would load lots of modules and
> > register many drivers which never gets probed.
> 
> The module file will contain the devicetree compatible they match on,
> and they will only be loaded if they match a compatible on the loaded DT.
> Some modules which doesn't match on a compatible, PCI id, .... like yours
> would be loaded on __all__ arm64 platforms.
> 
> Just have a look at modinfo and the whole Linux module handling.
> 
Thank you for the details. we touched upon modalias part here [1] but I
did not know that the modules like this i.e no compatible match gets
loaded automatically by user space. Thanks for the clarification.

Thanks,
Pavan

[1]
https://lore.kernel.org/all/491de94c-e3c5-4f81-8e1a-82596413cede@quicinc.com/

Powered by blists - more mailing lists