On Fri, Jun 20, 2025 at 09:18:32PM +0200, Thomas Gleixner wrote:
> On Wed, Jun 18 2025 at 12:17, Lorenzo Pieralisi wrote:
> > drivers/of/irq.c | 21 +
> > drivers/pci/msi/irqdomain.c | 19 +
> > include/linux/msi.h | 5 +
> > include/linux/of_irq.h | 7 +
>
> This are preparatory changes. Please split them out into a seperate patch.
Done. I did not do it because some maintainers require changes to
be added in the patch(es) that uses them.
> > ...3-its-msi-parent.c => irq-gic-its-msi-parent.c} | 187 ++-
> > drivers/irqchip/irq-gic-its-msi-parent.h | 14 +
> > drivers/irqchip/irq-gic-v3-its.c | 3 +-
>
> Ditto, i.e. the rename and code move, not the v5 add ons.
Done.
> > +static bool its_v5_init_dev_msi_info(struct device *dev, struct irq_domain *domain,
> > + struct irq_domain *real_parent, struct msi_domain_info *info)
> > +{
> > + if (!msi_lib_init_dev_msi_info(dev, domain, real_parent, info))
> > + return false;
> > +
> > + switch (info->bus_token) {
> > + case DOMAIN_BUS_PCI_DEVICE_MSI:
> > + case DOMAIN_BUS_PCI_DEVICE_MSIX:
> > + /*
> > + * FIXME: This probably should be done after a (not yet
> > + * existing) post domain creation callback once to make
> > + * support for dynamic post-enable MSI-X allocations
> > + * work without having to reevaluate the domain size
> > + * over and over. It is known already at allocation
> > + * time via info->hwsize.
> > + *
> > + * That should work perfectly fine for MSI/MSI-X but needs
> > + * some thoughts for purely software managed MSI domains
> > + * where the index space is only limited artificially via
> > + * %MSI_MAX_INDEX.
>
>
> This comment is stale after Marc moved the prepare callback into
> the domain creation, where the prepare callback gets hwsize for scaling.
>
> The only valid caveat are software managed domains, where hwsize is
> unspecified, but that's a different problem (and not used as of today).
Right, I should have removed it. Done.
> > + */
> > + info->ops->msi_prepare = its_v5_pci_msi_prepare;
> > + info->ops->msi_teardown = its_msi_teardown;
> > + break;
> > + case DOMAIN_BUS_DEVICE_MSI:
> > + case DOMAIN_BUS_WIRED_TO_MSI:
> > + /*
> > + * FIXME: See the above PCI prepare comment. The domain
> > + * size is also known at domain creation time.
> > + */
>
> See above.
>
> > +void gicv5_irs_syncr(void)
> > +{
> > + struct gicv5_irs_chip_data *irs_data;
> > + u32 syncr;
> > +
> > + irs_data = list_first_entry_or_null(&irs_nodes,
> > + struct gicv5_irs_chip_data, entry);
>
> Let it stick out. You have 100 characters.
Done.
> > + if (WARN_ON(!irs_data))
>
> WARN_ON_ONCE() ?
Done.
>
> > +static unsigned int gicv5_its_l2sz_to_l2_bits(unsigned int sz)
> > +{
> > + switch (sz) {
> > + case GICV5_ITS_DT_ITT_CFGR_L2SZ_64k:
> > + return 13;
> > + case GICV5_ITS_DT_ITT_CFGR_L2SZ_16k:
> > + return 11;
> > + case GICV5_ITS_DT_ITT_CFGR_L2SZ_4k:
> > + default:
> > + return 9;
>
> Magic numbers pulled out of thin air?
I will add defines but I thought the function name would be enough to tag
the value with a meaning.
> > +static __le64 *gicv5_its_device_get_itte_ref(struct gicv5_its_dev *its_dev,
> > + u16 event_id)
> > +{
> > + unsigned int l1_idx, l2_idx, l2_bits;
> > + __le64 *l2_itt, *l1_itt;
> > +
> > + if (!its_dev->itt_cfg.l2itt) {
> > + __le64 *itt = its_dev->itt_cfg.linear.itt;
> > +
> > + return &itt[event_id];
> > + }
> > +
> > + l1_itt = its_dev->itt_cfg.l2.l1itt;
> > +
> > + l2_bits = gicv5_its_l2sz_to_l2_bits(its_dev->itt_cfg.l2.l2sz);
> > +
> > + l1_idx = event_id >> l2_bits;
> > +
> > + BUG_ON(!FIELD_GET(GICV5_ITTL1E_VALID, le64_to_cpu(l1_itt[l1_idx])));
>
> I assume this is truly unrecoverable
At this stage, we mapped an IRQ and we are activating it, this must not
happen (like any bug out there, granted). I can remove it, I understand
it bothers you adding BUG_ON(), if something goes wrong there we will
notice.
> > + l2_idx = event_id & GENMASK(l2_bits - 1, 0);
> > +
> > + l2_itt = its_dev->itt_cfg.l2.l2ptrs[l1_idx];
> > +
> > + return &l2_itt[l2_idx];
> > +}
> > +
> ....
> > +
> > + return 0;
> > +out_free_lpi:
>
> It's amazing. All the code has a gazillion of empty newlines, which just
> take up space and have no delimiting value. But on these error labels,
> you glue them right at the return statement (not only here, noticed that
> before).
You have a point here, will change it (and I will check other error
labels sites).
> > + gicv5_free_lpi(lpi);
> > +out_eventid:
> > + gicv5_its_free_eventid(its_dev, event_id_base, nr_irqs);
> > +
> > + return ret;
> > +}
>
> > + * Taken from msi_lib_irq_domain_select(). The only difference is that
> > + * we have to match the fwspec->fwnode parent against the domain->fwnode
> > + * in that in GICv5 the ITS domain is represented by the ITS fwnode but
> > + * the MSI controller (ie the ITS frames) are ITS child nodes.
> > + */
> > +static int gicv5_its_irq_domain_select(struct irq_domain *d, struct irq_fwspec *fwspec,
> > + enum irq_domain_bus_token bus_token)
> > +{
> > + const struct msi_parent_ops *ops = d->msi_parent_ops;
> > + u32 busmask = BIT(bus_token);
> > +
> > + if (!ops)
> > + return 0;
> > +
> > + if (fwnode_get_parent(fwspec->fwnode) != d->fwnode ||
> > + fwspec->param_count != 0)
> > + return 0;
>
> Just add a MSI flag and set it in parent_ops::required_flags and extend
> the lib with
>
> struct fwnode_handle *fwh;
>
> fwh = d->flags & MAGIC ? fwnode_get_parent(fwspec->fwnode) : fwspec->fwnode;
>
> No?
I did not know if this use case granted a new flag. You cleared my
concern.
> > diff --git a/drivers/irqchip/irq-gic-v5.c b/drivers/irqchip/irq-gic-v5.c
> > index c2e7ba7e38f7..4a0990f46358 100644
> > --- a/drivers/irqchip/irq-gic-v5.c
> > +++ b/drivers/irqchip/irq-gic-v5.c
> > @@ -57,12 +57,12 @@ static void release_lpi(u32 lpi)
> > ida_free(&lpi_ida, lpi);
> > }
> >
> > -static int gicv5_alloc_lpi(void)
> > +int gicv5_alloc_lpi(void)
> > {
> > return alloc_lpi();
> > }
> >
> > -static void gicv5_free_lpi(u32 lpi)
> > +void gicv5_free_lpi(u32 lpi)
> > {
> > release_lpi(lpi);
> > }
>
> Just make them global right away when you implement them. No point for
> this kind of churn.
Yes, hopefully someone will not point out that in the respective patch
the function could be static :).
Thanks a lot for your review.
Lorenzo
