[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <d190793c-fe6b-263e-7793-ccd73f9ccad4@oracle.com>
Date: Sat, 7 Mar 2020 14:41:44 -0500
From: Boris Ostrovsky <boris.ostrovsky@...cle.com>
To: Yan Yankovskyi <yyankovskyi@...il.com>,
Jan Beulich <jbeulich@...e.com>
Cc: Juergen Gross <jgross@...e.com>,
Stefano Stabellini <sstabellini@...nel.org>,
xen-devel@...ts.xenproject.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] xen: Use evtchn_type_t as a type for event channels
On 3/7/20 8:43 AM, Yan Yankovskyi wrote:
> Make event channel functions pass event channel port using
> evtchn_port_t type. It eliminates signed <-> unsigned conversion.
>
> static int find_virq(unsigned int virq, unsigned int cpu)
> {
> struct evtchn_status status;
> - int port, rc = -ENOENT;
> + evtchn_port_t port;
> + int rc = -ENOENT;
>
> memset(&status, 0, sizeof(status));
> for (port = 0; port < xen_evtchn_max_channels(); port++) {
> @@ -962,7 +963,8 @@ EXPORT_SYMBOL_GPL(xen_evtchn_nr_channels);
> int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
> {
> struct evtchn_bind_virq bind_virq;
> - int evtchn, irq, ret;
> + evtchn_port_t evtchn = xen_evtchn_max_channels();
> + int irq, ret;
>
> mutex_lock(&irq_mapping_update_lock);
>
> @@ -990,7 +992,6 @@ int bind_virq_to_irq(unsigned int virq, unsigned int cpu, bool percpu)
> if (ret == -EEXIST)
> ret = find_virq(virq, cpu);
> BUG_ON(ret < 0);
> - evtchn = ret;
This looks suspicious. What would you be passing to
xen_irq_info_virq_setup() below?
I also think that, given that this patch is trying to get types in
order, find_virq() will need more changes: it is supposed to return
evtchn_port_t. But then it also wants to return a (signed) error.
> }
>
> ret = xen_irq_info_virq_setup(cpu, irq, evtchn, virq);
> @@ -1019,7 +1020,7 @@ static void unbind_from_irq(unsigned int irq)
> mutex_unlock(&irq_mapping_update_lock);
> }
>
> --- a/drivers/xen/xenbus/xenbus_client.c
> +++ b/drivers/xen/xenbus/xenbus_client.c
> @@ -391,7 +391,7 @@ EXPORT_SYMBOL_GPL(xenbus_grant_ring);
> * error, the device will switch to XenbusStateClosing, and the error will be
> * saved in the store.
> */
> -int xenbus_alloc_evtchn(struct xenbus_device *dev, int *port)
> +int xenbus_alloc_evtchn(struct xenbus_device *dev, evtchn_port_t *port)
Right. But then why is the declaration in include/xen/xenbus.h (at the
very end of the patch) different?
> {
> struct evtchn_alloc_unbound alloc_unbound;
> int err;
> @@ -414,7 +414,7 @@ EXPORT_SYMBOL_GPL(xenbus_alloc_evtchn);
> /**
> * Free an existing event channel. Returns 0 on success or -errno on error.
> */
> -int xenbus_free_evtchn(struct xenbus_device *dev, int port)
> +int xenbus_free_evtchn(struct xenbus_device *dev, evtchn_port_t port)
Here too.
> {
> struct evtchn_close close;
> int err;
> @@ -423,7 +423,7 @@ int xenbus_free_evtchn(struct xenbus_device *dev, int port)
And why not here, especially since you updated format?
>
> err = HYPERVISOR_event_channel_op(EVTCHNOP_close, &close);
> if (err)
> - xenbus_dev_error(dev, err, "freeing event channel %d", port);
> + xenbus_dev_error(dev, err, "freeing event channel %u", port);
>
> return err;
> }
>
> diff --git a/include/xen/interface/event_channel.h b/include/xen/interface/event_channel.h
> index 45650c9a06d5..cf80e338fbb0 100644
> --- a/include/xen/interface/event_channel.h
> +++ b/include/xen/interface/event_channel.h
> @@ -220,7 +220,7 @@ struct evtchn_expand_array {
> #define EVTCHNOP_set_priority 13
> struct evtchn_set_priority {
> /* IN parameters. */
> - uint32_t port;
> + evtchn_port_t port;
This definition comes from Xen so I think it needs to be fixed there first.
> uint32_t priority;
> };
>
> diff --git a/include/xen/xenbus.h b/include/xen/xenbus.h
> index 89a889585ba0..4f35216064ba 100644
> --- a/include/xen/xenbus.h
> +++ b/include/xen/xenbus.h
> @@ -218,8 +218,8 @@ int xenbus_unmap_ring(struct xenbus_device *dev,
> grant_handle_t *handles, unsigned int nr_handles,
> unsigned long *vaddrs);
>
> -int xenbus_alloc_evtchn(struct xenbus_device *dev, int *port);
> -int xenbus_free_evtchn(struct xenbus_device *dev, int port);
> +int xenbus_alloc_evtchn(struct xenbus_device *dev, unsigned int *port);
> +int xenbus_free_evtchn(struct xenbus_device *dev, unsigned int port);
These.
-boris
Powered by blists - more mailing lists