Re: [Minios-devel] [UNIKRAFT PATCH 7/7] lib/ukconsdev: Enable interrupts on console device

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

So far it looks okay. What happens with your receive (read) function 
when interrupts are used? When reading from the ring/queue are the 
interrupts temporarily disabled until you read everything. Is this the 
case when the receive function returns 0? Otherwise interrupts are 
internally switched off?
You probably should add an explanation to your read function.

Thanks,

Simon

On 19.06.19 15:35, Birlea Costin wrote:
> Signed-off-by: Birlea Costin <costin.birlea@xxxxxxxxx>
> ---
>   lib/ukconsdev/consdev.c                 |  2 ++
>   lib/ukconsdev/exportsyms.uk             |  2 ++
>   lib/ukconsdev/include/uk/consdev.h      | 44 +++++++++++++++++++++++++++++++++
>   lib/ukconsdev/include/uk/consdev_core.h |  8 ++++++
>   4 files changed, 56 insertions(+)
>
> diff --git a/lib/ukconsdev/consdev.c b/lib/ukconsdev/consdev.c
> index b91b0a7f..fd01540b 100644
> --- a/lib/ukconsdev/consdev.c
> +++ b/lib/ukconsdev/consdev.c
> @@ -445,6 +445,8 @@ int uk_consdev_drv_register(struct uk_consdev *dev, struct 
> uk_alloc *a,
>         UK_ASSERT(dev->ops->rx_configure);
>         UK_ASSERT(dev->ops->tx_configure);
>         UK_ASSERT(dev->ops->release);
> +       UK_ASSERT((dev->ops->rx_intr_enable && dev->ops->rx_intr_disable)
> +               || (!dev->ops->rx_intr_enable && !dev->ops->rx_intr_disable));
>         UK_ASSERT(dev->ops->write);
>         UK_ASSERT(dev->ops->read);
>         UK_ASSERT(dev->ops->start);
> diff --git a/lib/ukconsdev/exportsyms.uk b/lib/ukconsdev/exportsyms.uk
> index 4d39941d..4b68d711 100644
> --- a/lib/ukconsdev/exportsyms.uk
> +++ b/lib/ukconsdev/exportsyms.uk
> @@ -7,6 +7,8 @@ uk_consdev_tx_configure
>   uk_consdev_release
>   uk_consdev_start
>   uk_consdev_stop
> +uk_consdev_rx_intr_enable
> +uk_consdev_rx_intr_disable
>   uk_consdev_write
>   uk_consdev_read
>   uk_consdev_count
> diff --git a/lib/ukconsdev/include/uk/consdev.h 
> b/lib/ukconsdev/include/uk/consdev.h
> index 9f2f0567..844c2aca 100644
> --- a/lib/ukconsdev/include/uk/consdev.h
> +++ b/lib/ukconsdev/include/uk/consdev.h
> @@ -219,6 +219,50 @@ int uk_consdev_start(struct uk_consdev *dev);
>   int uk_consdev_stop(struct uk_consdev *dev);
>   
>   /**
> + * Enable interrupts for rx.
> + *
> + * @param dev
> + *     The Unikraft Console Device in running state.
> + * @return
> + *     - (0): Success, interrupts enabled.
> + *     - (-ENOTSUP): Driver does not support interrupts.
> + */
> +static inline int uk_consdev_rx_intr_enable(struct uk_consdev *dev)
> +{
> +       UK_ASSERT(dev);
> +       UK_ASSERT(dev->ops);
> +       UK_ASSERT(dev->_data);
> +       UK_ASSERT(dev->_data->state == UK_CONSDEV_RUNNING);
> +
> +       if (unlikely(!dev->ops->rx_intr_enable))
> +               return -ENOTSUP;
> +
> +       return dev->ops->rx_intr_enable(dev);
> +}
> +
> +/**
> + * Disable interrupts for rx.
> + *
> + * @param dev
> + *     The Unikraft Console Device in running state.
> + * @return
> + *     - (0): Success, interrupts disabled.
> + *     - (-ENOTSUP): Driver does not support interrupts.
> + */
> +static inline int uk_consdev_rx_intr_disable(struct uk_consdev *dev)
> +{
> +       UK_ASSERT(dev);
> +       UK_ASSERT(dev->_data);
> +       UK_ASSERT(dev->ops);
> +       UK_ASSERT(dev->_data->state == UK_CONSDEV_RUNNING);
> +
> +       if (unlikely(!dev->ops->rx_intr_disable))
> +               return -ENOTSUP;
> +
> +       return dev->ops->rx_intr_disable(dev);
> +}
> +
> +/**
>    * @param dev
>    *  The Unikraft Console Device in running state.
>    * @param buf
> diff --git a/lib/ukconsdev/include/uk/consdev_core.h 
> b/lib/ukconsdev/include/uk/consdev_core.h
> index 6c675fa7..d30655ef 100644
> --- a/lib/ukconsdev/include/uk/consdev_core.h
> +++ b/lib/ukconsdev/include/uk/consdev_core.h
> @@ -170,6 +170,12 @@ typedef int (*uk_consdev_tx_configure_t)(struct uk_consdev 
> *dev,
>   /** Driver callback type to release a configured Unikraft console device */
>   typedef int (*uk_consdev_release_t)(struct uk_consdev *dev);
>   
> +/** Driver callback type to enable interrupts of an Unikraft console device. */
> +typedef int (*uk_consdev_rx_intr_enable_t)(struct uk_consdev *dev);
> +
> +/** Driver callback type to disable interrupts of an Unikraft console device. 
> */
> +typedef int (*uk_consdev_rx_intr_disable_t)(struct uk_consdev *dev);
> +
>   /** Driver callback type to start a configured Unikraft console device. */
>   typedef int (*uk_consdev_start_t)(struct uk_consdev *dev);
>   
> @@ -197,6 +203,8 @@ struct uk_consdev_ops {
>         uk_consdev_tx_configure_t           tx_configure;
>         uk_consdev_start_t                  start;
>         uk_consdev_stop_t                   stop;
> +       uk_consdev_rx_intr_enable_t         rx_intr_enable;
> +       uk_consdev_rx_intr_disable_t        rx_intr_disable;
>         uk_consdev_write_t                  write;
>         uk_consdev_read_t                   read;
>         uk_consdev_close_t                  close;
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel