linux-kernel - Re: [v5,2/3] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CANMq1KB3UXg8QKwuv3mFaodx-s_AXSrOWp6C+RN7LaA69nsTyg@mail.gmail.com>
Date:   Tue, 22 Dec 2020 11:55:47 +0800
From:   Nicolas Boichat <drinkcat@...omium.org>
To:     Jianjun Wang <jianjun.wang@...iatek.com>
Cc:     youlin.pei@...iatek.com,
        Devicetree List <devicetree@...r.kernel.org>,
        Lorenzo Pieralisi <lorenzo.pieralisi@....com>,
        qizhong.cheng@...iatek.com,
        Chuanjia Liu <chuanjia.liu@...iatek.com>,
        Mauro Carvalho Chehab <mchehab+huawei@...nel.org>,
        linux-pci@...r.kernel.org, Ryder Lee <ryder.lee@...iatek.com>,
        lkml <linux-kernel@...r.kernel.org>,
        Matthias Brugger <matthias.bgg@...il.com>,
        Sj Huang <sj.huang@...iatek.com>,
        Rob Herring <robh+dt@...nel.org>,
        "moderated list:ARM/Mediatek SoC support" 
        <linux-mediatek@...ts.infradead.org>,
        Philipp Zabel <p.zabel@...gutronix.de>,
        Bjorn Helgaas <bhelgaas@...gle.com>, sin_jieyang@...iatek.com,
        "David S . Miller" <davem@...emloft.net>,
        linux-arm Mailing List <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [v5,2/3] PCI: mediatek-gen3: Add MediaTek Gen3 driver for MT8192

On Tue, Dec 22, 2020 at 11:38 AM Jianjun Wang <jianjun.wang@...iatek.com> wrote:
>
> On Mon, 2020-12-21 at 10:18 +0800, Nicolas Boichat wrote:
> > On Wed, Dec 2, 2020 at 9:39 PM Jianjun Wang <jianjun.wang@...iatek.com> wrote:
> > > [snip]
> > > +static irq_hw_number_t mtk_pcie_msi_get_hwirq(struct msi_domain_info *info,
> > > +                                             msi_alloc_info_t *arg)
> > > +{
> > > +       struct msi_desc *entry = arg->desc;
> > > +       struct mtk_pcie_port *port = info->chip_data;
> > > +       int hwirq;
> > > +
> > > +       mutex_lock(&port->lock);
> > > +
> > > +       hwirq = bitmap_find_free_region(port->msi_irq_in_use, PCIE_MSI_IRQS_NUM,
> > > +                                       order_base_2(entry->nvec_used));
> > > +       if (hwirq < 0) {
> > > +               mutex_unlock(&port->lock);
> > > +               return -ENOSPC;
> > > +       }
> > > +
> > > +       mutex_unlock(&port->lock);
> > > +
> > > +       return hwirq;
> >
> > Code is good, but I had to look twice to make sure the mutex is
> > unlocked. Is the following marginally better?
> >
> > hwirq = ...;
> >
> > mutex_unlock(&port->lock);
> >
> > if (hwirq < 0)
> >    return -ENOSPC;
> >
> > return hwirq;
>
> Impressive, I will fix it in the next version, and I think the hwirq can
> be returned directly since it will be a negative value if
> bitmap_find_free_region is failed. The code will be like the following:
>
> hwirq = ...;
>
> mutex_unlock(&port->lock);
>
> return hwirq;

SG, as long as you're okay with returning -ENOMEM instead of -ENOSPC.

But now I'm having doubt if negative return values are ok, as
irq_hw_number_t is unsigned long.

msi_domain_alloc
(https://elixir.bootlin.com/linux/latest/source/kernel/irq/msi.c#L143)
uses it to call irq_find_mapping
(https://elixir.bootlin.com/linux/latest/source/kernel/irq/irqdomain.c#L882)
without check, and I'm not convinced irq_find_mapping will error out
gracefully...

> >
> > > +}
> > > +
> > > [snip]
> > > +static void mtk_pcie_msi_handler(struct irq_desc *desc)
> > > +{
> > > +       struct mtk_pcie_msi *msi_info = irq_desc_get_handler_data(desc);
> > > +       struct irq_chip *irqchip = irq_desc_get_chip(desc);
> > > +       unsigned long msi_enable, msi_status;
> > > +       unsigned int virq;
> > > +       irq_hw_number_t bit, hwirq;
> > > +
> > > +       chained_irq_enter(irqchip, desc);
> > > +
> > > +       msi_enable = readl(msi_info->base + PCIE_MSI_ENABLE_OFFSET);
> > > +       while ((msi_status = readl(msi_info->base + PCIE_MSI_STATUS_OFFSET))) {
> > > +               msi_status &= msi_enable;
> >
> > I don't know much about MSI, but what happens if you have a bit that
> > is set in PCIE_MSI_STATUS_OFFSET register, but not in msi_enable?
>
> If the bit that in PCIE_MSI_STATUS_OFFSET register is set but not in
> msi_enable, it must be an abnormal usage of MSI or something goes wrong,
> it should be ignored in case we can not find the corresponding handler.
>
> > Sounds like you'll just spin-loop forever without acknowledging the
> > interrupt.
>
> The interrupt will be acknowledged in the irq_ack callback of
> mtk_msi_irq_chip, which belongs to the msi_domain.

Let's try to go through it (and please explain to me if I get this wrong).

Say we have:

msi_enable = [PCIE_MSI_ENABLE_OFFSET] = 0x1;

while loop:

msi_status = [PCIE_MSI_STATUS_OFFSET] = 0x3;
msi_status &= msi_enable => msi_status = 0x3 & 0x1 = 0x1;
for_each_set_bit(msi_status) {
   do something that presumably will disable the MSI interrupt status?
}
(next loop iteration)

msi_status = [PCIE_MSI_STATUS_OFFSET] = 0x2;
msi_status &= msi_enable => msi_status = 0x2 & 0x1 = 0x0;
for_each_set_bit(msi_status) => does nothing.

msi_status = [PCIE_MSI_STATUS_OFFSET] = 0x2;
(infinite loop)

Basically, I'm wondering if you should replace the while condition
statement with:

while ((msi_status = readl(msi_info->base + PCIE_MSI_STATUS_OFFSET) &
msi_enable))