Re: [Minios-devel] [UNIKRAFT PATCH 11/11] plat/common/pci: use new list api

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Yuri,

Please find the comment inline.

Thanks & Regards
Sharan

On 12/5/18 6:23 PM, Yuri Volchkov wrote:
> Signed-off-by: Yuri Volchkov <yuri.volchkov@xxxxxxxxx>
> ---
>   plat/common/include/pci/pci_bus.h |  6 ++----
>   plat/common/pci_bus.c             | 28 +++++++++-------------------
>   2 files changed, 11 insertions(+), 23 deletions(-)
>
> diff --git a/plat/common/include/pci/pci_bus.h 
> b/plat/common/include/pci/pci_bus.h
> index 4e02282..1362032 100644
> --- a/plat/common/include/pci/pci_bus.h
> +++ b/plat/common/include/pci/pci_bus.h
> @@ -130,13 +130,12 @@ typedef int (*pci_driver_add_func_t)(struct pci_device *);
>   typedef int (*pci_driver_init_func_t)(struct uk_alloc *a);
>   
>   struct pci_driver {
> -       UK_TAILQ_ENTRY(struct pci_driver) next;
> +       struct uk_list_head list;
>         /**< ANY-ID terminated list of device IDs that the driver handles */
>         const struct pci_device_id *device_ids;
>         pci_driver_init_func_t init; /* optional */
>         pci_driver_add_func_t add_dev;
>   };
> -UK_TAILQ_HEAD(pci_driver_list, struct pci_driver);
>   
>   enum pci_device_state {
>         PCI_DEVICE_STATE_RESET = 0,
> @@ -144,7 +143,7 @@ enum pci_device_state {
>   };
>   
>   struct pci_device {
> -       UK_TAILQ_ENTRY(struct pci_device) next; /**< used by pci_bus_handler */
> +       struct uk_list_head list;
>         struct pci_device_id  id;
>         struct pci_address    addr;
>         struct pci_driver     *drv;
> @@ -153,7 +152,6 @@ struct pci_device {
>         uint16_t base;
>         unsigned long irq;
>   };
> -UK_TAILQ_HEAD(pci_device_list, struct pci_device);
>   
>   
>   #define PCI_REGISTER_DRIVER(b)                  \
> diff --git a/plat/common/pci_bus.c b/plat/common/pci_bus.c
> index 71713c9..2816378 100644
> --- a/plat/common/pci_bus.c
> +++ b/plat/common/pci_bus.c
> @@ -60,9 +60,8 @@
>   struct pci_bus_handler {
>         struct uk_bus b;
>         struct uk_alloc *a;
> -       struct pci_driver_list drv_list;  /**< List of PCI drivers */
> -       int drv_list_initialized;
> -       struct pci_device_list dev_list;  /**< List of PCI devices */
> +       struct uk_list_head drv_list;  /**< List of PCI drivers */
> +       struct uk_list_head dev_list;  /**< List of PCI devices */
>   };
>   static struct pci_bus_handler ph;
>   
> @@ -165,7 +164,7 @@ static inline struct pci_driver *pci_find_driver(struct pci_device_id *id)
>         struct pci_driver *drv;
>         const struct pci_device_id *drv_id;
>   
> -	UK_TAILQ_FOREACH(drv, &ph.drv_list, next) {
> +       uk_list_for_each_entry(drv, &ph.drv_list, list) {
>                 for (drv_id = drv->device_ids;
>                      !pci_device_id_is_any(drv_id);
>                      drv_id++) {
> @@ -292,20 +291,14 @@ static int pci_init(struct uk_alloc *a)
>   
>   	ph.a = a;
>   
> -	if (!ph.drv_list_initialized) {
> -               UK_TAILQ_INIT(&ph.drv_list);
> -               ph.drv_list_initialized = 1;
> -       }
> -       UK_TAILQ_INIT(&ph.dev_list);
> -
> -       UK_TAILQ_FOREACH_SAFE(drv, &ph.drv_list, next, drv_next) {
> +       uk_list_for_each_entry_safe(drv, drv_next, &ph.drv_list, list) {
>                 if (drv->init) {
>                         ret = drv->init(a);
>                         if (ret == 0)
>                                 continue;
>                         uk_pr_err("Failed to initialize driver %p: %d\n",
>                                   drv, ret);
> -                       UK_TAILQ_REMOVE(&ph.drv_list, drv, next);
> +                       uk_list_del(&drv->list);
uk_list_del has some dangling reference to the list. But isn't it better 
to use uk_list_del_init instead?
>                 }
>         }
>         return 0;
> @@ -314,12 +307,7 @@ static int pci_init(struct uk_alloc *a)
>   void _pci_register_driver(struct pci_driver *drv)
>   {
>         UK_ASSERT(drv != NULL);
> -
> -       if (!ph.drv_list_initialized) {
> -               UK_TAILQ_INIT(&ph.drv_list);
> -               ph.drv_list_initialized = 1;
> -       }
> -       UK_TAILQ_INSERT_TAIL(&ph.drv_list, drv, next);
> +       uk_list_add_tail(&drv->list, &ph.drv_list);
>   }
>   
>   
> @@ -327,6 +315,8 @@ void _pci_register_driver(struct pci_driver *drv)
>    */
>   static struct pci_bus_handler ph = {
>         .b.init = pci_init,
> -       .b.probe = pci_probe
> +       .b.probe = pci_probe,
> +       .drv_list = UK_LIST_HEAD_INIT(ph.drv_list),
> +       .dev_list = UK_LIST_HEAD_INIT(ph.dev_list),
>   };
>   UK_BUS_REGISTER(&ph.b);
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel