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

[Grygorii Strashko <grygorii.strashko@ti.com>]

On 09/14/2016 05:14 PM, Richard Cochran wrote:
> On Wed, Sep 14, 2016 at 04:02:28PM +0300, Grygorii Strashko wrote:
>> From: WingMan Kwok <w-kwok2@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-kwok2@ti.com>
>> Signed-off-by: Grygorii Strashko <grygorii.strashko@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
>>
>

NP here - will update. Thanks for you review.


-- 
regards,
-grygorii