Re: [RFC PATCH v3 21/37] irqchip: Add irq-kvx-apic-mailbox driver

Thomas Gleixner
2024-07-27T13:35:31Z

On Mon, Jul 22 2024 at 11:41, ysionneau@kalrayinc.com wrote:
> +/**
> + * struct kvx_apic_mailbox - kvx apic mailbox
> + * @base: base address of the controller
> + * @device_domain: IRQ device domain for mailboxes
> + * @msi_domain: platform MSI domain for MSI interface
> + * @domain_info: Domain information needed for the MSI domain
> + * @mb_count: Count of mailboxes we are handling
> + * @available: bitmap of availables bits in mailboxes
> + * @mailboxes_lock: lock for irq migration
> + * @mask_lock: lock for irq masking
> + * @mb_data: data associated to each mailbox
> + */
> +struct kvx_apic_mailbox {
> + void __iomem *base;
> + phys_addr_t phys_base;
> + struct irq_domain *device_domain;
> + struct irq_domain *msi_domain;
> + struct msi_domain_info domain_info;
> + /* Start and count of device mailboxes */

Please put the relevant information into the kernel doc above.

> + unsigned int mb_count;
> + /* Bitmap of allocated bits in mailboxes */

and refrain from contradicting information. What is this bitmap for:

available bits as claimed above or allocated bits as claimed here?

If the latter then the name 'available' is just wrong.

> + DECLARE_BITMAP(available, MAILBOXES_MAX_BIT_COUNT);

> +static void kvx_mailbox_set_irq_enable(struct irq_data *data,
> + bool enabled)
> +{
> + struct kvx_irq_data *kd = irq_data_get_irq_chip_data(data);
> + struct kvx_apic_mailbox *mb = kd->mb;
> + unsigned int mb_num, mb_bit;
> + void __iomem *mb_addr;
> + u64 mask_value, mb_value;
> +
> + kvx_mailbox_get_from_hwirq(irqd_to_hwirq(data), &mb_num, &mb_bit);
> + mb_addr = kvx_mailbox_get_addr(mb, mb_num);
> +
> + raw_spin_lock(&mb->mask_lock);

guard()

> + mask_value = readq(mb_addr + KVX_MAILBOX_MASK_OFFSET);
> + if (enabled)
> + mask_value |= BIT_ULL(mb_bit);
> + else
> + mask_value &= ~BIT_ULL(mb_bit);
> +
> + writeq(mask_value, mb_addr + KVX_MAILBOX_MASK_OFFSET);
> +
> + raw_spin_unlock(&mb->mask_lock);
> +
> + /**

No kernel doc opener on regular comments please.

> + * Since interrupts on mailboxes are edge triggered and are only
> + * triggered when writing the value, we need to trigger it manually
> + * after updating the mask if enabled. If the interrupt was triggered by
> + * the device just after the mask write, we can trigger a spurious
> + * interrupt but that is still better than missing one...
> + * Moreover, the mailbox is configured in OR mode which means that even
> + * if we write a single bit, all other bits will be kept intact.
> + */
> + if (enabled) {
> + mb_value = readq(mb_addr + KVX_MAILBOX_VALUE_OFFSET);
> + if (mb_value & BIT_ULL(mb_bit))
> + writeq(BIT_ULL(mb_bit),
> + mb_addr + KVX_MAILBOX_VALUE_OFFSET);
> + }
> +}

> +static void kvx_mailbox_free_bit(struct kvx_apic_mailbox *mb, int hw_irq)
> +{
> + unsigned int mb_num, mb_bit;
> +
> + kvx_mailbox_get_from_hwirq(hw_irq, &mb_num, &mb_bit);
> + bitmap_clear(mb->available, hw_irq, 1);
> +
> + /* If there is no more IRQ on this mailbox, reset it to CPU 0 */
> + if (mb->available[mb_num] == 0)
> + kvx_mailbox_set_cpu(mb, mb_num, 0);
> +}
> +
> +struct irq_chip kvx_apic_mailbox_irq_chip = {
> + .name = "kvx apic mailbox",
> + .irq_compose_msi_msg = kvx_mailbox_msi_compose_msg,
> + .irq_mask = kvx_mailbox_mask,
> + .irq_unmask = kvx_mailbox_unmask,
> +};
> +
> +static int kvx_mailbox_allocate_bits(struct kvx_apic_mailbox *mb, int num_req)
> +{
> + int first, align_mask = 0;
> +
> + /* This must be a power of 2 for bitmap_find_next_zero_area to work */
> + BUILD_BUG_ON((MAILBOXES_BITS_PER_PAGE & (MAILBOXES_BITS_PER_PAGE - 1)));
> +
> + /*
> + * If user requested more than 1 mailbox, we must make sure it will be
> + * aligned on a page size for iommu_dma_prepare_msi to be correctly
> + * mapped in a single page.
> + */
> + if (num_req > 1)
> + align_mask = (MAILBOXES_BITS_PER_PAGE - 1);
> +
> + spin_lock(&mb->mailboxes_lock);

guard()

> + first = bitmap_find_next_zero_area(mb->available,
> + mb->mb_count * MAILBOXES_BIT_SIZE, 0,
> + num_req, align_mask);
> + if (first >= MAILBOXES_MAX_BIT_COUNT) {
> + spin_unlock(&mb->mailboxes_lock);
> + return -ENOSPC;
> + }
> +
> + bitmap_set(mb->available, first, num_req);
> +
> + spin_unlock(&mb->mailboxes_lock);
> +
> + return first;
> +}
> +
> +static int kvx_apic_mailbox_msi_alloc(struct irq_domain *domain,
> + unsigned int virq,
> + unsigned int nr_irqs, void *args)
> +{
> + int i, err;
> + int hwirq = 0;
> + u64 mb_addr;
> + struct irq_data *d;
> + struct kvx_irq_data *kd;
> + struct kvx_apic_mailbox *mb = domain->host_data;
> + struct msi_alloc_info *msi_info = (struct msi_alloc_info *)args;
> + struct msi_desc *desc = msi_info->desc;
> + unsigned int mb_num, mb_bit;
> +
> + /* We will not be able to guarantee page alignment ! */
> + if (nr_irqs > MAILBOXES_BITS_PER_PAGE)
> + return -EINVAL;
> +
> + hwirq = kvx_mailbox_allocate_bits(mb, nr_irqs);
> + if (hwirq < 0)
> + return hwirq;
> +
> + kvx_mailbox_get_from_hwirq(hwirq, &mb_num, &mb_bit);
> + mb_addr = (u64) kvx_mailbox_get_phys_addr(mb, mb_num);
> + err = iommu_dma_prepare_msi(desc, mb_addr);
> + if (err)
> + goto free_mb_bits;
> +
> + for (i = 0; i < nr_irqs; i++) {
> + kd = kmalloc(sizeof(*kd), GFP_KERNEL);
> + if (!kd) {
> + err = -ENOMEM;
> + goto free_irq_data;
> + }
> +
> + kd->mb = mb;
> + irq_domain_set_info(domain, virq + i, hwirq + i,
> + &kvx_apic_mailbox_irq_chip,
> + kd, handle_simple_irq,
> + NULL, NULL);
> + }
> +
> + return 0;
> +
> +free_irq_data:
> + for (i--; i >= 0; i--) {
> + d = irq_domain_get_irq_data(domain, virq + i);
> + kd = irq_data_get_irq_chip_data(d);
> + kfree(kd);
> + }
> +
> +free_mb_bits:
> + spin_lock(&mb->mailboxes_lock);
> + bitmap_clear(mb->available, hwirq, nr_irqs);
> + spin_unlock(&mb->mailboxes_lock);

kvx_mailbox_free_bit() does some magic vs. the mailbox and reset to
CPU0. Why is this not required here?

> + return err;
> +}
> +
> +
> +static void __init
> +apic_mailbox_reset(struct kvx_apic_mailbox *mb)

Pointless line break. Please get rid of all of them.

> +{
> + unsigned int i;
> + unsigned int mb_end = mb->mb_count;
> + void __iomem *mb_addr;
> + u64 funct_val = (KVX_MAILBOX_MODE_OR << KVX_MAILBOX_FUNCT_MODE_SHIFT) |
> + (KVX_MAILBOX_TRIG_DOORBELL << KVX_MAILBOX_FUNCT_TRIG_SHIFT);
> +
> + for (i = 0; i < mb_end; i++) {
> + mb_addr = kvx_mailbox_get_addr(mb, i);
> + /* Disable all interrupts */
> + writeq(0ULL, mb_addr + KVX_MAILBOX_MASK_OFFSET);
> + /* Set mailbox to OR mode + trigger */
> + writeq(funct_val, mb_addr + KVX_MAILBOX_FUNCT_OFFSET);
> + /* Load & Clear mailbox value */
> + readq(mb_addr + KVX_MAILBOX_LAC_OFFSET);
> + }
> +}
> +
> +static struct msi_domain_ops kvx_msi_domain_ops = {
> +};
> +
> +static struct msi_domain_info kvx_msi_domain_info = {
> + .flags = (MSI_FLAG_USE_DEF_DOM_OPS | MSI_FLAG_USE_DEF_CHIP_OPS),
> + .ops = &kvx_msi_domain_ops,
> + .chip = &kvx_msi_irq_chip,
> +};
> +
> +static int __init
> +kvx_init_apic_mailbox(struct device_node *node,
> + struct device_node *parent)
> +{
> + struct kvx_apic_mailbox *mb;
> + unsigned int parent_irq, irq_count;
> + struct resource res;
> + int ret, i;
> +
> + mb = kzalloc(sizeof(*mb), GFP_KERNEL);
> + if (!mb)
> + return -ENOMEM;
> +
> + ret = of_address_to_resource(node, 0, &res);
> + if (ret)
> + return -EINVAL;
> +
> + mb->phys_base = res.start;
> + mb->base = of_io_request_and_map(node, 0, node->name);
> + if (!mb->base) {
> + ret = -EINVAL;
> + goto err_kfree;
> + }
> +
> + spin_lock_init(&mb->mailboxes_lock);
> + raw_spin_lock_init(&mb->mask_lock);
> +
> + irq_count = of_irq_count(node);
> + if (irq_count == 0 || irq_count > MAILBOXES_MAX_COUNT) {
> + ret = -EINVAL;
> + goto err_kfree;
> + }
> + mb->mb_count = irq_count;
> +
> + apic_mailbox_reset(mb);
> +
> + mb->device_domain = irq_domain_add_tree(node,
> + &kvx_apic_mailbox_domain_ops,
> + mb);
> + if (!mb->device_domain) {
> + pr_err("Failed to setup device domain\n");
> + ret = -EINVAL;
> + goto err_iounmap;
> + }
> +
> + mb->msi_domain = platform_msi_create_irq_domain(of_node_to_fwnode(node),
> + &kvx_msi_domain_info,
> + mb->device_domain);

Function does not exist anymore.

> + if (!mb->msi_domain) {
> + ret = -EINVAL;
> + goto err_irq_domain_add_tree;
> + }

Thanks,

tglx