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-kwok2@xxxxxx>
> 
> 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-kwok2@xxxxxx>
> Signed-off-by: Grygorii Strashko <grygorii.strashko@xxxxxx>
> ---
>  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