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

[Grygorii Strashko]

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@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

NP here - will update. Thanks for you review.


--
regards,
-grygorii