[5/7] xen/events: drop xen_allocate_irqs_dynamic()
Commit Message
Instead of having a common function for allocating a single IRQ or a
consecutive number of IRQs, split up the functionality into the callers
of xen_allocate_irqs_dynamic().
This allows to handle any allocation error in xen_irq_init() gracefully
instead of panicing the system. Let xen_irq_init() return the irq_info
pointer or NULL in case of an allocation error.
Additionally set the IRQ into irq_info already at allocation time, as
otherwise the IRQ would be '0' (which is a valid IRQ number) until
being set.
Signed-off-by: Juergen Gross <jgross@suse.com>
---
drivers/xen/events/events_base.c | 74 +++++++++++++++++++-------------
1 file changed, 44 insertions(+), 30 deletions(-)
Comments
On 16.10.23 09:28, Juergen Gross wrote:
Hello Juergen
> Instead of having a common function for allocating a single IRQ or a
> consecutive number of IRQs, split up the functionality into the callers
> of xen_allocate_irqs_dynamic().
>
> This allows to handle any allocation error in xen_irq_init() gracefully
> instead of panicing the system. Let xen_irq_init() return the irq_info
> pointer or NULL in case of an allocation error.
>
> Additionally set the IRQ into irq_info already at allocation time, as
> otherwise the IRQ would be '0' (which is a valid IRQ number) until
> being set.
>
> Signed-off-by: Juergen Gross <jgross@suse.com>
> ---
> drivers/xen/events/events_base.c | 74 +++++++++++++++++++-------------
> 1 file changed, 44 insertions(+), 30 deletions(-)
>
[snip]
> @@ -1725,6 +1738,7 @@ void rebind_evtchn_irq(evtchn_port_t evtchn, int irq)
> so there should be a proper type */
> BUG_ON(info->type == IRQT_UNBOUND);
>
> + info->irq = irq;
I failed to understand why this is added here. Doesn't irq remain the
same, and info->irq remains valid? Could you please clarify.
Other changes lgtm.
> (void)xen_irq_info_evtchn_setup(irq, evtchn, NULL);
>
> mutex_unlock(&irq_mapping_update_lock);
On 14.11.23 09:20, Oleksandr Tyshchenko wrote:
>
>
> On 16.10.23 09:28, Juergen Gross wrote:
>
>
> Hello Juergen
>
>> Instead of having a common function for allocating a single IRQ or a
>> consecutive number of IRQs, split up the functionality into the callers
>> of xen_allocate_irqs_dynamic().
>>
>> This allows to handle any allocation error in xen_irq_init() gracefully
>> instead of panicing the system. Let xen_irq_init() return the irq_info
>> pointer or NULL in case of an allocation error.
>>
>> Additionally set the IRQ into irq_info already at allocation time, as
>> otherwise the IRQ would be '0' (which is a valid IRQ number) until
>> being set.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
>> ---
>> drivers/xen/events/events_base.c | 74 +++++++++++++++++++-------------
>> 1 file changed, 44 insertions(+), 30 deletions(-)
>>
>
> [snip]
>
>> @@ -1725,6 +1738,7 @@ void rebind_evtchn_irq(evtchn_port_t evtchn, int irq)
>> so there should be a proper type */
>> BUG_ON(info->type == IRQT_UNBOUND);
>>
>> + info->irq = irq;
>
>
> I failed to understand why this is added here. Doesn't irq remain the
> same, and info->irq remains valid? Could you please clarify.
The IRQ remains the same, but the event channel could change.
This setting of info->irq compensates for the related removal in
xen_irq_info_common_setup().
>
> Other changes lgtm.
Juergen
On 14.11.23 10:35, Juergen Gross wrote:
Hello Juergen
> On 14.11.23 09:20, Oleksandr Tyshchenko wrote:
>>
>>
>> On 16.10.23 09:28, Juergen Gross wrote:
>>
>>
>> Hello Juergen
>>
>>> Instead of having a common function for allocating a single IRQ or a
>>> consecutive number of IRQs, split up the functionality into the callers
>>> of xen_allocate_irqs_dynamic().
>>>
>>> This allows to handle any allocation error in xen_irq_init() gracefully
>>> instead of panicing the system. Let xen_irq_init() return the irq_info
>>> pointer or NULL in case of an allocation error.
>>>
>>> Additionally set the IRQ into irq_info already at allocation time, as
>>> otherwise the IRQ would be '0' (which is a valid IRQ number) until
>>> being set.
>>>
>>> Signed-off-by: Juergen Gross <jgross@suse.com>
>>> ---
>>> drivers/xen/events/events_base.c | 74
>>> +++++++++++++++++++-------------
>>> 1 file changed, 44 insertions(+), 30 deletions(-)
>>>
>>
>> [snip]
>>
>>> @@ -1725,6 +1738,7 @@ void rebind_evtchn_irq(evtchn_port_t evtchn,
>>> int irq)
>>> so there should be a proper type */
>>> BUG_ON(info->type == IRQT_UNBOUND);
>>> + info->irq = irq;
>>
>>
>> I failed to understand why this is added here. Doesn't irq remain the
>> same, and info->irq remains valid? Could you please clarify.
>
> The IRQ remains the same, but the event channel could change.
>
> This setting of info->irq compensates for the related removal in
> xen_irq_info_common_setup().
Thanks for the clarification, you can add my
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>
>
>>
>> Other changes lgtm.
>
>
> Juergen
>
@@ -302,6 +302,13 @@ static void channels_on_cpu_inc(struct irq_info *info)
info->is_accounted = 1;
}
+static void xen_irq_free_desc(unsigned int irq)
+{
+ /* Legacy IRQ descriptors are managed by the arch. */
+ if (irq >= nr_legacy_irqs())
+ irq_free_desc(irq);
+}
+
static void delayed_free_irq(struct work_struct *work)
{
struct irq_info *info = container_of(to_rcu_work(work), struct irq_info,
@@ -313,9 +320,7 @@ static void delayed_free_irq(struct work_struct *work)
kfree(info);
- /* Legacy IRQ descriptors are managed by the arch. */
- if (irq >= nr_legacy_irqs())
- irq_free_desc(irq);
+ xen_irq_free_desc(irq);
}
/* Constructors for packed IRQ information. */
@@ -330,7 +335,6 @@ static int xen_irq_info_common_setup(struct irq_info *info,
BUG_ON(info->type != IRQT_UNBOUND && info->type != type);
info->type = type;
- info->irq = irq;
info->evtchn = evtchn;
info->cpu = cpu;
info->mask_reason = EVT_MASK_REASON_EXPLICIT;
@@ -730,47 +734,45 @@ void xen_irq_lateeoi(unsigned int irq, unsigned int eoi_flags)
}
EXPORT_SYMBOL_GPL(xen_irq_lateeoi);
-static void xen_irq_init(unsigned irq)
+static struct irq_info *xen_irq_init(unsigned int irq)
{
struct irq_info *info;
info = kzalloc(sizeof(*info), GFP_KERNEL);
- if (info == NULL)
- panic("Unable to allocate metadata for IRQ%d\n", irq);
+ if (info) {
+ info->irq = irq;
+ info->type = IRQT_UNBOUND;
+ info->refcnt = -1;
+ INIT_RCU_WORK(&info->rwork, delayed_free_irq);
- info->type = IRQT_UNBOUND;
- info->refcnt = -1;
- INIT_RCU_WORK(&info->rwork, delayed_free_irq);
+ set_info_for_irq(irq, info);
+ /*
+ * Interrupt affinity setting can be immediate. No point
+ * in delaying it until an interrupt is handled.
+ */
+ irq_set_status_flags(irq, IRQ_MOVE_PCNTXT);
- set_info_for_irq(irq, info);
- /*
- * Interrupt affinity setting can be immediate. No point
- * in delaying it until an interrupt is handled.
- */
- irq_set_status_flags(irq, IRQ_MOVE_PCNTXT);
+ INIT_LIST_HEAD(&info->eoi_list);
+ list_add_tail(&info->list, &xen_irq_list_head);
+ }
- INIT_LIST_HEAD(&info->eoi_list);
- list_add_tail(&info->list, &xen_irq_list_head);
+ return info;
}
-static int __must_check xen_allocate_irqs_dynamic(int nvec)
+static inline int __must_check xen_allocate_irq_dynamic(void)
{
- int i, irq = irq_alloc_descs(-1, 0, nvec, -1);
+ int irq = irq_alloc_desc_from(0, -1);
if (irq >= 0) {
- for (i = 0; i < nvec; i++)
- xen_irq_init(irq + i);
+ if (!xen_irq_init(irq)) {
+ xen_irq_free_desc(irq);
+ irq = -1;
+ }
}
return irq;
}
-static inline int __must_check xen_allocate_irq_dynamic(void)
-{
-
- return xen_allocate_irqs_dynamic(1);
-}
-
static int __must_check xen_allocate_irq_gsi(unsigned gsi)
{
int irq;
@@ -790,7 +792,10 @@ static int __must_check xen_allocate_irq_gsi(unsigned gsi)
else
irq = irq_alloc_desc_at(gsi, -1);
- xen_irq_init(irq);
+ if (!xen_irq_init(irq)) {
+ xen_irq_free_desc(irq);
+ irq = -1;
+ }
return irq;
}
@@ -960,6 +965,11 @@ static void __unbind_from_irq(unsigned int irq)
evtchn_port_t evtchn = evtchn_from_irq(irq);
struct irq_info *info = info_for_irq(irq);
+ if (!info) {
+ xen_irq_free_desc(irq);
+ return;
+ }
+
if (info->refcnt > 0) {
info->refcnt--;
if (info->refcnt != 0)
@@ -1097,11 +1107,14 @@ int xen_bind_pirq_msi_to_irq(struct pci_dev *dev, struct msi_desc *msidesc,
mutex_lock(&irq_mapping_update_lock);
- irq = xen_allocate_irqs_dynamic(nvec);
+ irq = irq_alloc_descs(-1, 0, nvec, -1);
if (irq < 0)
goto out;
for (i = 0; i < nvec; i++) {
+ if (!xen_irq_init(irq + i))
+ goto error_irq;
+
irq_set_chip_and_handler_name(irq + i, &xen_pirq_chip, handle_edge_irq, name);
ret = xen_irq_info_pirq_setup(irq + i, 0, pirq + i, 0, domid,
@@ -1725,6 +1738,7 @@ void rebind_evtchn_irq(evtchn_port_t evtchn, int irq)
so there should be a proper type */
BUG_ON(info->type == IRQT_UNBOUND);
+ info->irq = irq;
(void)xen_irq_info_evtchn_setup(irq, evtchn, NULL);
mutex_unlock(&irq_mapping_update_lock);