diff mbox series

[4/7] xen/events: remove some simple helpers from events_base.c

Message ID 20231016062831.20630-5-jgross@suse.com
State New
Headers
Series xen/events: do some cleanups in events_base.c |

Commit Message

Juergen Gross Oct. 16, 2023, 6:28 a.m. UTC 
  The helper functions type_from_irq() and cpu_from_irq() are just one
line functions used only internally.

Open code them where needed. At the same time modify and rename
get_evtchn_to_irq() to return a struct irq_info instead of the IRQ
number.

Signed-off-by: Juergen Gross <jgross@suse.com>
---
 drivers/xen/events/events_base.c | 97 +++++++++++++-------------------
 1 file changed, 38 insertions(+), 59 deletions(-)

Comments

Oleksandr Tyshchenko Nov. 13, 2023, 5:35 p.m. UTC | #1 
On 16.10.23 09:28, Juergen Gross wrote:


Hello Juergen.


> The helper functions type_from_irq() and cpu_from_irq() are just one
> line functions used only internally.
> 
> Open code them where needed. At the same time modify and rename
> get_evtchn_to_irq() to return a struct irq_info instead of the IRQ
> number.
> 
> Signed-off-by: Juergen Gross <jgross@suse.com>



[snip]



> 
> @@ -1181,15 +1172,16 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
>   {
>   	int irq;
>   	int ret;
> +	struct irq_info *info;
>   
>   	if (evtchn >= xen_evtchn_max_channels())
>   		return -ENOMEM;


I assume this check is called here (*before* holding a lock) by 
intention, as evtchn_to_info() below contains the same check.

>   
>   	mutex_lock(&irq_mapping_update_lock);
>   
> -	irq = get_evtchn_to_irq(evtchn);
> +	info = evtchn_to_info(evtchn) >
> -	if (irq == -1) {
> +	if (!info) {
>   		irq = xen_allocate_irq_dynamic();
>   		if (irq < 0)
>   			goto out;
> @@ -1212,8 +1204,8 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
>   		 */
>   		bind_evtchn_to_cpu(evtchn, 0, false);
>   	} else {
> -		struct irq_info *info = info_for_irq(irq);
> -		WARN_ON(info == NULL || info->type != IRQT_EVTCHN);
> +		WARN_ON(info->type != IRQT_EVTCHN);
> +		irq = info->irq;
>   	}


This hunk doesn't apply clearly to the latest state, because of 
"9e90e58c11b7 xen: evtchn: Allow shared registration of IRQ handers" 
went in. Please rebase.


With that:
Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>


Also checkpatch.pl warns about BUG_ON usage in several places, but again 
you didn't introduce them in current patch, just touched their args.


[snip]
Juergen Gross Nov. 14, 2023, 8:28 a.m. UTC | #2 
On 13.11.23 18:35, Oleksandr Tyshchenko wrote:
> 
> 
> On 16.10.23 09:28, Juergen Gross wrote:
> 
> 
> Hello Juergen.
> 
> 
>> The helper functions type_from_irq() and cpu_from_irq() are just one
>> line functions used only internally.
>>
>> Open code them where needed. At the same time modify and rename
>> get_evtchn_to_irq() to return a struct irq_info instead of the IRQ
>> number.
>>
>> Signed-off-by: Juergen Gross <jgross@suse.com>
> 
> 
> 
> [snip]
> 
> 
> 
>>
>> @@ -1181,15 +1172,16 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
>>    {
>>    	int irq;
>>    	int ret;
>> +	struct irq_info *info;
>>    
>>    	if (evtchn >= xen_evtchn_max_channels())
>>    		return -ENOMEM;
> 
> 
> I assume this check is called here (*before* holding a lock) by
> intention, as evtchn_to_info() below contains the same check.

Yes.

> 
>>    
>>    	mutex_lock(&irq_mapping_update_lock);
>>    
>> -	irq = get_evtchn_to_irq(evtchn);
>> +	info = evtchn_to_info(evtchn) >
>> -	if (irq == -1) {
>> +	if (!info) {
>>    		irq = xen_allocate_irq_dynamic();
>>    		if (irq < 0)
>>    			goto out;
>> @@ -1212,8 +1204,8 @@ static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
>>    		 */
>>    		bind_evtchn_to_cpu(evtchn, 0, false);
>>    	} else {
>> -		struct irq_info *info = info_for_irq(irq);
>> -		WARN_ON(info == NULL || info->type != IRQT_EVTCHN);
>> +		WARN_ON(info->type != IRQT_EVTCHN);
>> +		irq = info->irq;
>>    	}
> 
> 
> This hunk doesn't apply clearly to the latest state, because of
> "9e90e58c11b7 xen: evtchn: Allow shared registration of IRQ handers"
> went in. Please rebase.
> 
> 
> With that:
> Reviewed-by: Oleksandr Tyshchenko <oleksandr_tyshchenko@epam.com>

Thanks,


Juergen
diff mbox series

Patch

diff --git a/drivers/xen/events/events_base.c b/drivers/xen/events/events_base.c
index 97d71c5e7c28..4ada3b6a4164 100644
--- a/drivers/xen/events/events_base.c
+++ b/drivers/xen/events/events_base.c
@@ -246,15 +246,6 @@  static int set_evtchn_to_irq(evtchn_port_t evtchn, unsigned int irq)
 	return 0;
 }
 
-static int get_evtchn_to_irq(evtchn_port_t evtchn)
-{
-	if (evtchn >= xen_evtchn_max_channels())
-		return -1;
-	if (evtchn_to_irq[EVTCHN_ROW(evtchn)] == NULL)
-		return -1;
-	return READ_ONCE(evtchn_to_irq[EVTCHN_ROW(evtchn)][EVTCHN_COL(evtchn)]);
-}
-
 /* Get info for IRQ */
 static struct irq_info *info_for_irq(unsigned irq)
 {
@@ -272,6 +263,19 @@  static void set_info_for_irq(unsigned int irq, struct irq_info *info)
 		irq_set_chip_data(irq, info);
 }
 
+static struct irq_info *evtchn_to_info(evtchn_port_t evtchn)
+{
+	int irq;
+
+	if (evtchn >= xen_evtchn_max_channels())
+		return NULL;
+	if (evtchn_to_irq[EVTCHN_ROW(evtchn)] == NULL)
+		return NULL;
+	irq = READ_ONCE(evtchn_to_irq[EVTCHN_ROW(evtchn)][EVTCHN_COL(evtchn)]);
+
+	return (irq < 0) ? NULL : info_for_irq(irq);
+}
+
 /* Per CPU channel accounting */
 static void channels_on_cpu_dec(struct irq_info *info)
 {
@@ -426,7 +430,9 @@  static evtchn_port_t evtchn_from_irq(unsigned int irq)
 
 unsigned int irq_from_evtchn(evtchn_port_t evtchn)
 {
-	return get_evtchn_to_irq(evtchn);
+	struct irq_info *info = evtchn_to_info(evtchn);
+
+	return info ? info->irq : -1;
 }
 EXPORT_SYMBOL_GPL(irq_from_evtchn);
 
@@ -470,25 +476,11 @@  static unsigned pirq_from_irq(unsigned irq)
 	return info->u.pirq.pirq;
 }
 
-static enum xen_irq_type type_from_irq(unsigned irq)
-{
-	return info_for_irq(irq)->type;
-}
-
-static unsigned cpu_from_irq(unsigned irq)
-{
-	return info_for_irq(irq)->cpu;
-}
-
 unsigned int cpu_from_evtchn(evtchn_port_t evtchn)
 {
-	int irq = get_evtchn_to_irq(evtchn);
-	unsigned ret = 0;
-
-	if (irq != -1)
-		ret = cpu_from_irq(irq);
+	struct irq_info *info = evtchn_to_info(evtchn);
 
-	return ret;
+	return info ? info->cpu : 0;
 }
 
 static void do_mask(struct irq_info *info, u8 reason)
@@ -537,13 +529,12 @@  static bool pirq_needs_eoi_flag(unsigned irq)
 static void bind_evtchn_to_cpu(evtchn_port_t evtchn, unsigned int cpu,
 			       bool force_affinity)
 {
-	int irq = get_evtchn_to_irq(evtchn);
-	struct irq_info *info = info_for_irq(irq);
+	struct irq_info *info = evtchn_to_info(evtchn);
 
-	BUG_ON(irq == -1);
+	BUG_ON(info == NULL);
 
 	if (IS_ENABLED(CONFIG_SMP) && force_affinity) {
-		struct irq_data *data = irq_get_irq_data(irq);
+		struct irq_data *data = irq_get_irq_data(info->irq);
 
 		irq_data_update_affinity(data, cpumask_of(cpu));
 		irq_data_update_effective_affinity(data, cpumask_of(cpu));
@@ -976,13 +967,13 @@  static void __unbind_from_irq(unsigned int irq)
 	}
 
 	if (VALID_EVTCHN(evtchn)) {
-		unsigned int cpu = cpu_from_irq(irq);
+		unsigned int cpu = info->cpu;
 		struct xenbus_device *dev;
 
 		if (!info->is_static)
 			xen_evtchn_close(evtchn);
 
-		switch (type_from_irq(irq)) {
+		switch (info->type) {
 		case IRQT_VIRQ:
 			per_cpu(virq_to_irq, cpu)[virq_from_irq(irq)] = -1;
 			break;
@@ -1181,15 +1172,16 @@  static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
 {
 	int irq;
 	int ret;
+	struct irq_info *info;
 
 	if (evtchn >= xen_evtchn_max_channels())
 		return -ENOMEM;
 
 	mutex_lock(&irq_mapping_update_lock);
 
-	irq = get_evtchn_to_irq(evtchn);
+	info = evtchn_to_info(evtchn);
 
-	if (irq == -1) {
+	if (!info) {
 		irq = xen_allocate_irq_dynamic();
 		if (irq < 0)
 			goto out;
@@ -1212,8 +1204,8 @@  static int bind_evtchn_to_irq_chip(evtchn_port_t evtchn, struct irq_chip *chip,
 		 */
 		bind_evtchn_to_cpu(evtchn, 0, false);
 	} else {
-		struct irq_info *info = info_for_irq(irq);
-		WARN_ON(info == NULL || info->type != IRQT_EVTCHN);
+		WARN_ON(info->type != IRQT_EVTCHN);
+		irq = info->irq;
 	}
 
 out:
@@ -1551,13 +1543,7 @@  EXPORT_SYMBOL_GPL(xen_set_irq_priority);
 
 int evtchn_make_refcounted(evtchn_port_t evtchn, bool is_static)
 {
-	int irq = get_evtchn_to_irq(evtchn);
-	struct irq_info *info;
-
-	if (irq == -1)
-		return -ENOENT;
-
-	info = info_for_irq(irq);
+	struct irq_info *info = evtchn_to_info(evtchn);
 
 	if (!info)
 		return -ENOENT;
@@ -1573,7 +1559,6 @@  EXPORT_SYMBOL_GPL(evtchn_make_refcounted);
 
 int evtchn_get(evtchn_port_t evtchn)
 {
-	int irq;
 	struct irq_info *info;
 	int err = -ENOENT;
 
@@ -1582,11 +1567,7 @@  int evtchn_get(evtchn_port_t evtchn)
 
 	mutex_lock(&irq_mapping_update_lock);
 
-	irq = get_evtchn_to_irq(evtchn);
-	if (irq == -1)
-		goto done;
-
-	info = info_for_irq(irq);
+	info = evtchn_to_info(evtchn);
 
 	if (!info)
 		goto done;
@@ -1606,10 +1587,11 @@  EXPORT_SYMBOL_GPL(evtchn_get);
 
 void evtchn_put(evtchn_port_t evtchn)
 {
-	int irq = get_evtchn_to_irq(evtchn);
-	if (WARN_ON(irq == -1))
+	struct irq_info *info = evtchn_to_info(evtchn);
+
+	if (WARN_ON(!info))
 		return;
-	unbind_from_irq(irq);
+	unbind_from_irq(info->irq);
 }
 EXPORT_SYMBOL_GPL(evtchn_put);
 
@@ -1639,12 +1621,10 @@  struct evtchn_loop_ctrl {
 
 void handle_irq_for_port(evtchn_port_t port, struct evtchn_loop_ctrl *ctrl)
 {
-	int irq;
-	struct irq_info *info;
+	struct irq_info *info = evtchn_to_info(port);
 	struct xenbus_device *dev;
 
-	irq = get_evtchn_to_irq(port);
-	if (irq == -1)
+	if (!info)
 		return;
 
 	/*
@@ -1669,7 +1649,6 @@  void handle_irq_for_port(evtchn_port_t port, struct evtchn_loop_ctrl *ctrl)
 		}
 	}
 
-	info = info_for_irq(irq);
 	if (xchg_acquire(&info->is_active, 1))
 		return;
 
@@ -1683,7 +1662,7 @@  void handle_irq_for_port(evtchn_port_t port, struct evtchn_loop_ctrl *ctrl)
 		info->eoi_time = get_jiffies_64() + event_eoi_delay;
 	}
 
-	generic_handle_irq(irq);
+	generic_handle_irq(info->irq);
 }
 
 int xen_evtchn_do_upcall(void)
@@ -1741,7 +1720,7 @@  void rebind_evtchn_irq(evtchn_port_t evtchn, int irq)
 	mutex_lock(&irq_mapping_update_lock);
 
 	/* After resume the irq<->evtchn mappings are all cleared out */
-	BUG_ON(get_evtchn_to_irq(evtchn) != -1);
+	BUG_ON(evtchn_to_info(evtchn));
 	/* Expect irq to have been bound before,
 	   so there should be a proper type */
 	BUG_ON(info->type == IRQT_UNBOUND);