From: Marc Zyngier <maz@kernel.org> To: Jianjun Wang <jianjun.wang@mediatek.com> Cc: Bjorn Helgaas <bhelgaas@google.com>, Rob Herring <robh+dt@kernel.org>, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, Ryder Lee <ryder.lee@mediatek.com>, Philipp Zabel <p.zabel@pengutronix.de>, "Matthias\ Brugger" <matthias.bgg@gmail.com>, <linux-pci@vger.kernel.org>, <linux-mediatek@lists.infradead.org>, <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, "Sj\ Huang" <sj.huang@mediatek.com>, <youlin.pei@mediatek.com>, <chuanjia.liu@mediatek.com>, <qizhong.cheng@mediatek.com>, <sin_jieyang@mediatek.com>, <drinkcat@chromium.org>, <Rex-BC.Chen@mediatek.com>, <anson.chuang@mediatek.com> Subject: Re: [v8,5/7] PCI: mediatek-gen3: Add MSI support Date: Wed, 10 Mar 2021 09:41:35 +0000 [thread overview] Message-ID: <87a6rbxs4w.wl-maz@kernel.org> (raw) In-Reply-To: <1615358929.25662.47.camel@mhfsdcap03> On Wed, 10 Mar 2021 06:48:49 +0000, Jianjun Wang <jianjun.wang@mediatek.com> wrote: > > > +static struct irq_chip mtk_msi_irq_chip = { > > > + .name = "MSI", > > > + .irq_enable = mtk_pcie_irq_unmask, > > > + .irq_disable = mtk_pcie_irq_mask, > > > > Same comment as for the previous patch: enable/disable serve no > > purpose here. > > Replied in the previous patch, the enable/disable callback is used when > the system suspend/resume. As I said, your suspend/resume should be self contained, and not rely on the irq subsystem to restore a viable state. [...] > > > @@ -408,6 +677,14 @@ static void mtk_pcie_irq_handler(struct irq_desc *desc) > > > generic_handle_irq(virq); > > > } > > > > > > + irq_bit = PCIE_MSI_SHIFT; > > > + for_each_set_bit_from(irq_bit, &status, PCIE_MSI_SET_NUM + > > > + PCIE_MSI_SHIFT) { > > > + mtk_pcie_msi_handler(port, irq_bit - PCIE_MSI_SHIFT); > > > + > > > + writel_relaxed(BIT(irq_bit), port->base + PCIE_INT_STATUS_REG); > > > > Isn't this write the same thing you have for EOI in the INTx case? > > While I could understand your description in that case (this is a > > resampling operation), I don't get what this does here. Either this is > > also an EOI, but your initial description doesn't make sense, or it is > > an Ack, and it should be moved to the right place. > > > > Which one is it? > > I think it should be an EOI which used to clear the interrupt status of > a single set in the PCIe intc field, maybe I should move it to the end > of the mtk_pcie_msi_handler() function. I doubt this is an EOI. If, as I suspect, it instructs the HW to clear the bit so that new pending bits can be recorded, it must take place *before* the interrupt is handled, or you may lose MSIs in the interval between the handling of the interrupt and the clearing of the pending bit. To satisfy this requirement, this should be an ACK, which is consistent with the way most MSI controllers such as this one work. > > +-----+ > | GIC | > +-----+ > ^ > | > port->irq > | > +-+-+-+-+-+-+-+-+ > |0|1|2|3|4|5|6|7| (PCIe intc) > +-+-+-+-+-+-+-+-+ > ^ ^ ^ > | | ... | > +-------+ +------+ +-----------+ > | | | > +-+-+---+--+--+ +-+-+---+--+--+ +-+-+---+--+--+ > |0|1|...|30|31| |0|1|...|30|31| |0|1|...|30|31| (MSI sets) > +-+-+---+--+--+ +-+-+---+--+--+ +-+-+---+--+--+ > ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ > | | | | | | | | | | | | (MSI vectors) > | | | | | | | | | | | | > > (MSI SET0) (MSI SET1) ... (MSI SET7) > > I would like to ask another question. In this interrupt architecture, we > cannot implement an affinity for PCIe interrupts, so we return a > negative value in the mtk_pcie_set_affinity callback as follows: > > +static int mtk_pcie_set_affinity(struct irq_data *data, > + const struct cpumask *mask, bool force) > +{ > + return -EINVAL; > +} > > But there will always be error logs when hotplug a CPU: > > ~ # echo 0 > /sys/devices/system/cpu/cpu1/online > [ 93.633059] IRQ255: set affinity failed(-22). > [ 93.633624] IRQ256: set affinity failed(-22). > [ 93.634222] CPU1: shutdown > [ 93.634586] psci: CPU1 killed (polled 0 ms) > > Or when the system suspends: > > ~ # echo mem > /sys/power/state > [ 93.635145] cpuhp: cpu_off cluster=0, cpu=1 > [ 169.835653] PM: suspend entry (deep) > [ 169.836717] Filesystems sync: 0.000 seconds > [ 169.837924] Freezing user space processes ... (elapsed 0.001 seconds) > done. > [ 169.839922] OOM killer disabled. > [ 169.840336] Freezing remaining freezable tasks ... (elapsed 0.001 > seconds) done. > [ 169.844715] Disabling non-boot CPUs ... > [ 169.846443] IRQ255: set affinity failed(-22). > [ 169.847002] IRQ256: set affinity failed(-22). > [ 169.847586] CPU2: shutdown > [ 169.847943] psci: CPU2 killed (polled 0 ms) > [ 169.848489] cpuhp: cpu_off cluster=0, cpu=2 > [ 169.850285] IRQ255: set affinity failed(-22). > [ 169.851369] IRQ256: set affinity failed(-22). > ... > > Sometimes this can cause misunderstandings to users, do we have a chance > to prevent this error log? No. This HW doesn't allow MSIs to be individually retargeted, and the kernel isn't happy about that. That's one of the many reasons why hiding MSIs behind a mux (or two in your case) is a *very bad idea*. Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ Linux-mediatek mailing list Linux-mediatek@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-mediatek
WARNING: multiple messages have this Message-ID (diff)
From: Marc Zyngier <maz@kernel.org> To: Jianjun Wang <jianjun.wang@mediatek.com> Cc: Bjorn Helgaas <bhelgaas@google.com>, Rob Herring <robh+dt@kernel.org>, Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>, Ryder Lee <ryder.lee@mediatek.com>, Philipp Zabel <p.zabel@pengutronix.de>, "Matthias\ Brugger" <matthias.bgg@gmail.com>, <linux-pci@vger.kernel.org>, <linux-mediatek@lists.infradead.org>, <devicetree@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <linux-arm-kernel@lists.infradead.org>, "Sj\ Huang" <sj.huang@mediatek.com>, <youlin.pei@mediatek.com>, <chuanjia.liu@mediatek.com>, <qizhong.cheng@mediatek.com>, <sin_jieyang@mediatek.com>, <drinkcat@chromium.org>, <Rex-BC.Chen@mediatek.com>, <anson.chuang@mediatek.com> Subject: Re: [v8,5/7] PCI: mediatek-gen3: Add MSI support Date: Wed, 10 Mar 2021 09:41:35 +0000 [thread overview] Message-ID: <87a6rbxs4w.wl-maz@kernel.org> (raw) In-Reply-To: <1615358929.25662.47.camel@mhfsdcap03> On Wed, 10 Mar 2021 06:48:49 +0000, Jianjun Wang <jianjun.wang@mediatek.com> wrote: > > > +static struct irq_chip mtk_msi_irq_chip = { > > > + .name = "MSI", > > > + .irq_enable = mtk_pcie_irq_unmask, > > > + .irq_disable = mtk_pcie_irq_mask, > > > > Same comment as for the previous patch: enable/disable serve no > > purpose here. > > Replied in the previous patch, the enable/disable callback is used when > the system suspend/resume. As I said, your suspend/resume should be self contained, and not rely on the irq subsystem to restore a viable state. [...] > > > @@ -408,6 +677,14 @@ static void mtk_pcie_irq_handler(struct irq_desc *desc) > > > generic_handle_irq(virq); > > > } > > > > > > + irq_bit = PCIE_MSI_SHIFT; > > > + for_each_set_bit_from(irq_bit, &status, PCIE_MSI_SET_NUM + > > > + PCIE_MSI_SHIFT) { > > > + mtk_pcie_msi_handler(port, irq_bit - PCIE_MSI_SHIFT); > > > + > > > + writel_relaxed(BIT(irq_bit), port->base + PCIE_INT_STATUS_REG); > > > > Isn't this write the same thing you have for EOI in the INTx case? > > While I could understand your description in that case (this is a > > resampling operation), I don't get what this does here. Either this is > > also an EOI, but your initial description doesn't make sense, or it is > > an Ack, and it should be moved to the right place. > > > > Which one is it? > > I think it should be an EOI which used to clear the interrupt status of > a single set in the PCIe intc field, maybe I should move it to the end > of the mtk_pcie_msi_handler() function. I doubt this is an EOI. If, as I suspect, it instructs the HW to clear the bit so that new pending bits can be recorded, it must take place *before* the interrupt is handled, or you may lose MSIs in the interval between the handling of the interrupt and the clearing of the pending bit. To satisfy this requirement, this should be an ACK, which is consistent with the way most MSI controllers such as this one work. > > +-----+ > | GIC | > +-----+ > ^ > | > port->irq > | > +-+-+-+-+-+-+-+-+ > |0|1|2|3|4|5|6|7| (PCIe intc) > +-+-+-+-+-+-+-+-+ > ^ ^ ^ > | | ... | > +-------+ +------+ +-----------+ > | | | > +-+-+---+--+--+ +-+-+---+--+--+ +-+-+---+--+--+ > |0|1|...|30|31| |0|1|...|30|31| |0|1|...|30|31| (MSI sets) > +-+-+---+--+--+ +-+-+---+--+--+ +-+-+---+--+--+ > ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ ^ > | | | | | | | | | | | | (MSI vectors) > | | | | | | | | | | | | > > (MSI SET0) (MSI SET1) ... (MSI SET7) > > I would like to ask another question. In this interrupt architecture, we > cannot implement an affinity for PCIe interrupts, so we return a > negative value in the mtk_pcie_set_affinity callback as follows: > > +static int mtk_pcie_set_affinity(struct irq_data *data, > + const struct cpumask *mask, bool force) > +{ > + return -EINVAL; > +} > > But there will always be error logs when hotplug a CPU: > > ~ # echo 0 > /sys/devices/system/cpu/cpu1/online > [ 93.633059] IRQ255: set affinity failed(-22). > [ 93.633624] IRQ256: set affinity failed(-22). > [ 93.634222] CPU1: shutdown > [ 93.634586] psci: CPU1 killed (polled 0 ms) > > Or when the system suspends: > > ~ # echo mem > /sys/power/state > [ 93.635145] cpuhp: cpu_off cluster=0, cpu=1 > [ 169.835653] PM: suspend entry (deep) > [ 169.836717] Filesystems sync: 0.000 seconds > [ 169.837924] Freezing user space processes ... (elapsed 0.001 seconds) > done. > [ 169.839922] OOM killer disabled. > [ 169.840336] Freezing remaining freezable tasks ... (elapsed 0.001 > seconds) done. > [ 169.844715] Disabling non-boot CPUs ... > [ 169.846443] IRQ255: set affinity failed(-22). > [ 169.847002] IRQ256: set affinity failed(-22). > [ 169.847586] CPU2: shutdown > [ 169.847943] psci: CPU2 killed (polled 0 ms) > [ 169.848489] cpuhp: cpu_off cluster=0, cpu=2 > [ 169.850285] IRQ255: set affinity failed(-22). > [ 169.851369] IRQ256: set affinity failed(-22). > ... > > Sometimes this can cause misunderstandings to users, do we have a chance > to prevent this error log? No. This HW doesn't allow MSIs to be individually retargeted, and the kernel isn't happy about that. That's one of the many reasons why hiding MSIs behind a mux (or two in your case) is a *very bad idea*. Thanks, M. -- Without deviation from the norm, progress is not possible. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2021-03-10 9:42 UTC|newest] Thread overview: 111+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-02-24 6:11 [v8,0/7] PCI: mediatek: Add new generation controller support Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang 2021-02-24 6:11 ` [v8,1/7] dt-bindings: PCI: mediatek-gen3: Add YAML schema Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang 2021-03-06 20:09 ` Rob Herring 2021-03-06 20:09 ` Rob Herring 2021-03-06 20:09 ` Rob Herring 2021-02-24 6:11 ` [v8,2/7] PCI: Export pci_pio_to_address() for module use Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang 2021-02-24 6:11 ` [v8,3/7] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192 Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang 2021-02-24 13:36 ` Krzysztof Wilczyński 2021-02-24 13:36 ` Krzysztof Wilczyński 2021-02-24 13:36 ` Krzysztof Wilczyński 2021-02-25 3:07 ` Jianjun Wang 2021-02-25 3:07 ` Jianjun Wang 2021-02-25 3:07 ` Jianjun Wang 2021-03-11 12:38 ` Pali Rohár 2021-03-11 12:38 ` Pali Rohár 2021-03-11 12:38 ` Pali Rohár 2021-03-13 7:43 ` Jianjun Wang 2021-03-13 7:43 ` Jianjun Wang 2021-03-13 7:43 ` Jianjun Wang 2021-03-18 0:02 ` Pali Rohár 2021-03-18 0:02 ` Pali Rohár 2021-03-18 0:02 ` Pali Rohár 2021-03-18 5:48 ` Jianjun Wang 2021-03-18 5:48 ` Jianjun Wang 2021-03-18 5:48 ` Jianjun Wang 2021-03-19 18:53 ` Pali Rohár 2021-03-19 18:53 ` Pali Rohár 2021-03-19 18:53 ` Pali Rohár 2021-03-23 1:31 ` Jianjun Wang 2021-03-23 1:31 ` Jianjun Wang 2021-03-23 1:31 ` Jianjun Wang 2021-03-23 14:51 ` Pali Rohár 2021-03-23 14:51 ` Pali Rohár 2021-03-23 14:51 ` Pali Rohár 2021-03-29 22:58 ` Pali Rohár 2021-03-29 22:58 ` Pali Rohár 2021-03-29 22:58 ` Pali Rohár 2021-02-24 6:11 ` [v8,4/7] PCI: mediatek-gen3: Add INTx support Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang 2021-02-24 14:24 ` Krzysztof Wilczyński 2021-02-24 14:24 ` Krzysztof Wilczyński 2021-02-24 14:24 ` Krzysztof Wilczyński 2021-02-25 3:10 ` Jianjun Wang 2021-02-25 3:10 ` Jianjun Wang 2021-02-25 3:10 ` Jianjun Wang 2021-03-09 11:10 ` Marc Zyngier 2021-03-09 11:10 ` Marc Zyngier 2021-03-09 11:10 ` Marc Zyngier 2021-03-10 3:05 ` Jianjun Wang 2021-03-10 3:05 ` Jianjun Wang 2021-03-10 3:05 ` Jianjun Wang 2021-03-10 9:29 ` Marc Zyngier 2021-03-10 9:29 ` Marc Zyngier 2021-02-24 6:11 ` [v8,5/7] PCI: mediatek-gen3: Add MSI support Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang 2021-02-24 14:31 ` Krzysztof Wilczyński 2021-02-24 14:31 ` Krzysztof Wilczyński 2021-02-24 14:31 ` Krzysztof Wilczyński 2021-02-25 3:09 ` Jianjun Wang 2021-02-25 3:09 ` Jianjun Wang 2021-02-25 3:09 ` Jianjun Wang 2021-03-09 11:23 ` Marc Zyngier 2021-03-09 11:23 ` Marc Zyngier 2021-03-09 11:23 ` Marc Zyngier 2021-03-10 6:48 ` Jianjun Wang 2021-03-10 6:48 ` Jianjun Wang 2021-03-10 6:48 ` Jianjun Wang 2021-03-10 9:41 ` Marc Zyngier [this message] 2021-03-10 9:41 ` Marc Zyngier 2021-03-11 9:47 ` Jianjun Wang 2021-03-11 9:47 ` Jianjun Wang 2021-03-11 9:47 ` Jianjun Wang 2021-03-12 9:14 ` Marc Zyngier 2021-03-12 9:14 ` Marc Zyngier 2021-03-11 0:05 ` Pali Rohár 2021-03-11 0:05 ` Pali Rohár 2021-03-11 0:05 ` Pali Rohár 2021-03-11 8:19 ` Marc Zyngier 2021-03-11 8:19 ` Marc Zyngier 2021-03-11 8:19 ` Marc Zyngier 2021-03-11 9:50 ` Jianjun Wang 2021-03-11 9:50 ` Jianjun Wang 2021-03-11 9:50 ` Jianjun Wang 2021-02-24 6:11 ` [v8,6/7] PCI: mediatek-gen3: Add system PM support Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang 2021-02-24 14:10 ` Krzysztof Wilczyński 2021-02-24 14:10 ` Krzysztof Wilczyński 2021-02-24 14:10 ` Krzysztof Wilczyński 2021-02-25 3:34 ` Jianjun Wang 2021-02-25 3:34 ` Jianjun Wang 2021-02-25 3:34 ` Jianjun Wang 2021-02-25 22:00 ` Krzysztof Wilczyński 2021-02-25 22:00 ` Krzysztof Wilczyński 2021-02-25 22:00 ` Krzysztof Wilczyński 2021-02-26 10:06 ` Jianjun Wang 2021-02-26 10:06 ` Jianjun Wang 2021-02-26 10:06 ` Jianjun Wang 2021-02-24 6:11 ` [v8,7/7] MAINTAINERS: Add Jianjun Wang as MediaTek PCI co-maintainer Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang 2021-02-24 6:11 ` Jianjun Wang
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=87a6rbxs4w.wl-maz@kernel.org \ --to=maz@kernel.org \ --cc=Rex-BC.Chen@mediatek.com \ --cc=anson.chuang@mediatek.com \ --cc=bhelgaas@google.com \ --cc=chuanjia.liu@mediatek.com \ --cc=devicetree@vger.kernel.org \ --cc=drinkcat@chromium.org \ --cc=jianjun.wang@mediatek.com \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mediatek@lists.infradead.org \ --cc=linux-pci@vger.kernel.org \ --cc=lorenzo.pieralisi@arm.com \ --cc=matthias.bgg@gmail.com \ --cc=p.zabel@pengutronix.de \ --cc=qizhong.cheng@mediatek.com \ --cc=robh+dt@kernel.org \ --cc=ryder.lee@mediatek.com \ --cc=sin_jieyang@mediatek.com \ --cc=sj.huang@mediatek.com \ --cc=youlin.pei@mediatek.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.