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 9806E3264EB for ; Thu, 4 Jun 2026 15:59:01 +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=1780588742; cv=none; b=Pw0cXotWyOhHC95FZf57Ysjk+b9+dQtAipaDbuVVjCr5OMRqSPsG0GniUjzwojgGAxnQ0/5lcD5f8bKZwUbK2J7USQxmdXoUlj0VZXdFPjVD4Ee1/Ot4AYanWg35VyqlQJP9NmWmf475HT3BbxsMscy8ue/3bwUv2JZgr3Rr2Uc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1780588742; c=relaxed/simple; bh=Hk62a6+RhYCYZTy+i42raPLZEII3FRzjH7VTZbZaQ8M=; h=From:To:Cc:Subject:In-Reply-To:References:Date:Message-ID: MIME-Version:Content-Type; b=OFSmViwS7H0oPx9z/S91nlJEnUUrldzM1M0stfjSlG+nHnytmhzemJggWVLpt3iaK2rL0xd0RKDWoU0se+ZDxZPCKexKZHXtAX2OJI4BVCAvRYqZ4j679jqslgDlNdfWcajLKoJLSmKDPp+VqbxXdeEcMM3a9QuqHaw47cXd55M= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=XrVH+Ifg; 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="XrVH+Ifg" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8920F1F00893; Thu, 4 Jun 2026 15:59:00 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1780588741; bh=sTxNYSkkkPXmp8R713GvIPtFLUqJ3M84MQtGguERJH4=; h=From:To:Cc:Subject:In-Reply-To:References:Date; b=XrVH+IfggHdJmDiRKfddR5QuziW1FT4klxvxrCyNk5A3LO57XtX5yRwU6u47Hc8S6 Q6pAUtQhQ0maGy2KHvW4xF6RTRUrlKmorWMVRCwt//0hZ+MfEcEKyL0N0iwWZcn9Ee OB/Su1ixwHlJiUNkQPHw3D6RrTyDPqsvGYxZRyCyljl4geyRRl6EqQqvrH1EsEJV9e yfcaR1A3gjnQzT98/gdX7lu1uScWm7XdMfCmnTVzGZGstZbai3tmw5Fc7ZOzPgJ/se Y/3W/lKZSoig1QZjmsJN6v27UZaQE5w+yQ7legA4YSdbw2ItNRGjeXi7oKpdVN19BQ sb5jVGPQH/Thg== From: Thomas Gleixner To: Markus Stockhausen , linux-kernel@vger.kernel.org Cc: Markus Stockhausen Subject: Re: [PATCH v2 2/2] irqchip/irq-realtek-rtl: Add multicore support In-Reply-To: <20260604130627.184242-3-markus.stockhausen@gmx.de> References: <20260604130627.184242-1-markus.stockhausen@gmx.de> <20260604130627.184242-3-markus.stockhausen@gmx.de> Date: Thu, 04 Jun 2026 17:58:57 +0200 Message-ID: <874iji5mbi.ffs@fw13> Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain On Thu, Jun 04 2026 at 15:06, Markus Stockhausen wrote: > static void realtek_ictl_unmask_irq(struct irq_data *i) > { > + unsigned int cpu; > + > guard(raw_spinlock)(&irq_lock); > - enable_gimr(i->hwirq); > + for_each_cpu(cpu, irq_data_get_effective_affinity_mask(i)) > + enable_gimr(cpu, i->hwirq); > } Ok. > static void realtek_ictl_mask_irq(struct irq_data *i) > { > + unsigned int cpu; > + > guard(raw_spinlock)(&irq_lock); > - disable_gimr(i->hwirq); > + for_each_cpu(cpu, &realtek_ictl_cpu_configurable) > + disable_gimr(cpu, i->hwirq); > +} Why not using the effective mask here? CPUs which are not in the effective mask are disabled already. > +static int realtek_ictl_irq_affinity(struct irq_data *i, const struct cpumask *dest, bool force) > +{ > + cpumask_t cpu_configure, cpu_disable, cpu_enable; > + unsigned int cpu; Looking deeper at this specific part: > + cpumask_and(&cpu_configure, cpu_present_mask, &realtek_ictl_cpu_configurable); Why do you need that? cpu_configurable should arguably never contain non-present CPUs. If you want to protect against a bonkers device tree, then do so in the probe function. But .... > + cpumask_and(&cpu_enable, &cpu_configure, dest); > + cpumask_andnot(&cpu_disable, &cpu_configure, dest); Assume that cpu_configure and dest do not overlap, then you end up with _zero_ target CPUs, i.e. a non-working interrupt. The general asssumption of the core interrupt code is that except for strict per CPU interrupts, which are managed separately, interrupts can be routed to any online CPU. So I think your init logic is broken: > + realtek_ictl_base[cpu] = of_iomap(node, cpu); > + if (realtek_ictl_base[cpu]) { If this mapping fails, then you should fail the initialization and your loop around that should do: for_each_present_cpu() and not try to figure that out via NR_CPUS: > + for (unsigned int cpu = 0; cpu < NR_CPUS; cpu++) { and the failed mapping logic. At the point where the driver is initialized the present CPU mask is stable and authoritative. If a mapping for a present CPU fails, then the driver has to fail the initialization as you otherwise run into the stale interrupt situation described above. With that fixed you can drop this whole realtek_ictl_cpu_configurable dance as the core will never set a non-present CPU in the destination mask. It even guarantees that the CPUs in the mask are online unless @force = true. The latter is only for scenarios where pseudo per CPU interrupts have to be affined before a CPU goes online, so irrelevant for your use case. > + scoped_guard(raw_spinlock, &irq_lock) { > + for_each_cpu(cpu, &cpu_disable) > + disable_gimr(cpu, i->hwirq); > + for_each_cpu(cpu, &cpu_enable) { > + if (!irqd_irq_masked(i)) > + enable_gimr(cpu, i->hwirq); > + } > + } This can be simplified to: if (!irqd_irq_masked(i)) realtek_ictl_mask_irq(i); irq_data_update_effective_affinity(i, &dest); if (!irqd_irq_masked(i)) realtek_ictl_unmask_irq(i); Sorry that I failed to catch those things right away. Thanks, tglx