From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753117AbdJ3LcG (ORCPT ); Mon, 30 Oct 2017 07:32:06 -0400 Received: from mail-lf0-f67.google.com ([209.85.215.67]:54857 "EHLO mail-lf0-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752536AbdJ3LcE (ORCPT ); Mon, 30 Oct 2017 07:32:04 -0400 X-Google-Smtp-Source: ABhQp+Q9HmQSoRHlOu7GbKbr0ay2Iho/cCfZeckqq3Ne73zDI0upiZVUTC75UPDeLC2vsa7ftdbGFQ== Date: Mon, 30 Oct 2017 12:32:02 +0100 From: Johan Hovold To: "Bryan O'Donoghue" Cc: Kees Cook , Greg Kroah-Hartman , linux-kernel@vger.kernel.org, Johan Hovold , Alex Elder , greybus-dev@lists.linaro.org, devel@driverdev.osuosl.org Subject: Re: [PATCH v2] staging: greybus: Convert timers to use timer_setup() Message-ID: <20171030113202.GB7223@localhost> References: <20171024144938.GA41224@beast> <55a6647a-8565-09e8-9f45-d9048c7dd916@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <55a6647a-8565-09e8-9f45-d9048c7dd916@linaro.org> User-Agent: Mutt/1.7.2 (2016-11-26) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Oct 24, 2017 at 04:54:59PM +0100, Bryan O'Donoghue wrote: > On 24/10/17 15:49, Kees Cook wrote: > > In preparation for unconditionally passing the struct timer_list pointer to > > all timer callbacks, switch to using the new timer_setup() and from_timer() > > to pass the timer pointer explicitly. > > > > Cc: Greg Kroah-Hartman > > Cc: "Bryan O'Donoghue" > > Cc: Johan Hovold > > Cc: Alex Elder > > Cc: greybus-dev@lists.linaro.org > > Cc: devel@driverdev.osuosl.org > > Signed-off-by: Kees Cook > > --- > > v2: Added back "get" in timer code, thanks to Bryan. :) > > --- > > drivers/staging/greybus/loopback.c | 19 +++++++++---------- > > drivers/staging/greybus/operation.c | 7 +++---- > > 2 files changed, 12 insertions(+), 14 deletions(-) > > > > diff --git a/drivers/staging/greybus/loopback.c b/drivers/staging/greybus/loopback.c > > index 08e255884206..1c0bafeb7ea5 100644 > > --- a/drivers/staging/greybus/loopback.c > > +++ b/drivers/staging/greybus/loopback.c > > @@ -572,16 +572,16 @@ static void gb_loopback_async_operation_work(struct work_struct *work) > > gb_loopback_async_operation_put(op_async); > > } > > > > -static void gb_loopback_async_operation_timeout(unsigned long data) > > +static void gb_loopback_async_operation_timeout(struct timer_list *t) > > { > > - struct gb_loopback_async_operation *op_async; > > - u16 id = data; > > + struct gb_loopback_async_operation *op_async = > > + from_timer(op_async, t, timer); > > + unsigned long flags; > > + > > + spin_lock_irqsave(&gb_dev.lock, flags); > > + gb_loopback_async_operation_get(op_async); > > + spin_unlock_irqrestore(&gb_dev.lock, flags); > > Damnit I'm just wrong (I hate that). > > The pointer can already have gone away by the time the timer runs - my > bad... Hmm. Then something is really broken in this driver, you obviously must never free the async operation which contains the timer while the timer is active. > see attached for update - with my Signed-off added. The right thing to do here is to respin your patch from last year which converts the loopback driver to use the timeout handling in greybus core. Otherwise, I'm afraid you're not addressing the underlying bug. Either way, Kees, please submit the operation.c conversion separately from the loopback one, as the latter is non-trivial. Thanks, Johan