On Wed, 14 Nov 2018 19:19:27 +0000,
Trent Piepho <tpiepho@xxxxxxxxxx> wrote:
>
> On Wed, 2018-11-14 at 09:54 +0000, Marc Zyngier wrote:
> > /* Initialize IRQ Status array */
> > - for (ctrl = 0; ctrl < num_ctrls; ctrl++)
> > - dw_pcie_rd_own_conf(pp, PCIE_MSI_INTR0_ENABLE +
> > + for (ctrl = 0; ctrl < num_ctrls; ctrl++) {
> > + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_MASK +
> > (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
> > - 4, &pp->irq_status[ctrl]);
> > + 4, ~0);
> > + dw_pcie_wr_own_conf(pp, PCIE_MSI_INTR0_ENABLE +
> > + (ctrl * MSI_REG_CTRL_BLOCK_SIZE),
> > + 4, ~0);
> > + pp->irq_status[ctrl] = 0;
> > + }
> >
>
> I tested yesterday before this patch was sent and fixed this issue
> another way. I pretty sure this would work as well, though it's not
> clear to me it's more correct.
Given that we don't have a spec or any form of useful documentation
(except for the information that Gustavo gave us), I don't think
*anything* we'll write here has a remote chance of being
correct. We're simply poking in the dark.
> It looks to me that the previous code was using whatever MSIs were
> already enabled when the driver is initialized as the set to leave
> enabled.
And I claim that this is a gross bug. We don't want to inherit
anything at all, rather start from a fresh start.
> This looks like it changing that behavior, and instead enabling all
> MSIs on initialization.
>
> I would think the default value for a MSI should be disabled until
> something enables it.
Sure, that's no big deal. We can plug in the enable/disable callbacks
to that effect, although we'll end-up with similar result, I'd expect.
> I speculated that the previous behavior was trying to work with an MSI
> enabled by the bootloader, ACPI firmware, etc. that should be left
> alone. Or perhaps there was no good reason not to disable everything
> on initialization and that code just got copied from somewhere else and
> no one thought about it. There's certainly evidence of that in this
> driver.
As you said, you're speculating. Nonetheless, there is no reason to
start with anything enabled the first place.
M.
--
Jazz is not dead, it just smell funny.
