From mboxrd@z Thu Jan 1 00:00:00 1970 From: Hans de Goede Subject: Re: [PATCH v2 10/24] uas: zap_pending: data urbs should have completed at this time Date: Sun, 14 Sep 2014 11:26:33 +0200 Message-ID: <54155F49.8060904@redhat.com> References: <1410604011-3828-1-git-send-email-hdegoede@redhat.com> <1410604011-3828-11-git-send-email-hdegoede@redhat.com> <54149BA6.3010305@cogentembedded.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Return-path: Received: from mx1.redhat.com ([209.132.183.28]:18080 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752516AbaINJ0m (ORCPT ); Sun, 14 Sep 2014 05:26:42 -0400 In-Reply-To: <54149BA6.3010305@cogentembedded.com> Sender: linux-scsi-owner@vger.kernel.org List-Id: linux-scsi@vger.kernel.org To: Sergei Shtylyov , Greg Kroah-Hartman Cc: linux-usb@vger.kernel.org, linux-scsi@vger.kernel.org Hi, On 09/13/2014 09:31 PM, Sergei Shtylyov wrote: > Hello. > > On 9/13/2014 1:26 PM, Hans de Goede wrote: > >> The data urbs are all killed before calling zap_pending, and their completion >> handler should have cleared their inflight flag. > >> Do not 0 the data inflight flags, and add a check for try_complete succeeding, >> as it should always succeed when called from zap_pending. > >> Signed-off-by: Hans de Goede >> --- >> drivers/usb/storage/uas.c | 10 +++++----- >> 1 file changed, 5 insertions(+), 5 deletions(-) >> >> diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c >> index 08edb6b..85bbc1d 100644 >> --- a/drivers/usb/storage/uas.c >> +++ b/drivers/usb/storage/uas.c >> @@ -145,6 +145,7 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result) >> struct uas_cmd_info *cmdinfo; >> struct uas_cmd_info *temp; >> unsigned long flags; >> + int err; > > Er, I don't see why this variable is necessary. > > [...] >> @@ -152,12 +153,11 @@ static void uas_zap_pending(struct uas_dev_info *devinfo, int result) >> struct scsi_cmnd *cmnd = container_of(scp, struct scsi_cmnd, >> SCp); >> uas_log_cmd_state(cmnd, __func__); >> - /* all urbs are killed, clear inflight bits */ >> - cmdinfo->state &= ~(COMMAND_INFLIGHT | >> - DATA_IN_URB_INFLIGHT | >> - DATA_OUT_URB_INFLIGHT); >> + /* Sense urbs were killed, clear COMMAND_INFLIGHT manually */ >> + cmdinfo->state &= ~COMMAND_INFLIGHT; >> cmnd->result = result << 16; >> - uas_try_complete(cmnd, __func__); >> + err = uas_try_complete(cmnd, __func__); >> + WARN_ON(err != 0); > > Why not: > > WARN_ON(uas_try_complete(cmnd, __func__)); > This was discussed already during v1 of this patch-set, WARN_ON may not have a side-effect, as it may be defined as an empty macro. Regards, Hans