From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3225D18872A for ; Wed, 3 Jun 2026 01:18:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780449492; cv=none; b=OgZsGS4fOoddErwucxmPcJqxzyJk0PwOVpAo3Dk2YsM44sud/8qcJ4ETdTsEA47diAIlqhsFxFoIKpJTwQdk7pjjgaq8ksC07CB6ui8NFgc+fL0Cb6VZ9n9jo07uksWUM7lFtT3HC2hmVqfJfScWu3NYEqSQMPHPqczP4s3Y5+M= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780449492; c=relaxed/simple; bh=H9BEHWsQhrBXf0/h3Yc2CIRLeo9x9Ch1qy9EFdHuKcQ=; h=From:Subject:To:Cc:In-Reply-To:References:Content-Type:Date: Message-Id; b=dNPEN68vuKCaVpX7XX9/K1vO3R3UFyUxF3ndWHWkbnfx20GK1Q+BjmrAQ1EcBwYgTi40/wDI1xT5RUnMy6OorA0rURle1TXKY7Ymg84OOtQPSaXkAjDK83GdT9sggDdWsE/YNUnvlEei7MGtJMfaxwKuaUZvP709Y+rSGseezjk= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=Q92s2o0H; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="Q92s2o0H" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 9AB5D1F00893; Wed, 3 Jun 2026 01:18:10 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1780449490; bh=okhqras6djzjntRptKtbQJ5vPAW6UL5ek6DK6vJsEwM=; h=From:Subject:Reply-To:To:Cc:In-Reply-To:References:Date; b=Q92s2o0HO050OcSvE+UNJsgnKS+u6XtNVJmF02FpK/n7lG1Qf191KZ7rFYzhtfSjp eMO4LvGBT1epHy8K8KWRv3U6ZMJP/8uDgxyGlG3FImtSsDGwmp3S8vbRTcGN+lzJU+ VdZsA36hS8NhQFVgXcxSlbhKsPnquamECtBSZIKbe0OVLmeBu64ET8pUlWln4yxSn6 uQ5eDdKbp5jza+hJezCCGTJA+iekgOxDpmk2O9VZOjXQbwzcrTQd6Nvs2lm5cp3RKA BFyZOzD6KlaEZ2tqU0O1Z7f49oQmNTc5uA5EGR9kF2OhXVS3KCta7v6CaLFy4h6SF8 GICw0gzFCOC/g== From: sashiko-bot@kernel.org Subject: Re: [PATCH] watchdog/sama5d4: set platform driver data before requesting IRQ Reply-To: sashiko-reviews@lists.linux.dev To: "Rosen Penev" Cc: linux-watchdog@vger.kernel.org In-Reply-To: <20260603010731.230581-1-rosenp@gmail.com> References: <20260603010731.230581-1-rosenp@gmail.com> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Date: Wed, 03 Jun 2026 01:18:09 +0000 Message-Id: <20260603011810.9AB5D1F00893@smtp.kernel.org> Precedence: bulk X-Mailing-List: linux-watchdog@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Thank you for your contribution! Sashiko AI review found 3 potential issue(= s) to consider: Pre-existing issues: - [High] The shared interrupt handler unconditionally returns IRQ_HANDLED e= ven when it determines that the interrupt was not generated by the watchdog= device. - [High] The driver requests an interrupt using both IRQF_SHARED and IRQF_N= O_SUSPEND, which is an unsafe combination. - [High] The physical hardware timeout is unconditionally reset to a hardco= ded 16 seconds during initialization and resume, ignoring any configured de= vice tree or user space timeout. -- commit 7ce180f45b6396bc468527a9890ba1852c13eb65 Author: Rosen Penev watchdog/sama5d4: set platform driver data before requesting IRQ sama5d4_wdt_probe() requests a shared IRQ before calling platform_set_drvdata(). Because the IRQ is shared, the handler can fire immediately, causing a NULL pointer crash when reading the missing driver data. This patch moves the drvdata setup earlier. > diff --git a/drivers/watchdog/sama5d4_wdt.c b/drivers/watchdog/sama5d4_wd= t.c > index 704b786cc2ec6..dcf5b8ac9e0d2 100644 > --- a/drivers/watchdog/sama5d4_wdt.c > +++ b/drivers/watchdog/sama5d4_wdt.c > @@ -268,6 +268,7 @@ static int sama5d4_wdt_probe(struct platform_device *= pdev) > : AT91_WDT_WDDIS; > =20 > watchdog_set_drvdata(wdd, wdt); > + platform_set_drvdata(pdev, wdt); [Severity: High] This isn't a bug introduced by this patch, but since this change fixes a=20 crash in the shared IRQ handler, is it safe to request this IRQ with both=20 IRQF_SHARED and IRQF_NO_SUSPEND? According to the suspend documentation, IRQF_NO_SUSPEND affects the entire IRQ line, keeping it enabled during system suspend. When the line is shared, any interrupt triggered during suspend causes all sharing handlers to run: drivers/watchdog/sama5d4_wdt.c:sama5d4_wdt_probe() { ... ret =3D devm_request_irq(dev, irq, sama5d4_wdt_irq_handler, IRQF_SHARED | IRQF_IRQPOLL | IRQF_NO_SUSPEND, pdev->name, pdev); ... } Could other suspended devices sharing the line crash or cause bus faults if they are unprepared to handle interrupts while powered down? [Severity: High] This is a pre-existing issue, but how does the handler deal with spurious interrupts on this shared line? The code unconditionally returns IRQ_HANDLED even when the interrupt was not generated by the watchdog: drivers/watchdog/sama5d4_wdt.c:sama5d4_wdt_irq_handler() { ... if (reg) { pr_crit("Atmel Watchdog Software Reset\n"); emergency_restart(); pr_crit("Reboot didn't succeed\n"); } return IRQ_HANDLED; } Might this prevent the kernel's spurious interrupt detection from disabling the IRQ line during an interrupt storm, potentially leading to a hard lockup if a malfunctioning device shares the same line? > regs =3D devm_platform_ioremap_resource(pdev, 0); > if (IS_ERR(regs)) > @@ -316,8 +317,6 @@ static int sama5d4_wdt_probe(struct platform_device *= pdev) > if (ret) > return ret; > =20 > - platform_set_drvdata(pdev, wdt); > - > dev_info(dev, "initialized (timeout =3D %d sec, nowayout =3D %d)\n", > wdd->timeout, nowayout); [Severity: High] This is a pre-existing issue, but does the hardware timeout get unconditionally overwritten with the default 16 seconds? In sama5d4_wdt_init(), the hardware timeout is hardcoded to WDT_DEFAULT_TIMEOUT (16 seconds), which seems to ignore the configured wdd->timeout logged above: drivers/watchdog/sama5d4_wdt.c:sama5d4_wdt_init() { ... val =3D WDT_SEC2TICKS(WDT_DEFAULT_TIMEOUT); if (wdt->sam9x60_support) { wdt_write(wdt, AT91_SAM9X60_WLR, AT91_SAM9X60_SET_COUNTER(v= al)); } else { wdt->mr |=3D AT91_WDT_SET_WDD(WDT_SEC2TICKS(MAX_WDT_TIMEOUT= )); wdt->mr |=3D AT91_WDT_SET_WDV(val); } ... } Because sama5d4_wdt_init() is called unconditionally during both probe and sama5d4_wdt_resume_early(), will a custom timeout configured via device tree or userspace be lost? If a userspace daemon expects a 32s timeout and pings at 20s intervals, it seems the unexpected 16s hardware timeout would cause the system to unexpectedly reboot. --=20 Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260603010731.2305= 81-1-rosenp@gmail.com?part=3D1