Re: [PATCH 6/9] net: ethernet: ti: cpts: clean up event list if event pool is empty

[Richard Cochran]

On Wed, Sep 14, 2016 at 04:02:28PM +0300, Grygorii Strashko wrote:
> From: WingMan Kwok <w-kw...@ti.com>
> 
> When a CPTS user does not exit gracefully by disabling cpts
> timestamping and leaving a joined multicast group, the system
> continues to receive and timestamps the ptp packets which eventually
> occupy all the event list entries.  When this happns, the added code
> tries to remove some list entries which are expired.
> 
> Signed-off-by: WingMan Kwok <w-kw...@ti.com>
> Signed-off-by: Grygorii Strashko <grygorii.stras...@ti.com>
> ---
>  drivers/net/ethernet/ti/cpts.c | 30 ++++++++++++++++++++++++++++--
>  1 file changed, 28 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/ti/cpts.c b/drivers/net/ethernet/ti/cpts.c
> index 970d4e2..ff8bb85 100644
> --- a/drivers/net/ethernet/ti/cpts.c
> +++ b/drivers/net/ethernet/ti/cpts.c
> @@ -57,22 +57,48 @@ static int cpts_fifo_pop(struct cpts *cpts, u32 *high, 
> u32 *low)
>       return -1;
>  }
>  
> +static int cpts_event_list_clean_up(struct cpts *cpts)

5 words, that is quite a mouth full.  How about this instead?

        static int cpts_purge_events(struct cpts *cpts);

> +{
> +     struct list_head *this, *next;
> +     struct cpts_event *event;
> +     int removed = 0;
> +
> +     list_for_each_safe(this, next, &cpts->events) {
> +             event = list_entry(this, struct cpts_event, list);
> +             if (event_expired(event)) {
> +                     list_del_init(&event->list);
> +                     list_add(&event->list, &cpts->pool);
> +                     ++removed;
> +             }
> +     }
> +     return removed;
> +}
> +
>  /*
>   * Returns zero if matching event type was found.
>   */
>  static int cpts_fifo_read(struct cpts *cpts, int match)
>  {
>       int i, type = -1;
> +     int removed;

No need for another variable, just change the return code above to

        return removed ? 0 : -1;

and then you have ...

>       u32 hi, lo;
>       struct cpts_event *event;
>  
>       for (i = 0; i < CPTS_FIFO_DEPTH; i++) {
>               if (cpts_fifo_pop(cpts, &hi, &lo))
>                       break;
> +
>               if (list_empty(&cpts->pool)) {
> -                     pr_err("cpts: event pool is empty\n");
> -                     return -1;
> +                     removed = cpts_event_list_clean_up(cpts);
> +                     if (!removed) {
> +                             dev_err(cpts->dev,
> +                                     "cpts: event pool is empty\n");
> +                             return -1;
> +                     }

                        if (cpts_purge_events(cpts)) {
                                dev_err(cpts->dev, "cpts: event pool empty\n");
                                return -1;
                        }

Notice how I avoided the ugly line break?

> +                     dev_dbg(cpts->dev,
> +                             "cpts: event pool cleaned up %d\n", removed);
>               }
> +
>               event = list_first_entry(&cpts->pool, struct cpts_event, list);
>               event->tmo = jiffies + 2;
>               event->high = hi;
> -- 
> 2.9.3
> 

Thanks,
Richard