On Wed, Jul 29, 2015 at 5:15 PM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote: > > On 29/07/15 09:52, Ley Foon Tan wrote: > > On Wed, Jul 29, 2015 at 1:58 AM, Marc Zyngier <marc.zyngier@xxxxxxx> wrote: > >> Hi Ley, > >> > >> On 28/07/15 11:45, Ley Foon Tan wrote: > >>> This patch adds Altera PCIe MSI driver. This soft IP supports configurable > >>> number of vectors, which is a dts parameter. > >> > >> Can't you read this configuration from the HW? > > No, we can't read from HW. > > Ah, that's a shame. Specially on HW that is configurable by design. > > [...] > > >>> + > >>> + irq = irq_find_mapping(msi->msi_domain->parent, offset); > >> > >> This would tend to indicate that you don't really need to store the > >> msi_domain pointer, but the inner_domain pointer instead. > > Will store the inner_domain pointer. But, I think we still need > > msi_domain for irq_domain_remove. > > Or do we have any way to retrieve msi_domain from inner_domain? > > Do you have any case where you remove the domains, aside from the > obvious error cases? Yes, if there is error in _probe(). > [...] > > >>> + > >>> +static struct msi_domain_info altera_msi_domain_info = { > >>> + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS), > >> > >> So you don't support MSIX? That's a bit weird. > > Yes, this MSI IP doesn't support it. > > This is not really a function of the MSI IP, but of the PCI device. In > your case, this is just a set of doorbells, so I hardly see what would > prevent MSI-X to be supported. Multi-MSI, I can see why. Yes, you are right. Will add MSI_FLAG_PCI_MSIX flag here. > > [...] > > >>> +static int altera_msi_set_affinity(struct irq_data *irq_data, > >>> + const struct cpumask *mask, bool force) > >>> +{ > >>> + return irq_set_affinity(irq_data->hwirq, mask); > >> > >> There is no way this can be right. irq_data->hwirq can *never* be passed > >> as a Linux IRQ. This really should be the IRQ to the GIC. > >> > >> Which raises another issue: as you only have a single interrupt to the > >> GIC, changing the affinity of a single MSI is going to affect all the > >> other MSIs as well. This doesn't seem like a desirable behaviour. > > Do we must provide '.irq_set_affinity" callback to msi domain? I have > > tried set it to NULL, > > but kernel got the NULL pointer deference error from msi_domain_set_affinity(). > > Recently, I saw this new patch for pci-tegra.c from [1], it doesn't > > set the ".irq_set_affinity". > > Just wonder how it can work. > > > > Do you have any recommendation way for this? > > > > [1] https://git.kernel.org/cgit/linux/kernel/git/maz/arm-platforms.git/commit/drivers/pci/host?h=irq/gsi-irq-domain-v2&id=17c22fc4e60e6ad54c7e3b73868cbc057360fa63 > > Please realize that I *never* tested this patch (I don't think I ever > posted it officially, I just keep here for convenience), and I wouldn't > take it as a reference. > > When it comes to msi_domain_set_affinity issue, this look more like an > oversight. I'll cook a patch for that. > > Anyway, whichever way you want to do it, you need to fix this. You could > always return -EINVAL in the meantime, Will add -EINVAL for now. Thanks for reviewing. Regards Ley Foon -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html